* [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
@ 2019-05-23 2:06 Liu Bo
2019-05-23 3:22 ` Eryu Guan
2019-07-31 13:01 ` Stefan Hajnoczi
0 siblings, 2 replies; 6+ messages in thread
From: Liu Bo @ 2019-05-23 2:06 UTC (permalink / raw)
To: virtio-fs
Reported by fstests/generic/467.
open_by_handle_at() called from fuse inside guest can carry fuse mount
point to daemon but lo_do_lookup() doesn't know its inode info because
it's out of fuse's scope, thus lo_inode(req, parent) ends up with
returning a NULL dir and breaks virtiofsd immediately.
Note that it'd break applications that uses open_by_handle_at.
It seems to me that nothing could be done to support open_by_handle_at in
this case.
This simply tells fuse a ENOENT error so that open_by_handle_at() in guest
can get a ESTALE.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 9b7e515..b58708f 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -640,6 +640,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
struct lo_data *lo = lo_data(req);
struct lo_inode *inode, *dir = lo_inode(req, parent);
+ /*
+ * name_to_handle_at() and open_by_handle_at() can reach here with fuse
+ * mount point in guest, but we don't have its inode info in the
+ * ino_map.
+ */
+ if (!dir)
+ return ENOENT;
+
memset(e, 0, sizeof(*e));
e->attr_timeout = lo->timeout;
e->entry_timeout = lo->timeout;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
2019-05-23 2:06 [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup Liu Bo
@ 2019-05-23 3:22 ` Eryu Guan
2019-05-28 21:29 ` Vivek Goyal
2019-07-31 13:01 ` Stefan Hajnoczi
1 sibling, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2019-05-23 3:22 UTC (permalink / raw)
To: Liu Bo; +Cc: virtio-fs
On Thu, May 23, 2019 at 10:06:34AM +0800, Liu Bo wrote:
> Reported by fstests/generic/467.
>
> open_by_handle_at() called from fuse inside guest can carry fuse mount
> point to daemon but lo_do_lookup() doesn't know its inode info because
> it's out of fuse's scope, thus lo_inode(req, parent) ends up with
> returning a NULL dir and breaks virtiofsd immediately.
>
> Note that it'd break applications that uses open_by_handle_at.
>
> It seems to me that nothing could be done to support open_by_handle_at in
> this case.
Then perhaps we should consider disabling exportfs support in virtio-fs
case? I noticed that currently fuse set
sb->s_export_op = &fuse_export_operations
unconditionally, maybe we could implement a feature mask in fuse and let
filesystem implementation registers what features it supports, e.g.
exportfs in this case.
Or we could try adding real exportfs support to virtiofs? Though I'm not
sure if it's possible or worth it at the moment.
>
> This simply tells fuse a ENOENT error so that open_by_handle_at() in guest
> can get a ESTALE.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
The patch itself looks fine to me.
Reviewed-by: Eryu Guan <eguan@linux.alibaba.com>
Thanks,
Eryu
> ---
> contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 9b7e515..b58708f 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -640,6 +640,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> struct lo_data *lo = lo_data(req);
> struct lo_inode *inode, *dir = lo_inode(req, parent);
>
> + /*
> + * name_to_handle_at() and open_by_handle_at() can reach here with fuse
> + * mount point in guest, but we don't have its inode info in the
> + * ino_map.
> + */
> + if (!dir)
> + return ENOENT;
> +
> memset(e, 0, sizeof(*e));
> e->attr_timeout = lo->timeout;
> e->entry_timeout = lo->timeout;
> --
> 1.8.3.1
>
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
2019-05-23 3:22 ` Eryu Guan
@ 2019-05-28 21:29 ` Vivek Goyal
2019-05-31 10:05 ` Eryu Guan
0 siblings, 1 reply; 6+ messages in thread
From: Vivek Goyal @ 2019-05-28 21:29 UTC (permalink / raw)
To: Eryu Guan; +Cc: virtio-fs
On Thu, May 23, 2019 at 11:22:54AM +0800, Eryu Guan wrote:
> On Thu, May 23, 2019 at 10:06:34AM +0800, Liu Bo wrote:
> > Reported by fstests/generic/467.
> >
> > open_by_handle_at() called from fuse inside guest can carry fuse mount
> > point to daemon but lo_do_lookup() doesn't know its inode info because
> > it's out of fuse's scope, thus lo_inode(req, parent) ends up with
> > returning a NULL dir and breaks virtiofsd immediately.
> >
> > Note that it'd break applications that uses open_by_handle_at.
> >
> > It seems to me that nothing could be done to support open_by_handle_at in
> > this case.
>
> Then perhaps we should consider disabling exportfs support in virtio-fs
> case? I noticed that currently fuse set
>
> sb->s_export_op = &fuse_export_operations
>
> unconditionally, maybe we could implement a feature mask in fuse and let
> filesystem implementation registers what features it supports, e.g.
> exportfs in this case.
>
> Or we could try adding real exportfs support to virtiofs? Though I'm not
> sure if it's possible or worth it at the moment.
If we don't support export operations, we should disable it. Looks like
we have enabled it in virtiofsd.
lo_init() {
if(conn->capable & FUSE_CAP_EXPORT_SUPPORT)
conn->want |= FUSE_CAP_EXPORT_SUPPORT;
}
So if we were to disable it, I guess we will have to remove it.
Not sure if it is possible to support export ops in virtio-fs case. Need
to read more about it.
Vivek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
2019-05-28 21:29 ` Vivek Goyal
@ 2019-05-31 10:05 ` Eryu Guan
0 siblings, 0 replies; 6+ messages in thread
From: Eryu Guan @ 2019-05-31 10:05 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs
On Tue, May 28, 2019 at 05:29:33PM -0400, Vivek Goyal wrote:
> On Thu, May 23, 2019 at 11:22:54AM +0800, Eryu Guan wrote:
> > On Thu, May 23, 2019 at 10:06:34AM +0800, Liu Bo wrote:
> > > Reported by fstests/generic/467.
> > >
> > > open_by_handle_at() called from fuse inside guest can carry fuse mount
> > > point to daemon but lo_do_lookup() doesn't know its inode info because
> > > it's out of fuse's scope, thus lo_inode(req, parent) ends up with
> > > returning a NULL dir and breaks virtiofsd immediately.
> > >
> > > Note that it'd break applications that uses open_by_handle_at.
> > >
> > > It seems to me that nothing could be done to support open_by_handle_at in
> > > this case.
> >
> > Then perhaps we should consider disabling exportfs support in virtio-fs
> > case? I noticed that currently fuse set
> >
> > sb->s_export_op = &fuse_export_operations
> >
> > unconditionally, maybe we could implement a feature mask in fuse and let
> > filesystem implementation registers what features it supports, e.g.
> > exportfs in this case.
> >
> > Or we could try adding real exportfs support to virtiofs? Though I'm not
> > sure if it's possible or worth it at the moment.
>
> If we don't support export operations, we should disable it. Looks like
> we have enabled it in virtiofsd.
>
> lo_init() {
> if(conn->capable & FUSE_CAP_EXPORT_SUPPORT)
> conn->want |= FUSE_CAP_EXPORT_SUPPORT;
> }
>
> So if we were to disable it, I guess we will have to remove it.
Looks like only removing above lines doesn't disable export support (or
only partially disabled), the test still runs & fails. Removing the
EXPORT_SUPPORT cap only tells fuse to set fc->export_support to 0 and
return -ESTALE on export operations like fuse_get_parent(), but the
name_to_handle_at(2) syscall still could encode file into handle (which
checks sb->s_export_op->fh_to_dentry), so the test thinks virtio-fs
supports exports.
I have to reset sb->s_export_op to NULL when fc->export_support is 0 to
make test not run, as there's no point to have a "half broken" export
operations. This makes me wonder if we could remove
fuse_conn.export_support field all together, as it serves no purpose
when we don't have s_export_op set.
Thanks,
Eryu
>
> Not sure if it is possible to support export ops in virtio-fs case. Need
> to read more about it.
>
> Vivek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
2019-05-23 2:06 [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup Liu Bo
2019-05-23 3:22 ` Eryu Guan
@ 2019-07-31 13:01 ` Stefan Hajnoczi
2019-07-31 16:07 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2019-07-31 13:01 UTC (permalink / raw)
To: Liu Bo; +Cc: virtio-fs
[-- Attachment #1: Type: text/plain, Size: 845 bytes --]
On Thu, May 23, 2019 at 10:06:34AM +0800, Liu Bo wrote:
> Reported by fstests/generic/467.
>
> open_by_handle_at() called from fuse inside guest can carry fuse mount
> point to daemon but lo_do_lookup() doesn't know its inode info because
> it's out of fuse's scope, thus lo_inode(req, parent) ends up with
> returning a NULL dir and breaks virtiofsd immediately.
>
> Note that it'd break applications that uses open_by_handle_at.
>
> It seems to me that nothing could be done to support open_by_handle_at in
> this case.
>
> This simply tells fuse a ENOENT error so that open_by_handle_at() in guest
> can get a ESTALE.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup
2019-07-31 13:01 ` Stefan Hajnoczi
@ 2019-07-31 16:07 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-07-31 16:07 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Thu, May 23, 2019 at 10:06:34AM +0800, Liu Bo wrote:
> > Reported by fstests/generic/467.
> >
> > open_by_handle_at() called from fuse inside guest can carry fuse mount
> > point to daemon but lo_do_lookup() doesn't know its inode info because
> > it's out of fuse's scope, thus lo_inode(req, parent) ends up with
> > returning a NULL dir and breaks virtiofsd immediately.
> >
> > Note that it'd break applications that uses open_by_handle_at.
> >
> > It seems to me that nothing could be done to support open_by_handle_at in
> > this case.
> >
> > This simply tells fuse a ENOENT error so that open_by_handle_at() in guest
> > can get a ESTALE.
> >
> > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> > ---
> > contrib/virtiofsd/passthrough_ll.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
OK, I've finally merged this.
Apologies for the delay; I got hung up on the questions about exportfs
that had been asked at the time and wanted to understand that
we had an answer.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-31 16:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 2:06 [Virtio-fs] [PATCH] virtiofsd: handle NULL dir in lo_do_lookup Liu Bo
2019-05-23 3:22 ` Eryu Guan
2019-05-28 21:29 ` Vivek Goyal
2019-05-31 10:05 ` Eryu Guan
2019-07-31 13:01 ` Stefan Hajnoczi
2019-07-31 16:07 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.