| Message ID | 154754755979.4244.14965151684224631403.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Improve disconnect scheme and avoid taking fpq->lock on hot paths | expand |
On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > > fuse_dev_alloc() may be called after fc->connected > is dropped (from ioctl), so here we add sanity check > for that case. AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to the fuse_conn's list after disconnection there would be no leak. In other words, it's irrelevant whether the connection reset comes just before the ioctl completes or just after. Or am I missing something? Thanks, Miklos > > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> > --- > fs/fuse/inode.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index 336844d0eb3a..0361a3d62356 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc) > fuse_pqueue_init(&fud->pq); > > spin_lock(&fc->lock); > + if (!fc->connected) { > + spin_unlock(&fc->lock); > + goto out_put; > + } > list_add_tail(&fud->entry, &fc->devices); > spin_unlock(&fc->lock); > > return fud; > +out_put: > + fuse_conn_put(fc); > + kfree(pq); > + kfree(fud); > + return NULL; > } > EXPORT_SYMBOL_GPL(fuse_dev_alloc); > >
On 18.01.2019 15:07, Miklos Szeredi wrote: > On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >> >> fuse_dev_alloc() may be called after fc->connected >> is dropped (from ioctl), so here we add sanity check >> for that case. > > AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to > the fuse_conn's list after disconnection there would be no leak. > > In other words, it's irrelevant whether the connection reset comes > just before the ioctl completes or just after. Or am I missing > something? Yeah, there won't be a leak. The only problem I see, userspace daemon may become waiting in fuse_dev_do_read() after abort is finished. This means fc->count won't be put, and manual killing signal will be needed. I.e., umount -f will wait till the daemon is killed manually. Not so big a problem, but not very pleasant... Kirill >> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >> --- >> fs/fuse/inode.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c >> index 336844d0eb3a..0361a3d62356 100644 >> --- a/fs/fuse/inode.c >> +++ b/fs/fuse/inode.c >> @@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc) >> fuse_pqueue_init(&fud->pq); >> >> spin_lock(&fc->lock); >> + if (!fc->connected) { >> + spin_unlock(&fc->lock); >> + goto out_put; >> + } >> list_add_tail(&fud->entry, &fc->devices); >> spin_unlock(&fc->lock); >> >> return fud; >> +out_put: >> + fuse_conn_put(fc); >> + kfree(pq); >> + kfree(fud); >> + return NULL; >> } >> EXPORT_SYMBOL_GPL(fuse_dev_alloc); >> >>
On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > > On 18.01.2019 15:07, Miklos Szeredi wrote: > > On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > >> > >> fuse_dev_alloc() may be called after fc->connected > >> is dropped (from ioctl), so here we add sanity check > >> for that case. > > > > AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to > > the fuse_conn's list after disconnection there would be no leak. > > > > In other words, it's irrelevant whether the connection reset comes > > just before the ioctl completes or just after. Or am I missing > > something? > > Yeah, there won't be a leak. The only problem I see, userspace daemon > may become waiting in fuse_dev_do_read() after abort is finished. By that time fiq->connected will be reset, so fuse_dev_do_read() will return ENODEV/ECONNABORTED. Am I missing something? Thanks, Miklos
On 23.01.2019 12:45, Miklos Szeredi wrote: > On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >> >> On 18.01.2019 15:07, Miklos Szeredi wrote: >>> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: >>>> >>>> fuse_dev_alloc() may be called after fc->connected >>>> is dropped (from ioctl), so here we add sanity check >>>> for that case. >>> >>> AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to >>> the fuse_conn's list after disconnection there would be no leak. >>> >>> In other words, it's irrelevant whether the connection reset comes >>> just before the ioctl completes or just after. Or am I missing >>> something? >> >> Yeah, there won't be a leak. The only problem I see, userspace daemon >> may become waiting in fuse_dev_do_read() after abort is finished. > > By that time fiq->connected will be reset, so fuse_dev_do_read() will > return ENODEV/ECONNABORTED. > > Am I missing something? I mean: [cpu0] [cpu1] fuse_device_clone() fuse_dev_alloc() fuse_abort_conn() spin_lock(&fc->lock) list_for_each_entry(fud, &fc->devices, entry) fpq->connected = 0 spin_unlock(&fc->lock) spin_lock(&fc->lock); list_add_tail(&fud->entry, &fc->devices); spin_unlock(&fc->lock); ... fuse_dev_do_read() wait_event_interruptible_exclusive_locked() <-- wait forever If you have manager-thread on cpu1, and it expects all worker-threads (like on cpu0) will finish after abort, this won't be true. Kirill
On Wed, Jan 23, 2019 at 10:55 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > > On 23.01.2019 12:45, Miklos Szeredi wrote: > > On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > >> > >> On 18.01.2019 15:07, Miklos Szeredi wrote: > >>> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote: > >>>> > >>>> fuse_dev_alloc() may be called after fc->connected > >>>> is dropped (from ioctl), so here we add sanity check > >>>> for that case. > >>> > >>> AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to > >>> the fuse_conn's list after disconnection there would be no leak. > >>> > >>> In other words, it's irrelevant whether the connection reset comes > >>> just before the ioctl completes or just after. Or am I missing > >>> something? > >> > >> Yeah, there won't be a leak. The only problem I see, userspace daemon > >> may become waiting in fuse_dev_do_read() after abort is finished. > > > > By that time fiq->connected will be reset, so fuse_dev_do_read() will > > return ENODEV/ECONNABORTED. > > > > Am I missing something? > > I mean: > > [cpu0] [cpu1] > fuse_device_clone() > fuse_dev_alloc() fuse_abort_conn() > spin_lock(&fc->lock) > list_for_each_entry(fud, &fc->devices, entry) > fpq->connected = 0 > spin_unlock(&fc->lock) > spin_lock(&fc->lock); > list_add_tail(&fud->entry, &fc->devices); Okay, so there's missing fpq->connected = fc->connected; here. > spin_unlock(&fc->lock); > ... > fuse_dev_do_read() > wait_event_interruptible_exclusive_locked() <-- wait forever That will exit immediately, because it checks fiq->connected (not fpq->connected) which is already zero. Thanks, Miklos
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 336844d0eb3a..0361a3d62356 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc) fuse_pqueue_init(&fud->pq); spin_lock(&fc->lock); + if (!fc->connected) { + spin_unlock(&fc->lock); + goto out_put; + } list_add_tail(&fud->entry, &fc->devices); spin_unlock(&fc->lock); return fud; +out_put: + fuse_conn_put(fc); + kfree(pq); + kfree(fud); + return NULL; } EXPORT_SYMBOL_GPL(fuse_dev_alloc);
fuse_dev_alloc() may be called after fc->connected is dropped (from ioctl), so here we add sanity check for that case. Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- fs/fuse/inode.c | 9 +++++++++ 1 file changed, 9 insertions(+)