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

[Julien Grall <julien@xxxxxxx>]

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?
> 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?

Cheers,

--
Julien Grall