[Qemu-devel] [PATCH 0/5] virtiofsd: multithreading preparation

[Qemu-devel] [PATCH 0/5] virtiofsd: multithreading preparation

[Stefan Hajnoczi]

virtiofsd is not ready for multithreading yet.  This patch series starts to
make the code capable of processing multiple FUSE requests simultaneously.  I'm
sending these fixes split into several patch series as I make progress auditing
the code for thread-safety issues.  The final patch series will use a
threadpool to process requests from a virtqueue in parallel and it will also
enable multiqueue.

Patches 1 & 2 are cleanups discovered when auditing the code.  They are not
related to multithreading.

Stefan Hajnoczi (5):
  virtiofsd: skip unnecessary vu_queue_get_avail_bytes()
  virtiofsd: prevent lo_lookup() NULL pointer dereference
  virtiofsd: make lo_release() atomic
  virtiofsd: drop lo_dirp->fd field
  virtiofsd: prevent races with lo_dirp_put()

 contrib/virtiofsd/fuse_virtio.c    | 13 +++---
 contrib/virtiofsd/passthrough_ll.c | 71 +++++++++++++++++++++++-------
 2 files changed, 60 insertions(+), 24 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/5] virtiofsd: skip unnecessary vu_queue_get_avail_bytes()

[Stefan Hajnoczi]

When debug output is disabled there is no need to calculate the number
of in/out bytes available.

There is also no need to skip a request if there are 0 out bytes.  The
request parsing code already handles invalid requests.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/fuse_virtio.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
index 083e4fc317..d543c6d30f 100644
--- a/contrib/virtiofsd/fuse_virtio.c
+++ b/contrib/virtiofsd/fuse_virtio.c
@@ -507,18 +507,16 @@ static void *fv_queue_thread(void *opaque)
                ret = pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock);
                assert(ret == 0); /* there is no possible error case */
 
-               /* out is from guest, in is too guest */
-               unsigned int in_bytes, out_bytes;
-               vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
+               if (se->debug) {
+                       /* out is from guest, in is too guest */
+                       unsigned int in_bytes, out_bytes;
+                       vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
 
-               if (se->debug)
                        fuse_debug("%s: Queue %d gave evalue: %zx available: in: %u out: %u\n",
 				  __func__, qi->qidx, (size_t)evalue, in_bytes,
 				  out_bytes);
-
-               if (!out_bytes) {
-                       goto next;
                }
+
                while (1) {
                        bool allocated_bufv = false;
                        struct fuse_bufvec bufv;
@@ -708,7 +706,6 @@ static void *fv_queue_thread(void *opaque)
                        elem = NULL;
                 }
 
-next:
                 pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock);
         }
         pthread_mutex_destroy(&ch.lock);
-- 
2.21.0

Re: [Qemu-devel] [Virtio-fs] [PATCH 1/5] virtiofsd: skip unnecessary vu_queue_get_avail_bytes()

[Liu Bo]

On Fri, Jul 26, 2019 at 10:10:59AM +0100, Stefan Hajnoczi wrote:
> When debug output is disabled there is no need to calculate the number
> of in/out bytes available.
> 
> There is also no need to skip a request if there are 0 out bytes.  The
> request parsing code already handles invalid requests.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/virtiofsd/fuse_virtio.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
> index 083e4fc317..d543c6d30f 100644
> --- a/contrib/virtiofsd/fuse_virtio.c
> +++ b/contrib/virtiofsd/fuse_virtio.c
> @@ -507,18 +507,16 @@ static void *fv_queue_thread(void *opaque)
>                 ret = pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock);
>                 assert(ret == 0); /* there is no possible error case */
>  
> -               /* out is from guest, in is too guest */
> -               unsigned int in_bytes, out_bytes;
> -               vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
> +               if (se->debug) {
> +                       /* out is from guest, in is too guest */
> +                       unsigned int in_bytes, out_bytes;
> +                       vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
>  
> -               if (se->debug)
>                         fuse_debug("%s: Queue %d gave evalue: %zx available: in: %u out: %u\n",
>  				  __func__, qi->qidx, (size_t)evalue, in_bytes,
>  				  out_bytes);
> -
> -               if (!out_bytes) {
> -                       goto next;
>                 }
> +
>                 while (1) {
>                         bool allocated_bufv = false;
>                         struct fuse_bufvec bufv;
> @@ -708,7 +706,6 @@ static void *fv_queue_thread(void *opaque)
>                         elem = NULL;
>                  }
>  
> -next:
>                  pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock);
>          }
>          pthread_mutex_destroy(&ch.lock);
> -- 
> 2.21.0
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

