| Message ID | 20190726091103.23503-3-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: multithreading preparation | expand |
On Fri, Jul 26, 2019 at 10:11:00AM +0100, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > Sigh...this one has been fixed by 3 different developers...Me, Pengtao and Stefan. The following one on the ML did the exactly same thing. --- Subject: [Virtio-fs] [PATCH] virtiofsd: fix lo_do_lookup panic It needs to check for invalid parent dir. Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com> --- thanks, -liubo > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ 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); > > + if (!dir) { > + return EBADF; > + } > + > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; > -- > 2.21.0 > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs
Hi Stefan, On 2019/7/26 17:11, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ 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); > > + if (!dir) { > + return EBADF; > + } > + I worry about that dir will be released or set NULL just after NULL checking. Or could we use some lock to prevent the simultaneity? Thanks, Jun > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; >
On Fri, Jul 26, 2019 at 02:26:14PM -0700, Liu Bo wrote: > On Fri, Jul 26, 2019 at 10:11:00AM +0100, Stefan Hajnoczi wrote: > > Most lo_do_lookup() have already checked that the parent inode exists. > > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > > lo_inode(req, parent) returns NULL. > > > > Sigh...this one has been fixed by 3 different developers...Me, Pengtao and Stefan. > > The following one on the ML did the exactly same thing. > --- > Subject: [Virtio-fs] [PATCH] virtiofsd: fix lo_do_lookup panic > > It needs to check for invalid parent dir. > > Signed-off-by: Peng Tao <tao.peng@linux.alibaba.com> > --- One of the previously posted patches will be merged before mine. It's a shame that work has been duplicated. As a contributor I will send a ping email if there has been no response to a patch after a few days. Stefan
Hi Stefan, On 2019/7/26 17:11, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ 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); > > + if (!dir) { > + return EBADF; > + } > + I worry about that dir will be released or set NULL just after NULL checking. Or could we use some lock to prevent the simultaneity? Thanks, Jun > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; >
On Mon, Jul 29, 2019 at 08:35:36PM +0800, piaojun wrote: > Hi Stefan, > > On 2019/7/26 17:11, Stefan Hajnoczi wrote: > > Most lo_do_lookup() have already checked that the parent inode exists. > > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > > lo_inode(req, parent) returns NULL. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > contrib/virtiofsd/passthrough_ll.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > > index 9ae1381618..277a17fc03 100644 > > --- a/contrib/virtiofsd/passthrough_ll.c > > +++ b/contrib/virtiofsd/passthrough_ll.c > > @@ -766,6 +766,10 @@ 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); > > > > + if (!dir) { > > + return EBADF; > > + } > > + > > I worry about that dir will be released or set NULL just after NULL > checking. Or could we use some lock to prevent the simultaneity? Yes, I agree. I haven't audited lo_inode yet, but it needs a refcount and/or lock to ensure accesses are safe. I'll do that and other things in a separate patch series. Stefan
On 2019/7/29 23:41, Stefan Hajnoczi wrote: > On Mon, Jul 29, 2019 at 08:35:36PM +0800, piaojun wrote: >> Hi Stefan, >> >> On 2019/7/26 17:11, Stefan Hajnoczi wrote: >>> Most lo_do_lookup() have already checked that the parent inode exists. >>> lo_lookup() hasn't and can therefore hit a NULL pointer dereference when >>> lo_inode(req, parent) returns NULL. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> contrib/virtiofsd/passthrough_ll.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c >>> index 9ae1381618..277a17fc03 100644 >>> --- a/contrib/virtiofsd/passthrough_ll.c >>> +++ b/contrib/virtiofsd/passthrough_ll.c >>> @@ -766,6 +766,10 @@ 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); >>> >>> + if (!dir) { >>> + return EBADF; >>> + } >>> + >> >> I worry about that dir will be released or set NULL just after NULL >> checking. Or could we use some lock to prevent the simultaneity? > > Yes, I agree. I haven't audited lo_inode yet, but it needs a refcount > and/or lock to ensure accesses are safe. I'll do that and other things > in a separate patch series. > > Stefan OK, that sounds good. Jun >
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 9ae1381618..277a17fc03 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -766,6 +766,10 @@ 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); + if (!dir) { + return EBADF; + } + memset(e, 0, sizeof(*e)); e->attr_timeout = lo->timeout; e->entry_timeout = lo->timeout;
Most lo_do_lookup() have already checked that the parent inode exists. lo_lookup() hasn't and can therefore hit a NULL pointer dereference when lo_inode(req, parent) returns NULL. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 4 ++++ 1 file changed, 4 insertions(+)