Bug #21004: fs: client/mds has wrong check to clear S_ISGID on chown - CephFS - Ceph

Custom queries

Bug queue
Bug triage
CephFS Bug Triage
CephFS task-easy
CephFS: Available Easy Issues
CephFS: Documentation
Crash queue
Crash triage
Feedback
My issues
Need Review
Pending backports
Product Backlog Scrub
Quincy Backports
Release: Quincy: Backports (open)
Release: Reef: Backports (open)
Release: Squid: Backports (open)
Release: Squid: Features
Release: Squid: Open Issues
Release: Tentacle: Features
Release: Tentacle: Open Issues
Zee CephFS Ticket Well

Actions

Copy link

Bug #21004

closed

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

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

Status:

Resolved

Priority:

High

Assignee:

Patrick Donnelly

Category:

Target version:

Ceph - v12.2.0

% Done:

Source:

Q/A

Tags:

Backport:

luminous

Regression:

Severity:

3 - minor

Reviewed:

Affected Versions:

Ceph - v12.2.0

ceph-qa-suite:

Component(FS):

MDS, ceph-fuse, kceph

Labels (FS):

Pull request ID:

Crash signature (v1):

Crash signature (v2):

Description

Reported in: https://bugzilla.redhat.com/show_bug.cgi?id=1480182

This causes the failure in test 88 from https://bugzilla.redhat.com/attachment.cgi?id=1311693

Related issues 1 (0 open — 1 closed)

Copied to CephFS - Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chown

Resolved

Nathan Cutler

Actions

Issue # Delay: days Cancel

History
Notes
Property changes

Actions

Copy link

Updated by Patrick Donnelly over 6 years ago

Status changed from New to In Progress

Actions

Copy link

Updated by Patrick Donnelly over 6 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

Copy link

Updated by Zheng Yan over 6 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

Copy link