[PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump

[Su Hui]

On 2025/6/23 23:06, Dan Carpenter wrote:
> On Mon, Jun 23, 2025 at 06:47:05PM +0800, Su Hui wrote:
>> There are three cleanups for vmcore_add_device_dump(). Adjust data_size's
>> type from 'size_t' to 'unsigned int' for the consistency of data->size.
>> Return -ENOMEM directly rather than goto the label to simplify the code.
>> Using scoped_guard() to simplify the lock/unlock code.
>>
>> Signed-off-by: Su Hui <suhui at nfschina.com>
>> ---
>>   fs/proc/vmcore.c | 33 ++++++++++++++-------------------
>>   1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
>> index 10d01eb09c43..9ac2863c68d8 100644
>> --- a/fs/proc/vmcore.c
>> +++ b/fs/proc/vmcore.c
>> @@ -1477,7 +1477,7 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
>>   {
>>   	struct vmcoredd_node *dump;
>>   	void *buf = NULL;
>> -	size_t data_size;
>> +	unsigned int data_size;
>>   	int ret;
> This was in reverse Christmas tree order before.  Move the data_size
> declaration up a line.
>
> 	long long_variable_name;
> 	medium variable_name;
> 	short name;
Got it,  and this 'usgined int' will be removed because of 'size_t' can
avoid overflow in some case.
>>   
>>   	if (vmcoredd_disabled) {
>> @@ -1490,10 +1490,8 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
>>   		return -EINVAL;
>>   
>>   	dump = vzalloc(sizeof(*dump));
>> -	if (!dump) {
>> -		ret = -ENOMEM;
>> -		goto out_err;
>> -	}
>> +	if (!dump)
>> +		return -ENOMEM;
>>   
>>   	/* Keep size of the buffer page aligned so that it can be mmaped */
>>   	data_size = roundup(sizeof(struct vmcoredd_header) + data->size,
>> @@ -1519,21 +1517,18 @@ int vmcore_add_device_dump(struct vmcoredd_data *data)
>>   	dump->size = data_size;
>>   
>>   	/* Add the dump to driver sysfs list and update the elfcore hdr */
>> -	mutex_lock(&vmcore_mutex);
>> -	if (vmcore_opened)
>> -		pr_warn_once("Unexpected adding of device dump\n");
>> -	if (vmcore_open) {
>> -		ret = -EBUSY;
>> -		goto unlock;
>> -	}
>> -
>> -	list_add_tail(&dump->list, &vmcoredd_list);
>> -	vmcoredd_update_size(data_size);
>> -	mutex_unlock(&vmcore_mutex);
>> -	return 0;
>> +	scoped_guard(mutex, &vmcore_mutex) {
>> +		if (vmcore_opened)
>> +			pr_warn_once("Unexpected adding of device dump\n");
>> +		if (vmcore_open) {
>> +			ret = -EBUSY;
>> +			goto out_err;
>> +		}
>>   
>> -unlock:
>> -	mutex_unlock(&vmcore_mutex);
>> +		list_add_tail(&dump->list, &vmcoredd_list);
>> +		vmcoredd_update_size(data_size);
>> +		return 0;
> Please, move this "return 0;" out of the scoped_guard().  Otherwise
> it's not obvious that we return zero on the success path.
Yes, it's better. Will update in v2 patch.
Thanks again!

Regards,
Su Hui