[PATCH v3 0/1] btrfs: Use exclusive lock for file_remove_privs

[PATCH v3 0/1] btrfs: 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.

v3: Removed file_needs_remove_privs, btrfs can check for S_NOSEC.
Christoph had suggested to benchmark if using file_remove_privs
has any performance improvement before using it, but I'm not sure
what exactly to run and actually I think IS_NOSEC should be fine
for local block device file systems. The actual patch got also
easier to read with that.

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: Christoph Hellwig <hch@infradead.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: David Sterba <dsterba@suse.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org


Bernd Schubert (1):
  btrfs: file_remove_privs needs an exclusive lock

 fs/btrfs/file.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/1] 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.

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: David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/btrfs/file.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fd03e689a6be..c4b304a2948e 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1466,8 +1466,12 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	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;
 
 relock:
@@ -1475,6 +1479,12 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	if (err < 0)
 		return err;
 
+	if (ilock_flags & BTRFS_ILOCK_SHARED && !IS_NOSEC(inode)) {
+		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
+		ilock_flags &= ~BTRFS_ILOCK_SHARED;
+		goto relock;
+	}
+
 	err = generic_write_checks(iocb, from);
 	if (err <= 0) {
 		btrfs_inode_unlock(BTRFS_I(inode), ilock_flags);
-- 
2.39.2

Re: [PATCH 1/1] btrfs: file_remove_privs needs an exclusive lock

[Christoph Hellwig]

On Wed, Sep 06, 2023 at 05:59:03PM +0200, Bernd Schubert wrote:
> file_remove_privs might call into notify_change(), which
> requires to hold an exclusive lock.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

FYI, I'd be really curious about benchmarking this against you version
that checks xattrs for shared locked writes on files that have xattrs
but not security ones or setuid bits.  On the one hand being able to
do the shared lock sounds nice, on the other hand even just looking up
the xattrs will probably make it slower at least for smaller I/O.

Re: [PATCH 1/1] btrfs: file_remove_privs needs an exclusive lock

[Bernd Schubert]

On 9/8/23 10:22, Christoph Hellwig wrote:
> On Wed, Sep 06, 2023 at 05:59:03PM +0200, Bernd Schubert wrote:
>> file_remove_privs might call into notify_change(), which
>> requires to hold an exclusive lock.
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> FYI, I'd be really curious about benchmarking this against you version
> that checks xattrs for shared locked writes on files that have xattrs
> but not security ones or setuid bits.  On the one hand being able to
> do the shared lock sounds nice, on the other hand even just looking up
> the xattrs will probably make it slower at least for smaller I/O.


I had checked the history of S_NOSEC and I guess that already tells that
the xattr lookup is too slow (commit 69b4573296469fd3f70cf7044693074980517067)
I don't promise that I benchmark it today, but I can
try to find some time in the next week or the week after. Although I
guess there won't be any difference with my initial patch, as
dentry_needs_remove_privs() also checks for IS_NOSEC(inode) - overhead
was just the additional non inlined function call to
file_needs_remove_privs(). And if the flag was not set, overhead was
looking up xattr two times.


Bernd

Re: [PATCH 1/1] btrfs: file_remove_privs needs an exclusive lock

[David Sterba]

On Wed, Sep 06, 2023 at 05:59:03PM +0200, Bernd Schubert wrote:
> file_remove_privs might call into notify_change(), which
> requires to hold an exclusive lock.
> 
> 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: David Sterba <dsterba@suse.com>
> Cc: linux-btrfs@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>

Added to btrfs tree, with slightly reformatted comments, some minor
coding style changes and updated changelog. Thanks.