Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2021 18:52, Julien Grall wrote:
> On 10/03/2021 16:21, Jan Beulich wrote:
>> On 10.03.2021 15:58, Julien Grall wrote:
>>> On 10/03/2021 10:13, Jan Beulich wrote:
>>>> Sadly I was wrong to suggest dropping vaddrs' initializer during review
>>>> of v2 of the patch introducing this code. gcc 4.3 can't cope.
>>>
>>> What's the error?
>>
>> The one quoted in the title.
>>
>>> Are you sure this is not going to hiding a potential
>>> miscompilation of the function?
>>
>> Miscompilation? It may hide us screwing up, but addressing such a
>> compiler warning by adding an initializer has been quite common
>> in the past.
> 
> Well... When a compiler tells me a value may be unitialized, I read it 
> as some optimization may decide to use the variable in a way I wasn't 
> expected.

I don't think that's how warnings like this work in general. Optimization
may help spot a case where an uninitialized variable gets used (because
optimization may result in sufficient simplification of the internal
representation of the original source), and variable lifetime analysis
may also be a prereq to optimization, but in general I'd recommend
viewing the two as independent aspects.

>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource(
>>>>        struct grant_table *gt = d->grant_table;
>>>>        unsigned int i, final_frame;
>>>>        mfn_t tmp;
>>>> -    void **vaddrs;
>>>> +    void **vaddrs = NULL;
>>> I am a bit nervous to inialize vaddrs to NULL for a few reasons:
>>>     1) It is not 100% obvious (e.g. it takes more than a second) that
>>> vaddrs will always be initialized.
>>
>> But convincing ourselves was necessary even more so prior to this
>> change. We must not ever rely on the compiler to tell us about
>> issues in our code. We can only leverage that in some cases it
>> does.
> 
> I didn't suggest that we should only rely on the compiler. I pointed out 
> that we are telling the compiler to not worry. This may hide a valid 
> concern from the compiler.

As we (have to) do elsewhere.

>> From this it (I think obviously) follows that without the
>> initializer we're at bigger risk than with it.
> 
> I thought deferencing a NULL pointer was still a concern for PV?

I could use ZERO_BLOCK_PTR or yet something else, if we decided we
want to work around this class of issues this way. Elsewhere we're
using NULL afaict (but see also below).

> For the other setup, I agree that it would only lead to a crash rather 
> than dereferencing anything. Yet I am not convinced this is that much 
> better...

When using an uninitialized variable, anything can happen. A crash
would still be on the better side of things, as a privilege
escalation then also is possible. Again - if you're worried about
us introducing an actual use of the thus initialized variable, you
should be even more worried about using it when it's uninitialized
(and the compiler _not_ being able to spot it).

>>>     2) A compiler will not be able to help us if we are adding code
>>> without initialized vaddrs.
>>>
>>> It also feels wrong to me to try to write Xen in a way that will make a
>>> 10 years compiler happy...
>>
>> As said above - we've worked around limitations quite a few times
>> in the past. This is just one more instance.
> 
> I find amusing you wrote that when you complained multiple time when 
> someone was re-using existing bad pattern. :)

Well, thing is - I don't view this as a bad pattern. The only question
really is whether NULL is a good initializer here. As per above a non-
canonical pointer may be better, but then we have quite a few places
elsewhere to fix. We could also deliberately leave the variable
uninitialized, by using past Linux'es uninitialized_var(), but they've
dropped that construct for what I think are very good reasons.

Jan