[next] io_uring: ensure variable ret is initialized to zero

Commit Message

Colin King Sept. 26, 2019, 9:50 a.m. UTC

From: Colin Ian King <[e-mail]>

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 <[e-mail]>
---
 fs/io_uring.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jens Axboe Sept. 26, 2019, 9:56 a.m. UTC | #1

On 9/26/19 11:50 AM, Colin King wrote:
> From: Colin Ian King <[e-mail]>
> 
> 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!

Dan Carpenter Sept. 26, 2019, 11:33 a.m. UTC | #2

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 <[e-mail]>
> > 
> > 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

Jens Axboe Sept. 26, 2019, 11:42 a.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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.

Rasmus Villemoes Sept. 26, 2019, 12:14 p.m. UTC | #4

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 <[e-mail]>
>>>>
>>>> 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-=[e-mail]/
(thread "tmpfs: fix uninitialized return value in shmem_link").

Rasmus

Jens Axboe Sept. 26, 2019, 12:26 p.m. UTC | #5

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 <[e-mail]>
>>>>>
>>>>> 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-=[e-mail]/
> (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.

Patch

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,