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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
