linux-kernel - Re: [PATCH v2 5/7] cxl/core: Use guard() to drop goto pattern of cxl_dpa

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

Message-ID: <55f67727-ed24-4ac4-97c1-95b65d110daf@zohomail.com>
Date: Thu, 20 Feb 2025 09:26:08 +0800
From: Li Ming <ming.li@...omail.com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
 dave@...olabs.net, jonathan.cameron@...wei.com, dave.jiang@...el.com,
 alison.schofield@...el.com, vishal.l.verma@...el.com, ira.weiny@...el.com
Subject: Re: [PATCH v2 5/7] cxl/core: Use guard() to drop goto pattern of
 cxl_dpa_alloc()

On 2/20/2025 9:01 AM, Dan Williams wrote:
> Li Ming wrote:
>> In cxl_dpa_alloc(), some checking and operations need to be protected by
>> a rwsem called cxl_dpa_rwsem, so there is a goto pattern in
>> cxl_dpa_alloc() to release the rwsem. The goto pattern can be optimized
>> by using guard() to hold the rwsem.
>>
>> Creating a new function called __cxl_dpa_alloc() to include all checking
>> and operations needed to be procted by cxl_dpa_rwsem. Using
>> guard(rwsem_write()) to hold cxl_dpa_rwsem at the beginning of the new
>> function.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> Signed-off-by: Li Ming <ming.li@...omail.com>
>> ---
>>  drivers/cxl/core/hdm.c | 29 ++++++++++++++---------------
>>  1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 874a791f8292..1edbf7873471 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
> [..]
>> @@ -504,21 +500,24 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>>  		skip = skip_end - skip_start + 1;
>>  	} else {
>>  		dev_dbg(dev, "mode not set\n");
>> -		rc = -EINVAL;
>> -		goto out;
>> +		return -EINVAL;
>>  	}
>>  
>>  	if (size > avail) {
>>  		dev_dbg(dev, "%pa exceeds available %s capacity: %pa\n", &size,
>>  			cxl_decoder_mode_name(cxled->mode), &avail);
>> -		rc = -ENOSPC;
>> -		goto out;
>> +		return -ENOSPC;
>>  	}
>>  
>> -	rc = __cxl_dpa_reserve(cxled, start, size, skip);
>> -out:
>> -	up_write(&cxl_dpa_rwsem);
>> +	return __cxl_dpa_reserve(cxled, start, size, skip);
>> +}
>> +
>> +int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, unsigned long long size)
>> +{
>> +	struct cxl_port *port = cxled_to_port(cxled);
> Am I missing something? This @port variable is unused?
@port will be used after below __cxl_dpa_alloc().
>
>> +	int rc;
>>  
>> +	rc = __cxl_dpa_alloc(cxled, size);
>>  	if (rc)
>>  		return rc;
>>  
> So I think you can drop this new cxl_dpa_alloc + __cxl_dpa_alloc scheme?

After __cxl_dpa_alloc(), a 'devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled)' will be invoked, cxl_dpa_rwsem is possible to be held in cxl_dpa_release() in devm_add_action_or_reset() failure case.

So I create __cxl_dpa_alloc() to hold cxl_dpa_rwsem for the operations needed cxl_dpa_resem protection, make sure that the cxl_dpa_rwsem is released before devm_add_action_or_reset() invoking.

>
> After that you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@...el.com>