Re: [PATCH v5 02/30] fs: pass dentry to set acl method

["Mickaël Salaün" <mic@digikod.net>]

On 18/11/2022 11:33, Christian Brauner wrote:
> 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.

Of course, I didn't realize this series was in -next. Please take a look 
at my proposal in the other thread. Thanks.

> 
> Thanks!
> Christian
> 
>>
>> [1]
>> https://lore.kernel.org/all/1373bbe5-16b1-bf0e-5f92-14c31cb94897@huawei.com/
>>
>>
>> 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@kernel.org [1]
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
>>> ---
>>>
>>> Notes:
>>>       /* v2 */
>>>       Christoph Hellwig <hch@lst.de>:
>>>       - 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(-)