Re: [PATCH v1] mm/slab: Allow cache creation to proceed even if sysfs registration fails

[Vlastimil Babka <vbabka@suse.cz>]

On 11/2/24 15:51, Hyeonggon Yoo wrote:
> On Sat, Nov 2, 2024 at 7:16 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>>
>> On Sat, Nov 2, 2024 at 7:07 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>> >
>> > On Sat, Nov 2, 2024 at 5:50 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>> > >
>> > > On 11/2/24 8:18 AM, Hyeonggon Yoo wrote:
>> > > >>
>> > > >> Also here, and simplified to "if (sysfs_slab_add(s)) ... " to avoid dealing
>> > > >> with err.
>> > > >
>> > > > Oh no. err is initialized to -EINVAL, so that will not work as intended.
>> > > > It is causing the following list corruption.
>> > >
>> > > Ooops, right, thanks a lot. Wrongly assumed that a test boot in
>> > > virtme-ng would catch silly mistakes like that. Looks like all caches
>> > > were created with SLAB_STATE < FULL.
>> > >
>> > > Fixed by setting err = 0 before trying sysfs add.
>> >
>> > Thanks!
>> >
>> > Hmm... by the way, why doesn't SLUB update 'err' in the event of an error in
>> > init_cache_random_seq(), init_kmem_cache_nodes(), or alloc_kmem_cache_cpus()?
>> > I may be missing something, but it doesn't seem to handle these errors
>> > properly to me...
>>
>> Oh, it seems like a recent change fc0eac57d08c ("slab: pull kmem_cache_open()
>> into do_kmem_cache_create()") incorrectly pulled kmem_cache_open()?
>>
>> Cc-ing Christian Brauner.
> 
> Apologies for the oversight; it slipped my mind. It was actually
> correct after all. :(

Yeah AFAICS that commit didn't change it. But yeah it would make sense to
clean up those init/alloc functions so they all return 0 on success and
error code on failure, and wire that up so allocation failures are -ENOMEM
and not -EINVAL. Probably only some fault injection environments are going
to notice, though.