Message-ID: <YbyMOvBEj9Oj3hkf@alley>
Date:   Fri, 17 Dec 2021 14:10:18 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     David Vernet <void@...ifault.com>, Miroslav Benes <mbenes@...e.cz>,
        linux-doc@...r.kernel.org, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org, jpoimboe@...hat.com,
        jikos@...nel.org, joe.lawrence@...hat.com, corbet@....net,
        yhs@...com, songliubraving@...com, Ming Lei <ming.lei@...hat.com>
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

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:
> > 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.


> > 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/
> 
> 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

Best Regards,
Petr

Powered by blists - more mailing lists