From: Alexei Starovoitov <ast@plumgrid.com>
To: David Miller <davem@davemloft.net>
Cc: jhs@mojatatu.com, daniel@iogearbox.net, jiri@resnulli.us,
netdev@vger.kernel.org
Subject: Re: [PATCH net-next] tc: fix tc actions in case of shared skb
Date: Mon, 13 Jul 2015 12:47:42 -0700 [thread overview]
Message-ID: <55A415DE.8020806@plumgrid.com> (raw)
In-Reply-To: <20150711.212917.1463596559900301434.davem@davemloft.net>
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.
next prev parent reply other threads:[~2015-07-13 19:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A415DE.8020806@plumgrid.com \
--to=ast@plumgrid.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.