[Patch net-next] net_sched: cancel nest attribute on failure in tcf_exts_dump()

[Patch net-next] net_sched: cancel nest attribute on failure in tcf_exts_dump()

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

Like other places, we need to cancel the nest attribute after
we start. Fortunately the netlink message will not be sent on
failure, so it's not a big problem at all.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Cong Wang <cwang@twopensource.com>
---
 net/sched/cls_api.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 45527e6..c28b0d3 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -561,13 +561,14 @@ EXPORT_SYMBOL(tcf_exts_change);
 int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 {
 #ifdef CONFIG_NET_CLS_ACT
+	struct nlattr *nest;
+
 	if (exts->action && !list_empty(&exts->actions)) {
 		/*
 		 * again for backward compatible mode - we want
 		 * to work with both old and new modes of entering
 		 * tc data even if iproute2  was newer - jhs
 		 */
-		struct nlattr *nest;
 		if (exts->type != TCA_OLD_COMPAT) {
 			nest = nla_nest_start(skb, exts->action);
 			if (nest == NULL)
@@ -585,10 +586,14 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 			nla_nest_end(skb, nest);
 		}
 	}
-#endif
 	return 0;
-nla_put_failure: __attribute__ ((unused))
+
+nla_put_failure:
+	nla_nest_cancel(skb, nest);
 	return -1;
+#else
+	return 0;
+#endif
 }
 EXPORT_SYMBOL(tcf_exts_dump);
 
-- 
1.8.3.1

[Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Cong Wang]

From: Cong Wang <cwang@twopensource.com>

We modify mirred action (m->tcfm_dev) in netdev event, we need to
prevent on-going mirred actions from reading freed m->tcfm_dev.
So we need to acquire this spin lock.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <cwang@twopensource.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.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 4f912c0..eb48306 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -218,10 +218,12 @@ static int mirred_device_event(struct notifier_block *unused,
 
 	if (event == NETDEV_UNREGISTER)
 		list_for_each_entry(m, &mirred_list, tcfm_list) {
+			spin_lock_bh(&m->tcf_lock);
 			if (m->tcfm_dev == dev) {
 				dev_put(dev);
 				m->tcfm_dev = NULL;
 			}
+			spin_unlock_bh(&m->tcf_lock);
 		}
 
 	return NOTIFY_DONE;
-- 
1.8.3.1

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Jamal Hadi Salim]

On 07/16/14 17:25, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
>
> We modify mirred action (m->tcfm_dev) in netdev event, we need to
> prevent on-going mirred actions from reading freed m->tcfm_dev.
> So we need to acquire this spin lock.
>

Cong,

What setup prompted this? We have no problems dealing with deleted
devices in the data path. It seems harmless otherwise.

cheers,
jamal

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Cong Wang]

On Thu, Jul 17, 2014 at 5:49 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 07/16/14 17:25, Cong Wang wrote:
>>
>> From: Cong Wang <cwang@twopensource.com>
>>
>> We modify mirred action (m->tcfm_dev) in netdev event, we need to
>> prevent on-going mirred actions from reading freed m->tcfm_dev.
>> So we need to acquire this spin lock.
>>
>
> Cong,
>
> What setup prompted this? We have no problems dealing with deleted
> devices in the data path. It seems harmless otherwise.
>


I found this during code review, don't see any real crash. This
is why I sent it for net-next not net. :)

The reason why it doesn't trigger a crash is I think dev_put()
doesn't immediately release dev when refcount hits zero,
instead it defers that to rtnl_unlock().

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Eric Dumazet]

On Thu, 2014-07-17 at 09:45 -0700, Cong Wang wrote:
> On Thu, Jul 17, 2014 at 5:49 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > On 07/16/14 17:25, Cong Wang wrote:
> >>
> >> From: Cong Wang <cwang@twopensource.com>
> >>
> >> We modify mirred action (m->tcfm_dev) in netdev event, we need to
> >> prevent on-going mirred actions from reading freed m->tcfm_dev.
> >> So we need to acquire this spin lock.
> >>
> >
> > Cong,
> >
> > What setup prompted this? We have no problems dealing with deleted
> > devices in the data path. It seems harmless otherwise.
> >
> 
> 
> I found this during code review, don't see any real crash. This
> is why I sent it for net-next not net. :)
> 
> The reason why it doesn't trigger a crash is I think dev_put()
> doesn't immediately release dev when refcount hits zero,
> instead it defers that to rtnl_unlock().

There is no way this dev_put() could make refcnt reaching 0, unless
there is a serious bug elsewhere.

Anyway, tcfm_dev should probably be RCU protected.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Cong Wang]

On Thu, Jul 17, 2014 at 11:17 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Anyway, tcfm_dev should probably be RCU protected.
>

Not sure how much we gain from making it RCU. We anyway
need to hold that spinlock on fast path since we need to
update its time stamp and stats.

I would just leave it as is.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Eric Dumazet]

