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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux