Re: [PATCH v2 net 3/5] tcp/udp: Call inet6_destroy_sock() in IPv6 sk->sk_destruct().

[Kuniyuki Iwashima]

From:   Eric Dumazet <edumazet@xxxxxxxxxx>
Date:   Tue, 27 Sep 2022 20:43:51 -0700
> On Tue, Sep 27, 2022 at 5:29 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> 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 IPv6 sk->sk_destruct() not to miss the future change and a similar
> > bug fixed in commit e27326009a3d ("net: ping6: Fix memleak in
> > ipv6_renew_options().")
> 
> I do not see how your patches prevent rxpmtu to be created at the time
> of IPV6_ADDRFROM ?
> 
> There seem to be races.
> 
> lockless UDP sendmsg() is a disaster really.

I think we are never able to prevent it and races exist unless we remove
the lockless path itself, so the patch makes sure to free rxpmtu at least
when we close() the socket.  Currently, we can not even free it.


> > We can now remove all inet6_destroy_sock() calls from IPv6 protocol
> > specific ->destroy() functions, but such changes are invasive to
> > backport.  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@xxxxxxxxxx>
> > ---
> >  include/net/ipv6.h  |  1 +
> >  include/net/udp.h   |  2 +-
> >  net/ipv4/udp.c      |  8 ++++++--
> >  net/ipv6/af_inet6.c |  9 ++++++++-
> >  net/ipv6/udp.c      | 15 ++++++++++++++-
> >  5 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> > index de9dcc5652c4..11f1a9a8b066 100644
> > --- a/include/net/ipv6.h
> > +++ b/include/net/ipv6.h
> > @@ -1178,6 +1178,7 @@ void ipv6_icmp_error(struct sock *sk, struct sk_buff *skb, int err, __be16 port,
> >  void ipv6_local_error(struct sock *sk, int err, struct flowi6 *fl6, u32 info);
> >  void ipv6_local_rxpmtu(struct sock *sk, struct flowi6 *fl6, u32 mtu);
> >
> > +void inet6_sock_destruct(struct sock *sk);
> >  int inet6_release(struct socket *sock);
> >  int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len);
> >  int inet6_getname(struct socket *sock, struct sockaddr *uaddr,
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index 5ee88ddf79c3..fee053bcd17c 100644
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -247,7 +247,7 @@ static inline bool udp_sk_bound_dev_eq(struct net *net, int bound_dev_if,
> >  }
> >
> >  /* net/ipv4/udp.c */
> > -void udp_destruct_sock(struct sock *sk);
> > +void udp_destruct_common(struct sock *sk);
> >  void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len);
> >  int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb);
> >  void udp_skb_destructor(struct sock *sk, struct sk_buff *skb);
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 560d9eadeaa5..a84ae44db7e2 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1598,7 +1598,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
> >  }
> >  EXPORT_SYMBOL_GPL(__udp_enqueue_schedule_skb);
> >
> > -void udp_destruct_sock(struct sock *sk)
> > +void udp_destruct_common(struct sock *sk)
> >  {
> >         /* reclaim completely the forward allocated memory */
> >         struct udp_sock *up = udp_sk(sk);
> > @@ -1611,10 +1611,14 @@ void udp_destruct_sock(struct sock *sk)
> >                 kfree_skb(skb);
> >         }
> >         udp_rmem_release(sk, total, 0, true);
> > +}
> > +EXPORT_SYMBOL_GPL(udp_destruct_common);
> >
> > +static void udp_destruct_sock(struct sock *sk)
> > +{
> > +       udp_destruct_common(sk);
> >         inet_sock_destruct(sk);
> >  }
> > -EXPORT_SYMBOL_GPL(udp_destruct_sock);
> >
> >  int udp_init_sock(struct sock *sk)
> >  {
> > diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> > index dbb1430d6cc2..0774cff62f2d 100644
> > --- a/net/ipv6/af_inet6.c
> > +++ b/net/ipv6/af_inet6.c
> > @@ -109,6 +109,13 @@ static __inline__ struct ipv6_pinfo *inet6_sk_generic(struct sock *sk)
> >         return (struct ipv6_pinfo *)(((u8 *)sk) + offset);
> >  }
> >
> > +void inet6_sock_destruct(struct sock *sk)
> > +{
> > +       inet6_destroy_sock(sk);
> > +       inet_sock_destruct(sk);
> > +}
> > +EXPORT_SYMBOL_GPL(inet6_sock_destruct);
> > +
> >  static int inet6_create(struct net *net, struct socket *sock, int protocol,
> >                         int kern)
> >  {
> > @@ -201,7 +208,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
> >                         inet->hdrincl = 1;
> >         }
> >
> > -       sk->sk_destruct         = inet_sock_destruct;
> > +       sk->sk_destruct         = inet6_sock_destruct;
> >         sk->sk_family           = PF_INET6;
> >         sk->sk_protocol         = protocol;
> >
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 3366d6a77ff2..a5256f7184ab 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -56,6 +56,19 @@
> >  #include <trace/events/skb.h>
> >  #include "udp_impl.h"
> >
> > +static void udpv6_destruct_sock(struct sock *sk)
> > +{
> > +       udp_destruct_common(sk);
> > +       inet6_sock_destruct(sk);
> > +}
> > +
> > +static int udpv6_init_sock(struct sock *sk)
> > +{
> > +       skb_queue_head_init(&udp_sk(sk)->reader_queue);
> > +       sk->sk_destruct = udpv6_destruct_sock;
> > +       return 0;
> > +}
> > +
> >  static u32 udp6_ehashfn(const struct net *net,
> >                         const struct in6_addr *laddr,
> >                         const u16 lport,
> > @@ -1723,7 +1736,7 @@ struct proto udpv6_prot = {
> >         .connect                = ip6_datagram_connect,
> >         .disconnect             = udp_disconnect,
> >         .ioctl                  = udp_ioctl,
> > -       .init                   = udp_init_sock,
> > +       .init                   = udpv6_init_sock,
> >         .destroy                = udpv6_destroy_sock,
> >         .setsockopt             = udpv6_setsockopt,
> >         .getsockopt             = udpv6_getsockopt,
> > --
> > 2.30.2