| Message ID | 20121012120030.18411.qmail@md.dent.med.uni-muenchen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Oct 12, 2012, at 8:00 AM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote: > When the NFS version isn't specified in the mount options, mount.nfs > attempts V4 first and appends 'vers=4' to the extra_options string in > the mount options. If the server isn't immediately reachable, this > attempt fails. However, if the background option is specified and the > server comes up later on, the extra_options are used again for all > further attempts and thus they fail if the server only supports > vers<4. > > Fix this by only amending extra_options on a successful vers=4 mount. > > This is now Debian bug #690181 and has apparently been around for > ages. > > Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de> > > --- utils/mount/stropts.c.orig 2012-08-23 19:41:56.000000000 +0200 > +++ utils/mount/stropts.c 2012-10-11 13:46:25.000000000 +0200 > @@ -680,6 +680,7 @@ > { > struct mount_options *options = po_dup(mi->options); > int result = 0; > + char *extra_opts = NULL; > > if (!options) { > errno = ENOMEM; > @@ -715,20 +716,26 @@ > goto out_fail; > } > > - /* > - * Update option string to be recorded in /etc/mtab. > - */ > - if (po_join(options, mi->extra_opts) == PO_FAILED) { > + if (po_join(options, &extra_opts) == PO_FAILED) { This doesn't look right to me, but I haven't had time to test it. Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts? > errno = ENOMEM; > goto out_fail; > } > > if (verbose) > printf(_("%s: trying text-based options '%s'\n"), > - progname, *mi->extra_opts); > + progname, extra_opts); > result = nfs_sys_mount(mi, options); > > + /* > + * If success, update option string to be recorded in /etc/mtab. > + */ > + if (result) { > + free(*mi->extra_opts); > + *mi->extra_opts = extra_opts; > + } else > + free(extra_opts); > + > out_fail: > po_destroy(options); > return result; > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > > - /* > > - * Update option string to be recorded in /etc/mtab. > > - */ > > - if (po_join(options, mi->extra_opts) == PO_FAILED) { > > + if (po_join(options, &extra_opts) == PO_FAILED) { > > This doesn't look right to me, but I haven't had time to test it. Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts? ... > > result = nfs_sys_mount(mi, options); No, nfs_sys_mount() does not use mi->extra_opts at all, only the binary options. It would perhaps be clearer to handle the update of mi->extra_opts in nfs_sys_mount(), but only after a successful mount(2) call. A more invasive patch. Regards, Wolfram. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 13, 2012, at 1:18 PM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote: > Hi, > >>> - /* >>> - * Update option string to be recorded in /etc/mtab. >>> - */ >>> - if (po_join(options, mi->extra_opts) == PO_FAILED) { >>> + if (po_join(options, &extra_opts) == PO_FAILED) { >> >> This doesn't look right to me, but I haven't had time to test it. Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts? > ... >>> result = nfs_sys_mount(mi, options); > > No, nfs_sys_mount() does not use mi->extra_opts at all, only the > binary options. This is the text-based code, which I wrote. nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object. That string contains all the FS-specific mount options specified by the user. But your patch makes that string empty, by my reading. I think this is incorrect. > It would perhaps be clearer to handle the update of mi->extra_opts in > nfs_sys_mount(), but only after a successful mount(2) call. > A more invasive patch. > > Regards, > Wolfram. -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > >>> result = nfs_sys_mount(mi, options); > > > > No, nfs_sys_mount() does not use mi->extra_opts at all, only the > > binary options. > > This is the text-based code, which I wrote. nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object. That string contains all the FS-specific mount options specified by the user. > > But your patch makes that string empty, by my reading. I think this is incorrect. Ok, I'm happy to go through this line-by-line. static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) { char *options = NULL; int result; if (mi->fake) return 1; if (po_join(opts, &options) == PO_FAILED) { // HERE errno = EIO; return 0; } result = mount(mi->spec, mi->node, mi->type, mi->flags & ~(MS_USER|MS_USERS), options); ... } nfs_sys_mount() constructs the text options _itself_ purely from the opts (2nd) argument HERE -- po_join has opts as input and options as output. My patch only changes the first argument (mi). So, no functional change within nfs_sys_mount() at all. The functional change is that with my patch, mi->extra_opts is kept unchanged unless the system call is successful. mi->extra_opts is actually reused later throughout the mount program, because nfsmount_string() has extra_opts as an input _and_ output argument, and propagates mi->extra_opts into the extra_opts variable in main.c. I have actually tested this patch extensively and it fixes the problem. Regards, Wolfram. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 13, 2012, at 5:46 PM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote: > Hi, > >>>>> result = nfs_sys_mount(mi, options); >>> >>> No, nfs_sys_mount() does not use mi->extra_opts at all, only the >>> binary options. >> >> This is the text-based code, which I wrote. nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object. That string contains all the FS-specific mount options specified by the user. >> >> But your patch makes that string empty, by my reading. I think this is incorrect. > > Ok, I'm happy to go through this line-by-line. > > static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) > { > char *options = NULL; > int result; > > if (mi->fake) > return 1; > > if (po_join(opts, &options) == PO_FAILED) { // HERE > errno = EIO; > return 0; > } > > result = mount(mi->spec, mi->node, mi->type, > mi->flags & ~(MS_USER|MS_USERS), options); > ... > } > > nfs_sys_mount() constructs the text options _itself_ purely from the > opts (2nd) argument HERE -- po_join has opts as input and options as > output. > > My patch only changes the first argument (mi). So, no functional change > within nfs_sys_mount() at all. Agreed. > The functional change is that with my patch, mi->extra_opts is kept > unchanged unless the system call is successful. mi->extra_opts is > actually reused later throughout the mount program, because > nfsmount_string() has extra_opts as an input _and_ output argument, > and propagates mi->extra_opts into the extra_opts variable in main.c. > > I have actually tested this patch extensively and it fixes the > problem. Unfortunately there are a rather unimaginable number of use cases for mount.nfs, which is why it is so complicated. "Extensively" could mean you've tested it for a long time but with just a few use cases. In any event: Reviewed-by: Chuck Lever <chuck.lever@oracle.com> -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > Unfortunately there are a rather unimaginable number of use cases for mount.nfs, which is why it is so complicated. "Extensively" could mean you've tested it for a long time but with just a few use cases. Admitted, I certainly didn't run an extensive test suite, mainly just the case in question by simulating server down with a temporary iptables DROP rule. > In any event: > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com> Thanks, Wolfram. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/10/12 08:00, Wolfram Gloger wrote: > When the NFS version isn't specified in the mount options, mount.nfs > attempts V4 first and appends 'vers=4' to the extra_options string in > the mount options. If the server isn't immediately reachable, this > attempt fails. However, if the background option is specified and the > server comes up later on, the extra_options are used again for all > further attempts and thus they fail if the server only supports > vers<4. > > Fix this by only amending extra_options on a successful vers=4 mount. > > This is now Debian bug #690181 and has apparently been around for > ages. > > Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de> Committed.... steved. > > --- utils/mount/stropts.c.orig 2012-08-23 19:41:56.000000000 +0200 > +++ utils/mount/stropts.c 2012-10-11 13:46:25.000000000 +0200 > @@ -680,6 +680,7 @@ > { > struct mount_options *options = po_dup(mi->options); > int result = 0; > + char *extra_opts = NULL; > > if (!options) { > errno = ENOMEM; > @@ -715,20 +716,26 @@ > goto out_fail; > } > > - /* > - * Update option string to be recorded in /etc/mtab. > - */ > - if (po_join(options, mi->extra_opts) == PO_FAILED) { > + if (po_join(options, &extra_opts) == PO_FAILED) { > errno = ENOMEM; > goto out_fail; > } > > if (verbose) > printf(_("%s: trying text-based options '%s'\n"), > - progname, *mi->extra_opts); > + progname, extra_opts); > > result = nfs_sys_mount(mi, options); > > + /* > + * If success, update option string to be recorded in /etc/mtab. > + */ > + if (result) { > + free(*mi->extra_opts); > + *mi->extra_opts = extra_opts; > + } else > + free(extra_opts); > + > out_fail: > po_destroy(options); > return result; > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- utils/mount/stropts.c.orig 2012-08-23 19:41:56.000000000 +0200 +++ utils/mount/stropts.c 2012-10-11 13:46:25.000000000 +0200 @@ -680,6 +680,7 @@ { struct mount_options *options = po_dup(mi->options); int result = 0; + char *extra_opts = NULL; if (!options) { errno = ENOMEM; @@ -715,20 +716,26 @@ goto out_fail; } - /* - * Update option string to be recorded in /etc/mtab. - */ - if (po_join(options, mi->extra_opts) == PO_FAILED) { + if (po_join(options, &extra_opts) == PO_FAILED) { errno = ENOMEM; goto out_fail; } if (verbose) printf(_("%s: trying text-based options '%s'\n"), - progname, *mi->extra_opts); + progname, extra_opts); result = nfs_sys_mount(mi, options); + /* + * If success, update option string to be recorded in /etc/mtab. + */ + if (result) { + free(*mi->extra_opts); + *mi->extra_opts = extra_opts; + } else + free(extra_opts); + out_fail: po_destroy(options); return result;
When the NFS version isn't specified in the mount options, mount.nfs attempts V4 first and appends 'vers=4' to the extra_options string in the mount options. If the server isn't immediately reachable, this attempt fails. However, if the background option is specified and the server comes up later on, the extra_options are used again for all further attempts and thus they fail if the server only supports vers<4. Fix this by only amending extra_options on a successful vers=4 mount. This is now Debian bug #690181 and has apparently been around for ages. Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html