Re: [Xen-devel] [RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Jan Beulich]

>>> On 26.10.15 at 17:03,  wrote:
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
>  align = 16;
>  
>  s = (scratch_start + align - 1) & ~(align - 1);
> -e = s + size - 1;
> +e = s + size;

This further increases the delta to the actually quite similar
mem_alloc(). I'd prefer the two to remain in sync as much as
possible, and hence either the other one to also be adjusted
or this one to be fixed the other way around (dropping the
first "- 1" in the assignment to s, but requiring changes
elsewhere too).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Ian Campbell]

On Mon, 2015-10-26 at 16:03 +, Anthony PERARD wrote:

Please remember to CC the relevant maintainers.

> scratch_alloc() set scratch_start to the last byte of the current
> allocation.  The value of scratch_start is then reused as is (if it is
> already aligned) in the next allocation.  This result in a potential reuse
> of the last byte of the previous allocation.

This will, in one obscure case, now result in the BUG_ON triggering when it
wouldn't before. Namely in the case where e was previously MAX_INT and is
now 0.

I'm not sure we care, since the end result previously would be that the
next scratch allocation came from address 0, which seems unlikely to end
well. So the only case we might actually care would be if the allocation
triggering the BUG_ON was the last one.

I think we probably don't care about this obscure corner case, especially
given that the allocations are starting from 0x0001 and using enough
scratch to wrap seems like an indication the world has gone wrong anyway.

Maybe you want to discuss some of this in an updated commit message.

> Signed-off-by: Anthony PERARD 

Acked-by: Ian Campbell 

(NB: hvmloader is formally maintained by the X86 ARCHITECTURE maintainers,
not the tools ones)

> ---
>  tools/firmware/hvmloader/util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/firmware/hvmloader/util.c
> b/tools/firmware/hvmloader/util.c
> index d779fd7..42e8af4 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
>  align = 16;
>  
>  s = (scratch_start + align - 1) & ~(align - 1);
> -e = s + size - 1;
> +e = s + size;
>  
>  BUG_ON(e < s);
>  

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [RFC PATCH v2 01/16] hvmloader: Fix scratch_alloc to avoid overlaps

[Anthony PERARD]

scratch_alloc() set scratch_start to the last byte of the current
allocation.  The value of scratch_start is then reused as is (if it is
already aligned) in the next allocation.  This result in a potential reuse
of the last byte of the previous allocation.

Signed-off-by: Anthony PERARD 
---
 tools/firmware/hvmloader/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index d779fd7..42e8af4 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -479,7 +479,7 @@ void *scratch_alloc(uint32_t size, uint32_t align)
 align = 16;
 
 s = (scratch_start + align - 1) & ~(align - 1);
-e = s + size - 1;
+e = s + size;
 
 BUG_ON(e < s);
 
-- 
Anthony PERARD


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel