[lvc-project] [PATCH 5.10 491/491] io_uring/poll: correctly handle io_poll_add() return value on update

[Fedor Pchelkin]

Hi Jens,

the patch has some issues even after "[PATCH 2/2] io_uring/poll: fix
backport of io_poll_add() changes" has been applied.  Please see below.

Jens Axboe wrote:
> From: Jens Axboe <axboe at kernel.dk>
> 
> Commit 84230ad2d2afbf0c44c32967e525c0ad92e26b4e upstream.
> 
> When the core of io_uring was updated to handle completions
> consistently and with fixed return codes, the POLL_REMOVE opcode
> with updates got slightly broken. If a POLL_ADD is pending and
> then POLL_REMOVE is used to update the events of that request, if that
> update causes the POLL_ADD to now trigger, then that completion is lost
> and a CQE is never posted.
> 
> Additionally, ensure that if an update does cause an existing POLL_ADD
> to complete, that the completion value isn't always overwritten with
> -ECANCELED. For that case, whatever io_poll_add() set the value to
> should just be retained.
> 
> Cc: stable at vger.kernel.org
> Fixes: 97b388d70b53 ("io_uring: handle completions in the core")

This commit is not present in 5.10/5.15 in any form, to my mind.  That
is, io_uring changes were imported in big chunks there without preserving
upstream git history in some places but still I can't find whether the
changes of the mentioned Fixes-commit are present in those old kernels.

So either the Fixes tag is not completely correct or this patch can just
be reverted from 5.10/5.15 stables.

> Reported-by: syzbot+641eec6b7af1f62f2b99 at syzkaller.appspotmail.com
> Tested-by: syzbot+641eec6b7af1f62f2b99 at syzkaller.appspotmail.com
> Signed-off-by: Jens Axboe <axboe at kernel.dk>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> ---
>  io_uring/io_uring.c |   26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -5980,7 +5980,7 @@ static int io_poll_add_prep(struct io_ki
>  	return 0;
>  }
>  
> -static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
> +static int __io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
>  {
>  	struct io_poll_iocb *poll = &req->poll;
>  	struct io_poll_table ipt;
> @@ -5992,11 +5992,21 @@ static int io_poll_add(struct io_kiocb *
>  	if (!ret && ipt.error)
>  		req_set_fail(req);
>  	ret = ret ?: ipt.error;
> -	if (ret)
> +	if (ret > 0) {
>  		__io_req_complete(req, issue_flags, ret, 0);
> +		return ret;
> +	}
>  	return 0;
>  }
>  
> +static int io_poll_add(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	int ret;
> +
> +	ret = __io_poll_add(req, issue_flags);
> +	return ret < 0 ? ret : 0;
> +}
> +
>  static int io_poll_update(struct io_kiocb *req, unsigned int issue_flags)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
> @@ -6012,6 +6022,7 @@ static int io_poll_update(struct io_kioc
>  		ret = preq ? -EALREADY : -ENOENT;
>  		goto out;
>  	}
> +	preq->result = -ECANCELED;
>  	spin_unlock(&ctx->completion_lock);
>  
>  	if (req->poll_update.update_events || req->poll_update.update_user_data) {
> @@ -6024,16 +6035,17 @@ static int io_poll_update(struct io_kioc
>  		if (req->poll_update.update_user_data)
>  			preq->user_data = req->poll_update.new_user_data;
>  
> -		ret2 = io_poll_add(preq, issue_flags);
> +		ret2 = __io_poll_add(preq, issue_flags);
>  		/* successfully updated, don't complete poll request */
>  		if (!ret2)
>  			goto out;
> +		preq->result = ret2;
> +
>  	}
> -	req_set_fail(preq);
> -	io_req_complete(preq, -ECANCELED);
> +	if (preq->result < 0)
> +		req_set_fail(preq);
> +	io_req_complete(preq, preq->result);

preq->result is of unsigned type in 5.10/5.15 kernels so the check for
negative values is a no-op here.  Also as Ben pointed out in the initial
report, __io_poll_add() already does complete a request if it returns
non-zero result.  Not sure if completing it twice is good.  The extra
patch from this thread doesn't address these issues.

I wonder whether these lines may be moved in the else-branch here like

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8b0dfea96ee0..1e3835bdaa9f 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -6036,10 +6036,10 @@ static int io_poll_update(struct io_kiocb *req, unsigned int issue_flags)
                        goto out;
                preq->result = ret2;
 
-       }
-       if (preq->result < 0)
+       } else {
                req_set_fail(preq);
-       io_req_complete(preq, preq->result);
+               io_req_complete(preq, preq->result);
+       }
 out:
        /* complete update request, we're done with it */
        io_req_complete(req, ret);


but, again, then the __io_poll_add() surrounding logic doesn't become
clear enough:

	ret2 = __io_poll_add(preq, issue_flags);
	/* successfully updated, don't complete poll request */
	if (!ret2)
		goto out;
	preq->result = ret2;


Thus currently I'm for reverting this patch if there is no bug it might
fix in 5.10/5.15.


Found by Linux Verification Center (linuxtesting.org) with Svace static
analysis tool.

--
Thanks,
Fedor