Received-SPF: pass (google.com: domain of linux-kernel+bounces-53982-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 5 Feb 2024 13:47:56 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Ira Weiny <ira.weiny@intel.com>,
	Peter Zijlstra <peterz@infradead.org>, "Fabio M. De Francesco"
	<fabio.maria.de.francesco@linux.intel.com>, Jonathan Cameron
	<Jonathan.Cameron@huawei.com>
CC: Ingo Molnar <mingo@kernel.org>, Oleg Nesterov <oleg@redhat.com>,
	<linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>, Ira Weiny
	<ira.weiny@intel.com>
Subject: RE: [PATCH RFC] cleanup/scoped_cond_guard: Fix multiple statements
 in fail
Message-ID: <65c1578c76def_37447929456@iweiny-mobl.notmuch>
References: <20240205-cond_guard-v1-1-b8d597a30cdd@intel.com>
 <65c131c2e2ec6_4e7f529442@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <65c131c2e2ec6_4e7f529442@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RE5OS3NDT0Y5aFdDemIrZWJOckpkMVdCelI3UXBtUVF5UUJRZk9XMkxSRGxa?=
 =?utf-8?B?TFpwcndveXh4SU9DUUp4ajJWNlRCa2tyTWQzMVhQd2FHL0ZHVVB0akpSbi9v?=
 =?utf-8?B?c1REYzdZMDN1SFpGSFZNTCsxSHo2MXlleVdXd1RqR1UwLzQxdlBMVWhUZk1j?=
 =?utf-8?B?cC9JYWpnekd5RnBCL29RRnRBbmhmR2pEcDEwWkV0RkgwWHZiZVR4bStMM1V4?=
 =?utf-8?B?dmU5ZFpzYTlGOVF4VUlZbHNxdnVyN0JyWXAwR2FLTVgvVDNZMmYxeVdBOWtH?=
 =?utf-8?B?azBSZWw4NllscFd3NWN3Y0pGdE1aOTRFbEpiditFODBQeHpBYlVEWG1zSFBj?=
 =?utf-8?B?MmU1TFZpRDA1QlJTNzhES05heUZxWUk3OTBFVnRjczhTazhPYTA4YkVnOHFE?=
 =?utf-8?B?UG0xMFk5ZWQzdlV2T1ROS3FZK05La0xCZFplV3Nqb0JiQ0hVRGM1dE5oUWdM?=
 =?utf-8?B?MjlHWUxaUXFrT1MzRTNseHkrOUI5c2I0aTVESm14c1M2K2F0QU1NSVI3c3Vp?=
 =?utf-8?B?aFgwYWl1MTl1L212bHlYeGRjS3ovNUlUREJwMzJMM3E1U1k5SzRZYVg4ZE9L?=
 =?utf-8?B?K2JFVmdXTzNuVzBpazZjZkYzWkZiNVhRRlBEb0JNeXJXcFg4c0tLVjBqelZv?=
 =?utf-8?B?VWkvRjFoOGo0UmFaKy9xVmtrQnF1QkhhYWlBN2QvV1ZwNTg4V1VYdlJmalNr?=
 =?utf-8?B?YXZoVUZJT1l0UVNKN25IMVN6amQ5c2YxVlZWb3dSRVZ5VE91L3N1VkpVWnE3?=
 =?utf-8?B?WkJ5anFSZi9DYmxqQkJYZHVQb29keFl3OUdzU09TQ1hJRXk5WmJFQzNsOGRR?=
 =?utf-8?B?TWVVR096VkFtVlJudlg5WVhEWWU5UDZkMjJCYlozdVFrZVpqaGZyK2JuakJM?=
 =?utf-8?B?eklHdWFmMGU2SjhFSFc1WDJUKzN0V2JJbExpV3VsSThHNE54aXN3U2cvdVhU?=
 =?utf-8?B?dkdoL21uNVVBZ0habjZYSXNvRFgybmdzKzc2cDJXQlVpS2xOYVNqK3M3aHV6?=
 =?utf-8?B?d1VtaUdsTGdLbC9yN0JzOFl2ODFLeXA0QWVIWlRER2xmaUZ6QTBITzBwQ1Nx?=
 =?utf-8?B?Z2w3a29rTTBpVjc2aHdoSEZVbytBWW9GNUZGaGlldFQvNllnQ0xQZElQU0FV?=
 =?utf-8?B?bHNYRy9sRGRCeDZBOE41SnMvaDA1T0NuaEhEMFY5SEtRQmRkbS9QU1ZyN1ZV?=
 =?utf-8?B?SVRTK1NqZXBCUTRCeWFTd3htMTJSQ1BieExNYzVtQXREeEdVV2lRdnlPa3hQ?=
 =?utf-8?B?ZDVjN1czUEpvR1hRWnpaZ2dBb0ZMYXFtdjNjWnczZDY1VXU4R0xTM3NVL29x?=
 =?utf-8?B?RDJZQk92T1NBeTlpdnpvUVVpMmNWNW40VGMvT1pJMjBRMGFnWDU5SDVqR3hH?=
 =?utf-8?B?N1VHcHZoemVVRW1KczRLVFl5QnpLV29uOFhDQlR1alBzczAyTlh3OVFQYllz?=
 =?utf-8?B?ZzJvdERnc3BaU0RyRm9pVEdiQk9lS1A5WGRjcTlsWkFPZkRqbEVFRXZKVXlY?=
 =?utf-8?B?b2FDQUpSWmFrb1A2QzhxTUVmNUxuZUErMnk3YlliWkl2UGVyYlExYy90WEhS?=
 =?utf-8?B?RW1vRGFPK255U1ZQU1ZBNmE2bytVT0xKYmphZ2VXY2F3R0ROZE9PRmFnQ3Ft?=
 =?utf-8?B?dmMwK2RqN2gwcU0rbm9NR0h4Q0J3SGV3ZnduazEyRHFPQllnbXUxenFRRjFV?=
 =?utf-8?B?d1RMVHlWeTRPd3g1UG9UamZVaC8rSlllTFcrU2Zzd1JxRUc4RldWNDZPY0k0?=
 =?utf-8?B?cGlWaGlObVpwdzZKVEpsOVAwaDVxTCtqb0xBOUNFQlhzcmJWODNSWjBvbGt1?=
 =?utf-8?B?MTVvaVV5ZEJ1c09SMjZwNUdJait1Y1paRGxjUkkyZUJPKzE0ZXJlZVdTUiti?=
 =?utf-8?B?YWRhTFA2Y3JWVjRoQUJKdDRaTll2UVJqdm1NaU5xVU5OT04xN2VJMnZuVHNk?=
 =?utf-8?B?S0U4dXhqdHRRaHd5R0lCREJsb3BCVXlhWXF1dmJRMTIrMWJoVzlPc2s3dC9S?=
 =?utf-8?B?WmtGL2k3c1Q1RUw5dk1HYTltWTVwOFpZY3dsN1Faekk1VFZFM0k1NVlTa3E1?=
 =?utf-8?B?MlBwTTNiSHIvSjJ4THdEWk5TUEZYUlY1WlR4VkJHY2thblhyQ28zY3pGTmN2?=
 =?utf-8?Q?eGqAZHv6S7IpFroRd3dHjXy8F?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b9b576e1-b8eb-4927-3f4a-08dc26941f01
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2024 21:47:59.8721
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Rx45XoN74GWH5+lsFU6LXL8Zl5SXaVLP7bSw9i289AXYYdH1GJXjEtR/OjUfR3ScJlTNyB9U7t7+DxqGZ+26NA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5347
X-OriginatorOrg: intel.com

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