Re: which way to update export_args structure?
Brooks Davis wrote this message on Mon, Oct 22, 2018 at 16:05 +:
> > + switch (len) {
> > + case (sizeof(struct oexport_args)):
> > + case (sizeof(struct o2export_args)):
> > + memset(, 0, sizeof(export));
>
> I think this is now redundant.
>
> > + memset(, 0, sizeof(o2export));
>
> This is certainly redundant given that you immediately copy over it.
This is not redundant if sizeof oexport_args < sizeof o2export, and
len == sizeof oexport_args... It zeros out the remaining of the buffer..
> > + memcpy(, bufp, len);
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
signature.asc
Description: PGP signature
Re: which way to update export_args structure?
> On Oct 22, 2018, at 09:49, Josh Paetzel wrote: > > > >> On Mon, Oct 22, 2018, at 11:05 AM, Brooks Davis wrote: >> >> >> 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 >> Email had 1 attachment: >> + signature.asc >> 1k (application/pgp-signature) > > > Brooks, > > What is the benefit or usecase for running a 32 bit mountd on a 64 bit kernel? There generally isn’t a case for doing this, but running a 32-bit mountd in a 32-bit chroot can allow someone with a working 32-bit environment at a company (for instance) to rebuild environments which rely on NFS mounts and the like. This is an esoteric usecase, but I’ve seen it used before (and I’ve used it myself ;)..). I don’t think this niche usecase should hinder forward progress in terms of modernizing the base OS though. Biarch usecases are diminishing over time. -Enji ___ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
On Mon, Oct 22, 2018 at 09:59:52PM +, Rick Macklem wrote:
>
>
> From: owner-freebsd-curr...@freebsd.org
> on behalf of Rick Macklem
> Sent: Monday, October 22, 2018 5:24 PM
> To: Brooks Davis
> Cc: FreeBSD Current; Josh Paetzel
> Subject: Re: which way to update export_args structure?
>
> Brooks Davis wrote:
> On Sat, Oct 20, 2018 at 01:17:37AM +, Rick Macklem wrote:
> [lots of stuff snipped]
> >> + if (vfs_getopt(mp->mnt_optnew, "export", , ) == 0) {
> >> + /* Assume that there is only 1 ABI for each length.
> >> */
> >> + switch (len) {
> >> + case (sizeof(struct oexport_args)):
> >> + case (sizeof(struct o2export_args)):
> ...
> >> + memset(, 0, sizeof(o2export));
> >
> >This is certainly redundant given that you immediately copy over it.
> Actually, I just looked and this code is correct. The trick is that "len"
> might
> equal sizeof(struct oexport_args) which is < sizeof(struct o2export_args).
> As such, the memcpy() may only replace the first fields of "struct
> o2export_args".
> I've added a comment to the patch to clarify this.
Ah, that makes sense.
-- Brooks
signature.asc
Description: PGP signature
Re: which way to update export_args structure?
From: owner-freebsd-curr...@freebsd.org on
behalf of Rick Macklem
Sent: Monday, October 22, 2018 5:24 PM
To: Brooks Davis
Cc: FreeBSD Current; Josh Paetzel
Subject: Re: which way to update export_args structure?
Brooks Davis wrote:
On Sat, Oct 20, 2018 at 01:17:37AM +, Rick Macklem wrote:
[lots of stuff snipped]
>> + if (vfs_getopt(mp->mnt_optnew, "export", , ) == 0) {
>> + /* Assume that there is only 1 ABI for each length. */
>> + switch (len) {
>> + case (sizeof(struct oexport_args)):
>> + case (sizeof(struct o2export_args)):
...
>> + memset(, 0, sizeof(o2export));
>
>This is certainly redundant given that you immediately copy over it.
Actually, I just looked and this code is correct. The trick is that "len" might
equal sizeof(struct oexport_args) which is < sizeof(struct o2export_args).
As such, the memcpy() may only replace the first fields of "struct
o2export_args".
I've added a comment to the patch to clarify this.
rick
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
Brooks Davis wrote:
On Sat, Oct 20, 2018 at 01:17:37AM +, Rick Macklem wrote:
[lots of stuff snipped]
>> + if (error == 0) {
>> + gotexp = 0;
>> + memset(, 0, sizeof(export));
>> + if (vfs_getopt(mp->mnt_optnew, "export.exflags", ,
>> + ) == 0) {
>> + gotexp = 1;
>> + export.ex_flags = strtouq(bufp, , 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).
Thanks for noticing this. I was "on the fence" w.r.t. whether the arguments
should
all be architecture neutral (ascii representation or ???).
(I didn't bother doing the code for the net addresses in ascii notation.)
It looks like you don't see that as needed, so I will just pass the binary
numbers in.
(We could put them in Big Endian order, if someone thinks that is necessary?)
>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.
Heh, heh. That would be a sneaky hack. I like it, but will resist and use the
bufp.
[more stuff snipped]
>> + if (vfs_getopt(mp->mnt_optnew, "export", , ) == 0) {
>> + /* Assume that there is only 1 ABI for each length. */
>> + switch (len) {
>> + case (sizeof(struct oexport_args)):
>> + case (sizeof(struct o2export_args)):
>> + memset(, 0, sizeof(export));
>
>I think this is now redundant.
I'll be looking at the code to-day, but I think there is at least one field
(ex_suppgroups) that needs to be set NULL so that the free() won't blow up.
(I tend to bzero()/memset(0) so that any future additional fields don't have to
be
explicitly set 0/NULL in the compatibility code, but it there aren't currently
any,
I may take this out. This code doesn't get executed frequently, so efficiency at
the "save a few instructions" level doesn't matter, imho.)
>> + memset(, 0, sizeof(o2export));
>
>This is certainly redundant given that you immediately copy over it.
Yep. This was just cruft left over from a previous version of the patch.
>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.
Cool. I'll finish the patch to-day and then let Josh work on mountd.c.
Thanks for your comments, rick
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
On Mon, Oct 22, 2018 at 11:49:23AM -0500, Josh Paetzel wrote: > > > On Mon, Oct 22, 2018, at 11:05 AM, Brooks Davis wrote: > > > > > 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. > What is the benefit or usecase for running a 32 bit mountd on a 64 bit kernel? In practice there isn't much of one. If the support had been there, I'd have tried to use it in CheriBSD to run with our 128-bit pointers. The bigger benefit is that now when you want to add something you just add it and handle the case when it's missing rather than having to add another compat shim for old structs. FWIW, I don't feel all that strongly about this case. If you two don't find it worthwhile to make this change, that's entierly reasonable. It just felt like the use of the export struct within the nmount interface was a conversion bandaid vs an intentional decision. -- Brooks signature.asc Description: PGP signature
Re: which way to update export_args structure?
On Mon, Oct 22, 2018, at 11:05 AM, Brooks Davis wrote: > > 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 > Email had 1 attachment: > + signature.asc > 1k (application/pgp-signature) Brooks, What is the benefit or usecase for running a 32 bit mountd on a 64 bit kernel? -- Thanks, Josh Paetzel ___ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
On Sat, Oct 20, 2018 at 01:17:37AM +, 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.92737 -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;
>
> opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
> TAILQ_INIT(opts);
> @@ -325,7 +326,7 @@ 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);
> @@ -335,6 +336,8 @@ vfs_buildopts(struct uio *auio, struct v
> if (error)
> goto bad;
> }
> + cp = (char *)opt->value;
> + cp[optlen] = '\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;
>
> ASSERT_VOP_ELOCKED(vp, __func__);
> KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
> @@ -1033,36 +1038,117 @@ vfs_domount_update(
>
> export_error = 0;
> /* Process the export option. */
> - if (error == 0 && vfs_getopt(mp->mnt_optnew, "export", ,
> - ) == 0) {
> - /* Assume that there is only 1 ABI for each length. */
> - switch (len) {
> - case (sizeof(struct oexport_args)):
> - case (sizeof(struct o2export_args)):
> - memset(, 0, sizeof(export));
> - memset(, 0, sizeof(o2export));
> - memcpy(, 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, );
> - break;
> - case (sizeof(export)):
> - bcopy(bufp, , len);
> - export_error = vfs_export(mp, );
> - break;
> - default:
> - export_error = EINVAL;
> - break;
> + if (error == 0) {
> + gotexp = 0;
> + memset(, 0, sizeof(export));
> + if (vfs_getopt(mp->mnt_optnew, "export.exflags", ,
> + ) == 0) {
> + gotexp = 1;
> + export.ex_flags = strtouq(bufp, , 0);
> + if (endptr == bufp)
> + export_error = EINVAL;
> + }
This pattern involving
Re: which way to update export_args structure?
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.nofsid22018-10-16 23:45:33.540348000 -0400
+++ kern/vfs_mount.c2018-10-19 20:01:14.92737 -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;
opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
TAILQ_INIT(opts);
@@ -325,7 +326,7 @@ 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);
@@ -335,6 +336,8 @@ vfs_buildopts(struct uio *auio, struct v
if (error)
goto bad;
}
+ cp = (char *)opt->value;
+ cp[optlen] = '\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;
ASSERT_VOP_ELOCKED(vp, __func__);
KASSERT((fsflags & MNT_UPDATE) != 0, ("MNT_UPDATE should be here"));
@@ -1033,36 +1038,117 @@ vfs_domount_update(
export_error = 0;
/* Process the export option. */
- if (error == 0 && vfs_getopt(mp->mnt_optnew, "export", ,
- ) == 0) {
- /* Assume that there is only 1 ABI for each length. */
- switch (len) {
- case (sizeof(struct oexport_args)):
- case (sizeof(struct o2export_args)):
- memset(, 0, sizeof(export));
- memset(, 0, sizeof(o2export));
- memcpy(, 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, );
- break;
- case (sizeof(export)):
- bcopy(bufp, , len);
- export_error = vfs_export(mp, );
- break;
- default:
- export_error = EINVAL;
- break;
+ if (error == 0) {
+ gotexp = 0;
+ memset(, 0, sizeof(export));
+ if (vfs_getopt(mp->mnt_optnew, "export.exflags", ,
+ ) == 0) {
+ gotexp = 1;
+ export.ex_flags = strtouq(bufp, , 0);
+ if (endptr == bufp)
+ export_error = EINVAL;
+ }
+ if (vfs_getopt(mp->mnt_optnew, "export.root", ,
+ ) == 0) {
+ gotexp = 1;
+ export.ex_root
Re: which way to update export_args structure?
Brooks Davis wrote:
>On Wed, Oct 03, 2018 at 12:40:27AM +, Rick Macklem 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.
>
>I see you've found a way to do compatibility for a new ABI. If you
>wanted to avoid changing the struct size, there is 3 bytes of usable
>padding after each ex_addrlen and ex_masklen.
Actually, you want the size to change, since that is how the code detects
a different version of the struct. (Take a look around line# 1037 of
vfs_mount.c).
The additions are a lot more than 6bytes. The reason I was a little hesitant to
change ex_flags to 64bits is that it makes the compatibility code a little
messier,
but it isn't that bad.
The tricky one is vfs_oexport_conv(), because it doesn't know the size of the
struct being passed in via a pointer. My current solution is to have this
function
remain in place for old file system binaries only and add a new function with
a different name (and takes a struct length argument as well as the pointer)
for the new code. This function is only used by three file systems to handle the
old pre-nmount(2) syscall.
>One general question: why does export_args still exist as an interface
>between userspace and the kernel? It's passed via nmount so it seems
>like the individual entries should be elements in the vector instead.
>This would be much friendlier if one wanted to do 32-bit compat support
>for mountd.
Not sure what you are thinking of here. Right now "struct export_args" is the
data
for a mount option
Re: which way to update export_args structure?
On Wed, Oct 03, 2018 at 12:40:27AM +, Rick Macklem 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.
I see you've found a way to do compatibility for a new ABI. If you
wanted to avoid changing the struct size, there is 3 bytes of usable
padding after each ex_addrlen and ex_masklen.
One general question: why does export_args still exist as an interface
between userspace and the kernel? It's passed via nmount so it seems
like the individual entries should be elements in the vector instead.
This would be much friendlier if one wanted to do 32-bit compat support
for mountd.
-- Brooks
signature.asc
Description: PGP signature
Re: which way to update export_args structure?
Gary Jennejohn wrote: [stuff snipped] >In B, shouldn't ex_flags become uint32_t if all 32 bits can contain >flag bits? You could. For B) my intent was to leave the structure exactly the same as the old versions and only add new fields at the end. The current compatibility code in head does bcopy() of the structure, which means a change to uint32_t would require that to be re-written. (For A) it gets re-written anyhow.) > And why make ex_flagshighbits a uint64_t? You could >make it uint32_t and simply shift the high bits around. Again, you could. I didn't do that, since the code then does require bit shifts every time you test a flag and I thought that would look messy. I also get worried that compilers will do weird things like truncate the constant to 32bits before the shift and drop the flag. (I do test cases for these in a little program, but can only test i386.) (The flags are constants defined in mount.h as MNT_xx for 64bits.) Since I figured out a way that I think allows A) to be MFC'd, I'm leaning towards A) anyhow. rick ___ freebsd-current@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
On Wed, 3 Oct 2018 00:40:27 +
Rick Macklem 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
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: which way to update export_args structure?
I 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;-).
I think I have now figured out a way to fix vfs_oexport_conv() so that it can
be MFC'd.
>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.
No longer an advantage, since I have figured out how to handle A) so that it
can be MFC'd too.
>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.
I'd still like to hear any opinions w.r.t. which is better? rick
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
which way to update export_args structure?
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.
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
