Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Miklos Szeredi <miklos@szeredi.hu>]

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