Reviewed-by: Liu Bo <bo.liu@linux.alibaba.com>

thanks,
-liubo

Re: [Qemu-devel] [PATCH 1/5] virtiofsd: skip unnecessary vu_queue_get_avail_bytes()

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> When debug output is disabled there is no need to calculate the number
> of in/out bytes available.
> 
> There is also no need to skip a request if there are 0 out bytes.  The
> request parsing code already handles invalid requests.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks, I've squashed this in to existing commits.

Dave

> ---
>  contrib/virtiofsd/fuse_virtio.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
> index 083e4fc317..d543c6d30f 100644
> --- a/contrib/virtiofsd/fuse_virtio.c
> +++ b/contrib/virtiofsd/fuse_virtio.c
> @@ -507,18 +507,16 @@ static void *fv_queue_thread(void *opaque)
>                 ret = pthread_rwlock_rdlock(&qi->virtio_dev->vu_dispatch_rwlock);
>                 assert(ret == 0); /* there is no possible error case */
>  
> -               /* out is from guest, in is too guest */
> -               unsigned int in_bytes, out_bytes;
> -               vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
> +               if (se->debug) {
> +                       /* out is from guest, in is too guest */
> +                       unsigned int in_bytes, out_bytes;
> +                       vu_queue_get_avail_bytes(dev, q, &in_bytes, &out_bytes, ~0, ~0);
>  
> -               if (se->debug)
>                         fuse_debug("%s: Queue %d gave evalue: %zx available: in: %u out: %u\n",
>  				  __func__, qi->qidx, (size_t)evalue, in_bytes,
>  				  out_bytes);
> -
> -               if (!out_bytes) {
> -                       goto next;
>                 }
> +
>                 while (1) {
>                         bool allocated_bufv = false;
>                         struct fuse_bufvec bufv;
> @@ -708,7 +706,6 @@ static void *fv_queue_thread(void *opaque)
>                         elem = NULL;
>                  }
>  
> -next:
>                  pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock);
>          }
>          pthread_mutex_destroy(&ch.lock);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[Stefan Hajnoczi]

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;
+	}
+
 	memset(e, 0, sizeof(*e));
 	e->attr_timeout = lo->timeout;
 	e->entry_timeout = lo->timeout;
-- 
2.21.0

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[Liu Bo]

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

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[piaojun]

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;
> 

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[piaojun]

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;
> 

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference

[piaojun]

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

> 

[Qemu-devel] [PATCH 3/5] virtiofsd: make lo_release() atomic

[Stefan Hajnoczi]

Hold the lock across both lo_map_get() and lo_map_remove() to prevent
races between two FUSE_RELEASE requests.  In this case I don't see a
serious bug but it's safer to do things atomically.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 277a17fc03..c1500e092d 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1759,14 +1759,18 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 static void lo_release(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 {
 	struct lo_data *lo = lo_data(req);
-	int fd;
+	struct lo_map_elem *elem;
+	int fd = -1;
 
 	(void) ino;
 
-	fd = lo_fi_fd(req, fi);
-
 	pthread_mutex_lock(&lo->mutex);
