lkml.org 
[lkml]   [2022]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1 net 3/5] tcp/udp: Call inet6_destroy_sock() in IPv4 sk_prot->destroy().
Date
From:   Eric Dumazet <edumazet@google.com>
Date: Tue, 27 Sep 2022 09:50:04 -0700
> On Tue, Sep 27, 2022 at 9:13 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > Originally, inet6_sk(sk)->XXX were changed under lock_sock(), so we were
> > able to clean them up by calling inet6_destroy_sock() during the IPv6 ->
> > IPv4 conversion by IPV6_ADDRFORM. However, commit 03485f2adcde ("udpv6:
> > Add lockless sendmsg() support") added a lockless memory allocation path,
> > which could cause a memory leak:
> >
> > setsockopt(IPV6_ADDRFORM) sendmsg()
> > +-----------------------+ +-------+
> > - do_ipv6_setsockopt(sk, ...) - udpv6_sendmsg(sk, ...)
> > - lock_sock(sk) ^._ called via udpv6_prot
> > - WRITE_ONCE(sk->sk_prot, &tcp_prot) before WRITE_ONCE()
> > - inet6_destroy_sock()
> > - release_sock(sk) - ip6_make_skb(sk, ...)
> > ^._ lockless fast path for
> > the non-corking case
> >
> > - __ip6_append_data(sk, ...)
> > - ipv6_local_rxpmtu(sk, ...)
> > - xchg(&np->rxpmtu, skb)
> > ^._ rxpmtu is never freed.
> >
> > - lock_sock(sk)
> >
> > For now, rxpmtu is only the case, but let's call inet6_destroy_sock()
> > in both TCP/UDP v4 destroy functions not to miss the future change.
> >
> > We can consolidate TCP/UDP v4/v6 destroy functions, but such changes
> > are too invasive to backport to stable. So, they can be posted as a
> > follow-up later for net-next.
> >
> > Fixes: 03485f2adcde ("udpv6: Add lockless sendmsg() support")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> > Cc: Vladislav Yasevich <vyasevic@redhat.com>
> > ---
> > net/ipv4/tcp_ipv4.c | 5 +++++
> > net/ipv4/udp.c | 6 ++++++
> > net/ipv6/tcp_ipv6.c | 1 -
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index 5b019ba2b9d2..035b6c52a243 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -2263,6 +2263,11 @@ void tcp_v4_destroy_sock(struct sock *sk)
> > tcp_saved_syn_free(tp);
> >
> > sk_sockets_allocated_dec(sk);
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + if (sk->sk_prot_creator == &tcpv6_prot)
> > + inet6_destroy_sock(sk);
> > +#endif
> > }
>
> This is ugly, and will not compile with CONFIG_IPV6=m, right ?

Ah, exactly...

ld: net/ipv4/tcp_ipv4.o: in function `tcp_v4_destroy_sock':
/mnt/ec2-user/kernel/214_tcp_ipv6_renew_options_memleak/net/ipv4/tcp_ipv4.c:2290: undefined reference to `tcpv6_prot'
ld: /mnt/ec2-user/kernel/214_tcp_ipv6_renew_options_memleak/net/ipv4/tcp_ipv4.c:2291: undefined reference to `inet6_destroy_sock'
ld: net/ipv4/udp.o: in function `udp_destroy_sock':
/mnt/ec2-user/kernel/214_tcp_ipv6_renew_options_memleak/net/ipv4/udp.c:2660: undefined reference to `udpv6_prot'
ld: /mnt/ec2-user/kernel/214_tcp_ipv6_renew_options_memleak/net/ipv4/udp.c:2661: undefined reference to `inet6_destroy_sock'

So, do we have to move these 4 under net/ipv4/ with #ifdef CONFIG_IPv6 ?


> > EXPORT_SYMBOL(tcp_v4_destroy_sock);
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 560d9eadeaa5..cdf131c0a819 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -115,6 +115,7 @@
> > #include <net/udp_tunnel.h>
> > #if IS_ENABLED(CONFIG_IPV6)
> > #include <net/ipv6_stubs.h>
> > +#include <net/transp_v6.h>
> > #endif
> >
> > struct udp_table udp_table __read_mostly;
> > @@ -2666,6 +2667,11 @@ void udp_destroy_sock(struct sock *sk)
> > if (up->encap_enabled)
> > static_branch_dec(&udp_encap_needed_key);
> > }
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + if (sk->sk_prot_creator == &udpv6_prot)
> > + inet6_destroy_sock(sk);
> > +#endif
> > }
> >
> > /*
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index e54eee80ce5f..1ff6a92f7774 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -1945,7 +1945,6 @@ static int tcp_v6_init_sock(struct sock *sk)
> > static void tcp_v6_destroy_sock(struct sock *sk)
> > {
> > tcp_v4_destroy_sock(sk);
> > - inet6_destroy_sock(sk);
> > }
> >
> > #ifdef CONFIG_PROC_FS
> > --
> > 2.30.2
> >

\
 
 \ /
  Last update: 2022-09-27 20:11    [W:0.177 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site