| Message ID | Y6Eu2604cqtryP4g@mail.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] i915/gvt: Replace one-element array with flexible-array member | expand |
On 2022.12.20 16:41:15 +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 gvt_firmware_header and refactor the > rest of the code accordingly. > > Additionally, previous implementation was allocating 8 bytes more than > required to represent firmware_header + cfg_space data + mmio data. > > 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]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > To make reviewing this patch easier, I'm pasting before/after struct > sizes. > > pahole -C gvt_firmware_header before/drivers/gpu/drm/i915/gvt/firmware.o > struct gvt_firmware_header { > u64 magic; /* 0 8 */ > u32 crc32; /* 8 4 */ > u32 version; /* 12 4 */ > u64 cfg_space_size; /* 16 8 */ > u64 cfg_space_offset; /* 24 8 */ > u64 mmio_size; /* 32 8 */ > u64 mmio_offset; /* 40 8 */ > unsigned char data[1]; /* 48 1 */ > > /* size: 56, cachelines: 1, members: 8 */ > /* padding: 7 */ > /* last cacheline: 56 bytes */ > }; > > pahole -C gvt_firmware_header after/drivers/gpu/drm/i915/gvt/firmware.o > struct gvt_firmware_header { > u64 magic; /* 0 8 */ > u32 crc32; /* 8 4 */ > u32 version; /* 12 4 */ > u64 cfg_space_size; /* 16 8 */ > u64 cfg_space_offset; /* 24 8 */ > u64 mmio_size; /* 32 8 */ > u64 mmio_offset; /* 40 8 */ > unsigned char data[]; /* 48 0 */ > > /* size: 48, cachelines: 1, members: 8 */ > /* last cacheline: 48 bytes */ > }; > > As you can see the additional byte of the fake-flexible array (data[1]) > forced the compiler to pad the struct but those bytes aren't actually used > as first & last bytes (of both cfg_space and mmio) are controlled by the > <>_size and <>_offset members present in the gvt_firmware_header struct. > --- > drivers/gpu/drm/i915/gvt/firmware.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c > index a683c22d5b64..dce93738e98a 100644 > --- a/drivers/gpu/drm/i915/gvt/firmware.c > +++ b/drivers/gpu/drm/i915/gvt/firmware.c > @@ -45,7 +45,7 @@ struct gvt_firmware_header { > u64 cfg_space_offset; /* offset in the file */ > u64 mmio_size; > u64 mmio_offset; /* offset in the file */ > - unsigned char data[1]; > + unsigned char data[]; > }; > > #define dev_to_drm_minor(d) dev_get_drvdata((d)) > @@ -77,7 +77,7 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt) > unsigned long size, crc32_start; > int ret; > > - size = sizeof(*h) + info->mmio_size + info->cfg_space_size; > + size = offsetof(struct gvt_firmware_header, data) + info->mmio_size + info->cfg_space_size; > firmware = vzalloc(size); > if (!firmware) > return -ENOMEM; > -- Looks good to me. Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com> Thanks!
diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c index a683c22d5b64..dce93738e98a 100644 --- a/drivers/gpu/drm/i915/gvt/firmware.c +++ b/drivers/gpu/drm/i915/gvt/firmware.c @@ -45,7 +45,7 @@ struct gvt_firmware_header { u64 cfg_space_offset; /* offset in the file */ u64 mmio_size; u64 mmio_offset; /* offset in the file */ - unsigned char data[1]; + unsigned char data[]; }; #define dev_to_drm_minor(d) dev_get_drvdata((d)) @@ -77,7 +77,7 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt) unsigned long size, crc32_start; int ret; - size = sizeof(*h) + info->mmio_size + info->cfg_space_size; + size = offsetof(struct gvt_firmware_header, data) + info->mmio_size + info->cfg_space_size; firmware = vzalloc(size); if (!firmware) return -ENOMEM;
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 gvt_firmware_header and refactor the rest of the code accordingly. Additionally, previous implementation was allocating 8 bytes more than required to represent firmware_header + cfg_space data + mmio data. 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]. Link: https://github.com/KSPP/linux/issues/79 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> --- To make reviewing this patch easier, I'm pasting before/after struct sizes. pahole -C gvt_firmware_header before/drivers/gpu/drm/i915/gvt/firmware.o struct gvt_firmware_header { u64 magic; /* 0 8 */ u32 crc32; /* 8 4 */ u32 version; /* 12 4 */ u64 cfg_space_size; /* 16 8 */ u64 cfg_space_offset; /* 24 8 */ u64 mmio_size; /* 32 8 */ u64 mmio_offset; /* 40 8 */ unsigned char data[1]; /* 48 1 */ /* size: 56, cachelines: 1, members: 8 */ /* padding: 7 */ /* last cacheline: 56 bytes */ }; pahole -C gvt_firmware_header after/drivers/gpu/drm/i915/gvt/firmware.o struct gvt_firmware_header { u64 magic; /* 0 8 */ u32 crc32; /* 8 4 */ u32 version; /* 12 4 */ u64 cfg_space_size; /* 16 8 */ u64 cfg_space_offset; /* 24 8 */ u64 mmio_size; /* 32 8 */ u64 mmio_offset; /* 40 8 */ unsigned char data[]; /* 48 0 */ /* size: 48, cachelines: 1, members: 8 */ /* last cacheline: 48 bytes */ }; As you can see the additional byte of the fake-flexible array (data[1]) forced the compiler to pad the struct but those bytes aren't actually used as first & last bytes (of both cfg_space and mmio) are controlled by the <>_size and <>_offset members present in the gvt_firmware_header struct. --- drivers/gpu/drm/i915/gvt/firmware.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)