Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DAOS-15484 dfs: store sticky/suid/sgid bits on setattr #14028

Merged
merged 4 commits into from
Mar 25, 2024
Merged

DAOS-15484 dfs: store sticky/suid/sgid bits on setattr #14028

merged 4 commits into from
Mar 25, 2024

Conversation

johannlombardi
Copy link
Contributor

Allow the sticky, suid and sgid bits to be stored in dfs_osetattr. Even if libdfs does not support those bits, it allows dfuse to support them via the kernel.

The lack of sgid support cause spack to fail over dfuse as reported in the jira ticket.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@johannlombardi
DAOS-15484 dfs: store sticky/suid/sgid bits on setattr
adb09b4 
Allow the sticky, suid and sgid bits to be stored in dfs_osetattr.
Even if libdfs does not support those bits, it allows dfuse to
support them via the kernel.

The lack of sgid support cause spack to fail over dfuse as
reported in the jira ticket.

Signed-off-by: Johann Lombardi <[email protected]>
@johannlombardi johannlombardi requested review from a team as code owners March 20, 2024 14:30
@github-actions GitHub Actions
Copy link

Ticket title is 'spack failed on DAOS'
Status is 'Open'
https://daosio.atlassian.net/browse/DAOS-15484

@mchaarawi
Copy link
Contributor

mchaarawi commented Mar 20, 2024 •
edited
Loading

could you please re-push and add this commit pragma
Features: dfs datamover

mchaarawi
mchaarawi reviewed Mar 20, 2024
View reviewed changes
src/include/daos_fs.h Outdated
* symlink the link itself is modified. See dfs_stat() for which entries
* are filled.
* Since libdfs does not perform any POSIX permission enforcement, it just
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not true technically. DFS does permission enforcement. see:
https://github.com/daos-stack/daos/blob/master/src/client/dfs/obj.c#L19

so we should adjust this slightly to say that since we don't run executables over DFS, we can allow SUID/SGID to be set without being enforced. for the sticky bit, im afraid we need to probably keep that check.

mchaarawi
mchaarawi reviewed Mar 20, 2024
View reviewed changes
docs/user/filesystem.md Outdated
Comment on lines 63 to 64
* POSIX permissions, setuid/gid programs, sticky bit, POSIX ACLs, supplementary groups are not
supported inside an encapsulated namespace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update this also similarly

@johannlombardi
- Fail when sticky bit it set (ENOTSUP instead of EINVAL)
e8f07dd 
- Update documentation

Features: dfs datamover

Signed-off-by: Johann Lombardi <[email protected]>
@johannlombardi
Fix typo
205ae37 
Features: dfs datamover

Signed-off-by: Johann Lombardi <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/378/log

@daosbuild1
Copy link
Collaborator

Test stage Build on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/384/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/289/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/292/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/354/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14028/3/execution/node/351/log

@johannlombardi
Fix another typo
59e6805 
Features: dfs datamover

Signed-off-by: Johann Lombardi <[email protected]>
@mchaarawi mchaarawi requested a review from daltonbohning March 21, 2024 16:00
daltonbohning
daltonbohning reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mchaarawi I guess we should unmask the gid and uid here and just let DFS handle it now? This is for daos fs copy

daos/src/utils/daos_hdlr.c

Lines 534 to 537 in 1349dbf

if (!ignore_unsup && mode & (S_ISVTX | S_ISGID | S_ISUID)) {
(*num_chmod_enotsup)++;
}
mode &= ~(S_ISVTX | S_ISGID | S_ISUID);

@mchaarawi
Copy link
Contributor

@mchaarawi I guess we should unmask the gid and uid here and just let DFS handle it now? This is for daos fs copy

daos/src/utils/daos_hdlr.c

Lines 534 to 537 in 1349dbf

if (!ignore_unsup && mode & (S_ISVTX | S_ISGID | S_ISUID)) {
(*num_chmod_enotsup)++;
}
mode &= ~(S_ISVTX | S_ISGID | S_ISUID);

yes that was my thought. we can do that separately. probably something similar can be done in mfu? but maybe leave the latter for 2.4 support.

@daltonbohning
Copy link
Contributor

@mchaarawi I guess we should unmask the gid and uid here and just let DFS handle it now? This is for daos fs copy

daos/src/utils/daos_hdlr.c

Lines 534 to 537 in 1349dbf

if (!ignore_unsup && mode & (S_ISVTX | S_ISGID | S_ISUID)) {
(*num_chmod_enotsup)++;
}
mode &= ~(S_ISVTX | S_ISGID | S_ISUID);

yes that was my thought. we can do that separately. probably something similar can be done in mfu? but maybe leave the latter for 2.4 support.

I don't think MFU has any special handling of this. If MFU tries to chmod and it fails because of this, it just logs an error and continues. But we could handle similar to how we do for fs copy, and wrap it with a version check.

@johannlombardi johannlombardi requested a review from a team March 25, 2024 09:32
@mchaarawi mchaarawi merged commit a37df0b into master Mar 25, 2024
49 checks passed
@mchaarawi mchaarawi deleted the jlo/setgid branch March 25, 2024 11:44
johannlombardi added a commit that referenced this pull request Mar 25, 2024
@johannlombardi
DAOS-15484 dfs: store sticky/suid/sgid bits on setattr (#14028)
b3174e9 
Allow the suid and sgid bits to be stored in dfs_osetattr.
Even if libdfs does not support those bits, it allows dfuse to
support them via the kernel.

The lack of sgid support cause spack to fail over dfuse as
reported in the jira ticket.

Signed-off-by: Johann Lombardi <[email protected]>
jolivier23 pushed a commit that referenced this pull request Apr 10, 2024
@johannlombardi @jolivier23
DAOS-15484 dfs: store sticky/suid/sgid bits on setattr (#14028)
e6b0a85 
Allow the suid and sgid bits to be stored in dfs_osetattr.
Even if libdfs does not support those bits, it allows dfuse to
support them via the kernel.

The lack of sgid support cause spack to fail over dfuse as
reported in the jira ticket.

Change-Id: I76b41d9b231fa2b7f1d434d6ae06e6252cadc2b4
Signed-off-by: Johann Lombardi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchaarawi mchaarawi mchaarawi approved these changes

@daltonbohning daltonbohning daltonbohning approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johannlombardi @mchaarawi @daosbuild1 @daltonbohning