Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221117142915.1366990-1-dwysocha@redhat.com>
In-Reply-To: <20221117142915.1366990-1-dwysocha@redhat.com>
From:   Daire Byrne <daire@dneg.com>
Date:   Wed, 7 Dec 2022 09:58:10 +0000
Message-ID: <CAPt2mGOYeJNbqzb8RD94BG0Y8HpHja_UCG6kifQR1kNuWyiMcg@mail.gmail.com>
Subject: Re: [PATCH v2] fscache: Fix oops due to race with cookie_lru and use_cookie
To:     Dave Wysochanski <dwysocha@redhat.com>
Cc:     David Howells <dhowells@redhat.com>,
        Daire Byrne <daire.byrne@gmail.com>,
        Benjamin Maynard <benmaynard@google.com>,
        linux-cachefs@redhat.com, linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

I have also now tested this v2 patch and can confirm that it also
fixes the race in fscache that we were reliably able to reproduce with
our (re-export) workloads..

Tested-by: Daire Byrne <daire@dneg.com>

Daire

On Thu, 17 Nov 2022 at 14:30, Dave Wysochanski <dwysocha@redhat.com> wrote:
>
> If a cookie expires from the LRU and the LRU_DISCARD flag is set,
> but the state machine has not run yet, it's possible another thread
> can call fscache_use_cookie and begin to use it.  When the
> cookie_worker finally runs, it will see the LRU_DISCARD flag set,
> transition the cookie->state to LRU_DISCARDING, which will then
> withdraw the cookie.  Once the cookie is withdrawn the object is
> removed the below oops will occur because the object associated
> with the cookie is now NULL.
>
> Fix the oops by clearing the LRU_DISCARD bit if another thread
> uses the cookie before the cookie_worker runs.
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000008
>   ...
>   CPU: 31 PID: 44773 Comm: kworker/u130:1 Tainted: G     E    6.0.0-5.dneg.x86_64 #1
>   Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 08/26/2022
>   Workqueue: events_unbound netfs_rreq_write_to_cache_work [netfs]
>   RIP: 0010:cachefiles_prepare_write+0x28/0x90 [cachefiles]
>   ...
>   Call Trace:
>    netfs_rreq_write_to_cache_work+0x11c/0x320 [netfs]
>    process_one_work+0x217/0x3e0
>    worker_thread+0x4a/0x3b0
>    ? process_one_work+0x3e0/0x3e0
>    kthread+0xd6/0x100
>    ? kthread_complete_and_exit+0x20/0x20
>    ret_from_fork+0x1f/0x30
>
> Fixes: 12bb21a29c19 ("fscache: Implement cookie user counting and resource pinning")
> Reported-by: Daire Byrne <daire.byrne@gmail.com>
> Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> ---
>  fs/fscache/cookie.c            | 8 ++++++++
>  include/trace/events/fscache.h | 2 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c
> index 451d8a077e12..bce2492186d0 100644
> --- a/fs/fscache/cookie.c
> +++ b/fs/fscache/cookie.c
> @@ -605,6 +605,14 @@ void __fscache_use_cookie(struct fscache_cookie *cookie, bool will_modify)
>                         set_bit(FSCACHE_COOKIE_DO_PREP_TO_WRITE, &cookie->flags);
>                         queue = true;
>                 }
> +               /*
> +                * We could race with cookie_lru which may set LRU_DISCARD bit
> +                * but has yet to run the cookie state machine.  If this happens
> +                * and another thread tries to use the cookie, clear LRU_DISCARD
> +                * so we don't end up withdrawing the cookie while in use.
> +                */
> +               if (test_and_clear_bit(FSCACHE_COOKIE_DO_LRU_DISCARD, &cookie->flags))
> +                       fscache_see_cookie(cookie, fscache_cookie_see_lru_discard_clear);
>                 break;
>
>         case FSCACHE_COOKIE_STATE_FAILED:
> diff --git a/include/trace/events/fscache.h b/include/trace/events/fscache.h
> index c078c48a8e6d..a6190aa1b406 100644
> --- a/include/trace/events/fscache.h
> +++ b/include/trace/events/fscache.h
> @@ -66,6 +66,7 @@ enum fscache_cookie_trace {
>         fscache_cookie_put_work,
>         fscache_cookie_see_active,
>         fscache_cookie_see_lru_discard,
> +       fscache_cookie_see_lru_discard_clear,
>         fscache_cookie_see_lru_do_one,
>         fscache_cookie_see_relinquish,
>         fscache_cookie_see_withdraw,
> @@ -149,6 +150,7 @@ enum fscache_access_trace {
>         EM(fscache_cookie_put_work,             "PQ  work ")            \
>         EM(fscache_cookie_see_active,           "-   activ")            \
>         EM(fscache_cookie_see_lru_discard,      "-   x-lru")            \
> +       EM(fscache_cookie_see_lru_discard_clear,"-   lrudc")            \
>         EM(fscache_cookie_see_lru_do_one,       "-   lrudo")            \
>         EM(fscache_cookie_see_relinquish,       "-   x-rlq")            \
>         EM(fscache_cookie_see_withdraw,         "-   x-wth")            \
> --
> 2.31.1
>