linux-kernel - RE: [PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements in fail

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]
Date: Mon, 5 Feb 2024 13:47:56 -0800
From: Ira Weiny <ira.weiny@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...el.com>,
	Peter Zijlstra <peterz@...radead.org>, "Fabio M. De Francesco"
	<fabio.maria.de.francesco@...ux.intel.com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>
CC: Ingo Molnar <mingo@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Ira Weiny
	<ira.weiny@...el.com>
Subject: RE: [PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements
 in fail

Dan Williams wrote:
> Ira Weiny wrote:
> > In attempting to create a cond_guard() macro[1] Fabio asked me to do
> > some testing of the macros he was creating.  The model for this macro
> > was scoped_cond_guard() and the ability to declare a block for the error
> > path.
> > 
> > A simple test for scoped_cond_guard() was created to learn how it
> > worked and to model cond_guard() after it.  Specifically compound
> > statements were tested as suggested to be used in cond_guard().[2]
> > 
> > static int test_scoped_cond_guard(void)
> > {
> >         scoped_cond_guard(rwsem_write_try,
> >                         { printk(KERN_DEBUG "Failed\n"); return -EINVAL; },
> >                         &my_sem) {
> >                 printk(KERN_DEBUG "Protected\n");
> >         }
> >         return 0;
> > }
> > 
> > This test fails with the current code:
> > 
> > lib/test-cleanup.c: In function ‘test_scoped_cond_guard’:
> > ./include/linux/cleanup.h:190:17: error: ‘else’ without a previous ‘if’
> >   190 |                 else
> >       |                 ^~~~
> > lib/test-cleanup.c:79:9: note: in expansion of macro ‘scoped_cond_guard’
> >    79 |         scoped_cond_guard(rwsem_write_try,
> >       |         ^~~~~~~~~~~~~~~~~
> > 
> > This is due to an extra statement between the if and else blocks created
> > by the ';' in the macro.
> 
> A statement-expression "({ })" builds for me, did you test that?

I did not test that, no.

Would that be the syntax expected?  I'm not familiar with any macros like
this so perhaps I misunderstood the expected use.

> 
> > 
> > Ensure the if block is delineated properly for the use of compound
> > statements within the macro.
> > 
> > [1] https://lore.kernel.org/all/20240204173105.935612-1-fabio.maria.de.francesco@linux.intel.com/
> > [2] https://lore.kernel.org/all/65b938c1ad435_5cc6f294eb@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> > ---
> > NOTE: There is no user of this syntax yet but this is the way that Dan
> > and I thought the macro worked.  An alternate syntax would be to require
> > termination of the statement (ie use ';') in the use of the macro; see
> > below.  But this change seemed better as the compiler should drop the
> > extra statements created and allows for a bit more flexibility in the
> > use of the macro.
> > 
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index 88af56600325..6cc4bfe61bc7 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -186,7 +186,7 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> >  #define scoped_cond_guard(_name, _fail, args...) \
> >         for (CLASS(_name, scope)(args), \
> >              *done = NULL; !done; done = (void *)1) \
> > -               if (!__guard_ptr(_name)(&scope)) _fail; \
> > +               if (!__guard_ptr(_name)(&scope)) _fail \
> >                 else
> > 
> >  /*
> > diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> > index 2fabd497d659..fae110e8b89f 100644
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -441,7 +441,7 @@ static int ptrace_attach(struct task_struct *task, long request,
> >          * SUID, SGID and LSM creds get determined differently
> >          * under ptrace.
> >          */
> > -       scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> > +       scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR;,
> 
> ...otherwise, that semicolon looks out of place and unnecessary.

Agreed.  This is an alternative I considered but rejected and was only
mentioning it as part of the commit message.

Sorry for the confusion on this.  This is not the patch suggested.  See
below.

> 
> >                            &task->signal->cred_guard_mutex) {
> > 
> >                 scoped_guard (task_lock, task) {
> > ---
> >  include/linux/cleanup.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index 88af56600325..d45452ce6222 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -186,7 +186,7 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> >  #define scoped_cond_guard(_name, _fail, args...) \
> >  	for (CLASS(_name, scope)(args), \
> >  	     *done = NULL; !done; done = (void *)1) \
> > -		if (!__guard_ptr(_name)(&scope)) _fail; \
> > +		if (!__guard_ptr(_name)(&scope)) { _fail; } \
> >  		else
> >  
> >  /*
> 
> Why 2 changes to include/linux/cleanup.h in the same patch?

Sorry for the confusion.  This is the patch itself and my suggested fix.
It does not require any changes to kernel/ptrace.c

The diff above was just an alternative I thought about.

Ira