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
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:18 UTC