[PATCH v2 0/2] Use exclusive lock for file_remove_privs

[PATCH v2 0/2] Use exclusive lock for file_remove_privs

[Bernd Schubert]

While adding shared direct IO write locks to fuse Miklos noticed
that file_remove_privs() needs an exclusive lock. I then
noticed that btrfs actually has the same issue as I had in my patch,
it was calling into that function with a shared lock.
This series adds a new exported function file_needs_remove_privs(),
which used by the follow up btrfs patch and will be used by the
DIO code path in fuse as well. If that function returns any mask
the shared lock needs to be dropped and replaced by the exclusive
variant.

Note: Compilation tested only.

v2:
Already check for IS_NOSEC in btrfs_direct_write before the first
lock is taken.
Slight modification to make the code easier to read (boolean pointer
is passed to btrfs_write_check, instead of flags).

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


Bernd Schubert (2):
  fs: Add and export file_needs_remove_privs
  btrfs: file_remove_privs needs an exclusive lock

 fs/btrfs/file.c    | 37 +++++++++++++++++++++++++++++--------
 fs/inode.c         |  8 ++++++++
 include/linux/fs.h |  1 +
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.39.2

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

[Bernd Schubert]

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);
+}
+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

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

[Christian Brauner]

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
> 

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

[Christian Brauner]

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...

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

[Bernd Schubert]

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

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

[Christian Brauner]

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.

[PATCH 2/2] btrfs: file_remove_privs needs an exclusive lock

[Bernd Schubert]

file_remove_privs might call into notify_change(), which
requires to hold an exclusive lock.
In order to keep the shared lock for most IOs it now first
checks if privilege changes are needed, then switches to
the exclusive lock, rechecks and only then calls file_remove_privs.
This makes usage of the new exported function
file_needs_remove_privs().

The file_remove_privs code path is not optimized, under the
assumption that it would be a rare call (file_remove_privs
calls file_needs_remove_privs a 2nd time).

Fixes: e9adabb9712e ("btrfs: use shared lock for direct writes within EOF")
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Goldwyn Rodrigues <rgoldwyn@suse.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/btrfs/file.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fd03e689a6be..3162ec245d57 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1125,7 +1125,7 @@ static void update_time_for_write(struct inode *inode)
 }
 
 static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
-			     size_t count)
+			     size_t count, bool *shared_lock)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
@@ -1145,9 +1145,17 @@ static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from,
 	    !(BTRFS_I(inode)->flags & (BTRFS_INODE_NODATACOW | BTRFS_INODE_PREALLOC)))
 		return -EAGAIN;
 
-	ret = file_remove_privs(file);
-	if (ret)
-		return ret;
+	ret = file_needs_remove_privs(file);
+	if (ret) {
+		if (shared_lock && *shared_lock) {
+			*shared_lock = false;
+			return -EAGAIN;
+		}
+
+		ret = file_remove_privs(file);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * We reserve space for updating the inode when we reserve space for the
@@ -1204,7 +1212,7 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb,
 	if (ret <= 0)
 		goto out;
 
-	ret = btrfs_write_check(iocb, i, ret);
+	ret = btrfs_write_check(iocb, i, ret, NULL);
 	if (ret < 0)
 		goto out;
 
@@ -1462,13 +1470,20 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t err;
 	unsigned int ilock_flags = 0;
 	struct iomap_dio *dio;
+	bool shared_lock;
 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		ilock_flags |= BTRFS_ILOCK_TRY;
 
-	/* If the write DIO is within EOF, use a shared lock */
-	if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode))
+	/* If the write DIO is within EOF, use a shared lock and also only
+	 * if security bits will likely not be dropped. Either will need
+	 * to be rechecked after the lock was acquired.
+	 */
+	if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode) &&
+	    IS_NOSEC(inode)) {
 		ilock_flags |= BTRFS_ILOCK_SHARED;
+		shared_lock = true;
+	}
 
 relock:
 	err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
@@ -1481,8 +1496,15 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 		return err;
 	}
 
-	err = btrfs_write_check(iocb, from, err);
+	err = btrfs_write_check(iocb, from, err, &shared_lock);
 	if (err < 0) {
+		if (err == -EAGAIN && ilock_flags & BTRFS_ILOCK_SHARED &&
+		    !shared_lock) {
+			btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
+			ilock_flags &= ~BTRFS_ILOCK_SHARED;
+			goto relock;
+		}
+
 		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
 		goto out;
 	}
