Re: [PATCH RFC 14/20] proc: store cookie in private data

[Jan Kara <jack@suse.cz>]

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@kernel.org>
> > ---
> >  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...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR