* [PATCH] [next] i915/gvt: Replace one-element array with flexible-array member
@ 2022-12-20 3:41 Paulo Miguel Almeida
2022-12-20 8:29 ` Zhenyu Wang
0 siblings, 1 reply; 2+ messages in thread
From: Paulo Miguel Almeida @ 2022-12-20 3:41 UTC (permalink / raw)
To: Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
intel-gvt-dev, intel-gfx, dri-devel
Cc: linux-kernel, linux-hardening, paulo.miguel.almeida.rodenas
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;
--
2.38.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] [next] i915/gvt: Replace one-element array with flexible-array member
2022-12-20 3:41 [PATCH] [next] i915/gvt: Replace one-element array with flexible-array member Paulo Miguel Almeida
@ 2022-12-20 8:29 ` Zhenyu Wang
0 siblings, 0 replies; 2+ messages in thread
From: Zhenyu Wang @ 2022-12-20 8:29 UTC (permalink / raw)
To: Paulo Miguel Almeida
Cc: Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Daniel Vetter, intel-gvt-dev,
intel-gfx, dri-devel, linux-kernel, linux-hardening
[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]
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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-12-20 8:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-20 3:41 [PATCH] [next] i915/gvt: Replace one-element array with flexible-array member Paulo Miguel Almeida
2022-12-20 8:29 ` Zhenyu Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).