| Message ID | 8491d89e8ae68206971f35c572190ac8b7882c1d.1722265212.git.jchapman@katalix.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ed8ebee6def7b7b760bd4fd90c03b9e86622701c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | l2tp: simplify tunnel and session cleanup | expand |
On Mon, Jul 29, 2024 at 04:38:02PM +0100, James Chapman wrote: > Use the recently exported ip_flush_pending_frames instead of a > free-coded version and lock the socket while we call it. Hmm? Isn't skb_queue_purge() closer to the original code? This is clearly not a trivial cleanup, so what are you trying to fix? > > Signed-off-by: James Chapman <jchapman@katalix.com> > Signed-off-by: Tom Parkin <tparkin@katalix.com> > --- > net/l2tp/l2tp_ip.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c > index 78243f993cda..f21dcbf3efd5 100644 > --- a/net/l2tp/l2tp_ip.c > +++ b/net/l2tp/l2tp_ip.c > @@ -236,10 +236,10 @@ static void l2tp_ip_close(struct sock *sk, long timeout) > static void l2tp_ip_destroy_sock(struct sock *sk) > { > struct l2tp_tunnel *tunnel; > - struct sk_buff *skb; > > - while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL) > - kfree_skb(skb); > + lock_sock(sk); Are you sure you really want this sock lock? > + ip_flush_pending_frames(sk); So who sets inet_sk(sk)->cork.base for l2tp socket? Thanks.
On 11/08/2024 23:40, Cong Wang wrote: > On Mon, Jul 29, 2024 at 04:38:02PM +0100, James Chapman wrote: >> Use the recently exported ip_flush_pending_frames instead of a >> free-coded version and lock the socket while we call it. > > Hmm? Isn't skb_queue_purge() closer to the original code? It is, but I thought l2tp_ip should also be calling ip_cork_release, even if it doesn't use cork. Having looked again, prompted by your comments below, I realise I made a mistake. > This is clearly not a trivial cleanup, so what are you trying to fix? This commit wasn't to fix a specific problem. I'm trying to make l2tp easier to maintain tbh. >> Signed-off-by: James Chapman <jchapman@katalix.com> >> Signed-off-by: Tom Parkin <tparkin@katalix.com> >> --- >> net/l2tp/l2tp_ip.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c >> index 78243f993cda..f21dcbf3efd5 100644 >> --- a/net/l2tp/l2tp_ip.c >> +++ b/net/l2tp/l2tp_ip.c >> @@ -236,10 +236,10 @@ static void l2tp_ip_close(struct sock *sk, long timeout) >> static void l2tp_ip_destroy_sock(struct sock *sk) >> { >> struct l2tp_tunnel *tunnel; >> - struct sk_buff *skb; >> >> - while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL) >> - kfree_skb(skb); >> + lock_sock(sk); > > > Are you sure you really want this sock lock? Hmm, you're right, it is unnecessary. I note l2tp_ip6 has similar unnecessary lock. >> + ip_flush_pending_frames(sk); > > So who sets inet_sk(sk)->cork.base for l2tp socket? I missed this. Thanks for catching it. Since this series has already been applied to net-next, I'll work on a patch to address the issues raised.
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index 78243f993cda..f21dcbf3efd5 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -236,10 +236,10 @@ static void l2tp_ip_close(struct sock *sk, long timeout) static void l2tp_ip_destroy_sock(struct sock *sk) { struct l2tp_tunnel *tunnel; - struct sk_buff *skb; - while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL) - kfree_skb(skb); + lock_sock(sk); + ip_flush_pending_frames(sk); + release_sock(sk); tunnel = l2tp_sk_to_tunnel(sk); if (tunnel) {