The aspeed irqchip driver overwrites the default irq domain name,
but doesn't free the existing domain name.
This patch frees the irq domain name before overwriting it.
kmemleak trace:
unreferenced object 0xb8004c40 (size 64):
comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
hex dump (first 32 bytes):
3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
backtrace:
[<086b59b8>] kmemleak_alloc+0xa8/0xc0
[<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
[<f59c7ced>] kvasprintf+0x5c/0xc0
[<49275eec>] kasprintf+0x30/0x50
[<5713064b>] __irq_domain_add+0x184/0x25c
[<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
[<d8d7017e>] of_irq_init+0x1ec/0x314
[<f8405bf1>] irqchip_init+0x1c/0x24
[<7ef974b3>] init_IRQ+0x30/0x90
[<87a1438f>] start_kernel+0x28c/0x458
[< (null)>] (null)
[<f0763fdf>] 0xffffffff
Signed-off-by: Roy van Doormaal <[email protected]>
---
drivers/irqchip/irq-aspeed-i2c-ic.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
index 8d591c179f81..8081b8483a79 100644
--- a/drivers/irqchip/irq-aspeed-i2c-ic.c
+++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
@@ -92,6 +92,8 @@ static int __init aspeed_i2c_ic_of_init(struct device_node *node,
goto err_iounmap;
}
+ if (i2c_ic->irq_domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
+ kfree(i2c_ic->irq_domain->name);
i2c_ic->irq_domain->name = "aspeed-i2c-domain";
irq_set_chained_handler_and_data(i2c_ic->parent_irq,
--
2.20.1
On Tue, 26 Nov 2019, at 06:59, Roy van Doormaal wrote:
> The aspeed irqchip driver overwrites the default irq domain name,
> but doesn't free the existing domain name.
> This patch frees the irq domain name before overwriting it.
>
> kmemleak trace:
>
> unreferenced object 0xb8004c40 (size 64):
> comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
> hex dump (first 32 bytes):
> 3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
> 38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
> backtrace:
> [<086b59b8>] kmemleak_alloc+0xa8/0xc0
> [<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
> [<f59c7ced>] kvasprintf+0x5c/0xc0
> [<49275eec>] kasprintf+0x30/0x50
> [<5713064b>] __irq_domain_add+0x184/0x25c
> [<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
> [<d8d7017e>] of_irq_init+0x1ec/0x314
> [<f8405bf1>] irqchip_init+0x1c/0x24
> [<7ef974b3>] init_IRQ+0x30/0x90
> [<87a1438f>] start_kernel+0x28c/0x458
> [< (null)>] (null)
> [<f0763fdf>] 0xffffffff
>
> Signed-off-by: Roy van Doormaal <[email protected]>
> ---
> drivers/irqchip/irq-aspeed-i2c-ic.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c
> b/drivers/irqchip/irq-aspeed-i2c-ic.c
> index 8d591c179f81..8081b8483a79 100644
> --- a/drivers/irqchip/irq-aspeed-i2c-ic.c
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -92,6 +92,8 @@ static int __init aspeed_i2c_ic_of_init(struct
> device_node *node,
> goto err_iounmap;
> }
>
> + if (i2c_ic->irq_domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
> + kfree(i2c_ic->irq_domain->name);
> i2c_ic->irq_domain->name = "aspeed-i2c-domain";
Given that the name is no-longer allocated I think you need to clear the
IRQ_DOMAIN_NAME_ALLOCATED bit from flags to avoid attempting to
free the const string in irq_domain_remove():
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/irqdomain.c?h=v5.4#n263
Or do a kstrdup().
Andrew
On Tue, 26 Nov 2019 10:08:36 +1030
"Andrew Jeffery" <[email protected]> wrote:
> On Tue, 26 Nov 2019, at 06:59, Roy van Doormaal wrote:
> > The aspeed irqchip driver overwrites the default irq domain name,
> > but doesn't free the existing domain name.
> > This patch frees the irq domain name before overwriting it.
> >
> > kmemleak trace:
> >
> > unreferenced object 0xb8004c40 (size 64):
> > comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
> > hex dump (first 32 bytes):
> > 3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
> > 38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
> > backtrace:
> > [<086b59b8>] kmemleak_alloc+0xa8/0xc0
> > [<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
> > [<f59c7ced>] kvasprintf+0x5c/0xc0
> > [<49275eec>] kasprintf+0x30/0x50
> > [<5713064b>] __irq_domain_add+0x184/0x25c
> > [<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
> > [<d8d7017e>] of_irq_init+0x1ec/0x314
> > [<f8405bf1>] irqchip_init+0x1c/0x24
> > [<7ef974b3>] init_IRQ+0x30/0x90
> > [<87a1438f>] start_kernel+0x28c/0x458
> > [< (null)>] (null)
> > [<f0763fdf>] 0xffffffff
> >
> > Signed-off-by: Roy van Doormaal <[email protected]>
> > ---
> > drivers/irqchip/irq-aspeed-i2c-ic.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c
> > b/drivers/irqchip/irq-aspeed-i2c-ic.c
> > index 8d591c179f81..8081b8483a79 100644
> > --- a/drivers/irqchip/irq-aspeed-i2c-ic.c
> > +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> > @@ -92,6 +92,8 @@ static int __init aspeed_i2c_ic_of_init(struct
> > device_node *node,
> > goto err_iounmap;
> > }
> >
> > + if (i2c_ic->irq_domain->flags & IRQ_DOMAIN_NAME_ALLOCATED)
> > + kfree(i2c_ic->irq_domain->name);
> > i2c_ic->irq_domain->name = "aspeed-i2c-domain";
>
> Given that the name is no-longer allocated I think you need to clear the
> IRQ_DOMAIN_NAME_ALLOCATED bit from flags to avoid attempting to
> free the const string in irq_domain_remove():
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/irqdomain.c?h=v5.4#n263
>
> Or do a kstrdup().
Or even better, drop the whole domain name assignment, which is pretty
pointless and makes debugging pointlessly difficult (see how the name
is used to build the irq debugfs).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
The aspeed irqchip driver overwrites the default irq domain name,
but doesn't free the existing domain name.
This patch frees the irq domain name before overwriting it.
kmemleak trace:
unreferenced object 0xb8004c40 (size 64):
comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
hex dump (first 32 bytes):
3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
backtrace:
[<086b59b8>] kmemleak_alloc+0xa8/0xc0
[<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
[<f59c7ced>] kvasprintf+0x5c/0xc0
[<49275eec>] kasprintf+0x30/0x50
[<5713064b>] __irq_domain_add+0x184/0x25c
[<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
[<d8d7017e>] of_irq_init+0x1ec/0x314
[<f8405bf1>] irqchip_init+0x1c/0x24
[<7ef974b3>] init_IRQ+0x30/0x90
[<87a1438f>] start_kernel+0x28c/0x458
[< (null)>] (null)
[<f0763fdf>] 0xffffffff
Signed-off-by: Roy van Doormaal <[email protected]>
---
Changes in v2:
- drop irq domain name assignment by the aspeed irqchip driver
---
drivers/irqchip/irq-aspeed-i2c-ic.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
index 8d591c179f81..0bd46f63a3c3 100644
--- a/drivers/irqchip/irq-aspeed-i2c-ic.c
+++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
@@ -92,8 +92,6 @@ static int __init aspeed_i2c_ic_of_init(struct device_node *node,
goto err_iounmap;
}
- i2c_ic->irq_domain->name = "aspeed-i2c-domain";
-
irq_set_chained_handler_and_data(i2c_ic->parent_irq,
aspeed_i2c_ic_irq_handler, i2c_ic);
--
2.20.1
On 2019-11-26 07:40, Roy van Doormaal wrote:
> The aspeed irqchip driver overwrites the default irq domain name,
> but doesn't free the existing domain name.
> This patch frees the irq domain name before overwriting it.
>
> kmemleak trace:
>
> unreferenced object 0xb8004c40 (size 64):
> comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
> hex dump (first 32 bytes):
> 3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
> 38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
> backtrace:
> [<086b59b8>] kmemleak_alloc+0xa8/0xc0
> [<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
> [<f59c7ced>] kvasprintf+0x5c/0xc0
> [<49275eec>] kasprintf+0x30/0x50
> [<5713064b>] __irq_domain_add+0x184/0x25c
> [<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
> [<d8d7017e>] of_irq_init+0x1ec/0x314
> [<f8405bf1>] irqchip_init+0x1c/0x24
> [<7ef974b3>] init_IRQ+0x30/0x90
> [<87a1438f>] start_kernel+0x28c/0x458
> [< (null)>] (null)
> [<f0763fdf>] 0xffffffff
>
> Signed-off-by: Roy van Doormaal
> <[email protected]>
> ---
> Changes in v2:
> - drop irq domain name assignment by the aspeed irqchip driver
Which is good, except that the commit message doesn't quite reflect
what this patch does anymore.
I'll fix it when picking it for -rc1.
Thanks,
M.
> ---
> drivers/irqchip/irq-aspeed-i2c-ic.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c
> b/drivers/irqchip/irq-aspeed-i2c-ic.c
> index 8d591c179f81..0bd46f63a3c3 100644
> --- a/drivers/irqchip/irq-aspeed-i2c-ic.c
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -92,8 +92,6 @@ static int __init aspeed_i2c_ic_of_init(struct
> device_node *node,
> goto err_iounmap;
> }
>
> - i2c_ic->irq_domain->name = "aspeed-i2c-domain";
> -
> irq_set_chained_handler_and_data(i2c_ic->parent_irq,
> aspeed_i2c_ic_irq_handler, i2c_ic);
--
Jazz is not dead. It just smells funny...
On Mon, Nov 25, 2019 at 11:41 PM Roy van Doormaal
<[email protected]> wrote:
>
> The aspeed irqchip driver overwrites the default irq domain name,
> but doesn't free the existing domain name.
> This patch frees the irq domain name before overwriting it.
>
> kmemleak trace:
>
> unreferenced object 0xb8004c40 (size 64):
> comm "swapper", pid 0, jiffies 4294937303 (age 747.660s)
> hex dump (first 32 bytes):
> 3a 61 68 62 3a 61 70 62 3a 62 75 73 40 31 65 37 :ahb:apb:bus@1e7
> 38 61 30 30 30 3a 69 6e 74 65 72 72 75 70 74 2d 8a000:interrupt-
> backtrace:
> [<086b59b8>] kmemleak_alloc+0xa8/0xc0
> [<b5a3490c>] __kmalloc_track_caller+0x118/0x1a0
> [<f59c7ced>] kvasprintf+0x5c/0xc0
> [<49275eec>] kasprintf+0x30/0x50
> [<5713064b>] __irq_domain_add+0x184/0x25c
> [<53c594d0>] aspeed_i2c_ic_of_init+0x9c/0x128
> [<d8d7017e>] of_irq_init+0x1ec/0x314
> [<f8405bf1>] irqchip_init+0x1c/0x24
> [<7ef974b3>] init_IRQ+0x30/0x90
> [<87a1438f>] start_kernel+0x28c/0x458
> [< (null)>] (null)
> [<f0763fdf>] 0xffffffff
>
> Signed-off-by: Roy van Doormaal <[email protected]>
Acked-by: Brendan Higgins <[email protected]>
Sorry for the delayed response.