[Acl-Devel] No extended attributes for sticky directories? (and samba)

[Timothy Shimmin]

--On 3 November 2006 6:43:17 PM +0100 Andreas Gruenbacher <agruen at suse.de> wrote:

> Hello,
>
> On Friday 03 November 2006 07:27, Gerard Neil wrote:
>> Andreas,
>>
>> On 03/11/06, Andreas Gruenbacher <agruen at suse.de> wrote:
>> > Indeed, yes. The xfs code has it right, and the patch is good. I'll fix
>> > the two typos in the comment above in the same go, and push the resulting
>> > patch (attached) upstream. Thanks for your help!
>>
>> That's quite exciting for me; I've taken my first open source step,
>> however small. Thanks to both you and Dave.
>>
>> But... I'm not sure that the patch and xfs can/should both be right at
>> the same time.
>>
>> Without the patch, the linux behaviour is the same for all
>> filesystems. There's no access at all to extended attributes on sticky
>> directories, for any user including root. Because the vfs behaviour is
>> more restictive than that of xfs this applies to xfs too.
>>
>> The patch does two things: firstly it aligns vfs behaviour with the
>> documentation and xfs, by allowing access for the owner and priveleged
>> users. Then, it goes further, and allows read access for all users. My
>> grounds for this latter change were that it felt intuitively right to
>> me, and that this behaviour appeared to have been in the kernel in
>> earlier versions (although I was going only from snippets, there
>> appeared to have been a return in the function for the read-only case
>> before stickyness was tested, the sort of thing that's easy to
>> overlook when making changes).
>>
>> Anyway, the latter change makes the vfs behaviour _less_ restrictive
>> than xfs, so the net effect is:
>>
>> vfs (therefore jfs/ext2/ext3 and most other filesystems):
>>   Read: allowed for all users
>>   Write: owner and CAP_FOWNER only
>>
>> xfs:
>>   Read: owner and CAP_FOWNER only
>>   Write: owner and CAP_FOWNER only
>
> Almost, except that the vfs also checks if the user is granted the respective
> access by the file permissions (mode or ACLs).
>
> The original intention of the user.* namespace was to use the same mechanism
> that controls access to the file data for extended attributes. The exceptions
> for special files and sticky directories are necessary additional
> restrictions for making the mechanism secure, and preventing users from
> storing xattr information where nobody would expect it.
>
>> My thinking was that either the patch should be aligned with xfs (by
>> leaving out the "allow read access part") or vice versa (by changing
>> xfs to allow read access). My intent was to query which path to take.
>> I apologise for not making this clear, I didn't quite believe I'd
>> found a problem or it's solution, and the point got a bit lost in my
>> uncertainty. It simply didn't occur to me that my patch would be
>> accepted as is. I'll be more careful in future: beware when posting a
>> patch... someone might just apply it!
>>
>> I don't believe the unmodified patch should be used because it would
>> introduce a slight difference in behaviour between xfs and all other
>> filesystems. That's fine if there's a good reason for such a
>> difference, but otherwise it should be avoided on general principles:
>> it can lead to unexpected, hard to track consequences. Case in point:
>> samba. The consequence would be that hidden files suddenly appear
>> unhidden if a sticky directory appears anywhere on a share with an
>> underlying xfs filesystem. Sure, the samba code should be improved,
>> but we're already in the margins with the sticky directory thing to
>> begin with; it's worse if it's just for xfs and for no good reason.
>>
>> I guess my "preference" was to modify xfs to allow read access... but
>> I didn't actually go ahead include the change in the patch because of
>> the following: according to my reading, the code has a notion of
>> attribute "capability" which is relative to process credentials; if an
>> i-node is attribute-capable, then it is allowed to take attributes,
>> otherwise not. My gut feeling is that the word "capable" precludes
>> making it relative to the type of access, otherwise a word like
>> "permission" would have been used; but it's impossible to tell for
>> sure from the code, that's just the way I relate to English. It's easy
>> enough to make the change (the type of access is passed to
>> attr_generic_get() via xflags, but that would involve either changing
>> the meaning of "capable" according to my reading or a slight refactor.
>> In any case, I felt the whole thing needed discussion first.
>
> I don't know what you are talking about in the above paragraph.
>
> Tim, I guess the attached patch would make sense for xfs -- what do you think?
>
Hmmm. Nathan wrote this stuff so I'm not certain what he was addressing.
However, I can't see now why we need any of the attr_capable hooks in xfs_attr.c -
they all seem to be covered by the vfs - for each of the namespaces;
(and we go thru the vfs first before getting to xfs).
  We have differences like "current_fsuid(cred)" instead of current->fsuid,
  but we don't actually use the cred's like we did in IRIX and you'll notice
  that we have:
  linux-2.6/xfs_linux.h
  #define current_fsuid(cred)     (current->fsuid)
Hmmm..we should have a qa test for this.

So, I'd like to rip out all the capable calls (not just the user one)
but I want to check with Nathan and have a better look first.

Cheers,
Tim.