This wasn''t defined correctly, thus allowing in the num_online_cpus() == NR_CPUS case to pass a corrupted MFN to Dom0. Signed-off-by: Jan Beulich <jbeulich@novell.com> --- 2010-06-15.orig/xen/common/trace.c 2010-06-28 11:58:37.000000000 +0200 +++ 2010-06-15/xen/common/trace.c 2010-06-28 11:58:37.000000000 +0200 @@ -51,7 +51,7 @@ static struct t_info *t_info; #define T_INFO_PAGES 2 /* Size fixed at 2 pages for now. */ #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE)) /* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */ -#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t)) +#define T_INFO_FIRST_OFFSET (((2 + NR_CPUS) * sizeof(uint16_t)) / sizeof(uint32_t)) static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs); static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data); static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jun-30 14:52 UTC
[Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
Good catch. This fix produces the correct index, but: * It''s not as clear, IMHO, where the math is coming from * I think it may give the wrong result if the t_info struct ever changes (e.g., more data before the cpu offset list) The bug in the original math was that I should have added 3 to round up, rather than 1. I''m attaching a patch that will hopefully fix the bug and make it more clear. Thoughts? -George On Tue, Jun 29, 2010 at 4:32 PM, Jan Beulich <JBeulich@novell.com> wrote:> This wasn''t defined correctly, thus allowing in the > num_online_cpus() == NR_CPUS case to pass a corrupted MFN to > Dom0. > > Signed-off-by: Jan Beulich <jbeulich@novell.com> > > --- 2010-06-15.orig/xen/common/trace.c 2010-06-28 11:58:37.000000000 +0200 > +++ 2010-06-15/xen/common/trace.c 2010-06-28 11:58:37.000000000 +0200 > @@ -51,7 +51,7 @@ static struct t_info *t_info; > #define T_INFO_PAGES 2 /* Size fixed at 2 pages for now. */ > #define T_INFO_SIZE ((T_INFO_PAGES)*(PAGE_SIZE)) > /* t_info.tbuf_size + list of mfn offsets + 1 to round up / sizeof uint32_t */ > -#define T_INFO_FIRST_OFFSET ((sizeof(int16_t) + NR_CPUS * sizeof(int16_t) + 1) / sizeof(uint32_t)) > +#define T_INFO_FIRST_OFFSET (((2 + NR_CPUS) * sizeof(uint16_t)) / sizeof(uint32_t)) > static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs); > static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data); > static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock); > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Jun-30 15:15 UTC
[Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
>>> On 30.06.10 at 16:52, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > Good catch. This fix produces the correct index, but: > * It''s not as clear, IMHO, where the math is coming from > * I think it may give the wrong result if the t_info struct ever > changes (e.g., more data before the cpu offset list)That part your patch doesn''t address either - rather than sizeof(uint16_t) as the first part of the expression you''d need to use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). Btw., didn''t we agree that public headers shouldn''t make use of language extensions? struct t_info uses a variable sized array, which is an extension (standard only in C99).> The bug in the original math was that I should have added 3 to round > up, rather than 1.Correct.> I''m attaching a patch that will hopefully fix the bug and make it more > clear. Thoughts?Leaving aside the comment above this looks okay to me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jun-30 15:28 UTC
[Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
Jan Beulich wrote:> That part your patch doesn''t address either - rather than > sizeof(uint16_t) as the first part of the expression you''d need to > use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). >I was assuming that when someone changed struct t_info that they''d modify this macro as well; I suppose then that the two complaints are really different aspects of the same one -- that it might not be clear to the person who adjusts struct t_info how to translate those changes into T_INFO_FIRST_OFFSET. I think this way is more clear. I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS] (or perhaps ...[num_possible_cpus]). Hmm... let me see what I can come up with.> Btw., didn''t we agree that public headers shouldn''t make use of > language extensions? struct t_info uses a variable sized array, > which is an extension (standard only in C99). >I''m not an expert in this. It''s lot more hassle to lay out the data the way I''d like without it. I''ll defer judgment to Keir. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jun-30 16:10 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
Here''s a version that calculates t_info_first_offset during initialization, based on the actual layout of struct t_info and NR_CPUs. -George On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote:> Jan Beulich wrote: >> >> That part your patch doesn''t address either - rather than >> sizeof(uint16_t) as the first part of the expression you''d need to >> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). >> > > I was assuming that when someone changed struct t_info that they''d modify > this macro as well; I suppose then that the two complaints are really > different aspects of the same one -- that it might not be clear to the > person who adjusts struct t_info how to translate those changes into > T_INFO_FIRST_OFFSET. I think this way is more clear. > > I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS] > (or perhaps ...[num_possible_cpus]). Hmm... let me see what I can come up > with. >> >> Btw., didn''t we agree that public headers shouldn''t make use of >> language extensions? struct t_info uses a variable sized array, >> which is an extension (standard only in C99). >> > > I''m not an expert in this. It''s lot more hassle to lay out the data the way > I''d like without it. I''ll defer judgment to Keir. > > -George > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jun-30 16:12 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
Oops, please use this version, which used the appropraite gkprintk() settings... -George On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:> Here''s a version that calculates t_info_first_offset during > initialization, based on the actual layout of struct t_info and > NR_CPUs. > > -George > > On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap > <george.dunlap@eu.citrix.com> wrote: >> Jan Beulich wrote: >>> >>> That part your patch doesn''t address either - rather than >>> sizeof(uint16_t) as the first part of the expression you''d need to >>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). >>> >> >> I was assuming that when someone changed struct t_info that they''d modify >> this macro as well; I suppose then that the two complaints are really >> different aspects of the same one -- that it might not be clear to the >> person who adjusts struct t_info how to translate those changes into >> T_INFO_FIRST_OFFSET. I think this way is more clear. >> >> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS] >> (or perhaps ...[num_possible_cpus]). Hmm... let me see what I can come up >> with. >>> >>> Btw., didn''t we agree that public headers shouldn''t make use of >>> language extensions? struct t_info uses a variable sized array, >>> which is an extension (standard only in C99). >>> >> >> I''m not an expert in this. It''s lot more hassle to lay out the data the way >> I''d like without it. I''ll defer judgment to Keir. >> >> -George >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jun-30 17:14 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
I''ve lost track of all these trace patches. Please send a new set of patches with finalised Sign-offs and Acks as appropriate when you reach agreement. -- Keir On 30/06/2010 17:12, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote:> Oops, please use this version, which used the appropraite gkprintk() > settings... > > -George > > On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap > <George.Dunlap@eu.citrix.com> wrote: >> Here''s a version that calculates t_info_first_offset during >> initialization, based on the actual layout of struct t_info and >> NR_CPUs. >> >> -George >> >> On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap >> <george.dunlap@eu.citrix.com> wrote: >>> Jan Beulich wrote: >>>> >>>> That part your patch doesn''t address either - rather than >>>> sizeof(uint16_t) as the first part of the expression you''d need to >>>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). >>>> >>> >>> I was assuming that when someone changed struct t_info that they''d modify >>> this macro as well; I suppose then that the two complaints are really >>> different aspects of the same one -- that it might not be clear to the >>> person who adjusts struct t_info how to translate those changes into >>> T_INFO_FIRST_OFFSET. I think this way is more clear. >>> >>> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS] >>> (or perhaps ...[num_possible_cpus]). Hmm... let me see what I can come up >>> with. >>>> >>>> Btw., didn''t we agree that public headers shouldn''t make use of >>>> language extensions? struct t_info uses a variable sized array, >>>> which is an extension (standard only in C99). >>>> >>> >>> I''m not an expert in this. It''s lot more hassle to lay out the data the way >>> I''d like without it. I''ll defer judgment to Keir. >>> >>> -George >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xensource.com >>> http://lists.xensource.com/xen-devel >>> >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jun-30 18:12 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
Jan, I''ve got all the ones I''ve acked / sent my own versions of in a mercurial queue, easy to patchbomb. Let me know if you have any more feedback on my versions. My only objection to the last one was the volatile stuff; if you''re OK with it, I''ll send a series with everything but the volatile stuff, and we can continue talking about it. -George Keir Fraser wrote:> I''ve lost track of all these trace patches. Please send a new set of patches > with finalised Sign-offs and Acks as appropriate when you reach agreement. > > -- Keir > > On 30/06/2010 17:12, "George Dunlap" <George.Dunlap@eu.citrix.com> wrote: > > >> Oops, please use this version, which used the appropraite gkprintk() >> settings... >> >> -George >> >> On Wed, Jun 30, 2010 at 5:10 PM, George Dunlap >> <George.Dunlap@eu.citrix.com> wrote: >> >>> Here''s a version that calculates t_info_first_offset during >>> initialization, based on the actual layout of struct t_info and >>> NR_CPUs. >>> >>> -George >>> >>> On Wed, Jun 30, 2010 at 4:28 PM, George Dunlap >>> <george.dunlap@eu.citrix.com> wrote: >>> >>>> Jan Beulich wrote: >>>> >>>>> That part your patch doesn''t address either - rather than >>>>> sizeof(uint16_t) as the first part of the expression you''d need to >>>>> use sizeof(struct t_info) or offsetof(struct t_info, mfn_offset). >>>>> >>>>> >>>> I was assuming that when someone changed struct t_info that they''d modify >>>> this macro as well; I suppose then that the two complaints are really >>>> different aspects of the same one -- that it might not be clear to the >>>> person who adjusts struct t_info how to translate those changes into >>>> T_INFO_FIRST_OFFSET. I think this way is more clear. >>>> >>>> I suppose even better might be to calculate t_info.mfn_mfn_offset[NR_CPUS] >>>> (or perhaps ...[num_possible_cpus]). Hmm... let me see what I can come up >>>> with. >>>> >>>>> Btw., didn''t we agree that public headers shouldn''t make use of >>>>> language extensions? struct t_info uses a variable sized array, >>>>> which is an extension (standard only in C99). >>>>> >>>>> >>>> I''m not an expert in this. It''s lot more hassle to lay out the data the way >>>> I''d like without it. I''ll defer judgment to Keir. >>>> >>>> -George >>>> >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xensource.com >>>> http://lists.xensource.com/xen-devel >>>> >>>> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Jul-01 09:49 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
>>> On 30.06.10 at 18:12, George Dunlap <George.Dunlap@eu.citrix.com> wrote: >+ offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL);Why can''t this just be offsetof(struct t_info, mfn_offset[NR_CPUS])? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Jul-01 09:50 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
>>> On 30.06.10 at 20:12, George Dunlap <george.dunlap@eu.citrix.com> wrote: > I''ve got all the ones I''ve acked / sent my own versions of in a > mercurial queue, easy to patchbomb. Let me know if you have any more > feedback on my versions. > > My only objection to the last one was the volatile stuff; if you''re OK > with it, I''ll send a series with everything but the volatile stuff, and > we can continue talking about it.Sounds good to me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Jul-01 09:53 UTC
Re: [Xen-devel] Re: [PATCH 2/6] trace: fix T_INFO_FIRST_OFFSET
On 01/07/10 10:49, Jan Beulich wrote:>>>> On 30.06.10 at 18:12, George Dunlap<George.Dunlap@eu.citrix.com> wrote: >> + offset_in_bytes = ((char *)&((struct t_info *)(NULL))->mfn_offset[NR_CPUS]) - (char *)(NULL); > > Why can''t this just be offsetof(struct t_info, mfn_offset[NR_CPUS])?Ah... forgot about that one. Thanks, I''ll modify that in my patch series. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel