Ceph » CephFS

The tests I'm mainly concered with are the pjd.sh tests in those. The rest seem to be transient failures of one sort or another.

Sorry, I mean this patch in my v4.8.10 kernel:

commit 0e2993ef5bfbca56949474942c2f6b31417e3d90
Author: Miklos Szeredi <mszeredi@redhat.com>
Date:   Sat Oct 1 07:32:32 2016 +0200

    fuse: fix killing s[ug]id in setattr

    commit a09f99eddef44035ec764075a37bace8181bec38 upstream.

    Fuse allowed VFS to set mode in setattr in order to clear suid/sgid on
    chown and truncate, and (since writeback_cache) write.  The problem with
    this is that it'll potentially restore a stale mode.

    The poper fix would be to let the filesystems do the suid/sgid clearing on
    the relevant operations.  Possibly some are already doing it but there's no
    way we can detect this.

    So fix this by refreshing and recalculating the mode.  Do this only if
    ATTR_KILL_S[UG]ID is set to not destroy performance for writes.  This is
    still racy but the size of the window is reduced.

    Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Definitely a kernel problem. Works correctly on v4.8.0, broken on v4.8.6 kernel for sure. I haven't bisected to be sure, but mainline commit a09f99eddef44035ec764075a37bace8181bec38 was taken into stable between those kernels.

Bisecting confirms that that is the patch where it breaks for me. John says he can reproduce on an ubuntu 4.4 kernel, and I'm trying to confirm now whether that might have this patch.

Ahh, I think I see the problem:

+               attr->ia_mode = inode->i_mode;
+               kill = should_remove_suid(entry);
+               if (kill & ATTR_KILL_SUID) {
+                       attr->ia_valid |= ATTR_MODE;
+                       attr->ia_mode &= ~S_ISUID;
+               }
+               if (kill & ATTR_KILL_SGID) {
+                       attr->ia_valid |= ATTR_MODE;
+                       attr->ia_mode &= ~S_ISGID;
+               }

...and should_remove_suid() is returning 0 here. The reason for that is because the test is running as root.

So basically, pjdtests is saying that when root chown/chgrps a file, that the setuid/setgid bits must be cleared. POSIX however is somewhat less definitive on the subject. It just says:

"The POSIX.1-1990 standard requires that the chown() function invoked by a non-appropriate privileged process clear the S_ISGID and the S_ISUID bits for regular files"

...which is strange verbiage, but I think just means "a non-privileged process". In Linux terms, I think that just means a process without CAP_FSETID.

IOW, if you chgrp as a non-privileged user then you have to clear the bits. If you do so as root, then you're allowed to clear the bits, but aren't required to do so. The kernel always tries to clear them on chown, even for root.

The fuse client used to clear them, but now it doesn't. The problem I think is the new use of should_remove_suid in this codepath. That's typically used in write/truncate codepath, but for chown, I think we want to do this, even for root.

So the upshot here is that I don't think this is anything that has broken in ceph, per-se. FUSE changed its behavior, and I'm pursuing getting FUSE fixed on upstream. As best I can tell, ceph has just relied on the kernel to do this.

On the ceph userland front, I think we ought to implement this in the setattr codepath as well. When we have a a uid/gid change without a corresponding mode change in the setattr request, and the setuid/setgid bits are set, then we should clear those bits as well. Might not hurt to do this on write and truncate as well.

Note that we'll need to fix this both in the client and MDS.

Testing a PR now that fixes the setattr codepaths. That's pretty simple to do from those codepaths since we're sending a setattr anyway.

The write path is a little harder. We may not have any auth caps at all and doing a read/modify/write cycle is racy if we can't get them.

One idea might be to declare a new CEPH_SETATTR_KILLSUGID flag that we can send in a SETATTR request that would just do the bit clearing without requiring a read/modify/write cycle. That would work and should close the race windows (I think).

Another idea (more radical) might be to have the MDS clear the bits when it hands out file write caps, and recall any write caps when someone tries to set those bits. That would be more strict than what posix requires. It's also possibly more strict than what it allows as we'd end up clearing those bits when the file was opened for write. Still, that might be worthwhile -- generally you don't want files with these bits set being modified anyway.

Another thing we need to sort out is that the write (and truncate) paths are different in that you're not required to clear those bits if you have "appropriate privileges". For Linux, that's CAP_FSETID. I'm not sure how to interpret that in ceph-land. For now, we'll just clear it for root as well. I think that's a sane thing to do.

Merged the chown part of this, and I think I have sorted out the problems I was having with the truncate and write codepaths. I'll be testing those later today. Basically, what I'm doing to do for write and truncate is adding a new CEPH_SETATTR_KILL_SGUID flag. When that's passed to the mds, it will grab Ax caps and clear the bits. That should ensure that we can't race during a read/modify/write cycle of the mode.

For write, we grab As when we get Fw caps and check the mode. If either bit is set, then we issue a __setattr to ask it to clear them. If we have Ax caps as well, then we'll cache the mode change. Otherwise we issue a setattr to the server for CEPH_SETATTR_KILL_SGUID, to have it clear the mode.

For CEPH_SETATTR_SIZE, we do the same -- cache the mode change if we have Ax caps, or ask the server to clear it for us if not.

@Jeff Lee: Which commit fixes the issue/should be backported to jewel?

master PR: https://github.com/ceph/ceph/pull/12331

Sorry for the late response. At the very least, we need these commits:

commit 18d2499d6c85a10b4b54f3b8c335cddf86c4588f
Author: Jeff Layton <jlayton@redhat.com>
Date:   Mon Dec 5 14:11:44 2016 -0500

    client: drop setuid/setgid bits on ownership change

commit 6da72500882d9749cb2be6eaa2568e6fe6e5ff4d
Author: Jeff Layton <jlayton@redhat.com>
Date:   Mon Dec 5 14:19:23 2016 -0500

    mds: clear setuid/setgid bits on ownership changes

That should ensure that we clear the setuid/setgid bits on ownership changeseven when a buggy kernel is in use. The other cases are still handled correctly by the kernel so I don't see a pressing need for them in jewel.