On Fri, Nov 18, 2022 at 11:09:20AM +0100, Mickaël Salaün wrote: > Hi Christian, > > We are working on updating the security_inode_*attr LSM hooks to use path > instead of inode [1]. Indeed, this is required for path-based LSMs such as > Landlock, AppArmor and Tomoyo to make sense of attr/xattr accesses. Could > you please update this new ACL API to use struct path instead of struct > dentry? Hey Michael, These patches have been sitting in -next since -rc1 and we are at -rc6 this weekend so this request is too late for the coming merge window. We can't do a fundamental change this late. I would ask you to please do these changes in a separate series next cycle. And this will need a separate discussion among the LSM people and VFS reviews anyway. Thanks! Christian > > [1] > https://lore.kernel.org/all/1373bbe5-16b1-bf0e-5f92-14c31cb94897@xxxxxxxxxx/ > > > On 18/10/2022 13:56, Christian Brauner wrote: > > The current way of setting and getting posix acls through the generic > > xattr interface is error prone and type unsafe. The vfs needs to > > interpret and fixup posix acls before storing or reporting it to > > userspace. Various hacks exist to make this work. The code is hard to > > understand and difficult to maintain in it's current form. Instead of > > making this work by hacking posix acls through xattr handlers we are > > building a dedicated posix acl api around the get and set inode > > operations. This removes a lot of hackiness and makes the codepaths > > easier to maintain. A lot of background can be found in [1]. > > > > Since some filesystem rely on the dentry being available to them when > > setting posix acls (e.g., 9p and cifs) they cannot rely on set acl inode > > operation. But since ->set_acl() is required in order to use the generic > > posix acl xattr handlers filesystems that do not implement this inode > > operation cannot use the handler and need to implement their own > > dedicated posix acl handlers. > > > > Update the ->set_acl() inode method to take a dentry argument. This > > allows all filesystems to rely on ->set_acl(). > > > > As far as I can tell all codepaths can be switched to rely on the dentry > > instead of just the inode. Note that the original motivation for passing > > the dentry separate from the inode instead of just the dentry in the > > xattr handlers was because of security modules that call > > security_d_instantiate(). This hook is called during > > d_instantiate_new(), d_add(), __d_instantiate_anon(), and > > d_splice_alias() to initialize the inode's security context and possibly > > to set security.* xattrs. Since this only affects security.* xattrs this > > is completely irrelevant for posix acls. > > > > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@xxxxxxxxxx [1] > > Reviewed-by: Christoph Hellwig <hch@xxxxxx> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx> > > --- > > > > Notes: > > /* v2 */ > > Christoph Hellwig <hch@xxxxxx>: > > - Split orangefs into a preparatory patch. > > /* v3 */ > > unchanged > > /* v4 */ > > unchanged > > /* v5 */ > > unchanged > > > > Documentation/filesystems/vfs.rst | 2 +- > > fs/bad_inode.c | 2 +- > > fs/btrfs/acl.c | 3 ++- > > fs/btrfs/ctree.h | 2 +- > > fs/btrfs/inode.c | 2 +- > > fs/ceph/acl.c | 3 ++- > > fs/ceph/inode.c | 2 +- > > fs/ceph/super.h | 2 +- > > fs/ext2/acl.c | 3 ++- > > fs/ext2/acl.h | 2 +- > > fs/ext2/inode.c | 2 +- > > fs/ext4/acl.c | 3 ++- > > fs/ext4/acl.h | 2 +- > > fs/ext4/inode.c | 2 +- > > fs/f2fs/acl.c | 4 +++- > > fs/f2fs/acl.h | 2 +- > > fs/f2fs/file.c | 2 +- > > fs/fuse/acl.c | 3 ++- > > fs/fuse/fuse_i.h | 2 +- > > fs/gfs2/acl.c | 3 ++- > > fs/gfs2/acl.h | 2 +- > > fs/gfs2/inode.c | 2 +- > > fs/jffs2/acl.c | 3 ++- > > fs/jffs2/acl.h | 2 +- > > fs/jffs2/fs.c | 2 +- > > fs/jfs/acl.c | 3 ++- > > fs/jfs/file.c | 2 +- > > fs/jfs/jfs_acl.h | 2 +- > > fs/ksmbd/smb2pdu.c | 4 ++-- > > fs/ksmbd/smbacl.c | 4 ++-- > > fs/ksmbd/vfs.c | 15 ++++++++------- > > fs/ksmbd/vfs.h | 4 ++-- > > fs/nfs/nfs3_fs.h | 2 +- > > fs/nfs/nfs3acl.c | 3 ++- > > fs/nfsd/nfs2acl.c | 4 ++-- > > fs/nfsd/nfs3acl.c | 4 ++-- > > fs/nfsd/vfs.c | 4 ++-- > > fs/ntfs3/file.c | 2 +- > > fs/ntfs3/ntfs_fs.h | 4 ++-- > > fs/ntfs3/xattr.c | 9 +++++---- > > fs/ocfs2/acl.c | 3 ++- > > fs/ocfs2/acl.h | 2 +- > > fs/orangefs/acl.c | 5 +++-- > > fs/orangefs/inode.c | 7 ++++--- > > fs/orangefs/orangefs-kernel.h | 4 ++-- > > fs/posix_acl.c | 18 +++++++++++------- > > fs/reiserfs/acl.h | 6 +++--- > > fs/reiserfs/inode.c | 2 +- > > fs/reiserfs/xattr_acl.c | 9 ++++++--- > > fs/xfs/xfs_acl.c | 3 ++- > > fs/xfs/xfs_acl.h | 2 +- > > fs/xfs/xfs_iops.c | 10 ++++++---- > > include/linux/fs.h | 2 +- > > include/linux/posix_acl.h | 12 ++++++------ > > mm/shmem.c | 2 +- > > 55 files changed, 119 insertions(+), 93 deletions(-)
