From: Jeff Layton <jlayton@kernel.org>
To: libaokun@huaweicloud.com, netfs@lists.linux.dev, dhowells@redhat.com
Cc: hsiangkao@linux.alibaba.com, jefflexu@linux.alibaba.com,
zhujia.zj@bytedance.com, linux-erofs@lists.ozlabs.org,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
yangerkun@huawei.com, houtao1@huawei.com, yukuai3@huawei.com,
wozizhi@huawei.com, Baokun Li <libaokun1@huawei.com>
Subject: Re: [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse
Date: Sun, 19 May 2024 07:11:03 -0400 [thread overview]
Message-ID: <f449f710b7e1ba725ec9f73cace6c1289b9225b6.camel@kernel.org> (raw)
In-Reply-To: <20240515125136.3714580-5-libaokun@huaweicloud.com>
On Wed, 2024-05-15 at 20:51 +0800, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
>
> Reusing the msg_id after a maliciously completed reopen request may cause
> a read request to remain unprocessed and result in a hung, as shown below:
>
> t1 | t2 | t3
> -------------------------------------------------
> cachefiles_ondemand_select_req
> cachefiles_ondemand_object_is_close(A)
> cachefiles_ondemand_set_object_reopening(A)
> queue_work(fscache_object_wq, &info->work)
> ondemand_object_worker
> cachefiles_ondemand_init_object(A)
> cachefiles_ondemand_send_req(OPEN)
> // get msg_id 6
> wait_for_completion(&req_A->done)
> cachefiles_ondemand_daemon_read
> // read msg_id 6 req_A
> cachefiles_ondemand_get_fd
> copy_to_user
> // Malicious completion msg_id 6
> copen 6,-1
> cachefiles_ondemand_copen
> complete(&req_A->done)
> // will not set the object to close
> // because ondemand_id && fd is valid.
>
> // ondemand_object_worker() is done
> // but the object is still reopening.
>
> // new open req_B
> cachefiles_ondemand_init_object(B)
> cachefiles_ondemand_send_req(OPEN)
> // reuse msg_id 6
> process_open_req
> copen 6,A.size
> // The expected failed copen was executed successfully
>
> Expect copen to fail, and when it does, it closes fd, which sets the
> object to close, and then close triggers reopen again. However, due to
> msg_id reuse resulting in a successful copen, the anonymous fd is not
> closed until the daemon exits. Therefore read requests waiting for reopen
> to complete may trigger hung task.
>
> To avoid this issue, allocate the msg_id cyclically to avoid reusing the
> msg_id for a very short duration of time.
>
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> fs/cachefiles/internal.h | 1 +
> fs/cachefiles/ondemand.c | 20 ++++++++++++++++----
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index 8ecd296cc1c4..9200c00f3e98 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -128,6 +128,7 @@ struct cachefiles_cache {
> unsigned long req_id_next;
> struct xarray ondemand_ids; /* xarray for ondemand_id allocation */
> u32 ondemand_id_next;
> + u32 msg_id_next;
> };
>
> static inline bool cachefiles_in_ondemand_mode(struct cachefiles_cache *cache)
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index f6440b3e7368..b10952f77472 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -433,20 +433,32 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
> smp_mb();
>
> if (opcode == CACHEFILES_OP_CLOSE &&
> - !cachefiles_ondemand_object_is_open(object)) {
> + !cachefiles_ondemand_object_is_open(object)) {
> WARN_ON_ONCE(object->ondemand->ondemand_id == 0);
> xas_unlock(&xas);
> ret = -EIO;
> goto out;
> }
>
> - xas.xa_index = 0;
> + /*
> + * Cyclically find a free xas to avoid msg_id reuse that would
> + * cause the daemon to successfully copen a stale msg_id.
> + */
> + xas.xa_index = cache->msg_id_next;
> xas_find_marked(&xas, UINT_MAX, XA_FREE_MARK);
> + if (xas.xa_node == XAS_RESTART) {
> + xas.xa_index = 0;
> + xas_find_marked(&xas, cache->msg_id_next - 1, XA_FREE_MARK);
> + }
> if (xas.xa_node == XAS_RESTART)
> xas_set_err(&xas, -EBUSY);
> +
> xas_store(&xas, req);
> - xas_clear_mark(&xas, XA_FREE_MARK);
> - xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> + if (xas_valid(&xas)) {
> + cache->msg_id_next = xas.xa_index + 1;
If you have a long-standing stuck request, could this counter wrap
around and you still end up with reuse? Maybe this should be using
ida_alloc/free instead, which would prevent that too?
> + xas_clear_mark(&xas, XA_FREE_MARK);
> + xas_set_mark(&xas, CACHEFILES_REQ_NEW);
> + }
> xas_unlock(&xas);
> } while (xas_nomem(&xas, GFP_KERNEL));
>
--
Jeff Layton <jlayton@kernel.org>
next prev parent reply other threads:[~2024-05-19 11:11 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-15 12:51 [PATCH v2 0/5] cachefiles: some bugfixes for clean object/send req/poll libaokun
2024-05-15 12:51 ` [PATCH v2 1/5] cachefiles: stop sending new request when dropping object libaokun
2024-05-15 12:51 ` [PATCH v2 2/5] cachefiles: flush all requests for the object that is being dropped libaokun
2024-05-15 12:51 ` [PATCH v2 3/5] cachefiles: flush ondemand_object_worker during clean object libaokun
2024-05-15 12:51 ` [PATCH v2 4/5] cachefiles: cyclic allocation of msg_id to avoid reuse libaokun
2024-05-19 11:11 ` Jeff Layton [this message]
2024-05-20 4:06 ` Baokun Li
2024-05-20 10:04 ` Jeff Layton
2024-05-20 12:42 ` Baokun Li
2024-05-20 12:54 ` Gao Xiang
2024-05-20 13:24 ` Baokun Li
2024-05-20 14:56 ` Gao Xiang
2024-05-21 2:36 ` Baokun Li
2024-05-21 2:53 ` Gao Xiang
2024-05-20 13:24 ` Jeff Layton
2024-05-15 12:51 ` [PATCH v2 5/5] cachefiles: add missing lock protection when polling libaokun
2024-06-26 3:04 ` [PATCH v2 0/5] cachefiles: some bugfixes for clean object/send req/poll Baokun Li
2024-06-26 3:28 ` Gao Xiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f449f710b7e1ba725ec9f73cace6c1289b9225b6.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=dhowells@redhat.com \
--cc=houtao1@huawei.com \
--cc=hsiangkao@linux.alibaba.com \
--cc=jefflexu@linux.alibaba.com \
--cc=libaokun1@huawei.com \
--cc=libaokun@huaweicloud.com \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netfs@lists.linux.dev \
--cc=wozizhi@huawei.com \
--cc=yangerkun@huawei.com \
--cc=yukuai3@huawei.com \
--cc=zhujia.zj@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).