Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path
From: Petr Mladek <pmladek@suse.com>
Date: 2021-12-17 13:10:23
Also in:
live-patching, lkml
On Thu 2021-12-16 15:35:42, Greg Kroah-Hartman wrote:
On Thu, Dec 16, 2021 at 03:14:38PM +0100, Petr Mladek wrote:quoted
There is the problem with kobject lifetime and module removal. module is removed after mod->exit() callback finishes. But some kobject release() callbacks might be delayed, especillay when CONFIG_DEBUG_KOBJECT_RELEASE is enabled.Ick, modules and kobjects, just say no :)
I will try to persuade you that it is not that uncommon scenario, see below.
quoted
I have proposed there a solution where kobject_add_internal() takes reference on the module. It would make sure that the module will stay in the memory until the release callbacks is called, see https://lore.kernel.org/all/Ya84O2%2FnYCyNb%2Ffp@alley/ (local)I don't want to add module pointers to kobjects. kobjects are data. modules are code. Module references control code lifespans. Kobject references control data lifespans. They are different, so let us never mix the two please.
I do not undestand this argument. The data are useless without the code. The code is needed to remove the data. Therefore the code should stay until the data are released. I am talking about data using statically defined kobj_type in modules.
Yes, release callbacks are code, but if you really need to worry about your release callback being unloaded, then just refuse to unload your module until your data is all released!
This is exactly what I am proposing. If the kobject release callbacks are defined in the module then the module should stay as long as they are needed.
The huge majority of kobject users never touch them directly, they use the driver model, which should not have this issue. Only code that wants to touch kobjects in the "raw" have problems like this, and if you want to mess with them at that level, you can handle the release data issue.
There seems to be 14 simple modules that define static strcut
kobj_type:
$> for file in `git grep "static struct kobj_type" | cut -d : -f1 | sort -u` ; \
do grep -q "module_init" $file && echo $file ; \
done
arch/sh/kernel/cpu/sh4/sq.c
drivers/block/pktcdvd.c
drivers/firmware/dmi-sysfs.c
drivers/firmware/efi/efivars.c
drivers/firmware/qemu_fw_cfg.c
drivers/net/bonding/bond_main.c
drivers/net/ethernet/ibm/ibmveth.c
drivers/parisc/pdc_stable.c
drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
drivers/platform/x86/intel/uncore-frequency.c
drivers/platform/x86/uv_sysfs.c
drivers/uio/uio.c
kernel/livepatch/core.c
samples/kobject/kset-example.c
The other struct kobj_type are fined in 81 source files:
$> for file in `git grep "static struct kobj_type" | cut -d : -f1 | sort -u` ; \
do grep -q "module_init" $file || echo $file ; \
done | wc -l
81
I believe that most of them might be compiled as modules as well.
There are many non-trivial drivers and file systes. From just a quick
look:
drivers/infiniband/hw/mlx4/sysfs.c
fs/btrfs/sysfs.c
fs/ext4/sysfs.c
Well, we should probably discuss this in the original thread
https://lore.kernel.org/r/20211129034509.2646872-3-ming.lei@redhat.com (local)
Best Regards,
Petr