@@ -1496,6 +1518,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	    pos + iov_iter_count(from) > i_size_read(inode)) {
 		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
 		ilock_flags &= ~BTRFS_ILOCK_SHARED;
+		shared_lock = false;
 		goto relock;
 	}
 
@@ -1632,7 +1655,7 @@ static ssize_t btrfs_encoded_write(struct kiocb *iocb, struct iov_iter *from,
 	if (ret || encoded->len == 0)
 		goto out;
 
-	ret = btrfs_write_check(iocb, from, encoded->len);
+	ret = btrfs_write_check(iocb, from, encoded->len, NULL);
 	if (ret < 0)
 		goto out;
 
-- 
2.39.2

Re: [PATCH v2 0/2] Use exclusive lock for file_remove_privs

[David Sterba]

On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote:
> While adding shared direct IO write locks to fuse Miklos noticed
> that file_remove_privs() needs an exclusive lock. I then
> noticed that btrfs actually has the same issue as I had in my patch,
> it was calling into that function with a shared lock.
> This series adds a new exported function file_needs_remove_privs(),
> which used by the follow up btrfs patch and will be used by the
> DIO code path in fuse as well. If that function returns any mask
> the shared lock needs to be dropped and replaced by the exclusive
> variant.
> 
> Note: Compilation tested only.

The fix makes sense, there should be no noticeable performance impact,
basically the same check is done in the newly exported helper for the
IS_NOSEC bit.  I can give it a test locally for the default case, I'm
not sure if we have specific tests for the security layers in fstests.

Regarding merge, I can take the two patches via btrfs tree or can wait
until the export is present in Linus' tree in case FUSE needs it
independently.

Re: [PATCH v2 0/2] Use exclusive lock for file_remove_privs

[Christian Brauner]

On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote:
> On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote:
> > While adding shared direct IO write locks to fuse Miklos noticed
> > that file_remove_privs() needs an exclusive lock. I then
> > noticed that btrfs actually has the same issue as I had in my patch,
> > it was calling into that function with a shared lock.
> > This series adds a new exported function file_needs_remove_privs(),
> > which used by the follow up btrfs patch and will be used by the
> > DIO code path in fuse as well. If that function returns any mask
> > the shared lock needs to be dropped and replaced by the exclusive
> > variant.
> > 
> > Note: Compilation tested only.
> 
> The fix makes sense, there should be no noticeable performance impact,
> basically the same check is done in the newly exported helper for the
> IS_NOSEC bit.  I can give it a test locally for the default case, I'm
> not sure if we have specific tests for the security layers in fstests.
> 
> Regarding merge, I can take the two patches via btrfs tree or can wait
> until the export is present in Linus' tree in case FUSE needs it
> independently.

Both fuse and btrfs need it afaict. We can grab it and provide a tag
post -rc1? Whatever works best.

Re: [PATCH v2 0/2] Use exclusive lock for file_remove_privs

[Bernd Schubert]

On 9/6/23 16:43, Christian Brauner wrote:
> On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote:
>> On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote:
>>> While adding shared direct IO write locks to fuse Miklos noticed
>>> that file_remove_privs() needs an exclusive lock. I then
>>> noticed that btrfs actually has the same issue as I had in my patch,
>>> it was calling into that function with a shared lock.
>>> This series adds a new exported function file_needs_remove_privs(),
>>> which used by the follow up btrfs patch and will be used by the
>>> DIO code path in fuse as well. If that function returns any mask
>>> the shared lock needs to be dropped and replaced by the exclusive
>>> variant.
>>>
>>> Note: Compilation tested only.
>>
>> The fix makes sense, there should be no noticeable performance impact,
>> basically the same check is done in the newly exported helper for the
>> IS_NOSEC bit.  I can give it a test locally for the default case, I'm
>> not sure if we have specific tests for the security layers in fstests.
>>
>> Regarding merge, I can take the two patches via btrfs tree or can wait
>> until the export is present in Linus' tree in case FUSE needs it
>> independently.
> 
> Both fuse and btrfs need it afaict. We can grab it and provide a tag
> post -rc1? Whatever works best.

fuse will need it for my direct IO patches - hopefully in 6.7.
For btrfs it is a bug fix, should go in asap?

Christoph has some objections for to use the new exported helper
(file_needs_remove_privs). Maybe I better send a version for btrfs
that only uses S_NOSEC? For fuse we cannot use it, unfortunately.


Thanks,
Bernd

Re: [PATCH v2 0/2] Use exclusive lock for file_remove_privs

[Christian Brauner]

On Wed, Sep 06, 2023 at 04:51:20PM +0200, Bernd Schubert wrote:
> 
> 
> On 9/6/23 16:43, Christian Brauner wrote:
> > On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote:
> > > On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote:
> > > > While adding shared direct IO write locks to fuse Miklos noticed
> > > > that file_remove_privs() needs an exclusive lock. I then
> > > > noticed that btrfs actually has the same issue as I had in my patch,
> > > > it was calling into that function with a shared lock.
> > > > This series adds a new exported function file_needs_remove_privs(),
> > > > which used by the follow up btrfs patch and will be used by the
> > > > DIO code path in fuse as well. If that function returns any mask
> > > > the shared lock needs to be dropped and replaced by the exclusive
> > > > variant.
> > > > 
> > > > Note: Compilation tested only.
> > > 
> > > The fix makes sense, there should be no noticeable performance impact,
> > > basically the same check is done in the newly exported helper for the
> > > IS_NOSEC bit.  I can give it a test locally for the default case, I'm
> > > not sure if we have specific tests for the security layers in fstests.
> > > 
> > > Regarding merge, I can take the two patches via btrfs tree or can wait
> > > until the export is present in Linus' tree in case FUSE needs it
> > > independently.
> > 
> > Both fuse and btrfs need it afaict. We can grab it and provide a tag
> > post -rc1? Whatever works best.
> 
> fuse will need it for my direct IO patches - hopefully in 6.7.
> For btrfs it is a bug fix, should go in asap?
> 
> Christoph has some objections for to use the new exported helper
> (file_needs_remove_privs). Maybe I better send a version for btrfs
> that only uses S_NOSEC? For fuse we cannot use it, unfortunately.

Sure.

Re: [PATCH v2 0/2] Use exclusive lock for file_remove_privs

[David Sterba]

On Wed, Sep 06, 2023 at 04:43:22PM +0200, Christian Brauner wrote:
> On Tue, Sep 05, 2023 at 08:02:59PM +0200, David Sterba wrote:
> > On Thu, Aug 31, 2023 at 01:24:29PM +0200, Bernd Schubert wrote:
> > > While adding shared direct IO write locks to fuse Miklos noticed
> > > that file_remove_privs() needs an exclusive lock. I then
> > > noticed that btrfs actually has the same issue as I had in my patch,
> > > it was calling into that function with a shared lock.
> > > This series adds a new exported function file_needs_remove_privs(),
> > > which used by the follow up btrfs patch and will be used by the
> > > DIO code path in fuse as well. If that function returns any mask
> > > the shared lock needs to be dropped and replaced by the exclusive
> > > variant.
> > > 
> > > Note: Compilation tested only.
> > 
> > The fix makes sense, there should be no noticeable performance impact,
> > basically the same check is done in the newly exported helper for the
> > IS_NOSEC bit.  I can give it a test locally for the default case, I'm
> > not sure if we have specific tests for the security layers in fstests.
> > 
> > Regarding merge, I can take the two patches via btrfs tree or can wait
> > until the export is present in Linus' tree in case FUSE needs it
> > independently.
> 
> Both fuse and btrfs need it afaict. We can grab it and provide a tag
> post -rc1? Whatever works best.

Git tree sync won't be needed, Bernd sent the fix within btrfs code.

[PATCH 0/2] Use exclusive lock for file_remove_privs

[Bernd Schubert]

While adding shared direct IO write locks to fuse Miklos noticed
that file_remove_privs() needs an exclusive lock. I then
noticed that btrfs actually has the same issue as I had in my patch,
it was calling into that function with a shared lock.
This series adds a new exported function file_needs_remove_privs(),
which used by the follow up btrfs patch and will be used by the
DIO code path in fuse as well. If that function returns any mask
the shared lock needs to be dropped and replaced by the exclusive
variant.

Note: Compilation tested only.

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


Bernd Schubert (2):
  fs: Add and export file_needs_remove_privs
  btrfs: file_remove_privs needs an exclusive lock

 fs/btrfs/file.c    | 37 +++++++++++++++++++++++++++++--------
 fs/inode.c         |  8 ++++++++
 include/linux/fs.h |  1 +
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.39.2

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

[Bernd Schubert]

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);
+}
+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

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

[Christoph Hellwig]

On Wed, Aug 30, 2023 at 08:15:18PM +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.

As said last time - the existing file systems with shared locking for
direct I/O just do the much more pessimistic IS_SEC check here.  I'd
suggest to just do that for btrfs first step.  If we then have numbers
justifying the finer grained check we should update veryone and not
just do it for one place.