[SRU T][PATCH 3/6] cachefiles: Fix refcounting bug in backing-file read monitoring

[Daniel Axtens]

Hi Kleber,

Thanks for catching this. Apologies for the trouble - I will open a
new bug in future.

Regards,
Daniel
On Sat, Aug 25, 2018 at 1:20 AM Kleber Souza <kleber.souza at canonical.com> wrote:
>
> On 08/02/18 06:18, Daniel Axtens wrote:
> > From: Kiran Kumar Modukuri <kiran.modukuri at gmail.com>
> >
> > BugLink: https://bugs.launchpad.net/bugs/1774336
>
> Hi Daniel,
>
> We didn't catch it before, but this LP bug had already been marked as
> 'Fix Released' when this patch series was sent.
>
> In the future, please do not re-use a LP bug whose fix has already been
> released. It makes it difficult to track the status of the follow-up
> fixes. The best approach is to create a new bug referencing the old one
> so we can track its status accordingly.
>
>
> Thanks,
> Kleber
>
>
>
> >
> > cachefiles_read_waiter() has the right to access a 'monitor' object by
> > virtue of being called under the waitqueue lock for one of the pages in its
> > purview.  However, it has no ref on that monitor object or on the
> > associated operation.
> >
> > What it is allowed to do is to move the monitor object to the operation's
> > to_do list, but once it drops the work_lock, it's actually no longer
> > permitted to access that object.  However, it is trying to enqueue the
> > retrieval operation for processing - but it can only do this via a pointer
> > in the monitor object, something it shouldn't be doing.
> >
> > If it doesn't enqueue the operation, the operation may not get processed.
> > If the order is flipped so that the enqueue is first, then it's possible
> > for the work processor to look at the to_do list before the monitor is
> > enqueued upon it.
> >
> > Fix this by getting a ref on the operation so that we can trust that it
> > will still be there once we've added the monitor to the to_do list and
> > dropped the work_lock.  The op can then be enqueued after the lock is
> > dropped.
> >
> > The bug can manifest in one of a couple of ways.  The first manifestation
> > looks like:
> >
> >  FS-Cache:
> >  FS-Cache: Assertion failed
> >  FS-Cache: 6 == 5 is false
> >  ------------[ cut here ]------------
> >  kernel BUG at fs/fscache/operation.c:494!
> >  RIP: 0010:fscache_put_operation+0x1e3/0x1f0
> >  ...
> >  fscache_op_work_func+0x26/0x50
> >  process_one_work+0x131/0x290
> >  worker_thread+0x45/0x360
> >  kthread+0xf8/0x130
> >  ? create_worker+0x190/0x190
> >  ? kthread_cancel_work_sync+0x10/0x10
> >  ret_from_fork+0x1f/0x30
> >
> > This is due to the operation being in the DEAD state (6) rather than
> > INITIALISED, COMPLETE or CANCELLED (5) because it's already passed through
> > fscache_put_operation().
> >
> > The bug can also manifest like the following:
> >
> >  kernel BUG at fs/fscache/operation.c:69!
> >  ...
> >     [exception RIP: fscache_enqueue_operation+246]
> >  ...
> >  #7 [ffff883fff083c10] fscache_enqueue_operation at ffffffffa0b793c6
> >  #8 [ffff883fff083c28] cachefiles_read_waiter at ffffffffa0b15a48
> >  #9 [ffff883fff083c48] __wake_up_common at ffffffff810af028
> >
> > I'm not entirely certain as to which is line 69 in Lei's kernel, so I'm not
> > entirely clear which assertion failed.
> >
> > Fixes: 9ae326a69004 ("CacheFiles: A cache that backs onto a mounted filesystem")
> > Reported-by: Lei Xue <carmark.dlut at gmail.com>
> > Reported-by: Vegard Nossum <vegard.nossum at gmail.com>
> > Reported-by: Anthony DeRobertis <aderobertis at metrics.net>
> > Reported-by: NeilBrown <neilb at suse.com>
> > Reported-by: Daniel Axtens <dja at axtens.net>
> > Reported-by: Kiran Kumar Modukuri <kiran.modukuri at gmail.com>
> > Signed-off-by: David Howells <dhowells at redhat.com>
> > Reviewed-by: Daniel Axtens <dja at axtens.net>
> > (cherry picked from commit 934140ab028713a61de8bca58c05332416d037d1)
> > Signed-off-by: Daniel Axtens <daniel.axtens at canonical.com>
> > ---
> >  fs/cachefiles/rdwr.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
> > index a1210b0322e0..725a0e20a913 100644
> > --- a/fs/cachefiles/rdwr.c
> > +++ b/fs/cachefiles/rdwr.c
> > @@ -27,6 +27,7 @@ static int cachefiles_read_waiter(wait_queue_t *wait, unsigned mode,
> >       struct cachefiles_one_read *monitor =
> >               container_of(wait, struct cachefiles_one_read, monitor);
> >       struct cachefiles_object *object;
> > +     struct fscache_retrieval *op = monitor->op;
> >       struct wait_bit_key *key = _key;
> >       struct page *page = wait->private;
> >
> > @@ -51,16 +52,22 @@ static int cachefiles_read_waiter(wait_queue_t *wait, unsigned mode,
> >       list_del(&wait->task_list);
> >
> >       /* move onto the action list and queue for FS-Cache thread pool */
> > -     ASSERT(monitor->op);
> > +     ASSERT(op);
> >
> > -     object = container_of(monitor->op->op.object,
> > -                           struct cachefiles_object, fscache);
> > +     /* We need to temporarily bump the usage count as we don't own a ref
> > +      * here otherwise cachefiles_read_copier() may free the op between the
> > +      * monitor being enqueued on the op->to_do list and the op getting
> > +      * enqueued on the work queue.
> > +      */
> > +     fscache_get_retrieval(op);
> >
> > +     object = container_of(op->op.object, struct cachefiles_object, fscache);
> >       spin_lock(&object->work_lock);
> > -     list_add_tail(&monitor->op_link, &monitor->op->to_do);
> > +     list_add_tail(&monitor->op_link, &op->to_do);
> >       spin_unlock(&object->work_lock);
> >
> > -     fscache_enqueue_retrieval(monitor->op);
> > +     fscache_enqueue_retrieval(op);
> > +     fscache_put_retrieval(op);
> >       return 0;
> >  }
> >
> >
>