[PATCH] trace: Fix incorrect number of pages used for trace metadata

[PATCH] trace: Fix incorrect number of pages used for trace metadata

[Igor Druzhinin]

As long as t_info_first_offset is calculated in uint32_t offsets we need to
multiply it by sizeof(uint32_t) in order to get the right number of pages
for trace metadata. Not doing that makes it impossible to read the trace
buffer correctly from userspace for some corner cases.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 xen/common/trace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/trace.c b/xen/common/trace.c
index f651cf3..a9563cc 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -149,7 +149,7 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
     }
 
     t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
-    t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
+    t_info_pages = PFN_UP(t_info_first_offset * sizeof(uint32_t) + t_info_words);
     printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
            "for %u trace pages on %u cpus\n",
            t_info_pages, pages, num_online_cpus());
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On 29/09/16 14:53, Igor Druzhinin wrote:
> As long as t_info_first_offset is calculated in uint32_t offsets we need to
> multiply it by sizeof(uint32_t) in order to get the right number of pages
> for trace metadata. Not doing that makes it impossible to read the trace
> buffer correctly from userspace for some corner cases.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
correct formula to calculate t_info_pages".  But that one was presumably
written (and Acked by me) because the variable name there,
t_info_first_offset, is confusing.

The other mistake in fbf96e6 is that before t_info_words was actually
denominated in words; but after it's denominated in bytes (which is
again confusing).

What about something like the attached instead?  This should fix your
problem while making the code clearer.

 -George



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xen-trace-Fix-trace-metadata-page-count-calculation-.patch --]
[-- Type: text/x-diff; name="0001-xen-trace-Fix-trace-metadata-page-count-calculation-.patch", Size: 1806 bytes --]

From b0252cec4a96fea28faa85c47ab0f5d5997444ad Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 30 Sep 2016 15:42:56 +0100
Subject: [PATCH] xen/trace: Fix trace metadata page count calculation (revert
 fbf96e6)

Changeset fbf96e6, "xentrace: correct formula to calculate
t_info_pages", broke the trace metadata page count calculation, by
mistaking t_info_first_offset as denominated in bytes, when in fact it
is denominated in words (uint32_t).

Effectively revert that change, and put a comment there to reduce the
chance that someone will make that mistake in the future.

Spotted-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Olaf Hering <olaf@aepfle.de>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/common/trace.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/trace.c b/xen/common/trace.c
index f651cf3..2f4ecca 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -148,8 +148,12 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
         pages = max_pages;
     }
 
-    t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
-    t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
+    /* 
+     * NB this calculation is correct, because t_info_first_offset is
+     * in words, not bytes, not bytes
+     */
+    t_info_words = num_online_cpus() * pages + t_info_first_offset;
+    t_info_pages = PFN_UP(t_info_words * sizeof(uint32_t));
     printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
            "for %u trace pages on %u cpus\n",
            t_info_pages, pages, num_online_cpus());
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2867 bytes --]

On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> > As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> > multiply it by sizeof(uint32_t) in order to get the right number of pages
>> > for trace metadata. Not doing that makes it impossible to read the trace
>> > buffer correctly from userspace for some corner cases.
>> > 
>> > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages".  But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
>
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
>
> What about something like the attached instead?  This should fix your
> problem while making the code clearer.
>
>  -George
>
>
>
> From b0252cec4a96fea28faa85c47ab0f5d5997444ad Mon Sep 17 00:00:00 2001
> From: George Dunlap <george.dunlap@citrix.com>
> Date: Fri, 30 Sep 2016 15:42:56 +0100
> Subject: [PATCH] xen/trace: Fix trace metadata page count calculation (revert
>  fbf96e6)
>
> Changeset fbf96e6, "xentrace: correct formula to calculate
> t_info_pages", broke the trace metadata page count calculation, by
> mistaking t_info_first_offset as denominated in bytes, when in fact it
> is denominated in words (uint32_t).
>
> Effectively revert that change, and put a comment there to reduce the
> chance that someone will make that mistake in the future.
>
> Spotted-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Olaf Hering <olaf@aepfle.de>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/common/trace.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/trace.c b/xen/common/trace.c
> index f651cf3..2f4ecca 100644
> --- a/xen/common/trace.c
> +++ b/xen/common/trace.c
> @@ -148,8 +148,12 @@ static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
>          pages = max_pages;
>      }
>  
> -    t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
> -    t_info_pages = PFN_UP(t_info_first_offset + t_info_words);
> +    /* 
> +     * NB this calculation is correct, because t_info_first_offset is
> +     * in words, not bytes, not bytes

s/, not bytes$/./

~Andrew

> +     */
> +    t_info_words = num_online_cpus() * pages + t_info_first_offset;
> +    t_info_pages = PFN_UP(t_info_words * sizeof(uint32_t));
>      printk(XENLOG_INFO "xentrace: requesting %u t_info pages "
>             "for %u trace pages on %u cpus\n",
>             t_info_pages, pages, num_online_cpus());
> -- 2.1.4


