| Message ID | 1449698860-20685-1-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote: > If the actions (re)allocation fails, or the actions list is larger than the > maximum size, and the conntrack action is the last action when these > problems are hit, then references to helper modules may be leaked. Fix > the issue. > > Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > net/openvswitch/conntrack.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index c2cc11168fd5..585a5aa81f89 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -53,6 +53,8 @@ struct ovs_conntrack_info { > struct md_labels labels; > }; > > +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info); > + > static u16 key_to_nfproto(const struct sw_flow_key *key) > { > switch (ntohs(key->eth.type)) { > @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, > nf_conntrack_get(&ct_info.ct->ct_general); I see another issue here. Updates to ct_info are done after it is copied to action list. ct action on the action list and ct_info are inconsistent, So it is still leaking nf-conntrack reference. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote: > On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote: >> If the actions (re)allocation fails, or the actions list is larger than the >> maximum size, and the conntrack action is the last action when these >> problems are hit, then references to helper modules may be leaked. Fix >> the issue. >> >> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") >> Signed-off-by: Joe Stringer <joe@ovn.org> >> --- >> net/openvswitch/conntrack.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index c2cc11168fd5..585a5aa81f89 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -53,6 +53,8 @@ struct ovs_conntrack_info { >> struct md_labels labels; >> }; >> >> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info); >> + >> static u16 key_to_nfproto(const struct sw_flow_key *key) >> { >> switch (ntohs(key->eth.type)) { >> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, >> nf_conntrack_get(&ct_info.ct->ct_general); > > I see another issue here. Updates to ct_info are done after it is > copied to action list. ct action on the action list and ct_info are > inconsistent, So it is still leaking nf-conntrack reference. You're referring to the below? __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status); nf_conntrack_get(&ct_info.ct->ct_general); ct_info.ct points to the same location as the version that has been copied into the action list. I agree it's a bit misleading though. If you're not seeing something else, it seems cosmetic so I can (should?) follow up on net-next. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 9, 2015 at 3:10 PM, Joe Stringer <joe@ovn.org> wrote: > On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote: >> On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote: >>> If the actions (re)allocation fails, or the actions list is larger than the >>> maximum size, and the conntrack action is the last action when these >>> problems are hit, then references to helper modules may be leaked. Fix >>> the issue. >>> >>> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") >>> Signed-off-by: Joe Stringer <joe@ovn.org> >>> --- >>> net/openvswitch/conntrack.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>> index c2cc11168fd5..585a5aa81f89 100644 >>> --- a/net/openvswitch/conntrack.c >>> +++ b/net/openvswitch/conntrack.c >>> @@ -53,6 +53,8 @@ struct ovs_conntrack_info { >>> struct md_labels labels; >>> }; >>> >>> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info); >>> + >>> static u16 key_to_nfproto(const struct sw_flow_key *key) >>> { >>> switch (ntohs(key->eth.type)) { >>> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, >>> nf_conntrack_get(&ct_info.ct->ct_general); >> >> I see another issue here. Updates to ct_info are done after it is >> copied to action list. ct action on the action list and ct_info are >> inconsistent, So it is still leaking nf-conntrack reference. > > You're referring to the below? > > __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status); > nf_conntrack_get(&ct_info.ct->ct_general); > > ct_info.ct points to the same location as the version that has been > copied into the action list. I agree it's a bit misleading though. If > you're not seeing something else, it seems cosmetic so I can (should?) > follow up on net-next. Right, there is no leak, cosmetic changes can be done later. I do not have problem with this patch as it is. Acked-by: Pravin B Shelar <pshelar@nicira.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Stringer <joe@ovn.org> Date: Wed, 9 Dec 2015 14:07:39 -0800 > If the actions (re)allocation fails, or the actions list is larger than the > maximum size, and the conntrack action is the last action when these > problems are hit, then references to helper modules may be leaked. Fix > the issue. > > Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") > Signed-off-by: Joe Stringer <joe@ovn.org> Applied and queued up for -stable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 December 2015 at 15:33, Pravin Shelar <pshelar@nicira.com> wrote: > On Wed, Dec 9, 2015 at 3:10 PM, Joe Stringer <joe@ovn.org> wrote: >> On 9 December 2015 at 14:50, Pravin Shelar <pshelar@nicira.com> wrote: >>> On Wed, Dec 9, 2015 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote: >>>> If the actions (re)allocation fails, or the actions list is larger than the >>>> maximum size, and the conntrack action is the last action when these >>>> problems are hit, then references to helper modules may be leaked. Fix >>>> the issue. >>>> >>>> Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") >>>> Signed-off-by: Joe Stringer <joe@ovn.org> >>>> --- >>>> net/openvswitch/conntrack.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>>> index c2cc11168fd5..585a5aa81f89 100644 >>>> --- a/net/openvswitch/conntrack.c >>>> +++ b/net/openvswitch/conntrack.c >>>> @@ -53,6 +53,8 @@ struct ovs_conntrack_info { >>>> struct md_labels labels; >>>> }; >>>> >>>> +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info); >>>> + >>>> static u16 key_to_nfproto(const struct sw_flow_key *key) >>>> { >>>> switch (ntohs(key->eth.type)) { >>>> @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, >>>> nf_conntrack_get(&ct_info.ct->ct_general); >>> >>> I see another issue here. Updates to ct_info are done after it is >>> copied to action list. ct action on the action list and ct_info are >>> inconsistent, So it is still leaking nf-conntrack reference. >> >> You're referring to the below? >> >> __set_bit(IPS_CONFIRMED_BIT, &ct_info.ct->status); >> nf_conntrack_get(&ct_info.ct->ct_general); >> >> ct_info.ct points to the same location as the version that has been >> copied into the action list. I agree it's a bit misleading though. If >> you're not seeing something else, it seems cosmetic so I can (should?) >> follow up on net-next. > > Right, there is no leak, cosmetic changes can be done later. I do not > have problem with this patch as it is. > > Acked-by: Pravin B Shelar <pshelar@nicira.com> Turns out that there wasn't a leak on nf-conntrack, but on the template. I sent a patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index c2cc11168fd5..585a5aa81f89 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -53,6 +53,8 @@ struct ovs_conntrack_info { struct md_labels labels; }; +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info); + static u16 key_to_nfproto(const struct sw_flow_key *key) { switch (ntohs(key->eth.type)) { @@ -708,7 +710,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr, nf_conntrack_get(&ct_info.ct->ct_general); return 0; err_free_ct: - nf_conntrack_free(ct_info.ct); + __ovs_ct_free_action(&ct_info); return err; } @@ -750,6 +752,11 @@ void ovs_ct_free_action(const struct nlattr *a) { struct ovs_conntrack_info *ct_info = nla_data(a); + __ovs_ct_free_action(ct_info); +} + +static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) +{ if (ct_info->helper) module_put(ct_info->helper->me); if (ct_info->ct)
If the actions (re)allocation fails, or the actions list is larger than the maximum size, and the conntrack action is the last action when these problems are hit, then references to helper modules may be leaked. Fix the issue. Fixes: cae3a2627520 ("openvswitch: Allow attaching helpers to ct action") Signed-off-by: Joe Stringer <joe@ovn.org> --- net/openvswitch/conntrack.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)