Message-ID: <4bb48a2d-5599-4728-b909-bbbaba2b33ee@oracle.com>
Date: Mon, 16 Jun 2025 16:30:10 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Jeff Layton <jlayton@...nel.org>, NeilBrown <neil@...wn.name>,
        Olga Kornievskaia <okorniev@...hat.com>, Dai Ngo <Dai.Ngo@...cle.com>,
        Tom Talpey <tom@...pey.com>
Cc: linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2][next] NFSD: Avoid multiple
 -Wflex-array-member-not-at-end warnings

On 6/11/25 6:58 PM, Gustavo A. R. Silva wrote:
> Update `struct knfsd_fh` to use indices into the `fh_raw` array,
> allowing removal of the flexible-array member `fh_fsid[]`. Refactor
> related code accordingly.
> 
> With this changes, fix many instances of the following type of
> warnings:
> 
> fs/nfsd/nfsfh.h:79:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> fs/nfsd/state.h:763:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> fs/nfsd/state.h:669:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> fs/nfsd/state.h:549:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> fs/nfsd/xdr4.h:705:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> fs/nfsd/xdr4.h:678:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Co-developed-by: Christoph Hellwig <hch@...radead.org>
> Signed-off-by: Christoph Hellwig <hch@...radead.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
> Changes in v2:
>  - Use indices into `fh_raw`. (Christoph)
>  - Remove union and flexible-array member `fh_fsid`. (Christoph)
> 
> v1:
>  - Link: https://lore.kernel.org/linux-hardening/aBp37ZXBJM09yAXp@kspp/

I prefer Neil's solution to address just the "flexible array member"
warnings.

The intent in struct knfsd_fh was that fh_fsid was to fill up the rest
of that arm of the union; it is not flexible in size.

Would it be enough to replace:

	u32	fh_fsid[];	/* flexible-array member */

with:

	u32	fh_fsid[NFS4_FHSIZE / 4 - 1];

or:

	u32	fh_fsid[0];

?


I agree that packing the u8 fields in a struct/union combination and
then putting the octets directly onto the wire, as is currently done, is
asking for trouble. However, replacing the union, as Christoph
suggested, is a clean-up that ought to be done over time in multiple
patches so that it is done mechanically (perhaps via cocchinelle) and
can be bisected over if needed.

I might even want to see helper functions to access those fields, rather
than poking them by symbolic offset into an array. Using accessor
functions is definitely appropriate for the nfs4layouts.c consumer of
knfsd_fh.


>  fs/nfsd/export.c      |  2 +-
>  fs/nfsd/export.h      |  2 +-
>  fs/nfsd/nfs4layouts.c | 10 ++++----
>  fs/nfsd/nfsfh.c       | 58 +++++++++++++++++++++++--------------------
>  fs/nfsd/nfsfh.h       | 30 +++++++++++-----------
>  5 files changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 88ae410b4113..654d54a7148f 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1231,7 +1231,7 @@ rqst_exp_get_by_name(struct svc_rqst *rqstp, struct path *path)
>  struct svc_export *
>  rqst_exp_find(struct cache_req *reqp, struct net *net,
>  	      struct auth_domain *cl, struct auth_domain *gsscl,
> -	      int fsid_type, u32 *fsidv)
> +	      int fsid_type, void *fsidv)
>  {
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	struct svc_export *gssexp, *exp = ERR_PTR(-ENOENT);
> diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h
> index 4d92b99c1ffd..9b2719d8a3e2 100644
> --- a/fs/nfsd/export.h
> +++ b/fs/nfsd/export.h
> @@ -131,6 +131,6 @@ static inline struct svc_export *exp_get(struct svc_export *exp)
>  }
>  struct svc_export *rqst_exp_find(struct cache_req *reqp, struct net *net,
>  				 struct auth_domain *cl, struct auth_domain *gsscl,
> -				 int fsid_type, u32 *fsidv);
> +				 int fsid_type, void *fsidv);
>  
>  #endif /* NFSD_EXPORT_H */
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 290271ac4245..6dcd2c9ec15b 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -56,7 +56,7 @@ static void
>  nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>  {
>  	const struct knfsd_fh *fh = &fhp->fh_handle;
> -	size_t fsid_len = key_len(fh->fh_fsid_type);
> +	size_t fsid_len = key_len(fh->fh_raw[FH_FSID_TYPE]);
>  	struct nfsd4_deviceid_map *map, *old;
>  	int i;
>  
> @@ -64,8 +64,8 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>  	if (!map)
>  		return;
>  
> -	map->fsid_type = fh->fh_fsid_type;
> -	memcpy(&map->fsid, fh->fh_fsid, fsid_len);
> +	map->fsid_type = fh->fh_raw[FH_FSID_TYPE];
> +	memcpy(&map->fsid, fh->fh_raw + FH_FSID, fsid_len);
>  
>  	spin_lock(&nfsd_devid_lock);
>  	if (fhp->fh_export->ex_devid_map)
> @@ -73,9 +73,9 @@ nfsd4_alloc_devid_map(const struct svc_fh *fhp)
>  
>  	for (i = 0; i < DEVID_HASH_SIZE; i++) {
>  		list_for_each_entry(old, &nfsd_devid_hash[i], hash) {
> -			if (old->fsid_type != fh->fh_fsid_type)
> +			if (old->fsid_type != map->fsid_type)
>  				continue;
> -			if (memcmp(old->fsid, fh->fh_fsid,
> +			if (memcmp(old->fsid, map->fsid,
>  					key_len(old->fsid_type)))
>  				continue;
>  
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index aef474f1b84b..01ff91a28fb6 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -161,37 +161,40 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
>  	if (fh->fh_size == 0)
>  		return nfserr_nofilehandle;
>  
> -	if (fh->fh_version != 1)
> +	if (fh->fh_raw[FH_VERSION] != 1)
>  		return error;
>  
>  	if (--data_left < 0)
>  		return error;
> -	if (fh->fh_auth_type != 0)
> +	if (fh->fh_raw[FH_AUTH_TYPE] != 0)
>  		return error;
> -	len = key_len(fh->fh_fsid_type) / 4;
> +	len = key_len(fh->fh_raw[FH_FSID_TYPE]) / 4;
>  	if (len == 0)
>  		return error;
> -	if (fh->fh_fsid_type == FSID_MAJOR_MINOR) {
> +	if (fh->fh_raw[FH_FSID_TYPE] == FSID_MAJOR_MINOR) {
>  		/* deprecated, convert to type 3 */
> +		u32 *fsidv = (u32 *)(fh->fh_raw + FH_FSID);
> +
>  		len = key_len(FSID_ENCODE_DEV)/4;
> -		fh->fh_fsid_type = FSID_ENCODE_DEV;
> +		fh->fh_raw[FH_FSID_TYPE] = FSID_ENCODE_DEV;
>  		/*
>  		 * struct knfsd_fh uses host-endian fields, which are
>  		 * sometimes used to hold net-endian values. This
>  		 * confuses sparse, so we must use __force here to
>  		 * keep it from complaining.
>  		 */
> -		fh->fh_fsid[0] = new_encode_dev(MKDEV(ntohl((__force __be32)fh->fh_fsid[0]),
> -						      ntohl((__force __be32)fh->fh_fsid[1])));
> -		fh->fh_fsid[1] = fh->fh_fsid[2];
> +		fsidv[0] = new_encode_dev(MKDEV(
> +			ntohl((__force __be32)fsidv[0]),
> +			ntohl((__force __be32)fsidv[1])));
> +		fsidv[1] = fsidv[2];
>  	}
>  	data_left -= len;
>  	if (data_left < 0)
>  		return error;
>  	exp = rqst_exp_find(rqstp ? &rqstp->rq_chandle : NULL,
>  			    net, client, gssclient,
> -			    fh->fh_fsid_type, fh->fh_fsid);
> -	fid = (struct fid *)(fh->fh_fsid + len);
> +			    fh->fh_raw[FH_FSID_TYPE], fh->fh_raw + FH_FSID);
> +	fid = (struct fid *)(fh->fh_raw + FH_FSID + len);
>  
>  	error = nfserr_stale;
>  	if (IS_ERR(exp)) {
> @@ -233,7 +236,7 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
>  	 */
>  	error = nfserr_badhandle;
>  
> -	fileid_type = fh->fh_fileid_type;
> +	fileid_type = fh->fh_raw[FH_FILEID_TYPE];
>  
>  	if (fileid_type == FILEID_ROOT)
>  		dentry = dget(exp->ex_path.dentry);
> @@ -463,18 +466,19 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
>  {
>  	if (dentry != exp->ex_path.dentry) {
>  		struct fid *fid = (struct fid *)
> -			(fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
> +			(fhp->fh_handle.fh_raw + FH_FSID +
> +			 fhp->fh_handle.fh_size - 1);
>  		int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
>  		int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
>  				EXPORT_FH_CONNECTABLE;
>  		int fileid_type =
>  			exportfs_encode_fh(dentry, fid, &maxsize, fh_flags);
>  
> -		fhp->fh_handle.fh_fileid_type =
> +		fhp->fh_handle.fh_raw[FH_FILEID_TYPE] =
>  			fileid_type > 0 ? fileid_type : FILEID_INVALID;
>  		fhp->fh_handle.fh_size += maxsize * 4;
>  	} else {
> -		fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
> +		fhp->fh_handle.fh_raw[FH_FILEID_TYPE] = FILEID_ROOT;
>  	}
>  }
>  
> @@ -520,8 +524,8 @@ static void set_version_and_fsid_type(struct svc_fh *fhp, struct svc_export *exp
>  retry:
>  	version = 1;
>  	if (ref_fh && ref_fh->fh_export == exp) {
> -		version = ref_fh->fh_handle.fh_version;
> -		fsid_type = ref_fh->fh_handle.fh_fsid_type;
> +		version = ref_fh->fh_handle.fh_raw[FH_VERSION];
> +		fsid_type = ref_fh->fh_handle.fh_raw[FH_FSID_TYPE];
>  
>  		ref_fh = NULL;
>  
> @@ -562,9 +566,9 @@ static void set_version_and_fsid_type(struct svc_fh *fhp, struct svc_export *exp
>  		fsid_type = FSID_ENCODE_DEV;
>  	else
>  		fsid_type = FSID_DEV;
> -	fhp->fh_handle.fh_version = version;
> +	fhp->fh_handle.fh_raw[FH_VERSION] = version;
>  	if (version)
> -		fhp->fh_handle.fh_fsid_type = fsid_type;
> +		fhp->fh_handle.fh_raw[FH_FSID_TYPE] = fsid_type;
>  }
>  
>  __be32
> @@ -610,18 +614,18 @@ fh_compose(struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry,
>  	fhp->fh_export = exp_get(exp);
>  
>  	fhp->fh_handle.fh_size =
> -		key_len(fhp->fh_handle.fh_fsid_type) + 4;
> -	fhp->fh_handle.fh_auth_type = 0;
> +		key_len(fhp->fh_handle.fh_raw[FH_FSID_TYPE]) + 4;
> +	fhp->fh_handle.fh_raw[FH_AUTH_TYPE] = 0;
>  
> -	mk_fsid(fhp->fh_handle.fh_fsid_type,
> -		fhp->fh_handle.fh_fsid,
> +	mk_fsid(fhp->fh_handle.fh_raw[FH_FSID_TYPE],
> +		fhp->fh_handle.fh_raw + FH_FSID,
>  		ex_dev,
>  		d_inode(exp->ex_path.dentry)->i_ino,
>  		exp->ex_fsid, exp->ex_uuid);
>  
>  	if (inode)
>  		_fh_update(fhp, exp, dentry);
> -	if (fhp->fh_handle.fh_fileid_type == FILEID_INVALID) {
> +	if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] == FILEID_INVALID) {
>  		fh_put(fhp);
>  		return nfserr_stale;
>  	}
> @@ -644,11 +648,11 @@ fh_update(struct svc_fh *fhp)
>  	dentry = fhp->fh_dentry;
>  	if (d_really_is_negative(dentry))
>  		goto out_negative;
> -	if (fhp->fh_handle.fh_fileid_type != FILEID_ROOT)
> +	if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] != FILEID_ROOT)
>  		return 0;
>  
>  	_fh_update(fhp, fhp->fh_export, dentry);
> -	if (fhp->fh_handle.fh_fileid_type == FILEID_INVALID)
> +	if (fhp->fh_handle.fh_raw[FH_FILEID_TYPE] == FILEID_INVALID)
>  		return nfserr_stale;
>  	return 0;
>  out_bad:
> @@ -776,9 +780,9 @@ char * SVCFH_fmt(struct svc_fh *fhp)
>  
>  enum fsid_source fsid_source(const struct svc_fh *fhp)
>  {
> -	if (fhp->fh_handle.fh_version != 1)
> +	if (fhp->fh_handle.fh_raw[FH_VERSION] != 1)
>  		return FSIDSOURCE_DEV;
> -	switch(fhp->fh_handle.fh_fsid_type) {
> +	switch (fhp->fh_handle.fh_raw[FH_FSID_TYPE]) {
>  	case FSID_DEV:
>  	case FSID_ENCODE_DEV:
>  	case FSID_MAJOR_MINOR:
> diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> index 5103c2f4d225..26975ede465a 100644
> --- a/fs/nfsd/nfsfh.h
> +++ b/fs/nfsd/nfsfh.h
> @@ -43,22 +43,17 @@
>   *   filesystems must not use the values '0' or '0xff'. 'See enum fid_type'
>   *   in include/linux/exportfs.h for currently registered values.
>   */
> -
>  struct knfsd_fh {
>  	unsigned int	fh_size;	/*
>  					 * Points to the current size while
>  					 * building a new file handle.
>  					 */
> -	union {
> -		char			fh_raw[NFS4_FHSIZE];
> -		struct {
> -			u8		fh_version;	/* == 1 */
> -			u8		fh_auth_type;	/* deprecated */
> -			u8		fh_fsid_type;
> -			u8		fh_fileid_type;
> -			u32		fh_fsid[]; /* flexible-array member */
> -		};
> -	};
> +	char		fh_raw[NFS4_FHSIZE];
> +#define FH_VERSION		0
> +#define FH_AUTH_TYPE		1
> +#define FH_FSID_TYPE		2
> +#define FH_FILEID_TYPE		3
> +#define FH_FSID			4
>  };
>  
>  static inline __u32 ino_t_to_u32(ino_t ino)
> @@ -141,10 +136,12 @@ extern enum fsid_source fsid_source(const struct svc_fh *fhp);
>   * sparse from complaining. Since these values are opaque to the
>   * client, that shouldn't be a problem.
>   */
> -static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
> -			   u32 fsid, unsigned char *uuid)
> +static inline void mk_fsid(int vers, void *fsid, dev_t dev, ino_t ino,
> +			   u32 fsid_num, unsigned char *uuid)
>  {
> +	u32 *fsidv = fsid;
>  	u32 *up;
> +
>  	switch(vers) {
>  	case FSID_DEV:
>  		fsidv[0] = (__force __u32)htonl((MAJOR(dev)<<16) |
> @@ -152,7 +149,7 @@ static inline void mk_fsid(int vers, u32 *fsidv, dev_t dev, ino_t ino,
>  		fsidv[1] = ino_t_to_u32(ino);
>  		break;
>  	case FSID_NUM:
> -		fsidv[0] = fsid;
> +		fsidv[0] = fsid_num;
>  		break;
>  	case FSID_MAJOR_MINOR:
>  		fsidv[0] = (__force __u32)htonl(MAJOR(dev));
> @@ -260,9 +257,10 @@ static inline bool fh_match(const struct knfsd_fh *fh1,
>  static inline bool fh_fsid_match(const struct knfsd_fh *fh1,
>  				 const struct knfsd_fh *fh2)
>  {
> -	if (fh1->fh_fsid_type != fh2->fh_fsid_type)
> +	if (fh1->fh_raw[FH_FSID_TYPE] != fh2->fh_raw[FH_FSID_TYPE])
>  		return false;
> -	if (memcmp(fh1->fh_fsid, fh2->fh_fsid, key_len(fh1->fh_fsid_type)) != 0)
> +	if (memcmp(fh1->fh_raw + FH_FSID, fh2->fh_raw + FH_FSID,
> +			key_len(fh1->fh_raw[FH_FSID_TYPE])) != 0)
>  		return false;
>  	return true;
>  }


-- 
Chuck Lever

Powered by blists - more mailing lists