Re: [PATCH 1/2] fs: Add and export file_needs_remove_privs

[Christian Brauner <brauner@kernel.org>]

On Thu, Aug 31, 2023 at 04:17:01PM +0200, Bernd Schubert wrote:
> 
> 
> On 8/31/23 15:40, Christian Brauner wrote:
> > On Thu, Aug 31, 2023 at 01:24:30PM +0200, Bernd Schubert wrote:
> > > File systems want to hold a shared lock for DIO writes,
> > > but may need to drop file priveliges - that a requires an
> > > exclusive lock. The new export function file_needs_remove_privs()
> > > is added in order to first check if that is needed.
> > > 
> > > Cc: Miklos Szeredi <miklos@szeredi.hu>
> > > Cc: Dharmendra Singh <dsingh@ddn.com>
> > > Cc: Josef Bacik <josef@toxicpanda.com>
> > > Cc: linux-btrfs@vger.kernel.org
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> > > ---
> > >   fs/inode.c         | 8 ++++++++
> > >   include/linux/fs.h | 1 +
> > >   2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/fs/inode.c b/fs/inode.c
> > > index 67611a360031..9b05db602e41 100644
> > > --- a/fs/inode.c
> > > +++ b/fs/inode.c
> > > @@ -2013,6 +2013,14 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
> > >   	return mask;
> > >   }
> > > +int file_needs_remove_privs(struct file *file)
> > > +{
> > > +	struct dentry *dentry = file_dentry(file);
> > > +
> > > +	return dentry_needs_remove_privs(file_mnt_idmap(file), dentry);
> > 
> > Ugh, I wanted to propose to get rid of this dentry dance but I propsed
> > that before and remembered it's because of __vfs_getxattr() which is
> > called from the capability security hook that we need it...
> 
> Is there anything specific you are suggesting?

No, it's not actionable for you here. It would require adding inode
methods to set and get filesystem capabilities and then converting it in
such a way that we don't need to rely on passing dentries around. That's
a separate larger patchset that we would need with surgery across a
bunch of filesystems and the vfs - Seth (Forshee) has been working on this.

The callchains are just pointless which I remembered when I saw the
patchset:

file_needs_remove_privs(file)
-> dentry_needs_remove_privs(dentry)
   -> inode = d_inode(dentry)
      // do inode stuff
      // security_needs_*(dentry)

point is ideally we shouldn't need the dentry in *remove_privs() at all.