netdev - Re: [PATCH] net: correct zerocopy refcnt with newly allocated UDP or RAW uarg

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]

Date:   Thu, 13 Aug 2020 14:34:19 +0200
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     David Miller <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: correct zerocopy refcnt with newly allocated UDP or
 RAW uarg

On Thu, Aug 13, 2020 at 1:59 PM Miaohe Lin <linmiaohe@...wei.com> wrote:
>
> The var extra_uref is introduced to pass the initial reference taken in
> sock_zerocopy_alloc to the first generated skb. But now we may fail to pass
> the initial reference with newly allocated UDP or RAW uarg when the skb is
> zcopied.
>
> If the skb is zcopied, we always set extra_uref to false. This is fine with
> reallocted uarg because no extra ref is taken by UDP and RAW zerocopy. But
> if uarg is newly allocated via sock_zerocopy_alloc(), we lost the initial
> reference because extra_uref is false and we missed to pass it to the first
> generated skb.
>
> To fix this, we should set extra_uref to true if UDP or RAW uarg is newly
> allocated when the skb is zcopied.

extra_uref is true if there is no previous skb to append to or there
is a previous skb, but that does not have zerocopy data associated yet
(because the previous call(s) did not set MSG_ZEROCOPY).

In other words, when first (allocating and) associating a zerocopy
struct with the skb.



> Fixes: 522924b58308 ("net: correct udp zerocopy refcnt also when zerocopy only on append")
> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
> ---
>  net/ipv4/ip_output.c  | 4 +++-
>  net/ipv6/ip6_output.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 61f802d5350c..78d3b5d48617 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1019,7 +1019,9 @@ static int __ip_append_data(struct sock *sk,
>                 uarg = sock_zerocopy_realloc(sk, length, skb_zcopy(skb));
>                 if (!uarg)
>                         return -ENOBUFS;
> -               extra_uref = !skb_zcopy(skb);   /* only ref on new uarg */
> +               /* Only ref on newly allocated uarg. */
> +               if (!skb_zcopy(skb) || (sk->sk_type != SOCK_STREAM && skb_zcopy(skb) != uarg))
> +                       extra_uref = true;

SOCK_STREAM does not use __ip_append_data.

This leaves as new branch skb_zcopy(skb) && skb_zcopy(skb) != uarg.

This function can only acquire a uarg through sock_zerocopy_realloc,
which on skb_zcopy(skb) only returns the existing uarg or NULL (for
not SOCK_STREAM).

So I don't see when that condition can happen.