Message ID | 1563466028-2531-1-git-send-email-yanhaishuang@cmss.chinamobile.com |
---|---|
State | Rejected |
Delegated to: | David Miller |
Headers | show |
Series | openvswitch: Fix a possible memory leak on dst_cache | expand |
From: Haishuang Yan <yanhaishuang@cmss.chinamobile.com> Date: Fri, 19 Jul 2019 00:07:08 +0800 > dst_cache should be destroyed when fail to add flow actions. > > Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel") > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com> OVS folks, please review.
On 7/18/2019 9:07 AM, Haishuang Yan wrote: > dst_cache should be destroyed when fail to add flow actions. > > Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel") > Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com> > --- > net/openvswitch/flow_netlink.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index d7559c6..1fd1cdd 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -2608,6 +2608,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr, > sizeof(*ovs_tun), log); > if (IS_ERR(a)) { > dst_release((struct dst_entry *)tun_dst); > + dst_cache_destroy(&tun_dst->u.tun_info.dst_cache); > return PTR_ERR(a); > } > Nack. dst_release will decrement the ref count and will call_rcu(&dst->rcu_head, dst_destroy_rcu) if the ref count is zero. No other net drivers call dst_destroy SFAICT. Haishuang, are you trying to fix some specific problem here? Thanks, - Greg
> On 2019年7月19日, at 上午6:12, Gregory Rose <gvrose8192@gmail.com> wrote: > > On 7/18/2019 9:07 AM, Haishuang Yan wrote: >> dst_cache should be destroyed when fail to add flow actions. >> >> Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel") >> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com> >> --- >> net/openvswitch/flow_netlink.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c >> index d7559c6..1fd1cdd 100644 >> --- a/net/openvswitch/flow_netlink.c >> +++ b/net/openvswitch/flow_netlink.c >> @@ -2608,6 +2608,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr, >> sizeof(*ovs_tun), log); >> if (IS_ERR(a)) { >> dst_release((struct dst_entry *)tun_dst); >> + dst_cache_destroy(&tun_dst->u.tun_info.dst_cache); >> return PTR_ERR(a); >> } >> > > Nack. > > dst_release will decrement the ref count and will call_rcu(&dst->rcu_head, dst_destroy_rcu) if the ref count is zero. No other net drivers call dst_destroy SFAICT. > > Haishuang, > > are you trying to fix some specific problem here? > > Thanks, > > - Greg > > Greg, You’re right, dst_cache would be freed in metadata_dst_free: 125 126 if (dst->flags & DST_METADATA) 127 metadata_dst_free((struct metadata_dst *)dst); 128 else 129 kmem_cache_free(dst->ops->kmem_cachep, dst); 130 I thought I encountered a memory leak, but it seems not an issue, thanks for you explanation.
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index d7559c6..1fd1cdd 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -2608,6 +2608,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr, sizeof(*ovs_tun), log); if (IS_ERR(a)) { dst_release((struct dst_entry *)tun_dst); + dst_cache_destroy(&tun_dst->u.tun_info.dst_cache); return PTR_ERR(a); }
dst_cache should be destroyed when fail to add flow actions. Fixes: d71785ffc7e7 ("net: add dst_cache to ovs vxlan lwtunnel") Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com> --- net/openvswitch/flow_netlink.c | 1 + 1 file changed, 1 insertion(+)