Project

General

Profile

Actions

Bug #21004

closed

fs: client/mds has wrong check to clear S_ISGID on chown

Added by Patrick Donnelly almost 7 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
High
Category:
-
Target version:
% Done:

0%

Source:
Q/A
Tags:
Backport:
luminous
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Component(FS):
MDS, ceph-fuse, kceph
Labels (FS):
Pull request ID:
Crash signature (v1):
Crash signature (v2):


Related issues 1 (0 open1 closed)

Copied to CephFS - Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chownResolvedNathan CutlerActions
Actions #1

Updated by Patrick Donnelly almost 7 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Patrick Donnelly almost 7 years ago

  • Subject changed from fs: client/mds do not always clear S_ISGID on chown to fs: client/mds has wrong check to clear S_ISGID on chown

Well actually the test above fails because the chown was a no-op due to an earlier chown failure. In any case I've found that S_ISGID is not cleared correctly which I identified in my own testing.

https://github.com/ceph/ceph/blob/a155b84508bb8dfff7180b73748f445e6d2d89ad/src/client/Client.cc#L6704

The condition should be:

-    } else if (kill_sguid && S_ISREG(in->mode)) {
+    } else if (kill_sguid && S_ISREG(in->mode) && (in->mode & (S_IXUSR|S_IXGRP|S_IXOTH))) {
       /* Must squash the any setuid/setgid bits with an ownership change */
-      in->mode &= ~S_ISUID;
-      if ((in->mode & (S_ISGID|S_IXGRP)) == (S_ISGID|S_IXGRP))
-       in->mode &= ~S_ISGID;
+      in->mode &= ~(S_ISUID|S_ISGID);

According to: http://pubs.opengroup.org/onlinepubs/9699919799/

Actions #3

Updated by Zheng Yan almost 7 years ago

The logical is from kernel_src/fs/attr.c

        if (ia_valid & ATTR_KILL_SUID) {
                if (mode & S_ISUID) {
                        ia_valid = attr->ia_valid |= ATTR_MODE;
                        attr->ia_mode = (inode->i_mode & ~S_ISUID);
                }
        }
        if (ia_valid & ATTR_KILL_SGID) {
                if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
                        if (!(ia_valid & ATTR_MODE)) {
                                ia_valid = attr->ia_valid |= ATTR_MODE;
                                attr->ia_mode = inode->i_mode;
                        }
                        attr->ia_mode &= ~S_ISGID;
                }
        }

Besides, it's possible that mds does not handle setattr request for chmod correctly. (client does not have CEPH_CAP_AUTH_EXCL)

Actions #4

Updated by Ken Dreyer almost 7 years ago

  • Backport set to luminous
Actions #6

Updated by Patrick Donnelly almost 7 years ago

  • Status changed from In Progress to Fix Under Review
Actions #7

Updated by Patrick Donnelly almost 7 years ago

  • Status changed from Fix Under Review to Pending Backport
Actions #8

Updated by Nathan Cutler almost 7 years ago

  • Copied to Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chown added
Actions #9

Updated by Nathan Cutler over 6 years ago

  • Status changed from Pending Backport to Resolved
Actions

Also available in: Atom PDF