linux-kernel - Re: [PATCH net-next] xdp: add xdp_skb_reserve

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68126b09c77f7_3080df29453@willemb.c.googlers.com.notmuch>
Date: Wed, 30 Apr 2025 14:25:13 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jon Kohler <jon@...anix.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Jason Wang <jasowang@...hat.com>, 
 Andrew Lunn <andrew+netdev@...n.ch>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Alexei Starovoitov <ast@...nel.org>, 
 Daniel Borkmann <daniel@...earbox.net>, 
 Jesper Dangaard Brouer <hawk@...nel.org>, 
 John Fastabend <john.fastabend@...il.com>, 
 Simon Horman <horms@...nel.org>, 
 netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 bpf@...r.kernel.org
Cc: Jon Kohler <jon@...anix.com>
Subject: Re: [PATCH net-next] xdp: add xdp_skb_reserve_put helper

Jon Kohler wrote:
> Add helper for calling skb_{put|reserve} to reduce repetitive pattern
> across various drivers.
> 
> Plumb into tap and tun to start.
> 
> No functional change intended.
> 
> Signed-off-by: Jon Kohler <jon@...anix.com>
> ---
>  drivers/net/tap.c | 3 +--
>  drivers/net/tun.c | 3 +--
>  include/net/xdp.h | 8 ++++++++
>  net/core/xdp.c    | 3 +--
>  4 files changed, 11 insertions(+), 6 deletions(-)

Subjective, but I prefer the existing code. I understand what
skb_reserve and skb_put do. While xdp_skb_reserve_put adds a layer of
indirection that I'd have to follow.

Sometimes deduplication makes sense, sometimes the indirection adds
more mental load than it's worth. In this case the code savings are
small. As said, subjective. Happy to hear other opinions.

> 
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d4ece538f1b2..54ce492da5e9 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -1062,8 +1062,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
>  		goto err;
>  	}
>  
> -	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> -	skb_put(skb, xdp->data_end - xdp->data);
> +	xdp_skb_reserve_put(xdp, skb);
>  
>  	skb_set_network_header(skb, ETH_HLEN);
>  	skb_reset_mac_header(skb);
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 7babd1e9a378..30701ad5c27d 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2415,8 +2415,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>  		goto out;
>  	}
>  
> -	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> -	skb_put(skb, xdp->data_end - xdp->data);
> +	xdp_skb_reserve_put(xdp, skb);
>  
>  	/* The externally provided xdp_buff may have no metadata support, which
>  	 * is marked by xdp->data_meta being xdp->data + 1. This will lead to a
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 48efacbaa35d..0e7414472464 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -345,6 +345,14 @@ struct sk_buff *xdp_build_skb_from_frame(struct xdp_frame *xdpf,
>  					 struct net_device *dev);
>  struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf);
>  
> +static __always_inline
> +void xdp_skb_reserve_put(const struct xdp_buff *xdp,
> +			 struct sk_buff *skb)
> +{
> +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> +	__skb_put(skb, xdp->data_end - xdp->data);
> +}
> +
>  static inline
>  void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
>  			       struct xdp_buff *xdp)
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index f86eedad586a..1fca2aa1d1fe 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -646,8 +646,7 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
>  	if (unlikely(!skb))
>  		return NULL;
>  
> -	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> -	__skb_put(skb, xdp->data_end - xdp->data);
> +	xdp_skb_reserve_put(xdp, skb);
>  
>  	metalen = xdp->data - xdp->data_meta;
>  	if (metalen > 0)
> -- 
> 2.43.0
>