[v2,bpf-next,3/3] bpf: Implement bpf_check_basics_ok() as a macro.

Commit Message

Sebastian Andrzej Siewior July 2, 2024, 2:21 p.m. UTC

sparse complains about the argument type for filter that is passed to
bpf_check_basics_ok(). There are two users of the function where the
variable is with __user attribute one without. The pointer is only
checked against NULL so there is no access to the content and so no need
for any user-wrapper.

Adding the __user to the declaration doesn't solve anything because
there is one kernel user so it will be wrong again.
Splitting the function in two seems an overkill because the function is
small and simple.

Make a macro based on the function which does not trigger a sparse
warning. The change to a macro and "unsigned int" -> "u16" for `flen'
alters gcc's code generation a bit.

Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
---
 net/core/filter.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Jiri Olsa July 2, 2024, 4:27 p.m. UTC | #1

On Tue, Jul 02, 2024 at 04:21:43PM +0200, Sebastian Andrzej Siewior wrote:
> sparse complains about the argument type for filter that is passed to
> bpf_check_basics_ok(). There are two users of the function where the
> variable is with __user attribute one without. The pointer is only
> checked against NULL so there is no access to the content and so no need
> for any user-wrapper.
> 
> Adding the __user to the declaration doesn't solve anything because
> there is one kernel user so it will be wrong again.
> Splitting the function in two seems an overkill because the function is
> small and simple.

could we just retype the __user argument? like

  bpf_check_basics_ok((const struct sock_filter *) fprog->filter, ...)


> 
> Make a macro based on the function which does not trigger a sparse
> warning. The change to a macro and "unsigned int" -> "u16" for `flen'
> alters gcc's code generation a bit.
> 
> Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
> ---
>  net/core/filter.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3f14c8019f26d..5747533ed5491 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1035,16 +1035,20 @@ static bool chk_code_allowed(u16 code_to_probe)
>  	return codes[code_to_probe];
>  }
>  
> -static bool bpf_check_basics_ok(const struct sock_filter *filter,
> -				unsigned int flen)
> -{
> -	if (filter == NULL)
> -		return false;
> -	if (flen == 0 || flen > BPF_MAXINSNS)
> -		return false;
> -
> -	return true;
> -}
> + /* macro instead of a function to avoid woring about _filter which might be a
> +  * user or kernel pointer. It does not matter for the NULL check.
> +  */
> +#define bpf_check_basics_ok(fprog_filter, fprog_flen)	\
> +({							\
> +	bool __ret = true;				\
> +	u16 __flen = fprog_flen;			\

why not use fprog_flen directly? I'm not sure I get the changelog
explanation 

thanks,
jirka


> +							\
> +	if (!(fprog_filter))				\
> +		__ret = false;				\
> +	else if (__flen == 0 || __flen > BPF_MAXINSNS)	\
> +		__ret = false;				\
> +	__ret;						\
> +})
>  
>  /**
>   *	bpf_check_classic - verify socket filter code
> -- 
> 2.45.2
>

Sebastian Andrzej Siewior July 2, 2024, 6:12 p.m. UTC | #2

On 2024-07-02 18:27:31 [+0200], Jiri Olsa wrote:
> On Tue, Jul 02, 2024 at 04:21:43PM +0200, Sebastian Andrzej Siewior wrote:
> > sparse complains about the argument type for filter that is passed to
> > bpf_check_basics_ok(). There are two users of the function where the
> > variable is with __user attribute one without. The pointer is only
> > checked against NULL so there is no access to the content and so no need
> > for any user-wrapper.
> > 
> > Adding the __user to the declaration doesn't solve anything because
> > there is one kernel user so it will be wrong again.
> > Splitting the function in two seems an overkill because the function is
> > small and simple.
> 
> could we just retype the __user argument? like
> 
>   bpf_check_basics_ok((const struct sock_filter *) fprog->filter, ...)

If we keep the function and add a cast here then cast the __user part
away and it would be wrong if we do something else with the pointer.
If it is understood that that will never happen…

> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -1035,16 +1035,20 @@ static bool chk_code_allowed(u16 code_to_probe)
…
> > + /* macro instead of a function to avoid woring about _filter which might be a
> > +  * user or kernel pointer. It does not matter for the NULL check.
> > +  */
> > +#define bpf_check_basics_ok(fprog_filter, fprog_flen)	\
> > +({							\
> > +	bool __ret = true;				\
> > +	u16 __flen = fprog_flen;			\
> 
> why not use fprog_flen directly? I'm not sure I get the changelog
> explanation 

This was to avoid expanding `fprog_flen' twice. But looking at the
actual output, the code generation seems to be unaffected.

> thanks,
> jirka

Sebastian

Andrii Nakryiko July 2, 2024, 10:26 p.m. UTC | #3

On Tue, Jul 2, 2024 at 7:25 AM Sebastian Andrzej Siewior
<[e-mail]> wrote:
>
> sparse complains about the argument type for filter that is passed to
> bpf_check_basics_ok(). There are two users of the function where the
> variable is with __user attribute one without. The pointer is only
> checked against NULL so there is no access to the content and so no need
> for any user-wrapper.
>
> Adding the __user to the declaration doesn't solve anything because
> there is one kernel user so it will be wrong again.
> Splitting the function in two seems an overkill because the function is
> small and simple.
>
> Make a macro based on the function which does not trigger a sparse
> warning. The change to a macro and "unsigned int" -> "u16" for `flen'
> alters gcc's code generation a bit.
>
> Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
> ---
>  net/core/filter.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3f14c8019f26d..5747533ed5491 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1035,16 +1035,20 @@ static bool chk_code_allowed(u16 code_to_probe)
>         return codes[code_to_probe];
>  }
>
> -static bool bpf_check_basics_ok(const struct sock_filter *filter,
> -                               unsigned int flen)
> -{
> -       if (filter == NULL)
> -               return false;
> -       if (flen == 0 || flen > BPF_MAXINSNS)
> -               return false;
> -
> -       return true;
> -}

Why not open-code part of it and then have a function for checking length limit:

if (!filter)
    return <error>;
if (!bpf_check_prog_len(flen))
    return <another-error>;

It's all local to a single file, no big deal adding a few if
(!pointer) checks explicitly, IMO. "basics" is super generic and not a
great name either way.

> + /* macro instead of a function to avoid woring about _filter which might be a
> +  * user or kernel pointer. It does not matter for the NULL check.
> +  */
> +#define bpf_check_basics_ok(fprog_filter, fprog_flen)  \
> +({                                                     \
> +       bool __ret = true;                              \
> +       u16 __flen = fprog_flen;                        \
> +                                                       \
> +       if (!(fprog_filter))                            \
> +               __ret = false;                          \
> +       else if (__flen == 0 || __flen > BPF_MAXINSNS)  \
> +               __ret = false;                          \
> +       __ret;                                          \
> +})
>
>  /**
>   *     bpf_check_classic - verify socket filter code
> --
> 2.45.2
>

Sebastian Andrzej Siewior July 3, 2024, 12:39 p.m. UTC | #4

On 2024-07-02 15:26:38 [-0700], Andrii Nakryiko wrote:
> 
> Why not open-code part of it and then have a function for checking length limit:
> 
> if (!filter)
>     return <error>;
> if (!bpf_check_prog_len(flen))
>     return <another-error>;
> 
> It's all local to a single file, no big deal adding a few if
> (!pointer) checks explicitly, IMO. "basics" is super generic and not a
> great name either way.

Okay.

Sebastian

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index 3f14c8019f26d..5747533ed5491 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1035,16 +1035,20 @@  static bool chk_code_allowed(u16 code_to_probe)
 	return codes[code_to_probe];
 }
 
-static bool bpf_check_basics_ok(const struct sock_filter *filter,
-				unsigned int flen)
-{
-	if (filter == NULL)
-		return false;
-	if (flen == 0 || flen > BPF_MAXINSNS)
-		return false;
-
-	return true;
-}
+ /* macro instead of a function to avoid woring about _filter which might be a
+  * user or kernel pointer. It does not matter for the NULL check.
+  */
+#define bpf_check_basics_ok(fprog_filter, fprog_flen)	\
+({							\
+	bool __ret = true;				\
+	u16 __flen = fprog_flen;			\
+							\
+	if (!(fprog_filter))				\
+		__ret = false;				\
+	else if (__flen == 0 || __flen > BPF_MAXINSNS)	\
+		__ret = false;				\
+	__ret;						\
+})
 
 /**
  *	bpf_check_classic - verify socket filter code