| Message ID | 20241212023256.3453396-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: bgmac-platform: fix an OF node reference leak | expand |
On Thu, Dec 12, 2024 at 11:32:56AM +0900, Joe Hattori wrote: > The OF node obtained by of_parse_phandle() is not freed. Define a > device node with __free(device_node) to fix the leak. > > This bug was found by an experimental static analysis tool that I am > developing. > > Fixes: 1676aba5ef7e ("net: ethernet: bgmac: device tree phy enablement") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/net/ethernet/broadcom/bgmac-platform.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c > index ecce23cecbea..ca07a6d26590 100644 > --- a/drivers/net/ethernet/broadcom/bgmac-platform.c > +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c > @@ -236,7 +236,10 @@ static int bgmac_probe(struct platform_device *pdev) > bgmac->cco_ctl_maskset = platform_bgmac_cco_ctl_maskset; > bgmac->get_bus_clock = platform_bgmac_get_bus_clock; > bgmac->cmn_maskset32 = platform_bgmac_cmn_maskset32; > - if (of_parse_phandle(np, "phy-handle", 0)) { > + > + struct device_node *phy_node __free(device_node) = > + of_parse_phandle(np, "phy-handle", 0); > + if (phy_node) { > bgmac->phy_connect = platform_phy_connect; > } else { > bgmac->phy_connect = bgmac_phy_connect_direct; Hi Joe, I agree this is a problem and that it was introduced by the cited commit. But I wonder if we can consider a different approach. I would suggest that rather than using __free the node is explicitly released. Something like this (untested): struct device_node *phy_node; ... phy_node = of_parse_phandle(np, "phy-handle", 0); if (phy_node) { of_node_put(phy_node); bgmac->phy_connect = platform_phy_connect; } ... That is, assuming that it is safe to release phy_node so early. If not, some adjustment should be made to when of_node_put() is called. This is for several reasons; 1. I could be wrong, but I believe your patch kfree's phy_node, but my understanding is that correct operation is to call of_node_put(). 2. More importantly, there is a preference in Newkorking code not to use __free and similar constructs. "Low level cleanup constructs (such as __free()) can be used when building APIs and helpers, especially scoped iterators. However, direct use of __free() within networking core and drivers is discouraged. Similar guidance applies to declaring variables mid-function. Link: https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs 3. As per the end of the quote above, there is a preference to declare all local variables at the top of the function (ideally, in reverse xmas tree order [*}) [*] https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
> Hi Joe, > > I agree this is a problem and that it was introduced by the > cited commit. But I wonder if we can consider a different approach. > > I would suggest that rather than using __free the node is explicitly > released. Something like this (untested): > > struct device_node *phy_node; > > ... > > phy_node = of_parse_phandle(np, "phy-handle", 0); > if (phy_node) { > of_node_put(phy_node); > bgmac->phy_connect = platform_phy_connect; > } ... > > That is, assuming that it is safe to release phy_node so early. > If not, some adjustment should be made to when of_node_put() > is called. > > This is for several reasons; > > 1. I could be wrong, but I believe your patch kfree's phy_node, > but my understanding is that correct operation is to call > of_node_put(). Hi Simon I _think_ that is wrong. More of the magic which i don't really like. The cleanup subsystem has to be taught all the types, and what operation to perform for each type. Despite the name __free(), i think it does actually call of_node_put(). The magic would be more readable if it was actually __put(), not __free(). > 2. More importantly, there is a preference in Newkorking code > not to use __free and similar constructs. > > "Low level cleanup constructs (such as __free()) can be used when > building APIs and helpers, especially scoped iterators. However, > direct use of __free() within networking core and drivers is > discouraged. Similar guidance applies to declaring variables > mid-function. And this is a good example of why. Andrew
On Fri, Dec 13, 2024 at 01:04:42PM +0100, Andrew Lunn wrote: > > Hi Joe, > > > > I agree this is a problem and that it was introduced by the > > cited commit. But I wonder if we can consider a different approach. > > > > I would suggest that rather than using __free the node is explicitly > > released. Something like this (untested): > > > > struct device_node *phy_node; > > > > ... > > > > phy_node = of_parse_phandle(np, "phy-handle", 0); > > if (phy_node) { > > of_node_put(phy_node); > > bgmac->phy_connect = platform_phy_connect; > > } ... > > > > That is, assuming that it is safe to release phy_node so early. > > If not, some adjustment should be made to when of_node_put() > > is called. > > > > This is for several reasons; > > > > 1. I could be wrong, but I believe your patch kfree's phy_node, > > but my understanding is that correct operation is to call > > of_node_put(). > > Hi Simon > > I _think_ that is wrong. More of the magic which i don't really > like. The cleanup subsystem has to be taught all the types, and what > operation to perform for each type. Despite the name __free(), i think > it does actually call of_node_put(). The magic would be more readable > if it was actually __put(), not __free(). Thanks, TIL. > > 2. More importantly, there is a preference in Newkorking code > > not to use __free and similar constructs. > > > > "Low level cleanup constructs (such as __free()) can be used when > > building APIs and helpers, especially scoped iterators. However, > > direct use of __free() within networking core and drivers is > > discouraged. Similar guidance applies to declaring variables > > mid-function. > > And this is a good example of why. > > Andrew >
diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c index ecce23cecbea..ca07a6d26590 100644 --- a/drivers/net/ethernet/broadcom/bgmac-platform.c +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c @@ -236,7 +236,10 @@ static int bgmac_probe(struct platform_device *pdev) bgmac->cco_ctl_maskset = platform_bgmac_cco_ctl_maskset; bgmac->get_bus_clock = platform_bgmac_get_bus_clock; bgmac->cmn_maskset32 = platform_bgmac_cmn_maskset32; - if (of_parse_phandle(np, "phy-handle", 0)) { + + struct device_node *phy_node __free(device_node) = + of_parse_phandle(np, "phy-handle", 0); + if (phy_node) { bgmac->phy_connect = platform_phy_connect; } else { bgmac->phy_connect = bgmac_phy_connect_direct;
The OF node obtained by of_parse_phandle() is not freed. Define a device node with __free(device_node) to fix the leak. This bug was found by an experimental static analysis tool that I am developing. Fixes: 1676aba5ef7e ("net: ethernet: bgmac: device tree phy enablement") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/net/ethernet/broadcom/bgmac-platform.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)