All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] tc: fix tc actions in case of shared skb
@ 2015-07-11  0:10 Alexei Starovoitov
  2015-07-12  4:29 ` David Miller
  2015-07-13 13:13 ` Jamal Hadi Salim
  0 siblings, 2 replies; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-11  0:10 UTC (permalink / raw)
  To: David S. Miller; +Cc: Jamal Hadi Salim, Daniel Borkmann, Jiri Pirko, netdev

TC actions need to check for very unlikely event skb->users != 1,
otherwise subsequent pskb_may_pull/pskb_expand_head will crash.
When skb_shared() just drop the packet, since in the middle of actions
it's too late to call skb_share_check(), since classifiers/actions assume
the same skb pointer.

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 net/sched/act_csum.c |    3 +++
 net/sched/act_nat.c  |    3 +++
 net/sched/act_vlan.c |    3 +++
 3 files changed, 9 insertions(+)

diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index b07c535ba8e7..ac150bdc24f4 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -510,6 +510,9 @@ static int tcf_csum(struct sk_buff *skb,
 	if (unlikely(action == TC_ACT_SHOT))
 		goto drop;
 
+	if (unlikely(skb_shared(skb)))
+		goto drop;
+
 	switch (tc_skb_protocol(skb)) {
 	case cpu_to_be16(ETH_P_IP):
 		if (!tcf_csum_ipv4(skb, update_flags))
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 5be0b3c1c5b0..8bb2657de635 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -114,6 +114,9 @@ static int tcf_nat(struct sk_buff *skb, const struct tc_action *a,
 	if (unlikely(action == TC_ACT_SHOT))
 		goto drop;
 
+	if (unlikely(skb_shared(skb)))
+		goto drop;
+
 	noff = skb_network_offset(skb);
 	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
 		goto drop;
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 796785e0bf96..6365ae036c6e 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -33,6 +33,9 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 	bstats_update(&v->tcf_bstats, skb);
 	action = v->tcf_action;
 
+	if (unlikely(skb_shared(skb)))
+		goto drop;
+
 	switch (v->tcfv_action) {
 	case TCA_VLAN_ACT_POP:
 		err = skb_vlan_pop(skb);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-11  0:10 [PATCH net-next] tc: fix tc actions in case of shared skb Alexei Starovoitov
@ 2015-07-12  4:29 ` David Miller
  2015-07-13 19:47   ` Alexei Starovoitov
  2015-07-13 13:13 ` Jamal Hadi Salim
  1 sibling, 1 reply; 16+ messages in thread
From: David Miller @ 2015-07-12  4:29 UTC (permalink / raw)
  To: ast; +Cc: jhs, daniel, jiri, netdev

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Fri, 10 Jul 2015 17:10:11 -0700

> TC actions need to check for very unlikely event skb->users != 1,
> otherwise subsequent pskb_may_pull/pskb_expand_head will crash.
> When skb_shared() just drop the packet, since in the middle of actions
> it's too late to call skb_share_check(), since classifiers/actions assume
> the same skb pointer.
> 
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>

I think whatever creates this skb->users != 1 situation should be fixed,
they should clone the packet.

In fact, it would really help enormously if you could explain in detail
how this situation can actually arise.  Especially since I do not consider
it acceptable to drop the packet in this situation.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-11  0:10 [PATCH net-next] tc: fix tc actions in case of shared skb Alexei Starovoitov
  2015-07-12  4:29 ` David Miller
@ 2015-07-13 13:13 ` Jamal Hadi Salim
  1 sibling, 0 replies; 16+ messages in thread
From: Jamal Hadi Salim @ 2015-07-13 13:13 UTC (permalink / raw)
  To: Alexei Starovoitov, David S. Miller; +Cc: Daniel Borkmann, Jiri Pirko, netdev

On 07/10/15 20:10, Alexei Starovoitov wrote:
> TC actions need to check for very unlikely event skb->users != 1,
> otherwise subsequent pskb_may_pull/pskb_expand_head will crash.
> When skb_shared() just drop the packet, since in the middle of actions
> it's too late to call skb_share_check(), since classifiers/actions assume
> the same skb pointer.
>

Alexei,
To add to what Dave said - are the rules specified here:
Documentation/networking/tc-actions-env-rules.txt
insufficient?

cheers,
jamal

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-12  4:29 ` David Miller
@ 2015-07-13 19:47   ` Alexei Starovoitov
  2015-07-13 20:04     ` David Miller
  0 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-13 19:47 UTC (permalink / raw)
  To: David Miller; +Cc: jhs, daniel, jiri, netdev

On 7/11/15 9:29 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Fri, 10 Jul 2015 17:10:11 -0700
>
>> TC actions need to check for very unlikely event skb->users != 1,
>> otherwise subsequent pskb_may_pull/pskb_expand_head will crash.
>> When skb_shared() just drop the packet, since in the middle of actions
>> it's too late to call skb_share_check(), since classifiers/actions assume
>> the same skb pointer.
>>
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>
> I think whatever creates this skb->users != 1 situation should be fixed,
> they should clone the packet.

In all normal cases skb->users == 1, but pktgen is using trick:
atomic_add(burst, &skb->users);
so when testing something like:
tc filter add dev $dev root pref 10 u32 match u32 0 0 flowid 1:2 \
   action vlan push id 2 action drop

it will crash:
[   31.999519] kernel BUG at ../net/core/skbuff.c:1130!
[   31.999519] invalid opcode: 0000 [#1] PREEMPT SMP
[   31.999519] Modules linked in: act_gact act_vlan cls_u32 sch_ingress 
veth pktgen
[   31.999519] CPU: 0 PID: 339 Comm: kpktgend_0 Not tainted 4.1.0+ #730
[   31.999519] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), [ 
   31.999519] Call Trace:
[   31.999519]  [<ffffffff8160eea7>] skb_vlan_push+0x1d7/0x200
[   31.999519]  [<ffffffffa0017108>] tcf_vlan+0x108/0x110 [act_vlan]
[   31.999519]  [<ffffffff81650d26>] tcf_action_exec+0x46/0x80
[   31.999519]  [<ffffffffa001f4fe>] u32_classify+0x30e/0x740 [cls_u32]
[   31.999519]  [<ffffffff810bcc6f>] ? __lock_acquire+0xbcf/0x1e80
[   31.999519]  [<ffffffff810bcc6f>] ? __lock_acquire+0xbcf/0x1e80
[   31.999519]  [<ffffffff8161f392>] ? __netif_receive_skb_core+0x1b2/0xce0
[   31.999519]  [<ffffffff8164c0c3>] tc_classify_compat+0xa3/0xb0
[   31.999519]  [<ffffffff8164ca03>] tc_classify+0x33/0x90
[   31.999519]  [<ffffffff8161f674>] __netif_receive_skb_core+0x494/0xce0
[   31.999519]  [<ffffffff8161f274>] ? __netif_receive_skb_core+0x94/0xce0
[   31.999519]  [<ffffffff810bf10d>] ? trace_hardirqs_on_caller+0xad/0x1d0
[   31.999519]  [<ffffffff8161fee1>] __netif_receive_skb+0x21/0x70
[   31.999519]  [<ffffffff81620b43>] netif_receive_skb_internal+0x23/0x1c0
[   31.999519]  [<ffffffff816219a9>] netif_receive_skb_sk+0x49/0x1e0
[   31.999519]  [<ffffffffa0006e8d>] pktgen_thread_worker+0x111d/0x1fa0 
[pktgen]

> In fact, it would really help enormously if you could explain in detail
> how this situation can actually arise.  Especially since I do not consider
> it acceptable to drop the packet in this situation.

It's not pretty to drop, but it's better than crash.
I don't think we can get rid of 'skb->users += burst' trick, since
that's where all performance comes from (for both TX and RX testing).

So the only cheap way I see to avoid crash is to do this
if (unlikely(skb_shared(skb)))
check in actions that call pskb_expand_head.

In all normal scenarios it won't be triggered and pktgen tests
won't be crashing.
Yes. pktgen numbers will be a bit meaningless, since act_vlan will be
dropping instead of adding vlan, so users cannot make any performance
conclusions, but still better than crash.

> the rules specified here:
> Documentation/networking/tc-actions-env-rules.txt
> insufficient?

Jamal,
that doc definitely needs updating. :)
It says:
"If you munge any packet thou shalt call pskb_expand_head in the case
someone else is referencing the skb. After that you "own" the skb."
that's incorrect. If somebody 'referencing' skb via skb->users > 1
it's too late to call pskb_expand_head. As you can see in the
crash trace above.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 19:47   ` Alexei Starovoitov
@ 2015-07-13 20:04     ` David Miller
  2015-07-13 20:17       ` Alexei Starovoitov
  0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2015-07-13 20:04 UTC (permalink / raw)
  To: ast; +Cc: jhs, daniel, jiri, netdev

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Mon, 13 Jul 2015 12:47:42 -0700

> In all normal cases skb->users == 1, but pktgen is using trick:
> atomic_add(burst, &skb->users);
> so when testing something like:

You can want pktgen rx (which is the only buggy case as far as I can
see, TX is fine) to run fast, but you must do so by abiding by the
appropriate SKB sharing rules.

You can't do an optimization in pktgen for RX processing that works
"some of the time".  We have shared SKB rules for a reason.

And I don't want to have to explain to someone in the future why that
drop check is there, and have to tell them "because pktgen is broken
and we decided to add a hack here rather than make pktgen send
properly formed SKBs into the RX path"

Ok?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 20:04     ` David Miller
@ 2015-07-13 20:17       ` Alexei Starovoitov
  2015-07-13 20:55         ` Daniel Borkmann
  0 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-13 20:17 UTC (permalink / raw)
  To: David Miller; +Cc: jhs, daniel, jiri, netdev

On 7/13/15 1:04 PM, David Miller wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Mon, 13 Jul 2015 12:47:42 -0700
>
>> In all normal cases skb->users == 1, but pktgen is using trick:
>> atomic_add(burst, &skb->users);
>> so when testing something like:
>
> You can want pktgen rx (which is the only buggy case as far as I can
> see, TX is fine) to run fast, but you must do so by abiding by the
> appropriate SKB sharing rules.
>
> You can't do an optimization in pktgen for RX processing that works
> "some of the time".  We have shared SKB rules for a reason.
>
> And I don't want to have to explain to someone in the future why that
> drop check is there, and have to tell them "because pktgen is broken
> and we decided to add a hack here rather than make pktgen send
> properly formed SKBs into the RX path"
>
> Ok?

in general all makes sense, but it is both RX and TX.
Without burst hack we cannot achieve line rate TX.
         atomic_add(burst, &pkt_dev->skb->users);
xmit_more:
         ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);

in pktgen we check that driver can work with users > 1 via:
pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING

so real hw driver are mostly ready for users > 1, it's only
few tc actions struggle a bit.
We cannot check tc actions from pktgen, since they can be added
dynamically.
So I see three options:
1 get rid of burst hack for both RX and TX in pktgen (kills performance)
2 add unlikely(skb_shread) check to few tc actions
3 do nothing

I think 2 isn't that bad after all if properly documented with
"because pktgen is doing this hack for performance" ?

I'm fine with 3 too, since the whole pktgen business is for root
and for kernel hackers who suppose to know what they're doing.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 20:17       ` Alexei Starovoitov
@ 2015-07-13 20:55         ` Daniel Borkmann
  2015-07-13 22:26           ` Alexei Starovoitov
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Borkmann @ 2015-07-13 20:55 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: David Miller, jhs, jiri, netdev

On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
...
> We cannot check tc actions from pktgen, since they can be added
> dynamically.
> So I see three options:
> 1 get rid of burst hack for both RX and TX in pktgen (kills performance)
> 2 add unlikely(skb_shread) check to few tc actions
> 3 do nothing
>
> I think 2 isn't that bad after all if properly documented with
> "because pktgen is doing this hack for performance" ?
>
> I'm fine with 3 too, since the whole pktgen business is for root
> and for kernel hackers who suppose to know what they're doing.

Hmm, one thing for option 3 could be that we add a modinfo tag
"experimental", so that on loading of pktgen module, we trigger
(like in case of staging) ...

   add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK);

... and add a pr_warn() to the user, it may be more visible/clear
than the "Packet Generator (USE WITH CAUTION)" Kconfig title? ;)

