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
