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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2023 08:58:51 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 4/4] genirq/irq_sim: shrink code by using cleanup helpers

On Mon, Aug 14, 2023 at 3:09 AM Yury Norov <yury.norov@...il.com> wrote:
>
> On Sat, Aug 12, 2023 at 09:44:57PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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@...aro.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@...libre.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