From: Jon Severinsson on

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;
>>> + 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
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. :-)