On Thu, 2014-07-17 at 14:25 -0700, Cong Wang wrote:
> On Thu, Jul 17, 2014 at 11:17 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Anyway, tcfm_dev should probably be RCU protected.
> >
> 
> Not sure how much we gain from making it RCU. We anyway
> need to hold that spinlock on fast path since we need to
> update its time stamp and stats.
> 
> I would just leave it as is.


I think you really should look at John Fastabend work, and many other
existing paths in the stack.

All this can be done lockless in fast path, using RCU and percpu
counters.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Cong Wang]

On Thu, Jul 17, 2014 at 10:37 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-07-17 at 14:25 -0700, Cong Wang wrote:
>> On Thu, Jul 17, 2014 at 11:17 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >
>> > Anyway, tcfm_dev should probably be RCU protected.
>> >
>>
>> Not sure how much we gain from making it RCU. We anyway
>> need to hold that spinlock on fast path since we need to
>> update its time stamp and stats.
>>
>> I would just leave it as is.
>
>
> I think you really should look at John Fastabend work, and many other
> existing paths in the stack.
>

I reviewed the last two verions of his patchset...


> All this can be done lockless in fast path, using RCU and percpu
> counters.
>

I don't think he ever removes this m->tcf_lock.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Eric Dumazet]

On Thu, 2014-07-17 at 22:51 -0700, Cong Wang wrote:
> On Thu, Jul 17, 2014 at 10:37 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2014-07-17 at 14:25 -0700, Cong Wang wrote:
> >> On Thu, Jul 17, 2014 at 11:17 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> >
> >> > Anyway, tcfm_dev should probably be RCU protected.
> >> >
> >>
> >> Not sure how much we gain from making it RCU. We anyway
> >> need to hold that spinlock on fast path since we need to
> >> update its time stamp and stats.
> >>
> >> I would just leave it as is.
> >
> >
> > I think you really should look at John Fastabend work, and many other
> > existing paths in the stack.
> >
> 
> I reviewed the last two verions of his patchset...
> 
> 
> > All this can be done lockless in fast path, using RCU and percpu
> > counters.
> >
> 
> I don't think he ever removes this m->tcf_lock.

Apparently you have hard time to understand the suggestion.

If this conversion was already done by John, I would have pointed the
patch to finalize.

There is nothing fundamental that requires this spinlock being held in
fast path.

It is perfectly possible to remove this, and first step is to use RCU,
then percpu counters.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[Jamal Hadi Salim]

On 07/18/14 03:08, Eric Dumazet wrote:
> On Thu, 2014-07-17 at 22:51 -0700, Cong Wang wrote:
>
> If this conversion was already done by John, I would have pointed the
> patch to finalize.
>
> There is nothing fundamental that requires this spinlock being held in
> fast path.
>
> It is perfectly possible to remove this, and first step is to use RCU,
> then percpu counters.
>

I think rcufication would help in some the shared action instances.
Note: post John's rcu-fication of classifier path this is not an
issue i.e
the most common use case default behavior (majority) is no sharing of
instances - in such a case there is no lock contention.

cheers,
jamal

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Jul 2014 14:25:31 -0700

> From: Cong Wang <cwang@twopensource.com>
> 
> We modify mirred action (m->tcfm_dev) in netdev event, we need to
> prevent on-going mirred actions from reading freed m->tcfm_dev.
> So we need to acquire this spin lock.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

FWIW, from looking at the code, this lock is definitely necessary.

The fact that device destruction is deferred to rtnl_unlock() only
means the problem is harder to hit, rather than impossible.

Re: [Patch net-next] net_sched: hold tcf_lock in netdevice notifier

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Jul 2014 14:25:31 -0700

> From: Cong Wang <cwang@twopensource.com>
> 
> We modify mirred action (m->tcfm_dev) in netdev event, we need to
> prevent on-going mirred actions from reading freed m->tcfm_dev.
> So we need to acquire this spin lock.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <cwang@twopensource.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, thanks Cong.

Re: [Patch net-next] net_sched: cancel nest attribute on failure in tcf_exts_dump()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Jul 2014 14:25:30 -0700

> From: Cong Wang <cwang@twopensource.com>
> 
> Like other places, we need to cancel the nest attribute after
> we start. Fortunately the netlink message will not be sent on
> failure, so it's not a big problem at all.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>

Jamal, if you could review these two changes from Cong I'd appreciate
it, thanks!

Re: [Patch net-next] net_sched: cancel nest attribute on failure in tcf_exts_dump()

[Jamal Hadi Salim]

On 07/16/14 17:25, Cong Wang wrote:
> From: Cong Wang <cwang@twopensource.com>
>
> Like other places, we need to cancel the nest attribute after
> we start. Fortunately the netlink message will not be sent on
> failure, so it's not a big problem at all.
>

Sure.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [Patch net-next] net_sched: cancel nest attribute on failure in tcf_exts_dump()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 16 Jul 2014 14:25:30 -0700

> From: Cong Wang <cwang@twopensource.com>
> 
> Like other places, we need to cancel the nest attribute after
> we start. Fortunately the netlink message will not be sent on
> failure, so it's not a big problem at all.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Cong Wang <cwang@twopensource.com>

Applied, thanks.