Re: [PATCH] topology: Keep the cpumask unchanged when printing cpumap

From: Li Huafei
Date: Wed Nov 13 2024 - 21:48:06 EST




On 2024/11/13 21:50, Greg KH wrote:
> On Thu, Nov 14, 2024 at 12:59:00AM +0800, Li Huafei wrote:
>> During fuzz testing, the following warning was discovered:
>>
>> different return values (15 and 11) from vsnprintf("%*pbl
>> ", ...)
>>
>> test:keyward is WARNING in kvasprintf
>> WARNING: CPU: 55 PID: 1168477 at lib/kasprintf.c:30 kvasprintf+0x121/0x130
>> Call Trace:
>> kvasprintf+0x121/0x130
>> kasprintf+0xa6/0xe0
>> bitmap_print_to_buf+0x89/0x100
>> core_siblings_list_read+0x7e/0xb0
>> kernfs_file_read_iter+0x15b/0x270
>> new_sync_read+0x153/0x260
>> vfs_read+0x215/0x290
>> ksys_read+0xb9/0x160
>> do_syscall_64+0x56/0x100
>> entry_SYSCALL_64_after_hwframe+0x78/0xe2
>
> What is happening when this fuzzing is going on? Removing/adding cpus?
>

Yes, logs of CPU offline also appeared before the warning.


>> The call trace shows that kvasprintf() reported this warning during the
>> printing of core_siblings_list. kvasprintf() has several steps:
>>
>> (1) First, calculate the length of the resulting formatted string.
>>
>> (2) Allocate a buffer based on the returned length.
>>
>> (3) Then, perform the actual string formatting.
>>
>> (4) Check whether the lengths of the formatted strings returned in
>> steps (1) and (2) are consistent.
>>
>> If the core_cpumask is modified between steps (1) and (3), the lengths
>> obtained in these two steps may not match. Indeed our test includes cpu
>> hotplugging, which should modify core_cpumask while printing.
>
> Ah, yes. Good catch.
>
>> To fix this issue, cache the cpumask into a temporary variable before
>> calling cpumap_print_{list, cpumask}_to_buf(), to keep it unchanged
>> during the printing process.
>
> Nice, but:
>
>>
>> Fixes: bb9ec13d156e ("topology: use bin_attribute to break the size limitation of cpumap ABI")
>
> No cc: stable?

Thanks for the reminder, v2 will be cc'd to stable.

>
>> Signed-off-by: Li Huafei <lihuafei1@xxxxxxxxxx>
>> ---
>> drivers/base/topology.c | 24 ++++++++++++++++++++----
>> 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/base/topology.c b/drivers/base/topology.c
>> index 89f98be5c5b9..70dbd7ef038d 100644
>> --- a/drivers/base/topology.c
>> +++ b/drivers/base/topology.c
>> @@ -27,9 +27,17 @@ static ssize_t name##_read(struct file *file, struct kobject *kobj, \
>> loff_t off, size_t count) \
>> { \
>> struct device *dev = kobj_to_dev(kobj); \
>> + cpumask_var_t mask; \
>> + ssize_t n; \
>> \
>> - return cpumap_print_bitmask_to_buf(buf, topology_##mask(dev->id), \
>> - off, count); \
>> + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) \
>> + return 0; \
>
> If this fails, return the error please. Don't return a size of 0, which
> will just confuse userspace as to why the read succeeded yet did not
> return any data?
>

Okay, v2 will change to return -ENOMEM, and the same applies to the
following places.

Thanks,
Huafei

>> + \
>> + cpumask_copy(mask, topology_##mask(dev->id)); \
>> + n = cpumap_print_bitmask_to_buf(buf, mask, off, count); \
>> + free_cpumask_var(mask); \
>> + \
>> + return n; \
>> } \
>> \
>> static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
>> @@ -37,9 +45,17 @@ static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
>> loff_t off, size_t count) \
>> { \
>> struct device *dev = kobj_to_dev(kobj); \
>> + cpumask_var_t mask; \
>> + ssize_t n; \
>> + \
>> + if (!alloc_cpumask_var(&mask, GFP_KERNEL)) \
>> + return 0; \
>
> Same here, please return the error.
>
> thanks,
>
> greg k-h
> .
>