Date: Mon, 22 Oct 2018 16:05:08 +0000 From: Brooks Davis <brooks@freebsd.org> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: Brooks Davis <brooks@freebsd.org>, FreeBSD Current <freebsd-current@freebsd.org>, Josh Paetzel <josh@tcbug.org> Subject: Re: which way to update export_args structure? Message-ID: <20181022160508.GB45769@spindle.one-eyed-alien.net> In-Reply-To: <YTOPR0101MB11626B32F73B520FBDA3C633DDFA0@YTOPR0101MB1162.CANPRD01.PROD.OUTLOOK.COM> References: <YTOPR0101MB182021549F8CF8277477A4C5DDE90@YTOPR0101MB1820.CANPRD01.PROD.OUTLOOK.COM> <20181003155133.GA57729@spindle.one-eyed-alien.net> <YTOPR0101MB18207FF98DED0232B9BB1B4FDDE50@YTOPR0101MB1820.CANPRD01.PROD.OUTLOOK.COM> <20181008170428.GB9766@spindle.one-eyed-alien.net> <YTOPR0101MB11626B32F73B520FBDA3C633DDFA0@YTOPR0101MB1162.CANPRD01.PROD.OUTLOOK.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20181022160508.GB45769>