netdev - Re: [PATCH v21 19/24] ovpn: implement peer add/get/dump/delete via netlink

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202f2906-a5d5-4bac-aab0-25223bfa7051@openvpn.net>
Date: Tue, 4 Mar 2025 22:42:10 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH v21 19/24] ovpn: implement peer add/get/dump/delete via
 netlink

On 04/03/2025 15:35, Sabrina Dubroca wrote:
> 2025-03-04, 01:33:49 +0100, Antonio Quartulli wrote:
>> @@ -1317,11 +1336,16 @@ void ovpn_peer_keepalive_work(struct work_struct *work)
>>   
>>   	/* prevent rearming if the interface is being destroyed */
>>   	if (next_run > 0 && ovpn->registered) {
>> +		time64_t delta = next_run - now;
>> +
>>   		netdev_dbg(ovpn->dev,
>>   			   "scheduling keepalive work: now=%llu next_run=%llu delta=%llu\n",
>> -			   next_run, now, next_run - now);
>> +			   next_run, now, delta > 0 ? delta : 0);
>> +		/* due to the waiting above, the next_run deadline may have
>> +		 * passed: in this case we reschedule the worker immediately
>> +		 */
> 
> I don't understand this bit. I don't see what waiting you're refering
> to (in particular within this patch), and I don't see how we could get
> next_run < now based on how next_run is computed in
> ovpn_peer_keepalive_work_single (next_run1/next_run2 is always set to
> now + X or something that we just tested to be > now).
> 
> Am I missing something?

Sorry - this is just noise.
Before getting to this final solution, I went for an intermediate 
implementation where I was calling wait_for_completion() on a list of 
removed peers (this is "the waiting above" the comment refers to).

Since I had no clue how long I waited, I had to update 'now'.
But at that point there was a chance that we had passed the deadline for 
the next run, hence ending up with a negative delta.

This said, all this logic was thrown away, but I accidentally kept the 
check on delta (that I even argued about in a previous email when you 
saw the bogus ternary-op)

so yeah, the ternary-op can go away.

Thanks for double/triple checking with me.

Cheers,

> 
>>   		schedule_delayed_work(&ovpn->keepalive_work,
>> -				      (next_run - now) * HZ);
>> +				      delta * HZ > 0 ? delta * HZ : 0);
>>   	}
>>   	unlock_ovpn(ovpn, &release_list);
>>   }
> 

-- 
Antonio Quartulli
OpenVPN Inc.