Message-ID: <58d77d45-d052-4431-91de-3912a9c675b5@icloud.com>
Date: Sun, 10 Nov 2024 05:10:53 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Matteo Martelli <matteomartelli3@...il.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Jonathan Cameron <jic23@...nel.org>, Joe Perches <joe@...ches.com>,
 Jens Axboe <axboe@...nel.dk>, Peter Zijlstra <peterz@...radead.org>
Cc: Marc Gonzalez <marc.w.gonzalez@...e.fr>, Peter Rosin <peda@...ntia.se>,
 "Rafael J. Wysocki" <rafael@...nel.org>, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: iio, syfs, devres: devm_kmalloc not aligned to pow2 size argument

On 2024/11/8 17:04, Matteo Martelli wrote:
> On Mon, 28 Oct 2024 13:04:10 +0100, matteomartelli3@...il.com wrote:
>> Hi everyone,
>>
>> I found an issue that might interest iio, sysfs and devres, about a
>> particular usage of devm_kmalloc() for buffers that later pass through
>> sysfs_emit() or sysfs_emit_at(). These sysfs helpers require the output
>> buffer to be PAGE_SIZE aligned since commit 2efc459d06f1 ("sysfs: Add
>> sysfs_emit and sysfs_emit_at to format sysfs output"). Such requirement
>> is satisfied when kmalloc(PAGE_SIZE, ...) is used but not when
>> devm_kmalloc(PAGE_SIZE,...) is used as it actually returns a pointer to
>> a buffer located after the devres metadata and thus aligned to
>> PAGE_SIZE+sizeof(struct devres).
>>
>> Specifically, I came across this issue during some testing of the
>> pac1921 iio driver together with the iio-mux iio consumer driver, which
>> allocates a page sized buffer to copy the ext_info of the producer
>> pac1921 iio producer driver. To fill the buffer, the latter calls
>> iio_format_value(), and so sysfs_emit_at() which fails due to the buffer
>> not being page aligned. This pattern seems common for many iio drivers
>> which fill the ext_info attributes through sysfs_emit*() helpers, likely
>> necessary as they are exposed on sysfs.
>>
>> I could reproduce the same error behavior with a minimal dummy char
>> device driver completely unrelated to iio. I will share the entire dummy
>> driver code if needed but essentially this is the only interesting part:
>>
>> 	data->info_buf = devm_kzalloc(data->dev, PAGE_SIZE, GFP_KERNEL);
>> 	if (!data->info_buf)
>> 		return -ENOMEM;
>>
>> 	if (offset_in_page(data->info_buf))
>> 		pr_err("dummy_test: buf not page algined\n");
>>
>> When running this, the error message is printed out for the reason above.
>>
>> I am not sure whether this should be addressed in the users of
>> devm_kmalloc() or in the devres implementation itself. I would say that
>> it would be more clear if devm_kmalloc() would return the pointer to the
>> size aligned buffer, as it would also comply to the following kmalloc
>> requirement (introduced in [1]):
>>
>> The address of a chunk allocated with `kmalloc` is aligned to at least
>> ARCH_KMALLOC_MINALIGN bytes. For sizes of power of two bytes, the
>> alignment is also guaranteed to be at least to the respective size.
>>
>> To do so I was thinking to try to move the devres metadata after the
>> data buffer, so that the latter would directly correspond to pointer
>> returned by kmalloc. I then found out that it had been already suggested
>> previously to address a memory optimization [2]. Thus I am reporting the
>> issue before submitting any patch as some discussions might be helpful
>> first.
>>

no, IMO, that is not good idea absolutely.

>> I am sending this to who I think might be interested based on previous
>> related activity. Feel free to extend the cc list if needed.
> 
> Adding some more context to better understand the impact of this.
> 
> With a trivial grep it looks like there are only few instances where
> devm_k*alloc() is used to allocate a PAGE_SIZE buffer:
> 
> $ git grep -n 'devm_.*alloc.*(.*PAGE_SIZE'
> block/badblocks.c:1584:         bb->page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> drivers/iio/multiplexer/iio-mux.c:287:          page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> drivers/mtd/nand/raw/mxc_nand.c:1702:   host->data_buf = devm_kzalloc(&pdev->dev, PAGE_SIZE, GFP_KERNEL);
> drivers/usb/gadget/udc/gr_udc.c:1987:           buf = devm_kzalloc(dev->dev, PAGE_SIZE, GFP_DMA | GFP_ATOMIC);
> sound/soc/sof/debug.c:277:              dfse->buf = devm_kmalloc(sdev->dev, PAGE_SIZE, GFP_KERNEL);
> 
> What takes my attention is the bb->page in blocks/badblocks.c, being the
> buffer named "page" maybe it is supposed to be page aligned?
> 
> Also in [3] it was suggested to add the page alignment check for
> sysfs_emit() and sysfs_emit_at(), but I haven't found why that's
> necessary. My guess is for optimizations to avoid the buffer to spread
> in more than one page. Is this correct? Are there other reasons? Can
> anyone add more details? I think it would help to understand whether
> page alignment is necessary in the other instances of devm_k*alloc().
> 
> Beside page alignment, there are plenty of devm_k*alloc() around the
> code base, is there any way to spot whether any of those instances
> expect the allocated buffer to be aligned to the provided size?
> 
> If this is a limited use-case it can be worked around with just regular
> k*alloc() + devm_add_action_or_reset() as Jonathan suggested. However, I
> still think it can be easy to introduce some alignment related bug,
> especially when transitioning from k*alloc() to devm_k*alloc() in an old
> implementation since it can be assumed that they have the same alignment
> guarantees. Maybe some comment in the devres APIs or documentation would
> help in this case?
> 
> Any thoughts?
>

why not to use existing APIs?

addr = devm_get_free_pages(dev, GFP_KERNEL|__GFP_ZERO, 0);
devm_free_pages(dev,addr);

>>
>> [1]: https://lore.kernel.org/all/20190826111627.7505-3-vbabka@suse.cz/
>> [2]: https://lore.kernel.org/all/20191220140655.GN2827@hirez.programming.kicks-ass.net/
> 
> [3]: https://lore.kernel.org/all/743a648dc817cddd2e7046283c868f1c08742f29.camel@perches.com/
> 
> Best regards,
> Matteo Martelli
> 

Powered by blists - more mailing lists