Re: [PATCH] slub: make sure struct kmem_cache_node is initialized before publication

[Alexander Potapenko]

Hi everyone,

On Mon, Jul 10, 2017 at 10:32 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 7 Jul 2017 18:18:31 -0500 (CDT) Christoph Lameter <cl@xxxxxxxxx> wrote:
>
>> On Fri, 7 Jul 2017, Andrew Morton wrote:
>>
>> > On Fri,  7 Jul 2017 10:34:08 +0200 Alexander Potapenko <glider@xxxxxxxxxx> wrote:
>> >
>> > > --- a/mm/slub.c
>> > > +++ b/mm/slub.c
>> > > @@ -3389,8 +3389,8 @@ static int init_kmem_cache_nodes(struct kmem_cache *s)
>> > >                   return 0;
>> > >           }
>> > >
>> > > -         s->node[node] = n;
>> > >           init_kmem_cache_node(n);
>> > > +         s->node[node] = n;
>> > >   }
>> > >   return 1;
>> > >  }
>> >
>> > If this matters then I have bad feelings about free_kmem_cache_nodes():
>>
>> At creation time the kmem_cache structure is private and no one can run a
>> free operation.
I've double-checked the code path and this turned out to be a false
positive caused by KMSAN not instrumenting the contents of mm/slub.c
(i.e. the initialization of the spinlock remained unnoticed).
Christoph is indeed right that kmem_cache_structure is private, so a
race is not possible here.
I am sorry for the false alarm.
>> > Inviting a use-after-free?  I guess not, as there should be no way
>> > to look up these items at this stage.
>>
>> Right.
>
> Still.   It looks bad, and other sites do these things in the other order.
If the maintainers agree the initialization order needs to be fixed,
we'll need to remove the (irrelevant) KMSAN report from the patch
description.
>> > Could the slab maintainers please take a look at these and also have a
>> > think about Alexander's READ_ONCE/WRITE_ONCE question?
>>
>> Was I cced on these?
>
> It's all on linux-mm.



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-StraÃe, 33
80636 MÃnchen

GeschÃftsfÃhrer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg