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, yangerkun <yangerkun@huawei.com>,
libaokun@huaweicloud.com
Subject: Re: [PATCH 08/12] cachefiles: never get a new anon fd if ondemand_id is valid
Date: Tue, 7 May 2024 17:32:37 +0800 [thread overview]
Message-ID: <e0c5708c-67c0-770b-6dd4-d85102bf6600@huaweicloud.com> (raw)
In-Reply-To: <625acc9e-b871-4912-965e-82fe3f9228d7@linux.alibaba.com>
Hi Jingbo,
On 2024/5/6 11:09, Jingbo Xu wrote:
>
> On 4/24/24 11:39 AM, libaokun@huaweicloud.com wrote:
>> From: Baokun Li <libaokun1@huawei.com>
>>
>> Now every time the daemon reads an open request, it requests a new anon fd
>> and ondemand_id. With the introduction of "restore", it is possible to read
>> the same open request more than once, and therefore have multiple anon fd's
>> for the same object.
>>
>> To avoid this, allocate a new anon fd only if no anon fd has been allocated
>> (ondemand_id == 0) or if the previously allocated anon fd has been closed
>> (ondemand_id == -1). Returns an error if ondemand_id is valid, letting the
>> daemon know that the current userland restore logic is abnormal and needs
>> to be checked.
> I have no obvious preference on strengthening this on kernel side or
> not. Could you explain more about what will happen if the daemon gets
> several distinct anon fd corresponding to one same object? IMHO the
> daemon should expect the side effect if it issues a 'restore' command
> when the daemon doesn't crash. IOW, it's something that shall be fixed
> or managed either on the kernel side, or on the daemon side.
If the anon_fd is not unique, the daemon will only close the anon_fd
corresponding to the newest object_id during drop_object, and the
other anon_fds will not be closed until the daemon exits.
However, the anon_fd holds the reference count of the object, so the
object will not be freed, and the cookie will also not be freed. So
mounting a same-named image at this point will cause a hung task
in fscache_hash_cookie() by waiting for the cookie to unhash.
The object_id and anon_fd of an object are supposed to be unique
under normal circumstances, this patch just provides that guarantee
even in the case of an exception.
Thank you very much for the review!
Regards,
Baokun
>> ---
>> fs/cachefiles/ondemand.c | 34 ++++++++++++++++++++++++++++------
>> 1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index b5e6a851ef04..0cf63bfedc9e 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -14,11 +14,18 @@ static int cachefiles_ondemand_fd_release(struct inode *inode,
>> struct file *file)
>> {
>> struct cachefiles_object *object = file->private_data;
>> - struct cachefiles_cache *cache = object->volume->cache;
>> - struct cachefiles_ondemand_info *info = object->ondemand;
>> + struct cachefiles_cache *cache;
>> + struct cachefiles_ondemand_info *info;
>> int object_id;
>> struct cachefiles_req *req;
>> - XA_STATE(xas, &cache->reqs, 0);
>> + XA_STATE(xas, NULL, 0);
>> +
>> + if (!object)
>> + return 0;
>> +
>> + info = object->ondemand;
>> + cache = object->volume->cache;
>> + xas.xa = &cache->reqs;
>>
>> xa_lock(&cache->reqs);
>> spin_lock(&info->lock);
>> @@ -269,22 +276,39 @@ static int cachefiles_ondemand_get_fd(struct cachefiles_req *req)
>> goto err_put_fd;
>> }
>>
>> + spin_lock(&object->ondemand->lock);
>> + if (object->ondemand->ondemand_id > 0) {
>> + spin_unlock(&object->ondemand->lock);
>> + ret = -EEXIST;
>> + /* Avoid performing cachefiles_ondemand_fd_release(). */
>> + file->private_data = NULL;
>> + goto err_put_file;
>> + }
>> +
>> file->f_mode |= FMODE_PWRITE | FMODE_LSEEK;
>> fd_install(fd, file);
>>
>> load = (void *)req->msg.data;
>> load->fd = fd;
>> object->ondemand->ondemand_id = object_id;
>> + spin_unlock(&object->ondemand->lock);
>>
>> cachefiles_get_unbind_pincount(cache);
>> trace_cachefiles_ondemand_open(object, &req->msg, load);
>> return 0;
>>
>> +err_put_file:
>> + fput(file);
>> err_put_fd:
>> put_unused_fd(fd);
>> err_free_id:
>> xa_erase(&cache->ondemand_ids, object_id);
>> err:
>> + spin_lock(&object->ondemand->lock);
>> + /* Avoid marking an opened object as closed. */
>> + if (object->ondemand->ondemand_id <= 0)
>> + cachefiles_ondemand_set_object_close(object);
>> + spin_unlock(&object->ondemand->lock);
>> cachefiles_put_object(object, cachefiles_obj_put_ondemand_fd);
>> return ret;
>> }
>> @@ -367,10 +391,8 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>>
>> if (msg->opcode == CACHEFILES_OP_OPEN) {
>> ret = cachefiles_ondemand_get_fd(req);
>> - if (ret) {
>> - cachefiles_ondemand_set_object_close(req->object);
>> + if (ret)
>> goto out;
>> - }
>> }
>>
>> msg->msg_id = xas.xa_index;
next prev parent reply other threads:[~2024-05-07 9:32 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
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 [this message]
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=e0c5708c-67c0-770b-6dd4-d85102bf6600@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).