On Tue, Sep 03, 2024 at 03:35:48PM GMT, Jan Kara wrote:
> On Tue 03-09-24 13:34:30, Christian Brauner wrote:
> > On Fri, Aug 30, 2024 at 03:04:55PM GMT, Christian Brauner wrote:
> > > Store the cookie to detect concurrent seeks on directories in
> > > file->private_data.
> > >
> > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > ---
> > > fs/proc/base.c | 18 ++++++++++++------
> > > 1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 72a1acd03675..8a8aab6b9801 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -3870,12 +3870,12 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > if (!dir_emit_dots(file, ctx))
> > > return 0;
> > >
> > > - /* f_version caches the tgid value that the last readdir call couldn't
> > > - * return. lseek aka telldir automagically resets f_version to 0.
> > > + /* We cache the tgid value that the last readdir call couldn't
> > > + * return and lseek resets it to 0.
> > > */
> > > ns = proc_pid_ns(inode->i_sb);
> > > - tid = (int)file->f_version;
> > > - file->f_version = 0;
> > > + tid = (int)(intptr_t)file->private_data;
> > > + file->private_data = NULL;
> > > for (task = first_tid(proc_pid(inode), tid, ctx->pos - 2, ns);
> > > task;
> > > task = next_tid(task), ctx->pos++) {
> > > @@ -3890,7 +3890,7 @@ static int proc_task_readdir(struct file *file, struct dir_context *ctx)
> > > proc_task_instantiate, task, NULL)) {
> > > /* returning this tgid failed, save it as the first
> > > * pid for the next readir call */
> > > - file->f_version = (u64)tid;
> > > + file->private_data = (void *)(intptr_t)tid;
> > > put_task_struct(task);
> > > break;
> > > }
> > > @@ -3915,6 +3915,12 @@ static int proc_task_getattr(struct mnt_idmap *idmap,
> > > return 0;
> > > }
> > >
> > > +static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > > +{
> > > + return generic_llseek_cookie(file, offset, whence,
> > > + (u64 *)(uintptr_t)&file->private_data);
> >
> > Btw, this is fixed in-tree (I did send out an unfixed version):
> >
> > static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> > {
> > u64 cookie = 1;
> > loff_t off;
> >
> > off = generic_llseek_cookie(file, offset, whence, &cookie);
> > if (!cookie)
> > file->private_data = NULL; /* serialized by f_pos_lock */
> > return off;
> > }
>
> Ah, midair collision :). This looks better just why don't you store the
> cookie unconditionally in file->private_data? This way proc_dir_llseek()
> makes assumptions about how generic_llseek_cookie() uses the cookie which
> unnecessarily spreads internal VFS knowledge into filesystems...
I tried to avoid an allocation for procfs (I assume that's what you're
getting at). That's basically all.
