| Message ID | 1327912654-8738-1-git-send-email-dmitry.antipov@linaro.org |
|---|---|
| State | New |
| Headers | show |
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>
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
On Mon, 30 Jan 2012, Tejun Heo wrote:
> Anyways, yeah, it seems we should improve this part too.
I agree.
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.
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.
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);
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(-)