Re: [PATCH] static_call: Handle module init failure correctly in static_call_del_module()

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Sep 05 2024 at 11:34, Jinjie Ruan wrote:
> On 2024/9/4 16:51, Thomas Gleixner wrote:
>> Can you spot the problem?
>
> It seems that it is not the memory leak source,

That's possible, but the code in there is definitely incorrect when
pci_register_driver() fails. Simply because module->exit() is not
invoked when module->init() fails.

> but with the Link patch, the problem solved and the memory leak missed.

Sure, but the change log of this patch is not really helpful at all and
you sent me a gazillion of leak dumps, but failed to explain what the
actual failure scenario is.

So without a coherent description how to reproduce this issue there is
not much I can do than looking at the code. The amdgpu init() function
was an obvious candidate, but there seems to be some other way to cause
that problem.

Now you at least provided the information that the missing cleanup in
the init() function is not the problem. So the obvious place to look is
in the module core code whether there is a failure path _after_
module->init() returned success.

do_init_module()
        ret = do_one_initcall(mod->init);
        ...
	ret = module_enable_rodata_ro(mod, true);
	if (ret)
		goto fail_mutex_unlock;

and that error path does _not_ invoke module->exit(), which is obviously
not correct. Luis?

I assume that's not the problem when looking at the change log of that:

 https://lore.kernel.org/all/20221112114602.1268989-4-liushixin2@huawei.com/

> Following the rules stated in the comment for kobject_init_and_add():
>  If this function returns an error, kobject_put() must be called to
>  properly clean up the memory associated with the object.
> 
> kobject_put() is more appropriate for error handling after kobject_init().

Why? The rules are exactly the same, no?

> And we can use this function to solve this problem.

Which function and which problem?

> For the cache created early, the related sysfs_slab_add() is called in
> slab_sysfs_init(). Skip free these kmem_cache since they are important
> for system. Keep them working without sysfs.

Let me try to decode this. I assume that sysfs_slab_add() fails after
kobject_init_and_add() or kobject_init_and_add) itself fails.

So the problem is that there are two ways how this can be invoked:

   1) from slab_sysfs_init() for the kmem_cache instances which have
      been allocated during early boot before sysfs was available.

   2) from kmem_cache_create() after early boot

So what Liu tries to avoid is to invoke kobject_put(), because
kobject_put() would not only free the name (which is what the leak
complains about), but also invoke the release callback, which would 
try to destroy the kmem_cache itself.

That's a problem for the mm people to solve, but that does not make my
observations about the amdgpu init() function and the error path in
do_init_module() moot :)

Thanks,

        tglx