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.
Signed-off-by: Jianyong Wu <[email protected]>
---
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);
+ } else
+ fid = v9fs_fid_lookup(dentry);
if(IS_ERR(fid))
return PTR_ERR(fid);
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 60328b21c5fb..ae714f1a630f 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -560,7 +560,11 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr)
p9attr.mtime_sec = iattr->ia_mtime.tv_sec;
p9attr.mtime_nsec = iattr->ia_mtime.tv_nsec;
- fid = v9fs_fid_lookup(dentry);
+ if (iattr->ia_valid & ATTR_FILE) {
+ fid = iattr->ia_file->private_data;
+ WARN_ON(!fid);
+ } else
+ fid = v9fs_fid_lookup(dentry);
if (IS_ERR(fid))
return PTR_ERR(fid);
--
2.17.1
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 :)
> Signed-off-by: Jianyong Wu <[email protected]>
> ---
> 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)
> + } else
> + fid = v9fs_fid_lookup(dentry);
--
Dominique
Hi Dominique,
> -----Original Message-----
> From: Dominique Martinet <[email protected]>
> Sent: Wednesday, July 1, 2020 6:59 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; v9fs-
> [email protected]; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin <[email protected]>; Justin He
> <[email protected]>; Wei Chen <[email protected]>
> 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 <[email protected]>
> > ---
> > 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.
Jianyong Wu wrote on Thu, Jul 02, 2020:
> 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);
Yes, that would be fine.
--
Dominique
Hi Dominique,
> -----Original Message-----
> From: Dominique Martinet <[email protected]>
> Sent: Thursday, July 2, 2020 3:56 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; v9fs-
> [email protected]; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin <[email protected]>; Justin He
> <[email protected]>; Wei Chen <[email protected]>
> Subject: Re: [PATCH v2] 9p: retrieve fid from file when file instance exist.
>
> Jianyong Wu wrote on Thu, Jul 02, 2020:
> > 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);
>
> Yes, that would be fine.
>
Ok, I will apply this change both in vfs_inode.c and vfs_inode_dotl.c next version.
Thanks
Jianyong
> --
> 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.