misc. POSIX compatibility fixes for chown changes #17053
Conversation
libfuse already does not set FUSE_SET_ATTR_UID if the chown uid is -1. However, another libcephfs client could call setattr directly with -1 as the uid/gid so we should handle that potential case. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
According to [1], the suid/sgid should be cleared if any of the executable bits are set. Found this while experimenting for [2]. [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/chown.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=1480182 Fixes: http://tracker.ceph.com/issues/21004 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
According to [1], these bits should be cleared regardless of any exe bits on the file. Also, add the required non-zero write check. [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/pwrite.html Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Partially addresses POSIX test failures in [1] due to the config setting being false by default. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1480182#c6 Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
| @@ -9018,8 +9018,7 @@ int Client::_write(Fh *f, int64_t offset, uint64_t size, const char *buf, | |||
| return r; | |||
|
|
|||
| /* clear the setuid/setgid bits, if any */ | |||
| if (unlikely((in->mode & S_ISUID) || | |||
| (in->mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) { | |||
| if (unlikely(in->mode & (S_ISUID|S_ISGID)) && size > 0) { | |||
There was a problem hiding this comment.
I'm not sure about this. We generally leave setgid bit set if group execute bit is not set. This combination makes no sense, so it was used as an indicator of mandatory locking. FWIW, POSIX omits this from the spec, but it's historical Linux and Unix behavior. It's probably harmless to leave it set in that case and may keep you from running afoul of some other testcases. I don't feel too strongly either way though.
There was a problem hiding this comment.
According to [1] from the 1997 POSIX spec, this is the right behavior?
[1] http://pubs.opengroup.org/onlinepubs/7908799/xsh/write.html
There was a problem hiding this comment.
Perhaps I'm misunderstanding the spec (I'm confused by your saying POSIX omits this)
There was a problem hiding this comment.
I agree with Jeff. keeping the logic sync with kernel should be OK
There was a problem hiding this comment.
I'm inclined to keep this change because (a) no one should be using mandatory locking this way on libcephfs and (b) it's simply stricter than what Linux does and not incompatible.
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
* refs/remotes/upstream/pull/17053/head: qa: add chown test clearing S_ISGID and S_ISUID ceph-fuse: load supplementary groups by default client: clear suid/sgid bits on non-zero write client: add missing space in debug msg cephfs: clear suid/sgid if regular file is exe client: refactor clear set uid/gid if -1 Reviewed-by: Zheng Yan <zyan@redhat.com>
Relating to;
http://tracker.ceph.com/issues/21004
https://bugzilla.redhat.com/show_bug.cgi?id=1480182#c6