Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] fs/9p: Fix TCREATE's fid in protocol
@ 2020-07-13 21:57 Victor Hsieh
  2020-07-14 12:12 ` Dominique Martinet
  0 siblings, 1 reply; 6+ messages in thread
From: Victor Hsieh @ 2020-07-13 21:57 UTC (permalink / raw)
  To: v9fs-developer, Eric Van Hensbergen, Latchesar Ionkov,
	Dominique Martinet
  Cc: linux-fsdevel, linux-kernel, Victor Hsieh, stable

The fid parameter of TCREATE represents the directory that the file
should be created at. The current implementation mistakenly passes a
locally created fid for the file. The correct file fid is usually
retrieved by another WALK call, which does happen right after.

The problem happens when a new created fd is read from (i.e. where
private_data->fid is used), but not write to.

Fixes: 5643135a2846 ("fs/9p: This patch implements TLCREATE for 9p2000.L protocol.")
Signed-off-by: Victor Hsieh <victorhsieh@google.com>
Cc: stable@vger.kernel.org
---
 fs/9p/vfs_inode_dotl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 60328b21c5fb..90a7aaea918d 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -285,7 +285,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
 			 err);
 		goto error;
 	}
-	err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags),
+	err = p9_client_create_dotl(dfid, name, v9fs_open_to_dotl_flags(flags),
 				    mode, gid, &qid);
 	if (err < 0) {
 		p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n",
-- 
2.27.0.383.g050319c2ae-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol
  2020-07-13 21:57 [PATCH] fs/9p: Fix TCREATE's fid in protocol Victor Hsieh
@ 2020-07-14 12:12 ` Dominique Martinet
  2020-07-14 20:54   ` Eric Biggers
  0 siblings, 1 reply; 6+ messages in thread
From: Dominique Martinet @ 2020-07-14 12:12 UTC (permalink / raw)
  To: Victor Hsieh
  Cc: v9fs-developer, Eric Van Hensbergen, Latchesar Ionkov,
	linux-fsdevel, linux-kernel, stable

Victor Hsieh wrote on Mon, Jul 13, 2020:
> The fid parameter of TCREATE represents the directory that the file

This is not TCREATE, this is TLCREATE.
The fid represents the directory before the call, but on success
represents the file that has been created.

> should be created at. The current implementation mistakenly passes a
> locally created fid for the file. The correct file fid is usually
> retrieved by another WALK call, which does happen right after.
> 
> The problem happens when a new created fd is read from (i.e. where
> private_data->fid is used), but not write to.

I'm not sure why the code currently does a 2nd walk from the directory
with the name which is prone to a race instead of cloning ofid without a
path, but I fail to see the problem you ran into - file->private_data is
a fid pointing to the file as it should be.

Could you describe what kind of errors you get and if possible how to
reproduce?

> Fixes: 5643135a2846 ("fs/9p: This patch implements TLCREATE for 9p2000.L protocol.")
> Signed-off-by: Victor Hsieh <victorhsieh@google.com>
> Cc: stable@vger.kernel.org

(afaiu it is normally frowned upon for developers to add this cc (I can
understand stable@ not wanting spam discussing issues left and right
before maintainers agreed on them!) ; I can add it to the commit itself
if requested but they normally pick most such fixes pretty nicely for
backport anyway; I see most 9p patches backported as long as the patch
applies cleanly which is pretty much all the time.
Please let me know if I understood that incorrectly)

> ---
>  fs/9p/vfs_inode_dotl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 60328b21c5fb..90a7aaea918d 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -285,7 +285,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
>  			 err);
>  		goto error;
>  	}
> -	err = p9_client_create_dotl(ofid, name, v9fs_open_to_dotl_flags(flags),
> +	err = p9_client_create_dotl(dfid, name, v9fs_open_to_dotl_flags(flags),
>  				    mode, gid, &qid);
>  	if (err < 0) {
>  		p9_debug(P9_DEBUG_VFS, "p9_client_open_dotl failed in creat %d\n",

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol
  2020-07-14 12:12 ` Dominique Martinet
@ 2020-07-14 20:54   ` Eric Biggers
  2020-07-14 21:10     ` Victor Hsieh
  2020-07-15  8:03     ` Greg KH
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Biggers @ 2020-07-14 20:54 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Victor Hsieh, v9fs-developer, Eric Van Hensbergen,
	Latchesar Ionkov, linux-fsdevel, linux-kernel, stable

On Tue, Jul 14, 2020 at 02:12:49PM +0200, Dominique Martinet wrote:
> 
> > Fixes: 5643135a2846 ("fs/9p: This patch implements TLCREATE for 9p2000.L protocol.")
> > Signed-off-by: Victor Hsieh <victorhsieh@google.com>
> > Cc: stable@vger.kernel.org
> 
> (afaiu it is normally frowned upon for developers to add this cc (I can
> understand stable@ not wanting spam discussing issues left and right
> before maintainers agreed on them!) ; I can add it to the commit itself
> if requested but they normally pick most such fixes pretty nicely for
> backport anyway; I see most 9p patches backported as long as the patch
> applies cleanly which is pretty much all the time.
> Please let me know if I understood that incorrectly)
> 

Some people assume this, but the stable maintainers themselves say that Cc'ing
stable@vger.kernel.org on in-development patches is fine:
https://lkml.kernel.org/r/20200423184219.GA80650@kroah.com

And doing so is pretty much inevitable, since the tag gets picked up by
'git send-email'.  (Yes, there's also "stable@kernel.org", but it's not actually
what is documented.)

- Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol
  2020-07-14 20:54   ` Eric Biggers
