[PATCH] Fix corruption error in rh_alloc_fixed()
There is an error in rh_alloc_fixed() of the Remote Heap code: If there is at least one free block blk won't be NULL at the end of the search loop, so -ENOMEM won't be returned and the else branch of "if (bs == s || be == e)" will be taken, corrupting the management structures. Signed-off-by: Guillaume Knispel <[EMAIL PROTECTED]> --- Fix an error in rh_alloc_fixed() that made allocations succeed when they should fail, and corrupted management structures. diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c index 29b2941..45907c1 100644 --- a/arch/powerpc/lib/rheap.c +++ b/arch/powerpc/lib/rheap.c @@ -556,6 +556,7 @@ unsigned long rh_alloc_fixed(rh_info_t * info, unsigned long start, int size, co be = blk->start + blk->size; if (s >= bs && e <= be) break; + blk = NULL; } if (blk == NULL) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
Guillaume Knispel wrote:
> There is an error in rh_alloc_fixed() of the Remote Heap code:
> If there is at least one free block blk won't be NULL at the end of the
> search loop, so -ENOMEM won't be returned and the else branch of
> "if (bs == s || be == e)" will be taken, corrupting the management
> structures.
>
> Signed-off-by: Guillaume Knispel <[EMAIL PROTECTED]>
> ---
> Fix an error in rh_alloc_fixed() that made allocations succeed when
> they should fail, and corrupted management structures.
>
> diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c
> index 29b2941..45907c1 100644
> --- a/arch/powerpc/lib/rheap.c
> +++ b/arch/powerpc/lib/rheap.c
> @@ -556,6 +556,7 @@ unsigned long rh_alloc_fixed(rh_info_t * info, unsigned
> long start, int size, co
> be = blk->start + blk->size;
> if (s >= bs && e <= be)
> break;
> + blk = NULL;
> }
>
> if (blk == NULL)
This is a good catch, however, wouldn't it be better to do this:
list_for_each(l, &info->free_list) {
blk = list_entry(l, rh_block_t, list);
/* The range must lie entirely inside one free block */
bs = blk->start;
be = blk->start + blk->size;
if (s >= bs && e <= be)
break;
}
- if (blk == NULL)
+ if (blk == &info->free_list)
return (unsigned long) -ENOMEM;
I haven't tested this, but the if-statement at the end of the loop is meant to
check whether the list_for_each() loop got to the end or not.
What do you think?
--
Timur Tabi
Linux kernel developer at Freescale
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
On Tue, 09 Dec 2008 09:03:19 -0600
Timur Tabi <[EMAIL PROTECTED]> wrote:
> Guillaume Knispel wrote:
> > There is an error in rh_alloc_fixed() of the Remote Heap code:
> > If there is at least one free block blk won't be NULL at the end of the
> > search loop, so -ENOMEM won't be returned and the else branch of
> > "if (bs == s || be == e)" will be taken, corrupting the management
> > structures.
> >
> > Signed-off-by: Guillaume Knispel <[EMAIL PROTECTED]>
> > ---
> > Fix an error in rh_alloc_fixed() that made allocations succeed when
> > they should fail, and corrupted management structures.
> >
> > diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c
> > index 29b2941..45907c1 100644
> > --- a/arch/powerpc/lib/rheap.c
> > +++ b/arch/powerpc/lib/rheap.c
> > @@ -556,6 +556,7 @@ unsigned long rh_alloc_fixed(rh_info_t * info, unsigned
> > long start, int size, co
> > be = blk->start + blk->size;
> > if (s >= bs && e <= be)
> > break;
> > + blk = NULL;
> > }
> >
> > if (blk == NULL)
>
> This is a good catch, however, wouldn't it be better to do this:
>
> list_for_each(l, &info->free_list) {
> blk = list_entry(l, rh_block_t, list);
> /* The range must lie entirely inside one free block */
> bs = blk->start;
> be = blk->start + blk->size;
> if (s >= bs && e <= be)
> break;
> }
>
> - if (blk == NULL)
> + if (blk == &info->free_list)
> return (unsigned long) -ENOMEM;
>
> I haven't tested this, but the if-statement at the end of the loop is meant to
> check whether the list_for_each() loop got to the end or not.
>
> What do you think?
blk = NULL; at the end of the loop is what is done in the more used
rh_alloc_align(), so for consistency either we change both or we use
the same construction here.
I also think that testing for &info->free_list is harder to understand
because you must have the linked list implementation in your head
(which a kernel developer should anyway so this is not so important)
Guillaume Knispel
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
Guillaume Knispel wrote: > blk = NULL; at the end of the loop is what is done in the more used > rh_alloc_align(), so for consistency either we change both or we use > the same construction here. > I also think that testing for &info->free_list is harder to understand > because you must have the linked list implementation in your head > (which a kernel developer should anyway so this is not so important) Fair enough. Acked-by: Timur Tabi <[EMAIL PROTECTED]> -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
On Tue, 09 Dec 2008 09:16:50 -0600 Timur Tabi wrote: > Guillaume Knispel wrote: > > > blk = NULL; at the end of the loop is what is done in the more used > > rh_alloc_align(), so for consistency either we change both or we use > > the same construction here. > > I also think that testing for &info->free_list is harder to understand > > because you must have the linked list implementation in your head > > (which a kernel developer should anyway so this is not so important) > > Fair enough. > > Acked-by: Timur Tabi > Kumar, can this go into your tree ? (copying the patch under so you have it at hand) There is an error in rh_alloc_fixed() of the Remote Heap code: If there is at least one free block blk won't be NULL at the end of the search loop, so -ENOMEM won't be returned and the else branch of "if (bs == s || be == e)" will be taken, corrupting the management structures. Signed-off-by: Guillaume Knispel --- Fix an error in rh_alloc_fixed() that made allocations succeed when they should fail, and corrupted management structures. diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c index 29b2941..45907c1 100644 --- a/arch/powerpc/lib/rheap.c +++ b/arch/powerpc/lib/rheap.c @@ -556,6 +556,7 @@ unsigned long rh_alloc_fixed(rh_info_t * info, unsigned long start, int size, co be = blk->start + blk->size; if (s >= bs && e <= be) break; + blk = NULL; } if (blk == NULL) -- Guillaume KNISPEL ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
Guillaume Knispel writes: > On Tue, 09 Dec 2008 09:16:50 -0600 > Timur Tabi wrote: > > > Guillaume Knispel wrote: > > > > > blk = NULL; at the end of the loop is what is done in the more used > > > rh_alloc_align(), so for consistency either we change both or we use > > > the same construction here. > > > I also think that testing for &info->free_list is harder to understand > > > because you must have the linked list implementation in your head > > > (which a kernel developer should anyway so this is not so important) > > > > Fair enough. > > > > Acked-by: Timur Tabi > > > > Kumar, can this go into your tree ? > (copying the patch under so you have it at hand) > > There is an error in rh_alloc_fixed() of the Remote Heap code: > If there is at least one free block blk won't be NULL at the end of the > search loop, so -ENOMEM won't be returned and the else branch of > "if (bs == s || be == e)" will be taken, corrupting the management > structures. > > Signed-off-by: Guillaume Knispel > --- > Fix an error in rh_alloc_fixed() that made allocations succeed when > they should fail, and corrupted management structures. What's the impact of this? Can it cause an oops? Is it a regression from 2.6.27? Should we be putting it in 2.6.28? Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
On Mon, 15 Dec 2008 08:21:05 +1100 Paul Mackerras wrote: > Guillaume Knispel writes: > > > On Tue, 09 Dec 2008 09:16:50 -0600 > > Timur Tabi wrote: > > > > > Guillaume Knispel wrote: > > > > > > > blk = NULL; at the end of the loop is what is done in the more used > > > > rh_alloc_align(), so for consistency either we change both or we use > > > > the same construction here. > > > > I also think that testing for &info->free_list is harder to understand > > > > because you must have the linked list implementation in your head > > > > (which a kernel developer should anyway so this is not so important) > > > > > > Fair enough. > > > > > > Acked-by: Timur Tabi > > > > > > > Kumar, can this go into your tree ? > > (copying the patch under so you have it at hand) > > > > There is an error in rh_alloc_fixed() of the Remote Heap code: > > If there is at least one free block blk won't be NULL at the end of the > > search loop, so -ENOMEM won't be returned and the else branch of > > "if (bs == s || be == e)" will be taken, corrupting the management > > structures. > > > > Signed-off-by: Guillaume Knispel > > --- > > Fix an error in rh_alloc_fixed() that made allocations succeed when > > they should fail, and corrupted management structures. > > What's the impact of this? Can it cause an oops? > > Is it a regression from 2.6.27? Should we be putting it in 2.6.28? > > Paul. The problem obviously only affect people that make use of rh_alloc_fixed(), which is the case when you program an MCC or a QMC controller of the CPM. Without the patch cpm_muram_alloc_fixed() succeed when it should not, for example when trying to allocate out of range areas or already allocated areas, so it is possible that buffer descriptors or other control structures used by other controllers get corrupted. Digging into old Linux (like 2.6.9, I haven't checked before), the problem seems to always have been present. Without this patch I experienced oops (sometimes panic, sometimes not) in various unrelated part (probably an indirect result of either corruption of rheap management structures or corruption caused by the CPM using crazy overwritten data) and also initialization of multi-channel control structures putting other communication controllers out-of-order. The only risk I can think off is that it could break some out of tree kernel space code which worked because of luck and a double error - for example when doing a single DPRam allocation from offset 0 while leaving an area reserved at the base of the DPRam. So I think it should be put in 2.6.28. Guillaume Knispel ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
The problem obviously only affect people that make use of rh_alloc_fixed(), which is the case when you program an MCC or a QMC controller of the CPM. Without the patch cpm_muram_alloc_fixed() succeed when it should not, for example when trying to allocate out of range areas or already allocated areas, so it is possible that buffer descriptors or other control structures used by other controllers get corrupted. Digging into old Linux (like 2.6.9, I haven't checked before), the problem seems to always have been present. Without this patch I experienced oops (sometimes panic, sometimes not) in various unrelated part (probably an indirect result of either corruption of rheap management structures or corruption caused by the CPM using crazy overwritten data) and also initialization of multi-channel control structures putting other communication controllers out-of-order. The only risk I can think off is that it could break some out of tree kernel space code which worked because of luck and a double error - for example when doing a single DPRam allocation from offset 0 while leaving an area reserved at the base of the DPRam. So I think it should be put in 2.6.28. Paul are you planning on picking this up for .28 if not I'll pick it up for .29 - k ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
Kumar Gala writes: > Paul are you planning on picking this up for .28 if not I'll pick it > up for .29 I was waiting for you to say if it needed to go in .28. Sounds like you don't think it's that urgent then? Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
On Dec 16, 2008, at 7:13 PM, Paul Mackerras wrote: Kumar Gala writes: Paul are you planning on picking this up for .28 if not I'll pick it up for .29 I was waiting for you to say if it needed to go in .28. Sounds like you don't think it's that urgent then? Since I have another patch for .28 I'll include in my merge tree. - k ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] Fix corruption error in rh_alloc_fixed()
On Dec 9, 2008, at 8:28 AM, Guillaume Knispel wrote: There is an error in rh_alloc_fixed() of the Remote Heap code: If there is at least one free block blk won't be NULL at the end of the search loop, so -ENOMEM won't be returned and the else branch of "if (bs == s || be == e)" will be taken, corrupting the management structures. Signed-off-by: Guillaume Knispel --- Fix an error in rh_alloc_fixed() that made allocations succeed when they should fail, and corrupted management structures. diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c index 29b2941..45907c1 100644 --- a/arch/powerpc/lib/rheap.c +++ b/arch/powerpc/lib/rheap.c @@ -556,6 +556,7 @@ unsigned long rh_alloc_fixed(rh_info_t * info, unsigned long start, int size, co be = blk->start + blk->size; if (s >= bs && e <= be) break; + blk = NULL; } if (blk == NULL) applied - k ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
