| Message ID | 20140218154406.GD4319@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
18.02.2014 19:44, J. Bruce Fields ?????: > On Tue, Feb 18, 2014 at 07:19:31PM +0400, Stanislav Kinsbursky wrote: >> 18.02.2014 02:19, J. Bruce Fields ?????: >>> On Sat, Feb 15, 2014 at 09:51:20AM +0800, Weng Meiling wrote: >>>> Hi Bruce, >>>> >>>> The upstream has merged your git tree for-3.14, but there is no this patch? >>>> Do you forget this patch? >>> >>> Apologies, I'm not sure what happened. >>> >>> Looking back at it.... The patch causes all my pynfs reboot recovery >>> tests to fail. They're just doing a "systemctl restart >>> nfs-server.service", and "systemctl status nfs-server.service" shows in >>> part >>> >>> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS $RPCNFSDCOUNT (code=exited, status=1/FAILURE) >>> >>> So the patch is causing rpc.nfsd to fail? No network namespaces should >>> be involved. >>> >>> I haven't investigated any further. >>> >> >> Hi Bruce, >> Are you sure, that exactly this patch broke your pynfs tests? >> BTW, systemd manipulates namespaces. Maybe the patch revealed some pynfs internal bugs? >> What do you think? > > It's really just "systemctl restart nfs-server.service" that the patch > breaks, pynfs isn't involved much. > > The patch I'm actually using follows, but I believe the only difference > is in the printk message? > Yep, looks true. That's strange: "systemctl restart nfs-server.service" works for me on Fedora 18 with kernel, based on your repo. > --b > > commit e1f2922c12cb59baba0f2c7726bee992a0861310 > Author: Stanislav Kinsbursky <skinsbursky@parallels.com> > Date: Mon Dec 30 17:23:59 2013 +0300 > > nfsd: check passed socket's net matches the NFSd superblock's > > The file descriptor written to the nfsd/portlist file could be for a > socket in a different network namespace from the network namespace that > the nfsd filesystem was mounted in, and this can cause a crash. > > For example: "ip netns exec" creates a new network and mount namespace, > which duplicates the nfsd mount point which was created in the init_net > context. Thus NFS server stop in the nested network context leads to > RPCBIND client destruction in init_net. Then, on nfsd start in the > nested network context, the rpc.nfsd process creates a socket in the > nested net and passes it into "write_ports", which leads to RPCBIND > socket creation in init_net context for the same reason (the nfsd mount > point was created in the init_net context). An attempt to register > passed socket in nested net leads to panic, because no RPCBIND client is > present in the nested network namespace. > > This patch adds a check that the passed the socket's net matches the > nfsd superblock's net, and returns -EINVAL error to user space if not. > > Reported-by: Weng Meiling <wengmeiling.weng@huawei.com> > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> > Cc: stable@vger.kernel.org > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 7f55517..1331766 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -699,6 +699,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net) > if (err != 0 || fd < 0) > return -EINVAL; > > + if (svc_alien_sock(net, fd)) { > + printk(KERN_ERR "%s: socket net is different from NFSd's\n", __func__); > + return -EINVAL; > + } > + > err = nfsd_create_serv(net); > if (err != 0) > return err; > diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h > index 62fd1b7..947009e 100644 > --- a/include/linux/sunrpc/svcsock.h > +++ b/include/linux/sunrpc/svcsock.h > @@ -56,6 +56,7 @@ int svc_recv(struct svc_rqst *, long); > int svc_send(struct svc_rqst *); > void svc_drop(struct svc_rqst *); > void svc_sock_update_bufs(struct svc_serv *serv); > +bool svc_alien_sock(struct net *net, int fd); > int svc_addsock(struct svc_serv *serv, const int fd, > char *name_return, const size_t len); > void svc_init_xprt_sock(void); > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c > index b6e59f0..3ba5b87 100644 > --- a/net/sunrpc/svcsock.c > +++ b/net/sunrpc/svcsock.c > @@ -1397,6 +1397,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv, > return svsk; > } > > +bool svc_alien_sock(struct net *net, int fd) > +{ > + int err; > + struct socket *sock = sockfd_lookup(fd, &err); > + > + if (sock && (sock_net(sock->sk) != net)) > + return true; > + return false; > +} > +EXPORT_SYMBOL_GPL(svc_alien_sock); > + > /** > * svc_addsock - add a listener socket to an RPC service > * @serv: pointer to RPC service to which to add a new listener >
On Wed, Feb 19, 2014 at 02:26:33PM +0400, Stanislav Kinsbursky wrote: > 18.02.2014 19:44, J. Bruce Fields ?????: > >On Tue, Feb 18, 2014 at 07:19:31PM +0400, Stanislav Kinsbursky wrote: > >>18.02.2014 02:19, J. Bruce Fields ?????: > >>>On Sat, Feb 15, 2014 at 09:51:20AM +0800, Weng Meiling wrote: > >>>>Hi Bruce, > >>>> > >>>>The upstream has merged your git tree for-3.14, but there is no this patch? > >>>>Do you forget this patch? > >>> > >>>Apologies, I'm not sure what happened. > >>> > >>>Looking back at it.... The patch causes all my pynfs reboot recovery > >>>tests to fail. They're just doing a "systemctl restart > >>>nfs-server.service", and "systemctl status nfs-server.service" shows in > >>>part > >>> > >>> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS $RPCNFSDCOUNT (code=exited, status=1/FAILURE) > >>> > >>>So the patch is causing rpc.nfsd to fail? No network namespaces should > >>>be involved. > >>> > >>>I haven't investigated any further. > >>> > >> > >>Hi Bruce, > >>Are you sure, that exactly this patch broke your pynfs tests? > >>BTW, systemd manipulates namespaces. Maybe the patch revealed some pynfs internal bugs? > >>What do you think? > > > >It's really just "systemctl restart nfs-server.service" that the patch > >breaks, pynfs isn't involved much. > > > >The patch I'm actually using follows, but I believe the only difference > >is in the printk message? > > > > Yep, looks true. > That's strange: "systemctl restart nfs-server.service" works for me on Fedora 18 with kernel, based on your repo. OK, I'll take a closer look and let you know what I find. --b. > > >--b > > > >commit e1f2922c12cb59baba0f2c7726bee992a0861310 > >Author: Stanislav Kinsbursky <skinsbursky@parallels.com> > >Date: Mon Dec 30 17:23:59 2013 +0300 > > > > nfsd: check passed socket's net matches the NFSd superblock's > > > > The file descriptor written to the nfsd/portlist file could be for a > > socket in a different network namespace from the network namespace that > > the nfsd filesystem was mounted in, and this can cause a crash. > > > > For example: "ip netns exec" creates a new network and mount namespace, > > which duplicates the nfsd mount point which was created in the init_net > > context. Thus NFS server stop in the nested network context leads to > > RPCBIND client destruction in init_net. Then, on nfsd start in the > > nested network context, the rpc.nfsd process creates a socket in the > > nested net and passes it into "write_ports", which leads to RPCBIND > > socket creation in init_net context for the same reason (the nfsd mount > > point was created in the init_net context). An attempt to register > > passed socket in nested net leads to panic, because no RPCBIND client is > > present in the nested network namespace. > > > > This patch adds a check that the passed the socket's net matches the > > nfsd superblock's net, and returns -EINVAL error to user space if not. > > > > Reported-by: Weng Meiling <wengmeiling.weng@huawei.com> > > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> > > Cc: stable@vger.kernel.org > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > >diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > >index 7f55517..1331766 100644 > >--- a/fs/nfsd/nfsctl.c > >+++ b/fs/nfsd/nfsctl.c > >@@ -699,6 +699,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net) > > if (err != 0 || fd < 0) > > return -EINVAL; > > > >+ if (svc_alien_sock(net, fd)) { > >+ printk(KERN_ERR "%s: socket net is different from NFSd's\n", __func__); > >+ return -EINVAL; > >+ } > >+ > > err = nfsd_create_serv(net); > > if (err != 0) > > return err; > >diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h > >index 62fd1b7..947009e 100644 > >--- a/include/linux/sunrpc/svcsock.h > >+++ b/include/linux/sunrpc/svcsock.h > >@@ -56,6 +56,7 @@ int svc_recv(struct svc_rqst *, long); > > int svc_send(struct svc_rqst *); > > void svc_drop(struct svc_rqst *); > > void svc_sock_update_bufs(struct svc_serv *serv); > >+bool svc_alien_sock(struct net *net, int fd); > > int svc_addsock(struct svc_serv *serv, const int fd, > > char *name_return, const size_t len); > > void svc_init_xprt_sock(void); > >diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c > >index b6e59f0..3ba5b87 100644 > >--- a/net/sunrpc/svcsock.c > >+++ b/net/sunrpc/svcsock.c > >@@ -1397,6 +1397,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv, > > return svsk; > > } > > > >+bool svc_alien_sock(struct net *net, int fd) > >+{ > >+ int err; > >+ struct socket *sock = sockfd_lookup(fd, &err); > >+ > >+ if (sock && (sock_net(sock->sk) != net)) > >+ return true; > >+ return false; > >+} > >+EXPORT_SYMBOL_GPL(svc_alien_sock); > >+ > > /** > > * svc_addsock - add a listener socket to an RPC service > > * @serv: pointer to RPC service to which to add a new listener > > > > > -- > Best regards, > Stanislav Kinsbursky -- 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
19.02.2014 18:50, J. Bruce Fields ?????: > On Wed, Feb 19, 2014 at 02:26:33PM +0400, Stanislav Kinsbursky wrote: >> 18.02.2014 19:44, J. Bruce Fields ?????: >>> On Tue, Feb 18, 2014 at 07:19:31PM +0400, Stanislav Kinsbursky wrote: >>>> 18.02.2014 02:19, J. Bruce Fields ?????: >>>>> On Sat, Feb 15, 2014 at 09:51:20AM +0800, Weng Meiling wrote: >>>>>> Hi Bruce, >>>>>> >>>>>> The upstream has merged your git tree for-3.14, but there is no this patch? >>>>>> Do you forget this patch? >>>>> >>>>> Apologies, I'm not sure what happened. >>>>> >>>>> Looking back at it.... The patch causes all my pynfs reboot recovery >>>>> tests to fail. They're just doing a "systemctl restart >>>>> nfs-server.service", and "systemctl status nfs-server.service" shows in >>>>> part >>>>> >>>>> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS $RPCNFSDCOUNT (code=exited, status=1/FAILURE) >>>>> >>>>> So the patch is causing rpc.nfsd to fail? No network namespaces should >>>>> be involved. >>>>> >>>>> I haven't investigated any further. >>>>> >>>> >>>> Hi Bruce, >>>> Are you sure, that exactly this patch broke your pynfs tests? >>>> BTW, systemd manipulates namespaces. Maybe the patch revealed some pynfs internal bugs? >>>> What do you think? >>> >>> It's really just "systemctl restart nfs-server.service" that the patch >>> breaks, pynfs isn't involved much. >>> >>> The patch I'm actually using follows, but I believe the only difference >>> is in the printk message? >>> >> >> Yep, looks true. >> That's strange: "systemctl restart nfs-server.service" works for me on Fedora 18 with kernel, based on your repo. > > OK, I'll take a closer look and let you know what I find. > Thank you, Bruce! > --b. > >> >>> --b >>> >>> commit e1f2922c12cb59baba0f2c7726bee992a0861310 >>> Author: Stanislav Kinsbursky <skinsbursky@parallels.com> >>> Date: Mon Dec 30 17:23:59 2013 +0300 >>> >>> nfsd: check passed socket's net matches the NFSd superblock's >>> >>> The file descriptor written to the nfsd/portlist file could be for a >>> socket in a different network namespace from the network namespace that >>> the nfsd filesystem was mounted in, and this can cause a crash. >>> >>> For example: "ip netns exec" creates a new network and mount namespace, >>> which duplicates the nfsd mount point which was created in the init_net >>> context. Thus NFS server stop in the nested network context leads to >>> RPCBIND client destruction in init_net. Then, on nfsd start in the >>> nested network context, the rpc.nfsd process creates a socket in the >>> nested net and passes it into "write_ports", which leads to RPCBIND >>> socket creation in init_net context for the same reason (the nfsd mount >>> point was created in the init_net context). An attempt to register >>> passed socket in nested net leads to panic, because no RPCBIND client is >>> present in the nested network namespace. >>> >>> This patch adds a check that the passed the socket's net matches the >>> nfsd superblock's net, and returns -EINVAL error to user space if not. >>> >>> Reported-by: Weng Meiling <wengmeiling.weng@huawei.com> >>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> >>> Cc: stable@vger.kernel.org >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> >>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >>> index 7f55517..1331766 100644 >>> --- a/fs/nfsd/nfsctl.c >>> +++ b/fs/nfsd/nfsctl.c >>> @@ -699,6 +699,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net) >>> if (err != 0 || fd < 0) >>> return -EINVAL; >>> >>> + if (svc_alien_sock(net, fd)) { >>> + printk(KERN_ERR "%s: socket net is different from NFSd's\n", __func__); >>> + return -EINVAL; >>> + } >>> + >>> err = nfsd_create_serv(net); >>> if (err != 0) >>> return err; >>> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h >>> index 62fd1b7..947009e 100644 >>> --- a/include/linux/sunrpc/svcsock.h >>> +++ b/include/linux/sunrpc/svcsock.h >>> @@ -56,6 +56,7 @@ int svc_recv(struct svc_rqst *, long); >>> int svc_send(struct svc_rqst *); >>> void svc_drop(struct svc_rqst *); >>> void svc_sock_update_bufs(struct svc_serv *serv); >>> +bool svc_alien_sock(struct net *net, int fd); >>> int svc_addsock(struct svc_serv *serv, const int fd, >>> char *name_return, const size_t len); >>> void svc_init_xprt_sock(void); >>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c >>> index b6e59f0..3ba5b87 100644 >>> --- a/net/sunrpc/svcsock.c >>> +++ b/net/sunrpc/svcsock.c >>> @@ -1397,6 +1397,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv, >>> return svsk; >>> } >>> >>> +bool svc_alien_sock(struct net *net, int fd) >>> +{ >>> + int err; >>> + struct socket *sock = sockfd_lookup(fd, &err); >>> + >>> + if (sock && (sock_net(sock->sk) != net)) >>> + return true; >>> + return false; >>> +} >>> +EXPORT_SYMBOL_GPL(svc_alien_sock); >>> + >>> /** >>> * svc_addsock - add a listener socket to an RPC service >>> * @serv: pointer to RPC service to which to add a new listener >>> >> >> >> -- >> Best regards, >> Stanislav Kinsbursky
On Wed, Feb 19, 2014 at 06:57:46PM +0400, Stanislav Kinsbursky wrote: > 19.02.2014 18:50, J. Bruce Fields ?????: > >On Wed, Feb 19, 2014 at 02:26:33PM +0400, Stanislav Kinsbursky wrote: > >>18.02.2014 19:44, J. Bruce Fields ?????: > >>>On Tue, Feb 18, 2014 at 07:19:31PM +0400, Stanislav Kinsbursky wrote: > >>>>18.02.2014 02:19, J. Bruce Fields ?????: > >>>>>On Sat, Feb 15, 2014 at 09:51:20AM +0800, Weng Meiling wrote: > >>>>>>Hi Bruce, > >>>>>> > >>>>>>The upstream has merged your git tree for-3.14, but there is no this patch? > >>>>>>Do you forget this patch? > >>>>> > >>>>>Apologies, I'm not sure what happened. > >>>>> > >>>>>Looking back at it.... The patch causes all my pynfs reboot recovery > >>>>>tests to fail. They're just doing a "systemctl restart > >>>>>nfs-server.service", and "systemctl status nfs-server.service" shows in > >>>>>part > >>>>> > >>>>> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS $RPCNFSDCOUNT (code=exited, status=1/FAILURE) > >>>>> > >>>>>So the patch is causing rpc.nfsd to fail? No network namespaces should > >>>>>be involved. > >>>>> > >>>>>I haven't investigated any further. > >>>>> > >>>> > >>>>Hi Bruce, > >>>>Are you sure, that exactly this patch broke your pynfs tests? > >>>>BTW, systemd manipulates namespaces. Maybe the patch revealed some pynfs internal bugs? > >>>>What do you think? > >>> > >>>It's really just "systemctl restart nfs-server.service" that the patch > >>>breaks, pynfs isn't involved much. > >>> > >>>The patch I'm actually using follows, but I believe the only difference > >>>is in the printk message? > >>> > >> > >>Yep, looks true. > >>That's strange: "systemctl restart nfs-server.service" works for me on Fedora 18 with kernel, based on your repo. > > > >OK, I'll take a closer look and let you know what I find. > > > > Thank you, Bruce! rpc.nfsd's attempt to bind port 2049 the second time is failing with EADDRINUSE. svc_alien_sock isn't failing, so the only different here is that we're running sockfd_lookup.... Does that take a reference on the fd or the sock that needs to be put? --b. -- 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
21.02.2014 01:31, J. Bruce Fields ?????: > On Wed, Feb 19, 2014 at 06:57:46PM +0400, Stanislav Kinsbursky wrote: >> 19.02.2014 18:50, J. Bruce Fields ?????: >>> On Wed, Feb 19, 2014 at 02:26:33PM +0400, Stanislav Kinsbursky wrote: >>>> 18.02.2014 19:44, J. Bruce Fields ?????: >>>>> On Tue, Feb 18, 2014 at 07:19:31PM +0400, Stanislav Kinsbursky wrote: >>>>>> 18.02.2014 02:19, J. Bruce Fields ?????: >>>>>>> On Sat, Feb 15, 2014 at 09:51:20AM +0800, Weng Meiling wrote: >>>>>>>> Hi Bruce, >>>>>>>> >>>>>>>> The upstream has merged your git tree for-3.14, but there is no this patch? >>>>>>>> Do you forget this patch? >>>>>>> >>>>>>> Apologies, I'm not sure what happened. >>>>>>> >>>>>>> Looking back at it.... The patch causes all my pynfs reboot recovery >>>>>>> tests to fail. They're just doing a "systemctl restart >>>>>>> nfs-server.service", and "systemctl status nfs-server.service" shows in >>>>>>> part >>>>>>> >>>>>>> ExecStart=/usr/sbin/rpc.nfsd $RPCNFSDARGS $RPCNFSDCOUNT (code=exited, status=1/FAILURE) >>>>>>> >>>>>>> So the patch is causing rpc.nfsd to fail? No network namespaces should >>>>>>> be involved. >>>>>>> >>>>>>> I haven't investigated any further. >>>>>>> >>>>>> >>>>>> Hi Bruce, >>>>>> Are you sure, that exactly this patch broke your pynfs tests? >>>>>> BTW, systemd manipulates namespaces. Maybe the patch revealed some pynfs internal bugs? >>>>>> What do you think? >>>>> >>>>> It's really just "systemctl restart nfs-server.service" that the patch >>>>> breaks, pynfs isn't involved much. >>>>> >>>>> The patch I'm actually using follows, but I believe the only difference >>>>> is in the printk message? >>>>> >>>> >>>> Yep, looks true. >>>> That's strange: "systemctl restart nfs-server.service" works for me on Fedora 18 with kernel, based on your repo. >>> >>> OK, I'll take a closer look and let you know what I find. >>> >> >> Thank you, Bruce! > > rpc.nfsd's attempt to bind port 2049 the second time is failing with > EADDRINUSE. > > svc_alien_sock isn't failing, so the only different here is that we're > running sockfd_lookup.... > > Does that take a reference on the fd or the sock that needs to be put? > Yes, you right. I missed it, sorry. Thank you. Will resend. > --b. >
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 7f55517..1331766 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -699,6 +699,11 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net) if (err != 0 || fd < 0) return -EINVAL; + if (svc_alien_sock(net, fd)) { + printk(KERN_ERR "%s: socket net is different from NFSd's\n", __func__); + return -EINVAL; + } + err = nfsd_create_serv(net); if (err != 0) return err; diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h index 62fd1b7..947009e 100644 --- a/include/linux/sunrpc/svcsock.h +++ b/include/linux/sunrpc/svcsock.h @@ -56,6 +56,7 @@ int svc_recv(struct svc_rqst *, long); int svc_send(struct svc_rqst *); void svc_drop(struct svc_rqst *); void svc_sock_update_bufs(struct svc_serv *serv); +bool svc_alien_sock(struct net *net, int fd); int svc_addsock(struct svc_serv *serv, const int fd, char *name_return, const size_t len); void svc_init_xprt_sock(void); diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c index b6e59f0..3ba5b87 100644 --- a/net/sunrpc/svcsock.c +++ b/net/sunrpc/svcsock.c @@ -1397,6 +1397,17 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv, return svsk; } +bool svc_alien_sock(struct net *net, int fd) +{ + int err; + struct socket *sock = sockfd_lookup(fd, &err); + + if (sock && (sock_net(sock->sk) != net)) + return true; + return false; +} +EXPORT_SYMBOL_GPL(svc_alien_sock); + /** * svc_addsock - add a listener socket to an RPC service * @serv: pointer to RPC service to which to add a new listener