From owner-freebsd-current@freebsd.org Mon Oct 22 16:05:17 2018 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E7BB71036B43 for ; Mon, 22 Oct 2018 16:05:16 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 86BC481BC3; Mon, 22 Oct 2018 16:05:16 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id D901F3C475F; Mon, 22 Oct 2018 16:05:08 +0000 (UTC) Date: Mon, 22 Oct 2018 16:05:08 +0000 From: Brooks Davis To: Rick Macklem Cc: Brooks Davis , FreeBSD Current , Josh Paetzel Subject: Re: which way to update export_args structure? Message-ID: <20181022160508.GB45769@spindle.one-eyed-alien.net> References: <20181003155133.GA57729@spindle.one-eyed-alien.net> <20181008170428.GB9766@spindle.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Qxx1br4bt0+wmkIi" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Oct 2018 16:05:17 -0000 --Qxx1br4bt0+wmkIi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 questi= on 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= =2Er.t. the > >> changes to mountd.c to do it. (We are still in the testing stage for t= he 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. >=20 > 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, giv= en > that only mountd does it. Would anyone run a 32bit mountd on a 64bit ker= nel?) >=20 > Anyhow, here's the partial patch showing the main changes when going from > passing in "struct export_args" to passing in separate fields: >=20 > --- 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 > @@ -277,6 +277,7 @@ vfs_buildopts(struct uio *auio, struct v > size_t memused, namelen, optlen; > unsigned int i, iovcnt; > int error; > + char *cp; > =20 > opts =3D malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK); > TAILQ_INIT(opts); > @@ -325,7 +326,7 @@ vfs_buildopts(struct uio *auio, struct v > } > if (optlen !=3D 0) { > opt->len =3D optlen; > - opt->value =3D malloc(optlen, M_MOUNT, M_WAITOK); > + opt->value =3D malloc(optlen + 1, M_MOUNT, M_WAITOK); > if (auio->uio_segflg =3D=3D UIO_SYSSPACE) { > bcopy(auio->uio_iov[i + 1].iov_base, opt->value, > optlen); > @@ -335,6 +336,8 @@ vfs_buildopts(struct uio *auio, struct v > if (error) > goto bad; > } > + cp =3D (char *)opt->value; > + cp[optlen] =3D '\0'; > } > } > vfs_sanitizeopts(opts); > @@ -961,6 +964,8 @@ vfs_domount_update( > int error, export_error, i, len; > uint64_t flag; > struct o2export_args o2export; > + char *endptr; > + int gotexp; > =20 > ASSERT_VOP_ELOCKED(vp, __func__); > KASSERT((fsflags & MNT_UPDATE) !=3D 0, ("MNT_UPDATE should be here")); > @@ -1033,36 +1038,117 @@ vfs_domount_update( > =20 > export_error =3D 0; > /* Process the export option. */ > - if (error =3D=3D 0 && vfs_getopt(mp->mnt_optnew, "export", &bufp, > - &len) =3D=3D 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 =3D (u_int)o2export.ex_flags; > - export.ex_root =3D o2export.ex_root; > - export.ex_anon =3D o2export.ex_anon; > - export.ex_addr =3D o2export.ex_addr; > - export.ex_addrlen =3D o2export.ex_addrlen; > - export.ex_mask =3D o2export.ex_mask; > - export.ex_masklen =3D o2export.ex_masklen; > - export.ex_indexfile =3D o2export.ex_indexfile; > - export.ex_numsecflavors =3D o2export.ex_numsecflavors; > - for (i =3D 0; i < MAXSECFLAVORS; i++) > - export.ex_secflavors[i] =3D > - o2export.ex_secflavors[i]; > - export_error =3D vfs_export(mp, &export); > - break; > - case (sizeof(export)): > - bcopy(bufp, &export, len); > - export_error =3D vfs_export(mp, &export); > - break; > - default: > - export_error =3D EINVAL; > - break; > + if (error =3D=3D 0) { > + gotexp =3D 0; > + memset(&export, 0, sizeof(export)); > + if (vfs_getopt(mp->mnt_optnew, "export.exflags", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_flags =3D strtouq(bufp, &endptr, 0); > + if (endptr =3D=3D bufp) > + export_error =3D 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) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_root =3D strtouq(bufp, &endptr, 0); > + if (endptr =3D=3D bufp) > + export_error =3D EINVAL; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.anonuid", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_anon.cr_uid =3D strtouq(bufp, &endptr, 0); > + if (endptr !=3D bufp) > + export.ex_anon.cr_version =3D XUCRED_VERSION; > + else > + export_error =3D EINVAL; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.anongroups", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_anon.cr_ngroups =3D len / sizeof(gid_t); > + if (export.ex_anon.cr_ngroups > XU_NGROUPS) { > + export.ex_suppgroups =3D 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) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_addr =3D malloc(len, M_TEMP, M_WAITOK); > + memcpy(export.ex_addr, bufp, len); > + export.ex_addrlen =3D len; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.mask", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_mask =3D malloc(len, M_TEMP, M_WAITOK); > + memcpy(export.ex_mask, bufp, len); > + export.ex_masklen =3D len; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.indexfile", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_indexfile =3D malloc(len + 1, M_TEMP, > + M_WAITOK); > + memcpy(export.ex_indexfile, bufp, len); > + export.ex_indexfile[len] =3D '\0'; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.secflavors", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_numsecflavors =3D len / sizeof(uint32_t); > + if (export.ex_numsecflavors <=3D MAXSECFLAVORS) > + memcpy(export.ex_secflavors, bufp, len); > + else > + export_error =3D EINVAL; > + } > + if (vfs_getopt(mp->mnt_optnew, "export.fsid", &bufp, > + &len) =3D=3D 0) { > + gotexp =3D 1; > + export.ex_fsid =3D strtouq(bufp, &endptr, 0); > + if (endptr =3D=3D bufp) > + export_error =3D EINVAL; > + } > + if (vfs_getopt(mp->mnt_optnew, "export", &bufp, &len) =3D=3D 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 =3D (u_int)o2export.ex_flags; > + export.ex_root =3D o2export.ex_root; > + export.ex_anon =3D o2export.ex_anon; > + export.ex_addr =3D o2export.ex_addr; > + export.ex_addrlen =3D o2export.ex_addrlen; > + export.ex_mask =3D o2export.ex_mask; > + export.ex_masklen =3D o2export.ex_masklen; > + export.ex_indexfile =3D o2export.ex_indexfile; > + export.ex_numsecflavors =3D o2export.ex_numsecflavors; > + for (i =3D 0; i < MAXSECFLAVORS; i++) > + export.ex_secflavors[i] =3D > + o2export.ex_secflavors[i]; > + export_error =3D vfs_export(mp, &export); > + break; > + default: > + export_error =3D EINVAL; > + break; > + } > + } else if (gotexp !=3D 0) { > + if (export_error =3D=3D 0) > + export_error =3D 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); > } > } > =20 > 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 --Qxx1br4bt0+wmkIi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJbzfU0AAoJEKzQXbSebgfARYkH+wU8A8kgPfMM4YqY2aH/iGFa DShMfk9QXYsq0scUXBx62LssofXMO6l3ptJo6OyfQFd7JW63K/GKE/5mM1/oXQmh SL7vSDqinh0TiqhZLhf1DEb94A96nZc9QnW29s+U1J0uSkrhCH3uk9MGBjqEHvmy L5S122NnFXxB9DMiZERUOD+VebjwBGBX30z6HLeO79b8ql3dIziTggGcnk6I90qz UvWz+02wr1g+TNy1QJJmKChhhtLwPiBOI6un09WwfLPGVf6b+Egwplvt1nWvCBdv VUZSBrgSFwurFb1uv53Cz/p9F32+qcdyPx6pl/P5TcktGUO+PNtBYLV6VoTn7F0= =JT8t -----END PGP SIGNATURE----- --Qxx1br4bt0+wmkIi--