On Wed, Feb 19, 2025 at 12:49 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> On 2/19/25 12:46 PM, Anna Schumaker wrote:
> > Hi Chuck,
> >
> > On Tue, Feb 18, 2025 at 1:10 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >>
> >> On 2/10/25 2:48 PM, Chuck Lever wrote:
> >>> On 2/3/25 10:03 PM, 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.
> >>>>
> >>>> So, in order to avoid ending up with a flexible-array member in the
> >>>> middle of other structs, we use the `struct_group_tagged()` helper
> >>>> to create a new tagged `struct posix_acl_hdr`. This structure
> >>>> groups together all the members of the flexible `struct posix_acl`
> >>>> except the flexible array.
> >>>>
> >>>> As a result, the array is effectively separated from the rest of the
> >>>> members without modifying the memory layout of the flexible structure.
> >>>> We then change the type of the middle struct member currently causing
> >>>> trouble from `struct posix_acl` to `struct posix_acl_hdr`.
> >>>>
> >>>> We also want to ensure that when new members need to be added to the
> >>>> flexible structure, they are always included within the newly created
> >>>> tagged struct. For this, we use `static_assert()`. This ensures that the
> >>>> memory layout for both the flexible structure and the new tagged struct
> >>>> is the same after any changes.
> >>>>
> >>>> This approach avoids having to implement `struct posix_acl_hdr` as a
> >>>> completely separate structure, thus preventing having to maintain two
> >>>> independent but basically identical structures, closing the door to
> >>>> potential bugs in the future.
> >>>>
> >>>> We also use `container_of()` whenever we need to retrieve a pointer to
> >>>> the flexible structure, through which we can access the flexible-array
> >>>> member, if necessary.
> >>>>
> >>>> So, with these changes, fix the following warning:
> >>>>
> >>>> fs/nfs_common/nfsacl.c:45:26: 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@xxxxxxxxxx>
> >>>> ---
> >>>> fs/nfs_common/nfsacl.c | 8 +++++---
> >>>> include/linux/posix_acl.h | 11 ++++++++---
> >>>> 2 files changed, 13 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfs_common/nfsacl.c b/fs/nfs_common/nfsacl.c
> >>>> index ea382b75b26c..e2eaac14fd8e 100644
> >>>> --- a/fs/nfs_common/nfsacl.c
> >>>> +++ b/fs/nfs_common/nfsacl.c
> >>>> @@ -42,7 +42,7 @@ struct nfsacl_encode_desc {
> >>>> };
> >>>>
> >>>> struct nfsacl_simple_acl {
> >>>> - struct posix_acl acl;
> >>>> + struct posix_acl_hdr acl;
> >>>> struct posix_acl_entry ace[4];
> >>>> };
> >>>>
> >>>> @@ -112,7 +112,8 @@ int nfsacl_encode(struct xdr_buf *buf, unsigned int base, struct inode *inode,
> >>>> xdr_encode_word(buf, base, entries))
> >>>> return -EINVAL;
> >>>> if (encode_entries && acl && acl->a_count == 3) {
> >>>> - struct posix_acl *acl2 = &aclbuf.acl;
> >>>> + struct posix_acl *acl2 =
> >>>> + container_of(&aclbuf.acl, struct posix_acl, hdr);
> >>>>
> >>>> /* Avoid the use of posix_acl_alloc(). nfsacl_encode() is
> >>>> * invoked in contexts where a memory allocation failure is
> >>>> @@ -177,7 +178,8 @@ bool nfs_stream_encode_acl(struct xdr_stream *xdr, struct inode *inode,
> >>>> return false;
> >>>>
> >>>> if (encode_entries && acl && acl->a_count == 3) {
> >>>> - struct posix_acl *acl2 = &aclbuf.acl;
> >>>> + struct posix_acl *acl2 =
> >>>> + container_of(&aclbuf.acl, struct posix_acl, hdr);
> >>>>
> >>>> /* Avoid the use of posix_acl_alloc(). nfsacl_encode() is
> >>>> * invoked in contexts where a memory allocation failure is
> >>>> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> >>>> index e2d47eb1a7f3..62d497763e25 100644
> >>>> --- a/include/linux/posix_acl.h
> >>>> +++ b/include/linux/posix_acl.h
> >>>> @@ -27,11 +27,16 @@ struct posix_acl_entry {
> >>>> };
> >>>>
> >>>> struct posix_acl {
> >>>> - refcount_t a_refcount;
> >>>> - unsigned int a_count;
> >>>> - struct rcu_head a_rcu;
> >>>> + /* New members MUST be added within the struct_group() macro below. */
> >>>> + struct_group_tagged(posix_acl_hdr, hdr,
> >>>> + refcount_t a_refcount;
> >>>> + unsigned int a_count;
> >>>> + struct rcu_head a_rcu;
> >>>> + );
> >>>> struct posix_acl_entry a_entries[] __counted_by(a_count);
> >>>> };
> >>>> +static_assert(offsetof(struct posix_acl, a_entries) == sizeof(struct posix_acl_hdr),
> >>>> + "struct member likely outside of struct_group_tagged()");
> >>>>
> >>>> #define FOREACH_ACL_ENTRY(pa, acl, pe) \
> >>>> for(pa=(acl)->a_entries, pe=pa+(acl)->a_count; pa<pe; pa++)
> >>>
> >>> Trond, Anna -
> >>>
> >>> Let me know if I need to take this one via the NFSD tree. If not,
> >>>
> >>> Acked-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> >>
> >> Gentle ping: Still waiting for a response.
> >
> > If you want it, it's yours! :)
> >
> > I am planning a bugfixes pull request soon, so I don't mind taking it
> > if you don't have anything else planned for 6.14 at the moment.
>
> This change doesn't look like a "fix" to me, so I would include it in
> nfsd-testing / nfsd-next, as long as you (or Trond) can send me an
> Acked-by.
Acked-by: Anna Schumaker <anna.schumaker@xxxxxxxxxx>
>
>
> --
> Chuck Lever
