Re: [PATCH 4.19 46/72] net: xilinx: fix possible object referenceleak

[Pavel Machek]

Hi!
> > > [ Upstream commit fa3a419d2f674b431d38748cb58fb7da17ee8949 ]
> > >
> > > The call to of_parse_phandle returns a node pointer with refcount
> > > incremented thus it must be explicitly decremented after the last
> > > usage.
> > >
> > > Detected by coccinelle with the following warnings:
> > > ./drivers/net/ethernet/xilinx/xilinx_axienet_main.c:1624:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1569, but without a corresponding object release within this function.
> > 
> > Bug is real, but fix is horrible. This already uses gotos for error
> > handling, so use them....
> > 
> > This fixes it up.
> > 
> > Plus... I do not think these "of_node_put" fixes belong in
> > stable. They are theoretical bugs; so we hold reference to device tree
> > structure. a) it is small, b) it stays in memory, anyway. This does
> > not fix any real problem.
> > 
> 
> Thank you very much for your comments.
> We developed the following coccinelle SmPL to look for places where
> there is an of_node_put on some path but not on others.

I agree that the fix is good. Thanks for doing coccinelle work.

> We use it to detect drivers/net/ethernet/xilinx/xilinx_axienet_main.c and found the following issue:
> 
> static int axienet_probe(struct platform_device *pdev)
> {
> ...
>         struct device_node *np;
> ...
>         if (ret) {
>                 dev_err(&pdev->dev, "unable to get DMA resource\n");
>                 goto free_netdev;  ---> leaked here
>         }
> ...
>         if (IS_ERR(lp->dma_regs)) {
>                 dev_err(&pdev->dev, "could not map DMA regs\n");
>                 ret = PTR_ERR(lp->dma_regs);
>                 goto free_netdev; ---> leaked here
>         }
> ...
>          of_node_put(np);   --->    released here
> ...
> free_netdev:
>         free_netdev(ndev);
> 
>         return ret;
> }
> 
> If we insmod/rmmod xilinx_emaclite.ko multiple times, 
> axienet_probe() may be called multiple times, then a resource leak
> may occur.

Yeah, well. I agree the bug is real. But how much memory will it leak
during each insmod? Kilobyte? (Is it actually anything at all? I'd
expect just reference counter to be increaed.) How often do you
usually insmod?

> At the same time, we also checked the code for handling resource leaks in the current kernel
> and found that the regular of_node_put mode is commonly used in
> addition to the goto target mode.

Ok, so this uglyness happens elsewhere. But I'd really prefer to use
goto if it is already used in the function.

Thanks,

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Attachment: signature.asc
Description: Digital signature