From: Baokun Li <libaokun@huaweicloud.com>
To: Jingbo Xu <jefflexu@linux.alibaba.com>, netfs@lists.linux.dev
Cc: dhowells@redhat.com, jlayton@kernel.org, zhujia.zj@bytedance.com,
linux-erofs@lists.ozlabs.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, libaokun@huaweicloud.com,
yangerkun <yangerkun@huawei.com>
Subject: Re: [PATCH 07/12] cachefiles: add spin_lock for cachefiles_ondemand_info
Date: Mon, 6 May 2024 11:23:36 +0800 [thread overview]
Message-ID: <18cc6974-b47d-f9fd-2576-366382d0e8d0@huaweicloud.com> (raw)
In-Reply-To: <1fef9ab5-ec33-4a14-beb3-ada41a8652b3@linux.alibaba.com>
On 2024/5/6 10:55, Jingbo Xu wrote:
>
> On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> The following concurrency may cause a read request to fail to be completed
>> and result in a hung:
>>
>> t1 | t2
>> ---------------------------------------------------------
>> cachefiles_ondemand_copen
>> req = xa_erase(&cache->reqs, id)
>> // Anon fd is maliciously closed.
>> cachefiles_ondemand_fd_release
>> xa_lock(&cache->reqs)
>> cachefiles_ondemand_set_object_close(object)
>> xa_unlock(&cache->reqs)
>> cachefiles_ondemand_set_object_open
>> // No one will ever close it again.
>> cachefiles_ondemand_daemon_read
>> cachefiles_ondemand_select_req
>> // Get a read req but its fd is already closed.
>> // The daemon can't issue a cread ioctl with an closed fd, then hung.
>>
>> So add spin_lock for cachefiles_ondemand_info to protect ondemand_id and
>> state, thus we can avoid the above problem in cachefiles_ondemand_copen()
>> by using ondemand_id to determine if fd has been released.
>>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> This indeed looks like a reasonable scenario where the kernel side
> should fix, as a non-malicious daemon could also run into this.
>
> How about reusing &cache->reqs spinlock rather than introducing a new
> spinlock, as &cache->reqs spinlock is already held when setting object
> to close state in cachefiles_ondemand_fd_release()?
We've considered reusing &cache->reqs spinlock before, but their
uses don't exactly overlap, and there are patches coming that will
use the new spin_lock,. In addition, this reduces competition for
&cache->reqs spinlock.
>> ---
>> fs/cachefiles/internal.h | 1 +
>> fs/cachefiles/ondemand.c | 16 +++++++++++++++-
>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
>> index 7745b8abc3aa..45c8bed60538 100644
>> --- a/fs/cachefiles/internal.h
>> +++ b/fs/cachefiles/internal.h
>> @@ -55,6 +55,7 @@ struct cachefiles_ondemand_info {
>> int ondemand_id;
>> enum cachefiles_object_state state;
>> struct cachefiles_object *object;
>> + spinlock_t lock;
>> };
>>
>> /*
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index 898fab68332b..b5e6a851ef04 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -16,13 +16,16 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
>> struct cachefiles_object *object = file->private_data;
>> struct cachefiles_cache *cache = object->volume->cache;
>> struct cachefiles_ondemand_info *info = object->ondemand;
>> - int object_id = info->ondemand_id;
>> + int object_id;
>> struct cachefiles_req *req;
>> XA_STATE(xas, &cache->reqs, 0);
>>
>> xa_lock(&cache->reqs);
>> + spin_lock(&info->lock);
>> + object_id = info->ondemand_id;
>> info->ondemand_id = CACHEFILES_ONDEMAND_ID_CLOSED;
>> cachefiles_ondemand_set_object_close(object);
>> + spin_unlock(&info->lock);
>>
>> /* Only flush CACHEFILES_REQ_NEW marked req to avoid race with daemon_read */
>> xas_for_each_marked(&xas, req, ULONG_MAX, CACHEFILES_REQ_NEW) {
>> @@ -127,6 +130,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>> {
>> struct cachefiles_req *req;
>> struct fscache_cookie *cookie;
>> + struct cachefiles_ondemand_info *info;
>> char *pid, *psize;
>> unsigned long id;
>> long size;
>> @@ -185,6 +189,14 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>> goto out;
>> }
>>
>> + info = req->object->ondemand;
>> + spin_lock(&info->lock);
>> + /* The anonymous fd was closed before copen ? */
> I would like describe more details in the comment, e.g. put the time
> sequence described in the commit message here.
OK, thanks for your suggestion, I will describe it in more detail
in the next revision.
Thanks,
Baokun
>
>> + if (info->ondemand_id == CACHEFILES_ONDEMAND_ID_CLOSED) {
>> + spin_unlock(&info->lock);
>> + req->error = -EBADFD;
>> + goto out;
>> + }
>> cookie = req->object->cookie;
>> cookie->object_size = size;
>> if (size)
>> @@ -194,6 +206,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>> trace_cachefiles_ondemand_copen(req->object, id, size);
>>
>> cachefiles_ondemand_set_object_open(req->object);
>> + spin_unlock(&info->lock);
>> wake_up_all(&cache->daemon_pollwq);
>>
>> out:
>> @@ -596,6 +609,7 @@ int cachefiles_ondemand_init_obj_info(struct cachefiles_object *object,
>> return -ENOMEM;
>>
>> object->ondemand->object = object;
>> + spin_lock_init(&object->ondemand->lock);
>> INIT_WORK(&object->ondemand->ondemand_work, ondemand_object_worker);
>> return 0;
>> }
next prev parent reply other threads:[~2024-05-06 3:23 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 3:39 [PATCH 00/12] cachefiles: some bugfixes and cleanups for ondemand requests libaokun
2024-04-24 3:39 ` [PATCH 01/12] cachefiles: remove request from xarry during flush requests libaokun
2024-04-25 3:13 ` Jia Zhu
2024-05-06 3:48 ` Jingbo Xu
2024-05-06 3:57 ` Baokun Li
2024-05-06 5:50 ` Jingbo Xu
2024-05-07 6:52 ` Baokun Li
2024-04-24 3:39 ` [PATCH 02/12] cachefiles: remove err_put_fd tag in cachefiles_ondemand_daemon_read() libaokun
2024-04-25 3:17 ` Jia Zhu
2024-05-06 3:55 ` Jingbo Xu
2024-05-06 4:02 ` Baokun Li
2024-04-24 3:39 ` [PATCH 03/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_get_fd() libaokun
2024-04-24 14:55 ` Jia Zhu
2024-04-25 1:33 ` Baokun Li
2024-04-25 3:39 ` Jia Zhu
2024-04-24 3:39 ` [PATCH 04/12] cachefiles: fix slab-use-after-free in cachefiles_ondemand_daemon_read() libaokun
2024-04-25 3:42 ` [External] " Jia Zhu
2024-04-24 3:39 ` [PATCH 05/12] cachefiles: add output string to cachefiles_obj_[get|put]_ondemand_fd libaokun
2024-04-24 3:39 ` [PATCH 06/12] cachefiles: add consistency check for copen/cread libaokun
2024-05-06 2:31 ` Jingbo Xu
2024-05-06 3:12 ` Baokun Li
2024-04-24 3:39 ` [PATCH 07/12] cachefiles: add spin_lock for cachefiles_ondemand_info libaokun
2024-05-06 2:55 ` Jingbo Xu
2024-05-06 3:23 ` Baokun Li [this message]
2024-04-24 3:39 ` [PATCH 08/12] cachefiles: never get a new anon fd if ondemand_id is valid libaokun
2024-05-06 3:09 ` Jingbo Xu
2024-05-07 9:32 ` Baokun Li
2024-04-24 3:39 ` [PATCH 09/12] cachefiles: defer exposing anon_fd until after copy_to_user() succeeds libaokun
2024-05-06 3:24 ` Jingbo Xu
2024-05-06 3:34 ` Baokun Li
2024-04-24 3:39 ` [PATCH 10/12] cachefiles: Set object to close if ondemand_id < 0 in copen libaokun
2024-04-25 4:56 ` Jia Zhu
2024-04-24 3:39 ` [PATCH 11/12] cachefiles: flush all requests after setting CACHEFILES_DEAD libaokun
2024-04-24 3:39 ` [PATCH 12/12] cachefiles: make on-demand read killable libaokun
2024-04-25 5:15 ` Jia Zhu
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=18cc6974-b47d-f9fd-2576-366382d0e8d0@huaweicloud.com \
--to=libaokun@huaweicloud.com \
--cc=dhowells@redhat.com \
--cc=jefflexu@linux.alibaba.com \
--cc=jlayton@kernel.org \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netfs@lists.linux.dev \
--cc=yangerkun@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).