Message-ID: <Ybi3qcA5ySDYpyib@dev0025.ash9.facebook.com>
Date:   Tue, 14 Dec 2021 07:26:33 -0800
From:   David Vernet <void@...ifault.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Petr Mladek <pmladek@...e.com>, 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,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

Miroslav Benes <mbenes@...e.cz> wrote on Tue [2021-Dec-14 10:17:08 +0100]:
> It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.

This is the output when running kpatch load livepatch-sample.ko if an extra
'struct klp_obj' entry is added that has a name but no funcs:

Without patch:

[   67.285762] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
[   67.286107] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_add_internal: parent: 'module', set: 'module'
[   67.286113] kobject: 'holders' (00000000858b03bf): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[   67.286126] kobject: 'notes' (00000000f2a3a4ce): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[   67.297856] kobject: 'holders' (00000000858b03bf): kobject_cleanup, parent 00000000cf89f7b6
[   67.297859] kobject: 'holders' (00000000858b03bf): auto cleanup kobject_del
[   67.297861] kobject: 'holders' (00000000858b03bf): calling ktype release
[   67.297862] kobject: (00000000858b03bf): dynamic_kobj_release
[   67.297863] kobject: 'holders': free name
[   67.297865] kobject: 'notes' (00000000f2a3a4ce): kobject_cleanup, parent 00000000cf89f7b6
[   67.297866] kobject: 'notes' (00000000f2a3a4ce): auto cleanup kobject_del
[   67.297867] kobject: 'notes' (00000000f2a3a4ce): calling ktype release
[   67.297868] kobject: (00000000f2a3a4ce): dynamic_kobj_release
[   67.297869] kobject: 'notes': free name
[   67.297874] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_cleanup, parent 000000002555fa2d
[   67.297876] kobject: 'livepatch_sample' (00000000cf89f7b6): auto cleanup kobject_del
[   67.297877] kobject: 'livepatch_sample' (00000000cf89f7b6): calling ktype release
[   67.297878] kobject: 'livepatch_sample': free name
[   99.445938] kobject: '0:40' (000000002a98d11d): kobject_add_internal: parent: 'bdi', set: 'devices'
[   99.445954] kobject: '0:40' (000000002a98d11d): kobject_uevent_env
[   99.445957] kobject: '0:40' (000000002a98d11d): fill_kobj_path: path = '/devices/virtual/bdi/0:40'

With patch:

[  162.275251] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
[  162.275985] kobject: 'livepatch_sample' (00000000e688ee30): kobject_add_internal: parent: 'module', set: 'module'
[  162.275993] kobject: 'holders' (000000004eee7860): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[  162.276010] kobject: 'notes' (00000000c4f390ab): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[  162.276028] kobject: '(null)' (000000003acccf72): kobject_cleanup, parent 0000000000000000
[  162.276031] kobject: '(null)' (000000003acccf72): calling ktype release
[  162.276033] kobject: '(null)' (00000000aeae6326): kobject_cleanup, parent 0000000000000000
[  162.276035] kobject: '(null)' (00000000aeae6326): calling ktype release
[  162.276037] kobject: '(null)' (0000000093b68297): kobject_cleanup, parent 0000000000000000
[  162.276039] kobject: '(null)' (0000000093b68297): calling ktype release
[  162.294063] kobject: 'holders' (000000004eee7860): kobject_cleanup, parent 00000000e688ee30
[  162.294070] kobject: 'holders' (000000004eee7860): auto cleanup kobject_del
[  162.294074] kobject: 'holders' (000000004eee7860): calling ktype release
[  162.294078] kobject: (000000004eee7860): dynamic_kobj_release
[  162.294081] kobject: 'holders': free name
[  162.294086] kobject: 'notes' (00000000c4f390ab): kobject_cleanup, parent 00000000e688ee30
[  162.294090] kobject: 'notes' (00000000c4f390ab): auto cleanup kobject_del
[  162.294094] kobject: 'notes' (00000000c4f390ab): calling ktype release
[  162.294097] kobject: (00000000c4f390ab): dynamic_kobj_release
[  162.294100] kobject: 'notes': free name
[  162.294114] kobject: 'livepatch_sample' (00000000e688ee30): kobject_cleanup, parent 00000000f9317c72
[  162.294118] kobject: 'livepatch_sample' (00000000e688ee30): auto cleanup kobject_del
[  162.294123] kobject: 'livepatch_sample' (00000000e688ee30): calling ktype release
[  162.294126] kobject: 'livepatch_sample': free name

The extra lines are of course the kobject: '(null)' entries, for which we
*don't* see auto cleanup kobject_del being called. So it seems that what's
there now is probably not actually leaking memory, and the question is
really whether or not the documentation in kobject.c is the source of truth
(i.e. whether the code needs to be "fixed" to honor the API contract).

> I think that this might be, once again, a false positive. We use kobjects 
> differently than what the kobject implementation and its documentation 
> assume.

I'm curious to hear what Greg says. As Petr pointed out, it seems that the
documentation for kobjects is inconsistent. If we're going by the function
comment header in kobject.c then what we have now should probably be
considered a bug? If we're going by what's in
Documentation/core-api/kobject.rst, I think what we have now is correct.

I do think it's a bit of a leaky abstraction to assume that the
implementation doesn't allocate anything, but I also see Petr's point that
it could be useful to make it explicit that kobject_init() doesn't allocate
anything, and instead just affords callers the option of using
kobject_put() if they want to the objects' destructors to be invoked.

Either way, we should fix the documentation to be consistent, which I'm
happy to do in another patch.

> And if it is not a false positive, we should implement some rollback for 
> processed klp_funcs and klp_objects if an error happens. It is not only 
> klp_patch kobject affected.

The patch (though it needs to be corrected in its current form, as Josh
pointed out) does already address this for the klp_funcs and klp_objects.
The 'err' label invokes klp_free_patch_start(), which eventually invokes
__klp_free_objects(), which itself invokes __klp_free_funcs().

Regards,
David

Powered by blists - more mailing lists