Re: [PATCH] xen/gnttab: fix gnttab_acquire_resource()

[Juergen Gross <jgross@xxxxxxxx>]

On 09.09.22 10:56, Julien Grall wrote:
> Hi Juergen,
>
> On 09/09/2022 09:09, Juergen Gross wrote:
> > Commit 9dc46386d89d ("gnttab: work around "may be used uninitialized"
> > warning") was wrong, as vaddr can legitimately be NULL in case
> > XENMEM_resource_grant_table_id_status was specified for a grant table
> > v1. This would result in crashes in debug builds due to
> > ASSERT_UNREACHABLE() triggering.
> >
> > Basically revert said commit, but keep returning -ENODATA in that case.
> This commit was introduced to silence a compiler warning (treated as error in 
> Xen build system). As you revert it, did you check the said compiler (IIRC GCC 
> 4.3) was still happy?
I didn't remove the vaddr initializer.

> > Fixes: 9dc46386d89d ("gnttab: work around "may be used uninitialized" warning")
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > Might be considered for 4.17 and for backporting
> > ---
> >   xen/common/grant_table.c | 14 +++-----------
> >   1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index ad773a6996..68e7f1df38 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -4125,7 +4125,10 @@ int gnttab_acquire_resource(
> >       case XENMEM_resource_grant_table_id_status:
> >           if ( gt->gt_version != 2 )
> > +        {
> > +            rc = -ENODATA;
> >               break;
> > +        }
> >           /* Check that void ** is a suitable representation for gt->status. */
> >           BUILD_BUG_ON(!__builtin_types_compatible_p(
> > @@ -4135,17 +4138,6 @@ int gnttab_acquire_resource(
> >           break;
> >       }
> > -    /*
> > -     * Some older toolchains can't spot that vaddrs won't remain uninitialized
> > -     * on non-error paths, and hence it needs setting to NULL at the top of the
> > -     * function.  Leave some runtime safety.
> > -     */
> > -    if ( !vaddrs )
> > -    {
> > -        ASSERT_UNREACHABLE();
> > -        rc = -ENODATA;
> > -    }
> > -
> >       /* Any errors?  Bad id, or from growing the table? */
> >       if ( rc )
> >           goto out;
> Looking at the code just below the loop is:
>
> for ( i = 0; i < nr_frames; ++i )
>     mfn_list[i] = virt_to_mfn(vaddrs[frame + 1]);
>
> Given that 'nr_frames' is provided by the caller it is a bit unclear how we 
> guarantee that 'vaddrs' will not be NULL when nr_frames > 0.
> Can you explain how you came to the conclusion that this is not possible?
We can reach this point only in case rc is 0.

rc can be 0 only in case gnttab_get_shared_frame_mfn() or
gnttab_get_status_frame_mfn() returned 0, which will be the case only, if
the value vaddrs was set to before calling those functions was valid.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature