RE: [PATCH v2] 9p: retrieve fid from file when file instance exist.

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> Sent: Wednesday, July 1, 2020 6:59 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> Cc: ericvh@xxxxxxxxx; lucho@xxxxxxxxxx; v9fs-
> developer@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Steve
> Capper <Steve.Capper@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; Justin He
> <Justin.He@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH v2] 9p: retrieve fid from file when file instance exist.
>
> Jianyong Wu wrote on Wed, Jul 01, 2020:
> > In the current setattr implementation in 9p, fid is always retrieved
> > from dentry no matter file instance exists or not. There may be some
> > info related to opened file instance dropped. so it's better to
> > retrieve fid from file instance if file instance is passed to setattr.
> >
> > for example:
> > fd=open("tmp", O_RDWR);
> > ftruncate(fd, 10);
> >
> > The file context related with fd will be lost as fid is always
> > retrieved from dentry, then the backend can't get the info of file
> > context. It is against the original intention of user and may lead to
> > bug.
>
> Thanks for the commit message - still feels a bit odd but at least correct
> enough for me :)
>
Thanks.

> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > ---
> >  fs/9p/vfs_inode.c      | 6 +++++-
> >  fs/9p/vfs_inode_dotl.c | 6 +++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index
> > c9255d399917..b33574d347fa 100644
> > --- a/fs/9p/vfs_inode.c
> > +++ b/fs/9p/vfs_inode.c
> > @@ -1100,7 +1100,11 @@ static int v9fs_vfs_setattr(struct dentry
> > *dentry, struct iattr *iattr)
> >
> >  retval = -EPERM;
> >  v9ses = v9fs_dentry2v9ses(dentry);
> > -fid = v9fs_fid_lookup(dentry);
> > +if (iattr->ia_valid & ATTR_FILE) {
> > +fid = iattr->ia_file->private_data;
> > +WARN_ON(!fid);
>
> That would crash in 9p_client_wstat a few lines below with the current else ;
> so I'm not sure WARN_ON is appropriate with this code.
>
> the snippet I had suggested had v9fs_fid_lookup in a different if, not as a else
> statement to avoid this crash (and then warning is OK)
>
Yeah, should check fid before "v9fs_fid_lookup", how about

if (iattr->ia_valid & ATTR_FILE) {
                fid = iattr->ia_file->private_data;
                WARN_ON(!fid);
 }
If (!fid)
fid = v9fs_fid_lookup(dentry);
...

Thanks
Jianyong

> > +} else
> > +fid = v9fs_fid_lookup(dentry);
>
> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.