xen/arch/x86/hvm/dm.c | 13 +++++++------ xen/arch/x86/mm/hap/hap.c | 21 +++++++++++---------- xen/arch/x86/mm/shadow/hvm.c | 16 ++++++++-------- xen/include/asm-x86/hap.h | 2 +- xen/include/asm-x86/shadow.h | 2 +- 5 files changed, 28 insertions(+), 26 deletions(-)
* Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the
lower levels.
* Use DIV_ROUND_UP() rather than opencoding it in several different ways
* The hypercall input is capped at uint32_t, so there is no need for
nr_frames to be unsigned long in the lower levels.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/dm.c | 13 +++++++------
xen/arch/x86/mm/hap/hap.c | 21 +++++++++++----------
xen/arch/x86/mm/shadow/hvm.c | 16 ++++++++--------
xen/include/asm-x86/hap.h | 2 +-
xen/include/asm-x86/shadow.h | 2 +-
5 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index e3f845165d..9930d68860 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -62,9 +62,10 @@ static bool _raw_copy_from_guest_buf_offset(void *dst,
sizeof(dst))
static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
- unsigned int nr, const struct xen_dm_op_buf *buf)
+ unsigned int nr_frames,
+ const struct xen_dm_op_buf *buf)
{
- if ( nr > (GB(1) >> PAGE_SHIFT) )
+ if ( nr_frames > (GB(1) >> PAGE_SHIFT) )
return -EINVAL;
if ( d->is_dying )
@@ -73,12 +74,12 @@ static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
if ( !d->max_vcpus || !d->vcpu[0] )
return -EINVAL;
- if ( ((nr + 7) / 8) > buf->size )
+ if ( DIV_ROUND_UP(nr_frames, BITS_PER_BYTE) > buf->size )
return -EINVAL;
- return shadow_mode_enabled(d) ?
- shadow_track_dirty_vram(d, first_pfn, nr, buf->h) :
- hap_track_dirty_vram(d, first_pfn, nr, buf->h);
+ return shadow_mode_enabled(d)
+ ? shadow_track_dirty_vram(d, first_pfn, nr_frames, buf->h)
+ : hap_track_dirty_vram(d, first_pfn, nr_frames, buf->h);
}
static int set_pci_intx_level(struct domain *d, uint16_t domain,
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 7f84d0c6ea..4eedd1a995 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -58,16 +58,16 @@
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
- unsigned long nr,
+ unsigned int nr_frames,
XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
long rc = 0;
struct sh_dirty_vram *dirty_vram;
uint8_t *dirty_bitmap = NULL;
- if ( nr )
+ if ( nr_frames )
{
- int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
+ unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
if ( !paging_mode_log_dirty(d) )
{
@@ -97,13 +97,13 @@ int hap_track_dirty_vram(struct domain *d,
}
if ( begin_pfn != dirty_vram->begin_pfn ||
- begin_pfn + nr != dirty_vram->end_pfn )
+ begin_pfn + nr_frames != dirty_vram->end_pfn )
{
unsigned long ostart = dirty_vram->begin_pfn;
unsigned long oend = dirty_vram->end_pfn;
dirty_vram->begin_pfn = begin_pfn;
- dirty_vram->end_pfn = begin_pfn + nr;
+ dirty_vram->end_pfn = begin_pfn + nr_frames;
paging_unlock(d);
@@ -115,7 +115,7 @@ int hap_track_dirty_vram(struct domain *d,
* Switch vram to log dirty mode, either by setting l1e entries of
* P2M table to be read-only, or via hardware-assisted log-dirty.
*/
- p2m_change_type_range(d, begin_pfn, begin_pfn + nr,
+ p2m_change_type_range(d, begin_pfn, begin_pfn + nr_frames,
p2m_ram_rw, p2m_ram_logdirty);
guest_flush_tlb_mask(d, d->dirty_cpumask);
@@ -132,7 +132,7 @@ int hap_track_dirty_vram(struct domain *d,
p2m_flush_hardware_cached_dirty(d);
/* get the bitmap */
- paging_log_dirty_range(d, begin_pfn, nr, dirty_bitmap);
+ paging_log_dirty_range(d, begin_pfn, nr_frames, dirty_bitmap);
domain_unpause(d);
}
@@ -153,14 +153,15 @@ int hap_track_dirty_vram(struct domain *d,
* then stop tracking
*/
begin_pfn = dirty_vram->begin_pfn;
- nr = dirty_vram->end_pfn - dirty_vram->begin_pfn;
+ nr_frames = dirty_vram->end_pfn - dirty_vram->begin_pfn;
xfree(dirty_vram);
d->arch.hvm.dirty_vram = NULL;
}
paging_unlock(d);
- if ( nr )
- p2m_change_type_range(d, begin_pfn, begin_pfn + nr,
+
+ if ( nr_frames )
+ p2m_change_type_range(d, begin_pfn, begin_pfn + nr_frames,
p2m_ram_logdirty, p2m_ram_rw);
}
out:
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
index c5da7a071c..b832272c10 100644
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -695,12 +695,12 @@ static void sh_emulate_unmap_dest(struct vcpu *v, void *addr,
/* VRAM dirty tracking support */
int shadow_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
- unsigned long nr,
+ unsigned int nr_frames,
XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
int rc = 0;
- unsigned long end_pfn = begin_pfn + nr;
- unsigned long dirty_size = (nr + 7) / 8;
+ unsigned long end_pfn = begin_pfn + nr_frames;
+ unsigned int dirty_size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
int flush_tlb = 0;
unsigned long i;
p2m_type_t t;
@@ -717,7 +717,7 @@ int shadow_track_dirty_vram(struct domain *d,
dirty_vram = d->arch.hvm.dirty_vram;
- if ( dirty_vram && (!nr ||
+ if ( dirty_vram && (!nr_frames ||
( begin_pfn != dirty_vram->begin_pfn
|| end_pfn != dirty_vram->end_pfn )) )
{
@@ -729,7 +729,7 @@ int shadow_track_dirty_vram(struct domain *d,
dirty_vram = d->arch.hvm.dirty_vram = NULL;
}
- if ( !nr )
+ if ( !nr_frames )
goto out;
dirty_bitmap = vzalloc(dirty_size);
@@ -759,9 +759,9 @@ int shadow_track_dirty_vram(struct domain *d,
dirty_vram->end_pfn = end_pfn;
d->arch.hvm.dirty_vram = dirty_vram;
- if ( (dirty_vram->sl1ma = xmalloc_array(paddr_t, nr)) == NULL )
+ if ( (dirty_vram->sl1ma = xmalloc_array(paddr_t, nr_frames)) == NULL )
goto out_dirty_vram;
- memset(dirty_vram->sl1ma, ~0, sizeof(paddr_t) * nr);
+ memset(dirty_vram->sl1ma, ~0, sizeof(paddr_t) * nr_frames);
if ( (dirty_vram->dirty_bitmap = xzalloc_array(uint8_t, dirty_size)) == NULL )
goto out_sl1ma;
@@ -780,7 +780,7 @@ int shadow_track_dirty_vram(struct domain *d,
void *map_sl1p = NULL;
/* Iterate over VRAM to track dirty bits. */
- for ( i = 0; i < nr; i++ )
+ for ( i = 0; i < nr_frames; i++ )
{
mfn_t mfn = get_gfn_query_unlocked(d, begin_pfn + i, &t);
struct page_info *page;
diff --git a/xen/include/asm-x86/hap.h b/xen/include/asm-x86/hap.h
index faf856913a..d489df3812 100644
--- a/xen/include/asm-x86/hap.h
+++ b/xen/include/asm-x86/hap.h
@@ -40,7 +40,7 @@ void hap_teardown(struct domain *d, bool *preempted);
void hap_vcpu_init(struct vcpu *v);
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
- unsigned long nr,
+ unsigned int nr_frames,
XEN_GUEST_HANDLE(void) dirty_bitmap);
extern const struct paging_mode *hap_paging_get_mode(struct vcpu *);
diff --git a/xen/include/asm-x86/shadow.h b/xen/include/asm-x86/shadow.h
index 224d1bc2f9..76e47f257f 100644
--- a/xen/include/asm-x86/shadow.h
+++ b/xen/include/asm-x86/shadow.h
@@ -64,7 +64,7 @@ int shadow_enable(struct domain *d, u32 mode);
/* Enable VRAM dirty bit tracking. */
int shadow_track_dirty_vram(struct domain *d,
unsigned long first_pfn,
- unsigned long nr,
+ unsigned int nr_frames,
XEN_GUEST_HANDLE(void) dirty_bitmap);
/* Handler for shadow control ops: operations from user-space to enable
--
2.11.0
At 16:15 +0100 on 22 Jul (1595434548), Andrew Cooper wrote: > * Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the > lower levels. > * Use DIV_ROUND_UP() rather than opencoding it in several different ways > * The hypercall input is capped at uint32_t, so there is no need for > nr_frames to be unsigned long in the lower levels. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Tim Deegan <tim@xen.org>
On 22.07.2020 17:15, Andrew Cooper wrote:
> * Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the
> lower levels.
> * Use DIV_ROUND_UP() rather than opencoding it in several different ways
> * The hypercall input is capped at uint32_t, so there is no need for
> nr_frames to be unsigned long in the lower levels.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
I'd like to note though that ...
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -58,16 +58,16 @@
>
> int hap_track_dirty_vram(struct domain *d,
> unsigned long begin_pfn,
> - unsigned long nr,
> + unsigned int nr_frames,
> XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
> {
> long rc = 0;
> struct sh_dirty_vram *dirty_vram;
> uint8_t *dirty_bitmap = NULL;
>
> - if ( nr )
> + if ( nr_frames )
> {
> - int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
> + unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
... with the change from long to int this construct will now no
longer be correct for the (admittedly absurd) case of a hypercall
input in the range of [0xfffffff9,0xffffffff]. We now fully
depend on this getting properly rejected at the top level hypercall
handler (which limits to 1Gb worth of tracked space).
Jan
On 22/07/2020 17:13, Jan Beulich wrote:
> On 22.07.2020 17:15, Andrew Cooper wrote:
>> * Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the
>> lower levels.
>> * Use DIV_ROUND_UP() rather than opencoding it in several different ways
>> * The hypercall input is capped at uint32_t, so there is no need for
>> nr_frames to be unsigned long in the lower levels.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I'd like to note though that ...
>
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -58,16 +58,16 @@
>>
>> int hap_track_dirty_vram(struct domain *d,
>> unsigned long begin_pfn,
>> - unsigned long nr,
>> + unsigned int nr_frames,
>> XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
>> {
>> long rc = 0;
>> struct sh_dirty_vram *dirty_vram;
>> uint8_t *dirty_bitmap = NULL;
>>
>> - if ( nr )
>> + if ( nr_frames )
>> {
>> - int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
>> + unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
> ... with the change from long to int this construct will now no
> longer be correct for the (admittedly absurd) case of a hypercall
> input in the range of [0xfffffff9,0xffffffff]. We now fully
> depend on this getting properly rejected at the top level hypercall
> handler (which limits to 1Gb worth of tracked space).
I don't see how this makes any difference at all.
Exactly the same would be true in the old code for an input in the range
[0xfffffffffffffff9,0xffffffffffffffff], where the aspect which protects
you is the fact that the hypercall ABI truncates to 32 bits.
If you want a non-overflowing DIV_ROUND_UP(), the appropriate expression
in (x / a) + !!(x % a), but I don't think its reasonable to use the type
of this variable as a credible defence-in-depth argument against the
audit logic making a mistake, or that it is worth worrying about audit
mistakes in the first place. If there are any audit mistakes, so much
more can potentially go wrong than this corner case.
~Andrew
On 23.07.2020 11:40, Andrew Cooper wrote:
> On 22/07/2020 17:13, Jan Beulich wrote:
>> On 22.07.2020 17:15, Andrew Cooper wrote:
>>> * Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the
>>> lower levels.
>>> * Use DIV_ROUND_UP() rather than opencoding it in several different ways
>>> * The hypercall input is capped at uint32_t, so there is no need for
>>> nr_frames to be unsigned long in the lower levels.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> I'd like to note though that ...
>>
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -58,16 +58,16 @@
>>>
>>> int hap_track_dirty_vram(struct domain *d,
>>> unsigned long begin_pfn,
>>> - unsigned long nr,
>>> + unsigned int nr_frames,
>>> XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
>>> {
>>> long rc = 0;
>>> struct sh_dirty_vram *dirty_vram;
>>> uint8_t *dirty_bitmap = NULL;
>>>
>>> - if ( nr )
>>> + if ( nr_frames )
>>> {
>>> - int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
>>> + unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
>> ... with the change from long to int this construct will now no
>> longer be correct for the (admittedly absurd) case of a hypercall
>> input in the range of [0xfffffff9,0xffffffff]. We now fully
>> depend on this getting properly rejected at the top level hypercall
>> handler (which limits to 1Gb worth of tracked space).
>
> I don't see how this makes any difference at all.
>
> Exactly the same would be true in the old code for an input in the range
> [0xfffffffffffffff9,0xffffffffffffffff], where the aspect which protects
> you is the fact that the hypercall ABI truncates to 32 bits.
Exactly: The hypercall ABI won't change. The GB(1) check up the call
tree may go away, without the then arising issue being noticed.
Jan
On 23/07/2020 11:25, Jan Beulich wrote:
> On 23.07.2020 11:40, Andrew Cooper wrote:
>> On 22/07/2020 17:13, Jan Beulich wrote:
>>> On 22.07.2020 17:15, Andrew Cooper wrote:
>>>> * Rename nr to nr_frames. A plain 'nr' is confusing to follow in the the
>>>> lower levels.
>>>> * Use DIV_ROUND_UP() rather than opencoding it in several different ways
>>>> * The hypercall input is capped at uint32_t, so there is no need for
>>>> nr_frames to be unsigned long in the lower levels.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I'd like to note though that ...
>>>
>>>> --- a/xen/arch/x86/mm/hap/hap.c
>>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>>> @@ -58,16 +58,16 @@
>>>>
>>>> int hap_track_dirty_vram(struct domain *d,
>>>> unsigned long begin_pfn,
>>>> - unsigned long nr,
>>>> + unsigned int nr_frames,
>>>> XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
>>>> {
>>>> long rc = 0;
>>>> struct sh_dirty_vram *dirty_vram;
>>>> uint8_t *dirty_bitmap = NULL;
>>>>
>>>> - if ( nr )
>>>> + if ( nr_frames )
>>>> {
>>>> - int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
>>>> + unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);
>>> ... with the change from long to int this construct will now no
>>> longer be correct for the (admittedly absurd) case of a hypercall
>>> input in the range of [0xfffffff9,0xffffffff]. We now fully
>>> depend on this getting properly rejected at the top level hypercall
>>> handler (which limits to 1Gb worth of tracked space).
>> I don't see how this makes any difference at all.
>>
>> Exactly the same would be true in the old code for an input in the range
>> [0xfffffffffffffff9,0xffffffffffffffff], where the aspect which protects
>> you is the fact that the hypercall ABI truncates to 32 bits.
> Exactly: The hypercall ABI won't change. The GB(1) check up the call
> tree may go away, without the then arising issue being noticed.
The ABI is equally as like to change as the 1G limit. Either both
issues will be fixed (and almost certainly together), or neither will
change forever more.
~Andrew
© 2016 - 2024 Red Hat, Inc.