* [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings @ 2025-04-03 16:45 Gustavo A. R. Silva 2025-04-07 19:50 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Gustavo A. R. Silva @ 2025-04-03 16:45 UTC (permalink / raw) To: Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter Cc: dri-devel, nouveau, linux-kernel, Gustavo A. R. Silva, linux-hardening -Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. Use the `DEFINE_RAW_FLEX()` helper for a few on-stack definitions of a flexible structure where the size of the flexible-array member is known at compile-time, and refactor the rest of the code, accordingly. So, with these changes, fix the following warnings: drivers/gpu/drm/nouveau/nouveau_chan.c:274:37: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/gpu/drm/nouveau/nouveau_chan.c:371:46: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/gpu/drm/nouveau/nouveau_chan.c:524:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/gpu/drm/nouveau/nouveau_chan.c | 115 ++++++++++++------------- 1 file changed, 56 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c index cd659b9fd1d9..a7e70517b7cd 100644 --- a/drivers/gpu/drm/nouveau/nouveau_chan.c +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c @@ -270,10 +270,7 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, { NV03_CHANNEL_DMA , 0 }, {} }; - 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); + args->runlist = __ffs64(runm); + args->runq = 0; + args->priv = priv; + args->devm = BIT(0); if (hosts[cid].oclass < NV50_CHANNEL_GPFIFO) { - args.chan.vmm = 0; - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); - args.chan.offset = chan->push.addr; - args.chan.length = 0; + args->vmm = 0; + args->ctxdma = nvif_handle(&chan->push.ctxdma); + args->offset = chan->push.addr; + args->length = 0; } else { - args.chan.vmm = nvif_handle(&chan->vmm->vmm.object); + args->vmm = nvif_handle(&chan->vmm->vmm.object); if (hosts[cid].oclass < FERMI_CHANNEL_GPFIFO) - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); + args->ctxdma = nvif_handle(&chan->push.ctxdma); else - args.chan.ctxdma = 0; - args.chan.offset = ioffset + chan->push.addr; - args.chan.length = ilength; + args->ctxdma = 0; + args->offset = ioffset + chan->push.addr; + args->length = ilength; } - args.chan.huserd = 0; - args.chan.ouserd = 0; + args->huserd = 0; + args->ouserd = 0; /* allocate userd */ if (hosts[cid].oclass >= VOLTA_CHANNEL_GPFIFO_A) { @@ -329,27 +326,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, if (ret) return ret; - args.chan.huserd = nvif_handle(&chan->mem_userd.object); - args.chan.ouserd = 0; + args->huserd = nvif_handle(&chan->mem_userd.object); + args->ouserd = 0; chan->userd = &chan->mem_userd.object; } else { chan->userd = &chan->user; } - snprintf(args.name, sizeof(args.name), "%s[%d]", current->comm, task_pid_nr(current)); + snprintf(args->name, __struct_size(args) - sizeof(*args), "%s[%d]", + current->comm, task_pid_nr(current)); ret = nvif_object_ctor(&device->object, "abi16ChanUser", 0, hosts[cid].oclass, - &args, sizeof(args), &chan->user); + args, __struct_size(args), &chan->user); if (ret) { nouveau_channel_del(pchan); return ret; } - chan->runlist = args.chan.runlist; - chan->chid = args.chan.chid; - chan->inst = args.chan.inst; - chan->token = args.chan.token; + chan->runlist = args->runlist; + chan->chid = args->chid; + chan->inst = args->inst; + chan->token = args->token; return 0; } @@ -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), - .v.channels.mthd = NV_DEVICE_HOST_CHANNELS, - .v.runlists.mthd = NV_DEVICE_HOST_RUNLISTS, - }; + DEFINE_RAW_FLEX(struct nv_device_info_v1, args, data, 2); + struct nv_device_info_v1_data *channels = &args->data[0]; + struct nv_device_info_v1_data *runlists = &args->data[1]; struct nvif_object *device = &drm->client.device.object; int ret, i; - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); + args->version = 1; + args->count = (__struct_size(args) - sizeof(*args)) / + sizeof(*args->data); + channels->mthd = NV_DEVICE_HOST_CHANNELS; + runlists->mthd = NV_DEVICE_HOST_RUNLISTS; + + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, + __struct_size(args)); if (ret || - args.v.runlists.mthd == NV_DEVICE_INFO_INVALID || !args.v.runlists.data || - args.v.channels.mthd == NV_DEVICE_INFO_INVALID) + runlists->mthd == NV_DEVICE_INFO_INVALID || !runlists->data || + channels->mthd == NV_DEVICE_INFO_INVALID) return -ENODEV; - drm->chan_nr = drm->chan_total = args.v.channels.data; - drm->runl_nr = fls64(args.v.runlists.data); + drm->chan_nr = drm->chan_total = channels->data; + drm->runl_nr = fls64(runlists->data); drm->runl = kcalloc(drm->runl_nr, sizeof(*drm->runl), GFP_KERNEL); if (!drm->runl) return -ENOMEM; if (drm->chan_nr == 0) { for (i = 0; i < drm->runl_nr; i++) { - if (!(args.v.runlists.data & BIT(i))) + if (!(runlists->data & BIT(i))) continue; - args.v.channels.mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; - args.v.channels.data = i; + channels->mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; + channels->data = i; - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); - if (ret || args.v.channels.mthd == NV_DEVICE_INFO_INVALID) + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, + __struct_size(args)); + if (ret || channels->mthd == NV_DEVICE_INFO_INVALID) return -ENODEV; - drm->runl[i].chan_nr = args.v.channels.data; + drm->runl[i].chan_nr = channels->data; drm->runl[i].chan_id_base = drm->chan_total; drm->runl[i].context_base = dma_fence_context_alloc(drm->runl[i].chan_nr); -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-03 16:45 [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva @ 2025-04-07 19:50 ` Kees Cook 2025-04-07 19:57 ` Gustavo A. R. Silva 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2025-04-07 19:50 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening On Thu, Apr 03, 2025 at 10:45:18AM -0600, Gustavo A. R. Silva wrote: > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are > getting ready to enable it, globally. > > Use the `DEFINE_RAW_FLEX()` helper for a few on-stack definitions > of a flexible structure where the size of the flexible-array member > is known at compile-time, and refactor the rest of the code, > accordingly. > > So, with these changes, fix the following warnings: > > drivers/gpu/drm/nouveau/nouveau_chan.c:274:37: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/gpu/drm/nouveau/nouveau_chan.c:371:46: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/gpu/drm/nouveau/nouveau_chan.c:524:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_chan.c | 115 ++++++++++++------------- > 1 file changed, 56 insertions(+), 59 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c > index cd659b9fd1d9..a7e70517b7cd 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_chan.c > +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c > @@ -270,10 +270,7 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, > { NV03_CHANNEL_DMA , 0 }, > {} > }; > - 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)? > + args->runlist = __ffs64(runm); > + args->runq = 0; > + args->priv = priv; > + args->devm = BIT(0); > if (hosts[cid].oclass < NV50_CHANNEL_GPFIFO) { > - args.chan.vmm = 0; > - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); > - args.chan.offset = chan->push.addr; > - args.chan.length = 0; > + args->vmm = 0; > + args->ctxdma = nvif_handle(&chan->push.ctxdma); > + args->offset = chan->push.addr; > + args->length = 0; > } else { > - args.chan.vmm = nvif_handle(&chan->vmm->vmm.object); > + args->vmm = nvif_handle(&chan->vmm->vmm.object); > if (hosts[cid].oclass < FERMI_CHANNEL_GPFIFO) > - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); > + args->ctxdma = nvif_handle(&chan->push.ctxdma); > else > - args.chan.ctxdma = 0; > - args.chan.offset = ioffset + chan->push.addr; > - args.chan.length = ilength; > + args->ctxdma = 0; > + args->offset = ioffset + chan->push.addr; > + args->length = ilength; > } > - args.chan.huserd = 0; > - args.chan.ouserd = 0; > + args->huserd = 0; > + args->ouserd = 0; > > /* allocate userd */ > if (hosts[cid].oclass >= VOLTA_CHANNEL_GPFIFO_A) { > @@ -329,27 +326,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, > if (ret) > return ret; > > - args.chan.huserd = nvif_handle(&chan->mem_userd.object); > - args.chan.ouserd = 0; > + args->huserd = nvif_handle(&chan->mem_userd.object); > + args->ouserd = 0; > > chan->userd = &chan->mem_userd.object; > } else { > chan->userd = &chan->user; > } > > - snprintf(args.name, sizeof(args.name), "%s[%d]", current->comm, task_pid_nr(current)); > + snprintf(args->name, __struct_size(args) - sizeof(*args), "%s[%d]", > + current->comm, task_pid_nr(current)); > > ret = nvif_object_ctor(&device->object, "abi16ChanUser", 0, hosts[cid].oclass, > - &args, sizeof(args), &chan->user); > + args, __struct_size(args), &chan->user); > if (ret) { > nouveau_channel_del(pchan); > return ret; > } > > - chan->runlist = args.chan.runlist; > - chan->chid = args.chan.chid; > - chan->inst = args.chan.inst; > - chan->token = args.chan.token; > + chan->runlist = args->runlist; > + chan->chid = args->chid; > + chan->inst = args->inst; > + chan->token = args->token; > return 0; > } > > @@ -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? > - .v.channels.mthd = NV_DEVICE_HOST_CHANNELS, > - .v.runlists.mthd = NV_DEVICE_HOST_RUNLISTS, > - }; > + DEFINE_RAW_FLEX(struct nv_device_info_v1, args, data, 2); > + struct nv_device_info_v1_data *channels = &args->data[0]; > + struct nv_device_info_v1_data *runlists = &args->data[1]; > struct nvif_object *device = &drm->client.device.object; > int ret, i; > > - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); > + args->version = 1; > + args->count = (__struct_size(args) - sizeof(*args)) / > + sizeof(*args->data); > + channels->mthd = NV_DEVICE_HOST_CHANNELS; > + runlists->mthd = NV_DEVICE_HOST_RUNLISTS; > + > + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, > + __struct_size(args)); > if (ret || > - args.v.runlists.mthd == NV_DEVICE_INFO_INVALID || !args.v.runlists.data || > - args.v.channels.mthd == NV_DEVICE_INFO_INVALID) > + runlists->mthd == NV_DEVICE_INFO_INVALID || !runlists->data || > + channels->mthd == NV_DEVICE_INFO_INVALID) > return -ENODEV; > > - drm->chan_nr = drm->chan_total = args.v.channels.data; > - drm->runl_nr = fls64(args.v.runlists.data); > + drm->chan_nr = drm->chan_total = channels->data; > + drm->runl_nr = fls64(runlists->data); > drm->runl = kcalloc(drm->runl_nr, sizeof(*drm->runl), GFP_KERNEL); > if (!drm->runl) > return -ENOMEM; > > if (drm->chan_nr == 0) { > for (i = 0; i < drm->runl_nr; i++) { > - if (!(args.v.runlists.data & BIT(i))) > + if (!(runlists->data & BIT(i))) > continue; > > - args.v.channels.mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; > - args.v.channels.data = i; > + channels->mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; > + channels->data = i; > > - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); > - if (ret || args.v.channels.mthd == NV_DEVICE_INFO_INVALID) > + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, > + __struct_size(args)); > + if (ret || channels->mthd == NV_DEVICE_INFO_INVALID) > return -ENODEV; > > - drm->runl[i].chan_nr = args.v.channels.data; > + drm->runl[i].chan_nr = channels->data; > drm->runl[i].chan_id_base = drm->chan_total; > drm->runl[i].context_base = dma_fence_context_alloc(drm->runl[i].chan_nr); > Otherwise looks good. -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-07 19:50 ` Kees Cook @ 2025-04-07 19:57 ` Gustavo A. R. Silva 2025-04-07 20:39 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Gustavo A. R. Silva @ 2025-04-07 19:57 UTC (permalink / raw) To: Kees Cook, Gustavo A. R. Silva Cc: Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening On 07/04/25 13:50, Kees Cook wrote: > On Thu, Apr 03, 2025 at 10:45:18AM -0600, Gustavo A. R. Silva wrote: >> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are >> getting ready to enable it, globally. >> >> Use the `DEFINE_RAW_FLEX()` helper for a few on-stack definitions >> of a flexible structure where the size of the flexible-array member >> is known at compile-time, and refactor the rest of the code, >> accordingly. >> >> So, with these changes, fix the following warnings: >> >> drivers/gpu/drm/nouveau/nouveau_chan.c:274:37: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/gpu/drm/nouveau/nouveau_chan.c:371:46: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/gpu/drm/nouveau/nouveau_chan.c:524:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/gpu/drm/nouveau/nouveau_chan.c | 115 ++++++++++++------------- >> 1 file changed, 56 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c >> index cd659b9fd1d9..a7e70517b7cd 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c >> @@ -270,10 +270,7 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, >> { NV03_CHANNEL_DMA , 0 }, >> {} >> }; >> - 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? I mean, this should be equivalent to `TASK_COMM_LEN+16`, I could use the latter if people prefer it (see my comments below). > >> + args->runlist = __ffs64(runm); >> + args->runq = 0; >> + args->priv = priv; >> + args->devm = BIT(0); >> if (hosts[cid].oclass < NV50_CHANNEL_GPFIFO) { >> - args.chan.vmm = 0; >> - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); >> - args.chan.offset = chan->push.addr; >> - args.chan.length = 0; >> + args->vmm = 0; >> + args->ctxdma = nvif_handle(&chan->push.ctxdma); >> + args->offset = chan->push.addr; >> + args->length = 0; >> } else { >> - args.chan.vmm = nvif_handle(&chan->vmm->vmm.object); >> + args->vmm = nvif_handle(&chan->vmm->vmm.object); >> if (hosts[cid].oclass < FERMI_CHANNEL_GPFIFO) >> - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); >> + args->ctxdma = nvif_handle(&chan->push.ctxdma); >> else >> - args.chan.ctxdma = 0; >> - args.chan.offset = ioffset + chan->push.addr; >> - args.chan.length = ilength; >> + args->ctxdma = 0; >> + args->offset = ioffset + chan->push.addr; >> + args->length = ilength; >> } >> - args.chan.huserd = 0; >> - args.chan.ouserd = 0; >> + args->huserd = 0; >> + args->ouserd = 0; >> >> /* allocate userd */ >> if (hosts[cid].oclass >= VOLTA_CHANNEL_GPFIFO_A) { >> @@ -329,27 +326,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, >> if (ret) >> return ret; >> >> - args.chan.huserd = nvif_handle(&chan->mem_userd.object); >> - args.chan.ouserd = 0; >> + args->huserd = nvif_handle(&chan->mem_userd.object); >> + args->ouserd = 0; >> >> chan->userd = &chan->mem_userd.object; >> } else { >> chan->userd = &chan->user; >> } >> >> - snprintf(args.name, sizeof(args.name), "%s[%d]", current->comm, task_pid_nr(current)); >> + snprintf(args->name, __struct_size(args) - sizeof(*args), "%s[%d]", >> + current->comm, task_pid_nr(current)); >> >> ret = nvif_object_ctor(&device->object, "abi16ChanUser", 0, hosts[cid].oclass, >> - &args, sizeof(args), &chan->user); >> + args, __struct_size(args), &chan->user); >> if (ret) { >> nouveau_channel_del(pchan); >> return ret; >> } >> >> - chan->runlist = args.chan.runlist; >> - chan->chid = args.chan.chid; >> - chan->inst = args.chan.inst; >> - chan->token = args.chan.token; >> + chan->runlist = args->runlist; >> + chan->chid = args->chid; >> + chan->inst = args->inst; >> + chan->token = args->token; >> return 0; >> } >> >> @@ -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`. :) Thanks -- Gustavo > >> - .v.channels.mthd = NV_DEVICE_HOST_CHANNELS, >> - .v.runlists.mthd = NV_DEVICE_HOST_RUNLISTS, >> - }; >> + DEFINE_RAW_FLEX(struct nv_device_info_v1, args, data, 2); >> + struct nv_device_info_v1_data *channels = &args->data[0]; >> + struct nv_device_info_v1_data *runlists = &args->data[1]; >> struct nvif_object *device = &drm->client.device.object; >> int ret, i; >> >> - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); >> + args->version = 1; >> + args->count = (__struct_size(args) - sizeof(*args)) / >> + sizeof(*args->data); >> + channels->mthd = NV_DEVICE_HOST_CHANNELS; >> + runlists->mthd = NV_DEVICE_HOST_RUNLISTS; >> + >> + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, >> + __struct_size(args)); >> if (ret || >> - args.v.runlists.mthd == NV_DEVICE_INFO_INVALID || !args.v.runlists.data || >> - args.v.channels.mthd == NV_DEVICE_INFO_INVALID) >> + runlists->mthd == NV_DEVICE_INFO_INVALID || !runlists->data || >> + channels->mthd == NV_DEVICE_INFO_INVALID) >> return -ENODEV; >> >> - drm->chan_nr = drm->chan_total = args.v.channels.data; >> - drm->runl_nr = fls64(args.v.runlists.data); >> + drm->chan_nr = drm->chan_total = channels->data; >> + drm->runl_nr = fls64(runlists->data); >> drm->runl = kcalloc(drm->runl_nr, sizeof(*drm->runl), GFP_KERNEL); >> if (!drm->runl) >> return -ENOMEM; >> >> if (drm->chan_nr == 0) { >> for (i = 0; i < drm->runl_nr; i++) { >> - if (!(args.v.runlists.data & BIT(i))) >> + if (!(runlists->data & BIT(i))) >> continue; >> >> - args.v.channels.mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; >> - args.v.channels.data = i; >> + channels->mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; >> + channels->data = i; >> >> - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); >> - if (ret || args.v.channels.mthd == NV_DEVICE_INFO_INVALID) >> + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, >> + __struct_size(args)); >> + if (ret || channels->mthd == NV_DEVICE_INFO_INVALID) >> return -ENODEV; >> >> - drm->runl[i].chan_nr = args.v.channels.data; >> + drm->runl[i].chan_nr = channels->data; >> drm->runl[i].chan_id_base = drm->chan_total; >> drm->runl[i].context_base = dma_fence_context_alloc(drm->runl[i].chan_nr); >> > > Otherwise looks good. > > -Kees > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-07 19:57 ` Gustavo A. R. Silva @ 2025-04-07 20:39 ` Kees Cook 2025-04-07 23:35 ` Gustavo A. R. Silva 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2025-04-07 20:39 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Gustavo A. R. Silva, Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening On Mon, Apr 07, 2025 at 01:57:48PM -0600, Gustavo A. R. Silva wrote: > > > On 07/04/25 13:50, Kees Cook wrote: > > On Thu, Apr 03, 2025 at 10:45:18AM -0600, Gustavo A. R. Silva wrote: > > > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are > > > getting ready to enable it, globally. > > > > > > Use the `DEFINE_RAW_FLEX()` helper for a few on-stack definitions > > > of a flexible structure where the size of the flexible-array member > > > is known at compile-time, and refactor the rest of the code, > > > accordingly. > > > > > > So, with these changes, fix the following warnings: > > > > > > drivers/gpu/drm/nouveau/nouveau_chan.c:274:37: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > > drivers/gpu/drm/nouveau/nouveau_chan.c:371:46: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > > drivers/gpu/drm/nouveau/nouveau_chan.c:524:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > > > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > > --- > > > drivers/gpu/drm/nouveau/nouveau_chan.c | 115 ++++++++++++------------- > > > 1 file changed, 56 insertions(+), 59 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c > > > index cd659b9fd1d9..a7e70517b7cd 100644 > > > --- a/drivers/gpu/drm/nouveau/nouveau_chan.c > > > +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c > > > @@ -270,10 +270,7 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, > > > { NV03_CHANNEL_DMA , 0 }, > > > {} > > > }; > > > - 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. :) > > > > > > + args->runlist = __ffs64(runm); > > > + args->runq = 0; > > > + args->priv = priv; > > > + args->devm = BIT(0); > > > if (hosts[cid].oclass < NV50_CHANNEL_GPFIFO) { > > > - args.chan.vmm = 0; > > > - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); > > > - args.chan.offset = chan->push.addr; > > > - args.chan.length = 0; > > > + args->vmm = 0; > > > + args->ctxdma = nvif_handle(&chan->push.ctxdma); > > > + args->offset = chan->push.addr; > > > + args->length = 0; > > > } else { > > > - args.chan.vmm = nvif_handle(&chan->vmm->vmm.object); > > > + args->vmm = nvif_handle(&chan->vmm->vmm.object); > > > if (hosts[cid].oclass < FERMI_CHANNEL_GPFIFO) > > > - args.chan.ctxdma = nvif_handle(&chan->push.ctxdma); > > > + args->ctxdma = nvif_handle(&chan->push.ctxdma); > > > else > > > - args.chan.ctxdma = 0; > > > - args.chan.offset = ioffset + chan->push.addr; > > > - args.chan.length = ilength; > > > + args->ctxdma = 0; > > > + args->offset = ioffset + chan->push.addr; > > > + args->length = ilength; > > > } > > > - args.chan.huserd = 0; > > > - args.chan.ouserd = 0; > > > + args->huserd = 0; > > > + args->ouserd = 0; > > > /* allocate userd */ > > > if (hosts[cid].oclass >= VOLTA_CHANNEL_GPFIFO_A) { > > > @@ -329,27 +326,28 @@ nouveau_channel_ctor(struct nouveau_cli *cli, bool priv, u64 runm, > > > if (ret) > > > return ret; > > > - args.chan.huserd = nvif_handle(&chan->mem_userd.object); > > > - args.chan.ouserd = 0; > > > + args->huserd = nvif_handle(&chan->mem_userd.object); > > > + args->ouserd = 0; > > > chan->userd = &chan->mem_userd.object; > > > } else { > > > chan->userd = &chan->user; > > > } > > > - snprintf(args.name, sizeof(args.name), "%s[%d]", current->comm, task_pid_nr(current)); > > > + snprintf(args->name, __struct_size(args) - sizeof(*args), "%s[%d]", > > > + current->comm, task_pid_nr(current)); > > > ret = nvif_object_ctor(&device->object, "abi16ChanUser", 0, hosts[cid].oclass, > > > - &args, sizeof(args), &chan->user); > > > + args, __struct_size(args), &chan->user); > > > if (ret) { > > > nouveau_channel_del(pchan); > > > return ret; > > > } > > > - chan->runlist = args.chan.runlist; > > > - chan->chid = args.chan.chid; > > > - chan->inst = args.chan.inst; > > > - chan->token = args.chan.token; > > > + chan->runlist = args->runlist; > > > + chan->chid = args->chid; > > > + chan->inst = args->inst; > > > + chan->token = args->token; > > > return 0; > > > } > > > @@ -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"). > > Thanks > -- > Gustavo > > > > > > - .v.channels.mthd = NV_DEVICE_HOST_CHANNELS, > > > - .v.runlists.mthd = NV_DEVICE_HOST_RUNLISTS, > > > - }; > > > + DEFINE_RAW_FLEX(struct nv_device_info_v1, args, data, 2); > > > + struct nv_device_info_v1_data *channels = &args->data[0]; > > > + struct nv_device_info_v1_data *runlists = &args->data[1]; > > > struct nvif_object *device = &drm->client.device.object; > > > int ret, i; > > > - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); > > > + args->version = 1; > > > + args->count = (__struct_size(args) - sizeof(*args)) / > > > + sizeof(*args->data); > > > + channels->mthd = NV_DEVICE_HOST_CHANNELS; > > > + runlists->mthd = NV_DEVICE_HOST_RUNLISTS; > > > + > > > + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, > > > + __struct_size(args)); > > > if (ret || > > > - args.v.runlists.mthd == NV_DEVICE_INFO_INVALID || !args.v.runlists.data || > > > - args.v.channels.mthd == NV_DEVICE_INFO_INVALID) > > > + runlists->mthd == NV_DEVICE_INFO_INVALID || !runlists->data || > > > + channels->mthd == NV_DEVICE_INFO_INVALID) > > > return -ENODEV; > > > - drm->chan_nr = drm->chan_total = args.v.channels.data; > > > - drm->runl_nr = fls64(args.v.runlists.data); > > > + drm->chan_nr = drm->chan_total = channels->data; > > > + drm->runl_nr = fls64(runlists->data); > > > drm->runl = kcalloc(drm->runl_nr, sizeof(*drm->runl), GFP_KERNEL); > > > if (!drm->runl) > > > return -ENOMEM; > > > if (drm->chan_nr == 0) { > > > for (i = 0; i < drm->runl_nr; i++) { > > > - if (!(args.v.runlists.data & BIT(i))) > > > + if (!(runlists->data & BIT(i))) > > > continue; > > > - args.v.channels.mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; > > > - args.v.channels.data = i; > > > + channels->mthd = NV_DEVICE_HOST_RUNLIST_CHANNELS; > > > + channels->data = i; > > > - ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, &args, sizeof(args)); > > > - if (ret || args.v.channels.mthd == NV_DEVICE_INFO_INVALID) > > > + ret = nvif_object_mthd(device, NV_DEVICE_V0_INFO, args, > > > + __struct_size(args)); > > > + if (ret || channels->mthd == NV_DEVICE_INFO_INVALID) > > > return -ENODEV; > > > - drm->runl[i].chan_nr = args.v.channels.data; > > > + drm->runl[i].chan_nr = channels->data; > > > drm->runl[i].chan_id_base = drm->chan_total; > > > drm->runl[i].context_base = dma_fence_context_alloc(drm->runl[i].chan_nr); > > > > Otherwise looks good. > > > > -Kees > > > -- Kees Cook ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-07 20:39 ` Kees Cook @ 2025-04-07 23:35 ` Gustavo A. R. Silva 2025-04-08 23:40 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Gustavo A. R. Silva @ 2025-04-07 23:35 UTC (permalink / raw) To: Kees Cook Cc: Gustavo A. R. Silva, Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening [..] >>>> - 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-07 23:35 ` Gustavo A. R. Silva @ 2025-04-08 23:40 ` Kees Cook 2025-04-11 7:26 ` Gustavo A. R. Silva 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2025-04-08 23:40 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Gustavo A. R. Silva, Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening On Mon, Apr 07, 2025 at 05:35:47PM -0600, Gustavo A. R. Silva wrote: > [..] > > > > > > - 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. Ah-ha, yes, I'm already testing this with KUnit: struct bar { int a; u32 counter; s16 array[]; }; ... DEFINE_RAW_FLEX(struct bar, two, array, 2); ... KUNIT_EXPECT_EQ(test, sizeof(*two), sizeof(struct bar)); KUNIT_EXPECT_EQ(test, __struct_size(two), sizeof(struct bar) + 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __member_size(two), sizeof(struct bar) + 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __struct_size(two->array), 2 * sizeof(s16)); KUNIT_EXPECT_EQ(test, __member_size(two->array), 2 * sizeof(s16)); > I really don't want to condition -Wflex-array-member-not-at-end patches > on counted_by patches, for now. Fair enough. :) One thing at a time is wise! -- Kees Cook ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings 2025-04-08 23:40 ` Kees Cook @ 2025-04-11 7:26 ` Gustavo A. R. Silva 0 siblings, 0 replies; 7+ messages in thread From: Gustavo A. R. Silva @ 2025-04-11 7:26 UTC (permalink / raw) To: Kees Cook Cc: Gustavo A. R. Silva, Lyude Paul, Danilo Krummrich, David Airlie, Simona Vetter, dri-devel, nouveau, linux-kernel, linux-hardening On 08/04/25 17:40, Kees Cook wrote: > On Mon, Apr 07, 2025 at 05:35:47PM -0600, Gustavo A. R. Silva wrote: >> [..] >> >>>>>> - 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. > > Ah-ha, yes, I'm already testing this with KUnit: > > struct bar { > int a; > u32 counter; > s16 array[]; > }; > ... > DEFINE_RAW_FLEX(struct bar, two, array, 2); > ... > KUNIT_EXPECT_EQ(test, sizeof(*two), sizeof(struct bar)); > KUNIT_EXPECT_EQ(test, __struct_size(two), sizeof(struct bar) + 2 * sizeof(s16)); > KUNIT_EXPECT_EQ(test, __member_size(two), sizeof(struct bar) + 2 * sizeof(s16)); > KUNIT_EXPECT_EQ(test, __struct_size(two->array), 2 * sizeof(s16)); > KUNIT_EXPECT_EQ(test, __member_size(two->array), 2 * sizeof(s16)); Nice! I was taking a look at this, and now I want to use __member_size(p->array) instead of __struct_size(p->array). ^.^ > > >> I really don't want to condition -Wflex-array-member-not-at-end patches >> on counted_by patches, for now. > > Fair enough. :) One thing at a time is wise! > \o/ -- Gustavo ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-11 7:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-03 16:45 [PATCH][next] drm/nouveau: chan: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva 2025-04-07 19:50 ` Kees Cook 2025-04-07 19:57 ` Gustavo A. R. Silva 2025-04-07 20:39 ` Kees Cook 2025-04-07 23:35 ` Gustavo A. R. Silva 2025-04-08 23:40 ` Kees Cook 2025-04-11 7:26 ` Gustavo A. R. Silva
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.