[PATCH] fs/9p: Fix TCREATE's fid in protocol

[PATCH] fs/9p: Fix TCREATE's fid in protocol

[Victor Hsieh]

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

Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol

[Dominique Martinet]

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",

Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol

[Eric Biggers]

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

Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol

[Victor Hsieh]

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

Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol

[Greg KH]

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

Re: [PATCH] fs/9p: Fix TCREATE's fid in protocol

[Dominique Martinet]

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