Bug #21004
fs: client/mds has wrong check to clear S_ISGID on chown
% 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):
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
History
#1 Updated by Patrick Donnelly over 5 years ago
- Status changed from New to In Progress
#2 Updated by Patrick Donnelly over 5 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.
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/
#3 Updated by Zheng Yan over 5 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)
#4 Updated by Ken Dreyer over 5 years ago
- Backport set to luminous
#5 Updated by Patrick Donnelly over 5 years ago
#6 Updated by Patrick Donnelly over 5 years ago
- Status changed from In Progress to Fix Under Review
#7 Updated by Patrick Donnelly over 5 years ago
- Status changed from Fix Under Review to Pending Backport
#8 Updated by Nathan Cutler over 5 years ago
- Copied to Backport #21107: luminous: fs: client/mds has wrong check to clear S_ISGID on chown added
#9 Updated by Nathan Cutler over 5 years ago
- Status changed from Pending Backport to Resolved