* [bug report] io_uring/poll: get rid of unlocked cancel hash
@ 2024-10-04 9:00 Dan Carpenter
2024-10-04 13:50 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-10-04 9:00 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
Hello Jens Axboe,
Commit 313314db5bcb ("io_uring/poll: get rid of unlocked cancel
hash") from Sep 30, 2024 (linux-next), leads to the following Smatch
static checker warning:
io_uring/poll.c:932 io_poll_remove()
warn: duplicate check 'ret2' (previous on line 930)
io_uring/poll.c
919 int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
920 {
921 struct io_poll_update *poll_update = io_kiocb_to_cmd(req, struct io_poll_update);
922 struct io_ring_ctx *ctx = req->ctx;
923 struct io_cancel_data cd = { .ctx = ctx, .data = poll_update->old_user_data, };
924 struct io_kiocb *preq;
925 int ret2, ret = 0;
926
927 io_ring_submit_lock(ctx, issue_flags);
928 preq = io_poll_find(ctx, true, &cd);
929 ret2 = io_poll_disarm(preq);
930 if (!ret2)
931 goto found;
--> 932 if (ret2) {
933 ret = ret2;
934 goto out;
935 }
A lot of the function is dead code now. ;)
936 found:
937 if (WARN_ON_ONCE(preq->opcode != IORING_OP_POLL_ADD)) {
938 ret = -EFAULT;
939 goto out;
940 }
941
942 if (poll_update->update_events || poll_update->update_user_data) {
943 /* only mask one event flags, keep behavior flags */
944 if (poll_update->update_events) {
945 struct io_poll *poll = io_kiocb_to_cmd(preq, struct io_poll);
946
947 poll->events &= ~0xffff;
948 poll->events |= poll_update->events & 0xffff;
949 poll->events |= IO_POLL_UNMASK;
950 }
951 if (poll_update->update_user_data)
952 preq->cqe.user_data = poll_update->new_user_data;
953
954 ret2 = io_poll_add(preq, issue_flags & ~IO_URING_F_UNLOCKED);
955 /* successfully updated, don't complete poll request */
956 if (!ret2 || ret2 == -EIOCBQUEUED)
957 goto out;
958 }
959
960 req_set_fail(preq);
961 io_req_set_res(preq, -ECANCELED, 0);
962 preq->io_task_work.func = io_req_task_complete;
963 io_req_task_work_add(preq);
964 out:
965 io_ring_submit_unlock(ctx, issue_flags);
966 if (ret < 0) {
967 req_set_fail(req);
968 return ret;
969 }
970 /* complete update request, we're done with it */
971 io_req_set_res(req, ret, 0);
972 return IOU_OK;
973 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] io_uring/poll: get rid of unlocked cancel hash
2024-10-04 9:00 [bug report] io_uring/poll: get rid of unlocked cancel hash Dan Carpenter
@ 2024-10-04 13:50 ` Jens Axboe
2024-10-04 13:54 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2024-10-04 13:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
On 10/4/24 3:00 AM, Dan Carpenter wrote:
> Hello Jens Axboe,
>
> Commit 313314db5bcb ("io_uring/poll: get rid of unlocked cancel
> hash") from Sep 30, 2024 (linux-next), leads to the following Smatch
> static checker warning:
>
> io_uring/poll.c:932 io_poll_remove()
> warn: duplicate check 'ret2' (previous on line 930)
>
> io_uring/poll.c
> 919 int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
> 920 {
> 921 struct io_poll_update *poll_update = io_kiocb_to_cmd(req, struct io_poll_update);
> 922 struct io_ring_ctx *ctx = req->ctx;
> 923 struct io_cancel_data cd = { .ctx = ctx, .data = poll_update->old_user_data, };
> 924 struct io_kiocb *preq;
> 925 int ret2, ret = 0;
> 926
> 927 io_ring_submit_lock(ctx, issue_flags);
> 928 preq = io_poll_find(ctx, true, &cd);
> 929 ret2 = io_poll_disarm(preq);
> 930 if (!ret2)
> 931 goto found;
> --> 932 if (ret2) {
> 933 ret = ret2;
> 934 goto out;
> 935 }
>
> A lot of the function is dead code now. ;)
Thanks, will revisit and fold in a fix!
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] io_uring/poll: get rid of unlocked cancel hash
2024-10-04 13:50 ` Jens Axboe
@ 2024-10-04 13:54 ` Jens Axboe
2024-10-04 14:54 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2024-10-04 13:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
On 10/4/24 7:50 AM, Jens Axboe wrote:
> On 10/4/24 3:00 AM, Dan Carpenter wrote:
>> Hello Jens Axboe,
>>
>> Commit 313314db5bcb ("io_uring/poll: get rid of unlocked cancel
>> hash") from Sep 30, 2024 (linux-next), leads to the following Smatch
>> static checker warning:
>>
>> io_uring/poll.c:932 io_poll_remove()
>> warn: duplicate check 'ret2' (previous on line 930)
>>
>> io_uring/poll.c
>> 919 int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
>> 920 {
>> 921 struct io_poll_update *poll_update = io_kiocb_to_cmd(req, struct io_poll_update);
>> 922 struct io_ring_ctx *ctx = req->ctx;
>> 923 struct io_cancel_data cd = { .ctx = ctx, .data = poll_update->old_user_data, };
>> 924 struct io_kiocb *preq;
>> 925 int ret2, ret = 0;
>> 926
>> 927 io_ring_submit_lock(ctx, issue_flags);
>> 928 preq = io_poll_find(ctx, true, &cd);
>> 929 ret2 = io_poll_disarm(preq);
>> 930 if (!ret2)
>> 931 goto found;
>> --> 932 if (ret2) {
>> 933 ret = ret2;
>> 934 goto out;
>> 935 }
>>
>> A lot of the function is dead code now. ;)
>
> Thanks, will revisit and fold in a fix!
Should just need this incremental. There's no dead code as far as I can
see, just a needless found label and jump.
diff --git a/io_uring/poll.c b/io_uring/poll.c
index 69382da48c00..217d667e0622 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -940,13 +940,10 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
ret2 = io_poll_disarm(preq);
if (bucket)
spin_unlock(&bucket->lock);
- if (!ret2)
- goto found;
if (ret2) {
ret = ret2;
goto out;
}
-found:
if (WARN_ON_ONCE(preq->opcode != IORING_OP_POLL_ADD)) {
ret = -EFAULT;
goto out;
--
Jens Axboe
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [bug report] io_uring/poll: get rid of unlocked cancel hash
2024-10-04 13:54 ` Jens Axboe
@ 2024-10-04 14:54 ` Dan Carpenter
2024-10-04 15:03 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2024-10-04 14:54 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
On Fri, Oct 04, 2024 at 07:54:32AM -0600, Jens Axboe wrote:
> On 10/4/24 7:50 AM, Jens Axboe wrote:
> > On 10/4/24 3:00 AM, Dan Carpenter wrote:
> >> Hello Jens Axboe,
> >>
> >> Commit 313314db5bcb ("io_uring/poll: get rid of unlocked cancel
> >> hash") from Sep 30, 2024 (linux-next), leads to the following Smatch
> >> static checker warning:
> >>
> >> io_uring/poll.c:932 io_poll_remove()
> >> warn: duplicate check 'ret2' (previous on line 930)
> >>
> >> io_uring/poll.c
> >> 919 int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
> >> 920 {
> >> 921 struct io_poll_update *poll_update = io_kiocb_to_cmd(req, struct io_poll_update);
> >> 922 struct io_ring_ctx *ctx = req->ctx;
> >> 923 struct io_cancel_data cd = { .ctx = ctx, .data = poll_update->old_user_data, };
> >> 924 struct io_kiocb *preq;
> >> 925 int ret2, ret = 0;
> >> 926
> >> 927 io_ring_submit_lock(ctx, issue_flags);
> >> 928 preq = io_poll_find(ctx, true, &cd);
> >> 929 ret2 = io_poll_disarm(preq);
> >> 930 if (!ret2)
> >> 931 goto found;
> >> --> 932 if (ret2) {
> >> 933 ret = ret2;
> >> 934 goto out;
> >> 935 }
> >>
> >> A lot of the function is dead code now. ;)
> >
> > Thanks, will revisit and fold in a fix!
>
> Should just need this incremental. There's no dead code as far as I can
> see, just a needless found label and jump.
>
> diff --git a/io_uring/poll.c b/io_uring/poll.c
> index 69382da48c00..217d667e0622 100644
> --- a/io_uring/poll.c
> +++ b/io_uring/poll.c
> @@ -940,13 +940,10 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
> ret2 = io_poll_disarm(preq);
> if (bucket)
> spin_unlock(&bucket->lock);
> - if (!ret2)
> - goto found;
Oh. I thought this was a goto out. That explains how the code was passing
tests. That was an easy fix.
regards,
dan carpenter
> if (ret2) {
> ret = ret2;
> goto out;
> }
> -found:
> if (WARN_ON_ONCE(preq->opcode != IORING_OP_POLL_ADD)) {
> ret = -EFAULT;
> goto out;
>
> --
> Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] io_uring/poll: get rid of unlocked cancel hash
2024-10-04 14:54 ` Dan Carpenter
@ 2024-10-04 15:03 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2024-10-04 15:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
On 10/4/24 8:54 AM, Dan Carpenter wrote:
> On Fri, Oct 04, 2024 at 07:54:32AM -0600, Jens Axboe wrote:
>> On 10/4/24 7:50 AM, Jens Axboe wrote:
>>> On 10/4/24 3:00 AM, Dan Carpenter wrote:
>>>> Hello Jens Axboe,
>>>>
>>>> Commit 313314db5bcb ("io_uring/poll: get rid of unlocked cancel
>>>> hash") from Sep 30, 2024 (linux-next), leads to the following Smatch
>>>> static checker warning:
>>>>
>>>> io_uring/poll.c:932 io_poll_remove()
>>>> warn: duplicate check 'ret2' (previous on line 930)
>>>>
>>>> io_uring/poll.c
>>>> 919 int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
>>>> 920 {
>>>> 921 struct io_poll_update *poll_update = io_kiocb_to_cmd(req, struct io_poll_update);
>>>> 922 struct io_ring_ctx *ctx = req->ctx;
>>>> 923 struct io_cancel_data cd = { .ctx = ctx, .data = poll_update->old_user_data, };
>>>> 924 struct io_kiocb *preq;
>>>> 925 int ret2, ret = 0;
>>>> 926
>>>> 927 io_ring_submit_lock(ctx, issue_flags);
>>>> 928 preq = io_poll_find(ctx, true, &cd);
>>>> 929 ret2 = io_poll_disarm(preq);
>>>> 930 if (!ret2)
>>>> 931 goto found;
>>>> --> 932 if (ret2) {
>>>> 933 ret = ret2;
>>>> 934 goto out;
>>>> 935 }
>>>>
>>>> A lot of the function is dead code now. ;)
>>>
>>> Thanks, will revisit and fold in a fix!
>>
>> Should just need this incremental. There's no dead code as far as I can
>> see, just a needless found label and jump.
>>
>> diff --git a/io_uring/poll.c b/io_uring/poll.c
>> index 69382da48c00..217d667e0622 100644
>> --- a/io_uring/poll.c
>> +++ b/io_uring/poll.c
>> @@ -940,13 +940,10 @@ int io_poll_remove(struct io_kiocb *req, unsigned int issue_flags)
>> ret2 = io_poll_disarm(preq);
>> if (bucket)
>> spin_unlock(&bucket->lock);
>> - if (!ret2)
>> - goto found;
>
> Oh. I thought this was a goto out. That explains how the code was passing
> tests. That was an easy fix.
Yeah, that check was just dead code, but the rest was fine. Thanks for
letting me know!
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-04 15:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-04 9:00 [bug report] io_uring/poll: get rid of unlocked cancel hash Dan Carpenter
2024-10-04 13:50 ` Jens Axboe
2024-10-04 13:54 ` Jens Axboe
2024-10-04 14:54 ` Dan Carpenter
2024-10-04 15:03 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox