Commit 31c417c
ip_gre, ip6_gre: Fix race condition on o_seqno in collect_md mode
As pointed out by Jakub Kicinski, currently using TUNNEL_SEQ in
collect_md mode is racy for [IP6]GRE[TAP] devices. Consider the
following sequence of events:
1. An [IP6]GRE[TAP] device is created in collect_md mode using "ip link
add ... external". "ip" ignores "[o]seq" if "external" is specified,
so TUNNEL_SEQ is off, and the device is marked as NETIF_F_LLTX (i.e.
it uses lockless TX);
2. Someone sets TUNNEL_SEQ on outgoing skb's, using e.g.
bpf_skb_set_tunnel_key() in an eBPF program attached to this device;
3. gre_fb_xmit() or __gre6_xmit() processes these skb's:
gre_build_header(skb, tun_hlen,
flags, protocol,
tunnel_id_to_key32(tun_info->key.tun_id),
(flags & TUNNEL_SEQ) ? htonl(tunnel->o_seqno++)
: 0); ^^^^^^^^^^^^^^^^^
Since we are not using the TX lock (&txq->_xmit_lock), multiple CPUs may
try to do this tunnel->o_seqno++ in parallel, which is racy. Fix it by
making o_seqno atomic_t.
As mentioned by Eric Dumazet in commit b790e01 ("ip_gre: lockless
xmit"), making o_seqno atomic_t increases "chance for packets being out
of order at receiver" when NETIF_F_LLTX is on.
Maybe a better fix would be:
1. Do not ignore "oseq" in external mode. Users MUST specify "oseq" if
they want the kernel to allow sequencing of outgoing packets;
2. Reject all outgoing TUNNEL_SEQ packets if the device was not created
with "oseq".
Unfortunately, that would break userspace.
We could now make [IP6]GRE[TAP] devices always NETIF_F_LLTX, but let us
do it in separate patches to keep this fix minimal.
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Fixes: 77a5196 ("gre: add sequence number for collect md mode.")
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
Acked-by: William Tu <u9012063@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>1 parent fde98ae commit 31c417c
4 files changed
Lines changed: 9 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
116 | 116 | | |
117 | 117 | | |
118 | 118 | | |
119 | | - | |
| 119 | + | |
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
464 | 464 | | |
465 | 465 | | |
466 | 466 | | |
467 | | - | |
| 467 | + | |
468 | 468 | | |
469 | 469 | | |
470 | 470 | | |
| |||
502 | 502 | | |
503 | 503 | | |
504 | 504 | | |
505 | | - | |
| 505 | + | |
506 | 506 | | |
507 | 507 | | |
508 | 508 | | |
| |||
579 | 579 | | |
580 | 580 | | |
581 | 581 | | |
582 | | - | |
| 582 | + | |
583 | 583 | | |
584 | 584 | | |
585 | 585 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
766 | 766 | | |
767 | 767 | | |
768 | 768 | | |
769 | | - | |
| 769 | + | |
770 | 770 | | |
771 | 771 | | |
772 | 772 | | |
| |||
777 | 777 | | |
778 | 778 | | |
779 | 779 | | |
780 | | - | |
| 780 | + | |
| 781 | + | |
781 | 782 | | |
782 | 783 | | |
783 | 784 | | |
| |||
1055 | 1056 | | |
1056 | 1057 | | |
1057 | 1058 | | |
1058 | | - | |
| 1059 | + | |
1059 | 1060 | | |
1060 | 1061 | | |
1061 | 1062 | | |
| |||
0 commit comments