| Message ID | 1307740096-19933-5-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > Look for and parse the srcaddr=n argument. If parsing > succeeds, pass this down the call chain. This fully > implements binding to a specified source address when > mounting. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > :100644 100644 71417df... aba4252... M utils/mount/stropts.c > utils/mount/stropts.c | 29 +++++++++++++++++++++++++---- > 1 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 71417df..aba4252 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -92,6 +92,7 @@ struct nfsmount_info { > int flags, /* MS_ flags */ > fake, /* actually do the mount? */ > child; /* forked bg child? */ > + struct local_bind_info *local_ip; /* Local IP binding info */ > }; > > #ifdef MOUNT_CONFIG > @@ -345,6 +346,7 @@ static int nfs_validate_options(struct nfsmount_info *mi) > }; > sa_family_t family; > int error; > + char *option; > > if (!nfs_parse_devname(mi->spec, &mi->hostname, NULL)) > return 0; > @@ -371,6 +373,20 @@ static int nfs_validate_options(struct nfsmount_info *mi) > mi->address->ai_addrlen, mi->options)) > return 0; > > + option = po_get(mi->options, "srcaddr"); > + if (option) { > + struct local_bind_info *local_ip; > + local_ip = malloc(sizeof(*local_ip)); > + memset(local_ip, 0, sizeof(*local_ip)); > + parse_local_bind(local_ip, option); Nit: Steve may not agree with this, possibly, but it is better IMO if we name new functions like parse_local_bind() with an nfs_ prefix so that, when debugging, we can immediately tell in a backtrace what functions are in mount.nfs and which are in a system library. > + > + if (!local_ip->is_set) { > + free(local_ip); > + return 0; > + } > + mi->local_ip = local_ip; > + } I'm wondering what kind of sanity checking is done on the srcaddr value. 1. Do we verify that srcaddr == clientaddr? 2. Do we verify that srcaddr.sa_family == addr.sa_family ? > + > return 1; > } > > @@ -484,7 +500,8 @@ static int nfs_construct_new_options(struct mount_options *options, > * FALSE is returned if some failure occurred. > */ > static int > -nfs_rewrite_pmap_mount_options(struct mount_options *options) > +nfs_rewrite_pmap_mount_options(struct mount_options *options, > + struct local_bind_info *local_ip) > { > union nfs_sockaddr nfs_address; > struct sockaddr *nfs_saddr = &nfs_address.sa; > @@ -534,7 +551,8 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options) > * negotiate. Bail now if we can't contact it. > */ > if (!nfs_probe_bothports(mnt_saddr, mnt_salen, &mnt_pmap, > - nfs_saddr, nfs_salen, &nfs_pmap, NULL)) { > + nfs_saddr, nfs_salen, &nfs_pmap, > + local_ip)) { > errno = ESPIPE; > if (rpc_createerr.cf_stat == RPC_PROGNOTREGISTERED) > errno = EOPNOTSUPP; > @@ -589,7 +607,7 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) > } > > static int nfs_do_mount_v3v2(struct nfsmount_info *mi, > - struct sockaddr *sap, socklen_t salen) > + struct sockaddr *sap, socklen_t salen) > { > struct mount_options *options = po_dup(mi->options); > int result = 0; > @@ -631,7 +649,7 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi, > printf(_("%s: trying text-based options '%s'\n"), > progname, *mi->extra_opts); > > - if (!nfs_rewrite_pmap_mount_options(options)) > + if (!nfs_rewrite_pmap_mount_options(options, mi->local_ip)) > goto out_fail; > > result = nfs_sys_mount(mi, options); > @@ -1039,6 +1057,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, > .flags = flags, > .fake = fake, > .child = child, > + .local_ip = NULL, > }; > int retval = EX_FAIL; > > @@ -1051,5 +1070,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, > > freeaddrinfo(mi.address); > free(mi.hostname); > + if (mi.local_ip) > + free(mi.local_ip); Nit: free(3) works when passed NULL, you don't need the extra "if (mi.local_ip)" in front of it. > return retval; > } > -- > 1.7.3.4 > > -- > 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 06/10/2011 03:07 PM, Chuck Lever wrote: > > On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote: > >> From: Ben Greear<greearb@candelatech.com> >> >> Look for and parse the srcaddr=n argument. If parsing >> succeeds, pass this down the call chain. This fully >> implements binding to a specified source address when >> mounting. >> + if (!local_ip->is_set) { >> + free(local_ip); >> + return 0; >> + } >> + mi->local_ip = local_ip; >> + } > > I'm wondering what kind of sanity checking is done on the srcaddr value. > > 1. Do we verify that srcaddr == clientaddr? No, and I'm not sure we should. If they are specifying both srcaddr and clientaddr, they are already in the rarely-used-options category, so maybe they know what they are doing. And, if it clientaddr is automatically figured out by the kernel, then I think it must necessarily always be srcaddr. Makes me wonder though..could you do some sort of lame security violation by making clientaddr some third-party IP? > > 2. Do we verify that srcaddr.sa_family == addr.sa_family ? Not directly, but it will blow up in the bind() call if you try it: strace -f mount -t nfs [2002::100:157]:/rpool/ben /mnt/lf/znfs36-sol-1 -o srcaddr=192.168.100.117,vers=3 ... [pid 1488] munmap(0x7f0cd7b20000, 4096) = 0 [pid 1488] socket(PF_INET6, SOCK_STREAM, IPPROTO_TCP) = 3 [pid 1488] bind(3, {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("192.168.100.117")}, 16) = -1 EINVAL (Invalid argument) [root@ice-si-dmz fileio]# mount -t nfs [2002::100:157]:/rpool/ben /mnt/lf/znfs36-sol-1 -o srcaddr=192.168.100.117,vers=3 mount.nfs: an incorrect mount option was specified That sufficient you think? Thanks, Ben
On Jun 10, 2011, at 6:30 PM, Ben Greear wrote: > On 06/10/2011 03:07 PM, Chuck Lever wrote: >> >> On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote: >> >>> From: Ben Greear<greearb@candelatech.com> >>> >>> Look for and parse the srcaddr=n argument. If parsing >>> succeeds, pass this down the call chain. This fully >>> implements binding to a specified source address when >>> mounting. > >>> + if (!local_ip->is_set) { >>> + free(local_ip); >>> + return 0; >>> + } >>> + mi->local_ip = local_ip; >>> + } >> >> I'm wondering what kind of sanity checking is done on the srcaddr value. >> >> 1. Do we verify that srcaddr == clientaddr? > > No, and I'm not sure we should. If they are specifying > both srcaddr and clientaddr, they are already in the rarely-used-options > category, so maybe they know what they are doing. > > And, if it clientaddr is automatically figured out by > the kernel, then I think it must necessarily always be > srcaddr. Today the kernel doesn't automatically figure out a value for clientaddr=, that value is provided by the mount.nfs command. I guess no harm is done if they are different, but you probably want to remove any comments or other documentation that suggests that is bad. Maybe you've already done that. > Makes me wonder though..could you do some sort of lame security > violation by making clientaddr some third-party IP? > >> >> 2. Do we verify that srcaddr.sa_family == addr.sa_family ? > > Not directly, but it will blow up in the bind() call if you > try it: > > strace -f mount -t nfs [2002::100:157]:/rpool/ben /mnt/lf/znfs36-sol-1 -o srcaddr=192.168.100.117,vers=3 > ... > [pid 1488] munmap(0x7f0cd7b20000, 4096) = 0 > [pid 1488] socket(PF_INET6, SOCK_STREAM, IPPROTO_TCP) = 3 > [pid 1488] bind(3, {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("192.168.100.117")}, 16) = -1 EINVAL (Invalid argument) > > [root@ice-si-dmz fileio]# mount -t nfs [2002::100:157]:/rpool/ben /mnt/lf/znfs36-sol-1 -o srcaddr=192.168.100.117,vers=3 > mount.nfs: an incorrect mount option was specified > > That sufficient you think? I think it would be more helpful to admins if mount.nfs were more explicit about the problem.
On 06/10/2011 06:07 PM, Chuck Lever wrote: > > On Jun 10, 2011, at 5:08 PM, greearb@candelatech.com wrote: > >> From: Ben Greear <greearb@candelatech.com> >> >> Look for and parse the srcaddr=n argument. If parsing >> succeeds, pass this down the call chain. This fully >> implements binding to a specified source address when >> mounting. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> :100644 100644 71417df... aba4252... M utils/mount/stropts.c >> utils/mount/stropts.c | 29 +++++++++++++++++++++++++---- >> 1 files changed, 25 insertions(+), 4 deletions(-) >> >> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c >> index 71417df..aba4252 100644 >> --- a/utils/mount/stropts.c >> +++ b/utils/mount/stropts.c >> @@ -92,6 +92,7 @@ struct nfsmount_info { >> int flags, /* MS_ flags */ >> fake, /* actually do the mount? */ >> child; /* forked bg child? */ >> + struct local_bind_info *local_ip; /* Local IP binding info */ >> }; >> >> #ifdef MOUNT_CONFIG >> @@ -345,6 +346,7 @@ static int nfs_validate_options(struct nfsmount_info *mi) >> }; >> sa_family_t family; >> int error; >> + char *option; >> >> if (!nfs_parse_devname(mi->spec, &mi->hostname, NULL)) >> return 0; >> @@ -371,6 +373,20 @@ static int nfs_validate_options(struct nfsmount_info *mi) >> mi->address->ai_addrlen, mi->options)) >> return 0; >> >> + option = po_get(mi->options, "srcaddr"); >> + if (option) { >> + struct local_bind_info *local_ip; >> + local_ip = malloc(sizeof(*local_ip)); >> + memset(local_ip, 0, sizeof(*local_ip)); >> + parse_local_bind(local_ip, option); > > Nit: Steve may not agree with this, possibly, but it is better IMO if we name new functions like parse_local_bind() with an nfs_ prefix so that, when debugging, we can immediately tell in a backtrace what functions are in mount.nfs and which are in a system library. I do agree... We might as well stay with the naming convention that already been established... steved. > >> + >> + if (!local_ip->is_set) { >> + free(local_ip); >> + return 0; >> + } >> + mi->local_ip = local_ip; >> + } > > I'm wondering what kind of sanity checking is done on the srcaddr value. > > 1. Do we verify that srcaddr == clientaddr? > > 2. Do we verify that srcaddr.sa_family == addr.sa_family ? > >> + >> return 1; >> } >> >> @@ -484,7 +500,8 @@ static int nfs_construct_new_options(struct mount_options *options, >> * FALSE is returned if some failure occurred. >> */ >> static int >> -nfs_rewrite_pmap_mount_options(struct mount_options *options) >> +nfs_rewrite_pmap_mount_options(struct mount_options *options, >> + struct local_bind_info *local_ip) >> { >> union nfs_sockaddr nfs_address; >> struct sockaddr *nfs_saddr = &nfs_address.sa; >> @@ -534,7 +551,8 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options) >> * negotiate. Bail now if we can't contact it. >> */ >> if (!nfs_probe_bothports(mnt_saddr, mnt_salen, &mnt_pmap, >> - nfs_saddr, nfs_salen, &nfs_pmap, NULL)) { >> + nfs_saddr, nfs_salen, &nfs_pmap, >> + local_ip)) { >> errno = ESPIPE; >> if (rpc_createerr.cf_stat == RPC_PROGNOTREGISTERED) >> errno = EOPNOTSUPP; >> @@ -589,7 +607,7 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) >> } >> >> static int nfs_do_mount_v3v2(struct nfsmount_info *mi, >> - struct sockaddr *sap, socklen_t salen) >> + struct sockaddr *sap, socklen_t salen) >> { >> struct mount_options *options = po_dup(mi->options); >> int result = 0; >> @@ -631,7 +649,7 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi, >> printf(_("%s: trying text-based options '%s'\n"), >> progname, *mi->extra_opts); >> >> - if (!nfs_rewrite_pmap_mount_options(options)) >> + if (!nfs_rewrite_pmap_mount_options(options, mi->local_ip)) >> goto out_fail; >> >> result = nfs_sys_mount(mi, options); >> @@ -1039,6 +1057,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, >> .flags = flags, >> .fake = fake, >> .child = child, >> + .local_ip = NULL, >> }; >> int retval = EX_FAIL; >> >> @@ -1051,5 +1070,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, >> >> freeaddrinfo(mi.address); >> free(mi.hostname); >> + if (mi.local_ip) >> + free(mi.local_ip); > > Nit: free(3) works when passed NULL, you don't need the extra "if (mi.local_ip)" in front of it. > >> return retval; >> } >> -- >> 1.7.3.4 >> >> -- >> 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
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c index 71417df..aba4252 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -92,6 +92,7 @@ struct nfsmount_info { int flags, /* MS_ flags */ fake, /* actually do the mount? */ child; /* forked bg child? */ + struct local_bind_info *local_ip; /* Local IP binding info */ }; #ifdef MOUNT_CONFIG @@ -345,6 +346,7 @@ static int nfs_validate_options(struct nfsmount_info *mi) }; sa_family_t family; int error; + char *option; if (!nfs_parse_devname(mi->spec, &mi->hostname, NULL)) return 0; @@ -371,6 +373,20 @@ static int nfs_validate_options(struct nfsmount_info *mi) mi->address->ai_addrlen, mi->options)) return 0; + option = po_get(mi->options, "srcaddr"); + if (option) { + struct local_bind_info *local_ip; + local_ip = malloc(sizeof(*local_ip)); + memset(local_ip, 0, sizeof(*local_ip)); + parse_local_bind(local_ip, option); + + if (!local_ip->is_set) { + free(local_ip); + return 0; + } + mi->local_ip = local_ip; + } + return 1; } @@ -484,7 +500,8 @@ static int nfs_construct_new_options(struct mount_options *options, * FALSE is returned if some failure occurred. */ static int -nfs_rewrite_pmap_mount_options(struct mount_options *options) +nfs_rewrite_pmap_mount_options(struct mount_options *options, + struct local_bind_info *local_ip) { union nfs_sockaddr nfs_address; struct sockaddr *nfs_saddr = &nfs_address.sa; @@ -534,7 +551,8 @@ nfs_rewrite_pmap_mount_options(struct mount_options *options) * negotiate. Bail now if we can't contact it. */ if (!nfs_probe_bothports(mnt_saddr, mnt_salen, &mnt_pmap, - nfs_saddr, nfs_salen, &nfs_pmap, NULL)) { + nfs_saddr, nfs_salen, &nfs_pmap, + local_ip)) { errno = ESPIPE; if (rpc_createerr.cf_stat == RPC_PROGNOTREGISTERED) errno = EOPNOTSUPP; @@ -589,7 +607,7 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) } static int nfs_do_mount_v3v2(struct nfsmount_info *mi, - struct sockaddr *sap, socklen_t salen) + struct sockaddr *sap, socklen_t salen) { struct mount_options *options = po_dup(mi->options); int result = 0; @@ -631,7 +649,7 @@ static int nfs_do_mount_v3v2(struct nfsmount_info *mi, printf(_("%s: trying text-based options '%s'\n"), progname, *mi->extra_opts); - if (!nfs_rewrite_pmap_mount_options(options)) + if (!nfs_rewrite_pmap_mount_options(options, mi->local_ip)) goto out_fail; result = nfs_sys_mount(mi, options); @@ -1039,6 +1057,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, .flags = flags, .fake = fake, .child = child, + .local_ip = NULL, }; int retval = EX_FAIL; @@ -1051,5 +1070,7 @@ int nfsmount_string(const char *spec, const char *node, const char *type, freeaddrinfo(mi.address); free(mi.hostname); + if (mi.local_ip) + free(mi.local_ip); return retval; }