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

[JeffleXu <jefflexu@linux.alibaba.com>]

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


> @@ -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,
Jingbo