[PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements in fail

[PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements in fail

[Ira Weiny]

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.

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@intel.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;,
                           &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
 
 /*

---
base-commit: 03c972291873663f15c78ff4ca07cbf5025735f8
change-id: 20240201-cond_guard-afa0566cecdf

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

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

[Dan Williams]

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?

> 
> 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@intel.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.

>                            &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?

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

[Ira Weiny]

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@intel.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