Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 14 Dec 2022 22:39:52 +0200 | Subject | Re: [PATCH] [next] pcmcia: synclink_cs: replace 1-element array with flex-array member |
| |
On Wed, Dec 14, 2022 at 10:09 PM Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: > On Wed, Dec 14, 2022 at 11:29:37AM -0800, Kees Cook wrote: > > On Wed, Dec 14, 2022 at 09:42:00PM +1300, Paulo Miguel Almeida wrote: > > > One-element arrays are deprecated, and we are replacing them with > > > flexible array members instead. So, replace one-element array with > > > flexible-array member in struct RXBUF and refactor the rest of the code > > > accordingly. > > > > > > It's worth mentioning that doing a build before/after this patch > > > results in no binary output differences. > > > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > > routines on memcpy() and help us make progress towards globally > > > enabling -fstrict-flex-arrays=3 [1].
...
> > > typedef struct { > > > int count; > > > unsigned char status; > > > - char data[1]; > > > + char data[]; > > > } RXBUF;
...
> As both of you had similar points, I will reply them here. > > The reasons why it had no binary changes was because of the combination > of this 2 things: > > 1) Existing padding - so sizeof(RXBUF) returned 8 bytes in both cases. > > pahole -C RXBUF gcc/before/drivers/char/pcmcia/synclink_cs.ko > typedef struct { > int count; /* 0 4 */ > unsigned char status; /* 4 1 */ > char data[1]; /* 5 1 */ > > /* size: 8, cachelines: 1, members: 3 */ > /* padding: 2 */ > /* last cacheline: 8 bytes */ > } RXBUF; > > pahole -C RXBUF gcc/after/drivers/char/pcmcia/synclink_cs.ko > typedef struct { > int count; /* 0 4 */ > unsigned char status; /* 4 1 */ > char data[]; /* 5 0 */ > > /* size: 8, cachelines: 1, members: 3 */ > /* padding: 3 */ > /* last cacheline: 8 bytes */ > } RXBUF;
Yes, and Try to make it work with __packed. As I said, the problem is that the code is relying on something which is architecture dependent strictly speaking. And hence I disagree with Kees that v2 is okay to go.
> 2) RXBUF (as implemented now) is just like a pair of lenses from which a > developer can have access to one of the circular buffers in MGSLPC_INFO > struct called 'rx_buf'.
> 2611 static int rx_alloc_buffers(MGSLPC_INFO *info) > 2612 { > 2613 /* each buffer has header and data */ > 2614 info->rx_buf_size = sizeof(RXBUF) + info->max_frame_size; > 2615 > 2616 /* calculate total allocation size for 8 buffers */ > 2617 info->rx_buf_total_size = info->rx_buf_size * 8; > 2618 > 2619 /* limit total allocated memory */ > 2620 if (info->rx_buf_total_size > 0x10000) > 2621 info->rx_buf_total_size = 0x10000; > 2622 > 2623 /* calculate number of buffers */ > 2624 info->rx_buf_count = info->rx_buf_total_size / info->rx_buf_size; > 2625 > 2626 info->rx_buf = kmalloc(info->rx_buf_total_size, GFP_KERNEL); > > To be honest, char data[_1_] in RXBUF was never required to be there. > The code base seems to make sure that it doesn't run past its limits by > keeping track of size buffer on MGSLPC_INFO->rx_buf_size (and sometimes > RXBUF->count) > > (Addressing one point made by Andy about using of of the macros in > overflow.h) > struct_size(buf, data, 1) would return 9 bytes which could > potentially break the existing driver as it produces binary > changes.
You got it incorrectly. I believe you should use something different than 1. In previous lines in the function it multiplies sizeof + max_frame_size by 8.
The full change should be something like
check_add(sizeof(), max_frame_size) kcalloc(8, size)
Think about it.
> Let me know your thoughts
-- With Best Regards, Andy Shevchenko
| |