Re: Caching semi-digested credentials in struct cred

Re: Caching semi-digested credentials in struct cred

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> I thought you were passing a generic cred as an argument? It should be
> possible to convert that into an rpc_cred.

Indeed I am, but I think I haven't gotten my questions across.

Take the rpc_cred struct as an example.  It contains some stuff that is
obtained by taking the credentials open was supplied and munging them into
other things perhaps by contacting a remote auth server.  Now, assuming that I
can replace rpc_cred by, say, a key struct with dangly bits, most of the
fields can be replaced by the key struct fields (cr_hash, cr_lru, cr_rcu,
cr_expire, cr_flags, cr_count) or cred struct fields (cr_uid possibly).  But
that still leaves cr_auth and cr_ops, which can be dangled from a key if
that's okay, but how tied are these to a particular open?  That I'm not sure of.

However:

 (1) If I then attach such a key to the cred struct, NFS would have to perform
     a search every time it wants to use the cred.  This might not be so bad,
     as the keyring search algorithm uses RCU to do all its locking, and I
     would guess there won't be that many keys.

     I could attach a cache to the cred struct so that any key that got used
     gets added to it.  The cache could be in the form of a keyring.

 (2) If I require that the keyrings pointed to by a cred struct be searched
     each time an NFS op takes place, then the credentials being used by an
     open file can change as the membership of the keyrings changes - which
     I'm fairly sure is the wrong thing to do.

     The problem is that the cred struct wants itself and all its dangly bits
     to be purely COW, but keyrings don't work like that because their
     contents need to be alterable.

     I could work around this by duplicating the cred struct for each open
     call, and allowing a fs to attach its own keys or whatever directly to
     it, but if we do that, we may as well use struct file.

> In the long run, we should get rid of the rpc_cred in the NFS layer, and
> replace it with the generic creds, but for the short term, converting
> one into the other in the NFS code should be acceptable.

Agreed, but I'm trying work out a way of avoiding a couple of problems (namely
having the creds on an open file changing and having to perform a search), if
indeed they are deemed to be problems.

David

Re: Caching semi-digested credentials in struct cred

[Trond Myklebust]

On Wed, 2007-10-24 at 18:10 +0100, David Howells wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > I thought you were passing a generic cred as an argument? It should be
> > possible to convert that into an rpc_cred.
> 
> Indeed I am, but I think I haven't gotten my questions across.
> 
> Take the rpc_cred struct as an example.  It contains some stuff that is
> obtained by taking the credentials open was supplied and munging them into
> other things perhaps by contacting a remote auth server.  Now, assuming that I
> can replace rpc_cred by, say, a key struct with dangly bits, most of the

What is a 'struct key'? Is that a credential?

> fields can be replaced by the key struct fields (cr_hash, cr_lru, cr_rcu,
> cr_expire, cr_flags, cr_count) or cred struct fields (cr_uid possibly).  But
> that still leaves cr_auth and cr_ops, which can be dangled from a key if
> that's okay, but how tied are these to a particular open?  That I'm not sure of.
> 
> However:
> 
>  (1) If I then attach such a key to the cred struct, NFS would have to perform
>      a search every time it wants to use the cred.  This might not be so bad,
>      as the keyring search algorithm uses RCU to do all its locking, and I
>      would guess there won't be that many keys.
> 
>      I could attach a cache to the cred struct so that any key that got used
>      gets added to it.  The cache could be in the form of a keyring.

NO! Keyrings are meant for communicating with userspace. They should not
be used as a 'generic cache' in the kernel.

>  (2) If I require that the keyrings pointed to by a cred struct be searched
>      each time an NFS op takes place, then the credentials being used by an
>      open file can change as the membership of the keyrings changes - which
>      I'm fairly sure is the wrong thing to do.
> 
>      The problem is that the cred struct wants itself and all its dangly bits
>      to be purely COW, but keyrings don't work like that because their
>      contents need to be alterable.
> 
>      I could work around this by duplicating the cred struct for each open
>      call, and allowing a fs to attach its own keys or whatever directly to
>      it, but if we do that, we may as well use struct file.

Why are you trying to replace the rpc_cred?

> > In the long run, we should get rid of the rpc_cred in the NFS layer, and
> > replace it with the generic creds, but for the short term, converting
> > one into the other in the NFS code should be acceptable.
> 
> Agreed, but I'm trying work out a way of avoiding a couple of problems (namely
> having the creds on an open file changing and having to perform a search), if
> indeed they are deemed to be problems.

Use the credential struct as the unique lookup key for an rpc_cred.
Where is the problem? If looking up rpc creds is a performance issue,
then that needs to be addressed separately. It should have nothing to do
with the design of a generic credential.

Trond

Re: Caching semi-digested credentials in struct cred

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> What is a 'struct key'? Is that a credential?

It can represent one yes.

> NO! Keyrings are meant for communicating with userspace. They should not
> be used as a 'generic cache' in the kernel.

Erm, I'm pretty sure that keys are primarily for caching credentials that the
kernel needs to use a lot, so that it doesn't have to keep asking userspace for
them.  Keyrings are a defined method of retaining and searching for keys.

It happens that userspace can also make use of the cached keys, given
appropriate rights.

However, I don't particularly like the idea of a cache as I can see it raising
various problems.

> Why are you trying to replace the rpc_cred?

So that NFS doesn't have to pass both rpc_cred and cred pointers around.  I
can't just give struct cred an rpc_cred pointer as it may the former may hold
information on several of the latter (for different NFS domains).

I'm not objecting to the existence of rpc_cred per se, but if it's retained by
struct cred, then it needs to be in some generic mechanism, and rpc_cred's
fields can be mapped more or less onto a key struct.

> Use the credential struct as the unique lookup key for an rpc_cred.

That's not good enough.  A cred struct may map to several rpc_creds as I
mentioned above.  I suppose the nfs_client struct address could be added to the
lookup key.

Furthermore, a cred struct may end up referring to different rpc_creds for the
same domain if a key in the keyrings changes - unless I add something to make a
COW mirror of the keyring contents from the keyrings.

> If looking up rpc creds is a performance issue, then that needs to be
> addressed separately. It should have nothing to do with the design of a
> generic credential.

If there's a cred -> rpc_cred mapping, then it might make sense to root the
mapping in the cred struct and to make it generic.  NFS is just one of the
kernel services that might want to use this.

David

Re: Caching semi-digested credentials in struct cred

[Trond Myklebust]

On Wed, 2007-10-24 at 19:41 +0100, David Howells wrote:

> > Use the credential struct as the unique lookup key for an rpc_cred.
> 
> That's not good enough.  A cred struct may map to several rpc_creds as I
> mentioned above.  I suppose the nfs_client struct address could be added to the
> lookup key.

Huh? The RPC cred lookup function takes a pointer to an rpc_auth struct,
which should already be tied to an rpc_client. Passing an nfs_client is
completely unnecessary aside from being a massive layering violation.

> Furthermore, a cred struct may end up referring to different rpc_creds for the
> same domain if a key in the keyrings changes - unless I add something to make a
> COW mirror of the keyring contents from the keyrings.

In the case where keyrings are enabled and a key changes, then we should
revalidate the rpc_cred and either dump it (replacing it with a new
cred) or reuse it. We can only cache so much garbage...

> > If looking up rpc creds is a performance issue, then that needs to be
> > addressed separately. It should have nothing to do with the design of a
> > generic credential.
> 
> If there's a cred -> rpc_cred mapping, then it might make sense to root the
> mapping in the cred struct and to make it generic.  NFS is just one of the
> kernel services that might want to use this.

Possibly, but I'm not accepting any single patch that completely
rewrites the basic security model of NFS in one fell swoop. If you want
to change the cred->rpc_cred mapping, then that will be done in
incremental patches.
Your first step is therefore to get the generic cred working with the
_existing_ RPC security model.

  Trond

Re: Caching semi-digested credentials in struct cred

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> > > Use the credential struct as the unique lookup key for an rpc_cred.
> > 
> > That's not good enough.  A cred struct may map to several rpc_creds as I
> > mentioned above.  I suppose the nfs_client struct address could be added
> > to the lookup key.
> 
> Huh? The RPC cred lookup function takes a pointer to an rpc_auth struct,
> which should already be tied to an rpc_client.

So where do you get the rpc_auth struct from?

