In all cases call the function just once instead of up to four times, at
the same time avoiding to store a dangling pointer in a local variable.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1587,11 +1587,11 @@ unmap_common_complete(struct gnttab_unma
else
status = &status_entry(rgt, op->ref);
- pg = mfn_to_page(op->mfn);
+ pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
if ( op->done & GNTMAP_device_map )
{
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
{
if ( op->done & GNTMAP_readonly )
put_page(pg);
@@ -1608,7 +1608,7 @@ unmap_common_complete(struct gnttab_unma
if ( op->done & GNTMAP_host_map )
{
- if ( !is_iomem_page(op->mfn) )
+ if ( pg )
{
if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
ld, rd) )
@@ -3778,7 +3778,7 @@ int gnttab_release_mappings(struct domai
else
status = &status_entry(rgt, ref);
- pg = mfn_to_page(act->mfn);
+ pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
if ( map->flags & GNTMAP_readonly )
{
@@ -3786,7 +3786,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_devr_mask));
act->pin -= GNTPIN_devr_inc;
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
put_page(pg);
}
@@ -3794,8 +3794,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_hstr_mask));
act->pin -= GNTPIN_hstr_inc;
- if ( gnttab_release_host_mappings(d) &&
- !is_iomem_page(act->mfn) )
+ if ( pg && gnttab_release_host_mappings(d) )
put_page(pg);
}
}
@@ -3805,7 +3804,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_devw_mask));
act->pin -= GNTPIN_devw_inc;
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
put_page_and_type(pg);
}
@@ -3813,8 +3812,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_hstw_mask));
act->pin -= GNTPIN_hstw_inc;
- if ( gnttab_release_host_mappings(d) &&
- !is_iomem_page(act->mfn) )
+ if ( pg && gnttab_release_host_mappings(d) )
{
if ( gnttab_host_mapping_get_page_type(false, d, rd) )
put_page_type(pg);
In all cases call the function just once instead of up to four times, at
the same time avoiding to store a dangling pointer in a local variable.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1587,11 +1587,11 @@ unmap_common_complete(struct gnttab_unma
else
status = &status_entry(rgt, op->ref);
- pg = mfn_to_page(op->mfn);
+ pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
if ( op->done & GNTMAP_device_map )
{
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
{
if ( op->done & GNTMAP_readonly )
put_page(pg);
@@ -1608,7 +1608,7 @@ unmap_common_complete(struct gnttab_unma
if ( op->done & GNTMAP_host_map )
{
- if ( !is_iomem_page(op->mfn) )
+ if ( pg )
{
if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
ld, rd) )
@@ -3778,7 +3778,7 @@ int gnttab_release_mappings(struct domai
else
status = &status_entry(rgt, ref);
- pg = mfn_to_page(act->mfn);
+ pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
if ( map->flags & GNTMAP_readonly )
{
@@ -3786,7 +3786,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_devr_mask));
act->pin -= GNTPIN_devr_inc;
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
put_page(pg);
}
@@ -3794,8 +3794,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_hstr_mask));
act->pin -= GNTPIN_hstr_inc;
- if ( gnttab_release_host_mappings(d) &&
- !is_iomem_page(act->mfn) )
+ if ( pg && gnttab_release_host_mappings(d) )
put_page(pg);
}
}
@@ -3805,7 +3804,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_devw_mask));
act->pin -= GNTPIN_devw_inc;
- if ( !is_iomem_page(act->mfn) )
+ if ( pg )
put_page_and_type(pg);
}
@@ -3813,8 +3812,7 @@ int gnttab_release_mappings(struct domai
{
BUG_ON(!(act->pin & GNTPIN_hstw_mask));
act->pin -= GNTPIN_hstw_inc;
- if ( gnttab_release_host_mappings(d) &&
- !is_iomem_page(act->mfn) )
+ if ( pg && gnttab_release_host_mappings(d) )
{
if ( gnttab_host_mapping_get_page_type(false, d, rd) )
put_page_type(pg);
Hi Jan,
On 26/08/2021 11:12, Jan Beulich wrote:
> In all cases call the function just once instead of up to four times, at
extra NIT: It is more like two because there is a else in
gnttab_release_mappings() :)
> the same time avoiding to store a dangling pointer in a local variable.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
[...]
Everything below looks a duplicate. Might be an issue in your tools?
> In all cases call the function just once instead of up to four times, at
> the same time avoiding to store a dangling pointer in a local variable.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1587,11 +1587,11 @@ unmap_common_complete(struct gnttab_unma
> else
> status = &status_entry(rgt, op->ref);
>
> - pg = mfn_to_page(op->mfn);
> + pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
>
> if ( op->done & GNTMAP_device_map )
> {
> - if ( !is_iomem_page(act->mfn) )
> + if ( pg )
> {
> if ( op->done & GNTMAP_readonly )
> put_page(pg);
> @@ -1608,7 +1608,7 @@ unmap_common_complete(struct gnttab_unma
>
> if ( op->done & GNTMAP_host_map )
> {
> - if ( !is_iomem_page(op->mfn) )
> + if ( pg )
> {
> if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
> ld, rd) )
> @@ -3778,7 +3778,7 @@ int gnttab_release_mappings(struct domai
> else
> status = &status_entry(rgt, ref);
>
> - pg = mfn_to_page(act->mfn);
> + pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
>
> if ( map->flags & GNTMAP_readonly )
> {
> @@ -3786,7 +3786,7 @@ int gnttab_release_mappings(struct domai
> {
> BUG_ON(!(act->pin & GNTPIN_devr_mask));
> act->pin -= GNTPIN_devr_inc;
> - if ( !is_iomem_page(act->mfn) )
> + if ( pg )
> put_page(pg);
> }
>
> @@ -3794,8 +3794,7 @@ int gnttab_release_mappings(struct domai
> {
> BUG_ON(!(act->pin & GNTPIN_hstr_mask));
> act->pin -= GNTPIN_hstr_inc;
> - if ( gnttab_release_host_mappings(d) &&
> - !is_iomem_page(act->mfn) )
> + if ( pg && gnttab_release_host_mappings(d) )
> put_page(pg);
> }
> }
> @@ -3805,7 +3804,7 @@ int gnttab_release_mappings(struct domai
> {
> BUG_ON(!(act->pin & GNTPIN_devw_mask));
> act->pin -= GNTPIN_devw_inc;
> - if ( !is_iomem_page(act->mfn) )
> + if ( pg )
> put_page_and_type(pg);
> }
>
> @@ -3813,8 +3812,7 @@ int gnttab_release_mappings(struct domai
> {
> BUG_ON(!(act->pin & GNTPIN_hstw_mask));
> act->pin -= GNTPIN_hstw_inc;
> - if ( gnttab_release_host_mappings(d) &&
> - !is_iomem_page(act->mfn) )
> + if ( pg && gnttab_release_host_mappings(d) )
> {
> if ( gnttab_host_mapping_get_page_type(false, d, rd) )
> put_page_type(pg);
>
--
Julien Grall
On 06.09.2021 15:35, Julien Grall wrote: > On 26/08/2021 11:12, Jan Beulich wrote: >> In all cases call the function just once instead of up to four times, at > > extra NIT: It is more like two because there is a else in > gnttab_release_mappings() :) Well, I was viewing things from code gen pov, not so much execution paths. And of course "two" is not wrongly covered by "up to four" ;-) >> the same time avoiding to store a dangling pointer in a local variable. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thanks. >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c > > [...] > > Everything below looks a duplicate. Might be an issue in your tools? Oops, indeed - some kind of glitch. Odd. Jan
© 2016 - 2026 Red Hat, Inc.