[PATCH] Revert "fsnotify: optionally pass access range in file permission hooks"

[PATCH] Revert "fsnotify: optionally pass access range in file permission hooks"

[Jens Axboe]

This reverts commit d9e5d31084b024734e64307521414ef0ae1d5333.

This commit added an extra fsnotify call from rw_verify_area(), which
can be a very hot path. In my testing, this adds roughly 5-6% extra
overhead per IO, which is quite a lot. As a result, throughput of
said test also drops by 5-6%, as it is CPU bound. Looking at perf,
it's apparent why:

     3.36%             [kernel.vmlinux]  [k] fsnotify
     2.32%             [kernel.vmlinux]  [k] __fsnotify_paren

which directly correlates with performance lost.

As the rationale for this patch isn't very strong, revert this commit
for now and reclaim the performance.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/open.c                |  4 ----
 fs/read_write.c          | 10 ++--------
 fs/readdir.c             |  4 ----
 fs/remap_range.c         |  8 +-------
 include/linux/fsnotify.h | 13 ++-----------
 security/security.c      |  8 +++++++-
 6 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index a84d21e55c39..f4f157405d1e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -304,10 +304,6 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (ret)
 		return ret;
 
-	ret = fsnotify_file_area_perm(file, MAY_WRITE, &offset, len);
-	if (ret)
-		return ret;
-
 	if (S_ISFIFO(inode->i_mode))
 		return -ESPIPE;
 
diff --git a/fs/read_write.c b/fs/read_write.c
index d4c036e82b6c..e3abf603eaaf 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -354,9 +354,6 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
 
 int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
 {
-	int mask = read_write == READ ? MAY_READ : MAY_WRITE;
-	int ret;
-
 	if (unlikely((ssize_t) count < 0))
 		return -EINVAL;
 
@@ -374,11 +371,8 @@ int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t
 		}
 	}
 
-	ret = security_file_permission(file, mask);
-	if (ret)
-		return ret;
-
-	return fsnotify_file_area_perm(file, mask, ppos, count);
+	return security_file_permission(file,
+				read_write == READ ? MAY_READ : MAY_WRITE);
 }
 EXPORT_SYMBOL(rw_verify_area);
 
diff --git a/fs/readdir.c b/fs/readdir.c
index 278bc0254732..c8c46e294431 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -96,10 +96,6 @@ int iterate_dir(struct file *file, struct dir_context *ctx)
 	if (res)
 		goto out;
 
-	res = fsnotify_file_perm(file, MAY_READ);
-	if (res)
-		goto out;
-
 	res = down_read_killable(&inode->i_rwsem);
 	if (res)
 		goto out;
diff --git a/fs/remap_range.c b/fs/remap_range.c
index f8c1120b8311..12131f2a6c9e 100644
--- a/fs/remap_range.c
+++ b/fs/remap_range.c
@@ -102,9 +102,7 @@ static int generic_remap_checks(struct file *file_in, loff_t pos_in,
 static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
 			     bool write)
 {
-	int mask = write ? MAY_WRITE : MAY_READ;
 	loff_t tmp;
-	int ret;
 
 	if (unlikely(pos < 0 || len < 0))
 		return -EINVAL;
@@ -112,11 +110,7 @@ static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
 	if (unlikely(check_add_overflow(pos, len, &tmp)))
 		return -EINVAL;
 
-	ret = security_file_permission(file, mask);
-	if (ret)
-		return ret;
-
-	return fsnotify_file_area_perm(file, mask, &pos, len);
+	return security_file_permission(file, write ? MAY_WRITE : MAY_READ);
 }
 
 /*
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 11e6434b8e71..0a9d6a8a747a 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -101,10 +101,9 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
 }
 
 /*
- * fsnotify_file_area_perm - permission hook before access to file range
+ * fsnotify_file_perm - permission hook before file access
  */
-static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
-					  const loff_t *ppos, size_t count)
+static inline int fsnotify_file_perm(struct file *file, int perm_mask)
 {
 	__u32 fsnotify_mask = FS_ACCESS_PERM;
 
@@ -121,14 +120,6 @@ static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
 	return fsnotify_file(file, fsnotify_mask);
 }
 
-/*
- * fsnotify_file_perm - permission hook before file access
- */
-static inline int fsnotify_file_perm(struct file *file, int perm_mask)
-{
-	return fsnotify_file_area_perm(file, perm_mask, NULL, 0);
-}
-
 /*
  * fsnotify_open_perm - permission hook before file open
  */
diff --git a/security/security.c b/security/security.c
index 2a7fc7881cbc..d7f3703c5905 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2580,7 +2580,13 @@ int security_kernfs_init_security(struct kernfs_node *kn_dir,
  */
 int security_file_permission(struct file *file, int mask)
 {
-	return call_int_hook(file_permission, 0, file, mask);
+	int ret;
+
+	ret = call_int_hook(file_permission, 0, file, mask);
+	if (ret)
+		return ret;
+
+	return fsnotify_file_perm(file, mask);
 }
 
 /**
-- 
2.43.0

-- 
Jens Axboe

Re: [PATCH] Revert "fsnotify: optionally pass access range in file permission hooks"

[Christian Brauner]

On Tue, Jan 09, 2024 at 09:08:40AM -0700, Jens Axboe wrote:
> This reverts commit d9e5d31084b024734e64307521414ef0ae1d5333.
> 
> This commit added an extra fsnotify call from rw_verify_area(), which
> can be a very hot path. In my testing, this adds roughly 5-6% extra
> overhead per IO, which is quite a lot. As a result, throughput of
> said test also drops by 5-6%, as it is CPU bound. Looking at perf,
> it's apparent why:
> 
>      3.36%             [kernel.vmlinux]  [k] fsnotify
>      2.32%             [kernel.vmlinux]  [k] __fsnotify_paren
> 
> which directly correlates with performance lost.
> 
> As the rationale for this patch isn't very strong, revert this commit
> for now and reclaim the performance.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---

Thanks for tracking this down! I think Amir, you, and I came to the
conclusion that we can fix this in without having to revert. Amir is
sending out a new patch later tonight. I'll get that fixed by the end of
the week.

Christian

Re: [PATCH] Revert "fsnotify: optionally pass access range in file permission hooks"

[Jens Axboe]

On 1/9/24 10:30 AM, Christian Brauner wrote:
> On Tue, Jan 09, 2024 at 09:08:40AM -0700, Jens Axboe wrote:
>> This reverts commit d9e5d31084b024734e64307521414ef0ae1d5333.
>>
>> This commit added an extra fsnotify call from rw_verify_area(), which
>> can be a very hot path. In my testing, this adds roughly 5-6% extra
>> overhead per IO, which is quite a lot. As a result, throughput of
>> said test also drops by 5-6%, as it is CPU bound. Looking at perf,
>> it's apparent why:
>>
>>      3.36%             [kernel.vmlinux]  [k] fsnotify
>>      2.32%             [kernel.vmlinux]  [k] __fsnotify_paren
>>
>> which directly correlates with performance lost.
>>
>> As the rationale for this patch isn't very strong, revert this commit
>> for now and reclaim the performance.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
> 
> Thanks for tracking this down! I think Amir, you, and I came to the
> conclusion that we can fix this in without having to revert. Amir is
> sending out a new patch later tonight. I'll get that fixed by the end of
> the week.

Yep, either revert or that patch fixes things for me.

-- 
Jens Axboe