[PATCH 0/4] genirq/irq_sim: fix a use-after-free bug + some

[PATCH 0/4] genirq/irq_sim: fix a use-after-free bug + some

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

The first patch is an issue reported by KASAN. We're accessing freed
memory if we remove the simulator domain with irs still requested. We
must dispose of all existing mappings before we remove the domain.

The following three patches are some improvements to the interrupt
simulator. A simple header cleanup and code shrink using the new
cleanup.h helpers.

Bartosz Golaszewski (4):
  genirq/irq_sim: dispose of remaining mappings before removing the
    domain
  genirq/irq_sim: order includes alphabetically
  bitmap: define a cleanup function for bitmaps
  genirq/irq_sim: shrink code by using cleanup helpers

 include/linux/bitmap.h |  3 +++
 kernel/irq/irq_sim.c   | 38 +++++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

If the device providing simulated interrupts is unbound (real life
example: gpio-sim is disabled with users that didn't free their irqs)
and removes the simulated domain while interrupts are still requested,
we will hit memory issues when they are eventually freed and the
mappings destroyed in the process.

Specifically we'll access freed memory in __irq_domain_deactivate_irq().

Dispose of all mappings before removing the simulator domain.

Fixes: b19af510e67e ("genirq/irq_sim: Add a simple interrupt simulator framework")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index dd76323ea3fd..2c8a9cc1faa6 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/list.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irq_work.h>
@@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
 	unsigned int		irq_count;
 	unsigned long		*pending;
 	struct irq_domain	*domain;
+	struct list_head	irqs;
 };
 
 struct irq_sim_irq_ctx {
 	int			irqnum;
 	bool			enabled;
 	struct irq_sim_work_ctx	*work_ctx;
+	struct list_head	siblings;
 };
 
 static void irq_sim_irqmask(struct irq_data *data)
@@ -129,6 +132,8 @@ static int irq_sim_domain_map(struct irq_domain *domain,
 	irq_set_handler(virq, handle_simple_irq);
 	irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
 	irq_ctx->work_ctx = work_ctx;
+	irq_ctx->irqnum = virq;
+	list_add_tail(&irq_ctx->siblings, &work_ctx->irqs);
 
 	return 0;
 }
@@ -141,6 +146,7 @@ static void irq_sim_domain_unmap(struct irq_domain *domain, unsigned int virq)
 	irqd = irq_domain_get_irq_data(domain, virq);
 	irq_ctx = irq_data_get_irq_chip_data(irqd);
 
+	list_del(&irq_ctx->siblings);
 	irq_set_handler(virq, NULL);
 	irq_domain_reset_irq_data(irqd);
 	kfree(irq_ctx);
@@ -182,6 +188,7 @@ struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
+	INIT_LIST_HEAD(&work_ctx->irqs);
 
 	return work_ctx->domain;
 
@@ -203,8 +210,13 @@ EXPORT_SYMBOL_GPL(irq_domain_create_sim);
 void irq_domain_remove_sim(struct irq_domain *domain)
 {
 	struct irq_sim_work_ctx *work_ctx = domain->host_data;
+	struct irq_sim_irq_ctx *irq_ctx, *aux;
 
 	irq_work_sync(&work_ctx->work);
+
+	list_for_each_entry_safe(irq_ctx, aux, &work_ctx->irqs, siblings)
+		irq_dispose_mapping(irq_ctx->irqnum);
+
 	bitmap_free(work_ctx->pending);
 	kfree(work_ctx);
 
-- 
2.39.2

[PATCH 2/4] genirq/irq_sim: order includes alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

For better maintenance and readability keep the included headers in
alphabetical order.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 2c8a9cc1faa6..a8b013d0c5be 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,11 +4,11 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
-#include <linux/list.h>
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irq_work.h>
-#include <linux/interrupt.h>
+#include <linux/list.h>
 #include <linux/slab.h>
 
 struct irq_sim_work_ctx {
-- 
2.39.2

[PATCH 3/4] bitmap: define a cleanup function for bitmaps

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add support for autopointers for bitmaps allocated with bitmap_alloc()
et al.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/bitmap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 03644237e1ef..6709807ebb59 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -6,6 +6,7 @@
 
 #include <linux/align.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/find.h>
 #include <linux/limits.h>
 #include <linux/string.h>
@@ -125,6 +126,8 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
 unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
 void bitmap_free(const unsigned long *bitmap);
 
+DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))
+
 /* Managed variants of the above. */
 unsigned long *devm_bitmap_alloc(struct device *dev,
 				 unsigned int nbits, gfp_t flags);
-- 
2.39.2

[PATCH 4/4] genirq/irq_sim: shrink code by using cleanup helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the new __free helper from linux/cleanup.h to remove all gotos and
simplify the error paths.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 kernel/irq/irq_sim.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index a8b013d0c5be..202beb1169c9 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
@@ -170,34 +171,29 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
 struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 					 unsigned int num_irqs)
 {
-	struct irq_sim_work_ctx *work_ctx;
+	struct irq_sim_work_ctx *work_ctx __free(kfree) = NULL;
+	unsigned long *pending __free(bitmap) = NULL;
 
 	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
 	if (!work_ctx)
-		goto err_out;
+		return ERR_PTR(-ENOMEM);
 
-	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
-	if (!work_ctx->pending)
-		goto err_free_work_ctx;
+	pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
+	if (!pending)
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
 						    &irq_sim_domain_ops,
 						    work_ctx);
 	if (!work_ctx->domain)
-		goto err_free_bitmap;
+		return ERR_PTR(-ENOMEM);
 
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
 	INIT_LIST_HEAD(&work_ctx->irqs);
+	work_ctx->pending = no_free_ptr(pending);
 
-	return work_ctx->domain;
-
-err_free_bitmap:
-	bitmap_free(work_ctx->pending);
-err_free_work_ctx:
-	kfree(work_ctx);
-err_out:
-	return ERR_PTR(-ENOMEM);
+	return no_free_ptr(work_ctx)->domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_sim);
 
-- 
2.39.2

Re: [PATCH 3/4] bitmap: define a cleanup function for bitmaps

[Yury Norov]

On Sat, Aug 12, 2023 at 09:44:56PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add support for autopointers for bitmaps allocated with bitmap_alloc()
> et al.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  include/linux/bitmap.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 03644237e1ef..6709807ebb59 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -6,6 +6,7 @@
>  
>  #include <linux/align.h>
>  #include <linux/bitops.h>
> +#include <linux/cleanup.h>
>  #include <linux/find.h>
>  #include <linux/limits.h>
>  #include <linux/string.h>
> @@ -125,6 +126,8 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
>  unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
>  void bitmap_free(const unsigned long *bitmap);
>  
> +DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))

bitmap_free() is a wrapper around kfree(), and kfree() checks against
NULL, and returns immediately. Why this tests again?

Re: [PATCH 4/4] genirq/irq_sim: shrink code by using cleanup helpers

[Yury Norov]

