netdev - Re: [PATCH net-next v3 2/3] udp: elide zerocopy operation in hot path

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, 29 Nov 2018 15:37:25 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Network Development <netdev@...r.kernel.org>
Cc:     David Miller <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v3 2/3] udp: elide zerocopy operation in hot path

On Thu, Nov 29, 2018 at 3:26 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> From: Willem de Bruijn <willemb@...gle.com>
>
> With MSG_ZEROCOPY, each skb holds a reference to a struct ubuf_info.
> Release of its last reference triggers a completion notification.
>
> The TCP stack in tcp_sendmsg_locked holds an extra ref independent of
> the skbs, because it can build, send and free skbs within its loop,
> possibly reaching refcount zero and freeing the ubuf_info too soon.
>
> The UDP stack currently also takes this extra ref, but does not need
> it as all skbs are sent after return from __ip(6)_append_data.
>
> Avoid the extra refcount_inc and refcount_dec_and_test, and generally
> the sock_zerocopy_put in the common path, by passing the initial
> reference to the first skb.
>
> This approach is taken instead of initializing the refcount to 0, as
> that would generate error "refcount_t: increment on 0" on the
> next skb_zcopy_set.
>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 9602746d7175..08ff04d12642 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -881,8 +881,8 @@ static int __ip_append_data(struct sock *sk,
>         int csummode = CHECKSUM_NONE;
>         struct rtable *rt = (struct rtable *)cork->dst;
>         unsigned int wmem_alloc_delta = 0;
> +       bool paged, extra_uref;
>         u32 tskey = 0;
> -       bool paged;
>
>         skb = skb_peek_tail(queue);
>
> @@ -921,12 +921,13 @@ static int __ip_append_data(struct sock *sk,
>                 uarg = sock_zerocopy_realloc(sk, length, skb_zcopy(skb));
>                 if (!uarg)
>                         return -ENOBUFS;
> +               extra_uref = true;
>                 if (rt->dst.dev->features & NETIF_F_SG &&
>                     csummode == CHECKSUM_PARTIAL) {
>                         paged = true;
>                 } else {
>                         uarg->zerocopy = 0;
> -                       skb_zcopy_set(skb, uarg);
> +                       skb_zcopy_set(skb, uarg, &extra_uref);
>                 }
>         }
>
> @@ -1019,7 +1020,7 @@ static int __ip_append_data(struct sock *sk,
>                         cork->tx_flags = 0;
>                         skb_shinfo(skb)->tskey = tskey;
>                         tskey = 0;
> -                       skb_zcopy_set(skb, uarg);
> +                       skb_zcopy_set(skb, uarg, &extra_uref);
>
>                         /*
>                          *      Find where to start putting bytes.
> @@ -1123,13 +1124,12 @@ static int __ip_append_data(struct sock *sk,
>
>         if (wmem_alloc_delta)
>                 refcount_add(wmem_alloc_delta, &sk->sk_wmem_alloc);
> -       sock_zerocopy_put(uarg);
>         return 0;
>
>  error_efault:
>         err = -EFAULT;
>  error:
> -       sock_zerocopy_put_abort(uarg);
> +       sock_zerocopy_put_abort(uarg, extra_uref);

I'll need another revision. Sorry for the spam.

In the draft patch I suggested that the skb_zcopy_set needs to be
moved below getfrag, so that the uarg is not freed on the only
error path that calls kfree_skb inside the main loop.

This is still needed. Else sock_zerocopy_put_abort here is
reached with a non-NULL, but already freed uarg.

Will send a v4. Will let this sit for at least a day in case anyone
else has comments.