[RFC PATCH 0/2] vfs:9p: fix open-unlink-fstat bug

[RFC PATCH 0/2] vfs:9p: fix open-unlink-fstat bug

[Jianyong Wu]

how to reproduce:
in 9p guest:

struct stat *statbuf;
int fd;

fd = open("tmp", O_RDWR);
unlink("tmp");
fstat(fd, statbuf);

fstat will fail as "tmp" in 9p server side has been removed. 9p server
can't retrieve the file context as the guest has not passed it down.
so we should pass the file info down in 9p guest in getattr op.
it need add a new file member in "struct kstat" as "struct istat" does.

Jianyong Wu (2):
  vfs: pass file down when getattr to avoid losing info.
  9p: retrieve fid from file if it exists when getattr.

 fs/9p/vfs_inode.c      | 9 +++++++--
 fs/9p/vfs_inode_dotl.c | 9 +++++++--
 fs/stat.c              | 1 +
 include/linux/stat.h   | 6 ++++++
 4 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.17.1

[RFC PATCH 1/2] vfs: pass file down when getattr to avoid losing info.

[Jianyong Wu]

Currently, getting attribute for a file represented by fd is always
by inode or path which may lead to bug for a certain network file system.
Adding file struct into struct kstat and assigning file for it in
vfs_statx_fd can avoid this issue. This change refers to struct istat.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 fs/stat.c            | 1 +
 include/linux/stat.h | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/fs/stat.c b/fs/stat.c
