| Message ID | 20200713215759.3701482-1-victorhsieh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/9p: Fix TCREATE's fid in protocol | expand |
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",
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
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
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
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",
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(-)