On Sat, Aug 12, 2023 at 09:44:57PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use the new __free helper from linux/cleanup.h to remove all gotos and
> simplify the error paths.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  kernel/irq/irq_sim.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index a8b013d0c5be..202beb1169c9 100644
> --- a/kernel/irq/irq_sim.c
> +++ b/kernel/irq/irq_sim.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irq_sim.h>
> @@ -170,34 +171,29 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
>  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
>  					 unsigned int num_irqs)
>  {
> -	struct irq_sim_work_ctx *work_ctx;
> +	struct irq_sim_work_ctx *work_ctx __free(kfree) = NULL;
> +	unsigned long *pending __free(bitmap) = NULL;

Why initializing here as NULL ...
  
>  	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
>  	if (!work_ctx)
> -		goto err_out;
> +		return ERR_PTR(-ENOMEM);
>  
> -	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> -	if (!work_ctx->pending)
> -		goto err_free_work_ctx;
> +	pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> +	if (!pending)
> +		return ERR_PTR(-ENOMEM);

and overriding immediately after that? Not familiar to __free()
machinery in details, although. Does it require initialization?

Thank,
Yury

Re: [PATCH 4/4] genirq/irq_sim: shrink code by using cleanup helpers

[Bartosz Golaszewski]

On Mon, Aug 14, 2023 at 3:09 AM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Sat, Aug 12, 2023 at 09:44:57PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Use the new __free helper from linux/cleanup.h to remove all gotos and
> > simplify the error paths.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  kernel/irq/irq_sim.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > index a8b013d0c5be..202beb1169c9 100644
> > --- a/kernel/irq/irq_sim.c
> > +++ b/kernel/irq/irq_sim.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >   */
> >
> > +#include <linux/cleanup.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> >  #include <linux/irq_sim.h>
> > @@ -170,34 +171,29 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
> >  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
> >                                        unsigned int num_irqs)
> >  {
> > -     struct irq_sim_work_ctx *work_ctx;
> > +     struct irq_sim_work_ctx *work_ctx __free(kfree) = NULL;
> > +     unsigned long *pending __free(bitmap) = NULL;
>
> Why initializing here as NULL ...
>
> >       work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> >       if (!work_ctx)
> > -             goto err_out;
> > +             return ERR_PTR(-ENOMEM);
> >
> > -     work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > -     if (!work_ctx->pending)
> > -             goto err_free_work_ctx;
> > +     pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> > +     if (!pending)
> > +             return ERR_PTR(-ENOMEM);
>
> and overriding immediately after that? Not familiar to __free()
> machinery in details, although. Does it require initialization?
>
> Thank,
> Yury

For the first variable: it's just good practice resulting from years
of user-space coding in GLib which makes heavy use of autopointers.
For the 'pending' variable, it's necessary as if the first kmalloc()
fails, it will go out of scope with random contents and bitmap_free()
will get called on it. Now if we ever add some statement that can
return between the start of the function and the first kmalloc() then
we'll save ourselves having to modify the variable declaration.

Bart

Re: [PATCH 3/4] bitmap: define a cleanup function for bitmaps

[Bartosz Golaszewski]

On Mon, Aug 14, 2023 at 3:02 AM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Sat, Aug 12, 2023 at 09:44:56PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Add support for autopointers for bitmaps allocated with bitmap_alloc()
> > et al.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  include/linux/bitmap.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 03644237e1ef..6709807ebb59 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -6,6 +6,7 @@
> >
> >  #include <linux/align.h>
> >  #include <linux/bitops.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/find.h>
> >  #include <linux/limits.h>
> >  #include <linux/string.h>
> > @@ -125,6 +126,8 @@ unsigned long *bitmap_alloc_node(unsigned int nbits, gfp_t flags, int node);
> >  unsigned long *bitmap_zalloc_node(unsigned int nbits, gfp_t flags, int node);
> >  void bitmap_free(const unsigned long *bitmap);
> >
> > +DEFINE_FREE(bitmap, unsigned long *, if (_T) bitmap_free(_T))
>
> bitmap_free() is a wrapper around kfree(), and kfree() checks against
> NULL, and returns immediately. Why this tests again?

I don't see any kernel docs making it a contract though. :)

It's a good question however. I carried it over from the kfree()'s
DEFINE_FREE() itself. I can't find any discussion about it in the
thread under Peter Zijlstra's patches.

May be worth revisiting it for free() functions that handle NULL.

Bart

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Andy Shevchenko]

On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> If the device providing simulated interrupts is unbound (real life
> example: gpio-sim is disabled with users that didn't free their irqs)
> and removes the simulated domain while interrupts are still requested,
> we will hit memory issues when they are eventually freed and the
> mappings destroyed in the process.
> 
> Specifically we'll access freed memory in __irq_domain_deactivate_irq().
> 
> Dispose of all mappings before removing the simulator domain.

...

> +#include <linux/list.h>

Maybe ordered?

>  #include <linux/irq.h>
>  #include <linux/irq_sim.h>
>  #include <linux/irq_work.h>

...

> @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
>  	unsigned int		irq_count;
>  	unsigned long		*pending;
>  	struct irq_domain	*domain;
> +	struct list_head	irqs;
>  };
>  
>  struct irq_sim_irq_ctx {
>  	int			irqnum;
>  	bool			enabled;
>  	struct irq_sim_work_ctx	*work_ctx;

> +	struct list_head	siblings;

You can reduce the code size by moving this to be the first member.
Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.

>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] genirq/irq_sim: order includes alphabetically