-	lo_map_remove(&lo->fd_map, fi->fh);
+	elem = lo_map_get(&lo->fd_map, fi->fh);
+	if (elem) {
+		fd = elem->fd;
+		elem = NULL;
+		lo_map_remove(&lo->fd_map, fi->fh);
+	}
 	pthread_mutex_unlock(&lo->mutex);
 
 	close(fd);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 3/5] virtiofsd: make lo_release() atomic

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> Hold the lock across both lo_map_get() and lo_map_remove() to prevent
> races between two FUSE_RELEASE requests.  In this case I don't see a
> serious bug but it's safer to do things atomically.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

OK, although I suspect there are lots of places the inode pointers 
are passed without the lock, so it might not help much.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


and applied

> ---
>  contrib/virtiofsd/passthrough_ll.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index 277a17fc03..c1500e092d 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1759,14 +1759,18 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  static void lo_release(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  {
>  	struct lo_data *lo = lo_data(req);
> -	int fd;
> +	struct lo_map_elem *elem;
> +	int fd = -1;
>  
>  	(void) ino;
>  
> -	fd = lo_fi_fd(req, fi);
> -
>  	pthread_mutex_lock(&lo->mutex);
> -	lo_map_remove(&lo->fd_map, fi->fh);
> +	elem = lo_map_get(&lo->fd_map, fi->fh);
> +	if (elem) {
> +		fd = elem->fd;
> +		elem = NULL;
> +		lo_map_remove(&lo->fd_map, fi->fh);
> +	}
>  	pthread_mutex_unlock(&lo->mutex);
>  
>  	close(fd);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 4/5] virtiofsd: drop lo_dirp->fd field

[Stefan Hajnoczi]

fdopendir(3) takes ownership of the file descriptor.  The presence of
the lo_dirp->fd field could lead to someone incorrectly adding a
close(d->fd) cleanup call in the future.

Do not store the file descriptor in struct lo_dirp since it is unused.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index c1500e092d..ad3abdd532 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1293,7 +1293,6 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
 }
 
 struct lo_dirp {
-	int fd;
 	DIR *dp;
 	struct dirent *entry;
 	off_t offset;
@@ -1319,16 +1318,17 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
 	struct lo_data *lo = lo_data(req);
 	struct lo_dirp *d;
 	ssize_t fh;
+	int fd = -1;
 
 	d = calloc(1, sizeof(struct lo_dirp));
 	if (d == NULL)
 		goto out_err;
 
-	d->fd = openat(lo_fd(req, ino), ".", O_RDONLY);
-	if (d->fd == -1)
+	fd = openat(lo_fd(req, ino), ".", O_RDONLY);
+	if (fd == -1)
 		goto out_errno;
 
-	d->dp = fdopendir(d->fd);
+	d->dp = fdopendir(fd);
 	if (d->dp == NULL)
 		goto out_errno;
 
@@ -1348,11 +1348,12 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
 out_errno:
 	error = errno;
 out_err:
+	if (fd != -1) {
+		close(fd);
+	}
 	if (d) {
 		if (d->dp)
 			closedir(d->dp);
-		if (d->fd != -1)
-			close(d->fd);
 		free(d);
 	}
 	fuse_reply_err(req, error);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 4/5] virtiofsd: drop lo_dirp->fd field

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> fdopendir(3) takes ownership of the file descriptor.  The presence of
> the lo_dirp->fd field could lead to someone incorrectly adding a
> close(d->fd) cleanup call in the future.
> 
> Do not store the file descriptor in struct lo_dirp since it is unused.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Thanks, applied; note:
  a) This looks like it can go into upstream libfuse
  b) I think we're probably leaking DIR *'s if we do an lo_shutdown;
I've added that to my todo

