[PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Dmitry Antipov]

Fix pcpu_alloc() to return ZERO_SIZE_PTR if requested size is 0;
fix free_percpu() to check passed pointer with ZERO_OR_NULL_PTR.

Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org>
---
 mm/percpu.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index f47af91..e903a19 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -702,7 +702,8 @@ static struct pcpu_chunk *pcpu_chunk_addr_search(void *addr)
  * Does GFP_KERNEL allocation.
  *
  * RETURNS:
- * Percpu pointer to the allocated area on success, NULL on failure.
+ * ZERO_SIZE_PTR if @size is zero, percpu pointer to the
+ * allocated area on success or NULL on failure.
  */
 static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved)
 {
@@ -713,7 +714,10 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved)
 	unsigned long flags;
 	void __percpu *ptr;
 
-	if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE)) {
+	if (unlikely(!size))
+		return ZERO_SIZE_PTR;
+
+	if (unlikely(size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE)) {
 		WARN(true, "illegal size (%zu) or align (%zu) for "
 		     "percpu allocation\n", size, align);
 		return NULL;
@@ -834,7 +838,8 @@ fail_unlock_mutex:
  * Does GFP_KERNEL allocation.
  *
  * RETURNS:
- * Percpu pointer to the allocated area on success, NULL on failure.
+ * ZERO_SIZE_PTR if @size is zero, percpu pointer to the
+ * allocated area on success, NULL on failure.
  */
 void __percpu *__alloc_percpu(size_t size, size_t align)
 {
@@ -856,7 +861,8 @@ EXPORT_SYMBOL_GPL(__alloc_percpu);
  * Does GFP_KERNEL allocation.
  *
  * RETURNS:
- * Percpu pointer to the allocated area on success, NULL on failure.
+ * ZERO_SIZE_PTR if @size is zero, percpu pointer to the
+ * allocated area on success or NULL on failure.
  */
 void __percpu *__alloc_reserved_percpu(size_t size, size_t align)
 {
@@ -917,7 +923,7 @@ void free_percpu(void __percpu *ptr)
 	unsigned long flags;
 	int off;
 
-	if (!ptr)
+	if (unlikely(ZERO_OR_NULL_PTR(ptr)))
 		return;
 
 	kmemleak_free_percpu(ptr);
-- 
1.7.7.6

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Dmitry Antipov wrote:

> Fix pcpu_alloc() to return ZERO_SIZE_PTR if requested size is 0;
> fix free_percpu() to check passed pointer with ZERO_OR_NULL_PTR.

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 12:37:34PM +0400, Dmitry Antipov wrote:
> Fix pcpu_alloc() to return ZERO_SIZE_PTR if requested size is 0;
> fix free_percpu() to check passed pointer with ZERO_OR_NULL_PTR.
> 
> Signed-off-by: Dmitry Antipov <dmitry.antipov@linaro.org>
> ---
>  mm/percpu.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index f47af91..e903a19 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -702,7 +702,8 @@ static struct pcpu_chunk *pcpu_chunk_addr_search(void *addr)
>   * Does GFP_KERNEL allocation.
>   *
>   * RETURNS:
> - * Percpu pointer to the allocated area on success, NULL on failure.
> + * ZERO_SIZE_PTR if @size is zero, percpu pointer to the
> + * allocated area on success or NULL on failure.
>   */
>  static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved)
>  {
> @@ -713,7 +714,10 @@ static void __percpu *pcpu_alloc(size_t size, size_t align, bool reserved)
>  	unsigned long flags;
>  	void __percpu *ptr;
>  
> -	if (unlikely(!size || size > PCPU_MIN_UNIT_SIZE || align > PAGE_SIZE)) {
> +	if (unlikely(!size))
> +		return ZERO_SIZE_PTR;

Percpu pointers are in a different address space and using
ZERO_SIZE_PTR directly will trigger sparse address space warning.
Also, I'm not entirely sure whether 16 is guaranteed to be unused in
percpu address space (maybe it is but I don't think we have anything
enforcing that).

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 09:15:58AM -0800, Tejun Heo wrote:
> Percpu pointers are in a different address space and using
> ZERO_SIZE_PTR directly will trigger sparse address space warning.
> Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> percpu address space (maybe it is but I don't think we have anything
> enforcing that).

Another thing is that percpu address dereferencing always goes through
rather unintuitive translation and 1. we can't (or rather currently
don't) guarantee that fault will occur for any address 2. even if it
does, the faulting address wouldn't be anything easily
distinguishible.  So, unless the above shortcomings is resolved, I
don't really see much point of using ZERO_SIZE_PTR for percpu
allocator.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> Percpu pointers are in a different address space and using
> ZERO_SIZE_PTR directly will trigger sparse address space warning.
> Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> percpu address space (maybe it is but I don't think we have anything
> enforcing that).

We are already checking for NULL on free. So there is a presumption that
these numbers are unused.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> On Mon, Jan 30, 2012 at 09:15:58AM -0800, Tejun Heo wrote:
> > Percpu pointers are in a different address space and using
> > ZERO_SIZE_PTR directly will trigger sparse address space warning.
> > Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> > percpu address space (maybe it is but I don't think we have anything
> > enforcing that).
>
> Another thing is that percpu address dereferencing always goes through
> rather unintuitive translation and 1. we can't (or rather currently
> don't) guarantee that fault will occur for any address 2. even if it
> does, the faulting address wouldn't be anything easily
> distinguishible.  So, unless the above shortcomings is resolved, I
> don't really see much point of using ZERO_SIZE_PTR for percpu
> allocator.

The same is true for the use of NULL pointers.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 11:22:57AM -0600, Christoph Lameter wrote:
> On Mon, 30 Jan 2012, Tejun Heo wrote:
> 
> > On Mon, Jan 30, 2012 at 09:15:58AM -0800, Tejun Heo wrote:
> > > Percpu pointers are in a different address space and using
> > > ZERO_SIZE_PTR directly will trigger sparse address space warning.
> > > Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> > > percpu address space (maybe it is but I don't think we have anything
> > > enforcing that).
> >
> > Another thing is that percpu address dereferencing always goes through
> > rather unintuitive translation and 1. we can't (or rather currently
> > don't) guarantee that fault will occur for any address 2. even if it
> > does, the faulting address wouldn't be anything easily
> > distinguishible.  So, unless the above shortcomings is resolved, I
> > don't really see much point of using ZERO_SIZE_PTR for percpu
> > allocator.
> 
> The same is true for the use of NULL pointers.

I'm pretty sure it never gives out NULL for a dynamic allocation.  The
base might be mapped to zero but we're guaranteed to have some static
percpu areas there and IIRC the percpu addresses aren't supposed to
wrap.

Also, if ZERO_SIZE_PTR doesn't actually help anything, it is not a
good idea to have it.  The only thing it does would be giving wrong
impressions.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> I'm pretty sure it never gives out NULL for a dynamic allocation.  The
> base might be mapped to zero but we're guaranteed to have some static
> percpu areas there and IIRC the percpu addresses aren't supposed to
> wrap.

True but there is a check for a NULL pointer on free. So a NULL pointer
currently has the semantics of being an unallocated per cpu structure.
If the allocator returns NULL by accident then we cannot free the per cpu
allocation anymore.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 11:22:14AM -0600, Christoph Lameter wrote:
> On Mon, 30 Jan 2012, Tejun Heo wrote:
> 
> > Percpu pointers are in a different address space and using
> > ZERO_SIZE_PTR directly will trigger sparse address space warning.
> > Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> > percpu address space (maybe it is but I don't think we have anything
> > enforcing that).
> 
> We are already checking for NULL on free. So there is a presumption that
> these numbers are unused.

Yes, we probably don't use 16 as valid dynamic address because static
area would be larger than that.  It's just fuzzier than NULL.  And, as
I wrote in another reply, ZERO_SIZE_PTR simply doesn't contribute
anything.  Maybe we can update the allocator to always not use the
lowest 4k for either static or dynamic and add debug code to
translation macros to check for percpu addresses < 4k, but without
such changes ZERO_SIZE_PTR simply doesn't do anything.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> On Mon, Jan 30, 2012 at 11:22:14AM -0600, Christoph Lameter wrote:
> > On Mon, 30 Jan 2012, Tejun Heo wrote:
> >
> > > Percpu pointers are in a different address space and using
> > > ZERO_SIZE_PTR directly will trigger sparse address space warning.
> > > Also, I'm not entirely sure whether 16 is guaranteed to be unused in
> > > percpu address space (maybe it is but I don't think we have anything
> > > enforcing that).
> >
> > We are already checking for NULL on free. So there is a presumption that
> > these numbers are unused.
>
> Yes, we probably don't use 16 as valid dynamic address because static
> area would be larger than that.  It's just fuzzier than NULL.  And, as
> I wrote in another reply, ZERO_SIZE_PTR simply doesn't contribute
> anything.  Maybe we can update the allocator to always not use the
> lowest 4k for either static or dynamic and add debug code to
> translation macros to check for percpu addresses < 4k, but without
> such changes ZERO_SIZE_PTR simply doesn't do anything.

We have two possibilities now:

1. We say that the value returned from the per cpu allocator is an opaque
value.

	This means that we have to remove the NULL check from the free
	function. And audit the kernel code for all occurrences where
	a per cpu pointer value of NULL is assumed to mean that no per
	cpu allocation has occurred.

2. We say that there are special values for the per cpu pointers (NULL,
	ZERO_SIZE_PTR)

	Then we would have to guarantee that the per cpu allocator never
	returns those values.

	Plus then the ZERO_SIZE_PTR patch will be fine.

	The danger exist of these values being passed as
	parameters to functions that do not support them (per_cpu_ptr
	etc). Those would need VM_BUG_ONs or some other checks to detect
	potential problems.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

Hello, Christoph.

On Mon, Jan 30, 2012 at 11:52:23AM -0600, Christoph Lameter wrote:
> We have two possibilities now:
> 
> 1. We say that the value returned from the per cpu allocator is an opaque
> value.
> 
> 	This means that we have to remove the NULL check from the free
> 	function. And audit the kernel code for all occurrences where
> 	a per cpu pointer value of NULL is assumed to mean that no per
> 	cpu allocation has occurred.

No, NULL is never gonna be a valid return from any allocator including
percpu.  Percpu allocator doesn't and will never do so.

> 2. We say that there are special values for the per cpu pointers (NULL,
> 	ZERO_SIZE_PTR)
> 
> 	Then we would have to guarantee that the per cpu allocator never
> 	returns those values.
> 
> 	Plus then the ZERO_SIZE_PTR patch will be fine.
> 
> 	The danger exist of these values being passed as
> 	parameters to functions that do not support them (per_cpu_ptr
> 	etc). Those would need VM_BUG_ONs or some other checks to detect
> 	potential problems.

I'm saying we don't have this for ZERO_SIZE_PTR in any meaningful way
at this point.  If somebody wants to implement it properly, please
feel free to, but simply applying ZERO_SIZE_PTR without other changes
doesn't make any sense.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> Hello, Christoph.
>
> On Mon, Jan 30, 2012 at 11:52:23AM -0600, Christoph Lameter wrote:
> > We have two possibilities now:
> >
> > 1. We say that the value returned from the per cpu allocator is an opaque
> > value.
> >
> > 	This means that we have to remove the NULL check from the free
> > 	function. And audit the kernel code for all occurrences where
> > 	a per cpu pointer value of NULL is assumed to mean that no per
> > 	cpu allocation has occurred.
>
> No, NULL is never gonna be a valid return from any allocator including
> percpu.  Percpu allocator doesn't and will never do so.

How do you prevent the percpu allocator from returning NULL? I thought the
per cpu offsets can wrap around?

> > 2. We say that there are special values for the per cpu pointers (NULL,
> > 	ZERO_SIZE_PTR)
> >
> > 	Then we would have to guarantee that the per cpu allocator never
> > 	returns those values.
> >
> > 	Plus then the ZERO_SIZE_PTR patch will be fine.
> >
> > 	The danger exist of these values being passed as
> > 	parameters to functions that do not support them (per_cpu_ptr
> > 	etc). Those would need VM_BUG_ONs or some other checks to detect
> > 	potential problems.
>
> I'm saying we don't have this for ZERO_SIZE_PTR in any meaningful way
> at this point.  If somebody wants to implement it properly, please
> feel free to, but simply applying ZERO_SIZE_PTR without other changes
> doesn't make any sense.

We have no clean notion of how a percpu pointer needs to be handled. Both
ways of handling things have drawbacks.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

Hello,

On Mon, Jan 30, 2012 at 11:58:52AM -0600, Christoph Lameter wrote:
> > No, NULL is never gonna be a valid return from any allocator including
> > percpu.  Percpu allocator doesn't and will never do so.
> 
> How do you prevent the percpu allocator from returning NULL? I thought the
> per cpu offsets can wrap around?

I thought it didn't.  I rememer thinking about this and determining
that NULL can't be allocated for dynamic addresses.  Maybe I'm
imagining things.  Anyways, if it can return NULL for valid
allocation, it is a bug and should be fixed.

> > I'm saying we don't have this for ZERO_SIZE_PTR in any meaningful way
> > at this point.  If somebody wants to implement it properly, please
> > feel free to, but simply applying ZERO_SIZE_PTR without other changes
> > doesn't make any sense.
> 
> We have no clean notion of how a percpu pointer needs to be handled. Both
> ways of handling things have drawbacks.

We don't have returned addr >= PAGE_SIZE guarantee yet but I'm fairly
sure that's the only acceptable direction if we want any improvement
in this area.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> Hello,
>
> On Mon, Jan 30, 2012 at 11:58:52AM -0600, Christoph Lameter wrote:
> > > No, NULL is never gonna be a valid return from any allocator including
> > > percpu.  Percpu allocator doesn't and will never do so.
> >
> > How do you prevent the percpu allocator from returning NULL? I thought the
> > per cpu offsets can wrap around?
>
> I thought it didn't.  I rememer thinking about this and determining
> that NULL can't be allocated for dynamic addresses.  Maybe I'm
> imagining things.  Anyways, if it can return NULL for valid
> allocation, it is a bug and should be fixed.

I dont see anything that would hinder an arbitrary value to be returned.
NULL is also used for the failure case. Definitely a bug.

> > > I'm saying we don't have this for ZERO_SIZE_PTR in any meaningful way
> > > at this point.  If somebody wants to implement it properly, please
> > > feel free to, but simply applying ZERO_SIZE_PTR without other changes
> > > doesn't make any sense.
> >
> > We have no clean notion of how a percpu pointer needs to be handled. Both
> > ways of handling things have drawbacks.
>
> We don't have returned addr >= PAGE_SIZE guarantee yet but I'm fairly
> sure that's the only acceptable direction if we want any improvement
> in this area.

The ZERO_SIZE_PTR patch would not make the situation that much worse.

If the per cpu allocator happens to return NULL for a valid allocation
then this allocation cannot be freed anymore since the free function
checks for NULL. Most callers check the result for NULL though and will
fail in other ways at a higher level. Such an allocation can only happen
once and from hen on some memory is wasted.

If the per cpu allocator just happens to return ZERO_SIZE_PTR for a valid
allocation then this value is going to be passed to other per cpu
functions. However, the size is 0 so no actual read or write should ever
take place.

On free its not going to be freed since the free function checks for
ZERO_SIZE_PTR. So we have a situation almost the same as for NULL pointer.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 10:02:24AM -0800, Tejun Heo wrote:
> I thought it didn't.  I rememer thinking about this and determining
> that NULL can't be allocated for dynamic addresses.  Maybe I'm
> imagining things.  Anyways, if it can return NULL for valid
> allocation, it is a bug and should be fixed.

So, the default translation is

#define __addr_to_pcpu_ptr(addr)					\
	(void __percpu *)((unsigned long)(addr) -			\
	(unsigned long)pcpu_base_addr +					\
	(unsigned long)__per_cpu_start)

It basically offsets the virtual address of the first unit against the
start of static percpu section, so if the linked percpu data address
is higher than the base address of the initial chunk, I *think*
overwrap is possible.  I don't think this can happen on x86 regardless
of first chunk allocation mode tho but there may be configurations
where __per_cpu_start is higher than pcpu_base_addr (IIRC some archs
locate vmalloc area lower than kernel image, dunno whether the used
address range actually is enough for causing overflow tho).

Anyways, yeah, it seems we should improve this part too.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Christoph Lameter]

On Mon, 30 Jan 2012, Tejun Heo wrote:

> Anyways, yeah, it seems we should improve this part too.

I agree.

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Tejun Heo]

On Mon, Jan 30, 2012 at 12:12:18PM -0600, Christoph Lameter wrote:
> > I thought it didn't.  I rememer thinking about this and determining
> > that NULL can't be allocated for dynamic addresses.  Maybe I'm
> > imagining things.  Anyways, if it can return NULL for valid
> > allocation, it is a bug and should be fixed.
> 
> I dont see anything that would hinder an arbitrary value to be returned.
> NULL is also used for the failure case. Definitely a bug.

Given the address translation we do and kernel image layout, I don't
think this can happen on x86.  It may theoretically possible on other
archs tho.  Anyways, yeah, this one needs improving.

> > We don't have returned addr >= PAGE_SIZE guarantee yet but I'm fairly
> > sure that's the only acceptable direction if we want any improvement
> > in this area.
> 
> The ZERO_SIZE_PTR patch would not make the situation that much worse.

I'm not objecting to marking zero-sized allocations per-se.  I'm
saying the patch is pointless at this point.  It doesn't contribute
anything while giving the illusion of better error checking than we
actually do.  Let's do it when it can actually work.

Thanks.

-- 
tejun

Re: [PATCH 1/3] percpu: use ZERO_SIZE_PTR / ZERO_OR_NULL_PTR

[Rusty Russell]

On Mon, 30 Jan 2012 10:16:39 -0800, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 30, 2012 at 12:12:18PM -0600, Christoph Lameter wrote:
> > > I thought it didn't.  I rememer thinking about this and determining
> > > that NULL can't be allocated for dynamic addresses.  Maybe I'm
> > > imagining things.  Anyways, if it can return NULL for valid
> > > allocation, it is a bug and should be fixed.
> > 
> > I dont see anything that would hinder an arbitrary value to be returned.
> > NULL is also used for the failure case. Definitely a bug.
> 
> Given the address translation we do and kernel image layout, I don't
> think this can happen on x86.  It may theoretically possible on other
> archs tho.  Anyways, yeah, this one needs improving.

I tried setting the lower bit on all percpu ptrs, but since non-dynamic
percpu vars could have odd alignments, that fails in general.

> > > We don't have returned addr >= PAGE_SIZE guarantee yet but I'm fairly
> > > sure that's the only acceptable direction if we want any improvement
> > > in this area.
> > 
> > The ZERO_SIZE_PTR patch would not make the situation that much worse.
> 
> I'm not objecting to marking zero-sized allocations per-se.  I'm
> saying the patch is pointless at this point.  It doesn't contribute
> anything while giving the illusion of better error checking than we
> actually do.  Let's do it when it can actually work.

Disagree: This patch works.  It allows zero-size per-cpu allocs, like
the other allocators.  Nor does it fail in practice.

We should do better, but the perfect is the enemy of the good.

Cheers,
Rusty.