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