[PATCH 0/4] genirq/irq_sim: misc updates

[PATCH 0/4] genirq/irq_sim: misc updates

[Bartosz Golaszewski]

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

Here are a couple of updates to the interrupt simulator. Two are minor:
remove an unused field and reorder includes for readability. The third
one simplifies the error paths by using new cleanup macros. To that end
we also add a cleanup definition for dynamic bitmaps.

Bartosz Golaszewski (4):
  bitmap: define a cleanup function for bitmaps
  genirq/irq_sim: remove unused field from struct irq_sim_irq_ctx
  genirq/irq_sim: order headers alphabetically
  genirq/irq_sim: shrink code by using cleanup helpers

 include/linux/bitmap.h |  3 +++
 kernel/irq/irq_sim.c   | 27 +++++++++++----------------
 2 files changed, 14 insertions(+), 16 deletions(-)

-- 
2.39.2

[PATCH 1/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..ba8c0d733842 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 *, 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

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

[Andy Shevchenko]

On Tue, Sep 12, 2023 at 10:55:38AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add support for autopointers for bitmaps allocated with bitmap_alloc()
> et al.

Makes sense,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Yury Norov]

On Tue, Sep 12, 2023 at 10:55:38AM +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..ba8c0d733842 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 *, bitmap_free(_T))

So now it doesn't do weird  "if (_T) bitmap_free(_T)". Have you got any
feedback from Peter for that?

My point is that if the above is correct, all the following
declarations need to be revisited:

yury:linux$ git grep DEFINE_FREE|grep if
include/linux/cleanup.h:25: * DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
include/linux/device.h:1058:DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T))
include/linux/device.h:1228:DEFINE_FREE(put_device, struct device *, if (_T) put_device(_T))
include/linux/mutex.h:224:DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T))
include/linux/rwsem.h:208:DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
include/linux/rwsem.h:209:DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
include/linux/sched/task.h:164:DEFINE_FREE(put_task, struct task_struct *, if (_T) put_task_struct(_T))
include/linux/slab.h:231:DEFINE_FREE(kfree, void *, if (_T) kfree(_T))

For the patch:
Acked-by: Yury Norov <yury.norov@gmail.com>

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

[Bartosz Golaszewski]

On Tue, Sep 12, 2023 at 3:43 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 10:55:38AM +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..ba8c0d733842 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 *, bitmap_free(_T))
>
> So now it doesn't do weird  "if (_T) bitmap_free(_T)". Have you got any
> feedback from Peter for that?
>

I did get feedback from Peter. Unfortunately it was right *after* I
sent this. Turns out the NULL-pointer check is there for a reason,
please see the comment in this patch:
https://lore.kernel.org/all/169451499208.27769.5856056754166699857.tip-bot2@tip-bot2/T/#u

That means I will have to resend this one.

Bart

> My point is that if the above is correct, all the following
> declarations need to be revisited:
>
> yury:linux$ git grep DEFINE_FREE|grep if
> include/linux/cleanup.h:25: * DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
> include/linux/device.h:1058:DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T))
> include/linux/device.h:1228:DEFINE_FREE(put_device, struct device *, if (_T) put_device(_T))
> include/linux/mutex.h:224:DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T))
> include/linux/rwsem.h:208:DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
> include/linux/rwsem.h:209:DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
> include/linux/sched/task.h:164:DEFINE_FREE(put_task, struct task_struct *, if (_T) put_task_struct(_T))
> include/linux/slab.h:231:DEFINE_FREE(kfree, void *, if (_T) kfree(_T))
>
> For the patch:
> Acked-by: Yury Norov <yury.norov@gmail.com>

[PATCH 2/4] genirq/irq_sim: remove unused field from struct irq_sim_irq_ctx

[Bartosz Golaszewski]

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

The irqnum field is unused. Remove it.

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

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index dd76323ea3fd..f5ebb3ba6f9a 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -19,7 +19,6 @@ struct irq_sim_work_ctx {
 };
 
 struct irq_sim_irq_ctx {
-	int			irqnum;
 	bool			enabled;
 	struct irq_sim_work_ctx	*work_ctx;
 };
-- 
2.39.2

[PATCH 3/4] genirq/irq_sim: order headers alphabetically

[Bartosz Golaszewski]

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

For better readability and maintenance keep headers in alphabetical
order.

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

diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index f5ebb3ba6f9a..b0d50b48dbd1 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -4,10 +4,10 @@
  * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irq_work.h>
-#include <linux/interrupt.h>
 #include <linux/slab.h>
 
 struct irq_sim_work_ctx {
-- 
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 b0d50b48dbd1..24d10267f582 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>
@@ -163,33 +164,28 @@ 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);
+	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 0/4] genirq/irq_sim: misc updates

[Yury Norov]

On Tue, Sep 12, 2023 at 10:55:37AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Here are a couple of updates to the interrupt simulator. Two are minor:
> remove an unused field and reorder includes for readability. The third
> one simplifies the error paths by using new cleanup macros. To that end
> we also add a cleanup definition for dynamic bitmaps.

This looks like a v2 for https://lore.kernel.org/lkml/20230812194457.6432-1-brgl@bgdev.pl/
Is it right? If so, can you keep history of submissions in the cover
email please?

Re: [PATCH 0/4] genirq/irq_sim: misc updates

[Bartosz Golaszewski]

On Tue, Sep 12, 2023 at 3:36 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 10:55:37AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Here are a couple of updates to the interrupt simulator. Two are minor:
> > remove an unused field and reorder includes for readability. The third
> > one simplifies the error paths by using new cleanup macros. To that end
> > we also add a cleanup definition for dynamic bitmaps.
>
> This looks like a v2 for https://lore.kernel.org/lkml/20230812194457.6432-1-brgl@bgdev.pl/
> Is it right? If so, can you keep history of submissions in the cover
> email please?

That series was mainly about fixing a resource leak. While it
contained a part of the changes that ended up here, I consider this
one a stand-alone enough to not make it a v2. Probably should have
mentioned that one though.

Bart

[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 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 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