[Andy Shevchenko]

On Sat, Aug 12, 2023 at 09:44:55PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better maintenance and readability keep the included headers in
> alphabetical order.

...

> -#include <linux/list.h>

> +#include <linux/list.h>

This can be done in the previous patch already.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Yury Norov]

On Tue, Aug 15, 2023 at 01:38:49PM +0300, Andy Shevchenko wrote:
> On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > 
> > If the device providing simulated interrupts is unbound (real life
> > example: gpio-sim is disabled with users that didn't free their irqs)
> > and removes the simulated domain while interrupts are still requested,
> > we will hit memory issues when they are eventually freed and the
> > mappings destroyed in the process.
> > 
> > Specifically we'll access freed memory in __irq_domain_deactivate_irq().
> > 
> > Dispose of all mappings before removing the simulator domain.
> 
> ...
> 
> > +#include <linux/list.h>
> 
> Maybe ordered?
> 
> >  #include <linux/irq.h>
> >  #include <linux/irq_sim.h>
> >  #include <linux/irq_work.h>
> 
> ...
> 
> > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
> >  	unsigned int		irq_count;
> >  	unsigned long		*pending;
> >  	struct irq_domain	*domain;
> > +	struct list_head	irqs;
> >  };
> >  
> >  struct irq_sim_irq_ctx {
> >  	int			irqnum;
> >  	bool			enabled;
> >  	struct irq_sim_work_ctx	*work_ctx;
> 
> > +	struct list_head	siblings;
> 
> You can reduce the code size by moving this to be the first member.
> Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.

Pahole you meant?

  yury:linux$ pahole -C irq_sim_irq_ctx /sys/kernel/btf/vmlinux
  struct irq_sim_irq_ctx {
  	int                        irqnum;               /*     0     4 */
  	bool                       enabled;              /*     4     1 */
  
  	/* XXX 3 bytes hole, try to pack */
  
  	struct irq_sim_work_ctx *  work_ctx;             /*     8     8 */
  
  	/* size: 16, cachelines: 1, members: 3 */
  	/* sum members: 13, holes: 1, sum holes: 3 */
  	/* last cacheline: 16 bytes */
  };

In this particular case, there will be no hole because list head
position (16) will be aligned to sizeof(struct list_head) == 16.

But as Bartosz said in the other email, "it's just good practice
resulting from years of" kernel coding to have:
 - members declared strongly according to the logic of the code, and
   if no strong preference: 
 - list head be the first element of the structure, to let compiler
   avoid generating offsets when traversing lists;
 - put elements of greater size at the beginning, so no holes will be
   emitted like in the example above.

So I'd suggest:

  struct irq_sim_irq_ctx {
     struct list_head        siblings;
     struct irq_sim_work_ctx *work_ctx;
     int                     irqnum;
     bool                    enabled;
  }
  
Again, if there's NO ANY reason to have the irq number at the
beginning.

While here, I wonder, why irqnum is signed? Looking at the very first
random function in kernel/irq/irq_sim.c, I see that it's initialized
from a function returning unsigned value:

  static void irq_sim_handle_irq(struct irq_work *work)
  {
          struct irq_sim_work_ctx *work_ctx;
          unsigned int offset = 0;
          int irqnum;
  
          work_ctx = container_of(work, struct irq_sim_work_ctx, work);
  
          while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) {
                  offset = find_next_bit(work_ctx->pending,
                                         work_ctx->irq_count, offset);
                  clear_bit(offset, work_ctx->pending);
                  irqnum = irq_find_mapping(work_ctx->domain, offset);
                  handle_simple_irq(irq_to_desc(irqnum));
          }
  }

Thanks,
Yury

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 09:09:10AM -0700, Yury Norov wrote:
> On Tue, Aug 15, 2023 at 01:38:49PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:

