Re: [PATCH v3 7/7] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 7/7] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
Date: Fri, 29 Jan 2021 18:18:04 +0000	[thread overview]
Message-ID: <4024cc9e-8c9a-601a-8f9d-fa480c42d569@citrix.com> (raw)
In-Reply-To: <46d73c0a-035c-db7f-3b1a-87ad88bc2518@suse.com>

On 29/01/2021 09:46, Jan Beulich wrote:
> On 29.01.2021 00:44, Andrew Cooper wrote:
>> On 15/01/2021 16:12, Jan Beulich wrote:
>>> On 12.01.2021 20:48, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -4628,7 +4628,6 @@ int arch_acquire_resource(struct domain *d, unsigned int type,
>>>>          if ( id != (unsigned int)ioservid )
>>>>              break;
>>>>  
>>>> -        rc = 0;
>>>>          for ( i = 0; i < nr_frames; i++ )
>>>>          {
>>>>              mfn_t mfn;
>>> How "good" are our chances that older gcc won't recognize that
>>> without this initialization ...
>>>
>>>> @@ -4639,6 +4638,9 @@ int arch_acquire_resource(struct domain *d, unsigned int type,
>>>>  
>>>>              mfn_list[i] = mfn_x(mfn);
>>>>          }
>>>> +        if ( i == nr_frames )
>>>> +            /* Success.  Passed nr_frames back to the caller. */
>>>> +            rc = nr_frames;
>>> ... rc will nevertheless not remain uninitialized when nr_frames
>>> is zero?
>> I don't anticipate this function getting complicated enough for us to
>> need to rely on tricks like that to spot bugs.
>>
>> AFAICT, it would take a rather larger diffstat to make it "uninitialised
>> clean" again.
> Well, we'll see how it goes then. We still allow rather ancient
> gcc, and I will eventually run into a build issue here once
> having rebased accordingly, if such old gcc won't cope.

But those problematic compilers have problems spotting that a variable
is actually initialised on all paths.

This case is opposite.  It is unconditionally initialised to -EINVAL
(just outside of the context above), which breaks some "the compiler
would warn us about error paths" logic that we try to use.

~Andrew