RE: [patch net v2 1/4] net/sched: Change tc_action refcnt and bindcnt to atomic

[Chris Mi <chrism@mellanox.com>]

> -----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.