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

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

[Su Hui]

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@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;
 
 	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;
+	}
 
 out_err:
 	vfree(buf);
-- 
2.30.2

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

[Baoquan He]

On 06/23/25 at 06:47pm, 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.

It's unclear to me why size_t is not suggested here. Isn't it assigned
a 'sizeof() + data->size' in which size_t should be used?

The rest two looks good to me, thanks.

> 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@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;
>  
>  	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;
> +	}
>  
>  out_err:
>  	vfree(buf);
> -- 
> 2.30.2
> 

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

[Dan Carpenter]

On Mon, Jun 23, 2025 at 10:36:45PM +0800, Baoquan He wrote:
> On 06/23/25 at 06:47pm, 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.
> 
> It's unclear to me why size_t is not suggested here. Isn't it assigned
> a 'sizeof() + data->size' in which size_t should be used?

Yeah...  That's a good point.  People should generally default to size_t
for sizes.  It really does prevent a lot of integer overflow bugs.  In
this case data->size is not controlled by the user, but if it were
then that would be an integer overflow on 32bit systems and not on
64bit systems, until we start declaring sizes as unsigned int and
then all the 32bit bugs start affecting everyone.

regards,
dan carpenter

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

[Su Hui]

On 2025/6/23 23:22, Dan Carpenter wrote:
> On Mon, Jun 23, 2025 at 10:36:45PM +0800, Baoquan He wrote:
>> On 06/23/25 at 06:47pm, 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.
>> It's unclear to me why size_t is not suggested here. Isn't it assigned
>> a 'sizeof() + data->size' in which size_t should be used?

Oh, sorry for this, I missed some things.

1497         data_size = roundup(sizeof(struct vmcoredd_header) + 
data->size,
1498                             PAGE_SIZE);
1499
1500         /* Allocate buffer for driver's to write their dumps */
1501         buf = vmcore_alloc_buf(data_size);
             [...]
1515
1516         dump->buf = buf;
1517         dump->size = data_size;
                  ^^^^^^^^^^^^^^^^^^^^^
If data_size is 64 bit and assume data_size is bigger than 32bit, 
dump->size will overflow.
Should we adjust dump->size's type to size_t? Or maybe it's impossible 
for data_size bigger
than 32bit?
> Yeah...  That's a good point.  People should generally default to size_t
> for sizes.  It really does prevent a lot of integer overflow bugs.  In
> this case data->size is not controlled by the user, but if it were
> then that would be an integer overflow on 32bit systems and not on
> 64bit systems, until we start declaring sizes as unsigned int and
> then all the 32bit bugs start affecting everyone.
Agreed, sorry for my fault again.
I will remove the 'unsigned int' in v2 patch.
Thanks for your suggestions!

regards,
Su Hui

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

[Dan Carpenter]

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@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;

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

regards,
dan carpenter

> +	}
>  
>  out_err:
>  	vfree(buf);
> -- 
> 2.30.2
> 

Re: [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@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