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 <mail@xxxxxxxxxxxxxxxx>
> Cc: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxxx>
> ---
> 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
>
>
