Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <07c32bf1-6c1a-49d9-b97d-f0ae4a2b42ab@p183>
In-Reply-To: <07c32bf1-6c1a-49d9-b97d-f0ae4a2b42ab@p183>
From:   Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Date:   Fri, 15 Sep 2023 12:09:28 +0200
Message-ID: <CACMJSeu1NYNbQkiZC1BOs4QKhR4+VwYRrpDgq8gczRujt13zEQ@mail.gmail.com>
Subject: Re: Buggy __free(kfree) usage pattern already in tree
To:     Alexey Dobriyan <adobriyan@gmail.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        linux-kernel@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        torvalds@linux-foundation.org, akpm@linux-foundation.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 15 Sept 2023 at 11:56, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> __free() got some usage and some of the usage is buggy:
>
>    832  static struct fwnode_handle *
>    833  gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
>    834                            struct fwnode_handle *parent)
>    835  {
>    838          char **line_names __free(kfree) = NULL;
>                 // returns NULL or ERR_PTR(-E)
>    848          line_names = gpio_sim_make_line_names(bank, &line_names_size);
>    849          if (IS_ERR(line_names))
>    850                  return ERR_CAST(line_names);
>

Thanks for the report, I'll send a fix.

Bart

>
> This pattern will result in calling kfree() on error value.
> And there are no compiler or sparse checking these things.
>
> This test module demonstrates the landmine:
>
> [  812.981089] ------------[ cut here ]------------
> [  812.981597] WARNING: CPU: 0 PID: 1326 at mm/slab_common.c:991 free_large_kmalloc+0x50/0x80
> [  813.013266] ---[ end trace 0000000000000000 ]---
> [  813.013800] object pointer: 0xfffffffffffffff4
>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/cleanup.h>
>
> struct S {
>         int x;
> };
>
> static struct S* f(void)
> {
>         struct S* s = kmalloc(sizeof(struct S), GFP_KERNEL);
>         s = NULL;
>         return s ?: ERR_PTR(-ENOMEM);
> }
>
> static int __init xxx_module_init(void)
> {
>         struct S *s __free(kfree) = NULL;
>         s = f();
>         if (IS_ERR(s)) {
>                 return PTR_ERR(s);
>         }
>         return 0;
> }
>
> static void __exit xxx_module_exit(void)
> {
> }
> module_init(xxx_module_init);
> module_exit(xxx_module_exit);
> MODULE_LICENSE("GPL");