| Message ID | 20201021052150.25914-1-dinghao.liu@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | can: vxcan: Fix memleak in vxcan_newlink | expand |
On 21.10.20 07:21, Dinghao Liu wrote: > When rtnl_configure_link() fails, peer needs to be > freed just like when register_netdevice() fails. > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> Btw. as the vxcan.c driver bases on veth.c the same issue can be found there! At this point: https://elixir.bootlin.com/linux/latest/source/drivers/net/veth.c#L1398 err_register_dev: /* nothing to do */ err_configure_peer: unregister_netdevice(peer); return err; <<<<<<<<<<<<<<<<<<<<<<< err_register_peer: free_netdev(peer); return err; } IMO the return must be removed to fall through the next label and free the netdevice too. Would you like so send a patch for veth.c too? Best regards, Oliver > --- > drivers/net/can/vxcan.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c > index d6ba9426be4d..aefc5a61d239 100644 > --- a/drivers/net/can/vxcan.c > +++ b/drivers/net/can/vxcan.c > @@ -244,6 +244,7 @@ static int vxcan_newlink(struct net *net, struct net_device *dev, > > unregister_network_device: > unregister_netdevice(peer); > + free_netdev(peer); > return err; > } > >
> > On 21.10.20 07:21, Dinghao Liu wrote: > > When rtnl_configure_link() fails, peer needs to be > > freed just like when register_netdevice() fails. > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> > > Btw. as the vxcan.c driver bases on veth.c the same issue can be found > there! > > At this point: > https://elixir.bootlin.com/linux/latest/source/drivers/net/veth.c#L1398 > > err_register_dev: > /* nothing to do */ > err_configure_peer: > unregister_netdevice(peer); > return err; <<<<<<<<<<<<<<<<<<<<<<< > > err_register_peer: > free_netdev(peer); > return err; > } > > IMO the return must be removed to fall through the next label and free > the netdevice too. > > Would you like so send a patch for veth.c too? > Sure. Regards, Dinghao
On Wed, 21 Oct 2020 13:20:16 +0200 Oliver Hartkopp wrote: > On 21.10.20 07:21, Dinghao Liu wrote: > > When rtnl_configure_link() fails, peer needs to be > > freed just like when register_netdevice() fails. > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> > > Btw. as the vxcan.c driver bases on veth.c the same issue can be found > there! > > At this point: > https://elixir.bootlin.com/linux/latest/source/drivers/net/veth.c#L1398 > > err_register_dev: > /* nothing to do */ > err_configure_peer: > unregister_netdevice(peer); > return err; <<<<<<<<<<<<<<<<<<<<<<< > > err_register_peer: > free_netdev(peer); > return err; > } > > IMO the return must be removed to fall through the next label and free > the netdevice too. > > Would you like so send a patch for veth.c too? Ah, this is where Liu Dinghao got the veth suggestion :) Does vxcan actually need this patch? static void vxcan_setup(struct net_device *dev) { [...] dev->needs_free_netdev = true;
On 22.10.20 18:14, Jakub Kicinski wrote: > On Wed, 21 Oct 2020 13:20:16 +0200 Oliver Hartkopp wrote: >> On 21.10.20 07:21, Dinghao Liu wrote: >>> When rtnl_configure_link() fails, peer needs to be >>> freed just like when register_netdevice() fails. >>> >>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> >> >> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net> >> >> Btw. as the vxcan.c driver bases on veth.c the same issue can be found >> there! >> >> At this point: >> https://elixir.bootlin.com/linux/latest/source/drivers/net/veth.c#L1398 >> >> err_register_dev: >> /* nothing to do */ >> err_configure_peer: >> unregister_netdevice(peer); >> return err; <<<<<<<<<<<<<<<<<<<<<<< >> >> err_register_peer: >> free_netdev(peer); >> return err; >> } >> >> IMO the return must be removed to fall through the next label and free >> the netdevice too. >> >> Would you like so send a patch for veth.c too? > > Ah, this is where Liu Dinghao got the veth suggestion :) > > Does vxcan actually need this patch? > > static void vxcan_setup(struct net_device *dev) > { > [...] > dev->needs_free_netdev = true; > Oh! In fact the vxcan.c is really similar to veth.c in these code snippets - so I wondered why this never had been seen in veth.c. Then vxcan.c doesn't need that patch too :-/ Thanks for the heads up! @Marc: Can you please make sure that it doesn't get into upstream? Tnx! Best, Oliver
On 10/22/20 7:34 PM, Oliver Hartkopp wrote:
> @Marc: Can you please make sure that it doesn't get into upstream? Tnx!
Ok, I've removed
can: vxcan: Fix memleak in vxcan_newlink [Dinghao Liu]
from my linux-can/testing.
Marc
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c index d6ba9426be4d..aefc5a61d239 100644 --- a/drivers/net/can/vxcan.c +++ b/drivers/net/can/vxcan.c @@ -244,6 +244,7 @@ static int vxcan_newlink(struct net *net, struct net_device *dev, unregister_network_device: unregister_netdevice(peer); + free_netdev(peer); return err; }
When rtnl_configure_link() fails, peer needs to be freed just like when register_netdevice() fails. Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- drivers/net/can/vxcan.c | 1 + 1 file changed, 1 insertion(+)