Re: which way to update export_args structure?

From: Brooks Davis <brooks_at_freebsd.org>
Date: Mon, 22 Oct 2018 16:05:08 +0000
On Sat, Oct 20, 2018 at 01:17:37AM +0000, Rick Macklem wrote:
> Brooks Davis wrote:
> > Yes, I think that's the right way foward.  Thanks for following up.
> >Rick Macklem wrote:
> >> Just in case you missed it in the email thread, in your general question below...
> >> Did you mean/suggest that the fields of "struct export_args" be passed in as
> >> separate options to nmount(2)?
> >>
> >> This sounds like a reasonable idea to me and I can ping Josh Paetzel w.r.t. the
> >> changes to mountd.c to do it. (We are still in the testing stage for the updated
> >> struct, so we might as well get that working first.)
> >>
> Well, Josh and I now have the code working via. passing the export_args
> structure into the kernel using the "export"  nmount(2) option.
> 
> I have coded a partial patch (not complete nor tested) to pass the fields in as
> separate nmount(2) arguments. Since the patch has gotten fairly large
> already, I wanted to see if people do think this is the correct approach.
> (I'll admit I don't understand why having the arguments would matter, given
>  that only mountd does it. Would anyone run a 32bit mountd on a 64bit kernel?)
> 
> Anyhow, here's the partial patch showing the main changes when going from
> passing in "struct export_args" to passing in separate fields:
> 
> --- kern/vfs_mount.c.nofsid2	2018-10-16 23:45:33.540348000 -0400
> +++ kern/vfs_mount.c	2018-10-19 20:01:14.927370000 -0400
> _at__at_ -277,6 +277,7 _at__at_ vfs_buildopts(struct uio *auio, struct v
>  	size_t memused, namelen, optlen;
>  	unsigned int i, iovcnt;
>  	int error;
> +	char *cp;
>  
>  	opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
>  	TAILQ_INIT(opts);
> _at__at_ -325,7 +326,7 _at__at_ vfs_buildopts(struct uio *auio, struct v
>  		}
>  		if (optlen != 0) {
>  			opt->len = optlen;
> -			opt->value = malloc(optlen, M_MOUNT, M_WAITOK);
> +			opt->value = malloc(optlen + 1, M_MOUNT, M_WAITOK);
>  			if (auio->uio_segflg == UIO_SYSSPACE) {
>  				bcopy(auio->uio_iov[i + 1].iov_base, opt->value,
>  				    optlen);
> _at__at_ -335,6 +336,8 _at__at_ vfs_buildopts(struct uio *auio, struct v
>  				if (error)
>  					goto bad;
>  			}
> +			cp = (char *)opt->value;
> +			cp[optlen] = '\0';
>  		}
>  	}
>  	vfs_sanitizeopts(opts);
> _at__at_ -961,6 +964,8 _at__at_ vfs_domount_update(
>  	int error, export_error, i, len;
>  	uint64_t flag;
>  	struct o2export_args o2export;
> +	char *endptr;
> +	int gotexp;
>  
>  	ASSERT_VOP_ELOCKED(vp, __func__);
>  	KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
> _at__at_ -1033,36 +1038,117 _at__at_ vfs_domount_update(
>  
>  	export_error = 0;
>  	/* Process the export option. */
> -	if (error == 0 && vfs_getopt(mp->mnt_optnew, "export", &bufp,
> -	    &len) == 0) {
> -		/* Assume that there is only 1 ABI for each length. */
> -		switch (len) {
> -		case (sizeof(struct oexport_args)):
> -		case (sizeof(struct o2export_args)):
> -			memset(&export, 0, sizeof(export));
> -			memset(&o2export, 0, sizeof(o2export));
> -			memcpy(&o2export, bufp, len);
> -			export.ex_flags = (u_int)o2export.ex_flags;
> -			export.ex_root = o2export.ex_root;
> -			export.ex_anon = o2export.ex_anon;
> -			export.ex_addr = o2export.ex_addr;
> -			export.ex_addrlen = o2export.ex_addrlen;
> -			export.ex_mask = o2export.ex_mask;
> -			export.ex_masklen = o2export.ex_masklen;
> -			export.ex_indexfile = o2export.ex_indexfile;
> -			export.ex_numsecflavors = o2export.ex_numsecflavors;
> -			for (i = 0; i < MAXSECFLAVORS; i++)
> -				export.ex_secflavors[i] =
> -				    o2export.ex_secflavors[i];
> -			export_error = vfs_export(mp, &export);
> -			break;
> -		case (sizeof(export)):
> -			bcopy(bufp, &export, len);
> -			export_error = vfs_export(mp, &export);
> -			break;
> -		default:
> -			export_error = EINVAL;
> -			break;
> +	if (error == 0) {
> +		gotexp = 0;
> +		memset(&export, 0, sizeof(export));
> +		if (vfs_getopt(mp->mnt_optnew, "export.exflags", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_flags = strtouq(bufp, &endptr, 0);
> +			if (endptr == bufp)
> +				export_error = EINVAL;
> +		}

This pattern involving strtouq seems wrong to me.  We should probably
pass a pointer to the integer type (which should always be an explicitly
sized type).

If it didn't contradict the first sentence of the description in
vfs_getopt.9, I'd say we should pass int and long values in the length
with a NULL buffer.

> +		if (vfs_getopt(mp->mnt_optnew, "export.root", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_root = strtouq(bufp, &endptr, 0);
> +			if (endptr == bufp)
> +				export_error = EINVAL;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.anonuid", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_anon.cr_uid = strtouq(bufp, &endptr, 0);
> +			if (endptr != bufp)
> +				export.ex_anon.cr_version = XUCRED_VERSION;
> +			else
> +				export_error = EINVAL;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.anongroups", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_anon.cr_ngroups = len / sizeof(gid_t);
> +			if (export.ex_anon.cr_ngroups > XU_NGROUPS) {
> +				export.ex_suppgroups = mallocarray(
> +				    sizeof(gid_t),
> +				    export.ex_anon.cr_ngroups, M_TEMP,
> +				    M_WAITOK);
> +				memcpy(export.ex_suppgroups, bufp, len);
> +			} else
> +				memcpy(export.ex_anon.cr_groups, bufp,
> +				    len);
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.addr", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_addr = malloc(len, M_TEMP, M_WAITOK);
> +			memcpy(export.ex_addr, bufp, len);
> +			export.ex_addrlen = len;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.mask", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_mask = malloc(len, M_TEMP, M_WAITOK);
> +			memcpy(export.ex_mask, bufp, len);
> +			export.ex_masklen = len;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.indexfile", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_indexfile = malloc(len + 1, M_TEMP,
> +			    M_WAITOK);
> +			memcpy(export.ex_indexfile, bufp, len);
> +			export.ex_indexfile[len] = '\0';
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.secflavors", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_numsecflavors = len / sizeof(uint32_t);
> +			if (export.ex_numsecflavors <= MAXSECFLAVORS)
> +				memcpy(export.ex_secflavors, bufp, len);
> +			else
> +				export_error = EINVAL;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export.fsid", &bufp,
> +		    &len) == 0) {
> +			gotexp = 1;
> +			export.ex_fsid = strtouq(bufp, &endptr, 0);
> +			if (endptr == bufp)
> +				export_error = EINVAL;
> +		}
> +		if (vfs_getopt(mp->mnt_optnew, "export", &bufp, &len) == 0) {
> +			/* Assume that there is only 1 ABI for each length. */
> +			switch (len) {
> +			case (sizeof(struct oexport_args)):
> +			case (sizeof(struct o2export_args)):
> +				memset(&export, 0, sizeof(export));

I think this is now redundant.

> +				memset(&o2export, 0, sizeof(o2export));

This is certainly redundant given that you immediately copy over it.

> +				memcpy(&o2export, bufp, len);
> +				export.ex_flags = (u_int)o2export.ex_flags;
> +				export.ex_root = o2export.ex_root;
> +				export.ex_anon = o2export.ex_anon;
> +				export.ex_addr = o2export.ex_addr;
> +				export.ex_addrlen = o2export.ex_addrlen;
> +				export.ex_mask = o2export.ex_mask;
> +				export.ex_masklen = o2export.ex_masklen;
> +				export.ex_indexfile = o2export.ex_indexfile;
> +				export.ex_numsecflavors = o2export.ex_numsecflavors;
> +				for (i = 0; i < MAXSECFLAVORS; i++)
> +					export.ex_secflavors[i] =
> +					    o2export.ex_secflavors[i];
> +				export_error = vfs_export(mp, &export);
> +				break;
> +			default:
> +				export_error = EINVAL;
> +				break;
> +			}
> +		} else if (gotexp != 0) {
> +			if (export_error == 0)
> +				export_error = vfs_export(mp, &export);
> +			free(export.ex_addr, M_TEMP);
> +			free(export.ex_mask, M_TEMP);
> +			free(export.ex_indexfile, M_TEMP);
> +			free(export.ex_suppgroups, M_TEMP);
>  		}
>  	}
>  
> So, what to people think about this? rick

This is the direction I'd been thinking.  FWIW, the usecase is more that
once you've moved away from the struct it's easy to make incremental
changes then to use a 32-bit mountd on a 64-bit kernel.  Moving toward
size-independent interfaces helps both causes though.

-- Brooks

Received on Mon Oct 22 2018 - 14:05:17 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:18 UTC