linux-kernel - Re: [PATCH net 2/2] net: ioam6: mitigate the two reallocations problem

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: <d6b0d5ff-d3b0-4eda-b6ed-11add69f4dff@uliege.be>
Date: Tue, 2 Jul 2024 20:30:13 +0200
From: Justin Iurman <justin.iurman@...ege.be>
To: netdev@...r.kernel.org
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org,
 justin.iurman@...ege.be
Subject: Re: [PATCH net 2/2] net: ioam6: mitigate the two reallocations
 problem

On 7/2/24 19:44, Justin Iurman wrote:
> Get the cache _before_ adding bytes. This way, we provide the dst entry
> to skb_cow_head(), so that we call LL_RESERVED_SPACE() on it and avoid
> two reallocations in some specific cases. We cannot do much when the dst
> entry is empty (cache is empty, this is the first time): in that case,
> we use skb->mac_len by default and two reallocations will happen in
> those specific cases. However, it will only happen once, not every
> single time.

This fix could also be applied to seg6 and rpl. Not sure if the problem 
would show up though (I did some quick computations, seems unlikely), 
but still... would probably be interesting to have it there too, just in 
case. Any opinion?

> Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
> ---
>   net/ipv6/ioam6_iptunnel.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index b08c13550144..e5a7e7472b71 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -220,14 +220,16 @@ static int ioam6_do_fill(struct net *net, struct sk_buff *skb)
>   }
>   
>   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 + (!dst ? skb->mac_len
> +					       : LL_RESERVED_SPACE(dst->dev)));
>   	if (unlikely(err))
>   		return err;
>   
> @@ -256,16 +258,17 @@ static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>   
>   static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>   			  struct ioam6_lwt_encap *tuninfo,
> -			  struct in6_addr *tundst)
> +			  struct in6_addr *tundst,
> +			  struct dst_entry *dst)
>   {
> -	struct dst_entry *dst = skb_dst(skb);
>   	struct ipv6hdr *hdr, *inner_hdr;
>   	int hdrlen, len, err;
>   
>   	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>   	len = sizeof(*hdr) + hdrlen;
>   
> -	err = skb_cow_head(skb, len + skb->mac_len);
> +	err = skb_cow_head(skb, len + (!dst ? skb->mac_len
> +					    : LL_RESERVED_SPACE(dst->dev)));
>   	if (unlikely(err))
>   		return err;
>   
> @@ -285,7 +288,7 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>   	hdr->nexthdr = NEXTHDR_HOP;
>   	hdr->payload_len = cpu_to_be16(skb->len - sizeof(*hdr));
>   	hdr->daddr = *tundst;
> -	ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> +	ipv6_dev_get_saddr(net, skb_dst(skb)->dev, &hdr->daddr,
>   			   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
>   
>   	skb_postpush_rcsum(skb, hdr, len);
> @@ -313,6 +316,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   
>   	orig_daddr = ipv6_hdr(skb)->daddr;
>   
> +	local_bh_disable();
> +	dst = dst_cache_get(&ilwt->cache);
> +	local_bh_enable();
> +
>   	switch (ilwt->mode) {
>   	case IOAM6_IPTUNNEL_MODE_INLINE:
>   do_inline:
> @@ -320,7 +327,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		if (ipv6_hdr(skb)->nexthdr == NEXTHDR_HOP)
>   			goto out;
>   
> -		err = ioam6_do_inline(net, skb, &ilwt->tuninfo);
> +		err = ioam6_do_inline(net, skb, &ilwt->tuninfo, dst);
>   		if (unlikely(err))
>   			goto drop;
>   
> @@ -328,7 +335,8 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   	case IOAM6_IPTUNNEL_MODE_ENCAP:
>   do_encap:
>   		/* Encapsulation (ip6ip6) */
> -		err = ioam6_do_encap(net, skb, &ilwt->tuninfo, &ilwt->tundst);
> +		err = ioam6_do_encap(net, skb,
> +				     &ilwt->tuninfo, &ilwt->tundst, dst);
>   		if (unlikely(err))
>   			goto drop;
>   
> @@ -346,10 +354,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		goto drop;
>   	}
>   
> -	local_bh_disable();
> -	dst = dst_cache_get(&ilwt->cache);
> -	local_bh_enable();
> -
>   	if (unlikely(!dst)) {
>   		struct ipv6hdr *hdr = ipv6_hdr(skb);
>   		struct flowi6 fl6;
> @@ -371,15 +375,15 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		local_bh_disable();
>   		dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
>   		local_bh_enable();
> +
> +		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> +		if (unlikely(err))
> +			goto drop;
>   	}
>   
>   	skb_dst_drop(skb);
>   	skb_dst_set(skb, dst);
>   
> -	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> -	if (unlikely(err))
> -		goto drop;
> -
>   	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr))
>   		return dst_output(net, sk, skb);
>   out: