[PATCH v2 bpf-next 0/3] bpf: sparse cleanup.

[PATCH v2 bpf-next 0/3] bpf: sparse cleanup.

[Sebastian Andrzej Siewior]

Hi,

after my last bpf series was merged I received a robot mail because I
added yet another sparse related warning.
I tried to address most of them, a few are left in the category "missing
prototype" but the whole block is marked as "ignore-warnings-for-gcc" so
left it as is.

v1…v2 https://lore.kernel.org/all/20240628084857.1719108-1-bigeasy@linutronix.de
  - Accidently posted the wrong version while editing 3/3, leading to an
    compile error. This has been corrected.

Sebastian

[PATCH v2 bpf-next 1/3] bpf: Add casts to keep sparse quiet.

[Sebastian Andrzej Siewior]

sparse complains about wrong data types within the BPF callbacks.
Functions like bpf_l3_csum_replace() are invoked with a specific set of
arguments and its further usage is based on a flag. So it can not be set
right upfront.
There is also access to variables in struct bpf_nh_params and struct
bpf_xfrm_state which should be __be32. The content comes directly
from the BPF program so conversion is already right.

Add __force casts for the right data type and update the members in
struct bpf_xfrm_state and bpf_nh_params in order to keep sparse happy.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202406261217.A4hdCnYa-lkp@intel.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/filter.h         |  2 +-
 include/uapi/linux/bpf.h       |  6 +++---
 net/core/filter.c              | 18 ++++++++++--------
 tools/include/uapi/linux/bpf.h |  6 +++---
 4 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 02ddcfdf94c46..15aee0143f1cf 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -728,7 +728,7 @@ struct bpf_skb_data_end {
 struct bpf_nh_params {
 	u32 nh_family;
 	union {
-		u32 ipv4_nh;
+		__be32 ipv4_nh;
 		struct in6_addr ipv6_nh;
 	};
 };
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 25ea393cf084b..f45b03706e4e9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6290,12 +6290,12 @@ struct bpf_tunnel_key {
  */
 struct bpf_xfrm_state {
 	__u32 reqid;
-	__u32 spi;	/* Stored in network byte order */
+	__be32 spi;	/* Stored in network byte order */
 	__u16 family;
 	__u16 ext;	/* Padding, future use. */
 	union {
-		__u32 remote_ipv4;	/* Stored in network byte order */
-		__u32 remote_ipv6[4];	/* Stored in network byte order */
+		__be32 remote_ipv4;	/* Stored in network byte order */
+		__be32 remote_ipv6[4];	/* Stored in network byte order */
 	};
 };
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 403d23faf22e1..3f14c8019f26d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1939,13 +1939,13 @@ BPF_CALL_5(bpf_l3_csum_replace, struct sk_buff *, skb, u32, offset,
 		if (unlikely(from != 0))
 			return -EINVAL;
 
-		csum_replace_by_diff(ptr, to);
+		csum_replace_by_diff(ptr, (__force __wsum)to);
 		break;
 	case 2:
-		csum_replace2(ptr, from, to);
+		csum_replace2(ptr, (__force __be16)from, (__force __be16)to);
 		break;
 	case 4:
-		csum_replace4(ptr, from, to);
+		csum_replace4(ptr, (__force __be32)from, (__force __be32)to);
 		break;
 	default:
 		return -EINVAL;
@@ -1990,13 +1990,15 @@ BPF_CALL_5(bpf_l4_csum_replace, struct sk_buff *, skb, u32, offset,
 		if (unlikely(from != 0))
 			return -EINVAL;
 
-		inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo);
+		inet_proto_csum_replace_by_diff(ptr, skb, (__force __wsum)to, is_pseudo);
 		break;
 	case 2:
-		inet_proto_csum_replace2(ptr, skb, from, to, is_pseudo);
+		inet_proto_csum_replace2(ptr, skb, (__force __be16)from, (__force __be16)to,
+					 is_pseudo);
 		break;
 	case 4:
-		inet_proto_csum_replace4(ptr, skb, from, to, is_pseudo);
+		inet_proto_csum_replace4(ptr, skb, (__force __be32)from, (__force __be32)to,
+					 is_pseudo);
 		break;
 	default:
 		return -EINVAL;
@@ -2046,7 +2048,7 @@ BPF_CALL_5(bpf_csum_diff, __be32 *, from, u32, from_size,
 
 	ret = csum_partial(sp->diff, diff_size, seed);
 	local_unlock_nested_bh(&bpf_sp.bh_lock);
-	return ret;
+	return (__force __u32)ret;
 }
 
 static const struct bpf_func_proto bpf_csum_diff_proto = {
@@ -2068,7 +2070,7 @@ BPF_CALL_2(bpf_csum_update, struct sk_buff *, skb, __wsum, csum)
 	 * as emulating csum_sub() can be done from the eBPF program.
 	 */
 	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		return (skb->csum = csum_add(skb->csum, csum));
+		return (__force __u32)(skb->csum = csum_add(skb->csum, csum));
 
 	return -ENOTSUPP;
 }
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 25ea393cf084b..f45b03706e4e9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6290,12 +6290,12 @@ struct bpf_tunnel_key {
  */
 struct bpf_xfrm_state {
 	__u32 reqid;
-	__u32 spi;	/* Stored in network byte order */
+	__be32 spi;	/* Stored in network byte order */
 	__u16 family;
 	__u16 ext;	/* Padding, future use. */
 	union {
-		__u32 remote_ipv4;	/* Stored in network byte order */
-		__u32 remote_ipv6[4];	/* Stored in network byte order */
+		__be32 remote_ipv4;	/* Stored in network byte order */
+		__be32 remote_ipv6[4];	/* Stored in network byte order */
 	};
 };
 
-- 
2.45.2

Re: [PATCH v2 bpf-next 1/3] bpf: Add casts to keep sparse quiet.

[Alexei Starovoitov]

On Tue, Jul 2, 2024 at 7:25 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> sparse complains about wrong data types within the BPF callbacks.
> Functions like bpf_l3_csum_replace() are invoked with a specific set of
> arguments and its further usage is based on a flag. So it can not be set
> right upfront.
> There is also access to variables in struct bpf_nh_params and struct
> bpf_xfrm_state which should be __be32. The content comes directly
> from the BPF program so conversion is already right.
>
> Add __force casts for the right data type and update the members in
> struct bpf_xfrm_state and bpf_nh_params in order to keep sparse happy.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202406261217.A4hdCnYa-lkp@intel.com
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/filter.h         |  2 +-
>  include/uapi/linux/bpf.h       |  6 +++---
>  net/core/filter.c              | 18 ++++++++++--------
>  tools/include/uapi/linux/bpf.h |  6 +++---
>  4 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 02ddcfdf94c46..15aee0143f1cf 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -728,7 +728,7 @@ struct bpf_skb_data_end {
>  struct bpf_nh_params {
>         u32 nh_family;
>         union {
> -               u32 ipv4_nh;
> +               __be32 ipv4_nh;
>                 struct in6_addr ipv6_nh;
>         };
>  };
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 25ea393cf084b..f45b03706e4e9 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -6290,12 +6290,12 @@ struct bpf_tunnel_key {
>   */
>  struct bpf_xfrm_state {
>         __u32 reqid;
> -       __u32 spi;      /* Stored in network byte order */
> +       __be32 spi;     /* Stored in network byte order */
>         __u16 family;
>         __u16 ext;      /* Padding, future use. */
>         union {
> -               __u32 remote_ipv4;      /* Stored in network byte order */
> -               __u32 remote_ipv6[4];   /* Stored in network byte order */
> +               __be32 remote_ipv4;     /* Stored in network byte order */
> +               __be32 remote_ipv6[4];  /* Stored in network byte order */
>         };
>  };

I don't think we should be changing uapi because of sparse.
I would ignore the warnings.
pw-bot: cr

Re: [PATCH v2 bpf-next 1/3] bpf: Add casts to keep sparse quiet.

[Sebastian Andrzej Siewior]

On 2024-07-03 14:39:16 [-0700], Alexei Starovoitov wrote:
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 25ea393cf084b..f45b03706e4e9 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6290,12 +6290,12 @@ struct bpf_tunnel_key {
> >   */
> >  struct bpf_xfrm_state {
> >         __u32 reqid;
> > -       __u32 spi;      /* Stored in network byte order */
> > +       __be32 spi;     /* Stored in network byte order */
> >         __u16 family;
> >         __u16 ext;      /* Padding, future use. */
> >         union {
> > -               __u32 remote_ipv4;      /* Stored in network byte order */
> > -               __u32 remote_ipv6[4];   /* Stored in network byte order */
> > +               __be32 remote_ipv4;     /* Stored in network byte order */
> > +               __be32 remote_ipv6[4];  /* Stored in network byte order */
> >         };
> >  };
> 
> I don't think we should be changing uapi because of sparse.
> I would ignore the warnings.

There are other struct member within this bpf.h which use __be32 so it
is known to userland (in terms of the compiler won't complain about an
unknown type due to missing include). The type is essentially the same
since the __bitwise attribute is empty except for sparse (which defines
__CHECKER_).
Therefore I wouldn't say this changes the uapi in an incompatible way.

Sebastian

Re: [PATCH v2 bpf-next 1/3] bpf: Add casts to keep sparse quiet.

[Alexei Starovoitov]

On Thu, Jul 4, 2024 at 1:00 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2024-07-03 14:39:16 [-0700], Alexei Starovoitov wrote:
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 25ea393cf084b..f45b03706e4e9 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -6290,12 +6290,12 @@ struct bpf_tunnel_key {
> > >   */
> > >  struct bpf_xfrm_state {
> > >         __u32 reqid;
> > > -       __u32 spi;      /* Stored in network byte order */
> > > +       __be32 spi;     /* Stored in network byte order */
> > >         __u16 family;
> > >         __u16 ext;      /* Padding, future use. */
> > >         union {
> > > -               __u32 remote_ipv4;      /* Stored in network byte order */
> > > -               __u32 remote_ipv6[4];   /* Stored in network byte order */
> > > +               __be32 remote_ipv4;     /* Stored in network byte order */
> > > +               __be32 remote_ipv6[4];  /* Stored in network byte order */
> > >         };
> > >  };
> >
> > I don't think we should be changing uapi because of sparse.
> > I would ignore the warnings.
>
> There are other struct member within this bpf.h which use __be32 so it
> is known to userland (in terms of the compiler won't complain about an
> unknown type due to missing include). The type is essentially the same
> since the __bitwise attribute is empty except for sparse (which defines
> __CHECKER_).
> Therefore I wouldn't say this changes the uapi in an incompatible way.

There are two remote_ipv[46] fields in uapi/bpf.h added in 2016 and 2018
with __u32. We're not going to change one them to __be32 now,
because you noticed a sparse warn on the _kernel_ side.

In general, sparse warn is never a reason to change uapi.
If you want to shut up sparse, adjust the kernel side only,
or better yet ignore it.

[PATCH v2 bpf-next 2/3] bpf: Move a few bpf_func_proto declarations.

[Sebastian Andrzej Siewior]

sparse complains about missing declarations and a few of them are in
another .c file. One has no other declaration because it is used localy
and marked weak because it might be defined in another .c file.

Move the declarations from bpf_trace.c to a common place and add one for
bpf_sk_storage_get_cg_sock_proto.

After this change there are only a few missing declartions within the
__bpf_kfunc_start_defs() block which explictlty disables this kind of
warning for the compiler. I am not aware of something similar for sparse
so I guess are stuck with them unless we add them.

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/bpf.h      | 5 +++++
 kernel/trace/bpf_trace.c | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f5c6bc9093a6b..d411bf52910cc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -840,6 +840,11 @@ struct bpf_func_proto {
 	bool (*allowed)(const struct bpf_prog *prog);
 };
 
+extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
+extern const struct bpf_func_proto bpf_skb_output_proto;
+extern const struct bpf_func_proto bpf_xdp_output_proto;
+extern const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto;
+
 /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
  * the first argument to eBPF programs.
  * For socket filters: 'struct bpf_context *' == 'struct sk_buff *'
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d1daeab1bbc14..d8d7ee6b06a6f 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1951,10 +1951,6 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
 	.arg5_type	= ARG_CONST_SIZE_OR_ZERO,
 };
 
-extern const struct bpf_func_proto bpf_skb_output_proto;
-extern const struct bpf_func_proto bpf_xdp_output_proto;
-extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
-
 BPF_CALL_3(bpf_get_stackid_raw_tp, struct bpf_raw_tracepoint_args *, args,
 	   struct bpf_map *, map, u64, flags)
 {
-- 
2.45.2

Re: [PATCH v2 bpf-next 2/3] bpf: Move a few bpf_func_proto declarations.

[Andrii Nakryiko]

On Tue, Jul 2, 2024 at 7:25 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> sparse complains about missing declarations and a few of them are in
> another .c file. One has no other declaration because it is used localy
> and marked weak because it might be defined in another .c file.
>
> Move the declarations from bpf_trace.c to a common place and add one for
> bpf_sk_storage_get_cg_sock_proto.
>
> After this change there are only a few missing declartions within the
> __bpf_kfunc_start_defs() block which explictlty disables this kind of
> warning for the compiler. I am not aware of something similar for sparse
> so I guess are stuck with them unless we add them.
>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/bpf.h      | 5 +++++
>  kernel/trace/bpf_trace.c | 4 ----
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index f5c6bc9093a6b..d411bf52910cc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -840,6 +840,11 @@ struct bpf_func_proto {
>         bool (*allowed)(const struct bpf_prog *prog);
>  };
>
> +extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
> +extern const struct bpf_func_proto bpf_skb_output_proto;
> +extern const struct bpf_func_proto bpf_xdp_output_proto;
> +extern const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto;
> +

There is a whole block of extern const declarations for bpf_*_proto
functions, let's keep them together?

pw-bot: cr


>  /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
>   * the first argument to eBPF programs.
>   * For socket filters: 'struct bpf_context *' == 'struct sk_buff *'
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d1daeab1bbc14..d8d7ee6b06a6f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1951,10 +1951,6 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
>         .arg5_type      = ARG_CONST_SIZE_OR_ZERO,
>  };
>
> -extern const struct bpf_func_proto bpf_skb_output_proto;
> -extern const struct bpf_func_proto bpf_xdp_output_proto;
> -extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
> -
>  BPF_CALL_3(bpf_get_stackid_raw_tp, struct bpf_raw_tracepoint_args *, args,
>            struct bpf_map *, map, u64, flags)
>  {
> --
> 2.45.2
>

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

[Sebastian Andrzej Siewior]

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 <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;			\
+							\
+	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

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

[Jiri Olsa]

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
> 

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

[Sebastian Andrzej Siewior]

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

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

[Andrii Nakryiko]

On Tue, Jul 2, 2024 at 7:25 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> 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 <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;
> -}

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
>

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

[Sebastian Andrzej Siewior]

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