It'd be a pity that we'd need the extra atomic read only for the
pktgen case. :/ With regards to option 2, you could hide that behind
a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
that is a veeeery ugly workaround/hack as well (and distros might
even ship it nevertheless). I wouldn't be surprised if there are
other usage combinations with pktgen that would crash your box. :/

Best,
Daniel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 20:55         ` Daniel Borkmann
@ 2015-07-13 22:26           ` Alexei Starovoitov
  2015-07-14 10:29             ` Daniel Borkmann
  2015-07-14 22:34             ` David Miller
  0 siblings, 2 replies; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-13 22:26 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: David Miller, jhs, jiri, netdev

On 7/13/15 1:55 PM, Daniel Borkmann wrote:
> On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
> ...
>> We cannot check tc actions from pktgen, since they can be added
>> dynamically.
>> So I see three options:
>> 1 get rid of burst hack for both RX and TX in pktgen (kills performance)
>> 2 add unlikely(skb_shread) check to few tc actions
>> 3 do nothing
...
> pktgen case. :/ With regards to option 2, you could hide that behind
> a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
> that is a veeeery ugly workaround/hack as well (and distros might
> even ship it nevertheless).

naming such helper is a headache as well.
static inline bool is_pktgen_shared_skb(struct sk_buff *skb)
{
#if IS_ENABLED(CONFIG_NET_PKTGEN)
	/* pktgen uses skb->users += burst trick to reuse skb */
	return skb_shared(skb);
#else
	return false;
#endif
}
and in actions:
if (unlikely(is_pktgen_shared_skb(skb))) goto drop;

