[RFC PATCH] CIFS posix acl permission checking
in [Kernel]
|
Prev: [PATCH -mmotm 1/4] memcg: dirty memory documentation
Next: io-controller: Document for blkio.weight_device
From: Jon Severinsson on 4 Mar 2010 06:20 Hello Early this weak I sent a patch implementing posix acl permission checking in the linux cifs filesystem module. Unfortunately I only sent it to linux-fsdev as I was unaware of the linux-cifs-client list. I later tried to submit it to linux-cifs-client as well, but my message seems to have been lost in the moderation queue, so I subscribed and am trying again. I don't believe my patch is perfect, but I think it's a good start, and would like some comments from more experienced cifs developers to be able to get it into shape for inclusion in the kernel. I did get some comments from Matthew Wilcox at linux-fsdev, but unfortunately he never followed up on my response, so I'm including some unresolved questions I still have, as well as attaching the patch for further comments. Best Regards Jon Severinsson On Monday 01 March 2010 19:33:41, Jon Severinsson wrote: > On Monday 01 March 2010 18:03:49, Matthew Wilcox wrote: >> You've included ifdefs around the check_acl entry in inode_operations, >> *and* inside the definition of cifs_check_acl. You only need to do >> one or the other, and opinion is divided on which is better. > > While I did recognize the redundancy, I decided to follow the same > convention the other functions in xattr.c did, and include ifdefs at both > locations. > > I also considered the possible reasons for the existing functions to do > both, and and came up with two reasons. The first simply being the paradigm > of defensive programming, always check before doing a call, but never assume > that the check has been done before being called. The second one is that of > performance. The ifdefs has to be in cifs_check_acl to protect against other > callers (while this patch doesn't introduce any, it doesn't prevent further > patches from adding them), and not including the ifdefs in inode_operations > would mean a completely useless function call when a feature was turned off > at compile time. The second one is a micro-optimization I don't really care > fore, but defensive programming I do respect. > > With this in mind, what do you recommend, double protection, breaking > convention or changing the existing code? > >>> +int cifs_check_acl(struct inode *inode, int mask) >>> +{ >>> + int rc = -EOPNOTSUPP; >>> +#ifdef CONFIG_CIFS_XATTR >>> +#ifdef CONFIG_CIFS_POSIX >>> + struct dentry *dentry; >>> + size_t buf_size; >>> + void *ea_value = NULL; >>> + ssize_t ea_size; >>> + struct posix_acl *acl = NULL; >> >> I don't think you need to initialise ea_value, do you? > > While currently correct, I find it a good idea to immediately null any > pointer that is freed in the exit section. Otherwise it is very easy to > forget to do that the day someone adds a goto prior to the first > assignment, and not nulling then can have unintended consequences in > unrelated code. That being said, if you say that the kernel community > frowns upon that kind of defensive programming I will definitely remove > it.
From: Michael Adam on 12 Mar 2010 03:10
Jeremy Allison wrote: > On Thu, Mar 11, 2010 at 11:45:29PM +0100, Michael Adam wrote: > > > > When discussing this with Volker today, he had a different idea: > > One could implement a trans2 impersonate call in samba (as a new > > call in the unix extensions) that could be used to transfer the > > session established by the privileged user (root, say) to a > > different user specified as an argument to the call -- without > > the need to give credentials! Then this call could be used in > > the multi user mount scenario: when uid 1000 accesse the cifs > > mount then the root-dispatcher mount would create a new session > > initially as root and issue an impersonate call to user 1000 > > directly afterwards. > > > > Wouldn't that be something worth considering? > > This world work, but protocol cleanliness-wise it's > *really* horrible :-). Agreed. :-) |