2014-01-03 22:22:37

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
> There could be a case, when NFSd file system is mounted in network, different
> to socket's one, like below:
>
> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
> mount point, created in init_net context. And thus NFS server stop in nested
> network context leads to RPCBIND client destruction in init_net.
> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
> creation in init_net context because of the same reason (NFSd monut point was
> created in init_net context). An attempt to register passed socket in nested
> net leads to panic, because no RPCBIND client present in nexted network
> namespace.

So it's the attempt to use a NULL ->rpcb_local_clnt4?

Interesting, thanks--applying with a minor fix to logged message.

--b.

>
> This patch add check that passed socket's net matches NFSd superblock's one.
> And returns -EINVAL error to user psace otherwise.
>
> Reported-by: Weng Meiling <[email protected]>
> Signed-off-by: Stanislav Kinsbursky <[email protected]>
> Cc: [email protected]
> ---
> fs/nfsd/nfsctl.c | 5 +++++
> include/linux/sunrpc/svcsock.h | 1 +
> net/sunrpc/svcsock.c | 11 +++++++++++
> 3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7f55517..f34d9de 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 to NFSd's one\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
>


2014-01-09 07:15:20

by Stanislav Kinsbursky

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

04.01.2014 02:22, J. Bruce Fields пишет:
> On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
>> There could be a case, when NFSd file system is mounted in network, different
>> to socket's one, like below:
>>
>> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
>> mount point, created in init_net context. And thus NFS server stop in nested
>> network context leads to RPCBIND client destruction in init_net.
>> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
>> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
>> creation in init_net context because of the same reason (NFSd monut point was
>> created in init_net context). An attempt to register passed socket in nested
>> net leads to panic, because no RPCBIND client present in nexted network
>> namespace.
>
> So it's the attempt to use a NULL ->rpcb_local_clnt4?
>

Correct.

--
Best regards,
Stanislav Kinsbursky

2014-02-15 01:52:11

by Weng Meiling

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

Hi Bruce,

The upstream has merged your git tree for-3.14, but there is no this patch?
Do you forget this patch?

Thanks!
Weng Meiling


On 2014/1/4 6:22, J. Bruce Fields wrote:
> On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
>> There could be a case, when NFSd file system is mounted in network, different
>> to socket's one, like below:
>>
>> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
>> mount point, created in init_net context. And thus NFS server stop in nested
>> network context leads to RPCBIND client destruction in init_net.
>> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
>> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
>> creation in init_net context because of the same reason (NFSd monut point was
>> created in init_net context). An attempt to register passed socket in nested
>> net leads to panic, because no RPCBIND client present in nexted network
>> namespace.
>
> So it's the attempt to use a NULL ->rpcb_local_clnt4?
>
> Interesting, thanks--applying with a minor fix to logged message.
>
> --b.
>


2014-02-17 22:19:15

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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.

--b.

>
> Thanks!
> Weng Meiling
>
>
> On 2014/1/4 6:22, J. Bruce Fields wrote:
> > On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
> >> There could be a case, when NFSd file system is mounted in network, different
> >> to socket's one, like below:
> >>
> >> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
> >> mount point, created in init_net context. And thus NFS server stop in nested
> >> network context leads to RPCBIND client destruction in init_net.
> >> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
> >> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
> >> creation in init_net context because of the same reason (NFSd monut point was
> >> created in init_net context). An attempt to register passed socket in nested
> >> net leads to panic, because no RPCBIND client present in nexted network
> >> namespace.
> >
> > So it's the attempt to use a NULL ->rpcb_local_clnt4?
> >
> > Interesting, thanks--applying with a minor fix to logged message.
> >
> > --b.
> >
>
>
>

2014-02-18 13:07:36

by Weng Meiling

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

Hi Bruce , Stanislav
On 2014/2/18 6:19, J. Bruce Fields wrote:
> 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.
>
the problem exists. Sorry for careless testing.
Stanislav, how do you think about this?

Thanks!
Weng Meiling
> --b.
>
>>
>> Thanks!
>> Weng Meiling
>>
>>
>> On 2014/1/4 6:22, J. Bruce Fields wrote:
>>> On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
>>>> There could be a case, when NFSd file system is mounted in network, different
>>>> to socket's one, like below:
>>>>
>>>> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
>>>> mount point, created in init_net context. And thus NFS server stop in nested
>>>> network context leads to RPCBIND client destruction in init_net.
>>>> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
>>>> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
>>>> creation in init_net context because of the same reason (NFSd monut point was
>>>> created in init_net context). An attempt to register passed socket in nested
>>>> net leads to panic, because no RPCBIND client present in nexted network
>>>> namespace.
>>>
>>> So it's the attempt to use a NULL ->rpcb_local_clnt4?
>>>
>>> Interesting, thanks--applying with a minor fix to logged message.
>>>
>>> --b.
>>>
>>
>>
>>
>
> .
>

2014-02-18 15:19:36

by Stanislav Kinsbursky

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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?

> --b.
>
>>
>> Thanks!
>> Weng Meiling
>>
>>
>> On 2014/1/4 6:22, J. Bruce Fields wrote:
>>> On Mon, Dec 30, 2013 at 05:23:59PM +0300, Stanislav Kinsbursky wrote:
>>>> There could be a case, when NFSd file system is mounted in network, different
>>>> to socket's one, like below:
>>>>
>>>> "ip netns exec" creates new network and mount namespace, which duplicates NFSd
>>>> mount point, created in init_net context. And thus NFS server stop in nested
>>>> network context leads to RPCBIND client destruction in init_net.
>>>> Then, on NFSd start in nested network context, rpc.nfsd process creates socket
>>>> in nested net and passes it into "write_ports", which leads to RPCBIND sockets
>>>> creation in init_net context because of the same reason (NFSd monut point was
>>>> created in init_net context). An attempt to register passed socket in nested
>>>> net leads to panic, because no RPCBIND client present in nexted network
>>>> namespace.
>>>
>>> So it's the attempt to use a NULL ->rpcb_local_clnt4?
>>>
>>> Interesting, thanks--applying with a minor fix to logged message.
>>>
>>> --b.
>>>
>>
>>
>>


--
Best regards,
Stanislav Kinsbursky

2014-02-18 15:44:13

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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?

--b

commit e1f2922c12cb59baba0f2c7726bee992a0861310
Author: Stanislav Kinsbursky <[email protected]>
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 <[email protected]>
Signed-off-by: Stanislav Kinsbursky <[email protected]>
Cc: [email protected]
Signed-off-by: J. Bruce Fields <[email protected]>

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

2014-02-19 10:26:45

by Stanislav Kinsbursky

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Stanislav Kinsbursky <[email protected]>
> Cc: [email protected]
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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

2014-02-19 14:51:03

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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 <[email protected]>
> >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 <[email protected]>
> > Signed-off-by: Stanislav Kinsbursky <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: J. Bruce Fields <[email protected]>
> >
> >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

2014-02-19 14:57:51

by Stanislav Kinsbursky

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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 <[email protected]>
>>> 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 <[email protected]>
>>> Signed-off-by: Stanislav Kinsbursky <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: J. Bruce Fields <[email protected]>
>>>
>>> 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


--
Best regards,
Stanislav Kinsbursky

2014-02-20 21:31:33

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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.

2014-02-21 09:18:37

by Stanislav Kinsbursky

[permalink] [raw]
Subject: Re: [PATCH] nfsd: check passed socket's net matches NFSd superblock's one

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.
>


--
Best regards,
Stanislav Kinsbursky