| Message ID | 1438301541-26192-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 07/31/2015 02:12 AM, Cong Wang wrote: > When we share an action within a filter, the bind refcnt > should increase, therefore we should not call tcf_hash_release(). > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Cong Wang <cwang@twopensource.com> > --- > net/sched/act_mirred.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c > index a42a3b2..2685450 100644 > --- a/net/sched/act_mirred.c > +++ b/net/sched/act_mirred.c > @@ -98,6 +98,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla, > return ret; > ret = ACT_P_CREATED; > } else { > + if (bind) > + return 0; > if (!ovr) { > tcf_hash_release(a, bind); > return -EEXIST; > Did you test all variants on this? I.e. what happens when you replace an existing one, I think the refcnt should also be dropped here. It looks like we only drop it, in case we tried to add an action to an already existing index ... [...] } else { if (bind) return 0; tcf_hash_release(a, bind); if (!ovr) return -EEXIST; } [...] Thanks, Daniel -- 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 Fri, Jul 31, 2015 at 3:06 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: > Did you test all variants on this? > > I.e. what happens when you replace an existing one, I think the > refcnt should also be dropped here. It looks like we only drop > it, in case we tried to add an action to an already existing index ... > Yeah, clearly the replace case is anti-pattern too, like you showed. But that is a different bug, right? Since 'bind' is independent of 'replace'. As in $subject, my patch only fixes the binding case. I will send a following patch to fix replace case. -- 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 08/01/2015 12:25 AM, Cong Wang wrote: > On Fri, Jul 31, 2015 at 3:06 AM, Daniel Borkmann <daniel@iogearbox.net> wrote: >> Did you test all variants on this? >> >> I.e. what happens when you replace an existing one, I think the >> refcnt should also be dropped here. It looks like we only drop >> it, in case we tried to add an action to an already existing index ... > > Yeah, clearly the replace case is anti-pattern too, like you showed. > > But that is a different bug, right? Since 'bind' is independent of > 'replace'. As in $subject, my patch only fixes the binding case. > I will send a following patch to fix replace case. I wasn't aware you planned to send an extra patch for the other issue, probably not worth the split, but I don't mind. Thanks, Daniel -- 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: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 30 Jul 2015 17:12:20 -0700 > When we share an action within a filter, the bind refcnt > should increase, therefore we should not call tcf_hash_release(). > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Cong Wang <cwang@twopensource.com> Applied, thanks. -- 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/sched/act_mirred.c b/net/sched/act_mirred.c index a42a3b2..2685450 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -98,6 +98,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla, return ret; ret = ACT_P_CREATED; } else { + if (bind) + return 0; if (!ovr) { tcf_hash_release(a, bind); return -EEXIST;