On Mon, Mar 05, 2018 at 03:45:55PM +0800, Jeffy Chen wrote:

The platform_device_register_full() will allocate dma_mask for hdmi->audio, so we should free before platform_device_unregister().

Reported by kmemleak: unreferenced object 0xffffffc0ef70ff00 (size 128): comm "kworker/4:1", pid 123, jiffies 4294670080 (age 189.604s) hex dump (first 32 bytes): ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<0000000021946f44>] kmemleak_alloc+0x58/0x8c [<000000009c43890d>] kmem_cache_alloc_memcg_trace+0x18c/0x25c [<000000000e17cd06>] platform_device_register_full+0x64/0x108 [<00000000418a0882>] __dw_hdmi_probe+0xb9c/0xcc0 [<00000000e0b720fd>] dw_hdmi_bind+0x30/0x88 [<000000009af347f6>] dw_hdmi_rockchip_bind+0x260/0x2e8

Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com

drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index f9802399cc0d..d9afdc59d4f4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2567,8 +2567,10 @@ __dw_hdmi_probe(struct platform_device *pdev,

static void __dw_hdmi_remove(struct dw_hdmi *hdmi) {

• if (hdmi->audio && !IS_ERR(hdmi->audio))

• if (hdmi->audio && !IS_ERR(hdmi->audio)) {

• kfree(hdmi->audio->dev.dma_mask);
  

  platform_device_unregister(hdmi->audio);
• } if (!IS_ERR(hdmi->cec)) platform_device_unregister(hdmi->cec);

NAK. This is a hack, plain and simple.

The audio device is created by platform_device_register_full(), and the lifetime of that is managed by the driver model. When it's time to clean that up, it's handled by the platform support code in drivers/base/platform.c. It is not the driver's responsibility to clean this up.

There's two issues here: - Since you're kfree()ing it _before_ unregistering the device, the memory you're freeing may still be accessed by the DMA API on behalf of the device driver, so you're introducing a use-after-free bug. - kfree()ing it _after_ unregistering the device means that you're then potentially accessing memory that has been kfree()'d (the underlying struct device) unless you save a pointer to it before unregistering and kfree() it after.

However, see the comment in platform_device_register_full() - if this gets fixed at the core platform level (since every driver using platform_device_register_full() with a DMA mask will suffer this same problem, that's the place to fix it) adding a workaround at driver level will introduce a non-obvious double-free bug.

I suspect the best solution to this is to arrange for struct platform_device to also contain a DMA mask which platform_device_register_full() can use, which means the DMA mask will be freed automatically along with the rest of the platform device, rather than separately allocating this. There may be some resistance to adding 64 bits to platform devices, but if we have lots of these separately kmalloc'd DMA masks, we're actually wasting a lot more memory not doing this (since kmalloc has a minimum size of the L1 cacheline size.)