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

Commit Message

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

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 <[e-mail]>
Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]
Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
---
 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(-)

Comments

Alexei Starovoitov July 3, 2024, 9:39 p.m. UTC | #1

On Tue, Jul 2, 2024 at 7:25 AM Sebastian Andrzej Siewior
<[e-mail]> 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 <[e-mail]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]
> Signed-off-by: Sebastian Andrzej Siewior <[e-mail]>
> ---
>  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

Sebastian Andrzej Siewior July 4, 2024, 8 a.m. UTC | #2

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

Alexei Starovoitov July 7, 2024, 12:42 a.m. UTC | #3

On Thu, Jul 4, 2024 at 1:00 AM Sebastian Andrzej Siewior
<[e-mail]> 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

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 */
 	};
 };