netdev - Re: [PATCH v2 net] tcp/dccp: Don't use timer_pending() in reqsk_queue

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6f19948a-aaaa-439d-9cad-64ac24d92303@linux.dev>
Date: Fri, 11 Oct 2024 21:13:47 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 kuba@...nel.org, kuni1840@...il.com, martin.lau@...nel.org,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH v2 net] tcp/dccp: Don't use timer_pending() in
 reqsk_queue_unlink().

On 10/10/24 10:36 AM, Kuniyuki Iwashima wrote:
> From: Martin KaFai Lau <martin.lau@...ux.dev>
> Date: Wed, 9 Oct 2024 22:46:57 -0700
>> On 10/9/24 10:42 AM, Kuniyuki Iwashima wrote:
>>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>>> index 2c5632d4fddb..23cff5278a64 100644
>>> --- a/net/ipv4/inet_connection_sock.c
>>> +++ b/net/ipv4/inet_connection_sock.c
>>> @@ -1045,12 +1045,13 @@ static bool reqsk_queue_unlink(struct request_sock *req)
>>>    		found = __sk_nulls_del_node_init_rcu(sk);
>>>    		spin_unlock(lock);
>>>    	}
>>> -	if (timer_pending(&req->rsk_timer) && del_timer_sync(&req->rsk_timer))
>>> -		reqsk_put(req);
>>> +
>>>    	return found;
>>>    }
>>>    
>>> -bool inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req)
>>> +static bool __inet_csk_reqsk_queue_drop(struct sock *sk,
>>> +					struct request_sock *req,
>>> +					bool from_timer)
>>>    {
>>>    	bool unlinked = reqsk_queue_unlink(req);
>>>    
>>> @@ -1058,8 +1059,17 @@ bool inet_csk_reqsk_queue_drop(struct sock *sk, struct request_sock *req)
>>>    		reqsk_queue_removed(&inet_csk(sk)->icsk_accept_queue, req);
>>>    		reqsk_put(req);
>>>    	}
>>> +
>>> +	if (!from_timer && timer_delete_sync(&req->rsk_timer))
>>
>> timer_delete_sync() is now done after the above reqsk_queue_removed().
>> The reqsk_timer_handler() may do the "req->num_timeout++" while the above
>> reqsk_queue_removed() needs to check for req->num_timeout. Would it race?
> 
> Ah thanks!
> I moved it for better @unlinked access, but will move above.
> 
> Btw, do you have any hint why the connection was processed on a different
> cpu, not one where reqsk timer was pinned ?

Just saw this after replying on v1. I don't know what exactly caused this. I am 
only aware we have a recent steering test to test different packet steering setup.

[ I had some email client issues, so the reply ordering has been wrong :( ]