| Message ID | 53682ece-f0e7-48de-9a1c-879ee34b0449@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "fsnotify: optionally pass access range in file permission hooks" | expand |
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
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.
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); } /**
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(-)