* [Qemu-devel] [PATCH 0/5] virtiofsd: multithreading preparation part 2 @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi This is the second in a multi-series effort to make virtiofsd thread-safe. The main goal in this installment is to make lo_inode thread-safe, but other fixes are included too. Like any good author I will build suspense and won't tell where this story is headed, but I still have some more code auditing to do before we can declare virtiofsd thread-safe :). Based-on: <20190726091103.23503-1-stefanha@redhat.com> ("virtiofsd: multithreading preparation") Stefan Hajnoczi (5): virtiofsd: take lo->mutex around lo_add_fd_mapping() virtiofsd: take lo->mutex around lo_add_dirp_mapping() virtiofsd: rename inode->refcount to inode->nlookup virtiofsd: fix inode nlookup leaks virtiofsd: introduce inode refcount to prevent use-after-free contrib/virtiofsd/passthrough_ll.c | 262 +++++++++++++++++++++++------ 1 file changed, 214 insertions(+), 48 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 0/5] virtiofsd: multithreading preparation part 2 @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel This is the second in a multi-series effort to make virtiofsd thread-safe. The main goal in this installment is to make lo_inode thread-safe, but other fixes are included too. Like any good author I will build suspense and won't tell where this story is headed, but I still have some more code auditing to do before we can declare virtiofsd thread-safe :). Based-on: <20190726091103.23503-1-stefanha@redhat.com> ("virtiofsd: multithreading preparation") Stefan Hajnoczi (5): virtiofsd: take lo->mutex around lo_add_fd_mapping() virtiofsd: take lo->mutex around lo_add_dirp_mapping() virtiofsd: rename inode->refcount to inode->nlookup virtiofsd: fix inode nlookup leaks virtiofsd: introduce inode refcount to prevent use-after-free contrib/virtiofsd/passthrough_ll.c | 262 +++++++++++++++++++++++------ 1 file changed, 214 insertions(+), 48 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 16:10 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi The lo_add_fd_mapping() function assumes lo->mutex is held, so we should acquire it. 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 65b7352c95..417a104218 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -1555,7 +1555,9 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, update_version(lo, lo_inode(req, parent)); + pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); fuse_reply_err(req, ENOMEM); @@ -1760,7 +1762,9 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) if (fd == -1) return (void) fuse_reply_err(req, errno); + pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); fuse_reply_err(req, ENOMEM); -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel The lo_add_fd_mapping() function assumes lo->mutex is held, so we should acquire it. 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 65b7352c95..417a104218 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -1555,7 +1555,9 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, update_version(lo, lo_inode(req, parent)); + pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); fuse_reply_err(req, ENOMEM); @@ -1760,7 +1762,9 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) if (fd == -1) return (void) fuse_reply_err(req, errno); + pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); fuse_reply_err(req, ENOMEM); -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 18:45 ` Dr. David Alan Gilbert -1 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-07-31 18:45 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Liu Bo, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > The lo_add_fd_mapping() function assumes lo->mutex is held, so we should > acquire it. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Thanks, applied Would it make sense for me to squash this into: virtiofsd: passthrough_ll: add fd_map to hide file descriptors ? Dave > --- > 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 65b7352c95..417a104218 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1555,7 +1555,9 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > > update_version(lo, lo_inode(req, parent)); > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_fd_mapping(req, fd); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) { > close(fd); > fuse_reply_err(req, ENOMEM); > @@ -1760,7 +1762,9 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) > if (fd == -1) > return (void) fuse_reply_err(req, errno); > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_fd_mapping(req, fd); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) { > close(fd); > fuse_reply_err(req, ENOMEM); > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Virtio-fs] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() @ 2019-07-31 18:45 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-07-31 18:45 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > The lo_add_fd_mapping() function assumes lo->mutex is held, so we should > acquire it. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Thanks, applied Would it make sense for me to squash this into: virtiofsd: passthrough_ll: add fd_map to hide file descriptors ? Dave > --- > 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 65b7352c95..417a104218 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1555,7 +1555,9 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > > update_version(lo, lo_inode(req, parent)); > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_fd_mapping(req, fd); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) { > close(fd); > fuse_reply_err(req, ENOMEM); > @@ -1760,7 +1762,9 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) > if (fd == -1) > return (void) fuse_reply_err(req, errno); > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_fd_mapping(req, fd); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) { > close(fd); > fuse_reply_err(req, ENOMEM); > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() 2019-07-31 18:45 ` [Virtio-fs] " Dr. David Alan Gilbert @ 2019-08-01 9:17 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-08-01 9:17 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs, Liu Bo, qemu-devel [-- Attachment #1: Type: text/plain, Size: 691 bytes --] On Wed, Jul 31, 2019 at 07:45:38PM +0100, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefanha@redhat.com) wrote: > > The lo_add_fd_mapping() function assumes lo->mutex is held, so we should > > acquire it. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Thanks, applied > > Would it make sense for me to squash this into: > virtiofsd: passthrough_ll: add fd_map to hide file descriptors > > ? Yes, please. Feel free to squash any of my virtio-fs patches as you see fit in order to produce a nice clean patch series for upstream. I don't mind if fine-grained authorship information is lost, but please keep my Signed-off-by. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Virtio-fs] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() @ 2019-08-01 9:17 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-08-01 9:17 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel [-- Attachment #1: Type: text/plain, Size: 691 bytes --] On Wed, Jul 31, 2019 at 07:45:38PM +0100, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefanha@redhat.com) wrote: > > The lo_add_fd_mapping() function assumes lo->mutex is held, so we should > > acquire it. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Thanks, applied > > Would it make sense for me to squash this into: > virtiofsd: passthrough_ll: add fd_map to hide file descriptors > > ? Yes, please. Feel free to squash any of my virtio-fs patches as you see fit in order to produce a nice clean patch series for upstream. I don't mind if fine-grained authorship information is lost, but please keep my Signed-off-by. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 2/5] virtiofsd: take lo->mutex around lo_add_dirp_mapping() 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 16:10 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi The lo_add_dirp_mapping() function assumes lo->mutex is held, so we should acquire it. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 417a104218..e1d729d26b 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -1357,7 +1357,9 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */ + pthread_mutex_lock(&lo->mutex); fh = lo_add_dirp_mapping(req, d); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) goto out_err; -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 2/5] virtiofsd: take lo->mutex around lo_add_dirp_mapping() @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel The lo_add_dirp_mapping() function assumes lo->mutex is held, so we should acquire it. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 417a104218..e1d729d26b 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -1357,7 +1357,9 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */ + pthread_mutex_lock(&lo->mutex); fh = lo_add_dirp_mapping(req, d); + pthread_mutex_unlock(&lo->mutex); if (fh == -1) goto out_err; -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] virtiofsd: take lo->mutex around lo_add_dirp_mapping() 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-08-01 11:45 ` Dr. David Alan Gilbert -1 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-01 11:45 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Liu Bo, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > The lo_add_dirp_mapping() function assumes lo->mutex is held, so we > should acquire it. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Squashed into: passthrough_ll: add dirp_map to hide lo_dirp pointers > --- > contrib/virtiofsd/passthrough_ll.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index 417a104218..e1d729d26b 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1357,7 +1357,9 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi > > g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */ > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_dirp_mapping(req, d); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) > goto out_err; > > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Virtio-fs] [PATCH 2/5] virtiofsd: take lo->mutex around lo_add_dirp_mapping() @ 2019-08-01 11:45 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-01 11:45 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > The lo_add_dirp_mapping() function assumes lo->mutex is held, so we > should acquire it. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Squashed into: passthrough_ll: add dirp_map to hide lo_dirp pointers > --- > contrib/virtiofsd/passthrough_ll.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index 417a104218..e1d729d26b 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -1357,7 +1357,9 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi > > g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */ > > + pthread_mutex_lock(&lo->mutex); > fh = lo_add_dirp_mapping(req, d); > + pthread_mutex_unlock(&lo->mutex); > if (fh == -1) > goto out_err; > > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 3/5] virtiofsd: rename inode->refcount to inode->nlookup 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 16:10 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi This reference counter plays a specific role in the FUSE protocol. It's not a generic object reference counter and the FUSE kernel code calls it "nlookup". Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index e1d729d26b..135123366a 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -97,7 +97,19 @@ struct lo_inode { int fd; bool is_symlink; struct lo_key key; - uint64_t refcount; /* protected by lo->mutex */ + + /* This counter keeps the inode alive during the FUSE session. + * Incremented when the FUSE inode number is sent in a reply + * (FUSE_LOOKUP, FUSE_READDIRPLUS, etc). Decremented when an inode is + * released by requests like FUSE_FORGET, FUSE_RMDIR, FUSE_RENAME, etc. + * + * Note that this value is untrusted because the client can manipulate + * it arbitrarily using FUSE_FORGET requests. + * + * Protected by lo->mutex. + */ + uint64_t nlookup; + uint64_t version_offset; uint64_t ireg_refid; fuse_ino_t fuse_ino; @@ -485,7 +497,7 @@ retry: if (last == path) { p = &lo->root; pthread_mutex_lock(&lo->mutex); - p->refcount++; + p->nlookup++; pthread_mutex_unlock(&lo->mutex); } else { *last = '\0'; @@ -688,8 +700,8 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) pthread_mutex_lock(&lo->mutex); p = g_hash_table_lookup(lo->inodes, &key); if (p) { - assert(p->refcount > 0); - p->refcount++; + assert(p->nlookup > 0); + p->nlookup++; } pthread_mutex_unlock(&lo->mutex); @@ -797,7 +809,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, goto out_err; inode->is_symlink = S_ISLNK(e->attr.st_mode); - inode->refcount = 1; + inode->nlookup = 1; inode->fd = newfd; newfd = -1; inode->key.ino = e->attr.st_ino; @@ -1051,7 +1063,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, goto out_err; pthread_mutex_lock(&lo->mutex); - inode->refcount++; + inode->nlookup++; pthread_mutex_unlock(&lo->mutex); e.ino = inode->fuse_ino; update_version(lo, inode); @@ -1200,9 +1212,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) return; pthread_mutex_lock(&lo->mutex); - assert(inode->refcount >= n); - inode->refcount -= n; - if (!inode->refcount) { + assert(inode->nlookup >= n); + inode->nlookup -= n; + if (!inode->nlookup) { lo_map_remove(&lo->ino_map, inode->fuse_ino); g_hash_table_remove(lo->inodes, &inode->key); if (g_hash_table_size(inode->posix_locks)) { @@ -1225,7 +1237,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, struct lo_inode *inode = value; struct lo_data *lo = user_data; - inode->refcount = 0; + inode->nlookup = 0; lo_map_remove(&lo->ino_map, inode->fuse_ino); close(inode->fd); @@ -1252,7 +1264,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) if (lo_debug(req)) { fuse_debug(" forget %lli %lli -%lli\n", (unsigned long long) ino, - (unsigned long long) inode->refcount, + (unsigned long long) inode->nlookup, (unsigned long long) nlookup); } @@ -2581,7 +2593,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) root->fd = fd; root->key.ino = stat.st_ino; root->key.dev = stat.st_dev; - root->refcount = 2; + root->nlookup = 2; } static void setup_proc_self_fd(struct lo_data *lo) -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 3/5] virtiofsd: rename inode->refcount to inode->nlookup @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel This reference counter plays a specific role in the FUSE protocol. It's not a generic object reference counter and the FUSE kernel code calls it "nlookup". Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index e1d729d26b..135123366a 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -97,7 +97,19 @@ struct lo_inode { int fd; bool is_symlink; struct lo_key key; - uint64_t refcount; /* protected by lo->mutex */ + + /* This counter keeps the inode alive during the FUSE session. + * Incremented when the FUSE inode number is sent in a reply + * (FUSE_LOOKUP, FUSE_READDIRPLUS, etc). Decremented when an inode is + * released by requests like FUSE_FORGET, FUSE_RMDIR, FUSE_RENAME, etc. + * + * Note that this value is untrusted because the client can manipulate + * it arbitrarily using FUSE_FORGET requests. + * + * Protected by lo->mutex. + */ + uint64_t nlookup; + uint64_t version_offset; uint64_t ireg_refid; fuse_ino_t fuse_ino; @@ -485,7 +497,7 @@ retry: if (last == path) { p = &lo->root; pthread_mutex_lock(&lo->mutex); - p->refcount++; + p->nlookup++; pthread_mutex_unlock(&lo->mutex); } else { *last = '\0'; @@ -688,8 +700,8 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) pthread_mutex_lock(&lo->mutex); p = g_hash_table_lookup(lo->inodes, &key); if (p) { - assert(p->refcount > 0); - p->refcount++; + assert(p->nlookup > 0); + p->nlookup++; } pthread_mutex_unlock(&lo->mutex); @@ -797,7 +809,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, goto out_err; inode->is_symlink = S_ISLNK(e->attr.st_mode); - inode->refcount = 1; + inode->nlookup = 1; inode->fd = newfd; newfd = -1; inode->key.ino = e->attr.st_ino; @@ -1051,7 +1063,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, goto out_err; pthread_mutex_lock(&lo->mutex); - inode->refcount++; + inode->nlookup++; pthread_mutex_unlock(&lo->mutex); e.ino = inode->fuse_ino; update_version(lo, inode); @@ -1200,9 +1212,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) return; pthread_mutex_lock(&lo->mutex); - assert(inode->refcount >= n); - inode->refcount -= n; - if (!inode->refcount) { + assert(inode->nlookup >= n); + inode->nlookup -= n; + if (!inode->nlookup) { lo_map_remove(&lo->ino_map, inode->fuse_ino); g_hash_table_remove(lo->inodes, &inode->key); if (g_hash_table_size(inode->posix_locks)) { @@ -1225,7 +1237,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, struct lo_inode *inode = value; struct lo_data *lo = user_data; - inode->refcount = 0; + inode->nlookup = 0; lo_map_remove(&lo->ino_map, inode->fuse_ino); close(inode->fd); @@ -1252,7 +1264,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) if (lo_debug(req)) { fuse_debug(" forget %lli %lli -%lli\n", (unsigned long long) ino, - (unsigned long long) inode->refcount, + (unsigned long long) inode->nlookup, (unsigned long long) nlookup); } @@ -2581,7 +2593,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) root->fd = fd; root->key.ino = stat.st_ino; root->key.dev = stat.st_dev; - root->refcount = 2; + root->nlookup = 2; } static void setup_proc_self_fd(struct lo_data *lo) -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] virtiofsd: rename inode->refcount to inode->nlookup 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-08-01 12:00 ` Dr. David Alan Gilbert -1 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-01 12:00 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Liu Bo, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > This reference counter plays a specific role in the FUSE protocol. It's > not a generic object reference counter and the FUSE kernel code calls it > "nlookup". > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index e1d729d26b..135123366a 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -97,7 +97,19 @@ struct lo_inode { > int fd; > bool is_symlink; > struct lo_key key; > - uint64_t refcount; /* protected by lo->mutex */ > + > + /* This counter keeps the inode alive during the FUSE session. > + * Incremented when the FUSE inode number is sent in a reply > + * (FUSE_LOOKUP, FUSE_READDIRPLUS, etc). Decremented when an inode is > + * released by requests like FUSE_FORGET, FUSE_RMDIR, FUSE_RENAME, etc. > + * > + * Note that this value is untrusted because the client can manipulate > + * it arbitrarily using FUSE_FORGET requests. > + * > + * Protected by lo->mutex. > + */ > + uint64_t nlookup; > + > uint64_t version_offset; > uint64_t ireg_refid; > fuse_ino_t fuse_ino; > @@ -485,7 +497,7 @@ retry: > if (last == path) { > p = &lo->root; > pthread_mutex_lock(&lo->mutex); > - p->refcount++; > + p->nlookup++; > pthread_mutex_unlock(&lo->mutex); > } else { > *last = '\0'; > @@ -688,8 +700,8 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) > pthread_mutex_lock(&lo->mutex); > p = g_hash_table_lookup(lo->inodes, &key); > if (p) { > - assert(p->refcount > 0); > - p->refcount++; > + assert(p->nlookup > 0); > + p->nlookup++; > } > pthread_mutex_unlock(&lo->mutex); > > @@ -797,7 +809,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > > inode->is_symlink = S_ISLNK(e->attr.st_mode); > - inode->refcount = 1; > + inode->nlookup = 1; > inode->fd = newfd; > newfd = -1; > inode->key.ino = e->attr.st_ino; > @@ -1051,7 +1063,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, > goto out_err; > > pthread_mutex_lock(&lo->mutex); > - inode->refcount++; > + inode->nlookup++; > pthread_mutex_unlock(&lo->mutex); > e.ino = inode->fuse_ino; > update_version(lo, inode); > @@ -1200,9 +1212,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) > return; > > pthread_mutex_lock(&lo->mutex); > - assert(inode->refcount >= n); > - inode->refcount -= n; > - if (!inode->refcount) { > + assert(inode->nlookup >= n); > + inode->nlookup -= n; > + if (!inode->nlookup) { > lo_map_remove(&lo->ino_map, inode->fuse_ino); > g_hash_table_remove(lo->inodes, &inode->key); > if (g_hash_table_size(inode->posix_locks)) { > @@ -1225,7 +1237,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, > struct lo_inode *inode = value; > struct lo_data *lo = user_data; > > - inode->refcount = 0; > + inode->nlookup = 0; > lo_map_remove(&lo->ino_map, inode->fuse_ino); > close(inode->fd); > > @@ -1252,7 +1264,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) > if (lo_debug(req)) { > fuse_debug(" forget %lli %lli -%lli\n", > (unsigned long long) ino, > - (unsigned long long) inode->refcount, > + (unsigned long long) inode->nlookup, > (unsigned long long) nlookup); > } > > @@ -2581,7 +2593,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) > root->fd = fd; > root->key.ino = stat.st_ino; > root->key.dev = stat.st_dev; > - root->refcount = 2; > + root->nlookup = 2; > } > > static void setup_proc_self_fd(struct lo_data *lo) > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Virtio-fs] [PATCH 3/5] virtiofsd: rename inode->refcount to inode->nlookup @ 2019-08-01 12:00 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 20+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-01 12:00 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, qemu-devel * Stefan Hajnoczi (stefanha@redhat.com) wrote: > This reference counter plays a specific role in the FUSE protocol. It's > not a generic object reference counter and the FUSE kernel code calls it > "nlookup". > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > contrib/virtiofsd/passthrough_ll.c | 36 ++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c > index e1d729d26b..135123366a 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -97,7 +97,19 @@ struct lo_inode { > int fd; > bool is_symlink; > struct lo_key key; > - uint64_t refcount; /* protected by lo->mutex */ > + > + /* This counter keeps the inode alive during the FUSE session. > + * Incremented when the FUSE inode number is sent in a reply > + * (FUSE_LOOKUP, FUSE_READDIRPLUS, etc). Decremented when an inode is > + * released by requests like FUSE_FORGET, FUSE_RMDIR, FUSE_RENAME, etc. > + * > + * Note that this value is untrusted because the client can manipulate > + * it arbitrarily using FUSE_FORGET requests. > + * > + * Protected by lo->mutex. > + */ > + uint64_t nlookup; > + > uint64_t version_offset; > uint64_t ireg_refid; > fuse_ino_t fuse_ino; > @@ -485,7 +497,7 @@ retry: > if (last == path) { > p = &lo->root; > pthread_mutex_lock(&lo->mutex); > - p->refcount++; > + p->nlookup++; > pthread_mutex_unlock(&lo->mutex); > } else { > *last = '\0'; > @@ -688,8 +700,8 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) > pthread_mutex_lock(&lo->mutex); > p = g_hash_table_lookup(lo->inodes, &key); > if (p) { > - assert(p->refcount > 0); > - p->refcount++; > + assert(p->nlookup > 0); > + p->nlookup++; > } > pthread_mutex_unlock(&lo->mutex); > > @@ -797,7 +809,7 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > goto out_err; > > inode->is_symlink = S_ISLNK(e->attr.st_mode); > - inode->refcount = 1; > + inode->nlookup = 1; > inode->fd = newfd; > newfd = -1; > inode->key.ino = e->attr.st_ino; > @@ -1051,7 +1063,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, > goto out_err; > > pthread_mutex_lock(&lo->mutex); > - inode->refcount++; > + inode->nlookup++; > pthread_mutex_unlock(&lo->mutex); > e.ino = inode->fuse_ino; > update_version(lo, inode); > @@ -1200,9 +1212,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) > return; > > pthread_mutex_lock(&lo->mutex); > - assert(inode->refcount >= n); > - inode->refcount -= n; > - if (!inode->refcount) { > + assert(inode->nlookup >= n); > + inode->nlookup -= n; > + if (!inode->nlookup) { > lo_map_remove(&lo->ino_map, inode->fuse_ino); > g_hash_table_remove(lo->inodes, &inode->key); > if (g_hash_table_size(inode->posix_locks)) { > @@ -1225,7 +1237,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, > struct lo_inode *inode = value; > struct lo_data *lo = user_data; > > - inode->refcount = 0; > + inode->nlookup = 0; > lo_map_remove(&lo->ino_map, inode->fuse_ino); > close(inode->fd); > > @@ -1252,7 +1264,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) > if (lo_debug(req)) { > fuse_debug(" forget %lli %lli -%lli\n", > (unsigned long long) ino, > - (unsigned long long) inode->refcount, > + (unsigned long long) inode->nlookup, > (unsigned long long) nlookup); > } > > @@ -2581,7 +2593,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) > root->fd = fd; > root->key.ino = stat.st_ino; > root->key.dev = stat.st_dev; > - root->refcount = 2; > + root->nlookup = 2; > } > > static void setup_proc_self_fd(struct lo_data *lo) > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 4/5] virtiofsd: fix inode nlookup leaks 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 16:10 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi utimensat_empty() and linkat_empty_nofollow() must unref the parent directory inode that was obtained from lo_parent_and_name(). Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 135123366a..125e9d9f96 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -567,8 +567,10 @@ static int utimensat_empty(struct lo_data *lo, struct lo_inode *inode, fallback: res = lo_parent_and_name(lo, inode, path, &parent); - if (res != -1) + if (res != -1) { res = utimensat(parent->fd, path, tv, AT_SYMLINK_NOFOLLOW); + unref_inode(lo, parent, 1); + } return res; } @@ -1024,8 +1026,10 @@ static int linkat_empty_nofollow(struct lo_data *lo, struct lo_inode *inode, fallback: res = lo_parent_and_name(lo, inode, path, &parent); - if (res != -1) + if (res != -1) { res = linkat(parent->fd, path, dfd, name, 0); + unref_inode(lo, parent, 1); + } return res; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 4/5] virtiofsd: fix inode nlookup leaks @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel utimensat_empty() and linkat_empty_nofollow() must unref the parent directory inode that was obtained from lo_parent_and_name(). Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 135123366a..125e9d9f96 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -567,8 +567,10 @@ static int utimensat_empty(struct lo_data *lo, struct lo_inode *inode, fallback: res = lo_parent_and_name(lo, inode, path, &parent); - if (res != -1) + if (res != -1) { res = utimensat(parent->fd, path, tv, AT_SYMLINK_NOFOLLOW); + unref_inode(lo, parent, 1); + } return res; } @@ -1024,8 +1026,10 @@ static int linkat_empty_nofollow(struct lo_data *lo, struct lo_inode *inode, fallback: res = lo_parent_and_name(lo, inode, path, &parent); - if (res != -1) + if (res != -1) { res = linkat(parent->fd, path, dfd, name, 0); + unref_inode(lo, parent, 1); + } return res; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 5/5] virtiofsd: introduce inode refcount to prevent use-after-free 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi @ 2019-07-31 16:10 ` Stefan Hajnoczi -1 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel; +Cc: Liu Bo, Dr. David Alan Gilbert, Stefan Hajnoczi If thread A is using an inode it must not be deleted by thread B when processing a FUSE_FORGET request. The FUSE protocol itself already has a counter called nlookup that is used in FUSE_FORGET messages. We cannot trust this counter since the untrusted client can manipulate it via FUSE_FORGET messages. Introduce a new refcount to keep inodes alive for the required lifespan. lo_inode_put() must be called to release a reference. FUSE's nlookup counter holds exactly one reference so that the inode stays alive as long as the client still wants to remember it. Note that the lo_inode->is_symlink field is moved to avoid creating a hole in the struct due to struct field alignment. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 212 ++++++++++++++++++++++++----- 1 file changed, 178 insertions(+), 34 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 125e9d9f96..0c90e352d2 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -95,7 +95,12 @@ struct lo_key { struct lo_inode { int fd; - bool is_symlink; + + /* Atomic reference count for this object. The nlookup field holds a + * reference and release it when nlookup reaches 0. + */ + gint refcount; + struct lo_key key; /* This counter keeps the inode alive during the FUSE session. @@ -115,6 +120,8 @@ struct lo_inode { fuse_ino_t fuse_ino; pthread_mutex_t plock_mutex; GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */ + + bool is_symlink; }; struct lo_cred { @@ -198,6 +205,7 @@ static const struct fuse_opt lo_opts[] = { FUSE_OPT_END }; static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n); +static void put_shared(struct lo_data *lo, struct lo_inode *inode); static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st); @@ -359,6 +367,24 @@ static ssize_t lo_add_inode_mapping(fuse_req_t req, struct lo_inode *inode) return elem - lo_data(req)->ino_map.elems; } +static void lo_inode_put(struct lo_data *lo, struct lo_inode **inodep) +{ + struct lo_inode *inode = *inodep; + + if (!inode) { + return; + } + + *inodep = NULL; + + if (g_atomic_int_dec_and_test(&inode->refcount)) { + close(inode->fd); + put_shared(lo, inode); + free(inode); + } +} + +/* Caller must release refcount using lo_inode_put() */ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) { struct lo_data *lo = lo_data(req); @@ -366,6 +392,9 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) pthread_mutex_lock(&lo->mutex); elem = lo_map_get(&lo->ino_map, ino); + if (elem) { + g_atomic_int_inc(&elem->inode->refcount); + } pthread_mutex_unlock(&lo->mutex); if (!elem) @@ -374,10 +403,22 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) return elem->inode; } +/* TODO Remove this helper and force callers to hold an inode refcount until + * they are done with the fd. This will be done in a later patch to make + * review easier. + */ static int lo_fd(fuse_req_t req, fuse_ino_t ino) { struct lo_inode *inode = lo_inode(req, ino); - return inode ? inode->fd : -1; + int fd; + + if (!inode) { + return -1; + } + + fd = inode->fd; + lo_inode_put(lo_data(req), &inode); + return fd; } static bool lo_debug(fuse_req_t req) @@ -463,6 +504,9 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino, fuse_reply_attr(req, &buf, lo->timeout); } +/* Increments parent->nlookup and caller must release refcount using + * lo_inode_put(&parent). + */ static int lo_parent_and_name(struct lo_data *lo, struct lo_inode *inode, char path[PATH_MAX], struct lo_inode **parent) { @@ -498,6 +542,7 @@ retry: p = &lo->root; pthread_mutex_lock(&lo->mutex); p->nlookup++; + g_atomic_int_inc(&p->refcount); pthread_mutex_unlock(&lo->mutex); } else { *last = '\0'; @@ -570,6 +615,7 @@ fallback: if (res != -1) { res = utimensat(parent->fd, path, tv, AT_SYMLINK_NOFOLLOW); unref_inode(lo, parent, 1); + lo_inode_put(lo, &parent); } return res; @@ -683,11 +729,13 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr, goto out_err; } update_version(lo, inode); + lo_inode_put(lo, &inode); return lo_getattr(req, ino, fi); out_err: saverr = errno; + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -704,6 +752,7 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) if (p) { assert(p->nlookup > 0); p->nlookup++; + g_atomic_int_inc(&p->refcount); } pthread_mutex_unlock(&lo->mutex); @@ -771,6 +820,9 @@ static void put_shared(struct lo_data *lo, struct lo_inode *inode) } } +/* Increments nlookup and caller must release refcount using + * lo_inode_put(&parent). + */ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, struct fuse_entry_param *e) { @@ -778,7 +830,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, int res; int saverr; struct lo_data *lo = lo_data(req); - struct lo_inode *inode, *dir = lo_inode(req, parent); + struct lo_inode *inode = NULL; + struct lo_inode *dir = lo_inode(req, parent); if (!dir) { return EBADF; @@ -811,6 +864,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, goto out_err; inode->is_symlink = S_ISLNK(e->attr.st_mode); + + /* One for the caller and one for nlookup (released in unref_inode()) */ + g_atomic_int_set(&inode->refcount, 2); + inode->nlookup = 1; inode->fd = newfd; newfd = -1; @@ -839,6 +896,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, e->ino = inode->fuse_ino; e->version_offset = inode->version_offset; + lo_inode_put(lo, &inode); + lo_inode_put(lo, &dir); if (lo_debug(req)) fuse_debug(" %lli/%s -> %lli (version_table[%lli]=%lli)\n", @@ -853,6 +912,8 @@ out_err: saverr = errno; if (newfd != -1) close(newfd); + lo_inode_put(lo, &inode); + lo_inode_put(lo, &dir); return saverr; } @@ -963,7 +1024,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, if (res == -1) goto out; - update_version(lo, lo_inode(req, parent)); + update_version(lo, dir); saverr = lo_do_lookup(req, parent, name, &e); if (saverr) @@ -975,9 +1036,11 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, (unsigned long long) e.ino); fuse_reply_entry(req, &e, lo->shared); + lo_inode_put(lo, &dir); return; out: + lo_inode_put(lo, &dir); if (newfd != -1) close(newfd); fuse_reply_err(req, saverr); @@ -1029,6 +1092,7 @@ fallback: if (res != -1) { res = linkat(parent->fd, path, dfd, name, 0); unref_inode(lo, parent, 1); + lo_inode_put(lo, &parent); } return res; @@ -1039,6 +1103,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, { int res; struct lo_data *lo = lo_data(req); + struct lo_inode *parent_inode; struct lo_inode *inode; struct fuse_entry_param e; int saverr; @@ -1048,17 +1113,18 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, return; } + parent_inode = lo_inode(req, parent); inode = lo_inode(req, ino); - if (!inode) { - fuse_reply_err(req, EBADF); - return; + if (!parent_inode || !inode) { + errno = EBADF; + goto out_err; } memset(&e, 0, sizeof(struct fuse_entry_param)); e.attr_timeout = lo->timeout; e.entry_timeout = lo->timeout; - res = linkat_empty_nofollow(lo, inode, lo_fd(req, parent), name); + res = linkat_empty_nofollow(lo, inode, parent_inode->fd, name); if (res == -1) goto out_err; @@ -1071,7 +1137,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, pthread_mutex_unlock(&lo->mutex); e.ino = inode->fuse_ino; update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + update_version(lo, parent_inode); if (lo_debug(req)) fuse_debug(" %lli/%s -> %lli\n", @@ -1079,13 +1145,18 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, (unsigned long long) e.ino); fuse_reply_entry(req, &e, lo->shared); + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &inode); return; out_err: saverr = errno; + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } +/* Increments nlookup and caller must release refcount using lo_inode_put() */ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent, const char *name) { @@ -1121,11 +1192,20 @@ static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name) if (res == -1) { fuse_reply_err(req, errno); } else { + struct lo_inode *parent_inode; + update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + + parent_inode = lo_inode(req, parent); + if (parent_inode) { + update_version(lo, parent_inode); + lo_inode_put(lo, &parent_inode); + } + fuse_reply_err(req, 0); } unref_inode(lo, inode, 1); + lo_inode_put(lo, &inode); } static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, @@ -1133,8 +1213,10 @@ 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_inode *parent_inode; + struct lo_inode *newparent_inode; + struct lo_inode *oldinode = NULL; + struct lo_inode *newinode = NULL; struct lo_data *lo = lo_data(req); if (!is_safe_path_component(name) || @@ -1143,6 +1225,13 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, return; } + parent_inode = lo_inode(req, parent); + newparent_inode = lo_inode(req, newparent); + if (!parent_inode || !newparent_inode) { + fuse_reply_err(req, EBADF); + goto out; + } + oldinode = lookup_name(req, parent, name); newinode = lookup_name(req, newparent, newname); if (!oldinode) { @@ -1155,8 +1244,8 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, #ifndef SYS_renameat2 fuse_reply_err(req, EINVAL); #else - res = syscall(SYS_renameat2, lo_fd(req, parent), name, - lo_fd(req, newparent), newname, flags); + res = syscall(SYS_renameat2, parent_inode->fd, name, + newparent_inode->fd, newname, flags); if (res == -1 && errno == ENOSYS) fuse_reply_err(req, EINVAL); else @@ -1165,21 +1254,24 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, goto out; } - res = renameat(lo_fd(req, parent), name, - lo_fd(req, newparent), newname); + res = renameat(parent_inode->fd, name, newparent_inode->fd, newname); if (res == -1) { fuse_reply_err(req, errno); } else { update_version(lo, oldinode); if (newinode) update_version(lo, newinode); - update_version(lo, lo_inode(req, parent)); - update_version(lo, lo_inode(req, newparent)); + update_version(lo, parent_inode); + update_version(lo, newparent_inode); fuse_reply_err(req, 0); } out: unref_inode(lo, oldinode, 1); unref_inode(lo, newinode, 1); + lo_inode_put(lo, &oldinode); + lo_inode_put(lo, &newinode); + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &newparent_inode); } static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) @@ -1203,11 +1295,20 @@ static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) if (res == -1) { fuse_reply_err(req, errno); } else { + struct lo_inode *parent_inode; + update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + + parent_inode = lo_inode(req, parent); + if (parent_inode) { + update_version(lo, parent_inode); + lo_inode_put(lo, &parent_inode); + } + fuse_reply_err(req, 0); } unref_inode(lo, inode, 1); + lo_inode_put(lo, &inode); } static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) @@ -1227,9 +1328,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) g_hash_table_destroy(inode->posix_locks); pthread_mutex_destroy(&inode->plock_mutex); pthread_mutex_unlock(&lo->mutex); - close(inode->fd); - put_shared(lo, inode); - free(inode); + + /* Drop our refcount from lo_do_lookup() */ + lo_inode_put(lo, &inode); } else { pthread_mutex_unlock(&lo->mutex); } @@ -1244,6 +1345,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, inode->nlookup = 0; lo_map_remove(&lo->ino_map, inode->fuse_ino); close(inode->fd); + lo_inode_put(lo, &inode); /* Drop our refcount from lo_do_lookup() */ return TRUE; } @@ -1273,6 +1375,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) } unref_inode(lo, inode, nlookup); + lo_inode_put(lo, &inode); } static void lo_forget(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) @@ -1492,6 +1595,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, err = 0; error: lo_dirp_put(&d); + lo_inode_put(lo, &dinode); // 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 @@ -1546,6 +1650,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, { int fd; struct lo_data *lo = lo_data(req); + struct lo_inode *parent_inode; struct fuse_entry_param e; int err; struct lo_cred old = {}; @@ -1559,11 +1664,17 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, return; } + parent_inode = lo_inode(req, parent); + if (!parent_inode) { + fuse_reply_err(req, EBADF); + return; + } + err = lo_change_cred(req, &old); if (err) goto out; - fd = openat(lo_fd(req, parent), name, + fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW, mode); err = fd == -1 ? errno : 0; lo_restore_cred(&old); @@ -1571,15 +1682,15 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, if (!err) { ssize_t fh; - update_version(lo, lo_inode(req, parent)); + update_version(lo, parent_inode); pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); - fuse_reply_err(req, ENOMEM); - return; + err = ENOMEM; + goto out; } fi->fh = fh; @@ -1591,6 +1702,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, fi->keep_cache = 1; out: + lo_inode_put(lo, &parent_inode); + if (err) fuse_reply_err(req, err); else @@ -1660,15 +1773,17 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, plock = lookup_create_plock_ctx(lo, inode, fi->lock_owner, lock->l_pid, &ret); if (!plock) { - pthread_mutex_unlock(&inode->plock_mutex); - fuse_reply_err(req, ret); - return; + saverr = ret; + goto out; } ret = fcntl(plock->fd, F_OFD_GETLK, lock); if (ret == -1) saverr = errno; + +out: pthread_mutex_unlock(&inode->plock_mutex); + lo_inode_put(lo, &inode); if (saverr) fuse_reply_err(req, saverr); @@ -1707,9 +1822,8 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, &ret); if (!plock) { - pthread_mutex_unlock(&inode->plock_mutex); - fuse_reply_err(req, ret); - return; + saverr = ret; + goto out; } /* TODO: Is it alright to modify flock? */ @@ -1718,7 +1832,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, if (ret == -1) { saverr = errno; } + +out: pthread_mutex_unlock(&inode->plock_mutex); + lo_inode_put(lo, &inode); + fuse_reply_err(req, saverr); } @@ -1849,6 +1967,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) res = close(dup(lo_fi_fd(req, fi))); fuse_reply_err(req, res == -1 ? errno : 0); + + lo_inode_put(lo_data(req), &inode); } static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync, @@ -1921,7 +2041,14 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino, if(res < 0) { fuse_reply_err(req, -res); } else { - update_version(lo, lo_inode(req, ino)); + struct lo_inode *inode; + + inode = lo_inode(req, ino); + if (inode) { + update_version(lo, inode); + lo_inode_put(lo, &inode); + } + fuse_reply_write(req, (size_t) res); } } @@ -1948,7 +2075,13 @@ static void lo_fallocate(fuse_req_t req, fuse_ino_t ino, int mode, if (err < 0) { err = errno; } else { - update_version(lo, lo_inode(req, ino)); + struct lo_inode *inode; + + inode = lo_inode(req, ino); + if (inode) { + update_version(lo, inode); + lo_inode_put(lo, &inode); + } } fuse_reply_err(req, err); @@ -2029,11 +2162,14 @@ out_free: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); return; out_err: saverr = errno; out: + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); goto out_free; } @@ -2101,11 +2237,14 @@ out_free: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); return; out_err: saverr = errno; out: + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); goto out_free; } @@ -2157,6 +2296,8 @@ out: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -2206,6 +2347,8 @@ out: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -2598,6 +2741,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) root->key.ino = stat.st_ino; root->key.dev = stat.st_dev; root->nlookup = 2; + g_atomic_int_set(&root->refcount, 2); } static void setup_proc_self_fd(struct lo_data *lo) -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Virtio-fs] [PATCH 5/5] virtiofsd: introduce inode refcount to prevent use-after-free @ 2019-07-31 16:10 ` Stefan Hajnoczi 0 siblings, 0 replies; 20+ messages in thread From: Stefan Hajnoczi @ 2019-07-31 16:10 UTC (permalink / raw) To: virtio-fs, qemu-devel If thread A is using an inode it must not be deleted by thread B when processing a FUSE_FORGET request. The FUSE protocol itself already has a counter called nlookup that is used in FUSE_FORGET messages. We cannot trust this counter since the untrusted client can manipulate it via FUSE_FORGET messages. Introduce a new refcount to keep inodes alive for the required lifespan. lo_inode_put() must be called to release a reference. FUSE's nlookup counter holds exactly one reference so that the inode stays alive as long as the client still wants to remember it. Note that the lo_inode->is_symlink field is moved to avoid creating a hole in the struct due to struct field alignment. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- contrib/virtiofsd/passthrough_ll.c | 212 ++++++++++++++++++++++++----- 1 file changed, 178 insertions(+), 34 deletions(-) diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c index 125e9d9f96..0c90e352d2 100644 --- a/contrib/virtiofsd/passthrough_ll.c +++ b/contrib/virtiofsd/passthrough_ll.c @@ -95,7 +95,12 @@ struct lo_key { struct lo_inode { int fd; - bool is_symlink; + + /* Atomic reference count for this object. The nlookup field holds a + * reference and release it when nlookup reaches 0. + */ + gint refcount; + struct lo_key key; /* This counter keeps the inode alive during the FUSE session. @@ -115,6 +120,8 @@ struct lo_inode { fuse_ino_t fuse_ino; pthread_mutex_t plock_mutex; GHashTable *posix_locks; /* protected by lo_inode->plock_mutex */ + + bool is_symlink; }; struct lo_cred { @@ -198,6 +205,7 @@ static const struct fuse_opt lo_opts[] = { FUSE_OPT_END }; static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n); +static void put_shared(struct lo_data *lo, struct lo_inode *inode); static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st); @@ -359,6 +367,24 @@ static ssize_t lo_add_inode_mapping(fuse_req_t req, struct lo_inode *inode) return elem - lo_data(req)->ino_map.elems; } +static void lo_inode_put(struct lo_data *lo, struct lo_inode **inodep) +{ + struct lo_inode *inode = *inodep; + + if (!inode) { + return; + } + + *inodep = NULL; + + if (g_atomic_int_dec_and_test(&inode->refcount)) { + close(inode->fd); + put_shared(lo, inode); + free(inode); + } +} + +/* Caller must release refcount using lo_inode_put() */ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) { struct lo_data *lo = lo_data(req); @@ -366,6 +392,9 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) pthread_mutex_lock(&lo->mutex); elem = lo_map_get(&lo->ino_map, ino); + if (elem) { + g_atomic_int_inc(&elem->inode->refcount); + } pthread_mutex_unlock(&lo->mutex); if (!elem) @@ -374,10 +403,22 @@ static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino) return elem->inode; } +/* TODO Remove this helper and force callers to hold an inode refcount until + * they are done with the fd. This will be done in a later patch to make + * review easier. + */ static int lo_fd(fuse_req_t req, fuse_ino_t ino) { struct lo_inode *inode = lo_inode(req, ino); - return inode ? inode->fd : -1; + int fd; + + if (!inode) { + return -1; + } + + fd = inode->fd; + lo_inode_put(lo_data(req), &inode); + return fd; } static bool lo_debug(fuse_req_t req) @@ -463,6 +504,9 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t ino, fuse_reply_attr(req, &buf, lo->timeout); } +/* Increments parent->nlookup and caller must release refcount using + * lo_inode_put(&parent). + */ static int lo_parent_and_name(struct lo_data *lo, struct lo_inode *inode, char path[PATH_MAX], struct lo_inode **parent) { @@ -498,6 +542,7 @@ retry: p = &lo->root; pthread_mutex_lock(&lo->mutex); p->nlookup++; + g_atomic_int_inc(&p->refcount); pthread_mutex_unlock(&lo->mutex); } else { *last = '\0'; @@ -570,6 +615,7 @@ fallback: if (res != -1) { res = utimensat(parent->fd, path, tv, AT_SYMLINK_NOFOLLOW); unref_inode(lo, parent, 1); + lo_inode_put(lo, &parent); } return res; @@ -683,11 +729,13 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr, goto out_err; } update_version(lo, inode); + lo_inode_put(lo, &inode); return lo_getattr(req, ino, fi); out_err: saverr = errno; + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -704,6 +752,7 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st) if (p) { assert(p->nlookup > 0); p->nlookup++; + g_atomic_int_inc(&p->refcount); } pthread_mutex_unlock(&lo->mutex); @@ -771,6 +820,9 @@ static void put_shared(struct lo_data *lo, struct lo_inode *inode) } } +/* Increments nlookup and caller must release refcount using + * lo_inode_put(&parent). + */ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, struct fuse_entry_param *e) { @@ -778,7 +830,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, int res; int saverr; struct lo_data *lo = lo_data(req); - struct lo_inode *inode, *dir = lo_inode(req, parent); + struct lo_inode *inode = NULL; + struct lo_inode *dir = lo_inode(req, parent); if (!dir) { return EBADF; @@ -811,6 +864,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, goto out_err; inode->is_symlink = S_ISLNK(e->attr.st_mode); + + /* One for the caller and one for nlookup (released in unref_inode()) */ + g_atomic_int_set(&inode->refcount, 2); + inode->nlookup = 1; inode->fd = newfd; newfd = -1; @@ -839,6 +896,8 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, e->ino = inode->fuse_ino; e->version_offset = inode->version_offset; + lo_inode_put(lo, &inode); + lo_inode_put(lo, &dir); if (lo_debug(req)) fuse_debug(" %lli/%s -> %lli (version_table[%lli]=%lli)\n", @@ -853,6 +912,8 @@ out_err: saverr = errno; if (newfd != -1) close(newfd); + lo_inode_put(lo, &inode); + lo_inode_put(lo, &dir); return saverr; } @@ -963,7 +1024,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, if (res == -1) goto out; - update_version(lo, lo_inode(req, parent)); + update_version(lo, dir); saverr = lo_do_lookup(req, parent, name, &e); if (saverr) @@ -975,9 +1036,11 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, (unsigned long long) e.ino); fuse_reply_entry(req, &e, lo->shared); + lo_inode_put(lo, &dir); return; out: + lo_inode_put(lo, &dir); if (newfd != -1) close(newfd); fuse_reply_err(req, saverr); @@ -1029,6 +1092,7 @@ fallback: if (res != -1) { res = linkat(parent->fd, path, dfd, name, 0); unref_inode(lo, parent, 1); + lo_inode_put(lo, &parent); } return res; @@ -1039,6 +1103,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, { int res; struct lo_data *lo = lo_data(req); + struct lo_inode *parent_inode; struct lo_inode *inode; struct fuse_entry_param e; int saverr; @@ -1048,17 +1113,18 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, return; } + parent_inode = lo_inode(req, parent); inode = lo_inode(req, ino); - if (!inode) { - fuse_reply_err(req, EBADF); - return; + if (!parent_inode || !inode) { + errno = EBADF; + goto out_err; } memset(&e, 0, sizeof(struct fuse_entry_param)); e.attr_timeout = lo->timeout; e.entry_timeout = lo->timeout; - res = linkat_empty_nofollow(lo, inode, lo_fd(req, parent), name); + res = linkat_empty_nofollow(lo, inode, parent_inode->fd, name); if (res == -1) goto out_err; @@ -1071,7 +1137,7 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, pthread_mutex_unlock(&lo->mutex); e.ino = inode->fuse_ino; update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + update_version(lo, parent_inode); if (lo_debug(req)) fuse_debug(" %lli/%s -> %lli\n", @@ -1079,13 +1145,18 @@ static void lo_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t parent, (unsigned long long) e.ino); fuse_reply_entry(req, &e, lo->shared); + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &inode); return; out_err: saverr = errno; + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } +/* Increments nlookup and caller must release refcount using lo_inode_put() */ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent, const char *name) { @@ -1121,11 +1192,20 @@ static void lo_rmdir(fuse_req_t req, fuse_ino_t parent, const char *name) if (res == -1) { fuse_reply_err(req, errno); } else { + struct lo_inode *parent_inode; + update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + + parent_inode = lo_inode(req, parent); + if (parent_inode) { + update_version(lo, parent_inode); + lo_inode_put(lo, &parent_inode); + } + fuse_reply_err(req, 0); } unref_inode(lo, inode, 1); + lo_inode_put(lo, &inode); } static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, @@ -1133,8 +1213,10 @@ 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_inode *parent_inode; + struct lo_inode *newparent_inode; + struct lo_inode *oldinode = NULL; + struct lo_inode *newinode = NULL; struct lo_data *lo = lo_data(req); if (!is_safe_path_component(name) || @@ -1143,6 +1225,13 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, return; } + parent_inode = lo_inode(req, parent); + newparent_inode = lo_inode(req, newparent); + if (!parent_inode || !newparent_inode) { + fuse_reply_err(req, EBADF); + goto out; + } + oldinode = lookup_name(req, parent, name); newinode = lookup_name(req, newparent, newname); if (!oldinode) { @@ -1155,8 +1244,8 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, #ifndef SYS_renameat2 fuse_reply_err(req, EINVAL); #else - res = syscall(SYS_renameat2, lo_fd(req, parent), name, - lo_fd(req, newparent), newname, flags); + res = syscall(SYS_renameat2, parent_inode->fd, name, + newparent_inode->fd, newname, flags); if (res == -1 && errno == ENOSYS) fuse_reply_err(req, EINVAL); else @@ -1165,21 +1254,24 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name, goto out; } - res = renameat(lo_fd(req, parent), name, - lo_fd(req, newparent), newname); + res = renameat(parent_inode->fd, name, newparent_inode->fd, newname); if (res == -1) { fuse_reply_err(req, errno); } else { update_version(lo, oldinode); if (newinode) update_version(lo, newinode); - update_version(lo, lo_inode(req, parent)); - update_version(lo, lo_inode(req, newparent)); + update_version(lo, parent_inode); + update_version(lo, newparent_inode); fuse_reply_err(req, 0); } out: unref_inode(lo, oldinode, 1); unref_inode(lo, newinode, 1); + lo_inode_put(lo, &oldinode); + lo_inode_put(lo, &newinode); + lo_inode_put(lo, &parent_inode); + lo_inode_put(lo, &newparent_inode); } static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) @@ -1203,11 +1295,20 @@ static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name) if (res == -1) { fuse_reply_err(req, errno); } else { + struct lo_inode *parent_inode; + update_version(lo, inode); - update_version(lo, lo_inode(req, parent)); + + parent_inode = lo_inode(req, parent); + if (parent_inode) { + update_version(lo, parent_inode); + lo_inode_put(lo, &parent_inode); + } + fuse_reply_err(req, 0); } unref_inode(lo, inode, 1); + lo_inode_put(lo, &inode); } static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) @@ -1227,9 +1328,9 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n) g_hash_table_destroy(inode->posix_locks); pthread_mutex_destroy(&inode->plock_mutex); pthread_mutex_unlock(&lo->mutex); - close(inode->fd); - put_shared(lo, inode); - free(inode); + + /* Drop our refcount from lo_do_lookup() */ + lo_inode_put(lo, &inode); } else { pthread_mutex_unlock(&lo->mutex); } @@ -1244,6 +1345,7 @@ static int unref_all_inodes_cb(gpointer key, gpointer value, inode->nlookup = 0; lo_map_remove(&lo->ino_map, inode->fuse_ino); close(inode->fd); + lo_inode_put(lo, &inode); /* Drop our refcount from lo_do_lookup() */ return TRUE; } @@ -1273,6 +1375,7 @@ static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) } unref_inode(lo, inode, nlookup); + lo_inode_put(lo, &inode); } static void lo_forget(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup) @@ -1492,6 +1595,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, err = 0; error: lo_dirp_put(&d); + lo_inode_put(lo, &dinode); // 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 @@ -1546,6 +1650,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, { int fd; struct lo_data *lo = lo_data(req); + struct lo_inode *parent_inode; struct fuse_entry_param e; int err; struct lo_cred old = {}; @@ -1559,11 +1664,17 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, return; } + parent_inode = lo_inode(req, parent); + if (!parent_inode) { + fuse_reply_err(req, EBADF); + return; + } + err = lo_change_cred(req, &old); if (err) goto out; - fd = openat(lo_fd(req, parent), name, + fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW, mode); err = fd == -1 ? errno : 0; lo_restore_cred(&old); @@ -1571,15 +1682,15 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, if (!err) { ssize_t fh; - update_version(lo, lo_inode(req, parent)); + update_version(lo, parent_inode); pthread_mutex_lock(&lo->mutex); fh = lo_add_fd_mapping(req, fd); pthread_mutex_unlock(&lo->mutex); if (fh == -1) { close(fd); - fuse_reply_err(req, ENOMEM); - return; + err = ENOMEM; + goto out; } fi->fh = fh; @@ -1591,6 +1702,8 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, fi->keep_cache = 1; out: + lo_inode_put(lo, &parent_inode); + if (err) fuse_reply_err(req, err); else @@ -1660,15 +1773,17 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino, plock = lookup_create_plock_ctx(lo, inode, fi->lock_owner, lock->l_pid, &ret); if (!plock) { - pthread_mutex_unlock(&inode->plock_mutex); - fuse_reply_err(req, ret); - return; + saverr = ret; + goto out; } ret = fcntl(plock->fd, F_OFD_GETLK, lock); if (ret == -1) saverr = errno; + +out: pthread_mutex_unlock(&inode->plock_mutex); + lo_inode_put(lo, &inode); if (saverr) fuse_reply_err(req, saverr); @@ -1707,9 +1822,8 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, &ret); if (!plock) { - pthread_mutex_unlock(&inode->plock_mutex); - fuse_reply_err(req, ret); - return; + saverr = ret; + goto out; } /* TODO: Is it alright to modify flock? */ @@ -1718,7 +1832,11 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino, if (ret == -1) { saverr = errno; } + +out: pthread_mutex_unlock(&inode->plock_mutex); + lo_inode_put(lo, &inode); + fuse_reply_err(req, saverr); } @@ -1849,6 +1967,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi) res = close(dup(lo_fi_fd(req, fi))); fuse_reply_err(req, res == -1 ? errno : 0); + + lo_inode_put(lo_data(req), &inode); } static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync, @@ -1921,7 +2041,14 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino, if(res < 0) { fuse_reply_err(req, -res); } else { - update_version(lo, lo_inode(req, ino)); + struct lo_inode *inode; + + inode = lo_inode(req, ino); + if (inode) { + update_version(lo, inode); + lo_inode_put(lo, &inode); + } + fuse_reply_write(req, (size_t) res); } } @@ -1948,7 +2075,13 @@ static void lo_fallocate(fuse_req_t req, fuse_ino_t ino, int mode, if (err < 0) { err = errno; } else { - update_version(lo, lo_inode(req, ino)); + struct lo_inode *inode; + + inode = lo_inode(req, ino); + if (inode) { + update_version(lo, inode); + lo_inode_put(lo, &inode); + } } fuse_reply_err(req, err); @@ -2029,11 +2162,14 @@ out_free: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); return; out_err: saverr = errno; out: + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); goto out_free; } @@ -2101,11 +2237,14 @@ out_free: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); return; out_err: saverr = errno; out: + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); goto out_free; } @@ -2157,6 +2296,8 @@ out: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -2206,6 +2347,8 @@ out: if (fd >= 0) { close(fd); } + + lo_inode_put(lo, &inode); fuse_reply_err(req, saverr); } @@ -2598,6 +2741,7 @@ static void setup_root(struct lo_data *lo, struct lo_inode *root) root->key.ino = stat.st_ino; root->key.dev = stat.st_dev; root->nlookup = 2; + g_atomic_int_set(&root->refcount, 2); } static void setup_proc_self_fd(struct lo_data *lo) -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-08-01 12:01 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-31 16:10 [Qemu-devel] [PATCH 0/5] virtiofsd: multithreading preparation part 2 Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi 2019-07-31 16:10 ` [Qemu-devel] [PATCH 1/5] virtiofsd: take lo->mutex around lo_add_fd_mapping() Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi 2019-07-31 18:45 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-07-31 18:45 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-08-01 9:17 ` [Qemu-devel] " Stefan Hajnoczi 2019-08-01 9:17 ` [Virtio-fs] " Stefan Hajnoczi 2019-07-31 16:10 ` [Qemu-devel] [PATCH 2/5] virtiofsd: take lo->mutex around lo_add_dirp_mapping() Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-01 11:45 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-01 11:45 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-07-31 16:10 ` [Qemu-devel] [PATCH 3/5] virtiofsd: rename inode->refcount to inode->nlookup Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi 2019-08-01 12:00 ` [Qemu-devel] " Dr. David Alan Gilbert 2019-08-01 12:00 ` [Virtio-fs] " Dr. David Alan Gilbert 2019-07-31 16:10 ` [Qemu-devel] [PATCH 4/5] virtiofsd: fix inode nlookup leaks Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi 2019-07-31 16:10 ` [Qemu-devel] [PATCH 5/5] virtiofsd: introduce inode refcount to prevent use-after-free Stefan Hajnoczi 2019-07-31 16:10 ` [Virtio-fs] " Stefan Hajnoczi
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.