From: zijun_hu <[email protected]>
the LSB of a chunk->map element is used for free/in-use flag of a area
and the other bits for offset, the sufficient and necessary condition of
this usage is that both size and alignment of a area must be even numbers
however, pcpu_alloc() doesn't force its @align parameter a even number
explicitly, so a odd @align maybe causes a series of errors, see below
example for concrete descriptions.
lets assume area [16, 36) is free but its previous one is in-use, we want
to allocate a @size == 8 and @align == 7 area. the larger area [16, 36) is
split to three areas [16, 21), [21, 29), [29, 36) eventually. however, due
to the usage for a chunk->map element, the actual offset of the aim area
[21, 29) is 21 but is recorded in relevant element as 20; moreover the
residual tail free area [29, 36) is mistook as in-use and is lost silently
as explained above, inaccurate either offset or free/in-use state of
a area is recorded into relevant chunk->map element if request a odd
alignment area, and so causes memory leakage issue
fix it by forcing the @align of a area to allocate a even number
as do for @size.
BTW, macro ALIGN() within pcpu_fit_in_area() is replaced by roundup() too
due to back reason. in order to align a value @v up to @a boundary, macro
roundup(v, a) is more generic than ALIGN(x, a); the latter doesn't work
well when @a isn't a power of 2 value. for example, roundup(10, 6) == 12
but ALIGN(10, 6) == 10, the former result is desired obviously
Signed-off-by: zijun_hu <[email protected]>
---
include/linux/kernel.h | 1 +
mm/percpu.c | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 74fd6f05bc5b..ddf46638ef21 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -45,6 +45,7 @@
#define REPEAT_BYTE(x) ((~0ul / 0xff) * (x))
+/* @a is a power of 2 value */
#define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
#define __ALIGN_MASK(x, mask) __ALIGN_KERNEL_MASK((x), (mask))
#define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a)))
diff --git a/mm/percpu.c b/mm/percpu.c
index c2f0d9734d8c..26d1c73bd9e2 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -502,7 +502,7 @@ static int pcpu_fit_in_area(struct pcpu_chunk *chunk, int off, int this_size,
int cand_off = off;
while (true) {
- int head = ALIGN(cand_off, align) - off;
+ int head = roundup(cand_off, align) - off;
int page_start, page_end, rs, re;
if (this_size < head + size)
@@ -879,11 +879,13 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved,
/*
* We want the lowest bit of offset available for in-use/free
- * indicator, so force >= 16bit alignment and make size even.
+ * indicator, so force alignment >= 2 even and make size even.
*/
if (unlikely(align < 2))
align = 2;
+ if (WARN_ON_ONCE(!IS_ALIGNED(align, 2)))
+ align = ALIGN(align, 2);
size = ALIGN(size, 2);
if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE)) {
--
1.9.1
On Tue 11-10-16 21:24:50, zijun_hu wrote:
> From: zijun_hu <[email protected]>
>
> the LSB of a chunk->map element is used for free/in-use flag of a area
> and the other bits for offset, the sufficient and necessary condition of
> this usage is that both size and alignment of a area must be even numbers
> however, pcpu_alloc() doesn't force its @align parameter a even number
> explicitly, so a odd @align maybe causes a series of errors, see below
> example for concrete descriptions.
Is or was there any user who would use a different than even (or power of 2)
alighment? If not is this really worth handling?
--
Michal Hocko
SUSE Labs
On 2016/10/12 1:22, Michal Hocko wrote:
> On Tue 11-10-16 21:24:50, zijun_hu wrote:
>> From: zijun_hu <[email protected]>
>>
>> the LSB of a chunk->map element is used for free/in-use flag of a area
>> and the other bits for offset, the sufficient and necessary condition of
>> this usage is that both size and alignment of a area must be even numbers
>> however, pcpu_alloc() doesn't force its @align parameter a even number
>> explicitly, so a odd @align maybe causes a series of errors, see below
>> example for concrete descriptions.
>
> Is or was there any user who would use a different than even (or power of 2)
> alighment? If not is this really worth handling?
>
it seems only a power of 2 alignment except 1 can make sure it work very well,
that is a strict limit, maybe this more strict limit should be checked
i don't know since there are too many sources and too many users and too many
use cases. even if nobody, i can't be sure that it doesn't happens in the future
it is worth since below reasons
1) if it is used in right ways, this patch have no impact; otherwise, it can alert
user by warning message and correct the behavior.
is it better that a warning message and correcting than resulting in many terrible
error silently under a special case by change?
it can make program more stronger.
2) does any alignment but 1 means a power of 2 alignment conventionally and implicitly?
if not, is it better that adjusting both @align and @size uniformly based on the sufficient
necessary condition than mixing supposing one part is right and correcting the other?
i find that there is BUG_ON(!is_power_of_2(align)) statement in mm/vmalloc.c
3) this simple fix can make the function applicable in wider range, it hints the reader
that the lowest requirement for alignment is a even number
4) for char a[10][10]; char (*p)[10]; if a user want to allocate a @size = 10 and
@align = 10 memory block, should we reject the user's request?
On Wed 12-10-16 08:28:17, zijun_hu wrote:
> On 2016/10/12 1:22, Michal Hocko wrote:
> > On Tue 11-10-16 21:24:50, zijun_hu wrote:
> >> From: zijun_hu <[email protected]>
> >>
> >> the LSB of a chunk->map element is used for free/in-use flag of a area
> >> and the other bits for offset, the sufficient and necessary condition of
> >> this usage is that both size and alignment of a area must be even numbers
> >> however, pcpu_alloc() doesn't force its @align parameter a even number
> >> explicitly, so a odd @align maybe causes a series of errors, see below
> >> example for concrete descriptions.
> >
> > Is or was there any user who would use a different than even (or power of 2)
> > alighment? If not is this really worth handling?
> >
>
> it seems only a power of 2 alignment except 1 can make sure it work very well,
> that is a strict limit, maybe this more strict limit should be checked
I fail to see how any other alignment would actually make any sense
what so ever. Look, I am not a maintainer of this code but adding a new
code to catch something that doesn't make any sense sounds dubious at
best to me.
I could understand this patch if you see a problem and want to prevent
it from repeating bug doing these kind of changes just in case sounds
like a bad idea.
--
Michal Hocko
SUSE Labs
On 10/12/2016 02:53 PM, Michal Hocko wrote:
> On Wed 12-10-16 08:28:17, zijun_hu wrote:
>> On 2016/10/12 1:22, Michal Hocko wrote:
>>> On Tue 11-10-16 21:24:50, zijun_hu wrote:
>>>> From: zijun_hu <[email protected]>
>>>>
>>>> the LSB of a chunk->map element is used for free/in-use flag of a area
>>>> and the other bits for offset, the sufficient and necessary condition of
>>>> this usage is that both size and alignment of a area must be even numbers
>>>> however, pcpu_alloc() doesn't force its @align parameter a even number
>>>> explicitly, so a odd @align maybe causes a series of errors, see below
>>>> example for concrete descriptions.
>>>
>>> Is or was there any user who would use a different than even (or power of 2)
>>> alighment? If not is this really worth handling?
>>>
>>
>> it seems only a power of 2 alignment except 1 can make sure it work very well,
>> that is a strict limit, maybe this more strict limit should be checked
>
> I fail to see how any other alignment would actually make any sense
> what so ever. Look, I am not a maintainer of this code but adding a new
> code to catch something that doesn't make any sense sounds dubious at
> best to me.
>
> I could understand this patch if you see a problem and want to prevent
> it from repeating bug doing these kind of changes just in case sounds
> like a bad idea.
>
thanks for your reply
should we have a generic discussion whether such patches which considers
many boundary or rare conditions are necessary.
should we make below declarations as conventions
1) when we say 'alignment', it means align to a power of 2 value
for example, aligning value @v to @b implicit @v is power of 2
, align 10 to 4 is 12
2) when we say 'round value @v up/down to boundary @b', it means the
result is a times of @b, it don't requires @b is a power of 2
On 10/12/2016 02:53 PM, Michal Hocko wrote:
> On Wed 12-10-16 08:28:17, zijun_hu wrote:
>> On 2016/10/12 1:22, Michal Hocko wrote:
>>> On Tue 11-10-16 21:24:50, zijun_hu wrote:
>>>> From: zijun_hu <[email protected]>
>>>>
>>>> the LSB of a chunk->map element is used for free/in-use flag of a area
>>>> and the other bits for offset, the sufficient and necessary condition of
>>>> this usage is that both size and alignment of a area must be even numbers
>>>> however, pcpu_alloc() doesn't force its @align parameter a even number
>>>> explicitly, so a odd @align maybe causes a series of errors, see below
>>>> example for concrete descriptions.
>>>
>>> Is or was there any user who would use a different than even (or power of 2)
>>> alighment? If not is this really worth handling?
>>>
>>
>> it seems only a power of 2 alignment except 1 can make sure it work very well,
>> that is a strict limit, maybe this more strict limit should be checked
>
> I fail to see how any other alignment would actually make any sense
> what so ever. Look, I am not a maintainer of this code but adding a new
> code to catch something that doesn't make any sense sounds dubious at
> best to me.
>
> I could understand this patch if you see a problem and want to prevent
> it from repeating bug doing these kind of changes just in case sounds
> like a bad idea.
>
thanks for your reply
should we have a generic discussion whether such patches which considers
many boundary or rare conditions are necessary.
i found the following code segments in mm/vmalloc.c
static struct vmap_area *alloc_vmap_area(unsigned long size,
unsigned long align,
unsigned long vstart, unsigned long vend,
int node, gfp_t gfp_mask)
{
...
BUG_ON(!size);
BUG_ON(offset_in_page(size));
BUG_ON(!is_power_of_2(align));
should we make below declarations as conventions
1) when we say 'alignment', it means align to a power of 2 value
for example, aligning value @v to @b implicit @v is power of 2
, align 10 to 4 is 12
2) when we say 'round value @v up/down to boundary @b', it means the
result is a times of @b, it don't requires @b is a power of 2
On 10/12/2016 04:25 PM, Michal Hocko wrote:
> On Wed 12-10-16 15:24:33, zijun_hu wrote:
>> On 10/12/2016 02:53 PM, Michal Hocko wrote:
>>> On Wed 12-10-16 08:28:17, zijun_hu wrote:
>>>> On 2016/10/12 1:22, Michal Hocko wrote:
>>>>> On Tue 11-10-16 21:24:50, zijun_hu wrote:
>>>>>> From: zijun_hu <[email protected]>
>>>>>>
>> should we have a generic discussion whether such patches which considers
>> many boundary or rare conditions are necessary.
>
> In general, I believe that kernel internal interfaces which have no
> userspace exposure shouldn't be cluttered with sanity checks.
>
you are right and i agree with you. but there are many internal interfaces
perform sanity checks in current linux sources
>> i found the following code segments in mm/vmalloc.c
>> static struct vmap_area *alloc_vmap_area(unsigned long size,
>> unsigned long align,
>> unsigned long vstart, unsigned long vend,
>> int node, gfp_t gfp_mask)
>> {
>> ...
>>
>> BUG_ON(!size);
>> BUG_ON(offset_in_page(size));
>> BUG_ON(!is_power_of_2(align));
>
> See a recent Linus rant about BUG_ONs. These BUG_ONs are quite old and
> from a quick look they are even unnecessary. So rather than adding more
> of those, I think removing those that are not needed is much more
> preferred.
>
i notice that, and the above code segments is used to illustrate that
input parameter checking is necessary sometimes
>> should we make below declarations as conventions
>> 1) when we say 'alignment', it means align to a power of 2 value
>> for example, aligning value @v to @b implicit @v is power of 2
>> , align 10 to 4 is 12
>
> alignment other than power-of-two makes only very limited sense to me.
>
you are right and i agree with you.
>> 2) when we say 'round value @v up/down to boundary @b', it means the
>> result is a times of @b, it don't requires @b is a power of 2
>
i will write to linus to ask for opinions whether we should declare
the meaning of 'align' and 'round up/down' formally and whether such
patches are necessary
On Wed 12-10-16 15:24:33, zijun_hu wrote:
> On 10/12/2016 02:53 PM, Michal Hocko wrote:
> > On Wed 12-10-16 08:28:17, zijun_hu wrote:
> >> On 2016/10/12 1:22, Michal Hocko wrote:
> >>> On Tue 11-10-16 21:24:50, zijun_hu wrote:
> >>>> From: zijun_hu <[email protected]>
> >>>>
> >>>> the LSB of a chunk->map element is used for free/in-use flag of a area
> >>>> and the other bits for offset, the sufficient and necessary condition of
> >>>> this usage is that both size and alignment of a area must be even numbers
> >>>> however, pcpu_alloc() doesn't force its @align parameter a even number
> >>>> explicitly, so a odd @align maybe causes a series of errors, see below
> >>>> example for concrete descriptions.
> >>>
> >>> Is or was there any user who would use a different than even (or power of 2)
> >>> alighment? If not is this really worth handling?
> >>>
> >>
> >> it seems only a power of 2 alignment except 1 can make sure it work very well,
> >> that is a strict limit, maybe this more strict limit should be checked
> >
> > I fail to see how any other alignment would actually make any sense
> > what so ever. Look, I am not a maintainer of this code but adding a new
> > code to catch something that doesn't make any sense sounds dubious at
> > best to me.
> >
> > I could understand this patch if you see a problem and want to prevent
> > it from repeating bug doing these kind of changes just in case sounds
> > like a bad idea.
> >
>
> thanks for your reply
>
> should we have a generic discussion whether such patches which considers
> many boundary or rare conditions are necessary.
In general, I believe that kernel internal interfaces which have no
userspace exposure shouldn't be cluttered with sanity checks.
> i found the following code segments in mm/vmalloc.c
> static struct vmap_area *alloc_vmap_area(unsigned long size,
> unsigned long align,
> unsigned long vstart, unsigned long vend,
> int node, gfp_t gfp_mask)
> {
> ...
>
> BUG_ON(!size);
> BUG_ON(offset_in_page(size));
> BUG_ON(!is_power_of_2(align));
See a recent Linus rant about BUG_ONs. These BUG_ONs are quite old and
from a quick look they are even unnecessary. So rather than adding more
of those, I think removing those that are not needed is much more
preferred.
> should we make below declarations as conventions
> 1) when we say 'alignment', it means align to a power of 2 value
> for example, aligning value @v to @b implicit @v is power of 2
> , align 10 to 4 is 12
alignment other than power-of-two makes only very limited sense to me.
> 2) when we say 'round value @v up/down to boundary @b', it means the
> result is a times of @b, it don't requires @b is a power of 2
--
Michal Hocko
SUSE Labs
On Wed 12-10-16 16:44:31, zijun_hu wrote:
> On 10/12/2016 04:25 PM, Michal Hocko wrote:
> > On Wed 12-10-16 15:24:33, zijun_hu wrote:
[...]
> >> i found the following code segments in mm/vmalloc.c
> >> static struct vmap_area *alloc_vmap_area(unsigned long size,
> >> unsigned long align,
> >> unsigned long vstart, unsigned long vend,
> >> int node, gfp_t gfp_mask)
> >> {
> >> ...
> >>
> >> BUG_ON(!size);
> >> BUG_ON(offset_in_page(size));
> >> BUG_ON(!is_power_of_2(align));
> >
> > See a recent Linus rant about BUG_ONs. These BUG_ONs are quite old and
> > from a quick look they are even unnecessary. So rather than adding more
> > of those, I think removing those that are not needed is much more
> > preferred.
> >
> i notice that, and the above code segments is used to illustrate that
> input parameter checking is necessary sometimes
Why do you think it is necessary here?
--
Michal Hocko
SUSE Labs
On 10/12/2016 05:54 PM, Michal Hocko wrote:
> On Wed 12-10-16 16:44:31, zijun_hu wrote:
>> On 10/12/2016 04:25 PM, Michal Hocko wrote:
>>> On Wed 12-10-16 15:24:33, zijun_hu wrote:
> [...]
>>>> i found the following code segments in mm/vmalloc.c
>>>> static struct vmap_area *alloc_vmap_area(unsigned long size,
>>>> unsigned long align,
>>>> unsigned long vstart, unsigned long vend,
>>>> int node, gfp_t gfp_mask)
>>>> {
>>>> ...
>>>>
>>>> BUG_ON(!size);
>>>> BUG_ON(offset_in_page(size));
>>>> BUG_ON(!is_power_of_2(align));
>>>
>>> See a recent Linus rant about BUG_ONs. These BUG_ONs are quite old and
>>> from a quick look they are even unnecessary. So rather than adding more
>>> of those, I think removing those that are not needed is much more
>>> preferred.
>>>
>> i notice that, and the above code segments is used to illustrate that
>> input parameter checking is necessary sometimes
>
> Why do you think it is necessary here?
>
i am sorry for reply late
i don't know whether it is necessary
i just find there are so many sanity checkup in current internal interfaces
On Tue, Oct 11, 2016 at 09:24:50PM +0800, zijun_hu wrote:
> From: zijun_hu <[email protected]>
>
> the LSB of a chunk->map element is used for free/in-use flag of a area
> and the other bits for offset, the sufficient and necessary condition of
> this usage is that both size and alignment of a area must be even numbers
> however, pcpu_alloc() doesn't force its @align parameter a even number
> explicitly, so a odd @align maybe causes a series of errors, see below
> example for concrete descriptions.
>
> lets assume area [16, 36) is free but its previous one is in-use, we want
> to allocate a @size == 8 and @align == 7 area. the larger area [16, 36) is
> split to three areas [16, 21), [21, 29), [29, 36) eventually. however, due
> to the usage for a chunk->map element, the actual offset of the aim area
> [21, 29) is 21 but is recorded in relevant element as 20; moreover the
> residual tail free area [29, 36) is mistook as in-use and is lost silently
>
> as explained above, inaccurate either offset or free/in-use state of
> a area is recorded into relevant chunk->map element if request a odd
> alignment area, and so causes memory leakage issue
>
> fix it by forcing the @align of a area to allocate a even number
> as do for @size.
>
> BTW, macro ALIGN() within pcpu_fit_in_area() is replaced by roundup() too
> due to back reason. in order to align a value @v up to @a boundary, macro
> roundup(v, a) is more generic than ALIGN(x, a); the latter doesn't work
> well when @a isn't a power of 2 value. for example, roundup(10, 6) == 12
> but ALIGN(10, 6) == 10, the former result is desired obviously
>
> Signed-off-by: zijun_hu <[email protected]>
Nacked-by: Tejun Heo <[email protected]>
This is a fix for an imaginary problem. The most we should do about
odd alignment is triggering a WARN_ON.
Thanks.
--
tejun
On 2016/10/14 7:31, Tejun Heo wrote:
> On Tue, Oct 11, 2016 at 09:24:50PM +0800, zijun_hu wrote:
>> From: zijun_hu <[email protected]>
>>
>> the LSB of a chunk->map element is used for free/in-use flag of a area
>> and the other bits for offset, the sufficient and necessary condition of
>> this usage is that both size and alignment of a area must be even numbers
>> however, pcpu_alloc() doesn't force its @align parameter a even number
>> explicitly, so a odd @align maybe causes a series of errors, see below
>> example for concrete descriptions.
>>
>> lets assume area [16, 36) is free but its previous one is in-use, we want
>> to allocate a @size == 8 and @align == 7 area. the larger area [16, 36) is
>> split to three areas [16, 21), [21, 29), [29, 36) eventually. however, due
>> to the usage for a chunk->map element, the actual offset of the aim area
>> [21, 29) is 21 but is recorded in relevant element as 20; moreover the
>> residual tail free area [29, 36) is mistook as in-use and is lost silently
>>
>> as explained above, inaccurate either offset or free/in-use state of
>> a area is recorded into relevant chunk->map element if request a odd
>> alignment area, and so causes memory leakage issue
>>
>> fix it by forcing the @align of a area to allocate a even number
>> as do for @size.
>>
>> BTW, macro ALIGN() within pcpu_fit_in_area() is replaced by roundup() too
>> due to back reason. in order to align a value @v up to @a boundary, macro
>> roundup(v, a) is more generic than ALIGN(x, a); the latter doesn't work
>> well when @a isn't a power of 2 value. for example, roundup(10, 6) == 12
>> but ALIGN(10, 6) == 10, the former result is desired obviously
>>
>> Signed-off-by: zijun_hu <[email protected]>
>
> Nacked-by: Tejun Heo <[email protected]>
>
> This is a fix for an imaginary problem. The most we should do about
> odd alignment is triggering a WARN_ON.
>
for the current code, only power of 2 alignment value can works well
is it acceptable to performing a power of 2 checking and returning error code
if fail?
Hello,
On Fri, Oct 14, 2016 at 08:23:06AM +0800, zijun_hu wrote:
> for the current code, only power of 2 alignment value can works well
>
> is it acceptable to performing a power of 2 checking and returning error code
> if fail?
Yeah, just add is_power_of_2() test to the existing sanity check.
Thanks.
--
tejun
On 2016/10/14 8:28, Tejun Heo wrote:
> Hello,
>
> On Fri, Oct 14, 2016 at 08:23:06AM +0800, zijun_hu wrote:
>> for the current code, only power of 2 alignment value can works well
>>
>> is it acceptable to performing a power of 2 checking and returning error code
>> if fail?
>
> Yeah, just add is_power_of_2() test to the existing sanity check.
>
> Thanks.
>
okay. i will do that