Have ceph clear setuid/setgid bits on chown #12331
Conversation
|
Good point. I'll roll some tests for this, but we may want to go ahead and merge this for kraken, just in case people end up using broken kernels with ceph-fuse. The write path is a little trickier here, and I'm still trying to sort out what we should do there. |
|
Sounds good to me. Should this stay DNM, or can we merge it once John's next test branch rolls it up? |
jtlayton
force-pushed
the
wip-jlayton-suid
branch
from
9956329
to
36b4fd2
Compare
Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
If we get a ownership change, POSIX mandates that you clear the setuid and setgid bits unless you are "appropriately privileged", in which case the OS is allowed to leave them intact. Linux however always clears those bits, regardless of the process privileges, as that makes it simpler to close some potential races. Have ceph do the same. Signed-off-by: Jeff Layton <jlayton@redhat.com>
When we hold exclusive auth caps, then the client is responsible for handling changes to the mode. Make sure we remove any setuid/setgid bits on an ownership change. Signed-off-by: Jeff Layton <jlayton@redhat.com>
jtlayton
force-pushed
the
wip-jlayton-suid
branch
from
36b4fd2
to
18d2499
Compare
|
Testing with a libcephfs case pointed out a couple of other issues so those should now be fixed. Also tossed in a fix for a minor build warning that I introduced a while ago in test.cc. I dropped the part that does this on truncate for now. We don't strictly need that to work around the kernel bug, and I'd like go over the cap handling a bit more closely and think more about how to handle races on write. New branch is building now and I'll kick off a test run once it's done. Assuming it passes, I'll remove the DNM tag and we'll call it good to go. I've also added a test to make sure that clearing the bits on chown stays working. |
jtlayton
changed the title
|
Changes look good to me, although I'm taking your word on the permission mode numbers. ;) |
jtlayton
changed the title
|
Removing [DNM] tag. Test run mostly passed (at least the failures don't seem to be related to any of these codepaths). I think this is ready for merge now. http://pulpito.ceph.com/jlayton-2016-12-07_20:13:58-fs-wip-jlayton-suid---basic-smithi/ |
This is a partial fix for http://tracker.ceph.com/issues/18131:
POSIX allows (suggests?) that you clear setuid and setgid bits on chown() and truncate(), and we should really be doing that in ceph as well. This adds that to the mds, and to the client (when it holds auth caps).
The reason that this wasn't a problem before is because the kernel fuse driver always passed down a mode change along with the uid/gid change. It recently stopped doing that (which is a bug, I think). I'm working on getting that fixed as well, but doing it in userland won't hurt anything and is good hygiene. The write path should be unaffected by this -- the kernel still sends down a mode change on write if it thinks the setuid/setgid bits are set.
Note that fuse recently added a way to tell the kernel that it will clear these bits so the kernel doesn't have to ask for it (which also closes a potential race). So we really will want to do this anyway.
The missing piece here is write(). That's a bit more complicated as we don't necessarily hold any auth caps now when we're writing. Acquiring them could also be nasty if Ax is held elsewhere. I'm still looking at that part.
In any case, this is building now and I'll kick off an fs suite run on it later this evening.