diff mbox series

[5.10,1/5] udp: Call inet6_destroy_sock() in setsockopt(IPV6_ADDRFORM).

Message ID e553cbe5451685574d097486135b804ab595d344.1680589114.git.william.xuanziyang@huawei.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series Backport complete patchset for call inet6_destroy_sock() in IPv6 sk->sk_destruct() | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Ziyang Xuan (William) April 4, 2023, 9:24 a.m. UTC 
From: Kuniyuki Iwashima <kuniyu@amazon.com>

commit 21985f43376cee092702d6cb963ff97a9d2ede68 upstream.

Commit 4b340ae20d0e ("IPv6: Complete IPV6_DONTFRAG support") forgot
to add a change to free inet6_sk(sk)->rxpmtu while converting an IPv6
socket into IPv4 with IPV6_ADDRFORM.  After conversion, sk_prot is
changed to udp_prot and ->destroy() never cleans it up, resulting in
a memory leak.

This is due to the discrepancy between inet6_destroy_sock() and
IPV6_ADDRFORM, so let's call inet6_destroy_sock() from IPV6_ADDRFORM
to remove the difference.

However, this is not enough for now because rxpmtu can be changed
without lock_sock() after commit 03485f2adcde ("udpv6: Add lockless
sendmsg() support").  We will fix this case in the following patch.

Note we will rename inet6_destroy_sock() to inet6_cleanup_sock() and
remove unnecessary inet6_destroy_sock() calls in sk_prot->destroy()
in the future.

Fixes: 4b340ae20d0e ("IPv6: Complete IPV6_DONTFRAG support")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
 include/net/ipv6.h       |  1 +
 net/ipv6/af_inet6.c      |  6 ++++++
 net/ipv6/ipv6_sockglue.c | 20 ++++++++------------
 3 files changed, 15 insertions(+), 12 deletions(-)

Comments

Greg KH April 18, 2023, 10:21 a.m. UTC | #1 
On Tue, Apr 04, 2023 at 05:24:28PM +0800, Ziyang Xuan wrote:
> From: Kuniyuki Iwashima <kuniyu@amazon.com>
> 
> commit 21985f43376cee092702d6cb963ff97a9d2ede68 upstream.

Why is this only relevant for 5.10.y?  What about 5.15.y?

For obvious reasons, we can not take patches only for older branches as
that would cause people to have regressions when moving to newer kernel
releases.  So can you please fix this up by sending a 5.15.y series, and
this 5.10.y series again, and we can queue them all up at the same time?

thanks,

greg k-h
Greg KH April 18, 2023, 10:21 a.m. UTC | #2 
On Tue, Apr 18, 2023 at 12:21:21PM +0200, Greg KH wrote:
> On Tue, Apr 04, 2023 at 05:24:28PM +0800, Ziyang Xuan wrote:
> > From: Kuniyuki Iwashima <kuniyu@amazon.com>
> > 
> > commit 21985f43376cee092702d6cb963ff97a9d2ede68 upstream.
> 
> Why is this only relevant for 5.10.y?  What about 5.15.y?
> 
> For obvious reasons, we can not take patches only for older branches as
> that would cause people to have regressions when moving to newer kernel
> releases.  So can you please fix this up by sending a 5.15.y series, and
> this 5.10.y series again, and we can queue them all up at the same time?

Also a 6.1.y series to be complete.

thanks,

greg k-h
Ziyang Xuan (William) April 18, 2023, 12:53 p.m. UTC | #3 
> On Tue, Apr 18, 2023 at 12:21:21PM +0200, Greg KH wrote:
>> On Tue, Apr 04, 2023 at 05:24:28PM +0800, Ziyang Xuan wrote:
>>> From: Kuniyuki Iwashima <kuniyu@amazon.com>
>>>
>>> commit 21985f43376cee092702d6cb963ff97a9d2ede68 upstream.
>>
>> Why is this only relevant for 5.10.y?  What about 5.15.y?
>>
>> For obvious reasons, we can not take patches only for older branches as
>> that would cause people to have regressions when moving to newer kernel
>> releases.  So can you please fix this up by sending a 5.15.y series, and
>> this 5.10.y series again, and we can queue them all up at the same time?
> 
> Also a 6.1.y series to be complete.
> 
4.14.y, 4.19.y, 5.4.y, 5.10.y, 5.15.y and 6.1.y are all involved.
Can I send series for them all together?

William Xuan

> thanks,
> 
> greg k-h
> .
>
Greg KH April 18, 2023, 12:57 p.m. UTC | #4 
On Tue, Apr 18, 2023 at 08:53:32PM +0800, Ziyang Xuan (William) wrote:
> > On Tue, Apr 18, 2023 at 12:21:21PM +0200, Greg KH wrote:
> >> On Tue, Apr 04, 2023 at 05:24:28PM +0800, Ziyang Xuan wrote:
> >>> From: Kuniyuki Iwashima <kuniyu@amazon.com>
> >>>
> >>> commit 21985f43376cee092702d6cb963ff97a9d2ede68 upstream.
> >>
> >> Why is this only relevant for 5.10.y?  What about 5.15.y?
> >>
> >> For obvious reasons, we can not take patches only for older branches as
> >> that would cause people to have regressions when moving to newer kernel
> >> releases.  So can you please fix this up by sending a 5.15.y series, and
> >> this 5.10.y series again, and we can queue them all up at the same time?
> > 
> > Also a 6.1.y series to be complete.
> > 
> 4.14.y, 4.19.y, 5.4.y, 5.10.y, 5.15.y and 6.1.y are all involved.
> Can I send series for them all together?

Yes please, as 5 different patch series.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 89ce8a50f236..5ab7616ce711 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1109,6 +1109,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_cleanup_sock(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/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 4df9dc9375c8..68d456c7abf9 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -503,6 +503,12 @@  void inet6_destroy_sock(struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(inet6_destroy_sock);
 
+void inet6_cleanup_sock(struct sock *sk)
+{
+	inet6_destroy_sock(sk);
+}
+EXPORT_SYMBOL_GPL(inet6_cleanup_sock);
+
 /*
  *	This does both peername and sockname.
  */
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 2017257cb278..7b4b457a8b87 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -429,9 +429,6 @@  static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 		if (optlen < sizeof(int))
 			goto e_inval;
 		if (val == PF_INET) {
-			struct ipv6_txoptions *opt;
-			struct sk_buff *pktopt;
-
 			if (sk->sk_type == SOCK_RAW)
 				break;
 
@@ -462,7 +459,6 @@  static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 				break;
 			}
 
-			fl6_free_socklist(sk);
 			__ipv6_sock_mc_close(sk);
 			__ipv6_sock_ac_close(sk);
 
@@ -497,14 +493,14 @@  static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 				sk->sk_socket->ops = &inet_dgram_ops;
 				sk->sk_family = PF_INET;
 			}
-			opt = xchg((__force struct ipv6_txoptions **)&np->opt,
-				   NULL);
-			if (opt) {
-				atomic_sub(opt->tot_len, &sk->sk_omem_alloc);
-				txopt_put(opt);
-			}
-			pktopt = xchg(&np->pktoptions, NULL);
-			kfree_skb(pktopt);
+
+			/* Disable all options not to allocate memory anymore,
+			 * but there is still a race.  See the lockless path
+			 * in udpv6_sendmsg() and ipv6_local_rxpmtu().
+			 */
+			np->rxopt.all = 0;
+
+			inet6_cleanup_sock(sk);
 
 			/*
 			 * ... and add it to the refcnt debug socks count