thoughts?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 22:26           ` Alexei Starovoitov
@ 2015-07-14 10:29             ` Daniel Borkmann
  2015-07-14 11:57               ` Jamal Hadi Salim
  2015-07-14 15:46               ` Alexei Starovoitov
  2015-07-14 22:34             ` David Miller
  1 sibling, 2 replies; 16+ messages in thread
From: Daniel Borkmann @ 2015-07-14 10:29 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: David Miller, jhs, jiri, netdev

On 07/14/2015 12:26 AM, Alexei Starovoitov wrote:
> On 7/13/15 1:55 PM, Daniel Borkmann wrote:
>> On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
>> ...
>>> We cannot check tc actions from pktgen, since they can be added
>>> dynamically.
>>> So I see three options:
>>> 1 get rid of burst hack for both RX and TX in pktgen (kills performance)
>>> 2 add unlikely(skb_shread) check to few tc actions
>>> 3 do nothing
> ...
>> pktgen case. :/ With regards to option 2, you could hide that behind
>> a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
>> that is a veeeery ugly workaround/hack as well (and distros might
>> even ship it nevertheless).
>
> naming such helper is a headache as well.
> static inline bool is_pktgen_shared_skb(struct sk_buff *skb)
> {
> #if IS_ENABLED(CONFIG_NET_PKTGEN)
>      /* pktgen uses skb->users += burst trick to reuse skb */
>      return skb_shared(skb);
> #else
>      return false;
> #endif
> }
> and in actions:
> if (unlikely(is_pktgen_shared_skb(skb))) goto drop;
>
> thoughts?

As I mentioned above, so Fedora, for example, ships pktgen by default.
That means, we'd run into the above test for shared skb in every case,
meaning it won't help much and it's also a pretty nasty hack. ;)

One other thing that comes to mind, not sure if it's worth it though,
would be to split the skb->tc_verd's TC_NCLS itself into TC_NCLS/TC_NACT,
so that you can go into the classifier, but skip the action part.

Since in tcf_action_exec(), we already test for that, you might be able
to add this with no extra cost. pktgen would then need to tag its skb
with TC_NACT, so that you'll always return with TC_ACT_OK. And if you
really would want to test tc actions, then w/o pktgen bursting ...

Best,
Daniel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-14 10:29             ` Daniel Borkmann
@ 2015-07-14 11:57               ` Jamal Hadi Salim
  2015-07-14 12:19                 ` Daniel Borkmann
  2015-07-14 15:46               ` Alexei Starovoitov
  1 sibling, 1 reply; 16+ messages in thread
From: Jamal Hadi Salim @ 2015-07-14 11:57 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov; +Cc: David Miller, jiri, netdev

On 07/14/15 06:29, Daniel Borkmann wrote:
> On 07/14/2015 12:26 AM, Alexei Starovoitov wrote:
>> On 7/13/15 1:55 PM, Daniel Borkmann wrote:
>>> On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
>>> ...
>>>> We cannot check tc actions from pktgen, since they can be added
>>>> dynamically.
>>>> So I see three options:
>>>> 1 get rid of burst hack for both RX and TX in pktgen (kills
>>>> performance)
>>>> 2 add unlikely(skb_shread) check to few tc actions
>>>> 3 do nothing
>> ...
>>> pktgen case. :/ With regards to option 2, you could hide that behind
>>> a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
>>> that is a veeeery ugly workaround/hack as well (and distros might
>>> even ship it nevertheless).
>>
>> naming such helper is a headache as well.
>> static inline bool is_pktgen_shared_skb(struct sk_buff *skb)
>> {
>> #if IS_ENABLED(CONFIG_NET_PKTGEN)
>>      /* pktgen uses skb->users += burst trick to reuse skb */
>>      return skb_shared(skb);
>> #else
>>      return false;
>> #endif
>> }
>> and in actions:
>> if (unlikely(is_pktgen_shared_skb(skb))) goto drop;
>>
>> thoughts?
>
> As I mentioned above, so Fedora, for example, ships pktgen by default.
> That means, we'd run into the above test for shared skb in every case,
> meaning it won't help much and it's also a pretty nasty hack. ;)
>
> One other thing that comes to mind, not sure if it's worth it though,
> would be to split the skb->tc_verd's TC_NCLS itself into TC_NCLS/TC_NACT,
> so that you can go into the classifier, but skip the action part.
>
> Since in tcf_action_exec(), we already test for that, you might be able
> to add this with no extra cost. pktgen would then need to tag its skb
> with TC_NACT, so that you'll always return with TC_ACT_OK. And if you
> really would want to test tc actions, then w/o pktgen bursting ...
>

Would just a simple skb->mark not work? Drop if skb->mark = x
using skbedit. Or a brand new pktgen_burst_mode action that drops?

cheers,
jamal

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-14 11:57               ` Jamal Hadi Salim
@ 2015-07-14 12:19                 ` Daniel Borkmann
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Borkmann @ 2015-07-14 12:19 UTC (permalink / raw)
  To: Jamal Hadi Salim, Alexei Starovoitov; +Cc: David Miller, jiri, netdev

On 07/14/2015 01:57 PM, Jamal Hadi Salim wrote:
> On 07/14/15 06:29, Daniel Borkmann wrote:
>> On 07/14/2015 12:26 AM, Alexei Starovoitov wrote:
>>> On 7/13/15 1:55 PM, Daniel Borkmann wrote:
>>>> On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
>>>> ...
>>>>> We cannot check tc actions from pktgen, since they can be added
>>>>> dynamically.
>>>>> So I see three options:
>>>>> 1 get rid of burst hack for both RX and TX in pktgen (kills
>>>>> performance)
>>>>> 2 add unlikely(skb_shread) check to few tc actions
>>>>> 3 do nothing
>>> ...
>>>> pktgen case. :/ With regards to option 2, you could hide that behind
>>>> a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
>>>> that is a veeeery ugly workaround/hack as well (and distros might
>>>> even ship it nevertheless).
>>>
>>> naming such helper is a headache as well.
>>> static inline bool is_pktgen_shared_skb(struct sk_buff *skb)
>>> {
>>> #if IS_ENABLED(CONFIG_NET_PKTGEN)
>>>      /* pktgen uses skb->users += burst trick to reuse skb */
>>>      return skb_shared(skb);
>>> #else
>>>      return false;
>>> #endif
>>> }
>>> and in actions:
>>> if (unlikely(is_pktgen_shared_skb(skb))) goto drop;
>>>
>>> thoughts?
>>
>> As I mentioned above, so Fedora, for example, ships pktgen by default.
>> That means, we'd run into the above test for shared skb in every case,
>> meaning it won't help much and it's also a pretty nasty hack. ;)
>>
>> One other thing that comes to mind, not sure if it's worth it though,
>> would be to split the skb->tc_verd's TC_NCLS itself into TC_NCLS/TC_NACT,
>> so that you can go into the classifier, but skip the action part.
>>
>> Since in tcf_action_exec(), we already test for that, you might be able
>> to add this with no extra cost. pktgen would then need to tag its skb
>> with TC_NACT, so that you'll always return with TC_ACT_OK. And if you
>> really would want to test tc actions, then w/o pktgen bursting ...
>
> Would just a simple skb->mark not work? Drop if skb->mark = x
> using skbedit. Or a brand new pktgen_burst_mode action that drops?

I think it's more about the fact that something could BUG() when used with
pktgen, otherwise you could just generally drop after classification.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-14 10:29             ` Daniel Borkmann
  2015-07-14 11:57               ` Jamal Hadi Salim
@ 2015-07-14 15:46               ` Alexei Starovoitov
  1 sibling, 0 replies; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-14 15:46 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: David Miller, jhs, jiri, netdev