[-- Attachment #1.2: Type: text/html, Size: 4969 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[George Dunlap]

On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> multiply it by sizeof(uint32_t) in order to get the right number of pages
>> for trace metadata. Not doing that makes it impossible to read the trace
>> buffer correctly from userspace for some corner cases.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> 
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages".  But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
> 
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
> 
> What about something like the attached instead?  This should fix your
> problem while making the code clearer.

Obviously that comment shouldn't include the second "not bytes".  We're
not writing folk songs here...

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[Igor Druzhinin]

On 30/09/16 15:46, George Dunlap wrote:
> On 29/09/16 14:53, Igor Druzhinin wrote:
>> As long as t_info_first_offset is calculated in uint32_t offsets we need to
>> multiply it by sizeof(uint32_t) in order to get the right number of pages
>> for trace metadata. Not doing that makes it impossible to read the trace
>> buffer correctly from userspace for some corner cases.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
> correct formula to calculate t_info_pages".  But that one was presumably
> written (and Acked by me) because the variable name there,
> t_info_first_offset, is confusing.
>
> The other mistake in fbf96e6 is that before t_info_words was actually
> denominated in words; but after it's denominated in bytes (which is
> again confusing).
>
> What about something like the attached instead?  This should fix your
> problem while making the code clearer.
>
>  -George
>
>

Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[George Dunlap]

On 30/09/16 17:04, Igor Druzhinin wrote:
> On 30/09/16 15:46, George Dunlap wrote:
>> On 29/09/16 14:53, Igor Druzhinin wrote:
>>> As long as t_info_first_offset is calculated in uint32_t offsets we
>>> need to
>>> multiply it by sizeof(uint32_t) in order to get the right number of
>>> pages
>>> for trace metadata. Not doing that makes it impossible to read the trace
>>> buffer correctly from userspace for some corner cases.
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
>> correct formula to calculate t_info_pages".  But that one was presumably
>> written (and Acked by me) because the variable name there,
>> t_info_first_offset, is confusing.
>>
>> The other mistake in fbf96e6 is that before t_info_words was actually
>> denominated in words; but after it's denominated in bytes (which is
>> again confusing).
>>
>> What about something like the attached instead?  This should fix your
>> problem while making the code clearer.
>>
>>  -George
>>
>>
> 
> Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Thanks.  Any chance I could get a Tested-by as well? :-)

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] trace: Fix incorrect number of pages used for trace metadata

[Igor Druzhinin]

Checked that.

Tested-by: Igor Druzhinin <igor.druzhinin@citrix.com>

On 30/09/16 17:12, George Dunlap wrote:
> On 30/09/16 17:04, Igor Druzhinin wrote:
>> On 30/09/16 15:46, George Dunlap wrote:
>>> On 29/09/16 14:53, Igor Druzhinin wrote:
>>>> As long as t_info_first_offset is calculated in uint32_t offsets we
>>>> need to
>>>> multiply it by sizeof(uint32_t) in order to get the right number of
>>>> pages
>>>> for trace metadata. Not doing that makes it impossible to read the trace
>>>> buffer correctly from userspace for some corner cases.
>>>>
>>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>>
>>> Hmm, looks like we actually want to revert this c/s fbf96e6, "xentrace:
>>> correct formula to calculate t_info_pages".  But that one was presumably
>>> written (and Acked by me) because the variable name there,
>>> t_info_first_offset, is confusing.
>>>
>>> The other mistake in fbf96e6 is that before t_info_words was actually
>>> denominated in words; but after it's denominated in bytes (which is
>>> again confusing).
>>>
>>> What about something like the attached instead?  This should fix your
>>> problem while making the code clearer.
>>>
>>>  -George
>>>
>>>
>>
>> Reviewed-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> Thanks.  Any chance I could get a Tested-by as well? :-)
>
>  -George
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel