lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <FR3P281MB175744F24BDD93870BB32ED0CE45A@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Fri, 27 Jun 2025 15:27:03 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Jonathan Cameron <jic23@...nel.org>
CC: David Lechner <dlechner@...libre.com>,
        Lars-Peter Clausen
	<lars@...afoo.de>,
        Nuno Sá <nuno.sa@...log.com>,
        Andy
 Shevchenko <andy@...nel.org>,
        "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers
 at the end

>
>________________________________________
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Thursday, June 26, 2025 20:48
>To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
>Cc: David Lechner <dlechner@...libre.com>; Lars-Peter Clausen <lars@...afoo.de>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
>Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Tue, 24 Jun 2025 08:43:37 +0000
>Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com> wrote:
>
>> >
>> >________________________________________
>> >From: David Lechner <dlechner@...libre.com>
>> >Sent: Tuesday, June 24, 2025 00:13
>> >To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
>> >Cc: Jonathan Cameron <jic23@...nel.org>; Lars-Peter Clausen <lars@...afoo.de>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
>> >Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
>> > 
>> >This Message Is From an External Sender
>> >This message came from outside your organization.
>> > 
>> >On Mon, Jun 23, 2025 at 6:56 AM Jean-Baptiste Maneyrol via B4 Relay
>> ><devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:  
>> >>
>> >> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>> >>
>> >> Move DMA buffers at the end of the structure to avoid overflow
>> >> bugs with unexpected effect.  
>> >
>> >If there is an overflow bug, we should fix that rather than hiding it.
>> >
>> >If I misunderstood the problem and timestamp and fifo should not be in
>> >the DMA aligned area and there is a problem with DMA cache writing
>> >over them, then I think we should reword the commit message.
>> >  
>> Hello David,
>> 
>> this change was asked by Jonathan so that potential DMA overflow would be more
>> easily caught by writing outside of the structure rather than writing inside
>> and do unexpected behavior.
>> 
>> >>
>> >> struct inv_icm42600_fifo has a DMA buffer at the end.
>> >>
>> >> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>> >> ---
>> >>  drivers/iio/imu/inv_icm42600/inv_icm42600.h | 8 ++++----
>> >>  1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> index 55ed1ddaa8cb5dd410d17db3866fa0f22f18e9d2..9b2cce172670c5513f18d5979a5ff563e9af4cb3 100644
>> >> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> @@ -148,9 +148,9 @@ struct inv_icm42600_suspended {
>> >>   *  @suspended:                suspended sensors configuration.
>> >>   *  @indio_gyro:       gyroscope IIO device.
>> >>   *  @indio_accel:      accelerometer IIO device.
>> >> - *  @buffer:           data transfer buffer aligned for DMA.
>> >> - *  @fifo:             FIFO management structure.
>> >>   *  @timestamp:                interrupt timestamps.
>> >> + *  @fifo:             FIFO management structure.
>> >> + *  @buffer:           data transfer buffer aligned for DMA.
>> >>   */
>> >>  struct inv_icm42600_state {
>> >>         struct mutex lock;
>> >> @@ -164,12 +164,12 @@ struct inv_icm42600_state {
>> >>         struct inv_icm42600_suspended suspended;
>> >>         struct iio_dev *indio_gyro;
>> >>         struct iio_dev *indio_accel;
>> >> -       u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >> -       struct inv_icm42600_fifo fifo;
>> >>         struct {
>> >>                 s64 gyro;
>> >>                 s64 accel;
>> >>         } timestamp;
>> >> +       struct inv_icm42600_fifo fifo;  
>> >
>> >I didn't look at how the drivers use timestamp and fifo, but if they
>> >are passed as a buffer to SPI, then they need to stay in the DMA
>> >aligned area of the struct.  
>> 
>> struct inv_icm42600_fifo has a buffer at its end that is passed to SPI.
>> Same things for buffer below. That's why both buffers are DMA
>> aligned.
>
>It's a tiny bit esoteric that this is relying on structure alignment rules
>that says (iirc) the structure element will be aligned to maximum of it's
>elements and there is tail padding to that size as well.  Thus the whole
>struct inv_icm42600 is __aligned(IIO_DMA_MINALIGN) and the buffer in there
>is itself after padding to ensure that it is __aligned(IIO_DMA_MINALIGN)
>
>
>Anyhow, all I think this actually does is avoid one lot of padding
>(as well as making it slightly easier to reason about!)
>
>outer struct {
>stuff
>padding to align #1
>fifo {
>	stuff
>	padding to align
>	buffer
>	padding to fill up align
>}
>struct timestamp;
>///this bit will go away as it can fit in the padding #1 (probably)
>Padding to align
>////
>u8 buffer[2] __ailgned(IIO_DMA_MINALIGN)
>
>

Hello David and Jonathan,

what should I changed for this patch? Rewrite the commit message?

Thanks for your feedback,
JB

>	
>> 
>> >  
>> >> +       u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >>  };
>> >>
>> >>
>> >>
>> >> --
>> >> 2.49.0
>> >>
>> >>  
>> >  
>> 
>> Thanks,
>> JB
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