| Message ID | 20221021022102.2231464-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix memory leak while kset_register() fails | expand |
On 2022-10-20 22:20, Yang Yingliang wrote: > The previous discussion link: > https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ The very first discussion on this was here: https://www.spinics.net/lists/dri-devel/msg368077.html Please use this link, and not the that one up there you which quoted above, and whose commit description is taken verbatim from the this link. > > kset_register() is currently used in some places without calling > kset_put() in error path, because the callers think it should be > kset internal thing to do, but the driver core can not know what > caller doing with that memory at times. The memory could be freed > both in kset_put() and error path of caller, if it is called in > kset_register(). As I explained in the link above, the reason there's a memory leak is that one cannot call kset_register() without the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, in this case, i.e. kset_register() fails with -EINVAL. Thus, the most common usage is something like this: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); So, what is being leaked, is the memory allocated in kobj_set_name(), by the common idiom shown above. This needs to be mentioned in the documentation, at least, in case, in the future this is absolved in kset_register() redesign, etc. Regards, Luben > > So make the function documentation more explicit about calling > kset_put() in the error path of caller first, so that people > have a chance to know what to do here, then fixes this leaks > by calling kset_put() from callers. > > Liu Shixin (1): > ubifs: Fix memory leak in ubifs_sysfs_init() > > Yang Yingliang (10): > kset: fix documentation for kset_register() > kset: add null pointer check in kset_put() > bus: fix possible memory leak in bus_register() > kobject: fix possible memory leak in kset_create_and_add() > class: fix possible memory leak in __class_register() > firmware: qemu_fw_cfg: fix possible memory leak in > fw_cfg_build_symlink() > f2fs: fix possible memory leak in f2fs_init_sysfs() > erofs: fix possible memory leak in erofs_init_sysfs() > ocfs2: possible memory leak in mlog_sys_init() > drm/amdgpu/discovery: fix possible memory leak > > drivers/base/bus.c | 4 +++- > drivers/base/class.c | 6 ++++++ > drivers/firmware/qemu_fw_cfg.c | 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- > fs/erofs/sysfs.c | 4 +++- > fs/f2fs/sysfs.c | 4 +++- > fs/ocfs2/cluster/masklog.c | 7 ++++++- > fs/ubifs/sysfs.c | 2 ++ > include/linux/kobject.h | 3 ++- > lib/kobject.c | 5 ++++- > 10 files changed, 33 insertions(+), 9 deletions(-) >
On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: > On 2022-10-20 22:20, Yang Yingliang wrote: > > The previous discussion link: > > https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ > > The very first discussion on this was here: > > https://www.spinics.net/lists/dri-devel/msg368077.html > > Please use this link, and not the that one up there you which quoted above, > and whose commit description is taken verbatim from the this link. > > > > > kset_register() is currently used in some places without calling > > kset_put() in error path, because the callers think it should be > > kset internal thing to do, but the driver core can not know what > > caller doing with that memory at times. The memory could be freed > > both in kset_put() and error path of caller, if it is called in > > kset_register(). > > As I explained in the link above, the reason there's > a memory leak is that one cannot call kset_register() without > the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, > in this case, i.e. kset_register() fails with -EINVAL. > > Thus, the most common usage is something like this: > > kobj_set_name(&kset->kobj, format, ...); > kset->kobj.kset = parent_kset; > kset->kobj.ktype = ktype; > res = kset_register(kset); > > So, what is being leaked, is the memory allocated in kobj_set_name(), > by the common idiom shown above. This needs to be mentioned in > the documentation, at least, in case, in the future this is absolved > in kset_register() redesign, etc. Based on this, can kset_register() just clean up from itself when an error happens? Ideally that would be the case, as the odds of a kset being embedded in a larger structure is probably slim, but we would have to search the tree to make sure. thanks, greg k-h
Hi, On 2022/10/21 13:29, Luben Tuikov wrote: > On 2022-10-20 22:20, Yang Yingliang wrote: >> The previous discussion link: >> https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ > The very first discussion on this was here: > > https://www.spinics.net/lists/dri-devel/msg368077.html > > Please use this link, and not the that one up there you which quoted above, > and whose commit description is taken verbatim from the this link. I found this leaks in bus_register()/class_register()/kset_create_and_add() at first, and describe the reason in these patches which is using kobject_set_name() description, here is the patches: https://lore.kernel.org/lkml/20221017014957.156645-1-yangyingliang@huawei.com/T/ https://lore.kernel.org/lkml/20221017031335.1845383-1-yangyingliang@huawei.com/ https://lore.kernel.org/lkml/Y0zfPKAgQSrYZg5o@kroah.com/T/ And then I found other subsystem also have this problem, so posted the fix patches for them (including qemu_fw_cfg/f2fs/erofs/ocfs2/amdgpu_discovery): https://www.mail-archive.com/qemu-devel@nongnu.org/msg915553.html https://lore.kernel.org/linux-f2fs-devel/7908686b-9a7c-b754-d312-d689fc28366e@kernel.org/T/#t https://lore.kernel.org/linux-erofs/20221018073947.693206-1-yangyingliang@huawei.com/ https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ https://www.spinics.net/lists/dri-devel/msg368092.html In the amdgpu_discovery patch, I sent a old one which using wrong description and you pointer out, and then I send a v2. And then the maintainer of ocfs2 has different thought about this, so we had a discussion in the link that I gave out, and Greg suggested me to update kset_register() documentation and then put the fix patches together in one series, so I sent this patchset and use the link. Thanks, Yang > >> kset_register() is currently used in some places without calling >> kset_put() in error path, because the callers think it should be >> kset internal thing to do, but the driver core can not know what >> caller doing with that memory at times. The memory could be freed >> both in kset_put() and error path of caller, if it is called in >> kset_register(). > As I explained in the link above, the reason there's > a memory leak is that one cannot call kset_register() without > the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, > in this case, i.e. kset_register() fails with -EINVAL. > > Thus, the most common usage is something like this: > > kobj_set_name(&kset->kobj, format, ...); > kset->kobj.kset = parent_kset; > kset->kobj.ktype = ktype; > res = kset_register(kset); > > So, what is being leaked, is the memory allocated in kobj_set_name(), > by the common idiom shown above. This needs to be mentioned in > the documentation, at least, in case, in the future this is absolved > in kset_register() redesign, etc. > > Regards, > Luben > >> So make the function documentation more explicit about calling >> kset_put() in the error path of caller first, so that people >> have a chance to know what to do here, then fixes this leaks >> by calling kset_put() from callers. >> >> Liu Shixin (1): >> ubifs: Fix memory leak in ubifs_sysfs_init() >> >> Yang Yingliang (10): >> kset: fix documentation for kset_register() >> kset: add null pointer check in kset_put() >> bus: fix possible memory leak in bus_register() >> kobject: fix possible memory leak in kset_create_and_add() >> class: fix possible memory leak in __class_register() >> firmware: qemu_fw_cfg: fix possible memory leak in >> fw_cfg_build_symlink() >> f2fs: fix possible memory leak in f2fs_init_sysfs() >> erofs: fix possible memory leak in erofs_init_sysfs() >> ocfs2: possible memory leak in mlog_sys_init() >> drm/amdgpu/discovery: fix possible memory leak >> >> drivers/base/bus.c | 4 +++- >> drivers/base/class.c | 6 ++++++ >> drivers/firmware/qemu_fw_cfg.c | 2 +- >> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- >> fs/erofs/sysfs.c | 4 +++- >> fs/f2fs/sysfs.c | 4 +++- >> fs/ocfs2/cluster/masklog.c | 7 ++++++- >> fs/ubifs/sysfs.c | 2 ++ >> include/linux/kobject.h | 3 ++- >> lib/kobject.c | 5 ++++- >> 10 files changed, 33 insertions(+), 9 deletions(-) >> > .
On 2022-10-21 01:37, Greg KH wrote: > On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >> On 2022-10-20 22:20, Yang Yingliang wrote: >>> The previous discussion link: >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C65b33f087ef245a9f23708dab3264840%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019274318153227%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1ZoieEob62iU9kI8fvpp20qGut9EeHKIHtCAT01t%2Bz8%3D&reserved=0 >> >> The very first discussion on this was here: >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C65b33f087ef245a9f23708dab3264840%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019274318153227%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9joWxGLUxZZMvrfkxCR8KbkoXifsqoMK0vGR%2FyEG62w%3D&reserved=0 >> >> Please use this link, and not the that one up there you which quoted above, >> and whose commit description is taken verbatim from the this link. >> >>> >>> kset_register() is currently used in some places without calling >>> kset_put() in error path, because the callers think it should be >>> kset internal thing to do, but the driver core can not know what >>> caller doing with that memory at times. The memory could be freed >>> both in kset_put() and error path of caller, if it is called in >>> kset_register(). >> >> As I explained in the link above, the reason there's >> a memory leak is that one cannot call kset_register() without >> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >> in this case, i.e. kset_register() fails with -EINVAL. >> >> Thus, the most common usage is something like this: >> >> kobj_set_name(&kset->kobj, format, ...); >> kset->kobj.kset = parent_kset; >> kset->kobj.ktype = ktype; >> res = kset_register(kset); >> >> So, what is being leaked, is the memory allocated in kobj_set_name(), >> by the common idiom shown above. This needs to be mentioned in >> the documentation, at least, in case, in the future this is absolved >> in kset_register() redesign, etc. > > Based on this, can kset_register() just clean up from itself when an > error happens? Ideally that would be the case, as the odds of a kset > being embedded in a larger structure is probably slim, but we would have > to search the tree to make sure. Looking at kset_register(), we can add kset_put() in the error path, when kobject_add_internal(&kset->kobj) fails. See the attached patch. It needs to be tested with the same error injection as Yang has been doing. Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c starting at line 575. If you're on an AMD system, it gets you the tree structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". That shouldn't be a problem though. Regards, Luben
On Fri, Oct 21, 2022 at 03:55:18AM -0400, Luben Tuikov wrote: > On 2022-10-21 01:37, Greg KH wrote: > > On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: > >> On 2022-10-20 22:20, Yang Yingliang wrote: > >>> The previous discussion link: > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C65b33f087ef245a9f23708dab3264840%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019274318153227%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1ZoieEob62iU9kI8fvpp20qGut9EeHKIHtCAT01t%2Bz8%3D&reserved=0 > >> > >> The very first discussion on this was here: > >> > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C65b33f087ef245a9f23708dab3264840%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019274318153227%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9joWxGLUxZZMvrfkxCR8KbkoXifsqoMK0vGR%2FyEG62w%3D&reserved=0 > >> > >> Please use this link, and not the that one up there you which quoted above, > >> and whose commit description is taken verbatim from the this link. > >> > >>> > >>> kset_register() is currently used in some places without calling > >>> kset_put() in error path, because the callers think it should be > >>> kset internal thing to do, but the driver core can not know what > >>> caller doing with that memory at times. The memory could be freed > >>> both in kset_put() and error path of caller, if it is called in > >>> kset_register(). > >> > >> As I explained in the link above, the reason there's > >> a memory leak is that one cannot call kset_register() without > >> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, > >> in this case, i.e. kset_register() fails with -EINVAL. > >> > >> Thus, the most common usage is something like this: > >> > >> kobj_set_name(&kset->kobj, format, ...); > >> kset->kobj.kset = parent_kset; > >> kset->kobj.ktype = ktype; > >> res = kset_register(kset); > >> > >> So, what is being leaked, is the memory allocated in kobj_set_name(), > >> by the common idiom shown above. This needs to be mentioned in > >> the documentation, at least, in case, in the future this is absolved > >> in kset_register() redesign, etc. > > > > Based on this, can kset_register() just clean up from itself when an > > error happens? Ideally that would be the case, as the odds of a kset > > being embedded in a larger structure is probably slim, but we would have > > to search the tree to make sure. > > Looking at kset_register(), we can add kset_put() in the error path, > when kobject_add_internal(&kset->kobj) fails. > > See the attached patch. It needs to be tested with the same error injection > as Yang has been doing. > > Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c > starting at line 575. If you're on an AMD system, it gets you the tree > structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". > That shouldn't be a problem though. Yes, that shouldn't be an issue as the kobject embedded in a kset is ONLY for that kset itself, the kset structure should not be controling the lifespan of the object it is embedded in, right? Note, the use of ksets by a device driver like you are doing here in the amd driver is BROKEN and will cause problems by userspace tools. Don't do that please, just use a single subdirectory for an attribute. Doing deeper stuff like this is sure to cause problems and be a headache. thanks, greg k-h
On 2022/10/21 13:37, Greg KH wrote: > On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >> On 2022-10-20 22:20, Yang Yingliang wrote: >>> The previous discussion link: >>> https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ >> The very first discussion on this was here: >> >> https://www.spinics.net/lists/dri-devel/msg368077.html >> >> Please use this link, and not the that one up there you which quoted above, >> and whose commit description is taken verbatim from the this link. >> >>> kset_register() is currently used in some places without calling >>> kset_put() in error path, because the callers think it should be >>> kset internal thing to do, but the driver core can not know what >>> caller doing with that memory at times. The memory could be freed >>> both in kset_put() and error path of caller, if it is called in >>> kset_register(). >> As I explained in the link above, the reason there's >> a memory leak is that one cannot call kset_register() without >> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >> in this case, i.e. kset_register() fails with -EINVAL. >> >> Thus, the most common usage is something like this: >> >> kobj_set_name(&kset->kobj, format, ...); >> kset->kobj.kset = parent_kset; >> kset->kobj.ktype = ktype; >> res = kset_register(kset); >> >> So, what is being leaked, is the memory allocated in kobj_set_name(), >> by the common idiom shown above. This needs to be mentioned in >> the documentation, at least, in case, in the future this is absolved >> in kset_register() redesign, etc. > Based on this, can kset_register() just clean up from itself when an > error happens? Ideally that would be the case, as the odds of a kset > being embedded in a larger structure is probably slim, but we would have > to search the tree to make sure. I have search the whole tree, the kset used in bus_register() - patch #3, kset_create_and_add() - patch #4 __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and amdgpu_discovery.c - patch #10 is embedded in a larger structure. In these cases, we can not call kset_put() in error path in kset_register() itself. Thanks, Yang > > thanks, > > greg k-h > .
On 2022-10-21 04:18, Greg KH wrote: > On Fri, Oct 21, 2022 at 03:55:18AM -0400, Luben Tuikov wrote: >> On 2022-10-21 01:37, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>> The previous discussion link: >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2Bj1THkHpzVGks5eqB%2Fm%2FPAkMRohR7CYvRnOCqUqdcM%3D&reserved=0 >>>> >>>> The very first discussion on this was here: >>>> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pSR10abmK8nAMvKSezqWC0SPUBL4qEwtCCizyIKW7Dc%3D&reserved=0 >>>> >>>> Please use this link, and not the that one up there you which quoted above, >>>> and whose commit description is taken verbatim from the this link. >>>> >>>>> >>>>> kset_register() is currently used in some places without calling >>>>> kset_put() in error path, because the callers think it should be >>>>> kset internal thing to do, but the driver core can not know what >>>>> caller doing with that memory at times. The memory could be freed >>>>> both in kset_put() and error path of caller, if it is called in >>>>> kset_register(). >>>> >>>> As I explained in the link above, the reason there's >>>> a memory leak is that one cannot call kset_register() without >>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>> in this case, i.e. kset_register() fails with -EINVAL. >>>> >>>> Thus, the most common usage is something like this: >>>> >>>> kobj_set_name(&kset->kobj, format, ...); >>>> kset->kobj.kset = parent_kset; >>>> kset->kobj.ktype = ktype; >>>> res = kset_register(kset); >>>> >>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>> by the common idiom shown above. This needs to be mentioned in >>>> the documentation, at least, in case, in the future this is absolved >>>> in kset_register() redesign, etc. >>> >>> Based on this, can kset_register() just clean up from itself when an >>> error happens? Ideally that would be the case, as the odds of a kset >>> being embedded in a larger structure is probably slim, but we would have >>> to search the tree to make sure. >> >> Looking at kset_register(), we can add kset_put() in the error path, >> when kobject_add_internal(&kset->kobj) fails. >> >> See the attached patch. It needs to be tested with the same error injection >> as Yang has been doing. >> >> Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c >> starting at line 575. If you're on an AMD system, it gets you the tree >> structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". >> That shouldn't be a problem though. > > Yes, that shouldn't be an issue as the kobject embedded in a kset is > ONLY for that kset itself, the kset structure should not be controling > the lifespan of the object it is embedded in, right? Yes, and it doesn't. It only does a kobject_get(parent) and kobject_put(parent). So that's fine and natural. Yang, do you want to try the patch in my previous email in this thread, since you've got the error injection set up already? Regards, Luben
On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: > > On 2022/10/21 13:37, Greg KH wrote: > > On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: > > > On 2022-10-20 22:20, Yang Yingliang wrote: > > > > The previous discussion link: > > > > https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ > > > The very first discussion on this was here: > > > > > > https://www.spinics.net/lists/dri-devel/msg368077.html > > > > > > Please use this link, and not the that one up there you which quoted above, > > > and whose commit description is taken verbatim from the this link. > > > > > > > kset_register() is currently used in some places without calling > > > > kset_put() in error path, because the callers think it should be > > > > kset internal thing to do, but the driver core can not know what > > > > caller doing with that memory at times. The memory could be freed > > > > both in kset_put() and error path of caller, if it is called in > > > > kset_register(). > > > As I explained in the link above, the reason there's > > > a memory leak is that one cannot call kset_register() without > > > the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, > > > in this case, i.e. kset_register() fails with -EINVAL. > > > > > > Thus, the most common usage is something like this: > > > > > > kobj_set_name(&kset->kobj, format, ...); > > > kset->kobj.kset = parent_kset; > > > kset->kobj.ktype = ktype; > > > res = kset_register(kset); > > > > > > So, what is being leaked, is the memory allocated in kobj_set_name(), > > > by the common idiom shown above. This needs to be mentioned in > > > the documentation, at least, in case, in the future this is absolved > > > in kset_register() redesign, etc. > > Based on this, can kset_register() just clean up from itself when an > > error happens? Ideally that would be the case, as the odds of a kset > > being embedded in a larger structure is probably slim, but we would have > > to search the tree to make sure. > I have search the whole tree, the kset used in bus_register() - patch #3, > kset_create_and_add() - patch #4 > __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and > amdgpu_discovery.c - patch #10 > is embedded in a larger structure. In these cases, we can not call > kset_put() in error path in kset_register() Yes you can as the kobject in the kset should NOT be controling the lifespan of those larger objects. If it is, please point out the call chain here as I don't think that should be possible. Note all of this is a mess because the kobject name stuff was added much later, after the driver model had been created and running for a while. We missed this error path when adding the dynamic kobject name logic, thank for looking into this. If you could test the patch posted with your error injection systems, that could make this all much simpler to solve. thanks, greg k-h
On 2022-10-21 04:24, Luben Tuikov wrote: > On 2022-10-21 04:18, Greg KH wrote: >> On Fri, Oct 21, 2022 at 03:55:18AM -0400, Luben Tuikov wrote: >>> On 2022-10-21 01:37, Greg KH wrote: >>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>> The previous discussion link: >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2Bj1THkHpzVGks5eqB%2Fm%2FPAkMRohR7CYvRnOCqUqdcM%3D&reserved=0 >>>>> >>>>> The very first discussion on this was here: >>>>> >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pSR10abmK8nAMvKSezqWC0SPUBL4qEwtCCizyIKW7Dc%3D&reserved=0 >>>>> >>>>> Please use this link, and not the that one up there you which quoted above, >>>>> and whose commit description is taken verbatim from the this link. >>>>> >>>>>> >>>>>> kset_register() is currently used in some places without calling >>>>>> kset_put() in error path, because the callers think it should be >>>>>> kset internal thing to do, but the driver core can not know what >>>>>> caller doing with that memory at times. The memory could be freed >>>>>> both in kset_put() and error path of caller, if it is called in >>>>>> kset_register(). >>>>> >>>>> As I explained in the link above, the reason there's >>>>> a memory leak is that one cannot call kset_register() without >>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>> >>>>> Thus, the most common usage is something like this: >>>>> >>>>> kobj_set_name(&kset->kobj, format, ...); >>>>> kset->kobj.kset = parent_kset; >>>>> kset->kobj.ktype = ktype; >>>>> res = kset_register(kset); >>>>> >>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>> by the common idiom shown above. This needs to be mentioned in >>>>> the documentation, at least, in case, in the future this is absolved >>>>> in kset_register() redesign, etc. >>>> >>>> Based on this, can kset_register() just clean up from itself when an >>>> error happens? Ideally that would be the case, as the odds of a kset >>>> being embedded in a larger structure is probably slim, but we would have >>>> to search the tree to make sure. >>> >>> Looking at kset_register(), we can add kset_put() in the error path, >>> when kobject_add_internal(&kset->kobj) fails. >>> >>> See the attached patch. It needs to be tested with the same error injection >>> as Yang has been doing. >>> >>> Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c >>> starting at line 575. If you're on an AMD system, it gets you the tree >>> structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". >>> That shouldn't be a problem though. >> >> Yes, that shouldn't be an issue as the kobject embedded in a kset is >> ONLY for that kset itself, the kset structure should not be controling >> the lifespan of the object it is embedded in, right? > > Yes, and it doesn't. It only does a kobject_get(parent) and kobject_put(parent). > So that's fine and natural. > > Yang, do you want to try the patch in my previous email in this thread, since you've > got the error injection set up already? I spoke too soon. I believe you're onto something, because looking at the idiom: kobj_set_name(&kset->kobj, format, ...); kset->kobj.kset = parent_kset; kset->kobj.ktype = ktype; res = kset_register(kset); The ktype defines a release method, which frees the larger struct the kset is embedded in. And this would result in double free, for instance in the amdgpu_discovery.c code, if kset_put() is called after kset_register() fails, since we kzalloc the larger object just before and kfree it on error just after. Ideally, we'd only "revert" the actions done by kobj_set_name(), as there's some error recovery on create_dir() in kobject_add_internal(). So, we cannot do this business with the kset_put() on error from kset_register(), after all. Not sure how this wasn't caught in Yang's testing--the kernel should've complained. Regards, Luben
On 2022-10-21 04:36, Greg KH wrote: > On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >> >> On 2022/10/21 13:37, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>> The previous discussion link: >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7Ca8206f9348e04b13e3da08dab33f4f53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019381738406942%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HqvF1p4ejF6%2BYS5u0pe15ZdDgUAIVP%2BB1xQXICWjNwY%3D&reserved=0 >>>> The very first discussion on this was here: >>>> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7Ca8206f9348e04b13e3da08dab33f4f53%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019381738406942%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=LmRgWUSQgK6wJvMdfBgjO4CiaKQ2TBoeW836r0UbcjU%3D&reserved=0 >>>> >>>> Please use this link, and not the that one up there you which quoted above, >>>> and whose commit description is taken verbatim from the this link. >>>> >>>>> kset_register() is currently used in some places without calling >>>>> kset_put() in error path, because the callers think it should be >>>>> kset internal thing to do, but the driver core can not know what >>>>> caller doing with that memory at times. The memory could be freed >>>>> both in kset_put() and error path of caller, if it is called in >>>>> kset_register(). >>>> As I explained in the link above, the reason there's >>>> a memory leak is that one cannot call kset_register() without >>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>> in this case, i.e. kset_register() fails with -EINVAL. >>>> >>>> Thus, the most common usage is something like this: >>>> >>>> kobj_set_name(&kset->kobj, format, ...); >>>> kset->kobj.kset = parent_kset; >>>> kset->kobj.ktype = ktype; >>>> res = kset_register(kset); >>>> >>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>> by the common idiom shown above. This needs to be mentioned in >>>> the documentation, at least, in case, in the future this is absolved >>>> in kset_register() redesign, etc. >>> Based on this, can kset_register() just clean up from itself when an >>> error happens? Ideally that would be the case, as the odds of a kset >>> being embedded in a larger structure is probably slim, but we would have >>> to search the tree to make sure. >> I have search the whole tree, the kset used in bus_register() - patch #3, >> kset_create_and_add() - patch #4 >> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >> amdgpu_discovery.c - patch #10 >> is embedded in a larger structure. In these cases, we can not call >> kset_put() in error path in kset_register() > > Yes you can as the kobject in the kset should NOT be controling the > lifespan of those larger objects. > > If it is, please point out the call chain here as I don't think that > should be possible. WLOG, I believe it is something like this: x = kzalloc(); kobject_set_name(&x->kset.kobj, format, ...); x->kset.kobj.kset = parent_kset; x->kset.kobj.ktype = this_ktype; /* this has a .release which frees x */ res = kset_register(&x->kset); if (res) { kset_put(&x->kset); /* calls this_ktype->release() which frees x */ kfree(x); /* <-- double free */ } And since kref is set to 1, in kset_register(), then we'd double free. This is why I don't have kset_put() in that error path in amdgpu. Regards, Luben
On 2022/10/21 16:36, Greg KH wrote: > On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >> On 2022/10/21 13:37, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>> The previous discussion link: >>>>> https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ >>>> The very first discussion on this was here: >>>> >>>> https://www.spinics.net/lists/dri-devel/msg368077.html >>>> >>>> Please use this link, and not the that one up there you which quoted above, >>>> and whose commit description is taken verbatim from the this link. >>>> >>>>> kset_register() is currently used in some places without calling >>>>> kset_put() in error path, because the callers think it should be >>>>> kset internal thing to do, but the driver core can not know what >>>>> caller doing with that memory at times. The memory could be freed >>>>> both in kset_put() and error path of caller, if it is called in >>>>> kset_register(). >>>> As I explained in the link above, the reason there's >>>> a memory leak is that one cannot call kset_register() without >>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>> in this case, i.e. kset_register() fails with -EINVAL. >>>> >>>> Thus, the most common usage is something like this: >>>> >>>> kobj_set_name(&kset->kobj, format, ...); >>>> kset->kobj.kset = parent_kset; >>>> kset->kobj.ktype = ktype; >>>> res = kset_register(kset); >>>> >>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>> by the common idiom shown above. This needs to be mentioned in >>>> the documentation, at least, in case, in the future this is absolved >>>> in kset_register() redesign, etc. >>> Based on this, can kset_register() just clean up from itself when an >>> error happens? Ideally that would be the case, as the odds of a kset >>> being embedded in a larger structure is probably slim, but we would have >>> to search the tree to make sure. >> I have search the whole tree, the kset used in bus_register() - patch #3, >> kset_create_and_add() - patch #4 >> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >> amdgpu_discovery.c - patch #10 >> is embedded in a larger structure. In these cases, we can not call >> kset_put() in error path in kset_register() > Yes you can as the kobject in the kset should NOT be controling the > lifespan of those larger objects. > > If it is, please point out the call chain here as I don't think that > should be possible. > > Note all of this is a mess because the kobject name stuff was added much > later, after the driver model had been created and running for a while. > We missed this error path when adding the dynamic kobject name logic, > thank for looking into this. > > If you could test the patch posted with your error injection systems, > that could make this all much simpler to solve. The patch posted by Luben will cause double free in some cases. From 71e0a22801c0699f67ea40ed96e0a7d7d9e0f318 Mon Sep 17 00:00:00 2001 From: Luben Tuikov <luben.tuikov@amd.com> Date: Fri, 21 Oct 2022 03:34:21 -0400 Subject: [PATCH] kobject: Add kset_put() if kset_register() fails X-check-string-leak: v1.0 If kset_register() fails, we call kset_put() before returning the error. This makes sure that we free memory allocated by kobj_set_name() for the kset, since kset_register() cannot be called unless the kset has a name, usually gotten via kobj_set_name(&kset->kobj, format, ...); Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Rafael J. Wysocki <rafael@kernel.org> Cc: Yang Yingliang <yangyingliang@huawei.com> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com> --- lib/kobject.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/kobject.c b/lib/kobject.c index a0b2dbfcfa2334..c122b979f2b75e 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,10 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + kset_put(k); return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; }
On 2022-10-21 04:59, Yang Yingliang wrote: > > On 2022/10/21 16:36, Greg KH wrote: >> On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >>> On 2022/10/21 13:37, Greg KH wrote: >>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>> The previous discussion link: >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RcK05cXm1J5%2BtYcLO2SMG7k6sjeymQzdBzMCDJSzfdE%3D&reserved=0 >>>>> The very first discussion on this was here: >>>>> >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sHZ6kfLF8HxrNXV6%2FVjgdH%2BmQM4T3Zv0U%2FAwddT97cE%3D&reserved=0 >>>>> >>>>> Please use this link, and not the that one up there you which quoted above, >>>>> and whose commit description is taken verbatim from the this link. >>>>> >>>>>> kset_register() is currently used in some places without calling >>>>>> kset_put() in error path, because the callers think it should be >>>>>> kset internal thing to do, but the driver core can not know what >>>>>> caller doing with that memory at times. The memory could be freed >>>>>> both in kset_put() and error path of caller, if it is called in >>>>>> kset_register(). >>>>> As I explained in the link above, the reason there's >>>>> a memory leak is that one cannot call kset_register() without >>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>> >>>>> Thus, the most common usage is something like this: >>>>> >>>>> kobj_set_name(&kset->kobj, format, ...); >>>>> kset->kobj.kset = parent_kset; >>>>> kset->kobj.ktype = ktype; >>>>> res = kset_register(kset); >>>>> >>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>> by the common idiom shown above. This needs to be mentioned in >>>>> the documentation, at least, in case, in the future this is absolved >>>>> in kset_register() redesign, etc. >>>> Based on this, can kset_register() just clean up from itself when an >>>> error happens? Ideally that would be the case, as the odds of a kset >>>> being embedded in a larger structure is probably slim, but we would have >>>> to search the tree to make sure. >>> I have search the whole tree, the kset used in bus_register() - patch #3, >>> kset_create_and_add() - patch #4 >>> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >>> amdgpu_discovery.c - patch #10 >>> is embedded in a larger structure. In these cases, we can not call >>> kset_put() in error path in kset_register() >> Yes you can as the kobject in the kset should NOT be controling the >> lifespan of those larger objects. >> >> If it is, please point out the call chain here as I don't think that >> should be possible. >> >> Note all of this is a mess because the kobject name stuff was added much >> later, after the driver model had been created and running for a while. >> We missed this error path when adding the dynamic kobject name logic, >> thank for looking into this. >> >> If you could test the patch posted with your error injection systems, >> that could make this all much simpler to solve. > > The patch posted by Luben will cause double free in some cases. Yes, I figured this out in the other email and posted the scenario Greg was asking about. But I believe the question still stands if we can do kset_put() after a *failed* kset_register(), namely if more is being done than necessary, which is just to free the memory allocated by kobject_set_name(). Regards, Luben
On 2022/10/21 16:36, Greg KH wrote: > On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >> On 2022/10/21 13:37, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>> The previous discussion link: >>>>> https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ >>>> The very first discussion on this was here: >>>> >>>> https://www.spinics.net/lists/dri-devel/msg368077.html >>>> >>>> Please use this link, and not the that one up there you which quoted above, >>>> and whose commit description is taken verbatim from the this link. >>>> >>>>> kset_register() is currently used in some places without calling >>>>> kset_put() in error path, because the callers think it should be >>>>> kset internal thing to do, but the driver core can not know what >>>>> caller doing with that memory at times. The memory could be freed >>>>> both in kset_put() and error path of caller, if it is called in >>>>> kset_register(). >>>> As I explained in the link above, the reason there's >>>> a memory leak is that one cannot call kset_register() without >>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>> in this case, i.e. kset_register() fails with -EINVAL. >>>> >>>> Thus, the most common usage is something like this: >>>> >>>> kobj_set_name(&kset->kobj, format, ...); >>>> kset->kobj.kset = parent_kset; >>>> kset->kobj.ktype = ktype; >>>> res = kset_register(kset); >>>> >>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>> by the common idiom shown above. This needs to be mentioned in >>>> the documentation, at least, in case, in the future this is absolved >>>> in kset_register() redesign, etc. >>> Based on this, can kset_register() just clean up from itself when an >>> error happens? Ideally that would be the case, as the odds of a kset >>> being embedded in a larger structure is probably slim, but we would have >>> to search the tree to make sure. >> I have search the whole tree, the kset used in bus_register() - patch #3, >> kset_create_and_add() - patch #4 >> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >> amdgpu_discovery.c - patch #10 >> is embedded in a larger structure. In these cases, we can not call >> kset_put() in error path in kset_register() > Yes you can as the kobject in the kset should NOT be controling the > lifespan of those larger objects. Read through the code the only leak in this case is the name, so can we free it directly in kset_register(): --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,11 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + kfree_const(k->kobj.name); + k->kobj.name = NULL; return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; } or unset ktype of kobject, then call kset_put(): --- a/lib/kobject.c +++ b/lib/kobject.c @@ -844,8 +844,11 @@ int kset_register(struct kset *k) kset_init(k); err = kobject_add_internal(&k->kobj); - if (err) + if (err) { + k->kobj.ktype = NULL; + kset_put(k); return err; + } kobject_uevent(&k->kobj, KOBJ_ADD); return 0; } > > If it is, please point out the call chain here as I don't think that > should be possible. > > Note all of this is a mess because the kobject name stuff was added much > later, after the driver model had been created and running for a while. > We missed this error path when adding the dynamic kobject name logic, > thank for looking into this. > > If you could test the patch posted with your error injection systems, > that could make this all much simpler to solve. > > thanks, > > greg k-h > .
On 2022/10/21 16:41, Luben Tuikov wrote: > On 2022-10-21 04:24, Luben Tuikov wrote: >> On 2022-10-21 04:18, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 03:55:18AM -0400, Luben Tuikov wrote: >>>> On 2022-10-21 01:37, Greg KH wrote: >>>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>>> The previous discussion link: >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=C%2Bj1THkHpzVGks5eqB%2Fm%2FPAkMRohR7CYvRnOCqUqdcM%3D&reserved=0 >>>>>> The very first discussion on this was here: >>>>>> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7Cd41da3fd6449492d01f808dab33cdb75%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019371236833115%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pSR10abmK8nAMvKSezqWC0SPUBL4qEwtCCizyIKW7Dc%3D&reserved=0 >>>>>> >>>>>> Please use this link, and not the that one up there you which quoted above, >>>>>> and whose commit description is taken verbatim from the this link. >>>>>> >>>>>>> kset_register() is currently used in some places without calling >>>>>>> kset_put() in error path, because the callers think it should be >>>>>>> kset internal thing to do, but the driver core can not know what >>>>>>> caller doing with that memory at times. The memory could be freed >>>>>>> both in kset_put() and error path of caller, if it is called in >>>>>>> kset_register(). >>>>>> As I explained in the link above, the reason there's >>>>>> a memory leak is that one cannot call kset_register() without >>>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>>> >>>>>> Thus, the most common usage is something like this: >>>>>> >>>>>> kobj_set_name(&kset->kobj, format, ...); >>>>>> kset->kobj.kset = parent_kset; >>>>>> kset->kobj.ktype = ktype; >>>>>> res = kset_register(kset); >>>>>> >>>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>>> by the common idiom shown above. This needs to be mentioned in >>>>>> the documentation, at least, in case, in the future this is absolved >>>>>> in kset_register() redesign, etc. >>>>> Based on this, can kset_register() just clean up from itself when an >>>>> error happens? Ideally that would be the case, as the odds of a kset >>>>> being embedded in a larger structure is probably slim, but we would have >>>>> to search the tree to make sure. >>>> Looking at kset_register(), we can add kset_put() in the error path, >>>> when kobject_add_internal(&kset->kobj) fails. >>>> >>>> See the attached patch. It needs to be tested with the same error injection >>>> as Yang has been doing. >>>> >>>> Now, struct kset is being embedded in larger structs--see amdgpu_discovery.c >>>> starting at line 575. If you're on an AMD system, it gets you the tree >>>> structure you'll see when you run "tree /sys/class/drm/card0/device/ip_discovery/". >>>> That shouldn't be a problem though. >>> Yes, that shouldn't be an issue as the kobject embedded in a kset is >>> ONLY for that kset itself, the kset structure should not be controling >>> the lifespan of the object it is embedded in, right? >> Yes, and it doesn't. It only does a kobject_get(parent) and kobject_put(parent). >> So that's fine and natural. >> >> Yang, do you want to try the patch in my previous email in this thread, since you've >> got the error injection set up already? > I spoke too soon. I believe you're onto something, because looking at the idiom: > > kobj_set_name(&kset->kobj, format, ...); > kset->kobj.kset = parent_kset; > kset->kobj.ktype = ktype; > res = kset_register(kset); > > The ktype defines a release method, which frees the larger struct the kset is embedded in. > And this would result in double free, for instance in the amdgpu_discovery.c code, if > kset_put() is called after kset_register() fails, since we kzalloc the larger object > just before and kfree it on error just after. Ideally, we'd only "revert" the actions > done by kobj_set_name(), as there's some error recovery on create_dir() in kobject_add_internal(). > > So, we cannot do this business with the kset_put() on error from kset_register(), after all. > Not sure how this wasn't caught in Yang's testing--the kernel should've complained. I have already tried the change that in your posted patch when I was debugging this issue before sent these fixes, because it may lead double free in some cases, and I had mentioned it in this mail: https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/#m68eade1993859dfc6c3d14d35f988d35a32ef837 Thanks, Yang > > Regards, > Luben > > .
On 2022/10/21 17:08, Luben Tuikov wrote: > On 2022-10-21 04:59, Yang Yingliang wrote: >> On 2022/10/21 16:36, Greg KH wrote: >>> On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >>>> On 2022/10/21 13:37, Greg KH wrote: >>>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>>> The previous discussion link: >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=RcK05cXm1J5%2BtYcLO2SMG7k6sjeymQzdBzMCDJSzfdE%3D&reserved=0 >>>>>> The very first discussion on this was here: >>>>>> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C74aa9b57192b406ef27408dab3429db4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019395979868103%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=sHZ6kfLF8HxrNXV6%2FVjgdH%2BmQM4T3Zv0U%2FAwddT97cE%3D&reserved=0 >>>>>> >>>>>> Please use this link, and not the that one up there you which quoted above, >>>>>> and whose commit description is taken verbatim from the this link. >>>>>> >>>>>>> kset_register() is currently used in some places without calling >>>>>>> kset_put() in error path, because the callers think it should be >>>>>>> kset internal thing to do, but the driver core can not know what >>>>>>> caller doing with that memory at times. The memory could be freed >>>>>>> both in kset_put() and error path of caller, if it is called in >>>>>>> kset_register(). >>>>>> As I explained in the link above, the reason there's >>>>>> a memory leak is that one cannot call kset_register() without >>>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>>> >>>>>> Thus, the most common usage is something like this: >>>>>> >>>>>> kobj_set_name(&kset->kobj, format, ...); >>>>>> kset->kobj.kset = parent_kset; >>>>>> kset->kobj.ktype = ktype; >>>>>> res = kset_register(kset); >>>>>> >>>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>>> by the common idiom shown above. This needs to be mentioned in >>>>>> the documentation, at least, in case, in the future this is absolved >>>>>> in kset_register() redesign, etc. >>>>> Based on this, can kset_register() just clean up from itself when an >>>>> error happens? Ideally that would be the case, as the odds of a kset >>>>> being embedded in a larger structure is probably slim, but we would have >>>>> to search the tree to make sure. >>>> I have search the whole tree, the kset used in bus_register() - patch #3, >>>> kset_create_and_add() - patch #4 >>>> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >>>> amdgpu_discovery.c - patch #10 >>>> is embedded in a larger structure. In these cases, we can not call >>>> kset_put() in error path in kset_register() >>> Yes you can as the kobject in the kset should NOT be controling the >>> lifespan of those larger objects. >>> >>> If it is, please point out the call chain here as I don't think that >>> should be possible. >>> >>> Note all of this is a mess because the kobject name stuff was added much >>> later, after the driver model had been created and running for a while. >>> We missed this error path when adding the dynamic kobject name logic, >>> thank for looking into this. >>> >>> If you could test the patch posted with your error injection systems, >>> that could make this all much simpler to solve. >> The patch posted by Luben will cause double free in some cases. > Yes, I figured this out in the other email and posted the scenario Greg > was asking about. > > But I believe the question still stands if we can do kset_put() > after a *failed* kset_register(), namely if more is being done than > necessary, which is just to free the memory allocated by > kobject_set_name(). The name memory is allocated in kobject_set_name() in caller, and I think caller free the memory that it allocated is reasonable, it's weird that some callers allocate some memory and use function (kset_register) failed, then it free the memory allocated in callers, I think use kset_put()/kfree_const(name) in caller seems more reasonable. Thanks, Yang > > Regards, > Luben > .
On 2022-10-21 05:56, Yang Yingliang wrote: > > On 2022/10/21 17:08, Luben Tuikov wrote: >> On 2022-10-21 04:59, Yang Yingliang wrote: >>> On 2022/10/21 16:36, Greg KH wrote: >>>> On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >>>>> On 2022/10/21 13:37, Greg KH wrote: >>>>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>>>> The previous discussion link: >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C2597f1097c204be54c7c08dab34a8654%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019429914730071%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NNVCtbTxI5uzxxJA9mKvnsy8d3jyudtl1u4CTcm3tsU%3D&reserved=0 >>>>>>> The very first discussion on this was here: >>>>>>> >>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C2597f1097c204be54c7c08dab34a8654%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019429914886316%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ByCQk0qGktyyoNQg8IFj5AGxmaeWOXnbIA4rFnX%2B6%2BA%3D&reserved=0 >>>>>>> >>>>>>> Please use this link, and not the that one up there you which quoted above, >>>>>>> and whose commit description is taken verbatim from the this link. >>>>>>> >>>>>>>> kset_register() is currently used in some places without calling >>>>>>>> kset_put() in error path, because the callers think it should be >>>>>>>> kset internal thing to do, but the driver core can not know what >>>>>>>> caller doing with that memory at times. The memory could be freed >>>>>>>> both in kset_put() and error path of caller, if it is called in >>>>>>>> kset_register(). >>>>>>> As I explained in the link above, the reason there's >>>>>>> a memory leak is that one cannot call kset_register() without >>>>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>>>> >>>>>>> Thus, the most common usage is something like this: >>>>>>> >>>>>>> kobj_set_name(&kset->kobj, format, ...); >>>>>>> kset->kobj.kset = parent_kset; >>>>>>> kset->kobj.ktype = ktype; >>>>>>> res = kset_register(kset); >>>>>>> >>>>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>>>> by the common idiom shown above. This needs to be mentioned in >>>>>>> the documentation, at least, in case, in the future this is absolved >>>>>>> in kset_register() redesign, etc. >>>>>> Based on this, can kset_register() just clean up from itself when an >>>>>> error happens? Ideally that would be the case, as the odds of a kset >>>>>> being embedded in a larger structure is probably slim, but we would have >>>>>> to search the tree to make sure. >>>>> I have search the whole tree, the kset used in bus_register() - patch #3, >>>>> kset_create_and_add() - patch #4 >>>>> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >>>>> amdgpu_discovery.c - patch #10 >>>>> is embedded in a larger structure. In these cases, we can not call >>>>> kset_put() in error path in kset_register() >>>> Yes you can as the kobject in the kset should NOT be controling the >>>> lifespan of those larger objects. >>>> >>>> If it is, please point out the call chain here as I don't think that >>>> should be possible. >>>> >>>> Note all of this is a mess because the kobject name stuff was added much >>>> later, after the driver model had been created and running for a while. >>>> We missed this error path when adding the dynamic kobject name logic, >>>> thank for looking into this. >>>> >>>> If you could test the patch posted with your error injection systems, >>>> that could make this all much simpler to solve. >>> The patch posted by Luben will cause double free in some cases. >> Yes, I figured this out in the other email and posted the scenario Greg >> was asking about. >> >> But I believe the question still stands if we can do kset_put() >> after a *failed* kset_register(), namely if more is being done than >> necessary, which is just to free the memory allocated by >> kobject_set_name(). > The name memory is allocated in kobject_set_name() in caller, and I > think caller > free the memory that it allocated is reasonable, it's weird that some > callers allocate > some memory and use function (kset_register) failed, then it free the > memory allocated > in callers, I think use kset_put()/kfree_const(name) in caller seems > more reasonable. kset_put() would work only in implementations, such as amdgpu_discovery.c, where the ktype.release is defined and it frees the embedding object in which the kset is embedded. Depending on the implementation, you may need to call kfree_const(name). And this is why this needs to be documented in kset_register(), as I noted in the review earlier. Regards, Luben
On 2022-10-21 05:12, Yang Yingliang wrote: > > On 2022/10/21 16:36, Greg KH wrote: >> On Fri, Oct 21, 2022 at 04:24:23PM +0800, Yang Yingliang wrote: >>> On 2022/10/21 13:37, Greg KH wrote: >>>> On Fri, Oct 21, 2022 at 01:29:31AM -0400, Luben Tuikov wrote: >>>>> On 2022-10-20 22:20, Yang Yingliang wrote: >>>>>> The previous discussion link: >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F0db486eb-6927-927e-3629-958f8f211194%40huawei.com%2FT%2F&data=05%7C01%7Cluben.tuikov%40amd.com%7C26ed7dc8053f4793d54d08dab344731e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019403819761348%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PD93EC%2FcBmkfSBbdmK8FNtXhqS%2FKmmcByfkx5lqQfpY%3D&reserved=0 >>>>> The very first discussion on this was here: >>>>> >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdri-devel%2Fmsg368077.html&data=05%7C01%7Cluben.tuikov%40amd.com%7C26ed7dc8053f4793d54d08dab344731e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638019403819761348%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=k0fTSmAPTnLFCe4zN4z%2FY1Z7CvwO4gR2vgj%2FLH%2FSRRk%3D&reserved=0 >>>>> >>>>> Please use this link, and not the that one up there you which quoted above, >>>>> and whose commit description is taken verbatim from the this link. >>>>> >>>>>> kset_register() is currently used in some places without calling >>>>>> kset_put() in error path, because the callers think it should be >>>>>> kset internal thing to do, but the driver core can not know what >>>>>> caller doing with that memory at times. The memory could be freed >>>>>> both in kset_put() and error path of caller, if it is called in >>>>>> kset_register(). >>>>> As I explained in the link above, the reason there's >>>>> a memory leak is that one cannot call kset_register() without >>>>> the kset->kobj.name being set--kobj_add_internal() returns -EINVAL, >>>>> in this case, i.e. kset_register() fails with -EINVAL. >>>>> >>>>> Thus, the most common usage is something like this: >>>>> >>>>> kobj_set_name(&kset->kobj, format, ...); >>>>> kset->kobj.kset = parent_kset; >>>>> kset->kobj.ktype = ktype; >>>>> res = kset_register(kset); >>>>> >>>>> So, what is being leaked, is the memory allocated in kobj_set_name(), >>>>> by the common idiom shown above. This needs to be mentioned in >>>>> the documentation, at least, in case, in the future this is absolved >>>>> in kset_register() redesign, etc. >>>> Based on this, can kset_register() just clean up from itself when an >>>> error happens? Ideally that would be the case, as the odds of a kset >>>> being embedded in a larger structure is probably slim, but we would have >>>> to search the tree to make sure. >>> I have search the whole tree, the kset used in bus_register() - patch #3, >>> kset_create_and_add() - patch #4 >>> __class_register() - patch #5, fw_cfg_build_symlink() - patch #6 and >>> amdgpu_discovery.c - patch #10 >>> is embedded in a larger structure. In these cases, we can not call >>> kset_put() in error path in kset_register() >> Yes you can as the kobject in the kset should NOT be controling the >> lifespan of those larger objects. > Read through the code the only leak in this case is the name, so can we > free it > directly in kset_register(): > > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -844,8 +844,11 @@ int kset_register(struct kset *k) > > kset_init(k); > err = kobject_add_internal(&k->kobj); > - if (err) > + if (err) { > + kfree_const(k->kobj.name); > + k->kobj.name = NULL; > return err; > + } > kobject_uevent(&k->kobj, KOBJ_ADD); > return 0; > } This may work, but absolutely needs to be documented since we don't exactly know how the name was allocated by the caller! FWIW, the caller may have set the name pointer to point to a static array of strings... > or unset ktype of kobject, then call kset_put(): > > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -844,8 +844,11 @@ int kset_register(struct kset *k) > > kset_init(k); > err = kobject_add_internal(&k->kobj); > - if (err) > + if (err) { > + k->kobj.ktype = NULL; > + kset_put(k); > return err; > + } > kobject_uevent(&k->kobj, KOBJ_ADD); > return 0; > } That's a no. You shouldn't set the ktype to NULL--maybe the caller is relying on it... Regards, Luben
The previous discussion link: https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@huawei.com/T/ kset_register() is currently used in some places without calling kset_put() in error path, because the callers think it should be kset internal thing to do, but the driver core can not know what caller doing with that memory at times. The memory could be freed both in kset_put() and error path of caller, if it is called in kset_register(). So make the function documentation more explicit about calling kset_put() in the error path of caller first, so that people have a chance to know what to do here, then fixes this leaks by calling kset_put() from callers. Liu Shixin (1): ubifs: Fix memory leak in ubifs_sysfs_init() Yang Yingliang (10): kset: fix documentation for kset_register() kset: add null pointer check in kset_put() bus: fix possible memory leak in bus_register() kobject: fix possible memory leak in kset_create_and_add() class: fix possible memory leak in __class_register() firmware: qemu_fw_cfg: fix possible memory leak in fw_cfg_build_symlink() f2fs: fix possible memory leak in f2fs_init_sysfs() erofs: fix possible memory leak in erofs_init_sysfs() ocfs2: possible memory leak in mlog_sys_init() drm/amdgpu/discovery: fix possible memory leak drivers/base/bus.c | 4 +++- drivers/base/class.c | 6 ++++++ drivers/firmware/qemu_fw_cfg.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 5 +++-- fs/erofs/sysfs.c | 4 +++- fs/f2fs/sysfs.c | 4 +++- fs/ocfs2/cluster/masklog.c | 7 ++++++- fs/ubifs/sysfs.c | 2 ++ include/linux/kobject.h | 3 ++- lib/kobject.c | 5 ++++- 10 files changed, 33 insertions(+), 9 deletions(-)