* [PATCH v5 for-4.9 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors
2017-04-07 19:35 [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Andrew Cooper
@ 2017-04-07 19:35 ` Andrew Cooper
2017-04-10 9:48 ` Jan Beulich
2017-04-07 19:35 ` [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers Andrew Cooper
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-07 19:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Paul Durrant, Jan Beulich
This allows the usual cases to be simplified, by omitting an unnecessary buf
parameters, and because the macros can appropriately size the object.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/x86/hvm/dm.c | 47 +++++++++++++++++++++++++++++------------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 63d15ec..3d8ae89 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -37,36 +37,48 @@ struct dmop_bufs {
#undef MAX_NR_BUFS
};
-static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
- unsigned int nr_bufs, void *dst,
- unsigned int idx, size_t dst_size)
+static bool _raw_copy_from_guest_buf(
+ const struct dmop_bufs *bufs, unsigned int idx,
+ void *dst, size_t dst_bytes)
{
- size_t size;
+ size_t buf_bytes;
- if ( idx >= nr_bufs )
+ if ( idx >= bufs->nr )
return false;
- memset(dst, 0, dst_size);
+ buf_bytes = bufs->buf[idx].size;
- size = min_t(size_t, dst_size, bufs[idx].size);
+ if ( dst_bytes > buf_bytes )
+ return false;
+
+ memset(dst, 0, dst_bytes);
- return !copy_from_guest(dst, bufs[idx].h, size);
+ return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
}
-static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
- unsigned int nr_bufs, unsigned int idx,
- const void *src, size_t src_size)
+static bool _raw_copy_to_guest_buf(
+ struct dmop_bufs *bufs, unsigned int idx,
+ const void *src, size_t src_bytes)
{
- size_t size;
+ size_t buf_bytes;
- if ( idx >= nr_bufs )
+ if ( idx >= bufs->nr )
return false;
- size = min_t(size_t, bufs[idx].size, src_size);
+ buf_bytes = bufs->buf[idx].size;
+
+ if ( src_bytes > buf_bytes )
+ return false;
- return !copy_to_guest(bufs[idx].h, src, size);
+ return !copy_to_guest(bufs->buf[idx].h, src, src_bytes);
}
+#define copy_from_guest_buf(bufs, buf_idx, dst) \
+ _raw_copy_from_guest_buf(bufs, buf_idx, dst, sizeof(*(dst)))
+
+#define copy_to_guest_buf(bufs, buf_idx, src) \
+ _raw_copy_to_guest_buf(bufs, buf_idx, src, sizeof(*(src)))
+
static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
unsigned int nr, struct xen_dm_op_buf *buf)
{
@@ -317,7 +329,7 @@ static int dm_op(domid_t domid, struct dmop_bufs *bufs)
if ( rc )
goto out;
- if ( !copy_buf_from_guest(&bufs->buf[0], bufs->nr, &op, 0, sizeof(op)) )
+ if ( !copy_from_guest_buf(bufs, 0, &op) )
{
rc = -EFAULT;
goto out;
@@ -573,8 +585,7 @@ static int dm_op(domid_t domid, struct dmop_bufs *bufs)
}
if ( (!rc || rc == -ERESTART) &&
- !const_op &&
- !copy_buf_to_guest(&bufs->buf[0], bufs->nr, 0, &op, sizeof(op)) )
+ !const_op && !copy_to_guest_buf(bufs, 0, &op) )
rc = -EFAULT;
out:
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors
2017-04-07 19:35 ` [PATCH v5 for-4.9 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors Andrew Cooper
@ 2017-04-10 9:48 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2017-04-10 9:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Julien Grall, Paul Durrant, Xen-devel
>>> On 07.04.17 at 21:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -37,36 +37,48 @@ struct dmop_bufs {
> #undef MAX_NR_BUFS
> };
>
> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> - unsigned int nr_bufs, void *dst,
> - unsigned int idx, size_t dst_size)
> +static bool _raw_copy_from_guest_buf(
> + const struct dmop_bufs *bufs, unsigned int idx,
> + void *dst, size_t dst_bytes)
If the parameters all get re-arranged anyway, could I talk you into
ordering them in the more memcpy()-like destination, source, size
sequence?
Also, while certainly a matter of taste, I liked the previous way of
indenting better.
> {
> - size_t size;
> + size_t buf_bytes;
>
> - if ( idx >= nr_bufs )
> + if ( idx >= bufs->nr )
> return false;
>
> - memset(dst, 0, dst_size);
> + buf_bytes = bufs->buf[idx].size;
>
> - size = min_t(size_t, dst_size, bufs[idx].size);
> + if ( dst_bytes > buf_bytes )
> + return false;
> +
> + memset(dst, 0, dst_bytes);
>
> - return !copy_from_guest(dst, bufs[idx].h, size);
> + return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> }
The change away from using min() is a behavioral change which you
give no reason for, and which I don't see belonging here (if it is
wanted at all). And if that's what we want, then the memset() is
now pointless.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-07 19:35 [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Andrew Cooper
2017-04-07 19:35 ` [PATCH v5 for-4.9 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors Andrew Cooper
@ 2017-04-07 19:35 ` Andrew Cooper
2017-04-10 9:11 ` Paul Durrant
2017-04-07 19:35 ` [PATCH v5 for-4.9 4/4] dmop: Add xendevicemodel_modified_memory_bulk() Andrew Cooper
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-07 19:35 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Paul Durrant, Jan Beulich
copy_{to,from}_guest_buf() are now implemented using an offset of 0.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 3d8ae89..d584aba 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -37,9 +37,9 @@ struct dmop_bufs {
#undef MAX_NR_BUFS
};
-static bool _raw_copy_from_guest_buf(
+static bool _raw_copy_from_guest_buf_offset(
const struct dmop_bufs *bufs, unsigned int idx,
- void *dst, size_t dst_bytes)
+ size_t offset_bytes, void *dst, size_t dst_bytes)
{
size_t buf_bytes;
@@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
buf_bytes = bufs->buf[idx].size;
- if ( dst_bytes > buf_bytes )
+ if ( offset_bytes >= dst_bytes ||
+ (offset_bytes + dst_bytes) < offset_bytes ||
+ (offset_bytes + dst_bytes) > dst_bytes )
return false;
memset(dst, 0, dst_bytes);
- return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
+ return !copy_from_guest_offset(dst, bufs->buf[idx].h,
+ offset_bytes, dst_bytes);
}
-static bool _raw_copy_to_guest_buf(
+static bool _raw_copy_to_guest_buf_offset(
struct dmop_bufs *bufs, unsigned int idx,
- const void *src, size_t src_bytes)
+ size_t offset_bytes, const void *src, size_t src_bytes)
{
size_t buf_bytes;
@@ -67,17 +70,28 @@ static bool _raw_copy_to_guest_buf(
buf_bytes = bufs->buf[idx].size;
- if ( src_bytes > buf_bytes )
+ if ( offset_bytes >= src_bytes ||
+ (offset_bytes + src_bytes) < offset_bytes ||
+ (offset_bytes + src_bytes) > src_bytes )
return false;
- return !copy_to_guest(bufs->buf[idx].h, src, src_bytes);
+ return !copy_to_guest_offset(bufs->buf[idx].h, offset_bytes,
+ src, src_bytes);
}
+#define copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, dst) \
+ _raw_copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, \
+ dst, sizeof(*(dst)))
+
+#define copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, src) \
+ _raw_copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, \
+ src, sizeof(*(src)))
+
#define copy_from_guest_buf(bufs, buf_idx, dst) \
- _raw_copy_from_guest_buf(bufs, buf_idx, dst, sizeof(*(dst)))
+ copy_from_guest_buf_offset(bufs, buf_idx, 0, dst)
#define copy_to_guest_buf(bufs, buf_idx, src) \
- _raw_copy_to_guest_buf(bufs, buf_idx, src, sizeof(*(src)))
+ copy_to_guest_buf_offset(bufs, buf_idx, 0, src)
static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
unsigned int nr, struct xen_dm_op_buf *buf)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-07 19:35 ` [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers Andrew Cooper
@ 2017-04-10 9:11 ` Paul Durrant
2017-04-10 9:35 ` Andrew Cooper
0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 9:11 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 07 April 2017 20:36
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
>
> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 3d8ae89..d584aba 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -37,9 +37,9 @@ struct dmop_bufs {
> #undef MAX_NR_BUFS
> };
>
> -static bool _raw_copy_from_guest_buf(
> +static bool _raw_copy_from_guest_buf_offset(
> const struct dmop_bufs *bufs, unsigned int idx,
> - void *dst, size_t dst_bytes)
> + size_t offset_bytes, void *dst, size_t dst_bytes)
> {
> size_t buf_bytes;
>
> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
>
> buf_bytes = bufs->buf[idx].size;
>
> - if ( dst_bytes > buf_bytes )
> + if ( offset_bytes >= dst_bytes ||
> + (offset_bytes + dst_bytes) < offset_bytes ||
> + (offset_bytes + dst_bytes) > dst_bytes )
> return false;
>
> memset(dst, 0, dst_bytes);
>
> - return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> + return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> + offset_bytes, dst_bytes);
Not sure what's going on here. 'buf_bytes' is being assigned but no longer seems to be used (since it's dropped from the if statement). Also, I'm not entirely sure what range check that if statement is trying to perform. Can we at least have a comment it it's actually correct (which I'm not at all convinced of).
Paul
> }
>
> -static bool _raw_copy_to_guest_buf(
> +static bool _raw_copy_to_guest_buf_offset(
> struct dmop_bufs *bufs, unsigned int idx,
> - const void *src, size_t src_bytes)
> + size_t offset_bytes, const void *src, size_t src_bytes)
> {
> size_t buf_bytes;
>
> @@ -67,17 +70,28 @@ static bool _raw_copy_to_guest_buf(
>
> buf_bytes = bufs->buf[idx].size;
>
> - if ( src_bytes > buf_bytes )
> + if ( offset_bytes >= src_bytes ||
> + (offset_bytes + src_bytes) < offset_bytes ||
> + (offset_bytes + src_bytes) > src_bytes )
> return false;
>
> - return !copy_to_guest(bufs->buf[idx].h, src, src_bytes);
> + return !copy_to_guest_offset(bufs->buf[idx].h, offset_bytes,
> + src, src_bytes);
> }
>
> +#define copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, dst) \
> + _raw_copy_from_guest_buf_offset(bufs, buf_idx, offset_bytes, \
> + dst, sizeof(*(dst)))
> +
> +#define copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, src) \
> + _raw_copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, \
> + src, sizeof(*(src)))
> +
> #define copy_from_guest_buf(bufs, buf_idx, dst) \
> - _raw_copy_from_guest_buf(bufs, buf_idx, dst, sizeof(*(dst)))
> + copy_from_guest_buf_offset(bufs, buf_idx, 0, dst)
>
> #define copy_to_guest_buf(bufs, buf_idx, src) \
> - _raw_copy_to_guest_buf(bufs, buf_idx, src, sizeof(*(src)))
> + copy_to_guest_buf_offset(bufs, buf_idx, 0, src)
>
> static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
> unsigned int nr, struct xen_dm_op_buf *buf)
> --
> 2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-10 9:11 ` Paul Durrant
@ 2017-04-10 9:35 ` Andrew Cooper
2017-04-10 9:52 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-10 9:35 UTC (permalink / raw)
To: Paul Durrant, Xen-devel; +Cc: Julien Grall, Jan Beulich
On 10/04/17 10:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 07 April 2017 20:36
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien Grall
>> <julien.grall@arm.com>
>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>> copy_{to,from}_guest_buf_offset() helpers
>>
>> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>> xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
>> 1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index 3d8ae89..d584aba 100644
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -37,9 +37,9 @@ struct dmop_bufs {
>> #undef MAX_NR_BUFS
>> };
>>
>> -static bool _raw_copy_from_guest_buf(
>> +static bool _raw_copy_from_guest_buf_offset(
>> const struct dmop_bufs *bufs, unsigned int idx,
>> - void *dst, size_t dst_bytes)
>> + size_t offset_bytes, void *dst, size_t dst_bytes)
>> {
>> size_t buf_bytes;
>>
>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
>>
>> buf_bytes = bufs->buf[idx].size;
>>
>> - if ( dst_bytes > buf_bytes )
>> + if ( offset_bytes >= dst_bytes ||
>> + (offset_bytes + dst_bytes) < offset_bytes ||
>> + (offset_bytes + dst_bytes) > dst_bytes )
>> return false;
>>
>> memset(dst, 0, dst_bytes);
>>
>> - return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
>> + return !copy_from_guest_offset(dst, bufs->buf[idx].h,
>> + offset_bytes, dst_bytes);
> Not sure what's going on here. 'buf_bytes' is being assigned but no longer seems to be used (since it's dropped from the if statement). Also, I'm not entirely sure what range check that if statement is trying to perform. Can we at least have a comment it it's actually correct (which I'm not at all convinced of).
That is actually because the if statement isn't correct. The final
comparison should be against buf_bytes, not dst_bytes.
The problem is that copy_from_guest_offset() takes offset in units of
sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
the type of buf.h changes), while nr is strictly in bytes. My
conclusion after Friday's hacking is that this is also a recipe
security-relevant mistakes, and is fiendishly complicated to reason about.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-10 9:35 ` Andrew Cooper
@ 2017-04-10 9:52 ` Paul Durrant
2017-04-10 9:57 ` Andrew Cooper
0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 9:52 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:36
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
>
> On 10/04/17 10:11, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: 07 April 2017 20:36
> >> To: Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
> Grall
> >> <julien.grall@arm.com>
> >> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >> copy_{to,from}_guest_buf_offset() helpers
> >>
> >> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >> xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
> >> 1 file changed, 24 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >> index 3d8ae89..d584aba 100644
> >> --- a/xen/arch/x86/hvm/dm.c
> >> +++ b/xen/arch/x86/hvm/dm.c
> >> @@ -37,9 +37,9 @@ struct dmop_bufs {
> >> #undef MAX_NR_BUFS
> >> };
> >>
> >> -static bool _raw_copy_from_guest_buf(
> >> +static bool _raw_copy_from_guest_buf_offset(
> >> const struct dmop_bufs *bufs, unsigned int idx,
> >> - void *dst, size_t dst_bytes)
> >> + size_t offset_bytes, void *dst, size_t dst_bytes)
> >> {
> >> size_t buf_bytes;
> >>
> >> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
> >>
> >> buf_bytes = bufs->buf[idx].size;
> >>
> >> - if ( dst_bytes > buf_bytes )
> >> + if ( offset_bytes >= dst_bytes ||
> >> + (offset_bytes + dst_bytes) < offset_bytes ||
> >> + (offset_bytes + dst_bytes) > dst_bytes )
> >> return false;
> >>
> >> memset(dst, 0, dst_bytes);
> >>
> >> - return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> >> + return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> >> + offset_bytes, dst_bytes);
> > Not sure what's going on here. 'buf_bytes' is being assigned but no longer
> seems to be used (since it's dropped from the if statement). Also, I'm not
> entirely sure what range check that if statement is trying to perform. Can we
> at least have a comment it it's actually correct (which I'm not at all convinced
> of).
>
> That is actually because the if statement isn't correct. The final
> comparison should be against buf_bytes, not dst_bytes.
Ok, that makes more sense... so the first clause is checking for size_t overflow, right?
>
> The problem is that copy_from_guest_offset() takes offset in units of
> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
> the type of buf.h changes), while nr is strictly in bytes. My
> conclusion after Friday's hacking is that this is also a recipe
> security-relevant mistakes, and is fiendishly complicated to reason about.
Anything using typeof() is a PITA to reason about IMO, so using the offset variant is definitely going to be an improvement.
Paul
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-10 9:52 ` Paul Durrant
@ 2017-04-10 9:57 ` Andrew Cooper
2017-04-10 10:04 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-10 9:57 UTC (permalink / raw)
To: Paul Durrant, Xen-devel; +Cc: Julien Grall, Jan Beulich
On 10/04/17 10:52, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 10 April 2017 10:36
>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
>> devel@lists.xen.org>
>> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>> copy_{to,from}_guest_buf_offset() helpers
>>
>> On 10/04/17 10:11, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: 07 April 2017 20:36
>>>> To: Xen-devel <xen-devel@lists.xen.org>
>>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>>>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
>> Grall
>>>> <julien.grall@arm.com>
>>>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
>>>> copy_{to,from}_guest_buf_offset() helpers
>>>>
>>>> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>> xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
>>>> 1 file changed, 24 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>> index 3d8ae89..d584aba 100644
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -37,9 +37,9 @@ struct dmop_bufs {
>>>> #undef MAX_NR_BUFS
>>>> };
>>>>
>>>> -static bool _raw_copy_from_guest_buf(
>>>> +static bool _raw_copy_from_guest_buf_offset(
>>>> const struct dmop_bufs *bufs, unsigned int idx,
>>>> - void *dst, size_t dst_bytes)
>>>> + size_t offset_bytes, void *dst, size_t dst_bytes)
>>>> {
>>>> size_t buf_bytes;
>>>>
>>>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
>>>>
>>>> buf_bytes = bufs->buf[idx].size;
>>>>
>>>> - if ( dst_bytes > buf_bytes )
>>>> + if ( offset_bytes >= dst_bytes ||
>>>> + (offset_bytes + dst_bytes) < offset_bytes ||
>>>> + (offset_bytes + dst_bytes) > dst_bytes )
>>>> return false;
>>>>
>>>> memset(dst, 0, dst_bytes);
>>>>
>>>> - return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
>>>> + return !copy_from_guest_offset(dst, bufs->buf[idx].h,
>>>> + offset_bytes, dst_bytes);
>>> Not sure what's going on here. 'buf_bytes' is being assigned but no longer
>> seems to be used (since it's dropped from the if statement). Also, I'm not
>> entirely sure what range check that if statement is trying to perform. Can we
>> at least have a comment it it's actually correct (which I'm not at all convinced
>> of).
>>
>> That is actually because the if statement isn't correct. The final
>> comparison should be against buf_bytes, not dst_bytes.
> Ok, that makes more sense... so the first clause is checking for size_t overflow, right?
The first is a straight "is the offset off the end of the buffer", the
middle is a size_t overflow check (this is defined behaviour as size_t
is unsigned. It would be UB if size_t was signed), and the final was
supposed to be "(offset_bytes + dst_bytes) > buf_bytes" to check whether
the entire region wanting copying resides inside the buffer.
>
>> The problem is that copy_from_guest_offset() takes offset in units of
>> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
>> the type of buf.h changes), while nr is strictly in bytes. My
>> conclusion after Friday's hacking is that this is also a recipe
>> security-relevant mistakes, and is fiendishly complicated to reason about.
> Anything using typeof() is a PITA to reason about IMO, so using the offset variant is definitely going to be an improvement.
I'm mulling over rewriting it all, because it is starting to get
embarrassing how many security issues we have in the existing
infrastructure.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers
2017-04-10 9:57 ` Andrew Cooper
@ 2017-04-10 10:04 ` Paul Durrant
0 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 10:04 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:57
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> copy_{to,from}_guest_buf_offset() helpers
>
> On 10/04/17 10:52, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 10 April 2017 10:36
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> >> devel@lists.xen.org>
> >> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: Re: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >> copy_{to,from}_guest_buf_offset() helpers
> >>
> >> On 10/04/17 10:11, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>>> Sent: 07 April 2017 20:36
> >>>> To: Xen-devel <xen-devel@lists.xen.org>
> >>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >>>> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Julien
> >> Grall
> >>>> <julien.grall@arm.com>
> >>>> Subject: [PATCH v5 for-4.9 3/4] hvm/dmop: Implement
> >>>> copy_{to,from}_guest_buf_offset() helpers
> >>>>
> >>>> copy_{to,from}_guest_buf() are now implemented using an offset of
> 0.
> >>>>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> ---
> >>>> CC: Jan Beulich <JBeulich@suse.com>
> >>>> CC: Paul Durrant <paul.durrant@citrix.com>
> >>>> CC: Julien Grall <julien.grall@arm.com>
> >>>> ---
> >>>> xen/arch/x86/hvm/dm.c | 34 ++++++++++++++++++++++++----------
> >>>> 1 file changed, 24 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >>>> index 3d8ae89..d584aba 100644
> >>>> --- a/xen/arch/x86/hvm/dm.c
> >>>> +++ b/xen/arch/x86/hvm/dm.c
> >>>> @@ -37,9 +37,9 @@ struct dmop_bufs {
> >>>> #undef MAX_NR_BUFS
> >>>> };
> >>>>
> >>>> -static bool _raw_copy_from_guest_buf(
> >>>> +static bool _raw_copy_from_guest_buf_offset(
> >>>> const struct dmop_bufs *bufs, unsigned int idx,
> >>>> - void *dst, size_t dst_bytes)
> >>>> + size_t offset_bytes, void *dst, size_t dst_bytes)
> >>>> {
> >>>> size_t buf_bytes;
> >>>>
> >>>> @@ -48,17 +48,20 @@ static bool _raw_copy_from_guest_buf(
> >>>>
> >>>> buf_bytes = bufs->buf[idx].size;
> >>>>
> >>>> - if ( dst_bytes > buf_bytes )
> >>>> + if ( offset_bytes >= dst_bytes ||
> >>>> + (offset_bytes + dst_bytes) < offset_bytes ||
> >>>> + (offset_bytes + dst_bytes) > dst_bytes )
> >>>> return false;
> >>>>
> >>>> memset(dst, 0, dst_bytes);
> >>>>
> >>>> - return !copy_from_guest(dst, bufs->buf[idx].h, dst_bytes);
> >>>> + return !copy_from_guest_offset(dst, bufs->buf[idx].h,
> >>>> + offset_bytes, dst_bytes);
> >>> Not sure what's going on here. 'buf_bytes' is being assigned but no
> longer
> >> seems to be used (since it's dropped from the if statement). Also, I'm not
> >> entirely sure what range check that if statement is trying to perform. Can
> we
> >> at least have a comment it it's actually correct (which I'm not at all
> convinced
> >> of).
> >>
> >> That is actually because the if statement isn't correct. The final
> >> comparison should be against buf_bytes, not dst_bytes.
> > Ok, that makes more sense... so the first clause is checking for size_t
> overflow, right?
>
> The first is a straight "is the offset off the end of the buffer", the
> middle is a size_t overflow check (this is defined behaviour as size_t
> is unsigned. It would be UB if size_t was signed), and the final was
> supposed to be "(offset_bytes + dst_bytes) > buf_bytes" to check whether
> the entire region wanting copying resides inside the buffer.
>
Sorry, I meant the middle clause, so with the fix that all makes sense now.
> >
> >> The problem is that copy_from_guest_offset() takes offset in units of
> >> sizeof(typeof(*bufs->buf[idx].h)) (which in this case is bytes, until
> >> the type of buf.h changes), while nr is strictly in bytes. My
> >> conclusion after Friday's hacking is that this is also a recipe
> >> security-relevant mistakes, and is fiendishly complicated to reason about.
> > Anything using typeof() is a PITA to reason about IMO, so using the offset
> variant is definitely going to be an improvement.
>
> I'm mulling over rewriting it all, because it is starting to get
> embarrassing how many security issues we have in the existing
> infrastructure.
>
Certainly removing some of the macrotization would make it easier to follow and, if copy_from_guest() is going to use typeof (for convenience of copying an op struct in the common case I guess) then it really should be single instance for safety, or preferably take an explicit type.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 for-4.9 4/4] dmop: Add xendevicemodel_modified_memory_bulk()
2017-04-07 19:35 [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Andrew Cooper
2017-04-07 19:35 ` [PATCH v5 for-4.9 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors Andrew Cooper
2017-04-07 19:35 ` [PATCH v5 for-4.9 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers Andrew Cooper
@ 2017-04-07 19:35 ` Andrew Cooper
2017-04-10 9:04 ` [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Paul Durrant
2017-04-10 9:39 ` Jan Beulich
4 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2017-04-07 19:35 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Jennifer Herbert, Ian Jackson, Julien Grall,
Paul Durrant, Jan Beulich
From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
This new lib devicemodel call allows multiple extents of pages to be
marked as modified in a single call. This is something needed for a
usecase I'm working on.
The xen side of the modified_memory call has been modified to accept
an array of extents. The devicemodle library either provides an array
of length 1, to support the original library function, or a second
function allows an array to be provided.
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
tools/libs/devicemodel/core.c | 30 ++++--
tools/libs/devicemodel/include/xendevicemodel.h | 19 +++-
xen/arch/x86/hvm/dm.c | 117 ++++++++++++++++--------
xen/include/public/hvm/dm_op.h | 19 +++-
4 files changed, 134 insertions(+), 51 deletions(-)
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index ff09819..4cb023f 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -459,22 +459,36 @@ int xendevicemodel_track_dirty_vram(
dirty_bitmap, (size_t)(nr + 7) / 8);
}
-int xendevicemodel_modified_memory(
- xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
- uint32_t nr)
+int xendevicemodel_modified_memory_bulk(
+ xendevicemodel_handle *dmod, domid_t domid,
+ struct xen_dm_op_modified_memory_extent *extents, uint32_t nr)
{
struct xen_dm_op op;
- struct xen_dm_op_modified_memory *data;
+ struct xen_dm_op_modified_memory *header;
+ size_t extents_size = nr * sizeof(struct xen_dm_op_modified_memory_extent);
memset(&op, 0, sizeof(op));
op.op = XEN_DMOP_modified_memory;
- data = &op.u.modified_memory;
+ header = &op.u.modified_memory;
- data->first_pfn = first_pfn;
- data->nr = nr;
+ header->nr_extents = nr;
+ header->pfns_processed = 0;
- return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+ return xendevicemodel_op(dmod, domid, 2, &op, sizeof(op),
+ extents, extents_size);
+}
+
+int xendevicemodel_modified_memory(
+ xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
+ uint32_t nr)
+{
+ struct xen_dm_op_modified_memory_extent extent;
+
+ extent.first_pfn = first_pfn;
+ extent.nr = nr;
+
+ return xendevicemodel_modified_memory_bulk(dmod, domid, &extent, 1);
}
int xendevicemodel_set_mem_type(
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
index 1da216f..580fad2 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -254,8 +254,8 @@ int xendevicemodel_track_dirty_vram(
uint32_t nr, unsigned long *dirty_bitmap);
/**
- * This function notifies the hypervisor that a set of domain pages
- * have been modified.
+ * This function notifies the hypervisor that a set of contiguous
+ * domain pages have been modified.
*
* @parm dmod a handle to an open devicemodel interface.
* @parm domid the domain id to be serviced
@@ -268,6 +268,21 @@ int xendevicemodel_modified_memory(
uint32_t nr);
/**
+ * This function notifies the hypervisor that a set of discontiguous
+ * domain pages have been modified.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced
+ * @parm extents an array of extent structs, which each hold
+ a start_pfn and nr (number of pfns).
+ * @parm nr the number of extents in the array
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_modified_memory_bulk(
+ xendevicemodel_handle *dmod, domid_t domid,
+ struct xen_dm_op_modified_memory_extent extents[], uint32_t nr);
+
+/**
* This function notifies the hypervisor that a set of domain pages
* are to be treated in a specific way. (See the definition of
* hvmmem_type_t).
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d584aba..77b2585 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -157,56 +157,102 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
}
static int modified_memory(struct domain *d,
- struct xen_dm_op_modified_memory *data)
+ struct dmop_bufs *bufs,
+ struct xen_dm_op_modified_memory *header)
{
- xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
- unsigned int iter = 0;
- int rc = 0;
+#define EXTENTS_BUFFER 1
- if ( (data->first_pfn > last_pfn) ||
- (last_pfn > domain_get_maximum_gpfn(d)) )
- return -EINVAL;
+ /* Process maximum of 256 pfns before checking for continuation. */
+ const unsigned int cont_check_interval = 0x100;
+ unsigned int *rem_extents = &header->nr_extents;
+ unsigned int batch_rem_pfns = cont_check_interval;
+ /* Used for continuation. */
+ unsigned int *pfns_done = &header->opaque;
if ( !paging_mode_log_dirty(d) )
return 0;
- while ( iter < data->nr )
+ if ( (bufs->buf[EXTENTS_BUFFER].size /
+ sizeof(struct xen_dm_op_modified_memory_extent)) <
+ *rem_extents )
+ return -EINVAL;
+
+ while ( *rem_extents > 0 )
{
- unsigned long pfn = data->first_pfn + iter;
- struct page_info *page;
+ struct xen_dm_op_modified_memory_extent extent;
+ unsigned int batch_nr;
+ xen_pfn_t pfn, end_pfn;
+ int rc;
+
+ rc = copy_from_guest_buf_offset(
+ bufs, EXTENTS_BUFFER, (*rem_extents - 1) * sizeof(extent), &extent);
+ if ( rc )
+ return -EFAULT;
+
+ if ( extent.pad )
+ return -EINVAL;
+
+ end_pfn = extent.first_pfn + extent.nr;
+
+ if ( end_pfn <= extent.first_pfn ||
+ end_pfn > domain_get_maximum_gpfn(d) )
+ return -EINVAL;
- page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
- if ( page )
+ if ( *pfns_done >= extent.nr )
+ return -EINVAL;
+
+ pfn = extent.first_pfn + *pfns_done;
+ batch_nr = extent.nr - *pfns_done;
+
+ if ( batch_nr > batch_rem_pfns )
+ {
+ batch_nr = batch_rem_pfns;
+ *pfns_done += batch_nr;
+ end_pfn = pfn + batch_nr;
+ }
+ else
{
- mfn_t gmfn = _mfn(page_to_mfn(page));
-
- paging_mark_dirty(d, gmfn);
- /*
- * These are most probably not page tables any more
- * don't take a long time and don't die either.
- */
- sh_remove_shadows(d, gmfn, 1, 0);
- put_page(page);
+ (*rem_extents)--;
+ *pfns_done = 0;
}
- iter++;
+ batch_rem_pfns -= batch_nr;
+
+ for ( ; pfn < end_pfn; pfn++ )
+ {
+ struct page_info *page;
+
+ page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
+ if ( page )
+ {
+ mfn_t gmfn = _mfn(page_to_mfn(page));
+
+ paging_mark_dirty(d, gmfn);
+ /*
+ * These are most probably not page tables any more
+ * don't take a long time and don't die either.
+ */
+ sh_remove_shadows(d, gmfn, 1, 0);
+ put_page(page);
+ }
+ }
/*
- * Check for continuation every 256th iteration and if the
- * iteration is not the last.
+ * After a full batch of cont_check_interval pfns
+ * have been processed, and there are still extents
+ * remaining to process, check for continuation.
*/
- if ( (iter < data->nr) && ((iter & 0xff) == 0) &&
- hypercall_preempt_check() )
+ if ( (batch_rem_pfns == 0) && (*rem_extents > 0) )
{
- data->first_pfn += iter;
- data->nr -= iter;
+ if ( hypercall_preempt_check() )
+ return -ERESTART;
- rc = -ERESTART;
- break;
+ batch_rem_pfns = cont_check_interval;
}
}
+ return 0;
- return rc;
+#undef EXTENTS_BUFFER
}
static bool allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
@@ -543,13 +589,8 @@ static int dm_op(domid_t domid, struct dmop_bufs *bufs)
struct xen_dm_op_modified_memory *data =
&op.u.modified_memory;
- const_op = false;
-
- rc = -EINVAL;
- if ( data->pad )
- break;
-
- rc = modified_memory(d, data);
+ rc = modified_memory(d, bufs, data);
+ const_op = !rc;
break;
}
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 5ea79ef..20c21b6 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -237,13 +237,26 @@ struct xen_dm_op_set_pci_link_route {
* XEN_DMOP_modified_memory: Notify that a set of pages were modified by
* an emulator.
*
- * NOTE: In the event of a continuation, the @first_pfn is set to the
- * value of the pfn of the remaining set of pages and @nr reduced
- * to the size of the remaining set.
+ * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
+ * @nr_extents entries.
+ *
+ * On error, @nr_extents will contain the index+1 of the extent that
+ * had the error. It is not defined if or which pages may have been
+ * marked as dirty, in this event.
*/
#define XEN_DMOP_modified_memory 11
struct xen_dm_op_modified_memory {
+ /*
+ * IN - Number of extents to be processed
+ * OUT -returns n+1 for failing extent
+ */
+ uint32_t nr_extents;
+ /* IN/OUT - Must be set to 0 */
+ uint32_t opaque;
+};
+
+struct xen_dm_op_modified_memory_extent {
/* IN - number of contiguous pages modified */
uint32_t nr;
uint32_t pad;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-07 19:35 [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Andrew Cooper
` (2 preceding siblings ...)
2017-04-07 19:35 ` [PATCH v5 for-4.9 4/4] dmop: Add xendevicemodel_modified_memory_bulk() Andrew Cooper
@ 2017-04-10 9:04 ` Paul Durrant
2017-04-10 9:29 ` Andrew Cooper
2017-04-10 9:39 ` Jan Beulich
4 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 9:04 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Julien Grall, Jennifer Herbert, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 07 April 2017 20:36
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
>
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>
> No functional change.
>
Why is this a good thing? Passing two parameters around allowed for them to be in registers. I preferred the code as it was before.
Paul
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/x86/hvm/dm.c | 49 +++++++++++++++++++++++++++++++-------
> -----------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..63d15ec 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -25,6 +25,18 @@
>
> #include <xsm/xsm.h>
>
> +struct dmop_bufs {
> +/*
> + * Small sanity limit. Enough for all current hypercalls.
> + */
> +#define MAX_NR_BUFS 2
> +
> + struct xen_dm_op_buf buf[MAX_NR_BUFS];
> + unsigned int nr;
> +
> +#undef MAX_NR_BUFS
> +};
> +
> static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> unsigned int nr_bufs, void *dst,
> unsigned int idx, size_t dst_size)
> @@ -287,9 +299,7 @@ static int inject_event(struct domain *d,
> return 0;
> }
>
> -static int dm_op(domid_t domid,
> - unsigned int nr_bufs,
> - xen_dm_op_buf_t bufs[])
> +static int dm_op(domid_t domid, struct dmop_bufs *bufs)
> {
> struct domain *d;
> struct xen_dm_op op;
> @@ -307,7 +317,7 @@ static int dm_op(domid_t domid,
> if ( rc )
> goto out;
>
> - if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
> + if ( !copy_buf_from_guest(&bufs->buf[0], bufs->nr, &op, 0, sizeof(op)) )
> {
> rc = -EFAULT;
> goto out;
> @@ -466,10 +476,10 @@ static int dm_op(domid_t domid,
> if ( data->pad )
> break;
>
> - if ( nr_bufs < 2 )
> + if ( bufs->nr < 2 )
> break;
>
> - rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
> + rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs->buf[1]);
> break;
> }
>
> @@ -564,7 +574,7 @@ static int dm_op(domid_t domid,
>
> if ( (!rc || rc == -ERESTART) &&
> !const_op &&
> - !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
> + !copy_buf_to_guest(&bufs->buf[0], bufs->nr, 0, &op, sizeof(op)) )
> rc = -EFAULT;
>
> out:
> @@ -587,20 +597,21 @@ CHECK_dm_op_set_mem_type;
> CHECK_dm_op_inject_event;
> CHECK_dm_op_inject_msi;
>
> -#define MAX_NR_BUFS 2
> -
> int compat_dm_op(domid_t domid,
> unsigned int nr_bufs,
> XEN_GUEST_HANDLE_PARAM(void) bufs)
> {
> - struct xen_dm_op_buf nat[MAX_NR_BUFS];
> + struct dmop_bufs buffers;
> +
> unsigned int i;
> int rc;
>
> - if ( nr_bufs > MAX_NR_BUFS )
> + if ( nr_bufs > ARRAY_SIZE(buffers.buf) )
> return -E2BIG;
>
> - for ( i = 0; i < nr_bufs; i++ )
> + buffers.nr = nr_bufs;
> +
> + for ( i = 0; i < buffers.nr; i++ )
> {
> struct compat_dm_op_buf cmp;
>
> @@ -610,12 +621,12 @@ int compat_dm_op(domid_t domid,
> #define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
> guest_from_compat_handle((_d_)->h, (_s_)->h)
>
> - XLAT_dm_op_buf(&nat[i], &cmp);
> + XLAT_dm_op_buf(&buffers.buf[i], &cmp);
>
> #undef XLAT_dm_op_buf_HNDL_h
> }
>
> - rc = dm_op(domid, nr_bufs, nat);
> + rc = dm_op(domid, &buffers);
>
> if ( rc == -ERESTART )
> rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> @@ -628,16 +639,18 @@ long do_dm_op(domid_t domid,
> unsigned int nr_bufs,
> XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
> {
> - struct xen_dm_op_buf nat[MAX_NR_BUFS];
> + struct dmop_bufs buffers;
> int rc;
>
> - if ( nr_bufs > MAX_NR_BUFS )
> + if ( nr_bufs > ARRAY_SIZE(buffers.buf) )
> return -E2BIG;
>
> - if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
> + buffers.nr = nr_bufs;
> +
> + if ( copy_from_guest_offset(&buffers.buf[0], bufs, 0, buffers.nr) )
> return -EFAULT;
>
> - rc = dm_op(domid, nr_bufs, nat);
> + rc = dm_op(domid, &buffers);
>
> if ( rc == -ERESTART )
> rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> --
> 2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 9:04 ` [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Paul Durrant
@ 2017-04-10 9:29 ` Andrew Cooper
2017-04-10 9:40 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-10 9:29 UTC (permalink / raw)
To: Paul Durrant, Xen-devel; +Cc: Julien Grall, Jennifer Herbert, Jan Beulich
On 10/04/17 10:04, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 07 April 2017 20:36
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Paul
>> Durrant <Paul.Durrant@citrix.com>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>> passing two parameters around
>>
>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>
>> No functional change.
>>
> Why is this a good thing? Passing two parameters around allowed for them to be in registers. I preferred the code as it was before.
a) It will always be inlined, so registers aren't relevant. Even if
they were, all values are available directly with the pointer as a base,
so there is no reduction in expressiveness. (i.e. the previous code
only increases register pressure).
b) passing multiple parameters like that is a recipe for mistakes, and
in this case, mistakes mean security vulnerabilities.
c) It is necessary to make legible code for the later changes in this
series.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 9:29 ` Andrew Cooper
@ 2017-04-10 9:40 ` Paul Durrant
2017-04-10 10:04 ` Andrew Cooper
2017-04-10 10:06 ` Jennifer Herbert
0 siblings, 2 replies; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 9:40 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jennifer Herbert, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 10:29
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
>
> On 10/04/17 10:04, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: 07 April 2017 20:36
> >> To: Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Paul
> >> Durrant <Paul.Durrant@citrix.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> >> passing two parameters around
> >>
> >> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>
> >> No functional change.
> >>
> > Why is this a good thing? Passing two parameters around allowed for them
> to be in registers. I preferred the code as it was before.
>
> a) It will always be inlined, so registers aren't relevant.
Why? I see nothing forcing the compiler to make it so.
> Even if
> they were, all values are available directly with the pointer as a base,
> so there is no reduction in expressiveness. (i.e. the previous code
> only increases register pressure).
> b) passing multiple parameters like that is a recipe for mistakes, and
> in this case, mistakes mean security vulnerabilities.
Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.
> c) It is necessary to make legible code for the later changes in this
> series.
>
From my reading I don't think it would have an overly negative effect on the subsequent patches if this patch were dropped.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 9:40 ` Paul Durrant
@ 2017-04-10 10:04 ` Andrew Cooper
2017-04-10 10:12 ` Paul Durrant
2017-04-10 10:06 ` Jennifer Herbert
1 sibling, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2017-04-10 10:04 UTC (permalink / raw)
To: Paul Durrant, Xen-devel; +Cc: Julien Grall, Jennifer Herbert, Jan Beulich
On 10/04/17 10:40, Paul Durrant wrote:
>>
>>> Why is this a good thing? Passing two parameters around allowed for them
>> to be in registers. I preferred the code as it was before.
>>
>> a) It will always be inlined, so registers aren't relevant.
> Why? I see nothing forcing the compiler to make it so.
Fine. Let me rephrase as "GCC does inline it".
>
>> Even if
>> they were, all values are available directly with the pointer as a base,
>> so there is no reduction in expressiveness. (i.e. the previous code
>> only increases register pressure).
>> b) passing multiple parameters like that is a recipe for mistakes, and
>> in this case, mistakes mean security vulnerabilities.
> Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.
Passing more than one parameter is of course fine.
Requiring the caller to pass two parameters which strictly must be in
sync for security reasons is not fine.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 10:04 ` Andrew Cooper
@ 2017-04-10 10:12 ` Paul Durrant
0 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 10:12 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jennifer Herbert, Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 10 April 2017 11:04
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
>
> On 10/04/17 10:40, Paul Durrant wrote:
> >>
> >>> Why is this a good thing? Passing two parameters around allowed for
> them
> >> to be in registers. I preferred the code as it was before.
> >>
> >> a) It will always be inlined, so registers aren't relevant.
> > Why? I see nothing forcing the compiler to make it so.
>
> Fine. Let me rephrase as "GCC does inline it".
>
That's better :-)
Yes, the fact it's a const pointer does allow for inlining (dereferencing stack pointer usually being a surefire way to stop inlining and burn stack frames) but perhaps there's a case for forcing an inline? (I don't know what clang will do).
> >
> >> Even if
> >> they were, all values are available directly with the pointer as a base,
> >> so there is no reduction in expressiveness. (i.e. the previous code
> >> only increases register pressure).
> >> b) passing multiple parameters like that is a recipe for mistakes, and
> >> in this case, mistakes mean security vulnerabilities.
> > Given the locality of the code I don't buy that as an argument unless you're
> going to assert that passing more than one parameter is always wrong.
>
> Passing more than one parameter is of course fine.
>
> Requiring the caller to pass two parameters which strictly must be in
> sync for security reasons is not fine.
>
For calling non-local functions, I agree. But the callee is local here and the intention was that the prototype be the same as the hypercall modulo the guest handle nastiness. So, if you want to pack things into a struct then I'd prefer a general 'dm_op_args' struct that includes the domid as well.
Paul
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 9:40 ` Paul Durrant
2017-04-10 10:04 ` Andrew Cooper
@ 2017-04-10 10:06 ` Jennifer Herbert
2017-04-10 10:18 ` Paul Durrant
1 sibling, 1 reply; 18+ messages in thread
From: Jennifer Herbert @ 2017-04-10 10:06 UTC (permalink / raw)
To: Paul Durrant, Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jan Beulich
On 10/04/17 10:40, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 10 April 2017 10:29
>> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
>> devel@lists.xen.org>
>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>> passing two parameters around
>>
>> On 10/04/17 10:04, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: 07 April 2017 20:36
>>>> To: Xen-devel <xen-devel@lists.xen.org>
>>>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
>>>> <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>; Paul
>>>> Durrant <Paul.Durrant@citrix.com>; Julien Grall <julien.grall@arm.com>
>>>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
>>>> passing two parameters around
>>>>
>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>
>>>> No functional change.
>>>>
>>> Why is this a good thing? Passing two parameters around allowed for them
>> to be in registers. I preferred the code as it was before.
>>
>> a) It will always be inlined, so registers aren't relevant.
> Why? I see nothing forcing the compiler to make it so.
>
>> Even if
>> they were, all values are available directly with the pointer as a base,
>> so there is no reduction in expressiveness. (i.e. the previous code
>> only increases register pressure).
>> b) passing multiple parameters like that is a recipe for mistakes, and
>> in this case, mistakes mean security vulnerabilities.
> Given the locality of the code I don't buy that as an argument unless you're going to assert that passing more than one parameter is always wrong.
To two parameters should go everywhere together - they work together,
and should never be separated. But by having them separate, you end up
forcing people to think about why do I need this nr_buffs, even though
in many situations it looks superfluous. Its mostly there as a check,
and not part of the core flow of how the code works.
It just confuses and clutters things to have it separate.
If its left separate, its possible someone will feed in a different, but
similar variable for this parameter, and its purpose defeated.
>> c) It is necessary to make legible code for the later changes in this
>> series.
>>
> From my reading I don't think it would have an overly negative effect on the subsequent patches if this patch were dropped.
No, but since we're having this debate, we clearly care about small
negative effects too.
-jenny
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-10 10:06 ` Jennifer Herbert
@ 2017-04-10 10:18 ` Paul Durrant
0 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2017-04-10 10:18 UTC (permalink / raw)
To: Jennifer Herbert, Andrew Cooper, Xen-devel; +Cc: Julien Grall, Jan Beulich
> -----Original Message-----
> From: Jennifer Herbert [mailto:Jennifer.Herbert@citrix.com]
> Sent: 10 April 2017 11:06
> To: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than
> passing two parameters around
>
>
> On 10/04/17 10:40, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 10 April 2017 10:29
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> >> devel@lists.xen.org>
> >> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
> >> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather
> than
> >> passing two parameters around
> >>
> >> On 10/04/17 10:04, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>>> Sent: 07 April 2017 20:36
> >>>> To: Xen-devel <xen-devel@lists.xen.org>
> >>>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> >>>> <Andrew.Cooper3@citrix.com>; Jan Beulich <JBeulich@suse.com>;
> Paul
> >>>> Durrant <Paul.Durrant@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> >>>> Subject: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather
> than
> >>>> passing two parameters around
> >>>>
> >>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>>>
> >>>> No functional change.
> >>>>
> >>> Why is this a good thing? Passing two parameters around allowed for
> them
> >> to be in registers. I preferred the code as it was before.
> >>
> >> a) It will always be inlined, so registers aren't relevant.
> > Why? I see nothing forcing the compiler to make it so.
> >
> >> Even if
> >> they were, all values are available directly with the pointer as a base,
> >> so there is no reduction in expressiveness. (i.e. the previous code
> >> only increases register pressure).
> >> b) passing multiple parameters like that is a recipe for mistakes, and
> >> in this case, mistakes mean security vulnerabilities.
> > Given the locality of the code I don't buy that as an argument unless you're
> going to assert that passing more than one parameter is always wrong.
>
> To two parameters should go everywhere together - they work together,
> and should never be separated. But by having them separate, you end up
> forcing people to think about why do I need this nr_buffs, even though
> in many situations it looks superfluous. Its mostly there as a check,
> and not part of the core flow of how the code works.
> It just confuses and clutters things to have it separate.
Well that's where I disagree. I don't think it is any more confusing than having multiple args to any function.
>
> If its left separate, its possible someone will feed in a different, but
> similar variable for this parameter, and its purpose defeated.
>
Well, that would indication fairly serious incompetence in the 'someone'. do_dm_op() only exists to be the common entry point to the hypercall after the compat and guest handle nastiness has been sorted out. It's static and clearly not some sort of general purpose utility function. Keeping the prototype aligned with that of the hypercall seems illustrative and entirely reasonable to me.
As I replied to Andrew though, I'm ok with a general args structure packaging the domid as well (since this is no less critical to security than the nr_bufs argument).
Paul
> >> c) It is necessary to make legible code for the later changes in this
> >> series.
> >>
> > From my reading I don't think it would have an overly negative effect on
> the subsequent patches if this patch were dropped.
>
> No, but since we're having this debate, we clearly care about small
> negative effects too.
>
> -jenny
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around
2017-04-07 19:35 [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Andrew Cooper
` (3 preceding siblings ...)
2017-04-10 9:04 ` [PATCH v5 for-4.9 1/4] hvm/dmop: Box dmop_bufs rather than passing two parameters around Paul Durrant
@ 2017-04-10 9:39 ` Jan Beulich
4 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2017-04-10 9:39 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Julien Grall, Paul Durrant, Jennifer Herbert, Xen-devel
>>> On 07.04.17 at 21:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -25,6 +25,18 @@
>
> #include <xsm/xsm.h>
>
> +struct dmop_bufs {
> +/*
> + * Small sanity limit. Enough for all current hypercalls.
> + */
> +#define MAX_NR_BUFS 2
> +
> + struct xen_dm_op_buf buf[MAX_NR_BUFS];
> + unsigned int nr;
> +
> +#undef MAX_NR_BUFS
What's the use of this constant? This is clearly more cluttering than
clarifying things. I'd much appreciate if 2 was used as array
dimension right away, and the constant dropped. With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread