On Tue, Jun 02, 2020 at 12:54:16PM -0700, James Bottomley wrote: > On Tue, 2020-06-02 at 19:36 +0200, Greg Kroah-Hartman wrote: > > On Tue, Jun 02, 2020 at 08:25:14AM -0700, James Bottomley wrote: > > > On Tue, 2020-06-02 at 05:10 -0700, Matthew Wilcox wrote: > > > > On Tue, Jun 02, 2020 at 07:50:33PM +0800, Wang Hai wrote: > > > > > syzkaller reports for memory leak when kobject_init_and_add() > > > > > returns an error in the function sysfs_slab_add() [1] > > > > > > > > > > When this happened, the function kobject_put() is not called > > > > > for the corresponding kobject, which potentially leads to > > > > > memory leak. > > > > > > > > > > This patch fixes the issue by calling kobject_put() even if > > > > > kobject_init_and_add() fails. > > > > > > > > I think this speaks to a deeper problem with > > > > kobject_init_and_add() > > > > -- the need to call kobject_put() if it fails is not readily > > > > apparent > > > > to most users. This same bug appears in the first three users of > > > > kobject_init_and_add() that I checked -- > > > > arch/ia64/kernel/topology.c > > > > drivers/firmware/dmi-sysfs.c > > > > drivers/firmware/efi/esrt.c > > > > drivers/scsi/iscsi_boot_sysfs.c > > > > > > > > Some do get it right -- > > > > arch/powerpc/kernel/cacheinfo.c > > > > drivers/gpu/drm/ttm/ttm_bo.c > > > > drivers/gpu/drm/ttm/ttm_memory.c > > > > drivers/infiniband/hw/mlx4/sysfs.c > > > > > > > > I'd argue that the current behaviour is wrong, > > > > > > Absolutely agree with this. We have a big meta pattern here where > > > we introduce functions with tortuous semantics then someone creates > > > a checker for the semantics and misuses come crawling out of the > > > woodwork leading to floods of patches, usually for little or never > > > used error paths, which really don't buy anything apart from > > > theoretical correctness. Just insisting on simple semantics would > > > have avoided this. > > > > I "introduced" this way back at the end of 2007. It's not exactly a > > new function. > > Heh, well, if it never fails, how you handle the failure become > unimportant semantics ... > > > > > that kobject_init_and_add() should call kobject_put() if the add > > > > fails. This would need a tree-wide audit. But somebody needs to > > > > do that anyway because based on my random sampling, half of the > > > > users currently get it wrong. > > > > > > Well, the semantics of kobject_init() are free on fail, so these > > > are the ones everyone seems to be using. The semantics of > > > kobject_add are put on fail. The problem is that put on fail isn't > > > necessarily correct in the kobject_init() case: the release > > > function may make assumptions about the object hierarchy which > > > aren't satisfied in the kobject_init() failure case. This argues > > > that kobject_init_and_add() can't ever have correct semantics and > > > we should eliminate it. > > > > At the time, it did reduce common functionality and error handling > > all into a simpler function. And, given it's history, it must have > > somehow worked for the past 12 years or so :) > > Well, like I said, as long as it never fails, no problem. > > It was just Matthew saying "couldn't we make it do kobject_put() > itself?" that got me thinking that perhaps that wouldn't work with all > cases. So now we're discussing failure handling, we're into the > esoteric rabbit hole case that never happens. > > > Odds are, lots of the callers shouldn't be messing around with > > kobjects in the first place. Originally it was only assumed that > > there would be very few users. But it has spread to filesystems and > > firmware subsystems. Drivers should never use it though, so it's a > > good hint something is wrong there... > > > > Anyway, patches to fix this up to make a "sane" api for kobjects is > > always appreciated. Personally I don't have the time at the moment. > > I think the only way we can make the failure semantics consistent is to > have the kobject_init() ones (so kfree on failure). That means for the > add part, the function would have to unwind everything it did from init > on so kfree() is still an option. If people agree, then I can produce > the patch ... it's just the current drive to transform everyone who's > doing kfree() into kobject_put() would become wrong ... Everyone should be putting their kfree into the kobject release anyway, right? Anyway, let's see your patch before I start to object further :) thanks, greg k-h
