Re: [External] Re: [PATCH 3/5] cachefiles: resend an open request if the read request's object is closed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 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.






[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux