linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Christian Brauner <brauner@kernel.org>,
	Amir Goldstein <amir73il@gmail.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: [PATCH] Revert "fsnotify: optionally pass access range in file permission hooks"
Date: Tue, 9 Jan 2024 09:08:40 -0700	[thread overview]
Message-ID: <53682ece-f0e7-48de-9a1c-879ee34b0449@kernel.dk> (raw)

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


             reply	other threads:[~2024-01-09 16:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-09 16:08 Jens Axboe [this message]
2024-01-09 17:30 ` [PATCH] Revert "fsnotify: optionally pass access range in file permission hooks" Christian Brauner
2024-01-09 17:33   ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53682ece-f0e7-48de-9a1c-879ee34b0449@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=amir73il@gmail.com \
    --cc=brauner@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).