On 7/14/15 3:29 AM, Daniel Borkmann wrote:
> One other thing that comes to mind, not sure if it's worth it though,
> would be to split the skb->tc_verd's TC_NCLS itself into TC_NCLS/TC_NACT,
> so that you can go into the classifier, but skip the action part.
>
> Since in tcf_action_exec(), we already test for that, you might be able
> to add this with no extra cost. pktgen would then need to tag its skb
> with TC_NACT, so that you'll always return with TC_ACT_OK. And if you
> really would want to test tc actions, then w/o pktgen bursting ...

imo it's even uglier. Majority of the actions are fine with shared skb,
so blank disable is no good at all.
The cost of 'unlikely(is_pktgen_shared_skb' is tiny, but fine,
we dug up the dirt enough.
I'm taking option 3 (do nothing) at this point.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-13 22:26           ` Alexei Starovoitov
  2015-07-14 10:29             ` Daniel Borkmann
@ 2015-07-14 22:34             ` David Miller
  2015-07-14 23:08               ` Alexei Starovoitov
  1 sibling, 1 reply; 16+ messages in thread
From: David Miller @ 2015-07-14 22:34 UTC (permalink / raw)
  To: ast; +Cc: daniel, jhs, jiri, netdev

From: Alexei Starovoitov <ast@plumgrid.com>
Date: Mon, 13 Jul 2015 15:26:46 -0700

> On 7/13/15 1:55 PM, Daniel Borkmann wrote:
>> On 07/13/2015 10:17 PM, Alexei Starovoitov wrote:
>> ...
>>> We cannot check tc actions from pktgen, since they can be added
>>> dynamically.
>>> So I see three options:
>>> 1 get rid of burst hack for both RX and TX in pktgen (kills
>>> performance)

#1 is a serious consideration if you don't come up with better ideas,
since an optimization is for nothing if it knowingly breaks things.

>>> 2 add unlikely(skb_shread) check to few tc actions
>>> 3 do nothing
> ...
>> pktgen case. :/ With regards to option 2, you could hide that behind
>> a static inline helper wrapped in IS_ENABLED(CONFIG_NET_PKTGEN), but
>> that is a veeeery ugly workaround/hack as well (and distros might
>> even ship it nevertheless).
> 
> naming such helper is a headache as well.
> static inline bool is_pktgen_shared_skb(struct sk_buff *skb)
> {
> #if IS_ENABLED(CONFIG_NET_PKTGEN)

As mentioned by others, this ifdef accomplishes nothing as indeed
every distribution turns pktgen on so %99.9999 of all users will not
benefit from this optimized check.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-14 22:34             ` David Miller
@ 2015-07-14 23:08               ` Alexei Starovoitov
  2015-07-15  0:58                 ` John Fastabend
  0 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-14 23:08 UTC (permalink / raw)
  To: David Miller; +Cc: daniel, jhs, jiri, netdev

