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

Commit Message

Bernd Schubert Sept. 6, 2023, 3:59 p.m. UTC

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 <[e-mail]>
Cc: Goldwyn Rodrigues <[e-mail]>
Cc: Miklos Szeredi <[e-mail]>
Cc: Dharmendra Singh <[e-mail]>
Cc: David Sterba <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Bernd Schubert <[e-mail]>
---
 fs/btrfs/file.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Sept. 8, 2023, 8:22 a.m. UTC | #1

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 <[e-mail]>

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.

Bernd Schubert Sept. 8, 2023, 9:16 a.m. UTC | #2

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 <[e-mail]>
> 
> 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

David Sterba Sept. 8, 2023, 9:58 p.m. UTC | #3

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 <[e-mail]>
> Cc: Goldwyn Rodrigues <[e-mail]>
> Cc: Miklos Szeredi <[e-mail]>
> Cc: Dharmendra Singh <[e-mail]>
> Cc: David Sterba <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Bernd Schubert <[e-mail]>

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

Patch

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