Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Fri, 8 Dec 2023 11:43:25 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     James Tai =?utf-8?B?W+aItOW/l+WzsF0=?= <james.tai@realtek.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        kernel test robot <lkp@intel.com>,
        Dan Carpenter <error27@gmail.com>
Subject: Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for
 Realtek DHC SoCs
Message-ID: <f154673d-f577-406e-adb0-c567b604e7f4@suswa.mountain>
References: <20231129054339.3054202-1-james.tai@realtek.com>
 <20231129054339.3054202-3-james.tai@realtek.com>
 <d94c79bf-04c4-4e87-bd7e-a8755508ac89@suswa.mountain>
 <8f87bfca-ab5f-4b32-a400-a90d09b64cf1@suswa.mountain>
 <c558b1de9a8841e498f6dfc406a43158@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c558b1de9a8841e498f6dfc406a43158@realtek.com>
Precedence: bulk

On Fri, Dec 08, 2023 at 08:21:10AM +0000, James Tai [戴志峰] wrote:
> Hi Dan,
> 
> >> devm_ allocations are cleaned up automatically so there is no need to
> >> call devm_kfree() before returning.
> >>
> >> regards,
> >> dan carpenter
> >
> I will remove it. 
> 
> >> > +   }
> >> > +
> >> > +   data->info = info;
> >> > +
> >> > +   raw_spin_lock_init(&data->lock);
> >> > +
> >> > +   data->domain = irq_domain_add_linear(node, 32,
> >> > + &realtek_intc_domain_ops, data);
> >
> >Btw, as I was testing the other static checker warning for <= 0, my static
> >checker really wants this irq_domain_add_linear() to be cleaned up on the error
> >path.
> >
> >Otherwise it probably leads to a use after free because we free data
> >(automatically or manually) but it's still on a list somewhere.
> >
> I will add 'irq_domain_remove()' to release it. 
> 
> >> > +   if (!data->domain) {
> >> > +           ret = -ENOMEM;
> >> > +           goto out_cleanup;
> >> > +   }
> >> > +
> >> > +   data->subset_data_num = info->cfg_num;
> >> > +   for (i = 0; i < info->cfg_num; i++) {
> >> > +           ret = realtek_intc_subset(node, data, i);
> >> > +           if (ret) {
> >> > +                   WARN(ret, "failed to init subset %d: %d", i, ret);
> >> > +                   ret = -ENOMEM;
> >> > +                   goto out_cleanup;
> >
> >This error path.
> >
> >regards,
> >dan carpenter
> >
> I will add 'irq_domain_remove()' before goto cleanup.
> 
> 	for (i = 0; i < info->cfg_num; i++) {
> 		ret = realtek_intc_subset(node, data, i);
> 		if (ret) {
> 			WARN(ret, "failed to init subset %d: %d", i, ret);
> 			irq_domain_remove(data->domain);
> 			ret = -ENOMEM;
> 			goto out_cleanup;
> 		}
> 	}
> 
> Thank you for your feedback.

You're running into the issue because you're using One Err Label style
error handling.  It would be better to use normal unwind laddering.
See my blog for more info:

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter