On Wed, Nov 29, 2023 at 11:21:06AM +0300, Dan Carpenter wrote:
> > +int realtek_intc_probe(struct platform_device *pdev, const struct realtek_intc_info *info)
> > +{
> > + struct realtek_intc_data *data;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = dev->of_node;
> > + int ret, i;
> > +
> > + data = devm_kzalloc(dev, struct_size(data, subset_data, info->cfg_num), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->base = of_iomap(node, 0);
> > + if (!data->base) {
> > + ret = -ENOMEM;
> > + goto out_cleanup;
>
> devm_ allocations are cleaned up automatically so there is no need to
> call devm_kfree() before returning.
>
> regards,
> dan carpenter
>
> > + }
> > +
> > + 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.
> > + 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
> > + }
> > + }
> > +
> > + platform_set_drvdata(pdev, data);
> > +
> > + return 0;
> > +
> > +out_cleanup:
> > +
> > + if (data->base)
> > + iounmap(data->base);
> > +
> > + devm_kfree(dev, data);
> > +
> > + return ret;
> > +}
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.
Regards,
James
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
Hi Dan,
>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/
>
Thanks for your guidance. I will adjust the error handling flow.
Regards,
James