On 12/03/2021 13:08, Jan Beulich wrote:
> On 12.03.2021 12:32, Andrew Cooper 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.
>>>
>>> Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition")
>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>
>>> --- 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;
>>> int rc = -EINVAL;
>>>
>>> if ( !nr_frames )
>> in v1, there was a companion check.
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index f937c1d350..2bb07f129f 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource(
>> if ( rc )
>> goto out;
>>
>> + /*
>> + * Some older toolchains can't spot that vaddrs is non-NULL on
>> non-error
>> + * paths. Leave some runtime safety.
>> + */
>> + if ( !vaddrs )
>> + {
>> + ASSERT_UNREACHABLE();
>> + goto out;
>> + }
>> +
>> for ( i = 0; i < nr_frames; ++i )
>> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
> Oh, I didn't realize this. Will add, but did you really mean to
> have the function return success in this case (on a release
> build)? I'd be inclined to put it ahead of if "if ( rc )" and
> set rc (to e.g. -ENODATA) in this case.
Oh - quite right. Returning 0 here will hit the assertion/failsafe
protecting against livelock.
I'd be tempted to chose -EINVAL because the only plausible way to get
here is a bad id, and that path should have errored out earlier.
And yeah, with the rc adjustment, fine to reposition.
~Andrew
