linux-kernel - Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings

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: <a6dccb66-3f97-443f-85e5-fe089cd93a5e@embeddedor.com>
Date: Mon, 7 Apr 2025 17:35:47 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Kees Cook <kees@...nel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Lyude Paul <lyude@...hat.com>, Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] drm/nouveau: chan: Avoid
 -Wflex-array-member-not-at-end warnings

[..]

>>>> -	struct {
>>>> -		struct nvif_chan_v0 chan;
>>>> -		char name[TASK_COMM_LEN+16];
>>>> -	} args;
>>>> +	DEFINE_RAW_FLEX(struct nvif_chan_v0, args, name, TASK_COMM_LEN + 16);
>>>>    	struct nvif_device *device = &cli->device;
>>>>    	struct nouveau_channel *chan;
>>>>    	const u64 plength = 0x10000;
>>>> @@ -298,28 +295,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm,
>>>>    		return ret;
>>>>    	/* create channel object */
>>>> -	args.chan.version = 0;
>>>> -	args.chan.namelen = sizeof(args.name);
>>>> -	args.chan.runlist = __ffs64(runm);
>>>> -	args.chan.runq = 0;
>>>> -	args.chan.priv = priv;
>>>> -	args.chan.devm = BIT(0);
>>>> +	args->version = 0;
>>>> +	args->namelen = __struct_size(args) - sizeof(*args);
>>>
>>> Does __struct_size(args->name) work here (and later)?
>>
>> Why not?
> 
> Uhm, I'm genuinely curious. I *think* it will work, but because it's
> within the struct, not outside of it, I'm unclear if it'll DTRT for
> finding the size (since __builtin_object_size() can be touchy).
> 
>> I mean, this should be equivalent to `TASK_COMM_LEN+16`, I could
>> use the latter if people prefer it (see my comments below).
> 
> Right, it should be the same. I think __struct_size(args->name) would be
> much more readable ... if it works. :)

OK, I'll double check this.

[..]

>>>> @@ -367,17 +365,17 @@ nouveau_channel_init(struct nouveau_channel *chan, u32 vram, u32 gart)
>>>>    		return ret;
>>>>    	if (chan->user.oclass >= FERMI_CHANNEL_GPFIFO) {
>>>> -		struct {
>>>> -			struct nvif_event_v0 base;
>>>> -			struct nvif_chan_event_v0 host;
>>>> -		} args;
>>>> +		DEFINE_RAW_FLEX(struct nvif_event_v0, args, data,
>>>> +				sizeof(struct nvif_chan_event_v0));
>>>> +		struct nvif_chan_event_v0 *host =
>>>> +				(struct nvif_chan_event_v0 *)args->data;
>>>> -		args.host.version = 0;
>>>> -		args.host.type = NVIF_CHAN_EVENT_V0_KILLED;
>>>> +		host->version = 0;
>>>> +		host->type = NVIF_CHAN_EVENT_V0_KILLED;
>>>>    		ret = nvif_event_ctor(&chan->user, "abi16ChanKilled", chan->chid,
>>>>    				      nouveau_channel_killed, false,
>>>> -				      &args.base, sizeof(args), &chan->kill);
>>>> +				      args, __struct_size(args), &chan->kill);
>>>>    		if (ret == 0)
>>>>    			ret = nvif_event_allow(&chan->kill);
>>>>    		if (ret) {
>>>> @@ -520,46 +518,45 @@ nouveau_channels_fini(struct nouveau_drm *drm)
>>>>    int
>>>>    nouveau_channels_init(struct nouveau_drm *drm)
>>>>    {
>>>> -	struct {
>>>> -		struct nv_device_info_v1 m;
>>>> -		struct {
>>>> -			struct nv_device_info_v1_data channels;
>>>> -			struct nv_device_info_v1_data runlists;
>>>> -		} v;
>>>> -	} args = {
>>>> -		.m.version = 1,
>>>> -		.m.count = sizeof(args.v) / sizeof(args.v.channels),
>>>
>>> sizeof(args.v) == sizeof(struct nv_device_info_v1_data) * 2
>>>
>>> and sizeof(args.v.channels) == sizeof(struct nv_device_info_v1_data).
>>>
>>> Isn't this just "2"? i.e. isn't struct nv_device_info_v1::count the
>>> counted_by for struct nv_device_info_v1::data?
>>
>> Yes, it's just `2`. However, I didn't want to explicitly use the magic
>> number, in case people don't like it, as in other similar patches (in
>> other subsystems).
>>
>> But, yeah, it's `2`. :)
> 
> Okay. So if "count" is set up as a counted_by, the assignment will
> happen automatically (in DEFINE_FLEX -- no longer "RAW").

I really don't want to condition -Wflex-array-member-not-at-end patches
on counted_by patches, for now.

Thanks
--
Gustavo