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 RegardsJon 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.