Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 21 Jun 2022 14:03:30 +0200
From:   Johan Hovold <johan@kernel.org>
To:     =?utf-8?B?5pm65a6L?= <zhi.song@bytedance.com>
Cc:     gregkh@linuxfoundation.org, rafael@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] node: put_device after failing to device_register
Message-ID: <YrGzkuFtna+blmA6@hovoldconsulting.com>
References: <20220615151738.1766206-1-zhi.song@bytedance.com>
 <YrB8bIJj3fvvsCHg@hovoldconsulting.com>
 <C6760D95-B26F-47AE-A8F6-F80129135A6C@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <C6760D95-B26F-47AE-A8F6-F80129135A6C@bytedance.com>
Precedence: bulk

On Tue, Jun 21, 2022 at 06:17:22PM +0800, 智宋 wrote:
> > On Jun 20, 2022, at 21:55, Johan Hovold <johan@kernel.org> wrote:

> > That's a pretty obvious use-after-free you just added here. You can't
> > access dev after you've just freed it.
> > 
> > The name is freed along with the rest of the struct device so you need
> > to remove the second explicit free. And you should rename the label too.
> > 
> >> free:
> >> 	kfree(access_node);
> > 
> > But here's another use after free... The put_device() call you added
> > will have freed access_node by calling node_access_release().

> dev_set_name() allocates new space to dev->name and assigns the address of space to dev->name if it allocates successfully. But if we fail to allocate space, there isn’t any new space for dev->name. Therefore, there’s no need for calling kfree_const(dev->kobj.name) in dev_set_name()’s error handling.

Note that your mails are being rejected by the mailing list since they
include HTML. Can you see if you can fix your mail client to send as
text instead?

That may fix lack line breaks too (break lines at 72 columns or so).
 
> If we fail to do device_register(dev), we just need to put_device(dev) which will do free access_node, kobj.name and other cleanup.
> 
> Maybe the code is: 
> 
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -144,21 +144,19 @@ static struct node_access_nodes *node_init_node_access(struct node *node,
>         dev->parent = &node->dev;
>         dev->release = node_access_release;
>         dev->groups = node_access_node_groups;
> -       if (dev_set_name(dev, "access%u", access))
> -               goto free;
> +       if (dev_set_name(dev, "access%u", access)) {
> +               kfree(access_node);
> +               return NULL;
> +       }
>  
> -       if (device_register(dev))
> -               goto free_name;
> +       if (device_register(dev)) {
> +               put_device(dev);
> +               return NULL;
> +       }
>  
>         pm_runtime_no_callbacks(dev);
>         list_add_tail(&access_node->list_node, &node->access_list);
>         return access_node;
> -free_name:
> -       put_device(dev);
> -       kfree_const(dev->kobj.name);
> -free:
> -       kfree(access_node);
> -       return NULL;
>  }

The above fix looks correct now.

> Thanks for your patience. :)

You're welcome. Thanks for finding and fixing the bug.

Johan