Re: which way to update export_args structure?

On Wed, 3 Oct 2018 00:40:27 +0000
Rick Macklem <rmacklem_at_uoguelph.ca> wrote:

> Hi,
> 
> I am working on updating "struct export_args" to fix/add a few things.
> One of these is that "ex_flags" is an int, but the flags are defined in mount.h
> as MNT_xx bits that now exceed 32bits (mnt_flag is now uint64_t).
> For now, this doesn't break anything, since the flags used by ex_flags are
> all defined in the low order 32bits but...it seems like this should be addressed
> by a new version of "struct export_args".
> 
> I have two versions of the updated structure:
> A)
> struct export_args {
> 	uint64_t ex_flags;		/* export related flags */
> 	uid_t	ex_root;		/* mapping for root uid */
> 	struct	xucred ex_anon;		/* mapping for anonymous user */
> 	struct	sockaddr *ex_addr;	/* net address to which exported */
> 	u_char	ex_addrlen;		/* and the net address length */
> 	struct	sockaddr *ex_mask;	/* mask of valid bits in saddr */
> 	u_char	ex_masklen;		/* and the smask length */
> 	char	*ex_indexfile;		/* index file for WebNFS URLs */
> 	int	ex_numsecflavors;	/* security flavor count */
> 	int	ex_secflavors[MAXSECFLAVORS]; /* list of security flavors */
> 	int32_t	ex_fsid;		/* mnt_stat.f_fsid.val[0] if */
> 					/* MNT_EXPORTFSID set in ex_flags64 */
> 	gid_t	*ex_suppgroups;		/* Supplemental groups if */
> 					/* ex_anon.cr_ngroups > XU_NGROUPS */
> };
> B)
> struct export_args {
> 	int	ex_flags;		/* export related flags */
> 	uid_t	ex_root;		/* mapping for root uid */
> 	struct	xucred ex_anon;		/* mapping for anonymous user */
> 	struct	sockaddr *ex_addr;	/* net address to which exported */
> 	u_char	ex_addrlen;		/* and the net address length */
> 	struct	sockaddr *ex_mask;	/* mask of valid bits in saddr */
> 	u_char	ex_masklen;		/* and the smask length */
> 	char	*ex_indexfile;		/* index file for WebNFS URLs */
> 	int	ex_numsecflavors;	/* security flavor count */
> 	int	ex_secflavors[MAXSECFLAVORS]; /* list of security flavors */
> 	uint64_t ex_flagshighbits;	/* High order bits of mnt_flag */
> 	int32_t	ex_fsid;		/* mnt_stat.f_fsid.val[0] if */
> 					/* MNT_EXPORTFSID set in ex_flags64 */
> 	gid_t	*ex_suppgroups;		/* Supplemental groups if */
> 					/* ex_anon.cr_ngroups > XU_NGROUPS */
> };
> 
> A) does the obvious thing. Unfortunately, this changes the vfs KABI
> (specifically the function vfs_oexport_conv()) such that a file system
> module compiled with an unpatched mount.h could crash a patched system.
> As such, I think it couldn't be MFC'd and would be stuck in head/current
> until FreeBSD13 (or FreeBSD14 if 13 gets skipped over;-).
> 
> B) doesn't change any fields, but adds a second ex_flagshighbits for the high
> order bit. Since it only adds fields where none of those bits are used after
> the exports are processed by vfs_export() and, as such, will not break
> the VFS KABI, since vfs_domount_update() differentiates which version
> of export_args is being used.
> As such, I believe this version can be MFC'd. However, it does seem confusing
> to have the two ex_flags fields for the low and high 32bits.
> 
> So, which do you think it preferable? rick
> ps: Josh Paetzel has volunteered to do the mountd.c changes and I have a kerne
>       patch that I'll put up on phabricator once Josh has been able to test it.
> 

In B, shouldn't ex_flags become uint32_t if all 32 bits can contain
flag bits?  And why make ex_flagshighbits a uint64_t?  You could
make it uint32_t and simply shift the high bits around.

-- 
Gary Jennejohn