> ---
>  contrib/virtiofsd/passthrough_ll.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index c1500e092d..ad3abdd532 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1293,7 +1293,6 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
>  }
>  
>  struct lo_dirp {
> -	int fd;
>  	DIR *dp;
>  	struct dirent *entry;
>  	off_t offset;
> @@ -1319,16 +1318,17 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
>  	struct lo_data *lo = lo_data(req);
>  	struct lo_dirp *d;
>  	ssize_t fh;
> +	int fd = -1;
>  
>  	d = calloc(1, sizeof(struct lo_dirp));
>  	if (d == NULL)
>  		goto out_err;
>  
> -	d->fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> -	if (d->fd == -1)
> +	fd = openat(lo_fd(req, ino), ".", O_RDONLY);
> +	if (fd == -1)
>  		goto out_errno;
>  
> -	d->dp = fdopendir(d->fd);
> +	d->dp = fdopendir(fd);
>  	if (d->dp == NULL)
>  		goto out_errno;
>  
> @@ -1348,11 +1348,12 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
>  out_errno:
>  	error = errno;
>  out_err:
> +	if (fd != -1) {
> +		close(fd);
> +	}
>  	if (d) {
>  		if (d->dp)
>  			closedir(d->dp);
> -		if (d->fd != -1)
> -			close(d->fd);
>  		free(d);
>  	}
>  	fuse_reply_err(req, error);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/5] virtiofsd: drop lo_dirp->fd field

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Wed, Jul 31, 2019 at 06:27:16PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > fdopendir(3) takes ownership of the file descriptor.  The presence of
> > the lo_dirp->fd field could lead to someone incorrectly adding a
> > close(d->fd) cleanup call in the future.
> > 
> > Do not store the file descriptor in struct lo_dirp since it is unused.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> Thanks, applied; note:
>   a) This looks like it can go into upstream libfuse
>   b) I think we're probably leaking DIR *'s if we do an lo_shutdown;
> I've added that to my todo

FUSE_DESTROY is current broken, it leaks resources and is not
thread-safe.  It's something I'll tackle in multithreading preparation
part 3.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 5/5] virtiofsd: prevent races with lo_dirp_put()

[Stefan Hajnoczi]

Introduce lo_dirp_put() so that FUSE_RELEASEDIR does not cause
use-after-free races with other threads that are accessing lo_dirp.

Also make lo_releasedir() atomic to prevent FUSE_RELEASEDIR racing with
itself.  This prevents double-frees.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index ad3abdd532..f74e7d2d21 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1293,11 +1293,28 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
 }
 
 struct lo_dirp {
+	gint refcount;
 	DIR *dp;
 	struct dirent *entry;
 	off_t offset;
 };
 
