| Message ID | 20250228-work-pidfs-kill_on_last_close-v1-3-5bd7e6bb428e@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pidfs: provide information after task has been reaped | expand |
On 02/28, Christian Brauner wrote: > > @@ -696,6 +696,10 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) > return ERR_PTR(ret); > > pidfd_file = dentry_open(&path, flags, current_cred()); > + /* Raise PIDFD_THREAD explicitly as dentry_open() strips it. */ ^^^^^^^^^^^^^^^^^^^^^^^ Hmm, does it? dentry_open(flags) just passes "flags" to alloc_empty_file()->init_file(), and init_file(flags) does f->f_flags = flags; so it seems that > @@ -2042,11 +2042,6 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re > if (IS_ERR(pidfd_file)) > return PTR_ERR(pidfd_file); > > - /* > - * anon_inode_getfile() ignores everything outside of the > - * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually. > - */ > - pidfd_file->f_flags |= (flags & PIDFD_THREAD); we can just kill this outdated code? Oleg.
On Sun, Mar 02, 2025 at 02:09:36PM +0100, Oleg Nesterov wrote: > On 02/28, Christian Brauner wrote: > > > > @@ -696,6 +696,10 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) > > return ERR_PTR(ret); > > > > pidfd_file = dentry_open(&path, flags, current_cred()); > > + /* Raise PIDFD_THREAD explicitly as dentry_open() strips it. */ > ^^^^^^^^^^^^^^^^^^^^^^^ > Hmm, does it? > > dentry_open(flags) just passes "flags" to alloc_empty_file()->init_file(), > and init_file(flags) does > > f->f_flags = flags; > > so it seems that dentry_open() -> do_dentry_open() { f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC); f->f_iocb_flags = iocb_flags(f); } > > > @@ -2042,11 +2042,6 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re > > if (IS_ERR(pidfd_file)) > > return PTR_ERR(pidfd_file); > > > > - /* > > - * anon_inode_getfile() ignores everything outside of the > > - * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually. > > - */ > > - pidfd_file->f_flags |= (flags & PIDFD_THREAD); > > we can just kill this outdated code? Unfortunately not.
On 03/02, Christian Brauner wrote: > > On Sun, Mar 02, 2025 at 02:09:36PM +0100, Oleg Nesterov wrote: > > On 02/28, Christian Brauner wrote: > > > > > > @@ -696,6 +696,10 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) > > > return ERR_PTR(ret); > > > > > > pidfd_file = dentry_open(&path, flags, current_cred()); > > > + /* Raise PIDFD_THREAD explicitly as dentry_open() strips it. */ > > ^^^^^^^^^^^^^^^^^^^^^^^ > > Hmm, does it? > > > > dentry_open(flags) just passes "flags" to alloc_empty_file()->init_file(), > > and init_file(flags) does > > > > f->f_flags = flags; > > > > dentry_open() > -> do_dentry_open() > { > f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC); Ah, indeed, thanks ;) so perhaps you can update the comment, s/dentry_open/do_dentry_open/ to make it more clear? Oleg.
On Sun, Mar 02, 2025 at 05:05:23PM +0100, Oleg Nesterov wrote: > On 03/02, Christian Brauner wrote: > > > > On Sun, Mar 02, 2025 at 02:09:36PM +0100, Oleg Nesterov wrote: > > > On 02/28, Christian Brauner wrote: > > > > > > > > @@ -696,6 +696,10 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) > > > > return ERR_PTR(ret); > > > > > > > > pidfd_file = dentry_open(&path, flags, current_cred()); > > > > + /* Raise PIDFD_THREAD explicitly as dentry_open() strips it. */ > > > ^^^^^^^^^^^^^^^^^^^^^^^ > > > Hmm, does it? > > > > > > dentry_open(flags) just passes "flags" to alloc_empty_file()->init_file(), > > > and init_file(flags) does > > > > > > f->f_flags = flags; > > > > > > > dentry_open() > > -> do_dentry_open() > > { > > f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC); > > Ah, indeed, thanks ;) so perhaps you can update the comment, > s/dentry_open/do_dentry_open/ to make it more clear? Will do!
diff --git a/fs/pidfs.c b/fs/pidfs.c index aa8c8bda8c8f..61be98f7ad0b 100644 --- a/fs/pidfs.c +++ b/fs/pidfs.c @@ -696,6 +696,10 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags) return ERR_PTR(ret); pidfd_file = dentry_open(&path, flags, current_cred()); + /* Raise PIDFD_THREAD explicitly as dentry_open() strips it. */ + if (!IS_ERR(pidfd_file)) + pidfd_file->f_flags |= (flags & PIDFD_THREAD); + path_put(&path); return pidfd_file; } diff --git a/kernel/fork.c b/kernel/fork.c index 6230f5256bc5..8eac9cd3385b 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2042,11 +2042,6 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re if (IS_ERR(pidfd_file)) return PTR_ERR(pidfd_file); - /* - * anon_inode_getfile() ignores everything outside of the - * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually. - */ - pidfd_file->f_flags |= (flags & PIDFD_THREAD); *ret = pidfd_file; return take_fd(pidfd); }
Instead od adding it into __pidfd_prepare() place it where the actual file allocation happens and update the outdated comment. Signed-off-by: Christian Brauner <brauner@kernel.org> --- fs/pidfs.c | 4 ++++ kernel/fork.c | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-)