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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 09/09/2022 10:08, Juergen Gross wrote:
> 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.
Ok so it is not a full revert as you implied above. I think it would be 
good to write "partially".
> > > 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.
This is somewhat fragile. As we had to silence the compiler, the check 
was added to avoid any addition of code that may not properly set 
'vaddrs' (The compiler can't help us anymore).
So I think I would prefer what Jan suggested. We should check 'rc' *and* 
then 'vaddrs'.
Cheers,

--
Julien Grall