On 7/14/15 3:34 PM, David Miller wrote:
>>>> 1 get rid of burst hack for both RX and TX in pktgen (kills
>>>> >>>performance)
> #1 is a serious consideration if you don't come up with better ideas,
> since an optimization is for nothing if it knowingly breaks things.

I've dug up the pktgen source from 2002 and see:
   atomic_inc(&skb->users);
   odev->hard_start_xmit(skb, odev);
so it did this trick forever.
Looks like it's a fundamental way how pktgen was working
and working still. Even when new 'burst' feature is not used,
pktgen still increments skb->users to hold skb.
At present I don't have good ideas how to redesign pktgen
and since apparently no one noticed this tc_action+pktgen
breakage for years, it's probably ok to leave everything as-is until
better ideas come. I'm not giving up yet. Just ran out of ideas.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-14 23:08               ` Alexei Starovoitov
@ 2015-07-15  0:58                 ` John Fastabend
  2015-07-15  1:01                   ` Alexei Starovoitov
  0 siblings, 1 reply; 16+ messages in thread
From: John Fastabend @ 2015-07-15  0:58 UTC (permalink / raw)
  To: Alexei Starovoitov, David Miller; +Cc: daniel, jhs, jiri, netdev

On 15-07-14 04:08 PM, Alexei Starovoitov wrote:
> On 7/14/15 3:34 PM, David Miller wrote:
>>>>> 1 get rid of burst hack for both RX and TX in pktgen (kills
>>>>> >>>performance)
>> #1 is a serious consideration if you don't come up with better ideas,
>> since an optimization is for nothing if it knowingly breaks things.
> 
> I've dug up the pktgen source from 2002 and see:
>   atomic_inc(&skb->users);
>   odev->hard_start_xmit(skb, odev);
> so it did this trick forever.
> Looks like it's a fundamental way how pktgen was working
> and working still. Even when new 'burst' feature is not used,
> pktgen still increments skb->users to hold skb.
> At present I don't have good ideas how to redesign pktgen
> and since apparently no one noticed this tc_action+pktgen
> breakage for years, it's probably ok to leave everything as-is until
> better ideas come. I'm not giving up yet. Just ran out of ideas.
> 

Right and we hit this issue when pktgen is run over any stacked device
with clone_skb set. I've always put it in the don't do this category but
a fix would be nice.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH net-next] tc: fix tc actions in case of shared skb
  2015-07-15  0:58                 ` John Fastabend
@ 2015-07-15  1:01                   ` Alexei Starovoitov
  0 siblings, 0 replies; 16+ messages in thread
From: Alexei Starovoitov @ 2015-07-15  1:01 UTC (permalink / raw)
  To: John Fastabend, David Miller; +Cc: daniel, jhs, jiri, netdev

On 7/14/15 5:58 PM, John Fastabend wrote:
> Right and we hit this issue when pktgen is run over any stacked device
> with clone_skb set. I've always put it in the don't do this category but
> a fix would be nice.

hmm, are you sure that commit
52d6c8c6ca12 ("net: pktgen: disable xmit_clone on virtual devices")
didn't fix it already?

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-07-15  1:02 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-11  0:10 [PATCH net-next] tc: fix tc actions in case of shared skb Alexei Starovoitov
2015-07-12  4:29 ` David Miller
2015-07-13 19:47   ` Alexei Starovoitov
2015-07-13 20:04     ` David Miller
2015-07-13 20:17       ` Alexei Starovoitov
2015-07-13 20:55         ` Daniel Borkmann
2015-07-13 22:26           ` Alexei Starovoitov
2015-07-14 10:29             ` Daniel Borkmann
2015-07-14 11:57               ` Jamal Hadi Salim
2015-07-14 12:19                 ` Daniel Borkmann
2015-07-14 15:46               ` Alexei Starovoitov
2015-07-14 22:34             ` David Miller
2015-07-14 23:08               ` Alexei Starovoitov
2015-07-15  0:58                 ` John Fastabend
2015-07-15  1:01                   ` Alexei Starovoitov
2015-07-13 13:13 ` Jamal Hadi Salim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.