Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
livepatch: Use kobjects the right way
There is the following note in Documentation/kobject/txt: One important point cannot be overstated: every kobject must have a release() method, and the kobject must persist (in a consistent state) until that method is called. If these constraints are not met, the code is flawed. The release() method is called when the reference count reaches zero. It typically happens when kobject_put() is called. But it might be delayed when the related sysfs file is opened for reading or writing. To find such bugs, the release() method is delayed using a delayed work when CONFIG_DEBUG_KOBJECT_RELEASE is defined. Livepatch is on the safe side but only because the patch/module could not be removed at the moment. A patchset improving the consistency model is flying around and it would allow to remove unused patches eventually. I have simulated the situation when the modules can be removed and enabled the following config options: CONFIG_DEBUG_OBJECTS=y CONFIG_DEBUG_OBJECTS_FREE=y CONFIG_DEBUG_OBJECTS_TIMERS=y CONFIG_DEBUG_OBJECTS_WORK=y CONFIG_DEBUG_OBJECTS_RCU_HEAD=y CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER=y CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT=1 CONFIG_DEBUG_KOBJECT=y CONFIG_DEBUG_KOBJECT_RELEASE=y Then I got the following crash: dhcp75 login: BUG: unable to handle kernel paging request at ffffffffa0002048 IP: [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60 PGD 1e07067 PUD 1e08063 PMD 139c9a067 PTE 0 Oops: 0000 [#1] SMP Modules linked in: [last unloaded: livepatch_sample] CPU: 1 PID: 5667 Comm: bash Tainted: G W E K 4.6.0-rc7-next-20160510-4-default+ torvalds#231 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 task: ffff8800ba24c740 ti: ffff880138d04000 task.ti: ffff880138d04000 RIP: 0010:[<ffffffff812aa868>] [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60 RSP: 0018:ffff880138d07dd8 EFLAGS: 00010202 RAX: 0000000000000001 RBX: ffffffffa0002020 RCX: 0000000000000000 RDX: 0000000000000001 RSI: ffff880035a51430 RDI: 0000000000000246 RBP: ffff880138d07de0 R08: 0000000000000001 R09: 0000000000000000 R10: ffff8800ba24c740 R11: 0000000000000001 R12: 0000000000000002 R13: ffff880139e617c0 R14: ffff880035813a18 R15: ffff880138d07f20 FS: 00007f13a3927700(0000) GS:ffff88013fc80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffffa0002048 CR3: 0000000139961000 CR4: 00000000000006e0 Stack: ffff880035813a00 ffff880138d07e08 ffffffff812aa8bf ffff880035813a00 ffff880139e617c0 0000000000000002 ffff880138d07e48 ffffffff812a9e84 0000000000000001 ffff8800b8a13540 ffff880138d07f20 00007f13a392c000 Call Trace: [<ffffffff812aa8bf>] sysfs_kf_write+0x1f/0x60 [<ffffffff812a9e84>] kernfs_fop_write+0x144/0x1e0 [<ffffffff81222828>] __vfs_write+0x28/0x120 [<ffffffff810c61b9>] ? percpu_down_read+0x49/0x80 [<ffffffff812261c1>] ? __sb_start_write+0xd1/0xf0 [<ffffffff812261c1>] ? __sb_start_write+0xd1/0xf0 [<ffffffff81222f22>] vfs_write+0xb2/0x1b0 [<ffffffff81244176>] ? __fget_light+0x66/0x90 [<ffffffff81224279>] SyS_write+0x49/0xa0 [<ffffffff819582fc>] entry_SYSCALL_64_fastpath+0x1f/0xbd Code: 44 00 00 0f 1f 44 00 00 55 48 89 e5 53 f6 87 89 00 00 00 01 48 8b 47 28 48 8b 98 80 00 00 00 74 0a 8b 05 7c 6d c7 00 85 c0 75 10 <48> 8b 43 28 48 85 c0 74 27 48 8b 40 08 5b 5d c3 48 83 c7 08 e8 RIP [<ffffffff812aa868>] sysfs_file_ops+0x28/0x60 RSP <ffff880138d07dd8> CR2: ffffffffa0002048 The problem was that struct klp_patch was defined statically in the livepatch module. And the module was removed before the kobject release() method was called. For a short time, we were able to open /sys/kernel/livepatch/livepatch_sample/enabled for writing while the related kobject was already gone. There are two basic approaches how to get out of this trap. Either we need to allocate all the structures dynamically so that they survive the module removal. Or we need to block the module removal until the sysfs entries are released[*]. This patch is an attempt of the first approach. All three structures, klp_patch, klp_object, and klp_func, are dynamically allocated because they all contains kobject and related sysfs entries. The question was how to define the user-provided data an easy way. One approach was to keep the static definition using reduced structures that would later be copied to dynamically allocated structures. It is definitely worth consideration[**]. This patch tries another approach. It adds one more level of functions that are called before the patch is registered. Then patch registration is a clear boundary that marks the patch as complete and ready for enabling. New objects or functions could not be added to the patch from this point on. The three new functions are klp_create_empty_patch(), klp_add_object(), and klp_add_func(). They initialize most of the struct members and contains the related code from the former klp_init_*() functions. The information about loaded objects is still detected during the patch registration and from the module callbacks. Also the sysfs entries are still created during the patch registration. Note that we should not create sysfs entries before the patch is complete to make the handling easier. Anyway, these parts of klp_init_*() functions are moved to klp_registration_*() functions to make it clear in which phase they happen. The patch avoids hardcoding the size of the arrays by using linked lists. Then the standard list_for_each_entry() macro could be used to iterate the list of objects and functions. Also there are helper macros that help with error handling and make the patch definition easier to do and review. The last thing is a patch removal. It can be done only when the patch is disabled. And it is done in one step. To make it symmetric, we need to remove the sysfs entries right after the patch is removed from the global list. The sysfs entries are handled via the kobjects. Therefore the kobjects should be removed at the same time together with the related structures. All the klp_free*() function are renamed to klp_release_*() functions to reduce a potential confusion. Note that the structures are freed by the klp_kobj_release_*() functions that might be called later. [*] We might block removing the module using a completion() that would wait until the top-level kobject release method is called. This approach is used by the module framework itself, see mod_kobject_put() and module_kobj_release(). It even seems to be safe. kobject_cleanup() do not longer access any data from the kobject once the release method is called. And it is a way how to make sure that the structure has valid data until the release method is called. [**] I deemed ugly to define the patch using reduced structures and just copy them from static defined arrays to a dynamically defined arrays. But it would keep the definition reasonable without hardcoded array size and other tricks. Also it would help to pass all data in call and avoid the three levels of functions. It will be possible to do this during the patch registration and avoid the three levels of functions: create+add/register/enable. Also it will avoid the asymmetry between the patch creation and release. Huh, I send this patch because it is working and to show how this approach looks like. I would personally would prefer using the completion. I think that it is not a hack after all. Signed-off-by: Petr Mladek <[email protected]>
- Loading branch information
