netdev - Re: [PATCH net-next 03/15] l2tp: have l2tp_ip_destroy_sock use ip_flush_pending

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9bf3b940-ca7d-21f4-2317-d133a34b57eb@katalix.com>
Date: Mon, 12 Aug 2024 16:28:09 +0100
From: James Chapman <jchapman@...alix.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, dsahern@...nel.org, tparkin@...alix.com
Subject: Re: [PATCH net-next 03/15] l2tp: have l2tp_ip_destroy_sock use
 ip_flush_pending_frames

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@...alix.com>
>> Signed-off-by: Tom Parkin <tparkin@...alix.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.