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

[Jiri Olsa <olsajiri@gmail.com>]

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 <bigeasy@linutronix.de>
> ---
>  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
>