Messages in this thread | | | From | Eric Dumazet <> | Date | Tue, 27 Sep 2022 20:43:51 -0700 | Subject | Re: [PATCH v2 net 3/5] tcp/udp: Call inet6_destroy_sock() in IPv6 sk->sk_destruct(). |
| |
On Tue, Sep 27, 2022 at 5:29 PM 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 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.
> > 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@amazon.com> > --- > 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 >
| |