+static void lo_dirp_put(struct lo_dirp **dp)
+{
+	struct lo_dirp *d = *dp;
+
+	if (!d) {
+		return;
+	}
+	*dp = NULL;
+
+	if (g_atomic_int_dec_and_test(&d->refcount)) {
+		closedir(d->dp);
+		free(d);
+	}
+}
+
+/* Call lo_dirp_put() on the return value when no longer needed */
 static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
 {
 	struct lo_data *lo = lo_data(req);
@@ -1305,6 +1322,9 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
 
 	pthread_mutex_lock(&lo->mutex);
 	elem = lo_map_get(&lo->dirp_map, fi->fh);
+	if (elem) {
+		g_atomic_int_inc(&elem->dirp->refcount);
+	}
 	pthread_mutex_unlock(&lo->mutex);
 	if (!elem)
 		return NULL;
@@ -1335,6 +1355,8 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
 	d->offset = 0;
 	d->entry = NULL;
 
+	g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */
+
 	fh = lo_add_dirp_mapping(req, d);
 	if (fh == -1)
 		goto out_err;
@@ -1363,7 +1385,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
 			  off_t offset, struct fuse_file_info *fi, int plus)
 {
 	struct lo_data *lo = lo_data(req);
-	struct lo_dirp *d;
+	struct lo_dirp *d = NULL;
 	struct lo_inode *dinode;
 	char *buf = NULL;
 	char *p;
@@ -1451,6 +1473,8 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
 
     err = 0;
 error:
+    lo_dirp_put(&d);
+
     // If there's an error, we can only signal it if we haven't stored
     // any entries yet - otherwise we'd end up with wrong lookup
     // counts for the entries that are already in the buffer. So we
@@ -1477,22 +1501,25 @@ static void lo_readdirplus(fuse_req_t req, fuse_ino_t ino, size_t size,
 static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 {
 	struct lo_data *lo = lo_data(req);
+	struct lo_map_elem *elem;
 	struct lo_dirp *d;
 
 	(void) ino;
 
-	d = lo_dirp(req, fi);
-	if (!d) {
+	pthread_mutex_lock(&lo->mutex);
+	elem = lo_map_get(&lo->dirp_map, fi->fh);
+	if (!elem) {
+		pthread_mutex_unlock(&lo->mutex);
 		fuse_reply_err(req, EBADF);
 		return;
 	}
 
-	pthread_mutex_lock(&lo->mutex);
+	d = elem->dirp;
 	lo_map_remove(&lo->dirp_map, fi->fh);
 	pthread_mutex_unlock(&lo->mutex);
 
-	closedir(d->dp);
-	free(d);
+	lo_dirp_put(&d); /* paired with lo_opendir() */
+
 	fuse_reply_err(req, 0);
 }
 
@@ -1701,6 +1728,9 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
 		res = fdatasync(fd);
 	else
 		res = fsync(fd);
+
+	lo_dirp_put(&d);
+
 	fuse_reply_err(req, res == -1 ? errno : 0);
 }
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH 5/5] virtiofsd: prevent races with lo_dirp_put()

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> Introduce lo_dirp_put() so that FUSE_RELEASEDIR does not cause
> use-after-free races with other threads that are accessing lo_dirp.
> 
> Also make lo_releasedir() atomic to prevent FUSE_RELEASEDIR racing with
> itself.  This prevents double-frees.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index ad3abdd532..f74e7d2d21 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1293,11 +1293,28 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
>  }
>  
>  struct lo_dirp {
> +	gint refcount;
>  	DIR *dp;
>  	struct dirent *entry;
>  	off_t offset;
>  };
>  
> +static void lo_dirp_put(struct lo_dirp **dp)
> +{
> +	struct lo_dirp *d = *dp;
> +
> +	if (!d) {
> +		return;
> +	}
> +	*dp = NULL;
> +
> +	if (g_atomic_int_dec_and_test(&d->refcount)) {
> +		closedir(d->dp);
> +		free(d);
> +	}
> +}
> +
> +/* Call lo_dirp_put() on the return value when no longer needed */
>  static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
>  {
>  	struct lo_data *lo = lo_data(req);
> @@ -1305,6 +1322,9 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
>  
>  	pthread_mutex_lock(&lo->mutex);
>  	elem = lo_map_get(&lo->dirp_map, fi->fh);
> +	if (elem) {
> +		g_atomic_int_inc(&elem->dirp->refcount);

I don't understand what protects against reading the elem->dirp
here at the same time it's free'd by lo_releasedir's call to lo_dirp_put

> +	}
>  	pthread_mutex_unlock(&lo->mutex);
>  	if (!elem)
>  		return NULL;
> @@ -1335,6 +1355,8 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
>  	d->offset = 0;
>  	d->entry = NULL;
>  
> +	g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */
> +
>  	fh = lo_add_dirp_mapping(req, d);
>  	if (fh == -1)
>  		goto out_err;
> @@ -1363,7 +1385,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
>  			  off_t offset, struct fuse_file_info *fi, int plus)
>  {
>  	struct lo_data *lo = lo_data(req);
> -	struct lo_dirp *d;
> +	struct lo_dirp *d = NULL;
>  	struct lo_inode *dinode;
>  	char *buf = NULL;
>  	char *p;
> @@ -1451,6 +1473,8 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
>  
>      err = 0;
>  error:
> +    lo_dirp_put(&d);
> +
>      // If there's an error, we can only signal it if we haven't stored
>      // any entries yet - otherwise we'd end up with wrong lookup
>      // counts for the entries that are already in the buffer. So we
> @@ -1477,22 +1501,25 @@ static void lo_readdirplus(fuse_req_t req, fuse_ino_t ino, size_t size,
>  static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  {
>  	struct lo_data *lo = lo_data(req);
> +	struct lo_map_elem *elem;
>  	struct lo_dirp *d;
>  
>  	(void) ino;
>  
> -	d = lo_dirp(req, fi);
> -	if (!d) {
> +	pthread_mutex_lock(&lo->mutex);
> +	elem = lo_map_get(&lo->dirp_map, fi->fh);
> +	if (!elem) {
> +		pthread_mutex_unlock(&lo->mutex);
>  		fuse_reply_err(req, EBADF);
>  		return;
>  	}
>  
> -	pthread_mutex_lock(&lo->mutex);
> +	d = elem->dirp;
>  	lo_map_remove(&lo->dirp_map, fi->fh);
>  	pthread_mutex_unlock(&lo->mutex);
>  
> -	closedir(d->dp);
> -	free(d);
> +	lo_dirp_put(&d); /* paired with lo_opendir() */

Is the &d really what's intended? That's the local stack variable, so
lo_dirp_put will set that local to NULL rather than the elem->dirp wont
it?

Dave

> +
>  	fuse_reply_err(req, 0);
>  }
>  
> @@ -1701,6 +1728,9 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
>  		res = fdatasync(fd);
>  	else
>  		res = fsync(fd);
> +
> +	lo_dirp_put(&d);
> +
>  	fuse_reply_err(req, res == -1 ? errno : 0);
>  }
>  
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 5/5] virtiofsd: prevent races with lo_dirp_put()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4905 bytes --]

