On Fri 2025-10-10 16:02:23, Song Liu wrote: > When there is only one function of the same name, old_sympos of 0 and 1 > are logically identical. Match them in klp_find_func(). > > Signed-off-by: Song Liu <[email protected]> > > ---
I would prefer to add the details into the commit message. It would make it clear that it was a real life problem. Also it explains the effects. It would just need to convert the text into the imperative style, like: There are two versions of kpatch-build, ... Even better would be to mention which version introduced the change. But it is not strictly necessary. > This is to avoid a corner case I hit in testing. > > I had two versions of kpatch-build, one assigns old_sympos == 0 for > unique local functions, the other assigns old_sympos == 1 for unique > local functions. Both versions work fine by themselves. > > However, when we do patch upgrade (with the replace flag) with a Imperative language: During the patch upgrade (with the replace ... > patch built with one version of kpatch-build to replace the same fix > bult with the other version of kpatch-build, we hit errors like: > > [ 14.218706] sysfs: cannot create duplicate filename 'xxx/somefunc,1' > ... > [ 14.219466] Call Trace: > [ 14.219468] <TASK> > [ 14.219469] dump_stack_lvl+0x47/0x60 > [ 14.219474] sysfs_warn_dup.cold+0x17/0x27 > [ 14.219476] sysfs_create_dir_ns+0x95/0xb0 > [ 14.219479] kobject_add_internal+0x9e/0x260 > [ 14.219483] kobject_add+0x68/0x80 > [ 14.219485] ? kstrdup+0x3c/0xa0 > [ 14.219486] klp_enable_patch+0x320/0x830 > [ 14.219488] patch_init+0x443/0x1000 [ccc_0_6] > [ 14.219491] ? 0xffffffffa05eb000 > [ 14.219492] do_one_initcall+0x2e/0x190 > [ 14.219494] do_init_module+0x67/0x270 > [ 14.219496] init_module_from_file+0x75/0xa0 > [ 14.219499] idempotent_init_module+0x15a/0x240 > [ 14.219501] __x64_sys_finit_module+0x61/0xc0 > [ 14.219503] do_syscall_64+0x5b/0x160 > [ 14.219505] entry_SYSCALL_64_after_hwframe+0x4b/0x53 > [ 14.219507] RIP: 0033:0x7f545a4bd96d > ... > [ 14.219516] kobject: kobject_add_internal failed for somefunc,1 with > -EEXIST, don't try to register things with the same name ... > > This happens because klp_find_func() thinks somefunc with old_sympos==0 > is not the same as somefunc with old_sympos==1, and klp_add_object_nops > adds another xxx/func,1 to the list of functions to patch. > > Yes, this is a really rare case, and the toolchain (kpatch-build) should > be consistent with old_sympos. But I think we may want to fix the > behavior in kernel just in case. The last paragraph should not be in the comment. Otherwise, I think that the change is worth it. As you say, it is a rare case. But it clearly might happen. And the change makes the livepatching code more robust. Best Regards, Petr
