| 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
| ||
|
Message-ID: <20251112120644.GD17382@unreal>
Date: Wed, 12 Nov 2025 14:06:44 +0200
From: Leon Romanovsky <leon@...nel.org>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
Zhu Yanjun <zyjzyj2000@...il.com>, Jason Gunthorpe <jgg@...pe.ca>,
linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] RDMA/rxe: Avoid -Wflex-array-member-not-at-end
warnings
On Wed, Nov 12, 2025 at 06:50:16PM +0900, Gustavo A. R. Silva wrote:
>
>
> On 11/12/25 18:32, Leon Romanovsky wrote:
> > On Wed, Nov 12, 2025 at 05:49:05PM +0900, Gustavo A. R. Silva wrote:
> > >
> > >
> > > On 11/11/25 23:19, Leon Romanovsky wrote:
> > > > On Tue, Nov 11, 2025 at 09:14:05PM +0900, Gustavo A. R. Silva wrote:
> > > > >
> > > > >
> > > > > On 11/11/25 20:56, Leon Romanovsky wrote:
> > > > > > On Tue, Nov 11, 2025 at 12:35:02PM +0900, 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 new TRAILING_OVERLAP() helper to fix the following warning:
> > > > > > >
> > > > > > > 21 drivers/infiniband/sw/rxe/rxe_verbs.h:271:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> > > > > > >
> > > > > > > This helper creates a union between a flexible-array member (FAM) and a
> > > > > > > set of MEMBERS that would otherwise follow it.
> > > > > > >
> > > > > > > This overlays the trailing MEMBER struct ib_sge sge[RXE_MAX_SGE]; onto
> > > > > > > the FAM struct rxe_recv_wqe::dma.sge, while keeping the FAM and the
> > > > > > > start of MEMBER aligned.
> > > > > > >
> > > > > > > The static_assert() ensures this alignment remains, and it's
> > > > > > > intentionally placed inmediately after the related structure --no
> > > > > > > blank line in between.
> > > > > > >
> > > > > > > Lastly, move the conflicting declaration struct rxe_resp_info resp;
> > > > > > > to the end of the corresponding structure.
> > > > > > >
> > > > > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> > > > > > > ---
> > > > > > > drivers/infiniband/sw/rxe/rxe_verbs.h | 18 +++++++++++-------
> > > > > > > 1 file changed, 11 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > > index fd48075810dd..6498d61e8956 100644
> > > > > > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > > @@ -219,12 +219,6 @@ struct rxe_resp_info {
> > > > > > > u32 rkey;
> > > > > > > u32 length;
> > > > > > > - /* SRQ only */
> > > > > > > - struct {
> > > > > > > - struct rxe_recv_wqe wqe;
> > > > > > > - struct ib_sge sge[RXE_MAX_SGE];
> > > > > > > - } srq_wqe;
> > > > > > > -
> > > > > > > /* Responder resources. It's a circular list where the oldest
> > > > > > > * resource is dropped first.
> > > > > > > */
> > > > > > > @@ -232,7 +226,15 @@ struct rxe_resp_info {
> > > > > > > unsigned int res_head;
> > > > > > > unsigned int res_tail;
> > > > > > > struct resp_res *res;
> > > > > > > +
> > > > > > > + /* SRQ only */
> > > > > > > + /* Must be last as it ends in a flexible-array member. */
> > > > > > > + TRAILING_OVERLAP(struct rxe_recv_wqe, wqe, dma.sge,
> > > > > > > + struct ib_sge sge[RXE_MAX_SGE];
> > > > > > > + ) srq_wqe;
> > > > > >
> > > > > > Will this change be enough?
> > > > > >
> > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > index fd48075810dd..9ab11421a585 100644
> > > > > > --- a/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
> > > > > > @@ -219,12 +219,6 @@ struct rxe_resp_info {
> > > > > > u32 rkey;
> > > > > > u32 length;
> > > > > > - /* SRQ only */
> > > > > > - struct {
> > > > > > - struct rxe_recv_wqe wqe;
> > > > > > - struct ib_sge sge[RXE_MAX_SGE];
> > > > > > - } srq_wqe;
> > > > > > -
> > > > > > /* Responder resources. It's a circular list where the oldest
> > > > > > * resource is dropped first.
> > > > > > */
> > > > > > @@ -232,6 +226,12 @@ struct rxe_resp_info {
> > > > > > unsigned int res_head;
> > > > > > unsigned int res_tail;
> > > > > > struct resp_res *res;
> > > > > > +
> > > > > > + /* SRQ only */
> > > > > > + struct {
> > > > > > + struct ib_sge sge[RXE_MAX_SGE];
> > > > > > + struct rxe_recv_wqe wqe;
> > > > > > + } srq_wqe;
> > > > > > };
> > > > >
> > > > > The question is if this is really what you want?
> > > > >
> > > > > sge[RXE_MAX_SGE] is of the following type:
> > > > >
> > > > > struct ib_sge {
> > > > > u64 addr;
> > > > > u32 length;
> > > > > u32 lkey;
> > > > > };
> > > > >
> > > > > and struct rxe_recv_wqe::dma.sge[] is of type:
> > > > >
> > > > > struct rxe_sge {
> > > > > __aligned_u64 addr;
> > > > > __u32 length;
> > > > > __u32 lkey;
> > > > > };
> > > > >
> > > > > Both types are basically the same, and the original code looks
> > > > > pretty much like what people do when they want to pre-allocate
> > > > > a number of elements (of the same element type as the flex array)
> > > > > for a flexible-array member.
> > > > >
> > > > > Based on the above, the change you suggest seems a bit suspicious,
> > > > > and I'm not sure that's actually what you want?
> > > >
> > > > You wrote about this error: "warning: structure containing a flexible array
> > > > member is not at the end of another structure".
> > > >
> > > > My suggestion was simply to move that flex array to be the last element
> > > > and save us from the need to have some complex, magic macro in RXE.
> > >
> > > Yep, but as I commented above, that doesn't seem to be the right change.
> > >
> > > Look at the following couple of lines:
> > >
> > > drivers/infiniband/sw/rxe/rxe_resp.c-286- size = sizeof(*wqe) + wqe->dma.num_sge*sizeof(struct rxe_sge);
> > > drivers/infiniband/sw/rxe/rxe_resp.c-287- memcpy(&qp->resp.srq_wqe, wqe, size);
> > >
> > > Notice that line 286 is the open-coded arithmetic (struct_size(wqe,
> > > dma.sge, wqe->dma.num_sge) is preferred) to get the number of bytes
> > > to allocate for a flexible structure, in this case struct rxe_recv_wqe,
> > > and its flexible-array member, in this case struct rxe_recv_wqe::dma.sge[].
> > >
> > > So, `size` bytes are written in qp->resp.srq_wqe, and the reason this works
> > > seems to be because of the pre-allocation of RXE_MAX_SGE number of elements
> > > for flex array struct rxe_recv_wqe::dma.sge[] given by:
> > >
> > > struct {
> > > struct rxe_recv_wqe wqe;
> > > struct ib_sge sge[RXE_MAX_SGE];
> > > } srq_wqe;
> >
> > So you are saying that it works because it is written properly, so what
> > is the problem? Why do we need to fix properly working and written code
> > to be less readable?
>
> No one said the original code is not working as expected. The issue here is
> that the FAM is not at the end, and this causes a -Wflex-array-member-not-at-end
> warning. The change I propose places the FAM at the end, and the functionality
> remains exactly the same.
>
> You're probably not aware of the work we've been doing to enable
> -Wflex-array-member-not-at-end in mainline. If you're interested, below you
> can take a look at other similar changes I (and others) have been doing to
> complete this work:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=grep&q=-Wflex-array-member-not-at-end
I'm aware of that work, just trying to find a simplest possible solution
for this specific driver. RXE is a development driver which won't benefit
from this hardening work at all.
>
> >
> > >
> > > So, unless I'm missing something, struct ib_sge sge[RXE_MAX_SGE];
> > > should be aligned with struct rxe_recv_wqe wqe::dma.sge[].
> >
> > It is and moving to the end of struct will continue to keep it aligned.
>
> I think there is something you are missing here. The following pieces of
> code are no equivalent:
>
> struct {
> struct rxe_recv_wqe wqe;
> struct ib_sge sge[RXE_MAX_SGE];
> } srq_wqe;
>
> struct {
> struct ib_sge sge[RXE_MAX_SGE];
> struct rxe_recv_wqe wqe;
> } srq_wqe;
>
> What I'm understanding from your last couple of responses is that you think
> the above are equivalent. My previous response tried to explain why that is
> not the case.
Yes, I already forgot about change in srq_wqe.
Thanks
>
> >
> > >
> > > The TRAILING_OVERLAP() macro is also designed to ensure alignment in these
> > > cases (and the static_assert() to preserve it). See this thread:
> > >
> > > https://lore.kernel.org/linux-hardening/aLiYrQGdGmaDTtLF@kspp/
> > >
>
> Thanks
> -Gustavo
Powered by blists - more mailing lists