Xen devel - Mar 2013 - [PATCH] xentrace: fix off-by-one in calculate_tbuf_size

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1362151425 -3600
# Node ID 13c43170cd5f653d9590916df82f60597d1d5326
# Parent  3eb62c576a1a315b9f8655a186ead470abe69b31
xentrace: fix off-by-one in calculate_tbuf_size

Commit "xentrace: reduce trace buffer size to something mfn_offset can
reach" contains an off-by-one bug. max_mfn_offset needs to be reduced by
exactly the value of t_info_first_offset.

If the system has two cpus and the number of requested trace pages is
very large, the final number of trace pages + the offset will not fit
into a short. As a result the variable offset in alloc_trace_bufs() will
wrap while allocating buffers for the second cpu. Later
share_xen_page_with_privileged_guests() will be called with a wrong page
and the ASSERT in this function triggers. If the ASSERT is ignored by
running a non-dbg hypervisor the asserts in xentrace itself trigger
because "cons" is not aligned because the very last trace page for the
second cpu is a random mfn.

Thanks to Jan for the quick analysis.


Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 3eb62c576a1a -r 13c43170cd5f xen/common/trace.c
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -133,7 +133,7 @@ static int calculate_tbuf_size(unsigned 
      * The array of mfns for the highest cpu can start at the maximum value
      * mfn_offset can hold. So reduce the number of cpus and also the
mfn_offset.
      */
-    max_mfn_offset -= t_info_first_offset - 1;
+    max_mfn_offset -= t_info_first_offset;
     max_cpus--;
     if ( max_cpus )
         max_mfn_offset /= max_cpus;

>>> On 01.03.13 at 16:24, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1362151425 -3600
> # Node ID 13c43170cd5f653d9590916df82f60597d1d5326
> # Parent  3eb62c576a1a315b9f8655a186ead470abe69b31
> xentrace: fix off-by-one in calculate_tbuf_size
> 
> Commit "xentrace: reduce trace buffer size to something mfn_offset can
> reach" contains an off-by-one bug. max_mfn_offset needs to be reduced
by
> exactly the value of t_info_first_offset.
> 
> If the system has two cpus and the number of requested trace pages is
> very large, the final number of trace pages + the offset will not fit
> into a short. As a result the variable offset in alloc_trace_bufs() will
> wrap while allocating buffers for the second cpu. Later
> share_xen_page_with_privileged_guests() will be called with a wrong page
> and the ASSERT in this function triggers. If the ASSERT is ignored by
> running a non-dbg hypervisor the asserts in xentrace itself trigger
> because "cons" is not aligned because the very last trace page
for the
> second cpu is a random mfn.
> 
> Thanks to Jan for the quick analysis.
And with that obviously ...
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Jan Beulich <jbeulich@suse.com>
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -133,7 +133,7 @@ static int calculate_tbuf_size(unsigned 
>       * The array of mfns for the highest cpu can start at the maximum
value
>       * mfn_offset can hold. So reduce the number of cpus and also the 
> mfn_offset.
>       */
> -    max_mfn_offset -= t_info_first_offset - 1;
> +    max_mfn_offset -= t_info_first_offset;
>      max_cpus--;
>      if ( max_cpus )
>          max_mfn_offset /= max_cpus;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 01.03.13 at 16:24, Olaf Hering <olaf@aepfle.de> wrote:
> xentrace: fix off-by-one in calculate_tbuf_size
> 
> Commit "xentrace: reduce trace buffer size to something mfn_offset can
> reach" contains an off-by-one bug. max_mfn_offset needs to be reduced
by
> exactly the value of t_info_first_offset.
> 
> If the system has two cpus and the number of requested trace pages is
> very large, the final number of trace pages + the offset will not fit
> into a short. As a result the variable offset in alloc_trace_bufs() will
> wrap while allocating buffers for the second cpu. Later
> share_xen_page_with_privileged_guests() will be called with a wrong page
> and the ASSERT in this function triggers. If the ASSERT is ignored by
> running a non-dbg hypervisor the asserts in xentrace itself trigger
> because "cons" is not aligned because the very last trace page
for the
> second cpu is a random mfn.
> 
> Thanks to Jan for the quick analysis.
> 
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
George?

Thanks, Jan
> diff -r 3eb62c576a1a -r 13c43170cd5f xen/common/trace.c
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -133,7 +133,7 @@ static int calculate_tbuf_size(unsigned 
>       * The array of mfns for the highest cpu can start at the maximum
value
>       * mfn_offset can hold. So reduce the number of cpus and also the 
> mfn_offset.
>       */
> -    max_mfn_offset -= t_info_first_offset - 1;
> +    max_mfn_offset -= t_info_first_offset;
>      max_cpus--;
>      if ( max_cpus )
>          max_mfn_offset /= max_cpus;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

On Fri, Mar 1, 2013 at 3:24 PM, Olaf Hering <olaf@aepfle.de>
wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1362151425 -3600
> # Node ID 13c43170cd5f653d9590916df82f60597d1d5326
> # Parent  3eb62c576a1a315b9f8655a186ead470abe69b31
> xentrace: fix off-by-one in calculate_tbuf_size
>
> Commit "xentrace: reduce trace buffer size to something mfn_offset can
> reach" contains an off-by-one bug. max_mfn_offset needs to be reduced
by
> exactly the value of t_info_first_offset.
>
> If the system has two cpus and the number of requested trace pages is
> very large, the final number of trace pages + the offset will not fit
> into a short. As a result the variable offset in alloc_trace_bufs() will
> wrap while allocating buffers for the second cpu. Later
> share_xen_page_with_privileged_guests() will be called with a wrong page
> and the ASSERT in this function triggers. If the ASSERT is ignored by
> running a non-dbg hypervisor the asserts in xentrace itself trigger
> because "cons" is not aligned because the very last trace page
for the
> second cpu is a random mfn.
>
> Thanks to Jan for the quick analysis.
>
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
> diff -r 3eb62c576a1a -r 13c43170cd5f xen/common/trace.c
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -133,7 +133,7 @@ static int calculate_tbuf_size(unsigned
>       * The array of mfns for the highest cpu can start at the maximum
value
>       * mfn_offset can hold. So reduce the number of cpus and also the
mfn_offset.
>       */
> -    max_mfn_offset -= t_info_first_offset - 1;
> +    max_mfn_offset -= t_info_first_offset;
Hmm -- I think my intention was actually to be more conservative;
i.e., max_mfn_offset = max_mfn_offset - t_info_first_offset - 1.  But
it seems I did the opposite. :-)

In any case, this looks good:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>