How about we come at it this way: nfs_readpage() is changed to have a struct
cred * instead of a struct file *.  How do we get the appropriate rpc_auth
struct (assuming it isn't held in the cred struct)?  We need something from
the mapping to use as part of the lookup key, be that an nfs_client or an
rpc_client.

> In the case where keyrings are enabled and a key changes, then we should
> revalidate the rpc_cred and either dump it (replacing it with a new
> cred) or reuse it. We can only cache so much garbage...

You can't necessarily just arbitrarily ditch an rpc_cred just because the key
it was derived from has been replaced.  Someone may have a file open that's
using it.  Unless, of course, you accept that it's okay to change the
credential that an open file is using...  If we're happy to do that, that makes
my life a lot easier.

Also, it occurs to me that if a cred struct acts as a lookup key to find cached
rpc_creds, what controls the lifetime of the latter with respect to the former?

> Possibly, but I'm not accepting any single patch that completely
> rewrites the basic security model of NFS in one fell swoop. If you want
> to change the cred->rpc_cred mapping, then that will be done in
> incremental patches.
> Your first step is therefore to get the generic cred working with the
> _existing_ RPC security model.

I would never have thought of that.

However, there are two points you should consider: firstly, I've a patch that's
a quarter of a Meg just to add struct cred pointers into NFS so that the
appropriate credentials get down into the SunRPC layer rather than using
current's credentials; and secondly, I want some idea of where I'm going.  If
you'd rather, I can just drop the credentials into the NFS VFS methods and
leave it for you to make work.

David

Re: Caching semi-digested credentials in struct cred

[Trond Myklebust]

On Wed, 2007-10-24 at 23:22 +0100, David Howells wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > > > Use the credential struct as the unique lookup key for an rpc_cred.
> > > 
> > > That's not good enough.  A cred struct may map to several rpc_creds as I
> > > mentioned above.  I suppose the nfs_client struct address could be added
> > > to the lookup key.
> > 
> > Huh? The RPC cred lookup function takes a pointer to an rpc_auth struct,
> > which should already be tied to an rpc_client.
> 
> So where do you get the rpc_auth struct from?
> 
> How about we come at it this way: nfs_readpage() is changed to have a struct
> cred * instead of a struct file *.  How do we get the appropriate rpc_auth
> struct (assuming it isn't held in the cred struct)?  We need something from
> the mapping to use as part of the lookup key, be that an nfs_client or an
> rpc_client.

You should be using the rpc_auth that is cached in the rpc_client:

  NFS_CLIENT(inode)->cl_auth

W.r.t. the nfs_find_open_context(), btw, there is no reason why we
couldn't change that to take a generic cred as its argument.

> > In the case where keyrings are enabled and a key changes, then we should
> > revalidate the rpc_cred and either dump it (replacing it with a new
> > cred) or reuse it. We can only cache so much garbage...
> 
> You can't necessarily just arbitrarily ditch an rpc_cred just because the key
> it was derived from has been replaced.  Someone may have a file open that's
> using it.  Unless, of course, you accept that it's okay to change the
> credential that an open file is using...  If we're happy to do that, that makes
> my life a lot easier.
> 
> Also, it occurs to me that if a cred struct acts as a lookup key to find cached
> rpc_creds, what controls the lifetime of the latter with respect to the former?

There is automatic garbage collection of the latter. If they are unused,
they may be culled.

> > Possibly, but I'm not accepting any single patch that completely
> > rewrites the basic security model of NFS in one fell swoop. If you want
> > to change the cred->rpc_cred mapping, then that will be done in
> > incremental patches.
> > Your first step is therefore to get the generic cred working with the
> > _existing_ RPC security model.
> 
> I would never have thought of that.

Just making things crystal clear. I did also consult with a number of
members of the NFS development team, and so far they all agree that we
want to see the generic cred go in first before we start working on
rewrites to the RPC auth code. We have yet to see any numbers comparing
the current vs. cred performance.

> However, there are two points you should consider: firstly, I've a patch that's
> a quarter of a Meg just to add struct cred pointers into NFS so that the
> appropriate credentials get down into the SunRPC layer rather than using
> current's credentials; and secondly, I want some idea of where I'm going.  If
> you'd rather, I can just drop the credentials into the NFS VFS methods and
> leave it for you to make work.

The latter sounds good for the moment. There are several people who are
working on the rpc auth code, and thus have an interest here, so it will
take some time to work out a consensus for what needs to be done.

Trond

Re: Caching semi-digested credentials in struct cred

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> You should be using the rpc_auth that is cached in the rpc_client:
> 
>   NFS_CLIENT(inode)->cl_auth
> 
> W.r.t. the nfs_find_open_context(), btw, there is no reason why we
> couldn't change that to take a generic cred as its argument.

I think I've been under a misapprehension as to what rpc_auth is for.  I
assumed it was the credential NFS was using for the operations by the user
that it was created for, but given it's a 'superblock' level thing, I guess
not.

> We have yet to see any numbers comparing the current vs. cred performance.

That's something that concerns me, as does the increased risk of overrunning
the stack.  I'm adding an extra argument to an awful lot of functions, and
it's going to degrade performance, no two ways about it.  The question is by
how much?

> > If you'd rather, I can just drop the credentials into the NFS VFS methods
> > and leave it for you to make work.
> 
> The latter sounds good for the moment. There are several people who are
> working on the rpc auth code, and thus have an interest here, so it will
> take some time to work out a consensus for what needs to be done.

The problem with that is that there are bits in the bottom of the sunrpc stuff
that assume they can use current->fs[ug]id (rpcauth_lookupcred() for example),
which is the reason for passing this pointer all the way down:-/

David

Re: Caching semi-digested credentials in struct cred

[Trond Myklebust]

On Thu, 2007-10-25 at 16:45 +0100, David Howells wrote:
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > > If you'd rather, I can just drop the credentials into the NFS VFS methods
> > > and leave it for you to make work.
> > 
> > The latter sounds good for the moment. There are several people who are
> > working on the rpc auth code, and thus have an interest here, so it will
> > take some time to work out a consensus for what needs to be done.
> 
> The problem with that is that there are bits in the bottom of the sunrpc stuff
> that assume they can use current->fs[ug]id (rpcauth_lookupcred() for example),
> which is the reason for passing this pointer all the way down:-/

I don't think anybody has problems with the concept of replacing
current-> with cred-> in rpcauth_lookupcred(). That reflects the
(desirable) change to the way the VFS communicates authorisation
information to the lower level code.

What we don't want at this time is for any changes that go beyond that
minimal approach. It will take time to digest the consequences of the
existence of a generic credential, and it is not clear to us at this
time that changes to the rpc_cred caching mechanism (as opposed to just
the lookup key) is the right thing to do.

Trond