netdev - Re: [PATCH net-next v16 06/26] kref/refcount: implement kref_put

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: <12b0dab0-600b-4d0b-8b0c-881d92d8ae7c@openvpn.net>
Date: Tue, 31 Dec 2024 08:31:27 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Will Deacon <will@...nel.org>
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>,
 sd@...asysnail.net, 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>, Peter Zijlstra <peterz@...radead.org>,
 Boqun Feng <boqun.feng@...il.com>, Mark Rutland <mark.rutland@....com>,
 Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH net-next v16 06/26] kref/refcount: implement
 kref_put_sock()

Hi Will,

Thanks a lot for chiming in and sorry for the delay.
See below.

On 19/12/24 18:20, Will Deacon wrote:
[...]
>> +/**
>> + * refcount_dec_and_lock_sock - return holding locked sock if able to decrement
>> + *				refcount to 0
>> + * @r: the refcount
>> + * @sock: the sock to be locked
>> + *
>> + * Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
>> + * decrement when saturated at REFCOUNT_SATURATED.
>> + *
>> + * Provides release memory ordering, such that prior loads and stores are done
>> + * before, and provides a control dependency such that free() must come after.
>> + * See the comment on top.
>> + *
>> + * Return: true and hold sock if able to decrement refcount to 0, false
>> + *	   otherwise
>> + */
>> +bool refcount_dec_and_lock_sock(refcount_t *r, struct sock *sock)
>> +{
>> +	if (refcount_dec_not_one(r))
>> +		return false;
>> +
>> +	bh_lock_sock(sock);
>> +	if (!refcount_dec_and_test(r)) {
>> +		bh_unlock_sock(sock);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(refcount_dec_and_lock_sock);
> 
> It feels a little out-of-place to me having socket-specific functions in
> lib/refcount.c. I'd suggest sticking this somewhere else _or_ maybe we
> could generate this pattern of code:
> 
> #define REFCOUNT_DEC_AND_LOCKNAME(lockname, locktype, lock, unlock)	\
> static __always_inline							\
> bool refcount_dec_and_lock_##lockname(refcount_t *r, locktype *l)	\
> {									\
> 	...
> 
> inside a generator macro in refcount.h, like we do for seqlocks in
> linux/seqlock.h. The downside of that is the cost of inlining.

Does your suggestion imply that I should convert already existing 
functions to this macro?
In that case I believe the change would be too invasive and other devs 
may not like the inlining, as you pointed out.

Secondly, I thought about moving this function to net/core/sock.c, but 
if you look at it, its logic is mostly about refcounting, with a little 
touch of sock. Hence, sock.c (or any other net file) does not seem 
appropriate either.

I guess for the time being it is more convenient to keep this function, 
and is kref counterpart, inside 'ovpn'.
They can be moved to better spots once they gained another user.

Best Regards,


-- 
Antonio Quartulli
OpenVPN Inc.