Re: [PATCH 00/11] fix memory leak while kset_register() fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx/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@xxxxxxxxxx/T/
https://lore.kernel.org/lkml/20221017031335.1845383-1-yangyingliang@xxxxxxxxxx/
https://lore.kernel.org/lkml/Y0zfPKAgQSrYZg5o@xxxxxxxxx/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@xxxxxxxxxx/msg915553.html
https://lore.kernel.org/linux-f2fs-devel/7908686b-9a7c-b754-d312-d689fc28366e@xxxxxxxxxx/T/#t
https://lore.kernel.org/linux-erofs/20221018073947.693206-1-yangyingliang@xxxxxxxxxx/
https://lore.kernel.org/lkml/0db486eb-6927-927e-3629-958f8f211194@xxxxxxxxxx/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(-)

.



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux