netdev - Re: [PATCH 09/14] ipv6: hand dst refs to cork setup

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: <CA+FuTSdJYwN=vxpj4nkpSxdyJ5_47PZuPTjQkRphYvLt47KdjQ@mail.gmail.com>
Date:   Tue, 11 Jan 2022 10:39:30 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Pavel Begunkov <asml.silence@...il.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/14] ipv6: hand dst refs to cork setup

On Mon, Jan 10, 2022 at 8:25 PM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> During cork->dst setup, ip6_make_skb() gets an additional reference to
> a passed in dst. However, udpv6_sendmsg() doesn't need dst after calling
> ip6_make_skb(), and so we can save two additional atomics by passing
> dst references to ip6_make_skb(). udpv6_sendmsg() is the only caller, so
> it's enough to make sure it doesn't use dst afterwards.
>
> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
> ---

There are two patches 9/14

>  net/ipv6/ip6_output.c | 9 ++++++---
>  net/ipv6/udp.c        | 3 ++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 0cc490f2cfbf..6a7bba4dd04d 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1356,6 +1356,8 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>         unsigned int mtu;
>         struct ipv6_txoptions *nopt, *opt = ipc6->opt;
>
> +       cork->base.dst = &rt->dst;
> +

Is there a reason to move this up from its original location next to
the other cork initialization assignments?

That the reference is taken in ip6_append_data for corked requests
(once, in setup cork branch) and inherited from udpv6_send_skb
otherwise is non-trivial. Worth a comment.

>         /*
>          * setup for corking
>          */
> @@ -1389,8 +1391,6 @@ static int ip6_setup_cork(struct sock *sk, struct inet_cork_full *cork,
>
>                 /* need source address above miyazawa*/
>         }
> -       dst_hold(&rt->dst);
> -       cork->base.dst = &rt->dst;
>         v6_cork->hop_limit = ipc6->hlimit;
>         v6_cork->tclass = ipc6->tclass;
>         if (rt->dst.flags & DST_XFRM_TUNNEL)
> @@ -1784,6 +1784,7 @@ int ip6_append_data(struct sock *sk,
>                 /*
>                  * setup for corking
>                  */
> +               dst_hold(&rt->dst);
>                 err = ip6_setup_cork(sk, &inet->cork, &np->cork,
>                                      ipc6, rt);
>                 if (err)
> @@ -1974,8 +1975,10 @@ struct sk_buff *ip6_make_skb(struct sock *sk,
>         int exthdrlen = (ipc6->opt ? ipc6->opt->opt_flen : 0);
>         int err;
>
> -       if (flags & MSG_PROBE)
> +       if (flags & MSG_PROBE) {
> +               dst_release(&rt->dst);
>                 return NULL;
> +       }
>
>         __skb_queue_head_init(&queue);
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index eec83e34ae27..3039dff7fe64 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1541,7 +1541,8 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>                 err = PTR_ERR(skb);
>                 if (!IS_ERR_OR_NULL(skb))
>                         err = udp_v6_send_skb(skb, fl6, &cork.base);
> -               goto out;
> +               /* ip6_make_skb steals dst reference */
> +               goto out_no_dst;
>         }
>
>         lock_sock(sk);
> --
> 2.34.1
>