On Wed, Jul 31, 2019 at 06:44:52PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > Introduce lo_dirp_put() so that FUSE_RELEASEDIR does not cause
> > use-after-free races with other threads that are accessing lo_dirp.
> > 
> > Also make lo_releasedir() atomic to prevent FUSE_RELEASEDIR racing with
> > itself.  This prevents double-frees.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > index ad3abdd532..f74e7d2d21 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -1293,11 +1293,28 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
> >  }
> >  
> >  struct lo_dirp {
> > +	gint refcount;
> >  	DIR *dp;
> >  	struct dirent *entry;
> >  	off_t offset;
> >  };
> >  
> > +static void lo_dirp_put(struct lo_dirp **dp)
> > +{
> > +	struct lo_dirp *d = *dp;
> > +
> > +	if (!d) {
> > +		return;
> > +	}
> > +	*dp = NULL;
> > +
> > +	if (g_atomic_int_dec_and_test(&d->refcount)) {
> > +		closedir(d->dp);
> > +		free(d);
> > +	}
> > +}
> > +
> > +/* Call lo_dirp_put() on the return value when no longer needed */
> >  static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
> >  {
> >  	struct lo_data *lo = lo_data(req);
> > @@ -1305,6 +1322,9 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
> >  
> >  	pthread_mutex_lock(&lo->mutex);
> >  	elem = lo_map_get(&lo->dirp_map, fi->fh);
> > +	if (elem) {
> > +		g_atomic_int_inc(&elem->dirp->refcount);
> 
> I don't understand what protects against reading the elem->dirp
> here at the same time it's free'd by lo_releasedir's call to lo_dirp_put

It is lo->mutex and not the refcount that prevents the race with
lo_releasedir().  Two cases:

1. lo_releasedir() runs before lo_dirp().  lo_map_get() returns NULL and
   lo_dirp() fails.

2. lo_releasedir() runs after lo_dirp().  lo_map_get() succeeds and the
   lo_dirp() caller keeps the object alive until lo_dirp_put(), when we
   finally free it.

There is no third case since lo->mutex ensures that lo_releasedir() and
lo_dirp() are serialized in the dirp_map lookup.

> > +	}
> >  	pthread_mutex_unlock(&lo->mutex);
> >  	if (!elem)
> >  		return NULL;
> > @@ -1335,6 +1355,8 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
> >  	d->offset = 0;
> >  	d->entry = NULL;
> >  
> > +	g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */
> > +
> >  	fh = lo_add_dirp_mapping(req, d);
> >  	if (fh == -1)
> >  		goto out_err;
> > @@ -1363,7 +1385,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
> >  			  off_t offset, struct fuse_file_info *fi, int plus)
> >  {
> >  	struct lo_data *lo = lo_data(req);
> > -	struct lo_dirp *d;
> > +	struct lo_dirp *d = NULL;
> >  	struct lo_inode *dinode;
> >  	char *buf = NULL;
> >  	char *p;
> > @@ -1451,6 +1473,8 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
> >  
> >      err = 0;
> >  error:
> > +    lo_dirp_put(&d);
> > +
> >      // If there's an error, we can only signal it if we haven't stored
> >      // any entries yet - otherwise we'd end up with wrong lookup
> >      // counts for the entries that are already in the buffer. So we
> > @@ -1477,22 +1501,25 @@ static void lo_readdirplus(fuse_req_t req, fuse_ino_t ino, size_t size,
> >  static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> >  {
> >  	struct lo_data *lo = lo_data(req);
> > +	struct lo_map_elem *elem;
> >  	struct lo_dirp *d;
> >  
> >  	(void) ino;
> >  
> > -	d = lo_dirp(req, fi);
> > -	if (!d) {
> > +	pthread_mutex_lock(&lo->mutex);
> > +	elem = lo_map_get(&lo->dirp_map, fi->fh);
> > +	if (!elem) {
> > +		pthread_mutex_unlock(&lo->mutex);
> >  		fuse_reply_err(req, EBADF);
> >  		return;
> >  	}
> >  
> > -	pthread_mutex_lock(&lo->mutex);
> > +	d = elem->dirp;
> >  	lo_map_remove(&lo->dirp_map, fi->fh);
> >  	pthread_mutex_unlock(&lo->mutex);
> >  
> > -	closedir(d->dp);
> > -	free(d);
> > +	lo_dirp_put(&d); /* paired with lo_opendir() */
> 
> Is the &d really what's intended? That's the local stack variable, so
> lo_dirp_put will set that local to NULL rather than the elem->dirp wont
> it?

Yes, the put(&ptr) pattern prevents user-after-free in the caller.  It's
slightly safer than put(ptr) since that leaves ptr initialized and the
caller might access it later by accident.

elem has already been returned to the freelist by lo_map_remove() and we
must not touch it anymore.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 5/5] virtiofsd: prevent races with lo_dirp_put()

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Jul 31, 2019 at 06:44:52PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > Introduce lo_dirp_put() so that FUSE_RELEASEDIR does not cause
> > > use-after-free races with other threads that are accessing lo_dirp.
> > > 
> > > Also make lo_releasedir() atomic to prevent FUSE_RELEASEDIR racing with
> > > itself.  This prevents double-frees.
> > > 
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  contrib/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++-----
> > >  1 file changed, 36 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> > > index ad3abdd532..f74e7d2d21 100644
> > > --- a/contrib/virtiofsd/passthrough_ll.c
> > > +++ b/contrib/virtiofsd/passthrough_ll.c
> > > @@ -1293,11 +1293,28 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t ino)
> > >  }
> > >  
> > >  struct lo_dirp {
> > > +	gint refcount;
> > >  	DIR *dp;
> > >  	struct dirent *entry;
> > >  	off_t offset;
> > >  };
> > >  
> > > +static void lo_dirp_put(struct lo_dirp **dp)
> > > +{
> > > +	struct lo_dirp *d = *dp;
> > > +
> > > +	if (!d) {
> > > +		return;
> > > +	}
> > > +	*dp = NULL;
> > > +
> > > +	if (g_atomic_int_dec_and_test(&d->refcount)) {
> > > +		closedir(d->dp);
> > > +		free(d);
> > > +	}
> > > +}
> > > +
> > > +/* Call lo_dirp_put() on the return value when no longer needed */
> > >  static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
> > >  {
> > >  	struct lo_data *lo = lo_data(req);
> > > @@ -1305,6 +1322,9 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
> > >  
> > >  	pthread_mutex_lock(&lo->mutex);
> > >  	elem = lo_map_get(&lo->dirp_map, fi->fh);
> > > +	if (elem) {
> > > +		g_atomic_int_inc(&elem->dirp->refcount);
> > 
> > I don't understand what protects against reading the elem->dirp
> > here at the same time it's free'd by lo_releasedir's call to lo_dirp_put
> 
> It is lo->mutex and not the refcount that prevents the race with
> lo_releasedir().  Two cases:
> 
> 1. lo_releasedir() runs before lo_dirp().  lo_map_get() returns NULL and
>    lo_dirp() fails.

Ah that's what I was missing; it's that the lo_releasedir doesn't need
to have completed before lo_dirp runs, it's just that it's lo_map_remove
has happened.

> 2. lo_releasedir() runs after lo_dirp().  lo_map_get() succeeds and the
>    lo_dirp() caller keeps the object alive until lo_dirp_put(), when we
>    finally free it.
> 
> There is no third case since lo->mutex ensures that lo_releasedir() and
> lo_dirp() are serialized in the dirp_map lookup.

> > > +	}
> > >  	pthread_mutex_unlock(&lo->mutex);
> > >  	if (!elem)
> > >  		return NULL;
> > > @@ -1335,6 +1355,8 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi
> > >  	d->offset = 0;
> > >  	d->entry = NULL;
> > >  
> > > +	g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */
> > > +
> > >  	fh = lo_add_dirp_mapping(req, d);
> > >  	if (fh == -1)
> > >  		goto out_err;
> > > @@ -1363,7 +1385,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
> > >  			  off_t offset, struct fuse_file_info *fi, int plus)
> > >  {
> > >  	struct lo_data *lo = lo_data(req);
> > > -	struct lo_dirp *d;
> > > +	struct lo_dirp *d = NULL;
> > >  	struct lo_inode *dinode;
> > >  	char *buf = NULL;
> > >  	char *p;
> > > @@ -1451,6 +1473,8 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
> > >  
> > >      err = 0;
> > >  error:
> > > +    lo_dirp_put(&d);
> > > +
> > >      // If there's an error, we can only signal it if we haven't stored
> > >      // any entries yet - otherwise we'd end up with wrong lookup
> > >      // counts for the entries that are already in the buffer. So we
> > > @@ -1477,22 +1501,25 @@ static void lo_readdirplus(fuse_req_t req, fuse_ino_t ino, size_t size,
> > >  static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > >  {
> > >  	struct lo_data *lo = lo_data(req);
> > > +	struct lo_map_elem *elem;
> > >  	struct lo_dirp *d;
> > >  
> > >  	(void) ino;
> > >  
> > > -	d = lo_dirp(req, fi);
> > > -	if (!d) {
> > > +	pthread_mutex_lock(&lo->mutex);
> > > +	elem = lo_map_get(&lo->dirp_map, fi->fh);
> > > +	if (!elem) {
> > > +		pthread_mutex_unlock(&lo->mutex);
> > >  		fuse_reply_err(req, EBADF);
> > >  		return;
> > >  	}
> > >  
> > > -	pthread_mutex_lock(&lo->mutex);
> > > +	d = elem->dirp;
> > >  	lo_map_remove(&lo->dirp_map, fi->fh);
> > >  	pthread_mutex_unlock(&lo->mutex);
> > >  
> > > -	closedir(d->dp);
> > > -	free(d);
> > > +	lo_dirp_put(&d); /* paired with lo_opendir() */
> > 
> > Is the &d really what's intended? That's the local stack variable, so
> > lo_dirp_put will set that local to NULL rather than the elem->dirp wont
> > it?
> 
> Yes, the put(&ptr) pattern prevents user-after-free in the caller.  It's
> slightly safer than put(ptr) since that leaves ptr initialized and the
> caller might access it later by accident.
> 
> elem has already been returned to the freelist by lo_map_remove() and we
> must not touch it anymore.

OK, thanks.

> Stefan


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK