From: Colin Ian King <[email protected]>
In the case where sig is NULL the error variable ret is not initialized
and may contain a garbage value on the final checks to see if ret is
-ERESTARTSYS. Best to initialize ret to zero before the do loop to
ensure the ret does not accidentially contain -ERESTARTSYS before the
loop.
Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: dd671c79e40b ("io_uring: make CQ ring wakeups be more efficient")
Signed-off-by: Colin Ian King <[email protected]>
---
fs/io_uring.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7b5710e3a18c..aa8ac557493c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2835,6 +2835,7 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
return ret;
}
+ ret = 0;
iowq.nr_timeouts = atomic_read(&ctx->cq_timeouts);
do {
prepare_to_wait_exclusive(&ctx->wait, &iowq.wq,
--
2.20.1
On 9/26/19 11:50 AM, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> In the case where sig is NULL the error variable ret is not initialized
> and may contain a garbage value on the final checks to see if ret is
> -ERESTARTSYS. Best to initialize ret to zero before the do loop to
> ensure the ret does not accidentially contain -ERESTARTSYS before the
> loop.
Oops, weird it didn't complain. I've folded in this fix, as that commit
isn't upstream yet. Thanks!
--
Jens Axboe
On Thu, Sep 26, 2019 at 11:56:30AM +0200, Jens Axboe wrote:
> On 9/26/19 11:50 AM, Colin King wrote:
> > From: Colin Ian King <[email protected]>
> >
> > In the case where sig is NULL the error variable ret is not initialized
> > and may contain a garbage value on the final checks to see if ret is
> > -ERESTARTSYS. Best to initialize ret to zero before the do loop to
> > ensure the ret does not accidentially contain -ERESTARTSYS before the
> > loop.
>
> Oops, weird it didn't complain. I've folded in this fix, as that commit
> isn't upstream yet. Thanks!
There is a bug in GCC where at certain optimization levels, instead of
complaining, it initializes it to zero.
regards,
dan carpenter
On 9/26/19 1:33 PM, Dan Carpenter wrote:
> On Thu, Sep 26, 2019 at 11:56:30AM +0200, Jens Axboe wrote:
>> On 9/26/19 11:50 AM, Colin King wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> In the case where sig is NULL the error variable ret is not initialized
>>> and may contain a garbage value on the final checks to see if ret is
>>> -ERESTARTSYS. Best to initialize ret to zero before the do loop to
>>> ensure the ret does not accidentially contain -ERESTARTSYS before the
>>> loop.
>>
>> Oops, weird it didn't complain. I've folded in this fix, as that commit
>> isn't upstream yet. Thanks!
>
> There is a bug in GCC where at certain optimization levels, instead of
> complaining, it initializes it to zero.
That's awfully nice of it ;-)
Tried with -O0 and still didn't complain for me.
$ gcc --version
gcc (Ubuntu 9.1.0-2ubuntu2~18.04) 9.1.0
Tried gcc 5/6/7/8 as well. Might have to go look at what code it's
generating.
--
Jens Axboe
On 26/09/2019 13.42, Jens Axboe wrote:
> On 9/26/19 1:33 PM, Dan Carpenter wrote:
>> On Thu, Sep 26, 2019 at 11:56:30AM +0200, Jens Axboe wrote:
>>> On 9/26/19 11:50 AM, Colin King wrote:
>>>> From: Colin Ian King <[email protected]>
>>>>
>>>> In the case where sig is NULL the error variable ret is not initialized
>>>> and may contain a garbage value on the final checks to see if ret is
>>>> -ERESTARTSYS. Best to initialize ret to zero before the do loop to
>>>> ensure the ret does not accidentially contain -ERESTARTSYS before the
>>>> loop.
>>>
>>> Oops, weird it didn't complain. I've folded in this fix, as that commit
>>> isn't upstream yet. Thanks!
>>
>> There is a bug in GCC where at certain optimization levels, instead of
>> complaining, it initializes it to zero.
>
> That's awfully nice of it ;-)
>
> Tried with -O0 and still didn't complain for me.
>
> $ gcc --version
> gcc (Ubuntu 9.1.0-2ubuntu2~18.04) 9.1.0
>
> Tried gcc 5/6/7/8 as well. Might have to go look at what code it's
> generating.
>
I think it's essentially the same as
https://lore.kernel.org/lkml/CAHk-=whP-9yPAWuJDwA6+rQ-9owuYZgmrMA9AqO3EGJVefe8vg@mail.gmail.com/
(thread "tmpfs: fix uninitialized return value in shmem_link").
Rasmus
On 9/26/19 2:14 PM, Rasmus Villemoes wrote:
> On 26/09/2019 13.42, Jens Axboe wrote:
>> On 9/26/19 1:33 PM, Dan Carpenter wrote:
>>> On Thu, Sep 26, 2019 at 11:56:30AM +0200, Jens Axboe wrote:
>>>> On 9/26/19 11:50 AM, Colin King wrote:
>>>>> From: Colin Ian King <[email protected]>
>>>>>
>>>>> In the case where sig is NULL the error variable ret is not initialized
>>>>> and may contain a garbage value on the final checks to see if ret is
>>>>> -ERESTARTSYS. Best to initialize ret to zero before the do loop to
>>>>> ensure the ret does not accidentially contain -ERESTARTSYS before the
>>>>> loop.
>>>>
>>>> Oops, weird it didn't complain. I've folded in this fix, as that commit
>>>> isn't upstream yet. Thanks!
>>>
>>> There is a bug in GCC where at certain optimization levels, instead of
>>> complaining, it initializes it to zero.
>>
>> That's awfully nice of it ;-)
>>
>> Tried with -O0 and still didn't complain for me.
>>
>> $ gcc --version
>> gcc (Ubuntu 9.1.0-2ubuntu2~18.04) 9.1.0
>>
>> Tried gcc 5/6/7/8 as well. Might have to go look at what code it's
>> generating.
>>
>
> I think it's essentially the same as
> https://lore.kernel.org/lkml/CAHk-=whP-9yPAWuJDwA6+rQ-9owuYZgmrMA9AqO3EGJVefe8vg@mail.gmail.com/
> (thread "tmpfs: fix uninitialized return value in shmem_link").
I think you're right, it's the same pattern. If I kill the:
if (ret)
return ret;
inside the if (sig) branch, then gcc does show the warning as it should.
--
Jens Axboe