* [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
@ 2025-06-23 10:47 Su Hui
2025-06-23 14:36 ` Baoquan He
2025-06-23 15:06 ` Dan Carpenter
0 siblings, 2 replies; 6+ messages in thread
From: Su Hui @ 2025-06-23 10:47 UTC (permalink / raw)
To: akpm, bhe, vgoyal, dyoung
Cc: Su Hui, kexec, linux-kernel, linux-fsdevel, kernel-janitors
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
2025-06-23 10:47 [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump Su Hui
@ 2025-06-23 14:36 ` Baoquan He
2025-06-23 15:22 ` Dan Carpenter
2025-06-23 15:06 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Baoquan He @ 2025-06-23 14:36 UTC (permalink / raw)
To: Su Hui
Cc: akpm, vgoyal, dyoung, kexec, linux-kernel, linux-fsdevel,
kernel-janitors
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
2025-06-23 10:47 [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump Su Hui
2025-06-23 14:36 ` Baoquan He
@ 2025-06-23 15:06 ` Dan Carpenter
2025-06-24 2:20 ` Su Hui
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2025-06-23 15:06 UTC (permalink / raw)
To: Su Hui
Cc: akpm, bhe, vgoyal, dyoung, kexec, linux-kernel, linux-fsdevel,
kernel-janitors
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
2025-06-23 14:36 ` Baoquan He
@ 2025-06-23 15:22 ` Dan Carpenter
2025-06-24 2:14 ` Su Hui
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2025-06-23 15:22 UTC (permalink / raw)
To: Baoquan He
Cc: Su Hui, akpm, vgoyal, dyoung, kexec, linux-kernel, linux-fsdevel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
2025-06-23 15:22 ` Dan Carpenter
@ 2025-06-24 2:14 ` Su Hui
0 siblings, 0 replies; 6+ messages in thread
From: Su Hui @ 2025-06-24 2:14 UTC (permalink / raw)
To: Dan Carpenter, Baoquan He
Cc: akpm, vgoyal, dyoung, kexec, linux-kernel, linux-fsdevel,
kernel-janitors, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump
2025-06-23 15:06 ` Dan Carpenter
@ 2025-06-24 2:20 ` Su Hui
0 siblings, 0 replies; 6+ messages in thread
From: Su Hui @ 2025-06-24 2:20 UTC (permalink / raw)
To: Dan Carpenter
Cc: akpm, bhe, vgoyal, dyoung, kexec, linux-kernel, linux-fsdevel,
kernel-janitors, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-24 2:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23 10:47 [PATCH] fs/proc/vmcore: a few cleanups for vmcore_add_device_dump Su Hui
2025-06-23 14:36 ` Baoquan He
2025-06-23 15:22 ` Dan Carpenter
2025-06-24 2:14 ` Su Hui
2025-06-23 15:06 ` Dan Carpenter
2025-06-24 2:20 ` Su Hui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).