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.

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

Linux 6.2 compat: set_acl arg2 is "struct dentry *" and no longer "struct inode *" #14415

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

ckane
Copy link
Contributor

@ckane ckane commented Jan 21, 2023

Motivation and Context

As of (at least) Linux 6.2rc4, the ZFS kernel module is no longer compiling the set_acl implementation as all of the HAVE_SET_ACL and related tests are failing in configure.

Results in the following:

[ckane@system foo]$ sudo setfacl -m u:root:w a
[sudo] password for ckane: 
setfacl: a: Operation not supported

Description

Linux 6.2 changes the second argument of the set_acl operation to be a "struct dentry *" rather than a "struct inode ". The inode parameter is still available as dentry->d_inode, so adjust the call to the _impl function call to dereference and pass that pointer to it.

Related to: #14403

How Has This Been Tested?

Testing on my machine, and it doesn't break. However, I still experience the failure when attempting to run setfacl on my system. However, I have not really made use of ACLs so I am putting together the PR to discuss the issue and diagnose further, tracking source code commits.

Need to also run the ZFS test suite and ensure that tests/zfs-tests/tests/functional/acl/posix yield a PASS, which I suspect to not be the case presently.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ckane
Copy link
Contributor Author

ckane commented Jan 21, 2023

I tested setfacl out on my Linux 6.1.7 kernel, and I was able to successfully set the ACL there, and when I boot back into 6.2rc4 with these changes applied, I can use ls to see that the ACL is applied:

[ckane@system foo]$ ls -l
total 1
-rw-rw-r--+ 1 ckane ckane 0 Jan 21 18:38 a

However, I still cannot set the ACL under 6.2 after these changes, so there must be some more implementation(s) that's missing.

@satmandu
Copy link
Contributor

@ckane If you already see the issue, I'll wait for more input before replicating what you've done on my end...

@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch from ec35b75 to 7443949 Compare January 22, 2023 17:05
config/kernel-acl.m4 Outdated Show resolved Hide resolved
@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch from 7443949 to ed0901d Compare January 22, 2023 19:46
@satmandu
Copy link
Contributor

The latest commit appears to fix the minimal touch a ; sudo setfacl -m u:root:w a test case for me on Linux kernel 6.2-rc5.

I'll continue to use it and see it any other issues crop up...

@ckane
Copy link
Contributor Author

ckane commented Jan 22, 2023

The latest commit appears to fix the minimal touch a ; sudo setfacl -m u:root:w a test case for me on Linux kernel 6.2-rc5.

I'll continue to use it and see it any other issues crop up...

Thanks for the report! I wonder if there was also a bug upstream in rc4 that got fixed in rc5, related to those API changes

@ckane
Copy link
Contributor Author

ckane commented Jan 22, 2023

@behlendorf I think this one is ready for your review

@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch from ed0901d to 7a64db5 Compare January 23, 2023 02:05
@ckane
Copy link
Contributor Author

ckane commented Jan 23, 2023

@satmandu - can you re-try with the latest version of this patch? Earlier I had accidentally included some stylistic modifications to zfs_acl.c that I did not mean to include in the patch, that were from a code cleanup commit I was looking at earlier when trying to diagnose what might have changed. My latest push has removed those changes.

@satmandu
Copy link
Contributor

@satmandu - can you re-try with the latest version of this patch? Earlier I had accidentally included some stylistic modifications to zfs_acl.c that I did not mean to include in the patch, that were from a code cleanup commit I was looking at earlier when trying to diagnose what might have changed. My latest push has removed those changes.

The latest version appears to work just fine:

satadru@cheekon ~$ uname -a
Linux cheekon 6.2.0-rc5 #1 SMP PREEMPT_DYNAMIC Sun Jan 22 12:26:53 EST 2023 x86_64 x86_64 x86_64 GNU/Linux
satadru@cheekon ~$ rm a
rm: cannot remove 'a': No such file or directory
satadru@cheekon ~$ touch a ; sudo setfacl -m u:root:w a
satadru@cheekon ~$ getfacl a
# file: a
# owner: satadru
# group: satadru
user::rw-
user:root:-w-
group::rw-
mask::rw-
other::r--

@satmandu
Copy link
Contributor

FYI the packages I built to test this PR on my Ubuntu 22.10 system are here:

https://launchpad.net/~satadru-umich/+archive/ubuntu/zfs-experimental/

config/kernel-acl.m4 Show resolved Hide resolved
@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch 2 times, most recently from 8036360 to 0167eb0 Compare January 23, 2023 17:01
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks again for sorting this out. Hopefully these interfaces won't change again, but at least since the code is now using ZFS_LINUX_REQUIRE_API in the check we'll detect it immediately if they do.

config/kernel-acl.m4 Show resolved Hide resolved
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 23, 2023
@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch from 0167eb0 to ae4eb5c Compare January 23, 2023 20:00
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 23, 2023
Linux 6.2 changes the second argument of the set_acl operation to be a
"struct dentry *" rather than a "struct inode *". The inode* parameter
is still available as dentry->d_inode, so adjust the call to the _impl
function call to dereference and pass that pointer to it.

Also document that the get_acl -> get_inode_acl member name change from
commit 884a693 was an API change also introduced in Linux 6.2.

Signed-off-by: Coleman Kane <ckane@colemankane.org>
@ckane ckane force-pushed the linux-6.2-set_acl_dentry-fix branch from ae4eb5c to 1cff3de Compare January 24, 2023 13:56
@behlendorf behlendorf merged commit 9cd71c8 into openzfs:master Jan 24, 2023
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 25, 2023
Linux 6.2 changes the second argument of the set_acl operation to be a
"struct dentry *" rather than a "struct inode *". The inode* parameter
is still available as dentry->d_inode, so adjust the call to the _impl
function call to dereference and pass that pointer to it.

Also document that the get_acl -> get_inode_acl member name change from
commit 884a693 was an API change also introduced in Linux 6.2.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Signed-off-by: Coleman Kane <ckane@colemankane.org>
Closes openzfs#14415
@ckane ckane deleted the linux-6.2-set_acl_dentry-fix branch January 25, 2023 04:03
@ckane
Copy link
Contributor Author

ckane commented Jan 25, 2023

Adding a note here that I also confirmed that on 6.2rc4 setfacl won't work, regardless of the patch, and there has been some fix to the ACL code added in 6.2rc5 that, along with this patch, fixes the bug on ZFS.

lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
Linux 6.2 changes the second argument of the set_acl operation to be a
"struct dentry *" rather than a "struct inode *". The inode* parameter
is still available as dentry->d_inode, so adjust the call to the _impl
function call to dereference and pass that pointer to it.

Also document that the get_acl -> get_inode_acl member name change from
commit 884a693 was an API change also introduced in Linux 6.2.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Signed-off-by: Coleman Kane <ckane@colemankane.org>
Closes openzfs#14415
Wenri pushed a commit to Wenri/zfs that referenced this pull request Sep 13, 2023
Linux 6.2 changes the second argument of the set_acl operation to be a
"struct dentry *" rather than a "struct inode *". The inode* parameter
is still available as dentry->d_inode, so adjust the call to the _impl
function call to dereference and pass that pointer to it.

Also document that the get_acl -> get_inode_acl member name change from
commit 884a693 was an API change also introduced in Linux 6.2.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Signed-off-by: Coleman Kane <ckane@colemankane.org>
Closes openzfs#14415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants