[bpf,3/3] xsk: Try to make xdp_umem_reg extension a bit more future-proof

Commit Message

Stanislav Fomichev July 13, 2024, 1:52 a.m. UTC

Add a couple of things:
1. Remove xdp_umem_reg_v2 since its sizeof is the same as xdp_umem_reg
2. Add BUILD_BUG_ON that checks that the size of xdp_umem_reg_v1 is less
   than xdp_umem_reg; presumably, when we get to v2, there is gonna
   be a similar line to enforce that sizeof(v2) > sizeof(v1)
3. Add BUILD_BUG_ON to make sure the last field plus its size matches
   the overall struct size. The intent is to demonstrate that we don't
   have any lingering padding.

Reported-by: Julian Schindel <[e-mail]>
Cc: Magnus Karlsson <[e-mail]>
Signed-off-by: Stanislav Fomichev <[e-mail]>
---
 net/xdp/xsk.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Maciej Fijalkowski July 24, 2024, 3:21 p.m. UTC | #1

On Fri, Jul 12, 2024 at 06:52:53PM -0700, Stanislav Fomichev wrote:
> Add a couple of things:
> 1. Remove xdp_umem_reg_v2 since its sizeof is the same as xdp_umem_reg

So thing here is that adding __attribute__((packed)) on kernel side
wouldn't help because we wouldn't fix old uapi with this, correct? old
uapi would still yield 32 bytes for xdp_umem_reg without tx_metadata_len.

Just explaining here to myself.

> 2. Add BUILD_BUG_ON that checks that the size of xdp_umem_reg_v1 is less
>    than xdp_umem_reg; presumably, when we get to v2, there is gonna
>    be a similar line to enforce that sizeof(v2) > sizeof(v1)
> 3. Add BUILD_BUG_ON to make sure the last field plus its size matches
>    the overall struct size. The intent is to demonstrate that we don't
>    have any lingering padding.

This is good stuff but I wonder wouldn't it be more feasible to squash
this with 1/3 ? And have it backported. Regarding the patch logistics, you
did not provide fixes tag here for some reason, but still include the
patch routed via bpf tree.

> 
> Reported-by: Julian Schindel <[e-mail]>
> Cc: Magnus Karlsson <[e-mail]>
> Signed-off-by: Stanislav Fomichev <[e-mail]>
> ---
>  net/xdp/xsk.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 7d1c0986f9bb..1d951d7e3797 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1331,14 +1331,6 @@ struct xdp_umem_reg_v1 {
>  	__u32 headroom;
>  };
>  
> -struct xdp_umem_reg_v2 {
> -	__u64 addr; /* Start of packet data area */
> -	__u64 len; /* Length of packet data area */
> -	__u32 chunk_size;
> -	__u32 headroom;
> -	__u32 flags;
> -};
> -
>  static int xsk_setsockopt(struct socket *sock, int level, int optname,
>  			  sockptr_t optval, unsigned int optlen)
>  {
> @@ -1382,10 +1374,19 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
>  
>  		if (optlen < sizeof(struct xdp_umem_reg_v1))
>  			return -EINVAL;
> -		else if (optlen < sizeof(struct xdp_umem_reg_v2))
> -			mr_size = sizeof(struct xdp_umem_reg_v1);
>  		else if (optlen < sizeof(mr))
> -			mr_size = sizeof(struct xdp_umem_reg_v2);
> +			mr_size = sizeof(struct xdp_umem_reg_v1);
> +
> +		BUILD_BUG_ON(sizeof(struct xdp_umem_reg_v1) >= sizeof(struct xdp_umem_reg));
> +
> +		/* Make sure the last field of the struct doesn't have
> +		 * uninitialized padding. All padding has to be explicit
> +		 * and has to be set to zero by the userspace to make
> +		 * struct xdp_umem_reg extensible in the future.
> +		 */
> +		BUILD_BUG_ON(offsetof(struct xdp_umem_reg, tx_metadata_len) +
> +			     sizeof_field(struct xdp_umem_reg, tx_metadata_len) !=
> +			     sizeof(struct xdp_umem_reg));
>  
>  		if (copy_from_sockptr(&mr, optval, mr_size))
>  			return -EFAULT;
> -- 
> 2.45.2
> 
>

Stanislav Fomichev July 25, 2024, 6:01 p.m. UTC | #2

On 07/24, Maciej Fijalkowski wrote:
> On Fri, Jul 12, 2024 at 06:52:53PM -0700, Stanislav Fomichev wrote:
> > Add a couple of things:
> > 1. Remove xdp_umem_reg_v2 since its sizeof is the same as xdp_umem_reg
> 
> So thing here is that adding __attribute__((packed)) on kernel side
> wouldn't help because we wouldn't fix old uapi with this, correct? old
> uapi would still yield 32 bytes for xdp_umem_reg without tx_metadata_len.
> 
> Just explaining here to myself.

Yea :-(

> > 2. Add BUILD_BUG_ON that checks that the size of xdp_umem_reg_v1 is less
> >    than xdp_umem_reg; presumably, when we get to v2, there is gonna
> >    be a similar line to enforce that sizeof(v2) > sizeof(v1)
> > 3. Add BUILD_BUG_ON to make sure the last field plus its size matches
> >    the overall struct size. The intent is to demonstrate that we don't
> >    have any lingering padding.
> 
> This is good stuff but I wonder wouldn't it be more feasible to squash
> this with 1/3 ? And have it backported. Regarding the patch logistics, you
> did not provide fixes tag here for some reason, but still include the
> patch routed via bpf tree.

SG, will resend this against bpf-next.

Patch

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 7d1c0986f9bb..1d951d7e3797 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1331,14 +1331,6 @@  struct xdp_umem_reg_v1 {
 	__u32 headroom;
 };
 
-struct xdp_umem_reg_v2 {
-	__u64 addr; /* Start of packet data area */
-	__u64 len; /* Length of packet data area */
-	__u32 chunk_size;
-	__u32 headroom;
-	__u32 flags;
-};
-
 static int xsk_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {
@@ -1382,10 +1374,19 @@  static int xsk_setsockopt(struct socket *sock, int level, int optname,
 
 		if (optlen < sizeof(struct xdp_umem_reg_v1))
 			return -EINVAL;
-		else if (optlen < sizeof(struct xdp_umem_reg_v2))
-			mr_size = sizeof(struct xdp_umem_reg_v1);
 		else if (optlen < sizeof(mr))
-			mr_size = sizeof(struct xdp_umem_reg_v2);
+			mr_size = sizeof(struct xdp_umem_reg_v1);
+
+		BUILD_BUG_ON(sizeof(struct xdp_umem_reg_v1) >= sizeof(struct xdp_umem_reg));
+
+		/* Make sure the last field of the struct doesn't have
+		 * uninitialized padding. All padding has to be explicit
+		 * and has to be set to zero by the userspace to make
+		 * struct xdp_umem_reg extensible in the future.
+		 */
+		BUILD_BUG_ON(offsetof(struct xdp_umem_reg, tx_metadata_len) +
+			     sizeof_field(struct xdp_umem_reg, tx_metadata_len) !=
+			     sizeof(struct xdp_umem_reg));
 
 		if (copy_from_sockptr(&mr, optval, mr_size))
 			return -EFAULT;