Messages in this thread | | | Date | Wed, 07 Jul 2010 13:49:46 -0400 | From | Chuck Lever <> | Subject | Re: [PATCH 07/10] NFSv4: Introduce new label structure |
| |
On 07/ 7/10 12:22 PM, David P. Quigley wrote: > Hello Chuck, > Thank you for the comments. I'll go through and address them inline > (Sorry for the top post). > > On Wed, 2010-07-07 at 12:01 -0400, Chuck Lever wrote: >> My comments below apply to the other NFS client patches as well. This >> seemed like the right one to use for code examples. >> >> On 07/ 7/10 10:31 AM, David P. Quigley wrote:
[ ... snipped ... ]
>> It seems to me you want something more generic, just like nfs_fh or >> nfs_fattr, to represent these. Over time, I'm guessing label support >> won't be tied to a specific NFS version. And, you are passing these >> around in the generic NFS functions (for post-op updates and inode >> revalidation, and what not). >> >> Can I recommend "struct nfs_seclabel" instead? Then have >> nfs_seclabel_alloc() and nfs_seclabel_free(). > > I can definitely rename them to be more generic. I don't see anything > else besides NFSv4 using them but its fine with me to rename them. The > reason we call them nfs4_label is because we modeled it after the NFSv4 > acl support code. I spoke with Christoph a long time ago and he > suggested that it should be handled the same way that the NFSv4 ACLs are > handled as opposed to the iattr thing we were trying. > >> >> Does it make sense to deduplicate these in the client's memory? It >> seems to me that you could have hundreds or thousands that all contain >> the same label information. > > I don't think it is worth the effort. We are only using these structures > until the security label is crammed into the inode. Once that happens > they get freed. You shouldn't have them sitting around for very long.
OK, the lifetime of these things wasn't clear.
> They will be pulled again when the inode attributes expire and need to > be revalidated. For things like SELinux you could argue that the LSM > might benefit from this (and it might already do it but I'm not sure) > but I think that is something to be handled by the LSM itself or the > credentials code (since it already supports COW credentials it should be > possible).
I think the lifetime of the label structure then is about the same as the lifetime of an nfs_attr, and not at all the same as an ACL. I'm just guessing here.
Would it then make sense to add a field that refers to the security label to struct nfs_fattr instead? That might simplify or eliminate all of the internal API changes.
| |