On Wed, Nov 13, 2024 at 5:30 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Nov 13, 2024 at 01:19:54AM +0000, Al Viro wrote:
> > On Tue, Nov 12, 2024 at 04:38:42PM -0800, Linus Torvalds wrote:
> > > Looking at that locking code in fadvise() just for the f_mode use does
> > > make me think this would be a really good cleanup.
> > >
> > > I note that our fcntl code seems buggy as-is, because while it does
> > > use f_lock for assignments (good), it clearly does *not* use them for
> > > reading.
> > >
> > > So it looks like you can actually read inconsistent values.
> > >
> > > I get the feeling that f_flags would want WRITE_ONCE/READ_ONCE in
> > > _addition_ to the f_lock use it has.
> >
> > AFAICS, fasync logics is the fishy part - the rest should be sane.
> >
> > > The f_mode thing with fadvise() smells like the same bug. Just because
> > > the modifications are serialized wrt each other doesn't mean that
> > > readers are then automatically ok.
> >
> > Reads are also under ->f_lock in there, AFAICS...
> >
> > Another thing in the vicinity is ->f_mode modifications after the calls
> > of anon_inode_getfile() in several callers - probably ought to switch
> > those to anon_inode_getfile_fmode(). That had been discussed back in
> > April when the function got merged, but "convert to using it" followup
> > series hadn't materialized...
>
> While we are at it, there's is a couple of kludges I really hate -
> mixing __FMODE_NONOTIFY and __FMODE_EXEC with O_... flags.
>
> E.g. for __FMODE_NONOTIFY all it takes is switching fanotify from
> anon_inode_getfd() to anon_inode_getfile_fmode() and adding
> a dentry_open_nonotify() to be used by fanotify on the other path.
> That's it - no more weird shit in OPEN_FMODE(), etc.
>
> For __FMODE_EXEC it might get trickier (nfs is the main consumer),
> but I seriously suspect that something like "have path_openat()
> check op->acc_mode & MAY_EXEC and set FMODE_EXEC in ->f_mode
> right after struct file allocation" would make a good starting
> point; yes, it would affect uselib(2), but... I've no idea whether
> it wouldn't be the right thing to do; would be hard to test.
>
> Anyway, untested __FMODE_NONOTIFY side of it:
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..ebd1c82bfb6b 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -1161,10 +1161,10 @@ static int __init fcntl_init(void)
> * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
> * is defined as O_NONBLOCK on some platforms and not on others.
> */
> - BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
> + BUILD_BUG_ON(20 - 1 /* for O_RDONLY being 0 */ !=
> HWEIGHT32(
> (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
> - __FMODE_EXEC | __FMODE_NONOTIFY));
> + __FMODE_EXEC));
>
> fasync_cache = kmem_cache_create("fasync_cache",
> sizeof(struct fasync_struct), 0,
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 9644bc72e457..43fbf29ef03a 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -101,8 +101,7 @@ static void __init fanotify_sysctls_init(void)
> *
> * Internal and external open flags are stored together in field f_flags of
> * struct file. Only external open flags shall be allowed in event_f_flags.
> - * Internal flags like FMODE_NONOTIFY, FMODE_EXEC, FMODE_NOCMTIME shall be
> - * excluded.
> + * Internal flags like FMODE_EXEC shall be excluded.
> */
> #define FANOTIFY_INIT_ALL_EVENT_F_BITS ( \
> O_ACCMODE | O_APPEND | O_NONBLOCK | \
> @@ -262,8 +261,8 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> * we need a new file handle for the userspace program so it can read even if it was
> * originally opened O_WRONLY.
> */
> - new_file = dentry_open(path,
> - group->fanotify_data.f_flags | __FMODE_NONOTIFY,
> + new_file = dentry_open_nonotify(path,
> + group->fanotify_data.f_flags,
I would make this a bit more generic helper and the comment above
is truly clueless:
/*
- * we need a new file handle for the userspace program so it
can read even if it was
- * originally opened O_WRONLY.
+ * We provide an fd for the userspace program, so it could access the
+ * file without generating fanotify events itself.
*/
- new_file = dentry_open(path,
- group->fanotify_data.f_flags | __FMODE_NONOTIFY,
- current_cred());
+ new_file = dentry_open_fmode(path, group->fanotify_data.f_flags,
+ FMODE_NONOTIFY, current_cred());
> current_cred());
> if (IS_ERR(new_file)) {
> /*
> @@ -1404,6 +1403,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
> unsigned int fid_mode = flags & FANOTIFY_FID_BITS;
> unsigned int class = flags & FANOTIFY_CLASS_BITS;
> unsigned int internal_flags = 0;
> + struct file *file;
>
> pr_debug("%s: flags=%x event_f_flags=%x\n",
> __func__, flags, event_f_flags);
> @@ -1472,7 +1472,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
> (!(fid_mode & FAN_REPORT_NAME) || !(fid_mode & FAN_REPORT_FID)))
> return -EINVAL;
>
> - f_flags = O_RDWR | __FMODE_NONOTIFY;
> + f_flags = O_RDWR;
> if (flags & FAN_CLOEXEC)
> f_flags |= O_CLOEXEC;
> if (flags & FAN_NONBLOCK)
> @@ -1550,10 +1550,18 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
> goto out_destroy_group;
> }
>
> - fd = anon_inode_getfd("[fanotify]", &fanotify_fops, group, f_flags);
> + fd = get_unused_fd_flags(flags);
s/flags/f_flags
> if (fd < 0)
> goto out_destroy_group;
>
> + file = anon_inode_getfile_fmode("[fanotify]", &fanotify_fops, group,
> + f_flags, FMODE_NONOTIFY);
> + if (IS_ERR(file)) {
> + fd = PTR_ERR(file);
> + put_unused_fd(fd);
> + goto out_destroy_group;
> + }
> + fd_install(fd, file);
> return fd;
>
> out_destroy_group:
> diff --git a/fs/open.c b/fs/open.c
> index acaeb3e25c88..04cb581528ff 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1118,6 +1118,23 @@ struct file *dentry_open(const struct path *path, int flags,
> }
> EXPORT_SYMBOL(dentry_open);
>
> +struct file *dentry_open_nonotify(const struct path *path, int flags,
> + const struct cred *cred)
> +{
> + struct file *f = alloc_empty_file(flags, cred);
> + if (!IS_ERR(f)) {
> + int error;
> +
> + f->f_mode |= FMODE_NONOTIFY;
> + error = vfs_open(path, f);
> + if (error) {
> + fput(f);
> + f = ERR_PTR(error);
> + }
> + }
> + return f;
> +}
> +
> /**
> * dentry_create - Create and open a file
> * @path: path to create
> @@ -1215,7 +1232,7 @@ inline struct open_how build_open_how(int flags, umode_t mode)
> inline int build_open_flags(const struct open_how *how, struct open_flags *op)
> {
> u64 flags = how->flags;
> - u64 strip = __FMODE_NONOTIFY | O_CLOEXEC;
> + u64 strip = O_CLOEXEC;
> int lookup_flags = 0;
> int acc_mode = ACC_MODE(flags);
>
Get rid of another stale comment:
/*
- * Strip flags that either shouldn't be set by userspace like
- * FMODE_NONOTIFY or that aren't relevant in determining struct
- * open_flags like O_CLOEXEC.
+ * Strip flags that aren't relevant in determining struct open_flags.
*/
With these changed, you can add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
With the f_flags typo fixed, this passed LTP sanity tests, but I am
going to test the NONOTIFY functionally some more.
Thanks,
Amir.
