On Thu, Aug 13, 2020 at 1:59 PM Miaohe Lin <linmia...@huawei.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 <linmia...@huawei.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.
