Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c6f5d729-2083-817d-fe7d-b01bce27e39f@bytedance.com>
Date:   Wed, 12 Oct 2022 23:37:31 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.2
Subject: Re: [External] Re: [PATCH 3/5] cachefiles: resend an open request if
 the read request's object is closed
To:     JeffleXu <jefflexu@linux.alibaba.com>, dhowells@redhat.com,
        xiang@kernel.org
Cc:     linux-cachefs@redhat.com, linux-erofs@lists.ozlabs.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        yinxin.x@bytedance.com
References: <20221011131552.23833-1-zhujia.zj@bytedance.com>
 <20221011131552.23833-4-zhujia.zj@bytedance.com>
 <28d64f00-e408-9fc2-9506-63c1d8b08b9c@linux.alibaba.com>
From:   Jia Zhu <zhujia.zj@bytedance.com>
In-Reply-To: <28d64f00-e408-9fc2-9506-63c1d8b08b9c@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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