| Message ID | 20210928182258.12451-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | proc/vmcore: sanitize access to virtio-mem memory | expand |
On 9/28/21 2:22 PM, David Hildenbrand wrote: > Let's simplify return handling. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/x86/xen/mmu_hvm.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/xen/mmu_hvm.c b/arch/x86/xen/mmu_hvm.c > index b242d1f4b426..eb61622df75b 100644 > --- a/arch/x86/xen/mmu_hvm.c > +++ b/arch/x86/xen/mmu_hvm.c > @@ -21,23 +21,16 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) > .domid = DOMID_SELF, > .pfn = pfn, > }; > - int ram; > > if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) > return -ENXIO; > > switch (a.mem_type) { > case HVMMEM_mmio_dm: > - ram = 0; > - break; > - case HVMMEM_ram_rw: > - case HVMMEM_ram_ro: > + return 0; > default: > - ram = 1; > - break; > + return 1; > } > - > - return ram; > } > #endif > How about return a.mem_type != HVMMEM_mmio_dm; Result should be promoted to int and this has added benefit of not requiring changes in patch 4. -boris
> > How about > > return a.mem_type != HVMMEM_mmio_dm; > Ha, how could I have missed that :) > > Result should be promoted to int and this has added benefit of not requiring changes in patch 4. > Can we go one step further and do @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) struct xen_hvm_get_mem_type a = { .domid = DOMID_SELF, .pfn = pfn, + .mem_type = HVMMEM_ram_rw, }; - int ram; - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) - return -ENXIO; - - switch (a.mem_type) { - case HVMMEM_mmio_dm: - ram = 0; - break; - case HVMMEM_ram_rw: - case HVMMEM_ram_ro: - default: - ram = 1; - break; - } - - return ram; + HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a); + return a.mem_type != HVMMEM_mmio_dm; } #endif Assuming that if HYPERVISOR_hvm_op() fails that .mem_type is not set to HVMMEM_mmio_dm.
On 29.09.21 10:45, David Hildenbrand wrote: >> >> How about >> >> return a.mem_type != HVMMEM_mmio_dm; >> > > Ha, how could I have missed that :) > >> >> Result should be promoted to int and this has added benefit of not requiring changes in patch 4. >> > > Can we go one step further and do > > > @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) > struct xen_hvm_get_mem_type a = { > .domid = DOMID_SELF, > .pfn = pfn, > + .mem_type = HVMMEM_ram_rw, > }; > - int ram; > > - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) > - return -ENXIO; > - > - switch (a.mem_type) { > - case HVMMEM_mmio_dm: > - ram = 0; > - break; > - case HVMMEM_ram_rw: > - case HVMMEM_ram_ro: > - default: > - ram = 1; > - break; > - } > - > - return ram; > + HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a); > + return a.mem_type != HVMMEM_mmio_dm; > } > #endif > > > Assuming that if HYPERVISOR_hvm_op() fails that > .mem_type is not set to HVMMEM_mmio_dm. > Okay we can't, due to "__must_check" ...
On 9/29/21 5:03 AM, David Hildenbrand wrote: > On 29.09.21 10:45, David Hildenbrand wrote: >>> >> Can we go one step further and do >> >> >> @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) >> struct xen_hvm_get_mem_type a = { >> .domid = DOMID_SELF, >> .pfn = pfn, >> + .mem_type = HVMMEM_ram_rw, >> }; >> - int ram; >> - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) >> - return -ENXIO; >> - >> - switch (a.mem_type) { >> - case HVMMEM_mmio_dm: >> - ram = 0; >> - break; >> - case HVMMEM_ram_rw: >> - case HVMMEM_ram_ro: >> - default: >> - ram = 1; >> - break; >> - } >> - >> - return ram; >> + HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a); >> + return a.mem_type != HVMMEM_mmio_dm; I was actually thinking of asking you to add another patch with pr_warn_once() here (and print error code as well). This call failing is indication of something going quite wrong and it would be good to know about this. >> } >> #endif >> >> >> Assuming that if HYPERVISOR_hvm_op() fails that >> .mem_type is not set to HVMMEM_mmio_dm. I don't think we can assume that argument described as OUT in the ABI will not be clobbered in case of error >> > > Okay we can't, due to "__must_check" ... so this is a good thing ;-) -boris
On 29.09.21 16:22, Boris Ostrovsky wrote: > > On 9/29/21 5:03 AM, David Hildenbrand wrote: >> On 29.09.21 10:45, David Hildenbrand wrote: >>>> >>> Can we go one step further and do >>> >>> >>> @@ -20,24 +20,11 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) >>> struct xen_hvm_get_mem_type a = { >>> .domid = DOMID_SELF, >>> .pfn = pfn, >>> + .mem_type = HVMMEM_ram_rw, >>> }; >>> - int ram; >>> - if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) >>> - return -ENXIO; >>> - >>> - switch (a.mem_type) { >>> - case HVMMEM_mmio_dm: >>> - ram = 0; >>> - break; >>> - case HVMMEM_ram_rw: >>> - case HVMMEM_ram_ro: >>> - default: >>> - ram = 1; >>> - break; >>> - } >>> - >>> - return ram; >>> + HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a); >>> + return a.mem_type != HVMMEM_mmio_dm; > > > I was actually thinking of asking you to add another patch with pr_warn_once() here (and print error code as well). This call failing is indication of something going quite wrong and it would be good to know about this. Will include a patch in v2, thanks!
diff --git a/arch/x86/xen/mmu_hvm.c b/arch/x86/xen/mmu_hvm.c index b242d1f4b426..eb61622df75b 100644 --- a/arch/x86/xen/mmu_hvm.c +++ b/arch/x86/xen/mmu_hvm.c @@ -21,23 +21,16 @@ static int xen_oldmem_pfn_is_ram(unsigned long pfn) .domid = DOMID_SELF, .pfn = pfn, }; - int ram; if (HYPERVISOR_hvm_op(HVMOP_get_mem_type, &a)) return -ENXIO; switch (a.mem_type) { case HVMMEM_mmio_dm: - ram = 0; - break; - case HVMMEM_ram_rw: - case HVMMEM_ram_ro: + return 0; default: - ram = 1; - break; + return 1; } - - return ram; } #endif
Let's simplify return handling. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/x86/xen/mmu_hvm.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)