xen/common/grant_table.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
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@suse.com>
---
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;
--
2.35.3On 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@suse.com>
> ---
> 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.
> @@ -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"?
Jan
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@suse.com>
>> ---
>> 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.
Juergen
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@suse.com>
>>> ---
>>> 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
Kind regards,
Henry
> -----Original Message-----
> From: Juergen Gross <jgross@suse.com>
> Sent: Friday, September 9, 2022 5:26 PM
> To: Jan Beulich <jbeulich@suse.com>
> Cc: Henry Wang <Henry.Wang@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH] xen/gnttab: fix gnttab_acquire_resource()
>
> 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@suse.com>
> >> ---
> >> 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.
>
>
> Juergen
On 09.09.2022 11:28, Henry Wang wrote: > > > Kind regards, > Henry Hmm, did you mean to actually add some text in your message? Jan
Hi Jan, > -----Original Message----- > On 09.09.2022 11:28, Henry Wang wrote: > > > > > > Kind regards, > > Henry > > Hmm, did you mean to actually add some text in your message? I am really sorry, my outlook just got frozen and I must mis-send an empty email somehow. No nothing from my side. Kind regards, Henry > > Jan
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@suse.com>
> ---
> 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
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@suse.com>
>> ---
>> 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
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@suse.com>
>>> ---
>>> 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
Hi Juergen,
> -----Original Message-----
> From: Juergen Gross <jgross@suse.com>
> Subject: [PATCH] xen/gnttab: fix gnttab_acquire_resource()
>
> 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@suse.com>
> ---
> Might be considered for 4.17 and for backporting
Of course, feel free to add:
Release-acked-by: Henry Wang <Henry.Wang@arm.com>
If the maintainer give an ack about this patch.
Kind regards,
Henry
Hi Henry,
On 09/09/2022 09:47, Henry Wang wrote:
>> -----Original Message-----
>> From: Juergen Gross <jgross@suse.com>
>> Subject: [PATCH] xen/gnttab: fix gnttab_acquire_resource()
>>
>> 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@suse.com>
>> ---
>> Might be considered for 4.17 and for backporting
>
> Of course, feel free to add:
>
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
Unrelated to this patch, but as this is your first Released-acked-by
tag, I wanted to check the policy going forward.
From now, will any new patch need your approval before been merged?
Cheers,
--
Julien Grall
Hi Julien,
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH] xen/gnttab: fix gnttab_acquire_resource()
>
> Hi Henry,
>
> On 09/09/2022 09:47, Henry Wang wrote:
> >> -----Original Message-----
> >> From: Juergen Gross <jgross@suse.com>
> >> Subject: [PATCH] xen/gnttab: fix gnttab_acquire_resource()
> >>
> >> 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@suse.com>
> >> ---
> >> Might be considered for 4.17 and for backporting
> >
> > Of course, feel free to add:
> >
> > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
>
> Unrelated to this patch, but as this is your first Released-acked-by
> tag, I wanted to check the policy going forward.
>
> From now, will any new patch need your approval before been merged?
I would say from now is a little bit too early. I think maintainers can
still commit patches (fixes and not new feature related) until the code
freeze (end of this month).
I am providing this tag just to show I am happy to add this to 4.17 since
Juergen mentioned this in scissors line comment.
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
On 09.09.2022 11:04, Julien Grall wrote: > Unrelated to this patch, but as this is your first Released-acked-by > tag, I wanted to check the policy going forward. > > From now, will any new patch need your approval before been merged? It was my understanding (from past releases) that bug fixes would be fine to go in without until code freeze (in 3 weeks as per the current schedule). Jan
Hi Jan, On 09/09/2022 10:10, Jan Beulich wrote: > On 09.09.2022 11:04, Julien Grall wrote: >> Unrelated to this patch, but as this is your first Released-acked-by >> tag, I wanted to check the policy going forward. >> >> From now, will any new patch need your approval before been merged? > > It was my understanding (from past releases) that bug fixes would be > fine to go in without until code freeze (in 3 weeks as per the current > schedule). This is my understanding as well. But I wanted to check with Henry just in case he decided something different. Cheers, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.