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

[Julien Grall <julien@xxxxxxx>]

On 09/09/2022 10:25, Juergen Gross wrote:
> On 09.09.22 11:15, Jan Beulich wrote:
> > On 09.09.2022 10: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.
> > >
> > > 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;
> > > +        }
> > This path is supposed to produce -EINVAL.
> Okay.
>
> > > @@ -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 )
> > I guess this wants amending by "&& !rc"?
> I can do that, if you like that better.
I would prefer that as well. Although I think it would be clear if we write

"
if ( rc )
   return rc
 else if ( !vaddrs )
"

Cheers,

--
Julien Grall