Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1498882 > unrolled thread

[RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a  odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-11 15:30 +0200
    Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area Michal Hocko <mhocko@kernel.org> - 2016-10-11 19:30 +0200
      Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-12 02:30 +0200
        Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area Michal Hocko <mhocko@kernel.org> - 2016-10-12 09:00 +0200
          Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-12 09:30 +0200
            Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-12 10:50 +0200
              Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area Michal Hocko <mhocko@kernel.org> - 2016-10-12 12:00 +0200
                Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-12 12:00 +0200
            Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area Michal Hocko <mhocko@kernel.org> - 2016-10-12 11:30 +0200
          Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when  allocate a odd alignment area zijun_hu <zijun_hu@zoho.com> - 2016-10-12 09:30 +0200

#1498882 — [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

From: zijun_hu <zijun_hu@htc.com>

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 <zijun_hu@htc.com>
---
 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

[toc] | [next] | [standalone]

#1499034 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

On Tue 11-10-16 21:24:50, zijun_hu wrote:
> From: zijun_hu <zijun_hu@htc.com>
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1499304 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

On 2016/10/12 1:22, Michal Hocko wrote:
> On Tue 11-10-16 21:24:50, zijun_hu wrote:
>> From: zijun_hu <zijun_hu@htc.com>
>>
>> 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?

[toc] | [prev] | [next] | [standalone]

#1499391 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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 <zijun_hu@htc.com>
> >>
> >> 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

[toc] | [prev] | [next] | [standalone]

#1499399 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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 <zijun_hu@htc.com>
>>>>
>>>> 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

[toc] | [prev] | [next] | [standalone]

#1499432 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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 <zijun_hu@htc.com>
>>>>>>
>> 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

[toc] | [prev] | [next] | [standalone]

#1499451 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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

[toc] | [prev] | [next] | [standalone]

#1499452 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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

[toc] | [prev] | [next] | [standalone]

#1499446 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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 <zijun_hu@htc.com>
> >>>>
> >>>> 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

[toc] | [prev] | [next] | [standalone]

#1499401 — Re: [RFC PATCH 1/1] mm/percpu.c: fix memory leakage issue when allocate a odd alignment area

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 <zijun_hu@htc.com>
>>>>
>>>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web