index 44f8ad346db4..0dee5487f6d6 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -147,6 +147,7 @@ int vfs_statx_fd(unsigned int fd, struct kstat *stat,
 		return -EINVAL;
 
 	f = fdget_raw(fd);
+	stat->filp = f.file;
 	if (f.file) {
 		error = vfs_getattr(&f.file->f_path, stat,
 				    request_mask, query_flags);
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 56614af83d4a..4755c528d49a 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -48,6 +48,12 @@ struct kstat {
 	struct timespec64 btime;			/* File creation time */
 	u64		blocks;
 	u64		mnt_id;
+
+	/*
+	 * Not an attribute, but an auxiliary info for filesystems wanting to
+	 * implement an fstat() like method.
+	 */
+	struct file	*filp;
 };
 
 #endif
-- 
2.17.1

Re: [RFC PATCH 1/2] vfs: pass file down when getattr to avoid losing info.

[Dominique Martinet]

Jianyong Wu wrote on Mon, Jul 20, 2020:
> Currently, getting attribute for a file represented by fd is always
> by inode or path which may lead to bug for a certain network file system.
> Adding file struct into struct kstat and assigning file for it in
> vfs_statx_fd can avoid this issue. This change refers to struct istat.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  fs/stat.c            | 1 +
>  include/linux/stat.h | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index 44f8ad346db4..0dee5487f6d6 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -147,6 +147,7 @@ int vfs_statx_fd(unsigned int fd, struct kstat *stat,
>  		return -EINVAL;
>  
>  	f = fdget_raw(fd);
> +	stat->filp = f.file;
>  	if (f.file) {
>  		error = vfs_getattr(&f.file->f_path, stat,
>  				    request_mask, query_flags);
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index 56614af83d4a..4755c528d49a 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -48,6 +48,12 @@ struct kstat {
>  	struct timespec64 btime;			/* File creation time */
>  	u64		blocks;
>  	u64		mnt_id;
> +
> +	/*
> +	 * Not an attribute, but an auxiliary info for filesystems wanting to
> +	 * implement an fstat() like method.
> +	 */
> +	struct file	*filp;

Just, no ; don't touch this, it isn't likely to get accepted ever.
file operations should all have a filp around already, we need to fix
this in our code.

(also missing quite a few Cc if you ever want to touch these bits, at
least linux-fsdevel@)



FWIW the problem has been discussed a few times previously.

I'd appreciate if you could take over from Eric and Greg's old series
that intended to fix that:
https://lore.kernel.org/lkml/146659832556.15781.17414806975641516683.stgit@bahia.lan/

it introduced a race somewhere, but the idea looked good at the time so
it's worth looking into.

-- 
Dominique

RE: [RFC PATCH 1/2] vfs: pass file down when getattr to avoid losing info.

[Jianyong Wu]

> -----Original Message-----
> From: Dominique Martinet <asmadeus@codewreck.org>
> Sent: Monday, July 20, 2020 10:54 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: ericvh@gmail.com; hch@lst.de; dhowells@redhat.com;
> lucho@ionkov.net; v9fs-developer@lists.sourceforge.net; linux-
> kernel@vger.kernel.org; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>
> Subject: Re: [RFC PATCH 1/2] vfs: pass file down when getattr to avoid losing
> info.
>
> Jianyong Wu wrote on Mon, Jul 20, 2020:
> > Currently, getting attribute for a file represented by fd is always by
> > inode or path which may lead to bug for a certain network file system.
> > Adding file struct into struct kstat and assigning file for it in
> > vfs_statx_fd can avoid this issue. This change refers to struct istat.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  fs/stat.c            | 1 +
> >  include/linux/stat.h | 6 ++++++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 44f8ad346db4..0dee5487f6d6 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -147,6 +147,7 @@ int vfs_statx_fd(unsigned int fd, struct kstat *stat,
> >  return -EINVAL;
> >
> >  f = fdget_raw(fd);
> > +stat->filp = f.file;
> >  if (f.file) {
> >  error = vfs_getattr(&f.file->f_path, stat,
> >      request_mask, query_flags);
> > diff --git a/include/linux/stat.h b/include/linux/stat.h index
> > 56614af83d4a..4755c528d49a 100644
> > --- a/include/linux/stat.h
> > +++ b/include/linux/stat.h
> > @@ -48,6 +48,12 @@ struct kstat {
> >  struct timespec64 btime;/* File creation time
> */
> >  u64blocks;
> >  u64mnt_id;
> > +
> > +/*
> > + * Not an attribute, but an auxiliary info for filesystems wanting to
> > + * implement an fstat() like method.
> > + */
> > +struct file*filp;
>
> Just, no ; don't touch this, it isn't likely to get accepted ever.
> file operations should all have a filp around already, we need to fix this in our
> code.
>
Ok, but I think it may not be an easy job to fix it inside 9p.

> (also missing quite a few Cc if you ever want to touch these bits, at least
> linux-fsdevel@)
> thanks. Should have added them.
>
>
> FWIW the problem has been discussed a few times previously.
>
> I'd appreciate if you could take over from Eric and Greg's old series that
> intended to fix that:
> https://lore.kernel.org/lkml/146659832556.15781.17414806975641516683.
> stgit@bahia.lan/
>
> it introduced a race somewhere, but the idea looked good at the time so it's
> worth looking into.

Thanks, I will look into it to find some ideas. if I can get the solution, I will be back.

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.

[RFC PATCH 2/2] 9p: retrieve fid from file if it exists when getattr.

[Jianyong Wu]

fid should be retrieved from file when it is not NULL in getattr.
it denotes that getattr is called by fdstat from userspace when
file is exist, which means we should get info from file context
not dentry to avoid the failure when the dentry has gone.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 fs/9p/vfs_inode.c      | 9 +++++++--
 fs/9p/vfs_inode_dotl.c | 9 +++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index c9255d399917..f562f5710eae 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1054,7 +1054,7 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 {
 	struct dentry *dentry = path->dentry;
 	struct v9fs_session_info *v9ses;
-	struct p9_fid *fid;
+	struct p9_fid *fid = NULL;
 	struct p9_wstat *st;
 
 	p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
@@ -1063,7 +1063,12 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 		generic_fillattr(d_inode(dentry), stat);
 		return 0;
 	}
-	fid = v9fs_fid_lookup(dentry);
+	if (stat->filp) {
+		fid = stat->filp->private_data;
+		WARN_ON(!fid);
+	}
+	if (!fid)
+		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..6b7cbe74b0bb 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -460,7 +460,7 @@ v9fs_vfs_getattr_dotl(const struct path *path, struct kstat *stat,
 {
 	struct dentry *dentry = path->dentry;
 	struct v9fs_session_info *v9ses;
-	struct p9_fid *fid;
+	struct p9_fid *fid = NULL;
 	struct p9_stat_dotl *st;
 
 	p9_debug(P9_DEBUG_VFS, "dentry: %p\n", dentry);
@@ -469,7 +469,12 @@ v9fs_vfs_getattr_dotl(const struct path *path, struct kstat *stat,
 		generic_fillattr(d_inode(dentry), stat);
 		return 0;
 	}
-	fid = v9fs_fid_lookup(dentry);
+	if (stat->filp) {
+		fid = stat->filp->private_data;
+		WARN_ON(!fid);
+	}
+	if (!fid)
+		fid = v9fs_fid_lookup(dentry);
 	if (IS_ERR(fid))
 		return PTR_ERR(fid);
 
-- 
2.17.1

Re: [RFC PATCH 0/2] vfs:9p: fix open-unlink-fstat bug

[Al Viro]

On Mon, Jul 20, 2020 at 09:46:20AM +0800, Jianyong Wu wrote:
> how to reproduce:
> in 9p guest:
> 
> struct stat *statbuf;
> int fd;
> 
> fd = open("tmp", O_RDWR);
> unlink("tmp");
> fstat(fd, statbuf);
> 
> fstat will fail as "tmp" in 9p server side has been removed. 9p server
> can't retrieve the file context as the guest has not passed it down.
> so we should pass the file info down in 9p guest in getattr op.
> it need add a new file member in "struct kstat" as "struct istat" does.

Er... what struct istat?

RE: [RFC PATCH 0/2] vfs:9p: fix open-unlink-fstat bug

[Jianyong Wu]

> -----Original Message-----
> From: Al Viro <viro@ftp.linux.org.uk> On Behalf Of Al Viro
> Sent: Monday, July 20, 2020 9:52 AM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: ericvh@gmail.com; hch@lst.de; dhowells@redhat.com;
> lucho@ionkov.net; asmadeus@codewreck.org; v9fs-
> developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [RFC PATCH 0/2] vfs:9p: fix open-unlink-fstat bug
>
> On Mon, Jul 20, 2020 at 09:46:20AM +0800, Jianyong Wu wrote:
> > how to reproduce:
> > in 9p guest:
> >
> > struct stat *statbuf;
> > int fd;
> >
> > fd = open("tmp", O_RDWR);
> > unlink("tmp");
> > fstat(fd, statbuf);
> >
> > fstat will fail as "tmp" in 9p server side has been removed. 9p server
> > can't retrieve the file context as the guest has not passed it down.
> > so we should pass the file info down in 9p guest in getattr op.
> > it need add a new file member in "struct kstat" as "struct istat" does.
>
> Er... what struct istat?
Oh, sorry, I make mistake here. it's "struct iattr" Not "istat".

Thanks
Jianyong
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.