| Message ID | 1508152718-28726-2-git-send-email-chrism@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/sched: Fix a system panic when deleting filters | expand |
On Mon, Oct 16, 2017 at 4:18 AM, Chris Mi <chrism@mellanox.com> wrote: > If many filters share the same action. That action's refcnt and bindcnt > could be manipulated by many RCU callbacks at the same time. This patch > makes these operations atomic. Actually I have been thinking about removing these RCU callbacks, they are not necessary AFAIK, callers hold RTNL lock so they are allowed to block. The only drawback is that adding a synchronize_rcu(), but these are slow paths anyway... I am not sure, it is arguable anyway, essentially it is: synchronize_rcu() in slow path vs. multiple RCU callback races > > Fixes commit in pre-git era. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") This is not true, the action RCU callbacks were introduced by: commit c7de2cf053420d63bac85133469c965d4b1083e1 Author: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed Jun 9 02:09:23 2010 +0000 pkt_sched: gen_kill_estimator() rcu fixes and the filter RCU callbacks were introduced by the patchset like this one: commit 1ce87720d456e471de0fbd814dc5d1fe10fc1c44 Author: John Fastabend <john.fastabend@gmail.com> Date: Fri Sep 12 20:09:16 2014 -0700 net: sched: make cls_u32 lockless
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Tuesday, October 17, 2017 1:06 AM > To: Chris Mi <chrism@mellanox.com> > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi > Salim <jhs@mojatatu.com>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko > <jiri@resnulli.us>; David Miller <davem@davemloft.net> > Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and > bindcnt to atomic > > On Mon, Oct 16, 2017 at 4:18 AM, Chris Mi <chrism@mellanox.com> wrote: > > If many filters share the same action. That action's refcnt and > > bindcnt could be manipulated by many RCU callbacks at the same time. > > This patch makes these operations atomic. > > Actually I have been thinking about removing these RCU callbacks, they are > not necessary AFAIK, callers hold RTNL lock so they are allowed to block. The > only drawback is that adding a synchronize_rcu(), but these are slow paths > anyway... > > I am not sure, it is arguable anyway, essentially it is: > > synchronize_rcu() in slow path vs. multiple RCU callback races > > > > > > Fixes commit in pre-git era. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > This is not true, the action RCU callbacks were introduced > by: > > commit c7de2cf053420d63bac85133469c965d4b1083e1 > Author: Eric Dumazet <eric.dumazet@gmail.com> > Date: Wed Jun 9 02:09:23 2010 +0000 > > pkt_sched: gen_kill_estimator() rcu fixes > > > and the filter RCU callbacks were introduced by the patchset like this one: > > > commit 1ce87720d456e471de0fbd814dc5d1fe10fc1c44 > Author: John Fastabend <john.fastabend@gmail.com> > Date: Fri Sep 12 20:09:16 2014 -0700 > > net: sched: make cls_u32 lockless I don't think this bug were introduced by above two commits only. Actually, this bug were introduced by several commits, at least the following: 1. refcnt and bindcnt are not atomic 2. passing actions using list instead of arrays (I think initially we are using arrays) 3. using RCU callbacks So instead of blaming the latest commit, it is better to say it is a pre-git error.
On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chrism@mellanox.com> wrote: > I don't think this bug were introduced by above two commits only. > Actually, this bug were introduced by several commits, at least the following: > 1. refcnt and bindcnt are not atomic Nope, it is perfectly okay with non-atomic as long as no parallel, and without RCU callback they are perfectly serialized by RTNL. > 2. passing actions using list instead of arrays (I think initially we are using arrays) We are discussing patch 1/4, this is patch 2/4, so irrelevant. > 3. using RCU callbacks This introduces problem 1. > So instead of blaming the latest commit, it is better to say it is a pre-git error. You are wrong.
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Tuesday, October 17, 2017 11:53 PM > To: Chris Mi <chrism@mellanox.com> > Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi > Salim <jhs@mojatatu.com>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko > <jiri@resnulli.us>; David Miller <davem@davemloft.net> > Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and > bindcnt to atomic > > On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chrism@mellanox.com> wrote: > > I don't think this bug were introduced by above two commits only. > > Actually, this bug were introduced by several commits, at least the > following: > > 1. refcnt and bindcnt are not atomic > > Nope, it is perfectly okay with non-atomic as long as no parallel, and without > RCU callback they are perfectly serialized by RTNL. Agree. > > > > 2. passing actions using list instead of arrays (I think initially we > > are using arrays) > > We are discussing patch 1/4, this is patch 2/4, so irrelevant. Agree. > > > > 3. using RCU callbacks > > This introduces problem 1. I think this patch set only fixes one problem, that's the race and the panic. What do you mean by problem 1. > > > > So instead of blaming the latest commit, it is better to say it is a pre-git error. > > You are wrong. OK, you are right. But could I know what's your suggestion for this patch set? 1. reject it? 2. change the "Fixes" as you suggested? 3. something else? Thanks, Chris
On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chrism@mellanox.com> wrote: >> -----Original Message----- >> From: Cong Wang [mailto:xiyou.wangcong@gmail.com] >> Sent: Tuesday, October 17, 2017 11:53 PM >> To: Chris Mi <chrism@mellanox.com> >> Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>; Jamal Hadi >> Salim <jhs@mojatatu.com>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko >> <jiri@resnulli.us>; David Miller <davem@davemloft.net> >> Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and >> bindcnt to atomic >> >> On Mon, Oct 16, 2017 at 6:14 PM, Chris Mi <chrism@mellanox.com> wrote: >> > I don't think this bug were introduced by above two commits only. >> > Actually, this bug were introduced by several commits, at least the >> following: >> > 1. refcnt and bindcnt are not atomic >> >> Nope, it is perfectly okay with non-atomic as long as no parallel, and without >> RCU callback they are perfectly serialized by RTNL. > Agree. >> >> >> > 2. passing actions using list instead of arrays (I think initially we >> > are using arrays) >> >> We are discussing patch 1/4, this is patch 2/4, so irrelevant. > Agree. >> >> >> > 3. using RCU callbacks >> >> This introduces problem 1. > I think this patch set only fixes one problem, that's the race and the panic. > What do you mean by problem 1. You listed 3 problems, and you think they are 3 different ones, here I argue problem 3 (using RCU callbacks) is the cause of problem 1 (refcnt not atomic). This is why I mentioned I have been thinking about removing RCU callbacks, because it probably could fix all of them. >> >> >> > So instead of blaming the latest commit, it is better to say it is a pre-git error. >> >> You are wrong. > OK, you are right. But could I know what's your suggestion for this patch set? > 1. reject it? > 2. change the "Fixes" as you suggested? > 3. something else? In my opinion we need to think about removing RCU callbacks rather than fixing all bugs they introduce, because it is really hard to prove we can fix all of them. In your patchset, you fix 2 bugs. Before, we fixed 2 bugs (I already list them in the other reply to you). In total, we have 4 bugs... Are we totally race-free even after your patches? It seems not at all without a lock, but as I said locking itself is hard... I will start a new thread to discuss this and keep you Cc'ed. So please hold your patches until we have a conclusion. Thanks.
On 17-10-18 12:43 PM, Cong Wang wrote: > On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chrism@mellanox.com> wrote: >>> -----Original Message----- > > You listed 3 problems, and you think they are 3 different ones, here > I argue problem 3 (using RCU callbacks) is the cause of problem 1 > (refcnt not atomic). This is why I mentioned I have been thinking about > removing RCU callbacks, because it probably could fix all of them. > Cong, Given this is a known bug (the test case Chris presented crashes the kernel) - would it make sense to have a patch that goes to -net to fix this while your approach and discussion outcome goes into net-next? cheers, jamal
On Thu, Oct 19, 2017 at 7:21 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On 17-10-18 12:43 PM, Cong Wang wrote: >> >> On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chrism@mellanox.com> wrote: >>>> >>>> -----Original Message----- > > >> >> You listed 3 problems, and you think they are 3 different ones, here >> I argue problem 3 (using RCU callbacks) is the cause of problem 1 >> (refcnt not atomic). This is why I mentioned I have been thinking about >> removing RCU callbacks, because it probably could fix all of them. >> > > Cong, > Given this is a known bug (the test case Chris presented crashes the > kernel) - would it make sense to have a patch that goes to -net > to fix this while your approach and discussion outcome goes into > net-next? I am not sure. Because Chris' patchset is large too and I don't think it could fix all crashes, so it has little value to just apply them for -net.
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Friday, October 20, 2017 11:00 AM > To: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Chris Mi <chrism@mellanox.com>; Linux Kernel Network Developers > <netdev@vger.kernel.org>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko > <jiri@resnulli.us>; David Miller <davem@davemloft.net> > Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and > bindcnt to atomic > > On Thu, Oct 19, 2017 at 7:21 AM, Jamal Hadi Salim <jhs@mojatatu.com> > wrote: > > On 17-10-18 12:43 PM, Cong Wang wrote: > >> > >> On Tue, Oct 17, 2017 at 6:03 PM, Chris Mi <chrism@mellanox.com> wrote: > >>>> > >>>> -----Original Message----- > > > > > >> > >> You listed 3 problems, and you think they are 3 different ones, here > >> I argue problem 3 (using RCU callbacks) is the cause of problem 1 > >> (refcnt not atomic). This is why I mentioned I have been thinking > >> about removing RCU callbacks, because it probably could fix all of them. > >> > > > > Cong, > > Given this is a known bug (the test case Chris presented crashes the > > kernel) - would it make sense to have a patch that goes to -net to fix > > this while your approach and discussion outcome goes into net-next? > > I am not sure. Because Chris' patchset is large too and I don't think it could fix > all crashes, so it has little value to just apply them for -net. It seems it is not easy to discard call_rcu(). I'm afraid even if we have a final solution without call_rcu(), it is not mature at the beginning as well. I mean we also need time to fix the possible bugs of the new design. And maybe to destroy the filters in parallel is the right direction. If this bug is the last bug brought by call_rcu(), then changing it may not be a good idea. Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to array. I think there is no harm to the new design. Patch 3 and 4 are useful test case. We also need it with new design to make sure there is no regression. So I think my patch set should not be held so long time. My two cents.
On Sun, Oct 22, 2017 at 7:47 PM, Chris Mi <chrism@mellanox.com> wrote: > > It seems it is not easy to discard call_rcu(). I'm afraid even if we have a final solution > without call_rcu(), it is not mature at the beginning as well. I mean we also need time Why do you believe it is not easy? RTNL lock is already there, list_splice_init_rcu() is there too. I can naturally divide my patches for each module so that they are much easier to backport than yours. > to fix the possible bugs of the new design. And maybe to destroy the filters in parallel > is the right direction. If this bug is the last bug brought by call_rcu(), then changing it > may not be a good idea. Again, you have to prove this is the last bug, I seriously doubt it is. > > Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to array. Both are big in size. > I think there is no harm to the new design. Patch 3 and 4 are useful test case. It definitely doesn't harm, but why do we waste time on it when we know there is a better way? It is clearly not easy for backport either, in fact it is harder w.r.t. size. > We also need it with new design to make sure there is no regression. > Are you saying I can't trust your test cases? ;) > So I think my patch set should not be held so long time. I think your patches should be dropped except the last two, I will take the last two for you. Thanks!
> -----Original Message----- > From: Cong Wang [mailto:xiyou.wangcong@gmail.com] > Sent: Monday, October 23, 2017 11:40 PM > To: Chris Mi <chrism@mellanox.com> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>; Linux Kernel Network Developers > <netdev@vger.kernel.org>; Lucas Bates <lucasb@mojatatu.com>; Jiri Pirko > <jiri@resnulli.us>; David Miller <davem@davemloft.net> > Subject: Re: [patch net v2 1/4] net/sched: Change tc_action refcnt and > bindcnt to atomic > > On Sun, Oct 22, 2017 at 7:47 PM, Chris Mi <chrism@mellanox.com> wrote: > > > > It seems it is not easy to discard call_rcu(). I'm afraid even if we > > have a final solution without call_rcu(), it is not mature at the > > beginning as well. I mean we also need time > > Why do you believe it is not easy? RTNL lock is already there, > list_splice_init_rcu() is there too. I can naturally divide my patches for each > module so that they are much easier to backport than yours. I tested your patches. It takes about 17 seconds to run the same test. I believe your code is simpler and easy to maintain. If it is stable, we will also get benefit. Thanks! > > > > to fix the possible bugs of the new design. And maybe to destroy the > > filters in parallel is the right direction. If this bug is the last > > bug brought by call_rcu(), then changing it may not be a good idea. > > Again, you have to prove this is the last bug, I seriously doubt it is. > > > > > > Patch 1 is straightforward to use atomic. Patch 2 is to convert the list to > array. > > Both are big in size. > > > > I think there is no harm to the new design. Patch 3 and 4 are useful test > case. > > It definitely doesn't harm, but why do we waste time on it when we know > there is a better way? It is clearly not easy for backport either, in fact it is > harder w.r.t. size. > > > > We also need it with new design to make sure there is no regression. > > > > Are you saying I can't trust your test cases? ;) > > > > So I think my patch set should not be held so long time. > > I think your patches should be dropped except the last two, I will take the > last two for you. > > Thanks!
diff --git a/include/net/act_api.h b/include/net/act_api.h index b944e0eb..a469ab6 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -25,8 +25,8 @@ struct tc_action { struct tcf_idrinfo *idrinfo; u32 tcfa_index; - int tcfa_refcnt; - int tcfa_bindcnt; + atomic_t tcfa_refcnt; + atomic_t tcfa_bindcnt; u32 tcfa_capab; int tcfa_action; struct tcf_t tcfa_tm; diff --git a/net/sched/act_api.c b/net/sched/act_api.c index da6fa82..9c0224d 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -88,12 +88,13 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict) if (p) { if (bind) - p->tcfa_bindcnt--; - else if (strict && p->tcfa_bindcnt > 0) + atomic_dec(&p->tcfa_bindcnt); + else if (strict && atomic_read(&p->tcfa_bindcnt) > 0) return -EPERM; - p->tcfa_refcnt--; - if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) { + atomic_dec(&p->tcfa_refcnt); + if (atomic_read(&p->tcfa_bindcnt) == 0 && + atomic_read(&p->tcfa_refcnt) == 0) { if (p->ops->cleanup) p->ops->cleanup(p, bind); tcf_idr_remove(p->idrinfo, p); @@ -245,8 +246,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a, if (index && p) { if (bind) - p->tcfa_bindcnt++; - p->tcfa_refcnt++; + atomic_inc(&p->tcfa_bindcnt); + atomic_inc(&p->tcfa_refcnt); *a = p; return true; } @@ -274,9 +275,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est, if (unlikely(!p)) return -ENOMEM; - p->tcfa_refcnt = 1; + atomic_set(&p->tcfa_refcnt, 1); if (bind) - p->tcfa_bindcnt = 1; + atomic_set(&p->tcfa_bindcnt, 1); if (cpustats) { p->cpu_bstats = netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu); @@ -727,7 +728,7 @@ static void cleanup_a(struct list_head *actions, int ovr) return; list_for_each_entry(a, actions, list) - a->tcfa_refcnt--; + atomic_dec(&a->tcfa_refcnt); } int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, @@ -751,7 +752,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, } act->order = i; if (ovr) - act->tcfa_refcnt++; + atomic_inc(&act->tcfa_refcnt); list_add_tail(&act->list, actions); } diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c index c0c707e..4ddf281 100644 --- a/net/sched/act_bpf.c +++ b/net/sched/act_bpf.c @@ -141,8 +141,8 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act, struct tcf_bpf *prog = to_bpf(act); struct tc_act_bpf opt = { .index = prog->tcf_index, - .refcnt = prog->tcf_refcnt - ref, - .bindcnt = prog->tcf_bindcnt - bind, + .refcnt = atomic_read(&prog->tcf_refcnt) - ref, + .bindcnt = atomic_read(&prog->tcf_bindcnt) - bind, .action = prog->tcf_action, }; struct tcf_t tm; diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c index 10b7a88..d2cf658 100644 --- a/net/sched/act_connmark.c +++ b/net/sched/act_connmark.c @@ -153,8 +153,8 @@ static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a, struct tc_connmark opt = { .index = ci->tcf_index, - .refcnt = ci->tcf_refcnt - ref, - .bindcnt = ci->tcf_bindcnt - bind, + .refcnt = atomic_read(&ci->tcf_refcnt) - ref, + .bindcnt = atomic_read(&ci->tcf_bindcnt) - bind, .action = ci->tcf_action, .zone = ci->zone, }; diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c index 1c40caa..b9ca424 100644 --- a/net/sched/act_csum.c +++ b/net/sched/act_csum.c @@ -575,8 +575,8 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind, .update_flags = p->update_flags, .index = p->tcf_index, .action = p->tcf_action, - .refcnt = p->tcf_refcnt - ref, - .bindcnt = p->tcf_bindcnt - bind, + .refcnt = atomic_read(&p->tcf_refcnt) - ref, + .bindcnt = atomic_read(&p->tcf_bindcnt) - bind, }; struct tcf_t t; diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c index e29a48e..b1326b7 100644 --- a/net/sched/act_gact.c +++ b/net/sched/act_gact.c @@ -169,8 +169,8 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_gact *gact = to_gact(a); struct tc_gact opt = { .index = gact->tcf_index, - .refcnt = gact->tcf_refcnt - ref, - .bindcnt = gact->tcf_bindcnt - bind, + .refcnt = atomic_read(&gact->tcf_refcnt) - ref, + .bindcnt = atomic_read(&gact->tcf_bindcnt) - bind, .action = gact->tcf_action, }; struct tcf_t t; diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c index 8ccd358..5cdcc87 100644 --- a/net/sched/act_ife.c +++ b/net/sched/act_ife.c @@ -551,8 +551,8 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind, struct tcf_ife_info *ife = to_ife(a); struct tc_ife opt = { .index = ife->tcf_index, - .refcnt = ife->tcf_refcnt - ref, - .bindcnt = ife->tcf_bindcnt - bind, + .refcnt = atomic_read(&ife->tcf_refcnt) - ref, + .bindcnt = atomic_read(&ife->tcf_bindcnt) - bind, .action = ife->tcf_action, .flags = ife->flags, }; diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c index d9e399a..02c14dc 100644 --- a/net/sched/act_ipt.c +++ b/net/sched/act_ipt.c @@ -277,8 +277,8 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind, if (unlikely(!t)) goto nla_put_failure; - c.bindcnt = ipt->tcf_bindcnt - bind; - c.refcnt = ipt->tcf_refcnt - ref; + c.bindcnt = atomic_read(&ipt->tcf_bindcnt) - bind; + c.refcnt = atomic_read(&ipt->tcf_refcnt) - ref; strcpy(t->u.user.name, ipt->tcfi_t->u.kernel.target->name); if (nla_put(skb, TCA_IPT_TARG, ipt->tcfi_t->u.user.target_size, t) || diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c index 416627c..aeeeb38 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -249,8 +249,8 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, struct tc_mirred opt = { .index = m->tcf_index, .action = m->tcf_action, - .refcnt = m->tcf_refcnt - ref, - .bindcnt = m->tcf_bindcnt - bind, + .refcnt = atomic_read(&m->tcf_refcnt) - ref, + .bindcnt = atomic_read(&m->tcf_bindcnt) - bind, .eaction = m->tcfm_eaction, .ifindex = m->tcfm_ifindex, }; diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c index c365d01..58fa1ae 100644 --- a/net/sched/act_nat.c +++ b/net/sched/act_nat.c @@ -256,8 +256,8 @@ static int tcf_nat_dump(struct sk_buff *skb, struct tc_action *a, .index = p->tcf_index, .action = p->tcf_action, - .refcnt = p->tcf_refcnt - ref, - .bindcnt = p->tcf_bindcnt - bind, + .refcnt = atomic_read(&p->tcf_refcnt) - ref, + .bindcnt = atomic_read(&p->tcf_bindcnt) - bind, }; struct tcf_t t; diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 491fe5de..27b9fea 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -391,8 +391,8 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a, opt->nkeys = p->tcfp_nkeys; opt->flags = p->tcfp_flags; opt->action = p->tcf_action; - opt->refcnt = p->tcf_refcnt - ref; - opt->bindcnt = p->tcf_bindcnt - bind; + opt->refcnt = atomic_read(&p->tcf_refcnt) - ref; + opt->bindcnt = atomic_read(&p->tcf_bindcnt) - bind; if (p->tcfp_keys_ex) { tcf_pedit_key_ex_dump(skb, p->tcfp_keys_ex, p->tcfp_nkeys); diff --git a/net/sched/act_police.c b/net/sched/act_police.c index 3bb2ebf..d8a1ac6 100644 --- a/net/sched/act_police.c +++ b/net/sched/act_police.c @@ -272,8 +272,8 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a, .action = police->tcf_action, .mtu = police->tcfp_mtu, .burst = PSCHED_NS2TICKS(police->tcfp_burst), - .refcnt = police->tcf_refcnt - ref, - .bindcnt = police->tcf_bindcnt - bind, + .refcnt = atomic_read(&police->tcf_refcnt) - ref, + .bindcnt = atomic_read(&police->tcf_bindcnt) - bind, }; struct tcf_t t; diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c index ec986ae..64889a4 100644 --- a/net/sched/act_sample.c +++ b/net/sched/act_sample.c @@ -179,8 +179,8 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a, struct tc_sample opt = { .index = s->tcf_index, .action = s->tcf_action, - .refcnt = s->tcf_refcnt - ref, - .bindcnt = s->tcf_bindcnt - bind, + .refcnt = atomic_read(&s->tcf_refcnt) - ref, + .bindcnt = atomic_read(&s->tcf_bindcnt) - bind, }; struct tcf_t t; diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c index e7b57e5..ec7e397 100644 --- a/net/sched/act_simple.c +++ b/net/sched/act_simple.c @@ -148,8 +148,8 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_defact *d = to_defact(a); struct tc_defact opt = { .index = d->tcf_index, - .refcnt = d->tcf_refcnt - ref, - .bindcnt = d->tcf_bindcnt - bind, + .refcnt = atomic_read(&d->tcf_refcnt) - ref, + .bindcnt = atomic_read(&d->tcf_bindcnt) - bind, .action = d->tcf_action, }; struct tcf_t t; diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c index 59949d6..2b05e56 100644 --- a/net/sched/act_skbedit.c +++ b/net/sched/act_skbedit.c @@ -172,8 +172,8 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_skbedit *d = to_skbedit(a); struct tc_skbedit opt = { .index = d->tcf_index, - .refcnt = d->tcf_refcnt - ref, - .bindcnt = d->tcf_bindcnt - bind, + .refcnt = atomic_read(&d->tcf_refcnt) - ref, + .bindcnt = atomic_read(&d->tcf_bindcnt) - bind, .action = d->tcf_action, }; struct tcf_t t; diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c index b642ad3..ca1ddf9 100644 --- a/net/sched/act_skbmod.c +++ b/net/sched/act_skbmod.c @@ -201,8 +201,8 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_skbmod_params *p = rtnl_dereference(d->skbmod_p); struct tc_skbmod opt = { .index = d->tcf_index, - .refcnt = d->tcf_refcnt - ref, - .bindcnt = d->tcf_bindcnt - bind, + .refcnt = atomic_read(&d->tcf_refcnt) - ref, + .bindcnt = atomic_read(&d->tcf_bindcnt) - bind, .action = d->tcf_action, }; struct tcf_t t; diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c index 30c9627..158d472 100644 --- a/net/sched/act_tunnel_key.c +++ b/net/sched/act_tunnel_key.c @@ -250,8 +250,8 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_tunnel_key_params *params; struct tc_tunnel_key opt = { .index = t->tcf_index, - .refcnt = t->tcf_refcnt - ref, - .bindcnt = t->tcf_bindcnt - bind, + .refcnt = atomic_read(&t->tcf_refcnt) - ref, + .bindcnt = atomic_read(&t->tcf_bindcnt) - bind, }; struct tcf_t tm; diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c index 16eb067..e2d7aab 100644 --- a/net/sched/act_vlan.c +++ b/net/sched/act_vlan.c @@ -208,8 +208,8 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a, struct tcf_vlan *v = to_vlan(a); struct tc_vlan opt = { .index = v->tcf_index, - .refcnt = v->tcf_refcnt - ref, - .bindcnt = v->tcf_bindcnt - bind, + .refcnt = atomic_read(&v->tcf_refcnt) - ref, + .bindcnt = atomic_read(&v->tcf_bindcnt) - bind, .action = v->tcf_action, .v_action = v->tcfv_action, };
If many filters share the same action. That action's refcnt and bindcnt could be manipulated by many RCU callbacks at the same time. This patch makes these operations atomic. Fixes commit in pre-git era. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Chris Mi <chrism@mellanox.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> --- include/net/act_api.h | 4 ++-- net/sched/act_api.c | 21 +++++++++++---------- net/sched/act_bpf.c | 4 ++-- net/sched/act_connmark.c | 4 ++-- net/sched/act_csum.c | 4 ++-- net/sched/act_gact.c | 4 ++-- net/sched/act_ife.c | 4 ++-- net/sched/act_ipt.c | 4 ++-- net/sched/act_mirred.c | 4 ++-- net/sched/act_nat.c | 4 ++-- net/sched/act_pedit.c | 4 ++-- net/sched/act_police.c | 4 ++-- net/sched/act_sample.c | 4 ++-- net/sched/act_simple.c | 4 ++-- net/sched/act_skbedit.c | 4 ++-- net/sched/act_skbmod.c | 4 ++-- net/sched/act_tunnel_key.c | 4 ++-- net/sched/act_vlan.c | 4 ++-- 18 files changed, 45 insertions(+), 44 deletions(-)