On Wed, Sep 04, 2024 at 04:16:07PM GMT, Jan Kara wrote:
> On Tue 03-09-24 16:00:56, Christian Brauner wrote:
> > 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.
>
> Yes, I just meant I'd find it safer to have:
>
> static loff_t proc_dir_llseek(struct file *file, loff_t offset, int whence)
> {
> u64 cookie = (u64)file->private_data;
> loff_t off;
>
> off = generic_llseek_cookie(file, offset, whence, &cookie);
> file->private_data = (void *)cookie; /* serialized by f_pos_lock */
> return off;
> }
>
> So that we don't presume what generic_llseek_cookie() can do with the
> cookie.
I switched to that, thanks!
