| Message ID | 20240515084601.3240503-2-libaokun@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cachefiles: some bugfixes and cleanups for ondemand requests | expand |
On 2024/5/15 16:45, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> The subject line can be "cachefiles: remove requests from xarray during flushing requests" > > Even with CACHEFILES_DEAD set, we can still read the requests, so in the > following concurrency the request may be used after it has been freed: > > mount | daemon_thread1 | daemon_thread2 > ------------------------------------------------------------ > cachefiles_ondemand_init_object > cachefiles_ondemand_send_req > REQ_A = kzalloc(sizeof(*req) + data_len) > wait_for_completion(&REQ_A->done) > cachefiles_daemon_read > cachefiles_ondemand_daemon_read > // close dev fd > cachefiles_flush_reqs > complete(&REQ_A->done) > kfree(REQ_A) > xa_lock(&cache->reqs); > cachefiles_ondemand_select_req > req->msg.opcode != CACHEFILES_OP_READ > // req use-after-free !!! > xa_unlock(&cache->reqs); > xa_destroy(&cache->reqs) > > Hence remove requests from cache->reqs when flushing them to avoid > accessing freed requests. > > Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie") > Signed-off-by: Baokun Li <libaokun1@huawei.com> > Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com> Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> Thanks, Gao Xiang > --- > fs/cachefiles/daemon.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c > index 6465e2574230..ccb7b707ea4b 100644 > --- a/fs/cachefiles/daemon.c > +++ b/fs/cachefiles/daemon.c > @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache) > xa_for_each(xa, index, req) { > req->error = -EIO; > complete(&req->done); > + __xa_erase(xa, index); > } > xa_unlock(xa); >
On 2024/5/20 10:20, Gao Xiang wrote: > > > On 2024/5/15 16:45, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> > > > The subject line can be > "cachefiles: remove requests from xarray during flushing requests" Thank you very much for the correction! I will update my patch. > >> >> Even with CACHEFILES_DEAD set, we can still read the requests, so in the >> following concurrency the request may be used after it has been freed: >> >> mount | daemon_thread1 | daemon_thread2 >> ------------------------------------------------------------ >> cachefiles_ondemand_init_object >> cachefiles_ondemand_send_req >> REQ_A = kzalloc(sizeof(*req) + data_len) >> wait_for_completion(&REQ_A->done) >> cachefiles_daemon_read >> cachefiles_ondemand_daemon_read >> // close dev fd >> cachefiles_flush_reqs >> complete(&REQ_A->done) >> kfree(REQ_A) >> xa_lock(&cache->reqs); >> cachefiles_ondemand_select_req >> req->msg.opcode != CACHEFILES_OP_READ >> // req use-after-free !!! >> xa_unlock(&cache->reqs); >> xa_destroy(&cache->reqs) >> >> Hence remove requests from cache->reqs when flushing them to avoid >> accessing freed requests. >> >> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking >> up cookie") >> Signed-off-by: Baokun Li <libaokun1@huawei.com> >> Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com> > > Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com> > > Thanks, > Gao Xiang Thanks a lot for the review! Cheers, Baokun > >> --- >> fs/cachefiles/daemon.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c >> index 6465e2574230..ccb7b707ea4b 100644 >> --- a/fs/cachefiles/daemon.c >> +++ b/fs/cachefiles/daemon.c >> @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct >> cachefiles_cache *cache) >> xa_for_each(xa, index, req) { >> req->error = -EIO; >> complete(&req->done); >> + __xa_erase(xa, index); >> } >> xa_unlock(xa);
On 5/15/24 4:45 PM, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > Even with CACHEFILES_DEAD set, we can still read the requests, so in the > following concurrency the request may be used after it has been freed: > > mount | daemon_thread1 | daemon_thread2 > ------------------------------------------------------------ > cachefiles_ondemand_init_object > cachefiles_ondemand_send_req > REQ_A = kzalloc(sizeof(*req) + data_len) > wait_for_completion(&REQ_A->done) > cachefiles_daemon_read > cachefiles_ondemand_daemon_read > // close dev fd > cachefiles_flush_reqs > complete(&REQ_A->done) > kfree(REQ_A) > xa_lock(&cache->reqs); > cachefiles_ondemand_select_req > req->msg.opcode != CACHEFILES_OP_READ > // req use-after-free !!! > xa_unlock(&cache->reqs); > xa_destroy(&cache->reqs) > > Hence remove requests from cache->reqs when flushing them to avoid > accessing freed requests. > > Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie") > Signed-off-by: Baokun Li <libaokun1@huawei.com> > Reviewed-by: Jia Zhu <zhujia.zj@bytedance.com> Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com> > --- > fs/cachefiles/daemon.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c > index 6465e2574230..ccb7b707ea4b 100644 > --- a/fs/cachefiles/daemon.c > +++ b/fs/cachefiles/daemon.c > @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache) > xa_for_each(xa, index, req) { > req->error = -EIO; > complete(&req->done); > + __xa_erase(xa, index); > } > xa_unlock(xa); >
diff --git a/fs/cachefiles/daemon.c b/fs/cachefiles/daemon.c index 6465e2574230..ccb7b707ea4b 100644 --- a/fs/cachefiles/daemon.c +++ b/fs/cachefiles/daemon.c @@ -159,6 +159,7 @@ static void cachefiles_flush_reqs(struct cachefiles_cache *cache) xa_for_each(xa, index, req) { req->error = -EIO; complete(&req->done); + __xa_erase(xa, index); } xa_unlock(xa);