Message ID | 1568734808-42628-1-git-send-email-xiangxia.m.yue@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: openvswitch: fix possible memleak on create vport fails | expand |
On Tue, 17 Sep 2019 23:40:08 +0800, xiangxia.m.yue@gmail.com wrote: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > If we register a net device which name is not valid > (dev_get_valid_name), register_netdevice will return err > codes and will not run dev->priv_destructor. The memory > will leak. This patch adds check in ovs_vport_free and > set the vport NULL. > > Fixes: 309b66970ee2 ("net: openvswitch: do not free vport if register_netdevice() is failed.") > Cc: Taehee Yoo <ap420073@gmail.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> Thanks for the patch, I see what you're trying to do, but.. > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > index d2437b5..074c43f 100644 > --- a/net/openvswitch/vport-internal_dev.c > +++ b/net/openvswitch/vport-internal_dev.c > @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > struct internal_dev *internal_dev; > struct net_device *dev; > int err; > - bool free_vport = true; > > vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms); > if (IS_ERR(vport)) { > @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > > rtnl_lock(); > err = register_netdevice(vport->dev); > - if (err) { > - free_vport = false; > + if (err) > goto error_unlock; > - } > > dev_set_promiscuity(vport->dev, 1); > rtnl_unlock(); > @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > error_free_netdev: > free_netdev(dev); > error_free_vport: > - if (free_vport) > - ovs_vport_free(vport); > + ovs_vport_free(vport); > error: > return ERR_PTR(err); > } > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index 3fc38d1..281259a 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops, > */ > void ovs_vport_free(struct vport *vport) > { > + /* We should check whether vport is NULL. > + * We may free it again, for example in internal_dev_create > + * if register_netdevice fails, vport may have been freed via > + * internal_dev_destructor. > + */ > + if (unlikely(!vport)) > + return; > + > /* vport is freed from RCU callback or error path, Therefore > * it is safe to use raw dereference. > */ > kfree(rcu_dereference_raw(vport->upcall_portids)); > kfree(vport); > + vport = NULL; vport here is a function argument, seems like setting it to NULL right before the function ends will do nothing. Should we rather set internal_dev->vport to NULL somehow? Perhaps someone more familiar with OvS can chime in and review.. > } > EXPORT_SYMBOL_GPL(ovs_vport_free); >
On Sat, Sep 21, 2019 at 8:07 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > If we register a net device which name is not valid > (dev_get_valid_name), register_netdevice will return err > codes and will not run dev->priv_destructor. The memory > will leak. This patch adds check in ovs_vport_free and > set the vport NULL. > > Fixes: 309b66970ee2 ("net: openvswitch: do not free vport if register_netdevice() is failed.") > Cc: Taehee Yoo <ap420073@gmail.com> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > net/openvswitch/vport-internal_dev.c | 8 ++------ > net/openvswitch/vport.c | 9 +++++++++ > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c > index d2437b5..074c43f 100644 > --- a/net/openvswitch/vport-internal_dev.c > +++ b/net/openvswitch/vport-internal_dev.c > @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > struct internal_dev *internal_dev; > struct net_device *dev; > int err; > - bool free_vport = true; > > vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms); > if (IS_ERR(vport)) { > @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > > rtnl_lock(); > err = register_netdevice(vport->dev); > - if (err) { > - free_vport = false; > + if (err) > goto error_unlock; > - } > > dev_set_promiscuity(vport->dev, 1); > rtnl_unlock(); > @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) > error_free_netdev: > free_netdev(dev); > error_free_vport: > - if (free_vport) > - ovs_vport_free(vport); > + ovs_vport_free(vport); > error: > return ERR_PTR(err); > } > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index 3fc38d1..281259a 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops, > */ > void ovs_vport_free(struct vport *vport) > { > + /* We should check whether vport is NULL. > + * We may free it again, for example in internal_dev_create > + * if register_netdevice fails, vport may have been freed via > + * internal_dev_destructor. > + */ > + if (unlikely(!vport)) > + return; > + > /* vport is freed from RCU callback or error path, Therefore > * it is safe to use raw dereference. > */ > kfree(rcu_dereference_raw(vport->upcall_portids)); > kfree(vport); > + vport = NULL; > } > EXPORT_SYMBOL_GPL(ovs_vport_free); > > -- There is already patch a patch to fix this memory leak. https://patchwork.ozlabs.org/patch/1144316/ Can you or Hillf post it on netdev list? Thanks.
On Sat, Sep 21, 2019 at 8:48 PM Hillf Danton <hdanton@sina.com> wrote: > > On Sun, 9 Sep 2019 11:14 from Pravin Shelar <pshelar@ovn.org> > > > > > > There is already patch a patch to fix this memory leak. > > > https://patchwork.ozlabs.org/patch/1144316/ > > > Can you or Hillf post it on netdev list? > > > > Was that posted without netdev Cced? I do not see your patch on netdev patchwork, repost of the patch would put it on netdev patchwork.
On Sun, Sep 22, 2019 at 2:50 PM Hillf Danton <hdanton@sina.com> wrote: > > On Sun, 22 Sep 2019 14:42 from Pravin Shelar <pshelar@ovn.org> > > >> > > >> On Sat, Sep 21, 2019 at 8:48 PM Hillf Danton <hdanton@sina.com> wrote: > > >> Was that posted without netdev Cced? > > > > > > I do not see your patch on netdev patchwork, repost of the patch would > > > put it on netdev patchwork. > > > > I did send it and you did see it, so no fault on your side and my side. > > Where went wrong? > > A bit baffled. I did't not find your patch in the linux upstream, so I send my patch. Please resent your patch and I hope you should add comment on the vport->dev->priv_destructor = internal_dev_destructor; To explain why you move the code here, that can help others review the codes.
diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c index d2437b5..074c43f 100644 --- a/net/openvswitch/vport-internal_dev.c +++ b/net/openvswitch/vport-internal_dev.c @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) struct internal_dev *internal_dev; struct net_device *dev; int err; - bool free_vport = true; vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms); if (IS_ERR(vport)) { @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) rtnl_lock(); err = register_netdevice(vport->dev); - if (err) { - free_vport = false; + if (err) goto error_unlock; - } dev_set_promiscuity(vport->dev, 1); rtnl_unlock(); @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms) error_free_netdev: free_netdev(dev); error_free_vport: - if (free_vport) - ovs_vport_free(vport); + ovs_vport_free(vport); error: return ERR_PTR(err); } diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c index 3fc38d1..281259a 100644 --- a/net/openvswitch/vport.c +++ b/net/openvswitch/vport.c @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops, */ void ovs_vport_free(struct vport *vport) { + /* We should check whether vport is NULL. + * We may free it again, for example in internal_dev_create + * if register_netdevice fails, vport may have been freed via + * internal_dev_destructor. + */ + if (unlikely(!vport)) + return; + /* vport is freed from RCU callback or error path, Therefore * it is safe to use raw dereference. */ kfree(rcu_dereference_raw(vport->upcall_portids)); kfree(vport); + vport = NULL; } EXPORT_SYMBOL_GPL(ovs_vport_free);