On Wed, Jan 23, 2019 at 10:55 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>
> On 23.01.2019 12:45, Miklos Szeredi wrote:
> > On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
> >>
> >> On 18.01.2019 15:07, Miklos Szeredi wrote:
> >>> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> 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
