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

[Fedor Pchelkin]

On Sat, 02. May 00:50, Fedor Pchelkin wrote:
> On Fri, 01. May 15:33, Jens Axboe wrote:
> > >> @@ -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);
> > 
> > This should all be handled in the fixup patch - yes this one ended up
> > being broken, but that's why there's the followup fix.
> > 
> > Now this is all pretty broken because some patches ended up in 5.15 and
> > some in 5.10 and honestly I've almost lost track at this point. Sasha
> > spotted some that were dropped in some broken commit from Greg. For
> > 5.15, the two attached are what I recently asked for to be added. 5.10
> > should ALWAYS get the exact same patches as 5.15, because of the whole
> > sale backport that was done years ago. I always ask for that explicitly
> > in the emails. But looks like that wasn't always done...
> > 
> > 5.10 doesn't look like it ever got what is sha
> > 349ef5d2e7bfb292e7000e6041a984ab56eccf28 in 5.15-stable, hence the fixup
> > can be merged with queueing that backport.
> > 
> > Sigh...
> 
> Oh, for the fixup patch - it is in the 5.10-queue (or at least was when I
> wrote up report today).  It was taken into consideration.  The concern
> for the Fixes tag is resolved now by you, thanks for clarification.
> 
> But that `if (preq->result) < 0` thing is not covered by the fixup patch.
> This check is not correct in 5.10/5.15 because preq->result is unsigned.
> 
> Taken that double complete is OK (I had doubts about that), the following
> change should do the job now:
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 8b0dfea96ee0..b17a26b1b5e1 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -6006,7 +6006,7 @@ static int io_poll_update(struct io_kiocb *req, unsigned int issue_flags)
>  {
>         struct io_ring_ctx *ctx = req->ctx;
>         struct io_kiocb *preq;
> -       int ret2, ret = 0;
> +       int ret2 = -ECANCELED, ret = 0;
>  
>         io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
>  
> @@ -6037,7 +6037,7 @@ static int io_poll_update(struct io_kiocb *req, unsigned int issue_flags)
>                 preq->result = ret2;
>  
>         }
> -       if (preq->result < 0)
> +       if (ret2 < 0)
>                 req_set_fail(preq);
>         io_req_complete(preq, preq->result);
>  out:
> 
> 
> The fixup patch may be updated with this if the changes look OK.
> 

Or maybe this one which is less hassle:

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 8b0dfea96ee0..bb01eaa9761f 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -6037,7 +6037,7 @@ static int io_poll_update(struct io_kiocb *req, unsigned int issue_flags)
                preq->result = ret2;
 
        }
-       if (preq->result < 0)
+       if (preq->result)
                req_set_fail(preq);
        io_req_complete(preq, preq->result);
 out:

--
Thanks,
Fedor