linux-kernel - Re: [RFC PATCH v2] cleanup: Add cond

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

Message-ID: <65b9979895a8e_5cc6f2942f@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Tue, 30 Jan 2024 16:43:04 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	"Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>,
	<linux-kernel@...r.kernel.org>
CC: "Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>, "Peter
 Zijlstra" <peterz@...radead.org>, Dan Williams <dan.j.williams@...el.com>,
	Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> > > Dan Williams wrote:
> > > > Fabio M. De Francesco wrote:
> > > > > Add cond_guard() to conditional guards.
> > > > > 
> > > > > cond_guard() is used for the _interruptible(), _killable(), and _try
> > > > > versions of locks. It expects a block where the failure can be handled
> > > > > (e.g., calling printk() and returning -EINTR in case of failure).
> > > > > 
> > > > > As the other guards, it avoids to open code the release of the lock
> > > > > after a goto to an 'out' label.
> > > > > 
> > > > > This remains an RFC because Dan suggested a slightly different syntax:
> > > > > 
> > > > > 	if (cond_guard(...))
> > > > > 		return -EINTR;
> > > > > 
> > > > > But the scoped_cond_guard() macro omits the if statement:
> > > > > 
> > > > >     	scoped_cond_guard (...) {
> > > > >     	}
> > > > > 
> > > > > Thus define cond_guard() similarly to scoped_cond_guard() but with a block
> > > > > to handle the failure case:
> > > > > 
> > > > > 	cond_guard(...)
> > > > > 		return -EINTR;
> > > > 
> > > > That's too subtle for me, because of the mistakes that can be made with
> > > > brackets how about a syntax like:
> > > > 
> > > >  	cond_guard(..., return -EINTR, ...)
> > > > 
> > > > ...to make it clear what happens if the lock acquisition fails without
> > > > having to remember there is a hidden incomplete "if ()" statement in
> > > > that macro? More below...
> > > 
> > > I sympathize with the hidden "if" being confusing but there is already
> > > precedent in the current *_guard macros.  So I'd like to know if Peter has
> > > an opinion.
> > 
> > What are you asking specifically? The current scoped_cond_guard()
> > already properly encapsulates the "if ()" and takes an "_fail" so why
> > wouldn't cond_guard() also safely encpsulate an "if ()" and take an
> > "_fail" statement argument?
> 
> Maybe I misunderstood you.  I thought you were advocating that the 'if'
> would not be encapsulated.  And I was wondering if Peter had an opinion.
> 

Last I sent to Fabio was this:

>> You can steal the embedded "if ()" trick from scoped_cond_guard() and do
>> something like (untested):
>> 
>> #define cond_guard(_name, _fail, args...) \
>>         CLASS(_name, scope)(args); \
>>         if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;


> But if you are agreeing with the direction of this patch regarding the if
> then ignore me.

I disagree with the proposal that the caller needs to understand that
the macro leaves a dangling "if ()". I am ok with aligning with
scoped_cond_guard() where the caller can assume that the "_fail"
statement has been executed with no "if ()" sequence to terminate.