netdev - Re: [RFC PATCH v2] Simply enable one to write code like:

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: <a4efffb9-9c63-4cb1-94a3-050260fe0a81@intel.com>
Date: Wed, 2 Oct 2024 11:55:45 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>, Andy Shevchenko
	<andriy.shevchenko@...el.com>, Dan Carpenter <dan.carpenter@...aro.org>
CC: <linux-kernel@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>,
	<amadeuszx.slawinski@...ux.intel.com>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, <nex.sw.ncis.osdt.itp.upstreaming@...el.com>,
	<netdev@...r.kernel.org>, Markus Elfring <Markus.Elfring@....de>
Subject: Re: [RFC PATCH v2] Simply enable one to write code like:

On 10/1/24 17:21, Dmitry Torokhov wrote:
> Hi Przemek,
> 
> On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
>> int foo(struct my_drv *adapter)
>> {
>> 	scoped_guard(spinlock, &adapter->some_spinlock)
>> 		return adapter->spinlock_protected_var;
>> }
> 
> Could you change the subject to say something like:
> 
> "Adjust cond_guard() implementation to avoid potential warnings"
> 
> And then give detailed explanation in the body?

thanks, sure
(and apologies that I forgot to add any subject :F (this was just my
very first non-subject paragraph))

> 
>>
>> Current scoped_guard() implementation does not support that,
>> due to compiler complaining:
>> error: control reaches end of non-void function [-Werror=return-type]
>>
>> One could argue that for such use case it would be better to use
>> guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
>> that coding with my proposed locking style is also very pleasant, as I'm
>> doing so for a few weeks already.
> 
> I'd drop this paragraph from the patch description (and moved past "---"
> if you prefer to keep it for additional context.

I will think about that, especially given that since v2 this patch is
not only fixing "my case", but just it's regular hardening for static
analysis needs.


>> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
> 
> This is not supposed to be used outside of cleanup.h so probably
> __DEFINE_CLASS_IS_CONDITIONAL()?

indeed

>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
>> +		     ({goto _label;}))
> 
> The "jump back" throws me a little, do you think if can be rewritten as:
> 
> 	if (true)
> 		for (...)
> 	else
> 		_label: /* dummy */ ;

user code must be glued at the end, so there must be "if (0) label:"
however I figured that you could reorder for and else:

	for (
		CLASS(...);
		__guard_ptr(...) || __is_cond_ptr(...);
		({ goto label; })
	)
		if (0)
			label:
				break;
		else
			// actual user code glued here

and this jumps forward

> 
>>   
>>   #define scoped_cond_guard(_name, _fail, args...) \
>>   	for (CLASS(_name, scope)(args), \
> 
> With your __is_cond_ptr() can this be made to warn or error if
> scoped_cond_guard() is used with a non-conditional lock/class? As that
> would make no sense.

good idea, thanks