linux-kernel - Re: [PATCH v2 4/4] io_uring: get next req on subm ref drop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Mar 2020 09:54:03 +0300
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Jens Axboe <axboe@...nel.dk>, io-uring@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] io_uring: get next req on subm ref drop

On 03/03/2020 07:26, Jens Axboe wrote:
> On 3/2/20 1:45 PM, Pavel Begunkov wrote:
>> Get next request when dropping the submission reference. However, if
>> there is an asynchronous counterpart (i.e. read/write, timeout, etc),
>> that would be dangerous to do, so ignore them using new
>> REQ_F_DONT_STEAL_NEXT flag.
> 
> Hmm, not so sure I like this one. It's not quite clear to me where we
> need REQ_F_DONT_STEAL_NEXT. If we have an async component, then we set
> REQ_F_DONT_STEAL_NEXT. So this is generally the case where our
> io_put_req() for submit is not the last drop. And for the other case,
> the put is generally in the caller anyway. So I don't really see what
> this extra flag buys us?

Because io_put_work() holds a reference, no async handler can achive req->refs
== 0, so it won't return next upon dropping the submission ref (i.e. by
put_find_nxt()). And I want to have next before io_put_work(), to, instead of as
currently:

run_work(work);
assign_cur_work(NULL); // spinlock + unlock worker->lock
new_work = put_work(work);
assign_cur_work(new_work); // the second time

do:

new_work = run_work(work);
assign_cur_work(new_work); // need new_work here
put_work(work);


The other way:

io_wq_submit_work() // for all async handlers
{
	...
	// Drop submission reference.
	// One extra ref will be put in io_put_work() right
	// after return, and it'll be done in the same thread
	if (atomic_dec_and_get(req) == 1)
		steal_next(req);
}

Maybe cleaner, but looks fragile as well. Would you prefer it?

> Few more comments below.
> 
>> +static void io_put_req_async_submission(struct io_kiocb *req,
>> +					struct io_wq_work **workptr)
>> +{
>> +	static struct io_kiocb *nxt;
>> +
>> +	nxt = io_put_req_submission(req);
>> +	if (nxt)
>> +		io_wq_assign_next(workptr, nxt);
>> +}
> 
> This really should be called io_put_req_async_completion() since it's
> called on completion. The naming is confusing.

Ok

>> @@ -2581,14 +2598,11 @@ static void __io_fsync(struct io_kiocb *req)
>>  static void io_fsync_finish(struct io_wq_work **workptr)
>>  {
>>  	struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
>> -	struct io_kiocb *nxt = NULL;
>>  
>>  	if (io_req_cancelled(req))
>>  		return;
>>  	__io_fsync(req);
>> -	io_put_req(req); /* drop submission reference */
>> -	if (nxt)
>> -		io_wq_assign_next(workptr, nxt);
>> +	io_put_req_async_submission(req, workptr);
>>  }
>>  
>>  static int io_fsync(struct io_kiocb *req, bool force_nonblock)
>> @@ -2617,14 +2631,11 @@ static void __io_fallocate(struct io_kiocb *req)
>>  static void io_fallocate_finish(struct io_wq_work **workptr)
>>  {
>>  	struct io_kiocb *req = container_of(*workptr, struct io_kiocb, work);
>> -	struct io_kiocb *nxt = NULL;
>>  
>>  	if (io_req_cancelled(req))
>>  		return;
>>  	__io_fallocate(req);
>> -	io_put_req(req); /* drop submission reference */
>> -	if (nxt)
>> -		io_wq_assign_next(workptr, nxt);
>> +	io_put_req_async_submission(req, workptr);
>>  }
> 
> All of these cleanups are nice (except the naming, as mentioned).
> 
>> @@ -3943,7 +3947,10 @@ static int io_poll_add(struct io_kiocb *req)
>>  	if (mask) {
>>  		io_cqring_ev_posted(ctx);
>>  		io_put_req(req);
>> +	} else {
>> +		req->flags |= REQ_F_DONT_STEAL_NEXT;
>>  	}
>> +
>>  	return ipt.error;
>>  }
> 
> Is this racy? I guess it doesn't matter since we're still holding the
> completion reference.

It's done by the same thread, that uses it. There could be a race if the async
counterpart is going to change req->flags, but we tolerate false negative (i.e.
put_req() will handle it).

-- 
Pavel Begunkov