netdev - Re: [PATCH net-next v3 1/3] net: ipv6: ioam6

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] [day] [month] [year] [list]

Message-ID: <f4668e71-796d-4fcf-994a-db032c6c43b6@linux.dev>
Date: Tue, 29 Oct 2024 10:58:42 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Justin Iurman <justin.iurman@...ege.be>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 kuba@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com,
 horms@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/3] net: ipv6: ioam6_iptunnel: mitigate
 2-realloc issue

On 28/10/2024 22:36, Justin Iurman wrote:
> This patch mitigates the two-reallocations issue with ioam6_iptunnel by
> providing the dst_entry (in the cache) to the first call to
> skb_cow_head(). As a result, the very first iteration would still
> trigger two reallocations (i.e., empty cache), while next iterations
> would only trigger a single reallocation.
> 
> Performance tests before/after applying this patch, which clearly shows
> the improvement:
> - inline mode:
>    - before: https://ibb.co/LhQ8V63
>    - after: https://ibb.co/x5YT2bS
> - encap mode:
>    - before: https://ibb.co/3Cjm5m0
>    - after: https://ibb.co/TwpsxTC
> - encap mode with tunsrc:
>    - before: https://ibb.co/Gpy9QPg
>    - after: https://ibb.co/PW1bZFT
> 
> This patch also fixes an incorrect behavior: after the insertion, the
> second call to skb_cow_head() makes sure that the dev has enough
> headroom in the skb for layer 2 and stuff. In that case, the "old"
> dst_entry was used, which is now fixed. After discussing with Paolo, it
> appears that both patches can be merged into a single one -this one-
> (for the sake of readability) and target net-next.
> 
> Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
> ---
>   net/ipv6/ioam6_iptunnel.c | 90 +++++++++++++++++++++------------------
>   1 file changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index beb6b4cfc551..07bfd557e08a 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -254,15 +254,24 @@ static int ioam6_do_fill(struct net *net, struct sk_buff *skb)
>   	return 0;
>   }
>   
> +static inline int dev_overhead(struct dst_entry *dst, struct sk_buff *skb)
> +{
> +	if (likely(dst))
> +		return LL_RESERVED_SPACE(dst->dev);
> +
> +	return skb->mac_len;
> +}

static inline functions in .c files are not welcome.
consider to move this helper to some header, probably dev.h or dst.h
and reuse it in other tunnels.

And please honor 24h rule before the next submission.


>   static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
> -			   struct ioam6_lwt_encap *tuninfo)
> +			   struct ioam6_lwt_encap *tuninfo,
> +			   struct dst_entry *dst)
>   {
>   	struct ipv6hdr *oldhdr, *hdr;
>   	int hdrlen, err;
>   
>   	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>   
> -	err = skb_cow_head(skb, hdrlen + skb->mac_len);
> +	err = skb_cow_head(skb, hdrlen + dev_overhead(dst, skb));
>   	if (unlikely(err))
>   		return err;
>   
[.. snip ..]