...

> > > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
> > >  	unsigned int		irq_count;
> > >  	unsigned long		*pending;
> > >  	struct irq_domain	*domain;
> > > +	struct list_head	irqs;
> > >  };
> > >  
> > >  struct irq_sim_irq_ctx {
> > >  	int			irqnum;
> > >  	bool			enabled;
> > >  	struct irq_sim_work_ctx	*work_ctx;
> > 
> > > +	struct list_head	siblings;
> > 
> > You can reduce the code size by moving this to be the first member.
> > Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.
> 
> Pahole you meant?

No. I meant bloat-o-meter.

...

> But as Bartosz said in the other email, "it's just good practice
> resulting from years of" kernel coding to have:
>  - members declared strongly according to the logic of the code, and
>    if no strong preference: 

>  - list head be the first element of the structure, to let compiler
>    avoid generating offsets when traversing lists;

Exactly.

>  - put elements of greater size at the beginning, so no holes will be
>    emitted like in the example above.
> 
> So I'd suggest:
> 
>   struct irq_sim_irq_ctx {
>      struct list_head        siblings;
>      struct irq_sim_work_ctx *work_ctx;
>      int                     irqnum;
>      bool                    enabled;
>   }

Yes, I like this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Bartosz Golaszewski]

On Tue, Aug 15, 2023 at 12:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > If the device providing simulated interrupts is unbound (real life
> > example: gpio-sim is disabled with users that didn't free their irqs)
> > and removes the simulated domain while interrupts are still requested,
> > we will hit memory issues when they are eventually freed and the
> > mappings destroyed in the process.
> >
> > Specifically we'll access freed memory in __irq_domain_deactivate_irq().
> >
> > Dispose of all mappings before removing the simulator domain.
>
> ...
>
> > +#include <linux/list.h>
>
> Maybe ordered?
>

This patch comes first in the series as it's meant to be a
backportable fix. Header ordering comes later.

Bart

> >  #include <linux/irq.h>
> >  #include <linux/irq_sim.h>
> >  #include <linux/irq_work.h>
>
> ...
>
> > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
> >       unsigned int            irq_count;
> >       unsigned long           *pending;
> >       struct irq_domain       *domain;
> > +     struct list_head        irqs;
> >  };
> >
> >  struct irq_sim_irq_ctx {
> >       int                     irqnum;
> >       bool                    enabled;
> >       struct irq_sim_work_ctx *work_ctx;
>
> > +     struct list_head        siblings;
>
> You can reduce the code size by moving this to be the first member.
> Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.
>
> >  };
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Bartosz Golaszewski]

