2008-09-09 15:30:38

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [RFC][PATCH] sunrpc: fix oops in rpc_create() when the mount namespace is unshared

Quoting Eric W. Biederman ([email protected]):
> "Serge E. Hallyn" <[email protected]> writes:
>
> > Thanks, Cedric. Eric is probably right about the long-term fix, but
> > yeah it might take a while to properly wade through the sunrpc and nfs
> > layers to store the nodename at nfs mount time, and in the meantime this
> > fixes a real oops.
>
> A very esoteric oops that hasn't shown up for two years.

But an easily reproducible one.

It's not as though we'll stop looking for the right fix just bc we have
this "bad" fix in for a short while.

> Please let's look at this and see what it would take to fix this
> properly.

Of course. Cedric is looking at the best way to fix it...

> What are we trying to achieve by reading utsname?

It looks like it gets copied into the sunrpc messages so I assume it is
a part of the sunrpc spec?

I don't want to do this, but we *could* put a conditional in utsname()
to have it return init_utsname if current->nsproxy is null...

-serge


2008-09-09 15:41:00

by Cedric Le Goater

[permalink] [raw]
Subject: Re: [RFC][PATCH] sunrpc: fix oops in rpc_create() when the mount namespace is unshared

Serge E. Hallyn wrote:
> Quoting Eric W. Biederman ([email protected]):
>> "Serge E. Hallyn" <[email protected]> writes:
>>
>>> Thanks, Cedric. Eric is probably right about the long-term fix, but
>>> yeah it might take a while to properly wade through the sunrpc and nfs
>>> layers to store the nodename at nfs mount time, and in the meantime this
>>> fixes a real oops.
>> A very esoteric oops that hasn't shown up for two years.
>
> But an easily reproducible one.
>
> It's not as though we'll stop looking for the right fix just bc we have
> this "bad" fix in for a short while.
>
>> Please let's look at this and see what it would take to fix this
>> properly.
>
> Of course. Cedric is looking at the best way to fix it...

yes. well, my eyes are making progress in the NFS code. it will take some
time :)

>> What are we trying to achieve by reading utsname?
>
> It looks like it gets copied into the sunrpc messages so I assume it is
> a part of the sunrpc spec?
>
> I don't want to do this, but we *could* put a conditional in utsname()
> to have it return init_utsname if current->nsproxy is null...

I nearly did that one but it will hide future misusage of utsname(). So
I think it's better to keep it that way, and let the machine oops when
we need to fix our code.

C.

2008-09-09 17:09:59

by Chuck Lever

[permalink] [raw]
Subject: Re: [RFC][PATCH] sunrpc: fix oops in rpc_create() when the mount namespace is unshared

On Sep 9, 2008, at Sep 9, 2008, 11:29 AM, Serge E. Hallyn wrote:
> Quoting Eric W. Biederman ([email protected]):
>> "Serge E. Hallyn" <[email protected]> writes:
>>
>>> Thanks, Cedric. Eric is probably right about the long-term fix, but
>>> yeah it might take a while to properly wade through the sunrpc and
>>> nfs
>>> layers to store the nodename at nfs mount time, and in the
>>> meantime this
>>> fixes a real oops.
>>
>> A very esoteric oops that hasn't shown up for two years.
>
> But an easily reproducible one.
>
> It's not as though we'll stop looking for the right fix just bc we
> have
> this "bad" fix in for a short while.
>
>> Please let's look at this and see what it would take to fix this
>> properly.
>
> Of course. Cedric is looking at the best way to fix it...

If the upper layers are responsible for providing the utsname, you
will need to fix up lockd and the NFS server's callback client too, at
least.

>> What are we trying to achieve by reading utsname?
>
> It looks like it gets copied into the sunrpc messages so I assume it
> is
> a part of the sunrpc spec?

It appears to be used only for RPC's AUTH_SYS credentials. The
nodename is used to identify the caller's host. See RFC 1831,
Appendix A:

http://rfclibrary.hosting.com/rfc/rfc1831/rfc1831-16.asp

I'm not terribly familiar with uts namespaces, though. Can someone
explain why we need to distinguish between these for AUTH_SYS if the
caller is on a remote system?

> I don't want to do this, but we *could* put a conditional in utsname()
> to have it return init_utsname if current->nsproxy is null...

I don't like the idea of an oops in here. Instead, (for now) it
should warn and fail to create the client, IMO.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

2008-09-09 18:25:01

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [RFC][PATCH] sunrpc: fix oops in rpc_create() when the mount namespace is unshared

Chuck Lever <[email protected]> writes:

> If the upper layers are responsible for providing the utsname, you will need to
> fix up lockd and the NFS server's callback client too, at least.

Actually looking at the code. It looks like a proper fix may be even simpler.
Why do we have both clnt->cl_server and clnt->cl_nodename? Or is cl_server
the other side of the connection?

>>> What are we trying to achieve by reading utsname?
>>
>> It looks like it gets copied into the sunrpc messages so I assume it is
>> a part of the sunrpc spec?
>
> It appears to be used only for RPC's AUTH_SYS credentials. The nodename is used
> to identify the caller's host. See RFC 1831, Appendix A:
>
> http://rfclibrary.hosting.com/rfc/rfc1831/rfc1831-16.asp

Thanks that helps a lot.

> I'm not terribly familiar with uts namespaces, though. Can someone explain why
> we need to distinguish between these for AUTH_SYS if the caller is on a remote
> system?

Semantically processes in different uts namespaces are on different machines.

> I don't like the idea of an oops in here. Instead, (for now) it should warn and
> fail to create the client, IMO.

Which is interesting when the problem happens during NFS unmount. Although
frankly it could fail anyway.

It seems strange that we are creating a client during unmount anyway.

Eric