This coming from a shared info field, it has to be assumed to possibly have any value. In particular, our kernels don''t set up this field at all if running as Dom0 and kexec isn''t enabled (along with not setting up the P2M frame lists, as that''s simply wasted memory in that case). So, with this being a guest provided value, we apparently have two problems: do_memory_op()''s "rc" variable is only of type "int", thus potentially truncating the result of domain_get_maximum_gpfn() (considering that we now support 16Tb, an 8Tb+ guest ought to be possible, and such would have a max GPFN with 32 significant bits). And zero or a very large number being returned by the latter will generally be mistaken as an error code by the caller of the hypercall. So, along with promoting "rc" to long, I''m considering enforcing a sane lower bound on domain_get_maximum_gpfn()''s return value, using d->tot_pages (under the assumption that each page would have a representation in the physical map, and hence the physical map can''t reasonably be smaller than this). That would overcome the subtraction of 1 done there for PV guests to convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound ought to be enforced (to avoid collisions with the -E range). Other thoughts? Is such a behavioral change acceptable for an existing interface? Jan
At 14:33 +0000 on 25 Feb (1361802812), Jan Beulich wrote:> This coming from a shared info field, it has to be assumed to possibly > have any value. In particular, our kernels don''t set up this field at all > if running as Dom0 and kexec isn''t enabled (along with not setting up > the P2M frame lists, as that''s simply wasted memory in that case). > > So, with this being a guest provided value, we apparently have two > problems: do_memory_op()''s "rc" variable is only of type "int", thus > potentially truncating the result of domain_get_maximum_gpfn() > (considering that we now support 16Tb, an 8Tb+ guest ought to > be possible, and such would have a max GPFN with 32 significant > bits). And zero or a very large number being returned by the latter > will generally be mistaken as an error code by the caller of the > hypercall. > > So, along with promoting "rc" to long, I''m considering enforcing a > sane lower bound on domain_get_maximum_gpfn()''s return value, > using d->tot_pages (under the assumption that each page would > have a representation in the physical map, and hence the > physical map can''t reasonably be smaller than this). That would > overcome the subtraction of 1 done there for PV guests to > convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound > ought to be enforced (to avoid collisions with the -E range). > > Other thoughts? Is such a behavioral change acceptable for an > existing interface?The new behaviour seems sensible but I''m a little worried about changing the behaviour of an existing call. I''d be inclined to add a new hypercall that DTRT and leave the old one, deprecated. Tim.
>>> On 26.02.13 at 16:27, Tim Deegan <tim@xen.org> wrote: > At 14:33 +0000 on 25 Feb (1361802812), Jan Beulich wrote: >> This coming from a shared info field, it has to be assumed to possibly >> have any value. In particular, our kernels don''t set up this field at all >> if running as Dom0 and kexec isn''t enabled (along with not setting up >> the P2M frame lists, as that''s simply wasted memory in that case). >> >> So, with this being a guest provided value, we apparently have two >> problems: do_memory_op()''s "rc" variable is only of type "int", thus >> potentially truncating the result of domain_get_maximum_gpfn() >> (considering that we now support 16Tb, an 8Tb+ guest ought to >> be possible, and such would have a max GPFN with 32 significant >> bits). And zero or a very large number being returned by the latter >> will generally be mistaken as an error code by the caller of the >> hypercall. >> >> So, along with promoting "rc" to long, I''m considering enforcing a >> sane lower bound on domain_get_maximum_gpfn()''s return value, >> using d->tot_pages (under the assumption that each page would >> have a representation in the physical map, and hence the >> physical map can''t reasonably be smaller than this). That would >> overcome the subtraction of 1 done there for PV guests to >> convert 0 to -1 (i.e. -EPERM). Similarly, a sane upper bound >> ought to be enforced (to avoid collisions with the -E range). >> >> Other thoughts? Is such a behavioral change acceptable for an >> existing interface? > > The new behaviour seems sensible but I''m a little worried about changing > the behaviour of an existing call. I''d be inclined to add a new > hypercall that DTRT and leave the old one, deprecated.So I think this should be two steps then - fix the current call (so that it doesn''t return -1 (== -EPERM) anymore when the field is zero, and so it doesn''t truncate big values anymore) and, should it ever turn out necessary, add a new one returning a sanitized value. I''ll send a patch for the first part in a minute, but I''ll leave the second step open as I think the context in which the problem was observed (xen-mceinj) is actually in need of fixing itself (i.e. should not be using this call). Jan