| Message ID | 20230831112431.2998368-2-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use exclusive lock for file_remove_privs | expand |
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 s/priveliges/privileges/ s/that a requires/that requires/ > 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); > +} > +EXPORT_SYMBOL_GPL(file_needs_remove_privs); > + > static int __remove_privs(struct mnt_idmap *idmap, > struct dentry *dentry, int kill) > { > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 562f2623c9c9..9245f0de00bc 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2721,6 +2721,7 @@ extern struct inode *new_inode_pseudo(struct super_block *sb); > extern struct inode *new_inode(struct super_block *sb); > extern void free_inode_nonrcu(struct inode *inode); > extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *); > +int file_needs_remove_privs(struct file *); > extern int file_remove_privs(struct file *); > int setattr_should_drop_sgid(struct mnt_idmap *idmap, > const struct inode *inode); > -- > 2.39.2 >
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...
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? And thanks for typo/grammar annotations, I'm going to wait for btrfs feedback before I'm going to send a new version. Thanks, Bernd
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.
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); +} +EXPORT_SYMBOL_GPL(file_needs_remove_privs); + static int __remove_privs(struct mnt_idmap *idmap, struct dentry *dentry, int kill) { diff --git a/include/linux/fs.h b/include/linux/fs.h index 562f2623c9c9..9245f0de00bc 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2721,6 +2721,7 @@ extern struct inode *new_inode_pseudo(struct super_block *sb); extern struct inode *new_inode(struct super_block *sb); extern void free_inode_nonrcu(struct inode *inode); extern int setattr_should_drop_suidgid(struct mnt_idmap *, struct inode *); +int file_needs_remove_privs(struct file *); extern int file_remove_privs(struct file *); int setattr_should_drop_sgid(struct mnt_idmap *idmap, const struct inode *inode);
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(+)