On Tue, Aug 15, 2023 at 6:09 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Tue, Aug 15, 2023 at 01:38:49PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > If the device providing simulated interrupts is unbound (real life
> > > example: gpio-sim is disabled with users that didn't free their irqs)
> > > and removes the simulated domain while interrupts are still requested,
> > > we will hit memory issues when they are eventually freed and the
> > > mappings destroyed in the process.
> > >
> > > Specifically we'll access freed memory in __irq_domain_deactivate_irq().
> > >
> > > Dispose of all mappings before removing the simulator domain.
> >
> > ...
> >
> > > +#include <linux/list.h>
> >
> > Maybe ordered?
> >
> > >  #include <linux/irq.h>
> > >  #include <linux/irq_sim.h>
> > >  #include <linux/irq_work.h>
> >
> > ...
> >
> > > @@ -16,12 +17,14 @@ struct irq_sim_work_ctx {
> > >     unsigned int            irq_count;
> > >     unsigned long           *pending;
> > >     struct irq_domain       *domain;
> > > +   struct list_head        irqs;
> > >  };
> > >
> > >  struct irq_sim_irq_ctx {
> > >     int                     irqnum;
> > >     bool                    enabled;
> > >     struct irq_sim_work_ctx *work_ctx;
> >
> > > +   struct list_head        siblings;
> >
> > You can reduce the code size by moving this to be the first member.
> > Not sure about struct irq_sim_work_ctx, you can play with bloat-o-meter.
>
> Pahole you meant?
>
>   yury:linux$ pahole -C irq_sim_irq_ctx /sys/kernel/btf/vmlinux
>   struct irq_sim_irq_ctx {
>         int                        irqnum;               /*     0     4 */
>         bool                       enabled;              /*     4     1 */
>
>         /* XXX 3 bytes hole, try to pack */
>
>         struct irq_sim_work_ctx *  work_ctx;             /*     8     8 */
>
>         /* size: 16, cachelines: 1, members: 3 */
>         /* sum members: 13, holes: 1, sum holes: 3 */
>         /* last cacheline: 16 bytes */
>   };
>
> In this particular case, there will be no hole because list head
> position (16) will be aligned to sizeof(struct list_head) == 16.
>
> But as Bartosz said in the other email, "it's just good practice
> resulting from years of" kernel coding to have:

Did I sound smug or what? I didn't mean to. I was merely pointing to
the fact that linux is not the first to use C autopointers. They've
been around for years.

>  - members declared strongly according to the logic of the code, and
>    if no strong preference:
>  - list head be the first element of the structure, to let compiler
>    avoid generating offsets when traversing lists;
>  - put elements of greater size at the beginning, so no holes will be
>    emitted like in the example above.
>
> So I'd suggest:
>
>   struct irq_sim_irq_ctx {
>      struct list_head        siblings;
>      struct irq_sim_work_ctx *work_ctx;
>      int                     irqnum;
>      bool                    enabled;
>   }

Sounds good.

>
> Again, if there's NO ANY reason to have the irq number at the
> beginning.
>
> While here, I wonder, why irqnum is signed? Looking at the very first
> random function in kernel/irq/irq_sim.c, I see that it's initialized
> from a function returning unsigned value:
>

This field is currently unused. I'm not sure how it ended up there,
maybe a leftover from some earlier iterations of the irq_sim. This
patch just makes use of it in the end. It may be that it should use
unsigned int. Before I change it, I'd like to hear Thomas' comments on
these changes in general.

Bart

>   static void irq_sim_handle_irq(struct irq_work *work)
>   {
>           struct irq_sim_work_ctx *work_ctx;
>           unsigned int offset = 0;
>           int irqnum;
>
>           work_ctx = container_of(work, struct irq_sim_work_ctx, work);
>
>           while (!bitmap_empty(work_ctx->pending, work_ctx->irq_count)) {
>                   offset = find_next_bit(work_ctx->pending,
>                                          work_ctx->irq_count, offset);
>                   clear_bit(offset, work_ctx->pending);
>                   irqnum = irq_find_mapping(work_ctx->domain, offset);
>                   handle_simple_irq(irq_to_desc(irqnum));
>           }
>   }
>
> Thanks,
> Yury

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 08:38:41PM +0200, Bartosz Golaszewski wrote:
> On Tue, Aug 15, 2023 at 12:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Aug 12, 2023 at 09:44:54PM +0200, Bartosz Golaszewski wrote:

...

> > > +#include <linux/list.h>
> >
> > Maybe ordered?
> >
> 
> This patch comes first in the series as it's meant to be a
> backportable fix. Header ordering comes later.

It does not prevent you from putting it in the better place.
So the followup will be a less churn.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] genirq/irq_sim: dispose of remaining mappings before removing the domain

[Bartosz Golaszewski]

On Sat, Aug 12, 2023 at 9:45 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> If the device providing simulated interrupts is unbound (real life
> example: gpio-sim is disabled with users that didn't free their irqs)
> and removes the simulated domain while interrupts are still requested,
> we will hit memory issues when they are eventually freed and the
> mappings destroyed in the process.
>
> Specifically we'll access freed memory in __irq_domain_deactivate_irq().
>
> Dispose of all mappings before removing the simulator domain.
>
> Fixes: b19af510e67e ("genirq/irq_sim: Add a simple interrupt simulator framework")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Please disregard this patch. I realized that it should be up to the
owner of the domain to dispose of mappings. Just like core GPIO does
for the domains it controls.

I will send the rest separately once they're reworked.

Bart