Re: [kernel-hardening] [RFC] memory allocations in genalloc
Hi, On 18/08/17 16:57, Laura Abbott wrote: > Again, if you have a specific patch or > proposal this would be easier to review. yes, I'm preparing it and will send it out soon, but it was somehow surprising to me that it was chosen to implement free with the size parameter. It made me think that I was overlooking some obvious reason behind the choice :-S -- thanks for answering, igor
Re: [kernel-hardening] [RFC] memory allocations in genalloc
Hi, On 18/08/17 16:57, Laura Abbott wrote: > Again, if you have a specific patch or > proposal this would be easier to review. yes, I'm preparing it and will send it out soon, but it was somehow surprising to me that it was chosen to implement free with the size parameter. It made me think that I was overlooking some obvious reason behind the choice :-S -- thanks for answering, igor
Re: [kernel-hardening] [RFC] memory allocations in genalloc
On 08/17/2017 09:26 AM, Igor Stoppa wrote:
> Foreword:
> If I should direct this message to someone else, please let me know.
> I couldn't get a clear idea, by looking at both MAINTAINERS and git blame.
>
>
>
> Hi,
>
> I'm currently trying to convert the SE Linux policy db into using a
> protectable memory allocator (pmalloc) that I have developed.
>
> Such allocator is based on genalloc: I had come up with an
> implementation that was pretty similar to what genalloc already does, so
> it was pointed out to me that I could have a look at it.
>
> And, indeed, it seemed a perfect choice.
>
> But ... when freeing memory, genalloc wants that the caller also states
> how large each specific memory allocation is.
>
> This, per se, is not an issue, although genalloc doesn't seen to check
> if the memory being freed is really matching a previous allocation request.
>
> However, this design doesn't sit well with the use case I have in mind.
>
> In particular, when the SE Linux policy db is populated, the creation of
> one or more specific entry of the db might fail.
> In this case, the memory previously allocated for said entry, is
> released with kfree, which doesn't need to know the size of the chunk
> being freed.
>
> I would like to add similar capability to genalloc.
>
> genalloc already uses bitmaps, to track what words are allocated (1) and
> which are free (0)
>
> What I would like to do is to add another bitmap, which would track the
> beginning of each individual allocation (1 on the first allocation unit
> of each allocation, 0 otherwise).
>
> Such enhancement would enable also the detection of calls to free with
> incorrect / misaligned addresses - right now it is possible to
> successfully free a memory area that overlaps the interface of two
> adjacent allocations, without fully covering either of them.
>
> Would this change be acceptable?
> Is there any better way to achieve what I want?
>
In general, I don't see anything wrong with wanting to let gen_pool_free
not take a size. It's hard to say anything more without a patch to review.
My biggest concern would be keeping existing behavior and managing two
bitmaps locklessly.
>
> ---
>
> I have also a question wrt the use of spinlocks in genalloc.
> Why a spinlock?
>
> Freeing a chunk of memory previously allocated with vmalloc requires
> invoking vfree_atomic, instead of vfree, because the list of chunks is
> walked with the spinlock held, and vfree can sleep.
>
> Why not using a mutex?
>
>From the git history, gen_pool used to use a reader/writer lock and
was switched to be lockless so it could be used in NMI contexts
7f184275aa30 ("lib, Make gen_pool memory allocator lockless").
This looks to be an intentional choice, presumably so regions can be
added in atomic contexts. Again, if you have a specific patch or
proposal this would be easier to review.
Thanks,
Laura
>
> --
> TIA, igor
>
Re: [kernel-hardening] [RFC] memory allocations in genalloc
On 08/17/2017 09:26 AM, Igor Stoppa wrote:
> Foreword:
> If I should direct this message to someone else, please let me know.
> I couldn't get a clear idea, by looking at both MAINTAINERS and git blame.
>
>
>
> Hi,
>
> I'm currently trying to convert the SE Linux policy db into using a
> protectable memory allocator (pmalloc) that I have developed.
>
> Such allocator is based on genalloc: I had come up with an
> implementation that was pretty similar to what genalloc already does, so
> it was pointed out to me that I could have a look at it.
>
> And, indeed, it seemed a perfect choice.
>
> But ... when freeing memory, genalloc wants that the caller also states
> how large each specific memory allocation is.
>
> This, per se, is not an issue, although genalloc doesn't seen to check
> if the memory being freed is really matching a previous allocation request.
>
> However, this design doesn't sit well with the use case I have in mind.
>
> In particular, when the SE Linux policy db is populated, the creation of
> one or more specific entry of the db might fail.
> In this case, the memory previously allocated for said entry, is
> released with kfree, which doesn't need to know the size of the chunk
> being freed.
>
> I would like to add similar capability to genalloc.
>
> genalloc already uses bitmaps, to track what words are allocated (1) and
> which are free (0)
>
> What I would like to do is to add another bitmap, which would track the
> beginning of each individual allocation (1 on the first allocation unit
> of each allocation, 0 otherwise).
>
> Such enhancement would enable also the detection of calls to free with
> incorrect / misaligned addresses - right now it is possible to
> successfully free a memory area that overlaps the interface of two
> adjacent allocations, without fully covering either of them.
>
> Would this change be acceptable?
> Is there any better way to achieve what I want?
>
In general, I don't see anything wrong with wanting to let gen_pool_free
not take a size. It's hard to say anything more without a patch to review.
My biggest concern would be keeping existing behavior and managing two
bitmaps locklessly.
>
> ---
>
> I have also a question wrt the use of spinlocks in genalloc.
> Why a spinlock?
>
> Freeing a chunk of memory previously allocated with vmalloc requires
> invoking vfree_atomic, instead of vfree, because the list of chunks is
> walked with the spinlock held, and vfree can sleep.
>
> Why not using a mutex?
>
>From the git history, gen_pool used to use a reader/writer lock and
was switched to be lockless so it could be used in NMI contexts
7f184275aa30 ("lib, Make gen_pool memory allocator lockless").
This looks to be an intentional choice, presumably so regions can be
added in atomic contexts. Again, if you have a specific patch or
proposal this would be easier to review.
Thanks,
Laura
>
> --
> TIA, igor
>
