netdev - Re: [RFC PATCH v2 01/10] udp: implement complete book-keeping for encap

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: <CAF=yD-KoNqqujRQ_BgDjX8R0qp+arf3fGqdRGVV+wdhX7rFYrw@mail.gmail.com>
Date:   Mon, 22 Oct 2018 12:06:35 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        steffen.klassert@...unet.com
Subject: Re: [RFC PATCH v2 01/10] udp: implement complete book-keeping for encap_needed

On Fri, Oct 19, 2018 at 10:30 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> The *encap_needed static keys are enabled by UDP tunnels
> and several UDP encapsulations type, but they are never
> turned off. This can cause unneeded overall performance
> degradation for systems where such features are used
> transiently.
>
> This patch introduces complete book-keeping for such keys,
> decreasing the usage at socket destruction time, if needed,
> and avoiding that the same socket could increase the key
> usage multiple times.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  include/linux/udp.h      |  7 ++++++-
>  include/net/udp_tunnel.h |  6 ++++++
>  net/ipv4/udp.c           | 18 ++++++++++++------
>  net/ipv6/udp.c           | 14 +++++++++-----
>  4 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index 320d49d85484..a4dafff407fb 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -49,7 +49,12 @@ struct udp_sock {
>         unsigned int     corkflag;      /* Cork is required */
>         __u8             encap_type;    /* Is this an Encapsulation socket? */
>         unsigned char    no_check6_tx:1,/* Send zero UDP6 checksums on TX? */
> -                        no_check6_rx:1;/* Allow zero UDP6 checksums on RX? */
> +                        no_check6_rx:1,/* Allow zero UDP6 checksums on RX? */
> +                        encap_enabled:1; /* This socket enabled encap
> +                                          * processing; UDP tunnels and
> +                                          * different encapsulation layer set
> +                                          * this
> +                                          */
>         /*
>          * Following member retains the information to create a UDP header
>          * when the socket is uncorked.
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index fe680ab6b15a..3fbe56430e3b 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -165,6 +165,12 @@ static inline int udp_tunnel_handle_offloads(struct sk_buff *skb, bool udp_csum)
>
>  static inline void udp_tunnel_encap_enable(struct socket *sock)
>  {
> +       struct udp_sock *up = udp_sk(sock->sk);
> +
> +       if (up->encap_enabled)
> +               return;
> +
> +       up->encap_enabled = 1;
>  #if IS_ENABLED(CONFIG_IPV6)
>         if (sock->sk->sk_family == PF_INET6)
>                 ipv6_stub->udpv6_encap_enable();
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index cf8252d05a01..9fcb5374e166 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2382,11 +2382,15 @@ void udp_destroy_sock(struct sock *sk)
>         bool slow = lock_sock_fast(sk);
>         udp_flush_pending_frames(sk);
>         unlock_sock_fast(sk, slow);
> -       if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_type) {
> -               void (*encap_destroy)(struct sock *sk);
> -               encap_destroy = READ_ONCE(up->encap_destroy);
> -               if (encap_destroy)
> -                       encap_destroy(sk);
> +       if (static_branch_unlikely(&udp_encap_needed_key)) {
> +               if (up->encap_type) {
> +                       void (*encap_destroy)(struct sock *sk);
> +                       encap_destroy = READ_ONCE(up->encap_destroy);
> +                       if (encap_destroy)
> +                               encap_destroy(sk);
> +               }
> +               if (up->encap_enabled)
> +                       static_branch_disable(&udp_encap_needed_key);
>         }
>  }
>
> @@ -2431,7 +2435,9 @@ int udp_lib_setsockopt(struct sock *sk, int level, int optname,
>                         /* FALLTHROUGH */
>                 case UDP_ENCAP_L2TPINUDP:
>                         up->encap_type = val;
> -                       udp_encap_enable();
> +                       if (!up->encap_enabled)
> +                               udp_encap_enable();
> +                       up->encap_enabled = 1;

nit: no need for the branch: udp_encap_enable already has a branch and
is static inline.

Perhaps it makes sense to convert that to take the udp_sock and handle
the state change within, to avoid having to open code at multiple
locations.