2020-09-29 13:15:12

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v3 01/10] mm: add Kernel Electric-Fence infrastructure

On Tue, Sep 29, 2020 at 02:42PM +0200, Andrey Konovalov wrote:
[...]
> > + */
> > + index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1;
>
> Why do we subtract 1 here? We do have the metadata entry reserved for something?

Above the declaration of __kfence_pool it says:

* We allocate an even number of pages, as it simplifies calculations to map
* address to metadata indices; effectively, the very first page serves as an
* extended guard page, but otherwise has no special purpose.

Hopefully that clarifies the `- 1` here.

[...]
> > + /* Allocation and free stack information. */
> > + int num_alloc_stack;
> > + int num_free_stack;
> > + unsigned long alloc_stack[KFENCE_STACK_DEPTH];
> > + unsigned long free_stack[KFENCE_STACK_DEPTH];
>
> It was a concious decision to not use stackdepot, right? Perhaps it
> makes sense to document the reason somewhere.

Yes; we want to avoid the dynamic allocations that stackdepot does.

[...]

Thanks,
-- Marco


2020-09-29 13:50:18

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH v3 01/10] mm: add Kernel Electric-Fence infrastructure

On Tue, Sep 29, 2020 at 3:11 PM Marco Elver <[email protected]> wrote:
>
> On Tue, Sep 29, 2020 at 02:42PM +0200, Andrey Konovalov wrote:
> [...]
> > > + */
> > > + index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1;
> >
> > Why do we subtract 1 here? We do have the metadata entry reserved for something?
>
> Above the declaration of __kfence_pool it says:
>
> * We allocate an even number of pages, as it simplifies calculations to map
> * address to metadata indices; effectively, the very first page serves as an
> * extended guard page, but otherwise has no special purpose.
>
> Hopefully that clarifies the `- 1` here.

So there are two guard pages at the beginning and only then a page
that holds an object?

2020-09-29 13:51:05

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v3 01/10] mm: add Kernel Electric-Fence infrastructure

On Tue, 29 Sep 2020 at 15:48, Andrey Konovalov <[email protected]> wrote:
> On Tue, Sep 29, 2020 at 3:11 PM Marco Elver <[email protected]> wrote:
> >
> > On Tue, Sep 29, 2020 at 02:42PM +0200, Andrey Konovalov wrote:
> > [...]
> > > > + */
> > > > + index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1;
> > >
> > > Why do we subtract 1 here? We do have the metadata entry reserved for something?
> >
> > Above the declaration of __kfence_pool it says:
> >
> > * We allocate an even number of pages, as it simplifies calculations to map
> > * address to metadata indices; effectively, the very first page serves as an
> > * extended guard page, but otherwise has no special purpose.
> >
> > Hopefully that clarifies the `- 1` here.
>
> So there are two guard pages at the beginning and only then a page
> that holds an object?

Yes, correct.

2020-09-29 14:05:56

by Andrey Konovalov

[permalink] [raw]
Subject: Re: [PATCH v3 01/10] mm: add Kernel Electric-Fence infrastructure

On Tue, Sep 29, 2020 at 3:49 PM Marco Elver <[email protected]> wrote:
>
> On Tue, 29 Sep 2020 at 15:48, Andrey Konovalov <[email protected]> wrote:
> > On Tue, Sep 29, 2020 at 3:11 PM Marco Elver <[email protected]> wrote:
> > >
> > > On Tue, Sep 29, 2020 at 02:42PM +0200, Andrey Konovalov wrote:
> > > [...]
> > > > > + */
> > > > > + index = (addr - (unsigned long)__kfence_pool) / (PAGE_SIZE * 2) - 1;
> > > >
> > > > Why do we subtract 1 here? We do have the metadata entry reserved for something?
> > >
> > > Above the declaration of __kfence_pool it says:
> > >
> > > * We allocate an even number of pages, as it simplifies calculations to map
> > > * address to metadata indices; effectively, the very first page serves as an
> > > * extended guard page, but otherwise has no special purpose.
> > >
> > > Hopefully that clarifies the `- 1` here.
> >
> > So there are two guard pages at the beginning and only then a page
> > that holds an object?
>
> Yes, correct.

OK, I see. This isn't directly clear from the comment though, at least for me :)