netdev - Re: [PATCH net] net: add and use skb_unclone

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: <CANpmjNM0iTZjrsxCam6JJ_gjJP+bXMfaVsw6Vfd56oD6d1rV0w@mail.gmail.com>
Date:   Tue, 2 Nov 2021 09:40:48 +0100
From:   Marco Elver <elver@...gle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net] net: add and use skb_unclone_keeptruesize() helper

On Tue, 2 Nov 2021 at 01:46, Eric Dumazet <eric.dumazet@...il.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
>
> While commit 097b9146c0e2 ("net: fix up truesize of cloned
> skb in skb_prepare_for_shift()") fixed immediate issues found
> when KFENCE was enabled/tested, there are still similar issues,
> when tcp_trim_head() hits KFENCE while the master skb
> is cloned.
>
> This happens under heavy networking TX workloads,
> when the TX completion might be delayed after incoming ACK.
>
> This patch fixes the WARNING in sk_stream_kill_queues
> when sk->sk_mem_queued/sk->sk_forward_alloc are not zero.
>
> Fixes: d3fb45f370d9 ("mm, kfence: insert KFENCE hooks for SLAB")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Marco Elver <elver@...gle.com>

Acked-by: Marco Elver <elver@...gle.com>

Thanks.

> ---
>  include/linux/skbuff.h | 16 ++++++++++++++++
>  net/core/skbuff.c      | 14 +-------------
>  net/ipv4/tcp_output.c  |  6 +++---
>  3 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 841e2f0f5240ba9e210bb9a3fc1cbedc2162b2a8..b8c273af2910c780dcfbc8f18fc05e115089010b 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1671,6 +1671,22 @@ static inline int skb_unclone(struct sk_buff *skb, gfp_t pri)
>         return 0;
>  }
>
> +/* This variant of skb_unclone() makes sure skb->truesize is not changed */
> +static inline int skb_unclone_keeptruesize(struct sk_buff *skb, gfp_t pri)
> +{
> +       might_sleep_if(gfpflags_allow_blocking(pri));
> +
> +       if (skb_cloned(skb)) {
> +               unsigned int save = skb->truesize;
> +               int res;
> +
> +               res = pskb_expand_head(skb, 0, 0, pri);
> +               skb->truesize = save;
> +               return res;
> +       }
> +       return 0;
> +}
> +
>  /**
>   *     skb_header_cloned - is the header a clone
>   *     @skb: buffer to check
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index fe9358437380c826d6438efe939afc4b38135cff..38d7dee4bbe9e96a811ff9cfca33429b5f7dbff1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3449,19 +3449,7 @@ EXPORT_SYMBOL(skb_split);
>   */
>  static int skb_prepare_for_shift(struct sk_buff *skb)
>  {
> -       int ret = 0;
> -
> -       if (skb_cloned(skb)) {
> -               /* Save and restore truesize: pskb_expand_head() may reallocate
> -                * memory where ksize(kmalloc(S)) != ksize(kmalloc(S)), but we
> -                * cannot change truesize at this point.
> -                */
> -               unsigned int save_truesize = skb->truesize;
> -
> -               ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> -               skb->truesize = save_truesize;
> -       }
> -       return ret;
> +       return skb_unclone_keeptruesize(skb, GFP_ATOMIC);
>  }
>
>  /**
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 6d72f3ea48c4ef0d193ec804653e4d4321f3f20a..0492f6942778db21f855216bf4387682fb37091e 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1562,7 +1562,7 @@ int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
>                 return -ENOMEM;
>         }
>
> -       if (skb_unclone(skb, gfp))
> +       if (skb_unclone_keeptruesize(skb, gfp))
>                 return -ENOMEM;
>
>         /* Get a new skb... force flag on. */
> @@ -1672,7 +1672,7 @@ int tcp_trim_head(struct sock *sk, struct sk_buff *skb, u32 len)
>  {
>         u32 delta_truesize;
>
> -       if (skb_unclone(skb, GFP_ATOMIC))
> +       if (skb_unclone_keeptruesize(skb, GFP_ATOMIC))
>                 return -ENOMEM;
>
>         delta_truesize = __pskb_trim_head(skb, len);
> @@ -3184,7 +3184,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
>                                  cur_mss, GFP_ATOMIC))
>                         return -ENOMEM; /* We'll try again later. */
>         } else {
> -               if (skb_unclone(skb, GFP_ATOMIC))
> +               if (skb_unclone_keeptruesize(skb, GFP_ATOMIC))
>                         return -ENOMEM;
>
>                 diff = tcp_skb_pcount(skb);
> --
> 2.33.1.1089.g2158813163f-goog
>