@ 2020-07-14 21:10     ` Victor Hsieh
  2020-07-15 13:35       ` Dominique Martinet
  2020-07-15  8:03     ` Greg KH
  1 sibling, 1 reply; 6+ messages in thread
From: Victor Hsieh @ 2020-07-14 21:10 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Dominique Martinet, v9fs-developer, Eric Van Hensbergen,
	Latchesar Ionkov, linux-fsdevel, linux-kernel, stable

Please disregard this patch.  I misunderstood the protocol and have
found the actual problem in the hypervisor's 9P implementation.  Sorry
about the noise.

On Tue, Jul 14, 2020 at 1:54 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jul 14, 2020 at 02:12:49PM +0200, Dominique Martinet wrote:
> >
> > > Fixes: 5643135a2846 ("fs/9p: This patch implements TLCREATE for 9p2000.L protocol.")
> > > Signed-off-by: Victor Hsieh <victorhsieh@google.com>
> > > Cc: stable@vger.kernel.org
> >
> > (afaiu it is normally frowned upon for developers to add this cc (I can
> > understand stable@ not wanting spam discussing issues left and right
> > before maintainers agreed on them!) ; I can add it to the commit itself
> > if requested but they normally pick most such fixes pretty nicely for
> > backport anyway; I see most 9p patches backported as long as the patch
> > applies cleanly which is pretty much all the time.
> > Please let me know if I understood that incorrectly)
> >
>
> Some people assume this, but the stable maintainers themselves say that Cc'ing
> stable@vger.kernel.org on in-development patches is fine:
> https://lkml.kernel.org/r/20200423184219.GA80650@kroah.com
>
> And doing so is pretty much inevitable, since the tag gets picked up by
> 'git send-email'.  (Yes, there's also "stable@kernel.org", but it's not actually
> what is documented.)
>
> - Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol
  2020-07-14 20:54   ` Eric Biggers
  2020-07-14 21:10     ` Victor Hsieh
@ 2020-07-15  8:03     ` Greg KH
  1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2020-07-15  8:03 UTC (permalink / raw)
  To: Dominique Martinet
  Cc: Eric Biggers, Victor Hsieh, v9fs-developer, Eric Van Hensbergen,
	Latchesar Ionkov, linux-fsdevel, linux-kernel, stable

On Tue, Jul 14, 2020 at 01:54:01PM -0700, Eric Biggers wrote:
> On Tue, Jul 14, 2020 at 02:12:49PM +0200, Dominique Martinet wrote:
> > 
> > > Fixes: 5643135a2846 ("fs/9p: This patch implements TLCREATE for 9p2000.L protocol.")
> > > Signed-off-by: Victor Hsieh <victorhsieh@google.com>
> > > Cc: stable@vger.kernel.org
> > 
> > (afaiu it is normally frowned upon for developers to add this cc (I can
> > understand stable@ not wanting spam discussing issues left and right
> > before maintainers agreed on them!) ; I can add it to the commit itself
> > if requested but they normally pick most such fixes pretty nicely for
> > backport anyway; I see most 9p patches backported as long as the patch
> > applies cleanly which is pretty much all the time.
> > Please let me know if I understood that incorrectly)

As Eric says, this is fine to cc: stable with this kind of thing.  It's
good to get a "heads up" on patches that are coming, and Sasha runs some
tests on them as well to make sure that they really are going to apply
to what trees you think they should apply to.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol
  2020-07-14 21:10     ` Victor Hsieh
@ 2020-07-15 13:35       ` Dominique Martinet
  0 siblings, 0 replies; 6+ messages in thread
From: Dominique Martinet @ 2020-07-15 13:35 UTC (permalink / raw)
  To: Victor Hsieh, Greg KH
  Cc: Eric Biggers, v9fs-developer, Eric Van Hensbergen,
	Latchesar Ionkov, linux-fsdevel, linux-kernel, stable

Victor Hsieh wrote on Tue, Jul 14, 2020:
> Please disregard this patch.  I misunderstood the protocol and have
> found the actual problem in the hypervisor's 9P implementation.  Sorry
> about the noise.

Ok, thanks for the notice.

Greg KH wrote on Wed, Jul 15, 2020:
> As Eric says, this is fine to cc: stable with this kind of thing.  It's
> good to get a "heads up" on patches that are coming, and Sasha runs some
> tests on them as well to make sure that they really are going to apply
> to what trees you think they should apply to.

Hmm, I'm really not sure how useful that is for first version of a patch
that actually got refused ;)
But if you both say it doesn't hurt I won't advise against it anymore,
thanks for correcting me.

-- 
Dominique

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-15 13:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 21:57 [PATCH] fs/9p: Fix TCREATE's fid in protocol Victor Hsieh
2020-07-14 12:12 ` Dominique Martinet
2020-07-14 20:54   ` Eric Biggers
2020-07-14 21:10     ` Victor Hsieh
2020-07-15 13:35       ` Dominique Martinet
2020-07-15  8:03     ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).