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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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