| Message ID | 20191212163904.159893-73-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs daemon [all] | expand |
> From: Miklos Szeredi <mszeredi@redhat.com> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> I'm not familiar with qemu convention but shouldn't we put at least one line of description like linux kernel? For code itself: Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> > --- > tools/virtiofsd/passthrough_ll.c | 50 +++++++++++++++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 0f33c3c5e9..1b84d4f313 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -1077,17 +1077,42 @@ out_err: > fuse_reply_err(req, saverr); > } > > +static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent, > + const char *name) > +{ > + int res; > + struct stat attr; > + > + res = fstatat(lo_fd(req, parent), name, &attr, > + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW); > + if (res == -1) { > + return NULL; > + } > + > + return lo_find(lo_data(req), &attr); > +} > + > static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name) > { > int res; > + struct lo_inode *inode; > + struct lo_data *lo = lo_data(req); > + > if (!is_safe_path_component(name)) { > fuse_reply_err(req, EINVAL); > return; > } > > + inode = lookup_name(req, parent, name); > + if (!inode) { > + fuse_reply_err(req, EIO); > + return; > + } > + > res = unlinkat(lo_fd(req, parent), name, AT_REMOVEDIR); > > fuse_reply_err(req, res == -1 ? errno : 0); > + unref_inode_lolocked(lo, inode, 1); > } > > static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > @@ -1095,12 +1120,23 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > unsigned int flags) > { > int res; > + struct lo_inode *oldinode; > + struct lo_inode *newinode; > + struct lo_data *lo = lo_data(req); > > if (!is_safe_path_component(name) || !is_safe_path_component(newname)) { > fuse_reply_err(req, EINVAL); > return; > } > > + oldinode = lookup_name(req, parent, name); > + newinode = lookup_name(req, newparent, newname); > + > + if (!oldinode) { > + fuse_reply_err(req, EIO); > + goto out; > + } > + > if (flags) { > #ifndef SYS_renameat2 > fuse_reply_err(req, EINVAL); > @@ -1113,26 +1149,38 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > fuse_reply_err(req, res == -1 ? errno : 0); > } > #endif > - return; > + goto out; > } > > res = renameat(lo_fd(req, parent), name, lo_fd(req, newparent), newname); > > fuse_reply_err(req, res == -1 ? errno : 0); > +out: > + unref_inode_lolocked(lo, oldinode, 1); > + unref_inode_lolocked(lo, newinode, 1); > } > > static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) > { > int res; > + struct lo_inode *inode; > + struct lo_data *lo = lo_data(req); > > if (!is_safe_path_component(name)) { > fuse_reply_err(req, EINVAL); > return; > } > > + inode = lookup_name(req, parent, name); > + if (!inode) { > + fuse_reply_err(req, EIO); > + return; > + } > + > res = unlinkat(lo_fd(req, parent), name, 0); > > fuse_reply_err(req, res == -1 ? errno : 0); > + unref_inode_lolocked(lo, inode, 1); > } > > static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode, > -- > 2.23.0
* Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > From: Miklos Szeredi <mszeredi@redhat.com> > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > I'm not familiar with qemu convention but shouldn't we put > at least one line of description like linux kernel? Miklos: would you like to suggest a better commit message? > For code itself: > Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Thanks! > > > --- > > tools/virtiofsd/passthrough_ll.c | 50 +++++++++++++++++++++++++++++++- > > 1 file changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > index 0f33c3c5e9..1b84d4f313 100644 > > --- a/tools/virtiofsd/passthrough_ll.c > > +++ b/tools/virtiofsd/passthrough_ll.c > > @@ -1077,17 +1077,42 @@ out_err: > > fuse_reply_err(req, saverr); > > } > > > > +static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent, > > + const char *name) > > +{ > > + int res; > > + struct stat attr; > > + > > + res = fstatat(lo_fd(req, parent), name, &attr, > > + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW); > > + if (res == -1) { > > + return NULL; > > + } > > + > > + return lo_find(lo_data(req), &attr); > > +} > > + > > static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name) > > { > > int res; > > + struct lo_inode *inode; > > + struct lo_data *lo = lo_data(req); > > + > > if (!is_safe_path_component(name)) { > > fuse_reply_err(req, EINVAL); > > return; > > } > > > > + inode = lookup_name(req, parent, name); > > + if (!inode) { > > + fuse_reply_err(req, EIO); > > + return; > > + } > > + > > res = unlinkat(lo_fd(req, parent), name, AT_REMOVEDIR); > > > > fuse_reply_err(req, res == -1 ? errno : 0); > > + unref_inode_lolocked(lo, inode, 1); > > } > > > > static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > > @@ -1095,12 +1120,23 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > > unsigned int flags) > > { > > int res; > > + struct lo_inode *oldinode; > > + struct lo_inode *newinode; > > + struct lo_data *lo = lo_data(req); > > > > if (!is_safe_path_component(name) || !is_safe_path_component(newname)) { > > fuse_reply_err(req, EINVAL); > > return; > > } > > > > + oldinode = lookup_name(req, parent, name); > > + newinode = lookup_name(req, newparent, newname); > > + > > + if (!oldinode) { > > + fuse_reply_err(req, EIO); > > + goto out; > > + } > > + > > if (flags) { > > #ifndef SYS_renameat2 > > fuse_reply_err(req, EINVAL); > > @@ -1113,26 +1149,38 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, > > fuse_reply_err(req, res == -1 ? errno : 0); > > } > > #endif > > - return; > > + goto out; > > } > > > > res = renameat(lo_fd(req, parent), name, lo_fd(req, newparent), newname); > > > > fuse_reply_err(req, res == -1 ? errno : 0); > > +out: > > + unref_inode_lolocked(lo, oldinode, 1); > > + unref_inode_lolocked(lo, newinode, 1); > > } > > > > static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) > > { > > int res; > > + struct lo_inode *inode; > > + struct lo_data *lo = lo_data(req); > > > > if (!is_safe_path_component(name)) { > > fuse_reply_err(req, EINVAL); > > return; > > } > > > > + inode = lookup_name(req, parent, name); > > + if (!inode) { > > + fuse_reply_err(req, EIO); > > + return; > > + } > > + > > res = unlinkat(lo_fd(req, parent), name, 0); > > > > fuse_reply_err(req, res == -1 ? errno : 0); > > + unref_inode_lolocked(lo, inode, 1); > > } > > > > static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode, > > -- > > 2.23.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jan 16, 2020 at 5:45 PM Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > > From: Miklos Szeredi <mszeredi@redhat.com> > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > > I'm not familiar with qemu convention but shouldn't we put > > at least one line of description like linux kernel? > > Miklos: would you like to suggest a better commit message? Hmm, the patch doesn't really make sense, since the looked up inode is not used. Not sure what happened here, this seems to be for supporting shared versions, and these changes are part of commit 06f78a397f00 ("virtiofsd: add initial support for shared versions") in our gitlab qemu tree. Was this intentionally split out? Thanks, Miklos
* Miklos Szeredi (mszeredi@redhat.com) wrote: > On Thu, Jan 16, 2020 at 5:45 PM Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > > > From: Miklos Szeredi <mszeredi@redhat.com> > > > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > > > > I'm not familiar with qemu convention but shouldn't we put > > > at least one line of description like linux kernel? > > > > Miklos: would you like to suggest a better commit message? > > Hmm, the patch doesn't really make sense, since the looked up inode is not used. > > Not sure what happened here, this seems to be for supporting shared > versions, and these changes are part of commit 06f78a397f00 > ("virtiofsd: add initial support for shared versions") in our gitlab > qemu tree. Was this intentionally split out? I remember I did split the shared version support out when trying to remove it into a separate patch; let me see if I can remove this without breaking the merge around it. Dave > Thanks, > Miklos > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Miklos Szeredi (mszeredi@redhat.com) wrote: > On Thu, Jan 16, 2020 at 5:45 PM Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > > > From: Miklos Szeredi <mszeredi@redhat.com> > > > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > > > > I'm not familiar with qemu convention but shouldn't we put > > > at least one line of description like linux kernel? > > > > Miklos: would you like to suggest a better commit message? > > Hmm, the patch doesn't really make sense, since the looked up inode is not used. > > Not sure what happened here, this seems to be for supporting shared > versions, and these changes are part of commit 06f78a397f00 > ("virtiofsd: add initial support for shared versions") in our gitlab > qemu tree. Was this intentionally split out? I think the reason I kept this here is because Stefan's 'introduce inode refcount to prevent use-after-free' does use the the inodes in lo_rename; is it also dependent on having done the in lo_rmdir and lo_unlink? Dave > Thanks, > Miklos > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes: > From: Miklos Szeredi <mszeredi@redhat.com> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 50 +++++++++++++++++++++++++++++++- > 1 file changed, 49 insertions(+), 1 deletion(-) This one is missing a commit message, and I think the patch isn't trivial enough to give it a pass without it. Sergio.
* Sergio Lopez (slp@redhat.com) wrote: > > Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes: > > > From: Miklos Szeredi <mszeredi@redhat.com> > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > --- > > tools/virtiofsd/passthrough_ll.c | 50 +++++++++++++++++++++++++++++++- > > 1 file changed, 49 insertions(+), 1 deletion(-) > > This one is missing a commit message, and I think the patch isn't > trivial enough to give it a pass without it. Yep, see discussion: https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg03296.html > Sergio. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 0f33c3c5e9..1b84d4f313 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -1077,17 +1077,42 @@ out_err: fuse_reply_err(req, saverr); } +static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent, + const char *name) +{ + int res; + struct stat attr; + + res = fstatat(lo_fd(req, parent), name, &attr, + AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW); + if (res == -1) { + return NULL; + } + + return lo_find(lo_data(req), &attr); +} + static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name) { int res; + struct lo_inode *inode; + struct lo_data *lo = lo_data(req); + if (!is_safe_path_component(name)) { fuse_reply_err(req, EINVAL); return; } + inode = lookup_name(req, parent, name); + if (!inode) { + fuse_reply_err(req, EIO); + return; + } + res = unlinkat(lo_fd(req, parent), name, AT_REMOVEDIR); fuse_reply_err(req, res == -1 ? errno : 0); + unref_inode_lolocked(lo, inode, 1); } static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, @@ -1095,12 +1120,23 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, unsigned int flags) { int res; + struct lo_inode *oldinode; + struct lo_inode *newinode; + struct lo_data *lo = lo_data(req); if (!is_safe_path_component(name) || !is_safe_path_component(newname)) { fuse_reply_err(req, EINVAL); return; } + oldinode = lookup_name(req, parent, name); + newinode = lookup_name(req, newparent, newname); + + if (!oldinode) { + fuse_reply_err(req, EIO); + goto out; + } + if (flags) { #ifndef SYS_renameat2 fuse_reply_err(req, EINVAL); @@ -1113,26 +1149,38 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, fuse_reply_err(req, res == -1 ? errno : 0); } #endif - return; + goto out; } res = renameat(lo_fd(req, parent), name, lo_fd(req, newparent), newname); fuse_reply_err(req, res == -1 ? errno : 0); +out: + unref_inode_lolocked(lo, oldinode, 1); + unref_inode_lolocked(lo, newinode, 1); } static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) { int res; + struct lo_inode *inode; + struct lo_data *lo = lo_data(req); if (!is_safe_path_component(name)) { fuse_reply_err(req, EINVAL); return; } + inode = lookup_name(req, parent, name); + if (!inode) { + fuse_reply_err(req, EIO); + return; + } + res = unlinkat(lo_fd(req, parent), name, 0); fuse_reply_err(req, res == -1 ? errno : 0); + unref_inode_lolocked(lo, inode, 1); } static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode,