在 2022/10/12 15:53, JeffleXu 写道:
On 10/11/22 9:15 PM, Jia Zhu wrote:
@@ -254,12 +282,18 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
* request distribution fair.
*/
xa_lock(&cache->reqs);
- req = xas_find_marked(&xas, UINT_MAX, CACHEFILES_REQ_NEW);
- if (!req && cache->req_id_next > 0) {
- xas_set(&xas, 0);
- req = xas_find_marked(&xas, cache->req_id_next - 1, CACHEFILES_REQ_NEW);
+retry:
+ xas_for_each_marked(&xas, req, xa_max, CACHEFILES_REQ_NEW) {
+ if (cachefiles_ondemand_skip_req(req))
+ continue;
+ break;
}
if (!req) {
+ if (cache->req_id_next > 0 && xa_max == ULONG_MAX) {
+ xas_set(&xas, 0);
+ xa_max = cache->req_id_next - 1;
+ goto retry;
+ }
I would suggest abstracting the "xas_for_each_marked(...,
CACHEFILES_REQ_NEW)" part into a helper function to avoid the "goto retry".
Hi JingBo,
Thanks for your advice. Are the following revises appropriate？
static struct cachefiles_req *cachefiles_ondemand_select_req(struct
xa_state *xas, unsigned long xa_max)
{
struct cachefiles_req *req;
struct cachefiles_ondemand_info *info;
xas_for_each_marked(xas, req, xa_max, CACHEFILES_REQ_NEW) {
if (!req || req->msg.opcode != CACHEFILES_OP_READ)
return req;
info = req->object->private;
if (info->state == CACHEFILES_ONDEMAND_OBJSTATE_close) {
cachefiles_ondemand_set_object_reopening(req->object);
queue_work(fscache_wq, &info->work);
continue;
} else if (info->state == CACHEFILES_ONDEMAND_OBJSTATE_reopening) {
continue;
}
return req;
}
return NULL;
}
...
xa_lock(&cache->reqs);
req = cachefiles_ondemand_select_req(&xas, ULONG_MAX);
if (!req && cache->req_id_next > 0) {
xas_set(&xas, 0);
req = cachefiles_ondemand_select_req(&xas, cache->req_id_next - 1);
}
if (!req) {
xa_unlock(&cache->reqs);
return 0;
}
@@ -392,8 +434,16 @@ static int cachefiles_ondemand_send_req(struct cachefiles_object *object,
wake_up_all(&cache->daemon_pollwq);
wait_for_completion(&req->done);
ret = req->error;
+ kfree(req);
+ return ret;
out:
kfree(req);
+ /* Reset the object to close state in error handling path.
+ * If error occurs after creating the anonymous fd,
+ * cachefiles_ondemand_fd_release() will set object to close.
+ */
+ if (opcode == CACHEFILES_OP_OPEN)
+ cachefiles_ondemand_set_object_close(req->object);
This may cause use-after-free since @req has been freed.
Thanks for catching this, I'll fix it in next version.
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
