[PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[Petr Machata]

This series mends some problems in headroom management in ip6_gre
module. The current code base has the following three closely-related
problems:

- ip6gretap tunnels neglect to ensure there's enough writable headroom
  before pushing GRE headers.

- ip6erspan does this, but assumes that dev->needed_headroom is primed.
  But that doesn't happen until ip6_tnl_xmit() is called later. Thus for
  the first packet, ip6erspan actually behaves like ip6gretap above.

- ip6erspan shares some of the code with ip6gretap, including
  calculations of needed header length. While there is custom
  ERSPAN-specific code for calculating the headroom, the computed
  values are overwritten by the ip6gretap code.

The first two issues lead to a kernel panic in situations where a packet
is mirrored from a veth device to the device in question. They are
fixed, respectively, in patches #1 and #2, which include the full panic
trace and a reproducer.

The rest of the patchset deals with the last issue. In patches #3 to #6,
several functions are split up into reusable parts. Finally in patch #7
these blocks are used to compose ERSPAN-specific callbacks where
necessary to fix the hlen calculation.

Petr Machata (7):
  net: ip6_gre: Request headroom in __gre6_xmit()
  net: ip6_gre: Fix headroom request in ip6erspan_tunnel_xmit()
  net: ip6_gre: Split up ip6gre_tnl_link_config()
  net: ip6_gre: Split up ip6gre_tnl_change()
  net: ip6_gre: Split up ip6gre_newlink()
  net: ip6_gre: Split up ip6gre_changelink()
  net: ip6_gre: Fix ip6erspan hlen calculation

 net/ipv6/ip6_gre.c | 184 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 145 insertions(+), 39 deletions(-)

-- 
2.4.11

[PATCH net 1/7] net: ip6_gre: Request headroom in __gre6_xmit()

[Petr Machata]

__gre6_xmit() pushes GRE headers before handing over to ip6_tnl_xmit()
for generic IP-in-IP processing. However it doesn't make sure that there
is enough headroom to push the header to. That can lead to the panic
cited below. (Reproducer below that).

Fix by requesting either needed_headroom if already primed, or just the
bare minimum needed for the header otherwise.

[  158.576725] kernel BUG at net/core/skbuff.c:104!
[  158.581510] invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
[  158.587174] Modules linked in: act_mirred cls_matchall ip6_gre ip6_tunnel tunnel6 gre sch_ingress vrf veth x86_pkg_temp_thermal mlx_platform nfsd e1000e leds_mlxcpld
[  158.602268] CPU: 1 PID: 16 Comm: ksoftirqd/1 Not tainted 4.17.0-rc4-net_master-custom-139 #10
[  158.610938] Hardware name: Mellanox Technologies Ltd. "MSN2410-CB2F"/"SA000874", BIOS 4.6.5 03/08/2016
[  158.620426] RIP: 0010:skb_panic+0xc3/0x100
[  158.624586] RSP: 0018:ffff8801d3f27110 EFLAGS: 00010286
[  158.629882] RAX: 0000000000000082 RBX: ffff8801c02cc040 RCX: 0000000000000000
[  158.637127] RDX: 0000000000000082 RSI: dffffc0000000000 RDI: ffffed003a7e4e18
[  158.644366] RBP: ffff8801bfec8020 R08: ffffed003aabce19 R09: ffffed003aabce19
[  158.651574] R10: 000000000000000b R11: ffffed003aabce18 R12: ffff8801c364de66
[  158.658786] R13: 000000000000002c R14: 00000000000000c0 R15: ffff8801c364de68
[  158.666007] FS:  0000000000000000(0000) GS:ffff8801d5400000(0000) knlGS:0000000000000000
[  158.674212] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  158.680036] CR2: 00007f4b3702dcd0 CR3: 0000000003228002 CR4: 00000000001606e0
[  158.687228] Call Trace:
[  158.689752]  ? __gre6_xmit+0x246/0xd80 [ip6_gre]
[  158.694475]  ? __gre6_xmit+0x246/0xd80 [ip6_gre]
[  158.699141]  skb_push+0x78/0x90
[  158.702344]  __gre6_xmit+0x246/0xd80 [ip6_gre]
[  158.706872]  ip6gre_tunnel_xmit+0x3bc/0x610 [ip6_gre]
[  158.711992]  ? __gre6_xmit+0xd80/0xd80 [ip6_gre]
[  158.716668]  ? debug_check_no_locks_freed+0x210/0x210
[  158.721761]  ? print_irqtrace_events+0x120/0x120
[  158.726461]  ? sched_clock_cpu+0x18/0x210
[  158.730572]  ? sched_clock_cpu+0x18/0x210
[  158.734692]  ? cyc2ns_read_end+0x10/0x10
[  158.738705]  ? skb_network_protocol+0x76/0x200
[  158.743216]  ? netif_skb_features+0x1b2/0x550
[  158.747648]  dev_hard_start_xmit+0x137/0x770
[  158.752010]  sch_direct_xmit+0x2ef/0x5d0
[  158.755992]  ? pfifo_fast_dequeue+0x3fa/0x670
[  158.760460]  ? pfifo_fast_change_tx_queue_len+0x810/0x810
[  158.765975]  ? __lock_is_held+0xa0/0x160
[  158.770002]  __qdisc_run+0x39e/0xfc0
[  158.773673]  ? _raw_spin_unlock+0x29/0x40
[  158.777781]  ? pfifo_fast_enqueue+0x24b/0x3e0
[  158.782191]  ? sch_direct_xmit+0x5d0/0x5d0
[  158.786372]  ? pfifo_fast_dequeue+0x670/0x670
[  158.790818]  ? __dev_queue_xmit+0x172/0x1770
[  158.795195]  ? preempt_count_sub+0xf/0xd0
[  158.799313]  __dev_queue_xmit+0x410/0x1770
[  158.803512]  ? ___slab_alloc+0x605/0x930
[  158.807525]  ? ___slab_alloc+0x605/0x930
[  158.811540]  ? memcpy+0x34/0x50
[  158.814768]  ? netdev_pick_tx+0x1c0/0x1c0
[  158.818895]  ? __skb_clone+0x2fd/0x3d0
[  158.822712]  ? __copy_skb_header+0x270/0x270
[  158.827079]  ? rcu_read_lock_sched_held+0x93/0xa0
[  158.831903]  ? kmem_cache_alloc+0x344/0x4d0
[  158.836199]  ? skb_clone+0x123/0x230
[  158.839869]  ? skb_split+0x820/0x820
[  158.843521]  ? tcf_mirred+0x554/0x930 [act_mirred]
[  158.848407]  tcf_mirred+0x554/0x930 [act_mirred]
[  158.853104]  ? tcf_mirred_act_wants_ingress.part.2+0x10/0x10 [act_mirred]
[  158.860005]  ? __lock_acquire+0x706/0x26e0
[  158.864162]  ? mark_lock+0x13d/0xb40
[  158.867832]  tcf_action_exec+0xcf/0x2a0
[  158.871736]  tcf_classify+0xfa/0x340
[  158.875402]  __netif_receive_skb_core+0x8e1/0x1c60
[  158.880334]  ? nf_ingress+0x500/0x500
[  158.884059]  ? process_backlog+0x347/0x4b0
[  158.888241]  ? lock_acquire+0xd8/0x320
[  158.892050]  ? process_backlog+0x1b6/0x4b0
[  158.896228]  ? process_backlog+0xc2/0x4b0
[  158.900291]  process_backlog+0xc2/0x4b0
[  158.904210]  net_rx_action+0x5cc/0x980
[  158.908047]  ? napi_complete_done+0x2c0/0x2c0
[  158.912525]  ? rcu_read_unlock+0x80/0x80
[  158.916534]  ? __lock_is_held+0x34/0x160
[  158.920541]  __do_softirq+0x1d4/0x9d2
[  158.924308]  ? trace_event_raw_event_irq_handler_exit+0x140/0x140
[  158.930515]  run_ksoftirqd+0x1d/0x40
[  158.934152]  smpboot_thread_fn+0x32b/0x690
[  158.938299]  ? sort_range+0x20/0x20
[  158.941842]  ? preempt_count_sub+0xf/0xd0
[  158.945940]  ? schedule+0x5b/0x140
[  158.949412]  kthread+0x206/0x300
[  158.952689]  ? sort_range+0x20/0x20
[  158.956249]  ? kthread_stop+0x570/0x570
[  158.960164]  ret_from_fork+0x3a/0x50
[  158.963823] Code: 14 3e ff 8b 4b 78 55 4d 89 f9 41 56 41 55 48 c7 c7 a0 cf db 82 41 54 44 8b 44 24 2c 48 8b 54 24 30 48 8b 74 24 20 e8 16 94 13 ff <0f> 0b 48 c7 c7 60 8e 1f 85 48 83 c4 20 e8 55 ef a6 ff 89 74 24
[  158.983235] RIP: skb_panic+0xc3/0x100 RSP: ffff8801d3f27110
[  158.988935] ---[ end trace 5af56ee845aa6cc8 ]---
[  158.993641] Kernel panic - not syncing: Fatal exception in interrupt
[  159.000176] Kernel Offset: disabled
[  159.003767] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---

Reproducer:

	ip link add h1 type veth peer name swp1
	ip link add h3 type veth peer name swp3

	ip link set dev h1 up
	ip address add 192.0.2.1/28 dev h1

	ip link add dev vh3 type vrf table 20
	ip link set dev h3 master vh3
	ip link set dev vh3 up
	ip link set dev h3 up

	ip link set dev swp3 up
	ip address add dev swp3 2001:db8:2::1/64

	ip link set dev swp1 up
	tc qdisc add dev swp1 clsact

	ip link add name gt6 type ip6gretap \
		local 2001:db8:2::1 remote 2001:db8:2::2
	ip link set dev gt6 up

	sleep 1

	tc filter add dev swp1 ingress pref 1000 matchall skip_hw \
		action mirred egress mirror dev gt6
	ping -I h1 192.0.2.2

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 69727bc..5d93c7c 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -698,6 +698,9 @@ static netdev_tx_t __gre6_xmit(struct sk_buff *skb,
 	else
 		fl6->daddr = tunnel->parms.raddr;
 
+	if (skb_cow_head(skb, dev->needed_headroom ?: tunnel->hlen))
+		return -ENOMEM;
+
 	/* Push GRE header. */
 	protocol = (dev->type == ARPHRD_ETHER) ? htons(ETH_P_TEB) : proto;
 
-- 
2.4.11

[PATCH net 2/7] net: ip6_gre: Fix headroom request in ip6erspan_tunnel_xmit()

[Petr Machata]

dev->needed_headroom is not primed until ip6_tnl_xmit(), so it starts
out zero. Thus the call to skb_cow_head() fails to actually make sure
there's enough headroom to push the ERSPAN headers to. That can lead to
the panic cited below. (Reproducer below that).

Fix by requesting either needed_headroom if already primed, or just the
bare minimum needed for the header otherwise.

[  190.703567] kernel BUG at net/core/skbuff.c:104!
[  190.708384] invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
[  190.714007] Modules linked in: act_mirred cls_matchall ip6_gre ip6_tunnel tunnel6 gre sch_ingress vrf veth x86_pkg_temp_thermal mlx_platform nfsd e1000e leds_mlxcpld
[  190.728975] CPU: 1 PID: 959 Comm: kworker/1:2 Not tainted 4.17.0-rc4-net_master-custom-139 #10
[  190.737647] Hardware name: Mellanox Technologies Ltd. "MSN2410-CB2F"/"SA000874", BIOS 4.6.5 03/08/2016
[  190.747006] Workqueue: ipv6_addrconf addrconf_dad_work
[  190.752222] RIP: 0010:skb_panic+0xc3/0x100
[  190.756358] RSP: 0018:ffff8801d54072f0 EFLAGS: 00010282
[  190.761629] RAX: 0000000000000085 RBX: ffff8801c1a8ecc0 RCX: 0000000000000000
[  190.768830] RDX: 0000000000000085 RSI: dffffc0000000000 RDI: ffffed003aa80e54
[  190.776025] RBP: ffff8801bd1ec5a0 R08: ffffed003aabce19 R09: ffffed003aabce19
[  190.783226] R10: 0000000000000001 R11: ffffed003aabce18 R12: ffff8801bf695dbe
[  190.790418] R13: 0000000000000084 R14: 00000000000006c0 R15: ffff8801bf695dc8
[  190.797621] FS:  0000000000000000(0000) GS:ffff8801d5400000(0000) knlGS:0000000000000000
[  190.805786] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  190.811582] CR2: 000055fa929aced0 CR3: 0000000003228004 CR4: 00000000001606e0
[  190.818790] Call Trace:
[  190.821264]  <IRQ>
[  190.823314]  ? ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
[  190.828940]  ? ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
[  190.834562]  skb_push+0x78/0x90
[  190.837749]  ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
[  190.843219]  ? ip6gre_tunnel_ioctl+0xd90/0xd90 [ip6_gre]
[  190.848577]  ? debug_check_no_locks_freed+0x210/0x210
[  190.853679]  ? debug_check_no_locks_freed+0x210/0x210
[  190.858783]  ? print_irqtrace_events+0x120/0x120
[  190.863451]  ? sched_clock_cpu+0x18/0x210
[  190.867496]  ? cyc2ns_read_end+0x10/0x10
[  190.871474]  ? skb_network_protocol+0x76/0x200
[  190.875977]  dev_hard_start_xmit+0x137/0x770
[  190.880317]  ? do_raw_spin_trylock+0x6d/0xa0
[  190.884624]  sch_direct_xmit+0x2ef/0x5d0
[  190.888589]  ? pfifo_fast_dequeue+0x3fa/0x670
[  190.892994]  ? pfifo_fast_change_tx_queue_len+0x810/0x810
[  190.898455]  ? __lock_is_held+0xa0/0x160
[  190.902422]  __qdisc_run+0x39e/0xfc0
[  190.906041]  ? _raw_spin_unlock+0x29/0x40
[  190.910090]  ? pfifo_fast_enqueue+0x24b/0x3e0
[  190.914501]  ? sch_direct_xmit+0x5d0/0x5d0
[  190.918658]  ? pfifo_fast_dequeue+0x670/0x670
[  190.923047]  ? __dev_queue_xmit+0x172/0x1770
[  190.927365]  ? preempt_count_sub+0xf/0xd0
[  190.931421]  __dev_queue_xmit+0x410/0x1770
[  190.935553]  ? ___slab_alloc+0x605/0x930
[  190.939524]  ? print_irqtrace_events+0x120/0x120
[  190.944186]  ? memcpy+0x34/0x50
[  190.947364]  ? netdev_pick_tx+0x1c0/0x1c0
[  190.951428]  ? __skb_clone+0x2fd/0x3d0
[  190.955218]  ? __copy_skb_header+0x270/0x270
[  190.959537]  ? rcu_read_lock_sched_held+0x93/0xa0
[  190.964282]  ? kmem_cache_alloc+0x344/0x4d0
[  190.968520]  ? cyc2ns_read_end+0x10/0x10
[  190.972495]  ? skb_clone+0x123/0x230
[  190.976112]  ? skb_split+0x820/0x820
[  190.979747]  ? tcf_mirred+0x554/0x930 [act_mirred]
[  190.984582]  tcf_mirred+0x554/0x930 [act_mirred]
[  190.989252]  ? tcf_mirred_act_wants_ingress.part.2+0x10/0x10 [act_mirred]
[  190.996109]  ? __lock_acquire+0x706/0x26e0
[  191.000239]  ? sched_clock_cpu+0x18/0x210
[  191.004294]  tcf_action_exec+0xcf/0x2a0
[  191.008179]  tcf_classify+0xfa/0x340
[  191.011794]  __netif_receive_skb_core+0x8e1/0x1c60
[  191.016630]  ? debug_check_no_locks_freed+0x210/0x210
[  191.021732]  ? nf_ingress+0x500/0x500
[  191.025458]  ? process_backlog+0x347/0x4b0
[  191.029619]  ? print_irqtrace_events+0x120/0x120
[  191.034302]  ? lock_acquire+0xd8/0x320
[  191.038089]  ? process_backlog+0x1b6/0x4b0
[  191.042246]  ? process_backlog+0xc2/0x4b0
[  191.046303]  process_backlog+0xc2/0x4b0
[  191.050189]  net_rx_action+0x5cc/0x980
[  191.053991]  ? napi_complete_done+0x2c0/0x2c0
[  191.058386]  ? mark_lock+0x13d/0xb40
[  191.062001]  ? clockevents_program_event+0x6b/0x1d0
[  191.066922]  ? print_irqtrace_events+0x120/0x120
[  191.071593]  ? __lock_is_held+0xa0/0x160
[  191.075566]  __do_softirq+0x1d4/0x9d2
[  191.079282]  ? ip6_finish_output2+0x524/0x1460
[  191.083771]  do_softirq_own_stack+0x2a/0x40
[  191.087994]  </IRQ>
[  191.090130]  do_softirq.part.13+0x38/0x40
[  191.094178]  __local_bh_enable_ip+0x135/0x190
[  191.098591]  ip6_finish_output2+0x54d/0x1460
[  191.102916]  ? ip6_forward_finish+0x2f0/0x2f0
[  191.107314]  ? ip6_mtu+0x3c/0x2c0
[  191.110674]  ? ip6_finish_output+0x2f8/0x650
[  191.114992]  ? ip6_output+0x12a/0x500
[  191.118696]  ip6_output+0x12a/0x500
[  191.122223]  ? ip6_route_dev_notify+0x5b0/0x5b0
[  191.126807]  ? ip6_finish_output+0x650/0x650
[  191.131120]  ? ip6_fragment+0x1a60/0x1a60
[  191.135182]  ? icmp6_dst_alloc+0x26e/0x470
[  191.139317]  mld_sendpack+0x672/0x830
[  191.143021]  ? igmp6_mcf_seq_next+0x2f0/0x2f0
[  191.147429]  ? __local_bh_enable_ip+0x77/0x190
[  191.151913]  ipv6_mc_dad_complete+0x47/0x90
[  191.156144]  addrconf_dad_completed+0x561/0x720
[  191.160731]  ? addrconf_rs_timer+0x3a0/0x3a0
[  191.165036]  ? mark_held_locks+0xc9/0x140
[  191.169095]  ? __local_bh_enable_ip+0x77/0x190
[  191.173570]  ? addrconf_dad_work+0x50d/0xa20
[  191.177886]  ? addrconf_dad_work+0x529/0xa20
[  191.182194]  addrconf_dad_work+0x529/0xa20
[  191.186342]  ? addrconf_dad_completed+0x720/0x720
[  191.191088]  ? __lock_is_held+0xa0/0x160
[  191.195059]  ? process_one_work+0x45d/0xe20
[  191.199302]  ? process_one_work+0x51e/0xe20
[  191.203531]  ? rcu_read_lock_sched_held+0x93/0xa0
[  191.208279]  process_one_work+0x51e/0xe20
[  191.212340]  ? pwq_dec_nr_in_flight+0x200/0x200
[  191.216912]  ? get_lock_stats+0x4b/0xf0
[  191.220788]  ? preempt_count_sub+0xf/0xd0
[  191.224844]  ? worker_thread+0x219/0x860
[  191.228823]  ? do_raw_spin_trylock+0x6d/0xa0
[  191.233142]  worker_thread+0xeb/0x860
[  191.236848]  ? process_one_work+0xe20/0xe20
[  191.241095]  kthread+0x206/0x300
[  191.244352]  ? process_one_work+0xe20/0xe20
[  191.248587]  ? kthread_stop+0x570/0x570
[  191.252459]  ret_from_fork+0x3a/0x50
[  191.256082] Code: 14 3e ff 8b 4b 78 55 4d 89 f9 41 56 41 55 48 c7 c7 a0 cf db 82 41 54 44 8b 44 24 2c 48 8b 54 24 30 48 8b 74 24 20 e8 16 94 13 ff <0f> 0b 48 c7 c7 60 8e 1f 85 48 83 c4 20 e8 55 ef a6 ff 89 74 24
[  191.275327] RIP: skb_panic+0xc3/0x100 RSP: ffff8801d54072f0
[  191.281024] ---[ end trace 7ea51094e099e006 ]---
[  191.285724] Kernel panic - not syncing: Fatal exception in interrupt
[  191.292168] Kernel Offset: disabled
[  191.295697] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---

Reproducer:

	ip link add h1 type veth peer name swp1
	ip link add h3 type veth peer name swp3

	ip link set dev h1 up
	ip address add 192.0.2.1/28 dev h1

	ip link add dev vh3 type vrf table 20
	ip link set dev h3 master vh3
	ip link set dev vh3 up
	ip link set dev h3 up

	ip link set dev swp3 up
	ip address add dev swp3 2001:db8:2::1/64

	ip link set dev swp1 up
	tc qdisc add dev swp1 clsact

	ip link add name gt6 type ip6erspan \
		local 2001:db8:2::1 remote 2001:db8:2::2 oseq okey 123
	ip link set dev gt6 up

	sleep 1

	tc filter add dev swp1 ingress pref 1000 matchall skip_hw \
		action mirred egress mirror dev gt6
	ping -I h1 192.0.2.2

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 5d93c7c..53b1531 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -911,7 +911,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		truncate = true;
 	}
 
-	if (skb_cow_head(skb, dev->needed_headroom))
+	if (skb_cow_head(skb, dev->needed_headroom ?: t->hlen))
 		goto tx_err;
 
 	t->parms.o_flags &= ~TUNNEL_KEY;
-- 
2.4.11

[PATCH net 3/7] net: ip6_gre: Split up ip6gre_tnl_link_config()

[Petr Machata]

The function ip6gre_tnl_link_config() is used for setting up
configuration of both ip6gretap and ip6erspan tunnels. Split the
function into the common part and the route-lookup part. The latter then
takes the calculated header length as an argument. This split will allow
the patches down the line to sneak in a custom header length computation
for the ERSPAN tunnel.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 53b1531..78ba6b9 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1022,12 +1022,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
+static void ip6gre_tnl_link_config_common(struct ip6_tnl *t)
 {
 	struct net_device *dev = t->dev;
 	struct __ip6_tnl_parm *p = &t->parms;
 	struct flowi6 *fl6 = &t->fl.u.ip6;
-	int t_hlen;
 
 	if (dev->type != ARPHRD_ETHER) {
 		memcpy(dev->dev_addr, &p->laddr, sizeof(struct in6_addr));
@@ -1054,12 +1053,13 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
 		dev->flags |= IFF_POINTOPOINT;
 	else
 		dev->flags &= ~IFF_POINTOPOINT;
+}
 
-	t->tun_hlen = gre_calc_hlen(t->parms.o_flags);
-
-	t->hlen = t->encap_hlen + t->tun_hlen;
-
-	t_hlen = t->hlen + sizeof(struct ipv6hdr);
+static void ip6gre_tnl_link_config_route(struct ip6_tnl *t, int set_mtu,
+					 int t_hlen)
+{
+	const struct __ip6_tnl_parm *p = &t->parms;
+	struct net_device *dev = t->dev;
 
 	if (p->flags & IP6_TNL_F_CAP_XMIT) {
 		int strict = (ipv6_addr_type(&p->raddr) &
@@ -1091,6 +1091,24 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
 	}
 }
 
+static int ip6gre_calc_hlen(struct ip6_tnl *tunnel)
+{
+	int t_hlen;
+
+	tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
+	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
+
+	t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
+	tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
+	return t_hlen;
+}
+
+static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
+{
+	ip6gre_tnl_link_config_common(t);
+	ip6gre_tnl_link_config_route(t, set_mtu, ip6gre_calc_hlen(t));
+}
+
 static int ip6gre_tnl_change(struct ip6_tnl *t,
 	const struct __ip6_tnl_parm *p, int set_mtu)
 {
@@ -1384,11 +1402,7 @@ static int ip6gre_tunnel_init_common(struct net_device *dev)
 		return ret;
 	}
 
-	tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
-	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
-	t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
-
-	dev->hard_header_len = LL_MAX_HEADER + t_hlen;
+	t_hlen = ip6gre_calc_hlen(tunnel);
 	dev->mtu = ETH_DATA_LEN - t_hlen;
 	if (dev->type == ARPHRD_ETHER)
 		dev->mtu -= ETH_HLEN;
-- 
2.4.11

[PATCH net 4/7] net: ip6_gre: Split up ip6gre_tnl_change()

[Petr Machata]

Split a reusable function ip6gre_tnl_copy_tnl_parm() from
ip6gre_tnl_change(). This will allow ERSPAN-specific code to
reuse the common parts while customizing the behavior for ERSPAN.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 78ba6b9..307ac6d 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1109,8 +1109,8 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
 	ip6gre_tnl_link_config_route(t, set_mtu, ip6gre_calc_hlen(t));
 }
 
-static int ip6gre_tnl_change(struct ip6_tnl *t,
-	const struct __ip6_tnl_parm *p, int set_mtu)
+static void ip6gre_tnl_copy_tnl_parm(struct ip6_tnl *t,
+				     const struct __ip6_tnl_parm *p)
 {
 	t->parms.laddr = p->laddr;
 	t->parms.raddr = p->raddr;
@@ -1126,6 +1126,12 @@ static int ip6gre_tnl_change(struct ip6_tnl *t,
 	t->parms.o_flags = p->o_flags;
 	t->parms.fwmark = p->fwmark;
 	dst_cache_reset(&t->dst_cache);
+}
+
+static int ip6gre_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p,
+			     int set_mtu)
+{
+	ip6gre_tnl_copy_tnl_parm(t, p);
 	ip6gre_tnl_link_config(t, set_mtu);
 	return 0;
 }
-- 
2.4.11

[PATCH net 5/7] net: ip6_gre: Split up ip6gre_newlink()

[Petr Machata]

Extract from ip6gre_newlink() a reusable function
ip6gre_newlink_common(). The ip6gre_tnl_link_config() call needs to be
made customizable for ERSPAN, thus reorder it with calls to
ip6_tnl_change_mtu() and dev_hold(), and extract the whole tail to the
caller, ip6gre_newlink(). Thus enable an ERSPAN-specific _newlink()
function without a lot of duplicity.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 307ac6d..4dfa21d 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1858,9 +1858,9 @@ static bool ip6gre_netlink_encap_parms(struct nlattr *data[],
 	return ret;
 }
 
-static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
-			  struct nlattr *tb[], struct nlattr *data[],
-			  struct netlink_ext_ack *extack)
+static int ip6gre_newlink_common(struct net *src_net, struct net_device *dev,
+				 struct nlattr *tb[], struct nlattr *data[],
+				 struct netlink_ext_ack *extack)
 {
 	struct ip6_tnl *nt;
 	struct net *net = dev_net(dev);
@@ -1897,18 +1897,30 @@ static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
 	if (err)
 		goto out;
 
-	ip6gre_tnl_link_config(nt, !tb[IFLA_MTU]);
-
 	if (tb[IFLA_MTU])
 		ip6_tnl_change_mtu(dev, nla_get_u32(tb[IFLA_MTU]));
 
 	dev_hold(dev);
-	ip6gre_tunnel_link(ign, nt);
 
 out:
 	return err;
 }
 
+static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
+			  struct nlattr *tb[], struct nlattr *data[],
+			  struct netlink_ext_ack *extack)
+{
+	int err = ip6gre_newlink_common(src_net, dev, tb, data, extack);
+	struct ip6_tnl *nt = netdev_priv(dev);
+	struct net *net = dev_net(dev);
+
+	if (!err) {
+		ip6gre_tnl_link_config(nt, !tb[IFLA_MTU]);
+		ip6gre_tunnel_link(net_generic(net, ip6gre_net_id), nt);
+	}
+	return err;
+}
+
 static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
 			     struct nlattr *data[],
 			     struct netlink_ext_ack *extack)
-- 
2.4.11

[PATCH net 6/7] net: ip6_gre: Split up ip6gre_changelink()

[Petr Machata]

Extract from ip6gre_changelink() a reusable function
ip6gre_changelink_common(). This will allow introduction of
ERSPAN-specific _changelink() function with not a lot of code
duplication.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 4dfa21d..c17e38b 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1921,37 +1921,52 @@ static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
 	return err;
 }
 
-static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
-			     struct nlattr *data[],
-			     struct netlink_ext_ack *extack)
+static struct ip6_tnl *
+ip6gre_changelink_common(struct net_device *dev, struct nlattr *tb[],
+			 struct nlattr *data[], struct __ip6_tnl_parm *p_p,
+			 struct netlink_ext_ack *extack)
 {
 	struct ip6_tnl *t, *nt = netdev_priv(dev);
 	struct net *net = nt->net;
 	struct ip6gre_net *ign = net_generic(net, ip6gre_net_id);
-	struct __ip6_tnl_parm p;
 	struct ip_tunnel_encap ipencap;
 
 	if (dev == ign->fb_tunnel_dev)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	if (ip6gre_netlink_encap_parms(data, &ipencap)) {
 		int err = ip6_tnl_encap_setup(nt, &ipencap);
 
 		if (err < 0)
-			return err;
+			return ERR_PTR(err);
 	}
 
-	ip6gre_netlink_parms(data, &p);
+	ip6gre_netlink_parms(data, p_p);
 
-	t = ip6gre_tunnel_locate(net, &p, 0);
+	t = ip6gre_tunnel_locate(net, p_p, 0);
 
 	if (t) {
 		if (t->dev != dev)
-			return -EEXIST;
+			return ERR_PTR(-EEXIST);
 	} else {
 		t = nt;
 	}
 
+	return t;
+}
+
+static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
+			     struct nlattr *data[],
+			     struct netlink_ext_ack *extack)
+{
+	struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
+	struct __ip6_tnl_parm p;
+	struct ip6_tnl *t;
+
+	t = ip6gre_changelink_common(dev, tb, data, &p, extack);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
 	ip6gre_tunnel_unlink(ign, t);
 	ip6gre_tnl_change(t, &p, !tb[IFLA_MTU]);
 	ip6gre_tunnel_link(ign, t);
-- 
2.4.11

[PATCH net 7/7] net: ip6_gre: Fix ip6erspan hlen calculation

[Petr Machata]

Even though ip6erspan_tap_init() sets up hlen and tun_hlen according to
what ERSPAN needs, it goes ahead to call ip6gre_tnl_link_config() which
overwrites these settings with GRE-specific ones.

Similarly for changelink callbacks, which are handled by
ip6gre_changelink() calls ip6gre_tnl_change() calls
ip6gre_tnl_link_config() as well.

The difference ends up being 12 vs. 20 bytes, and this is generally not
a problem, because a 12-byte request likely ends up allocating more and
the extra 8 bytes are thus available. However correct it is not.

So replace the newlink and changelink callbacks with an ERSPAN-specific
ones, reusing the newly-introduced _common() functions.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 net/ipv6/ip6_gre.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index c17e38b..36b1669 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -81,6 +81,7 @@ static int ip6gre_tunnel_init(struct net_device *dev);
 static void ip6gre_tunnel_setup(struct net_device *dev);
 static void ip6gre_tunnel_link(struct ip6gre_net *ign, struct ip6_tnl *t);
 static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu);
+static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu);
 
 /* Tunnel hash table */
 
@@ -1751,6 +1752,19 @@ static const struct net_device_ops ip6gre_tap_netdev_ops = {
 	.ndo_get_iflink = ip6_tnl_get_iflink,
 };
 
+static int ip6erspan_calc_hlen(struct ip6_tnl *tunnel)
+{
+	int t_hlen;
+
+	tunnel->tun_hlen = 8;
+	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
+		       erspan_hdr_len(tunnel->parms.erspan_ver);
+
+	t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
+	tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
+	return t_hlen;
+}
+
 static int ip6erspan_tap_init(struct net_device *dev)
 {
 	struct ip6_tnl *tunnel;
@@ -1774,12 +1788,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
 		return ret;
 	}
 
-	tunnel->tun_hlen = 8;
-	tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
-		       erspan_hdr_len(tunnel->parms.erspan_ver);
-	t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
-
-	dev->hard_header_len = LL_MAX_HEADER + t_hlen;
+	t_hlen = ip6erspan_calc_hlen(tunnel);
 	dev->mtu = ETH_DATA_LEN - t_hlen;
 	if (dev->type == ARPHRD_ETHER)
 		dev->mtu -= ETH_HLEN;
@@ -1787,7 +1796,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
 		dev->mtu -= 8;
 
 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
-	ip6gre_tnl_link_config(tunnel, 1);
+	ip6erspan_tnl_link_config(tunnel, 1);
 
 	return 0;
 }
@@ -2118,6 +2127,53 @@ static void ip6erspan_tap_setup(struct net_device *dev)
 	netif_keep_dst(dev);
 }
 
+static int ip6erspan_newlink(struct net *src_net, struct net_device *dev,
+			     struct nlattr *tb[], struct nlattr *data[],
+			     struct netlink_ext_ack *extack)
+{
+	int err = ip6gre_newlink_common(src_net, dev, tb, data, extack);
+	struct ip6_tnl *nt = netdev_priv(dev);
+	struct net *net = dev_net(dev);
+
+	if (!err) {
+		ip6erspan_tnl_link_config(nt, !tb[IFLA_MTU]);
+		ip6gre_tunnel_link(net_generic(net, ip6gre_net_id), nt);
+	}
+	return err;
+}
+
+static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu)
+{
+	ip6gre_tnl_link_config_common(t);
+	ip6gre_tnl_link_config_route(t, set_mtu, ip6erspan_calc_hlen(t));
+}
+
+static int ip6erspan_tnl_change(struct ip6_tnl *t,
+				const struct __ip6_tnl_parm *p, int set_mtu)
+{
+	ip6gre_tnl_copy_tnl_parm(t, p);
+	ip6erspan_tnl_link_config(t, set_mtu);
+	return 0;
+}
+
+static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
+				struct nlattr *data[],
+				struct netlink_ext_ack *extack)
+{
+	struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
+	struct __ip6_tnl_parm p;
+	struct ip6_tnl *t;
+
+	t = ip6gre_changelink_common(dev, tb, data, &p, extack);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+
+	ip6gre_tunnel_unlink(ign, t);
+	ip6erspan_tnl_change(t, &p, !tb[IFLA_MTU]);
+	ip6gre_tunnel_link(ign, t);
+	return 0;
+}
+
 static struct rtnl_link_ops ip6gre_link_ops __read_mostly = {
 	.kind		= "ip6gre",
 	.maxtype	= IFLA_GRE_MAX,
@@ -2154,8 +2210,8 @@ static struct rtnl_link_ops ip6erspan_tap_ops __read_mostly = {
 	.priv_size	= sizeof(struct ip6_tnl),
 	.setup		= ip6erspan_tap_setup,
 	.validate	= ip6erspan_tap_validate,
-	.newlink	= ip6gre_newlink,
-	.changelink	= ip6gre_changelink,
+	.newlink	= ip6erspan_newlink,
+	.changelink	= ip6erspan_changelink,
 	.get_size	= ip6gre_get_size,
 	.fill_info	= ip6gre_fill_info,
 	.get_link_net	= ip6_tnl_get_link_net,
-- 
2.4.11

Re: [PATCH net 1/7] net: ip6_gre: Request headroom in __gre6_xmit()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> __gre6_xmit() pushes GRE headers before handing over to ip6_tnl_xmit()
> for generic IP-in-IP processing. However it doesn't make sure that there
> is enough headroom to push the header to. That can lead to the panic
> cited below. (Reproducer below that).
>
> Fix by requesting either needed_headroom if already primed, or just the
> bare minimum needed for the header otherwise.
>
> [  158.576725] kernel BUG at net/core/skbuff.c:104!
> [  158.581510] invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
> [  158.587174] Modules linked in: act_mirred cls_matchall ip6_gre ip6_tunnel tunnel6 gre sch_ingress vrf veth x86_pkg_temp_thermal mlx_platform nfsd e1000e leds_mlxcpld
> [  158.602268] CPU: 1 PID: 16 Comm: ksoftirqd/1 Not tainted 4.17.0-rc4-net_master-custom-139 #10
> [  158.610938] Hardware name: Mellanox Technologies Ltd. "MSN2410-CB2F"/"SA000874", BIOS 4.6.5 03/08/2016
> [  158.620426] RIP: 0010:skb_panic+0xc3/0x100
> [  158.624586] RSP: 0018:ffff8801d3f27110 EFLAGS: 00010286
> [  158.629882] RAX: 0000000000000082 RBX: ffff8801c02cc040 RCX: 0000000000000000
> [  158.637127] RDX: 0000000000000082 RSI: dffffc0000000000 RDI: ffffed003a7e4e18
> [  158.644366] RBP: ffff8801bfec8020 R08: ffffed003aabce19 R09: ffffed003aabce19
> [  158.651574] R10: 000000000000000b R11: ffffed003aabce18 R12: ffff8801c364de66
> [  158.658786] R13: 000000000000002c R14: 00000000000000c0 R15: ffff8801c364de68
> [  158.666007] FS:  0000000000000000(0000) GS:ffff8801d5400000(0000) knlGS:0000000000000000
> [  158.674212] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  158.680036] CR2: 00007f4b3702dcd0 CR3: 0000000003228002 CR4: 00000000001606e0
> [  158.687228] Call Trace:
> [  158.689752]  ? __gre6_xmit+0x246/0xd80 [ip6_gre]
> [  158.694475]  ? __gre6_xmit+0x246/0xd80 [ip6_gre]
> [  158.699141]  skb_push+0x78/0x90
> [  158.702344]  __gre6_xmit+0x246/0xd80 [ip6_gre]
> [  158.706872]  ip6gre_tunnel_xmit+0x3bc/0x610 [ip6_gre]
> [  158.711992]  ? __gre6_xmit+0xd80/0xd80 [ip6_gre]
> [  158.716668]  ? debug_check_no_locks_freed+0x210/0x210
> [  158.721761]  ? print_irqtrace_events+0x120/0x120
> [  158.726461]  ? sched_clock_cpu+0x18/0x210
> [  158.730572]  ? sched_clock_cpu+0x18/0x210
> [  158.734692]  ? cyc2ns_read_end+0x10/0x10
> [  158.738705]  ? skb_network_protocol+0x76/0x200
> [  158.743216]  ? netif_skb_features+0x1b2/0x550
> [  158.747648]  dev_hard_start_xmit+0x137/0x770
> [  158.752010]  sch_direct_xmit+0x2ef/0x5d0
> [  158.755992]  ? pfifo_fast_dequeue+0x3fa/0x670
> [  158.760460]  ? pfifo_fast_change_tx_queue_len+0x810/0x810
> [  158.765975]  ? __lock_is_held+0xa0/0x160
> [  158.770002]  __qdisc_run+0x39e/0xfc0
> [  158.773673]  ? _raw_spin_unlock+0x29/0x40
> [  158.777781]  ? pfifo_fast_enqueue+0x24b/0x3e0
> [  158.782191]  ? sch_direct_xmit+0x5d0/0x5d0
> [  158.786372]  ? pfifo_fast_dequeue+0x670/0x670
> [  158.790818]  ? __dev_queue_xmit+0x172/0x1770
> [  158.795195]  ? preempt_count_sub+0xf/0xd0
> [  158.799313]  __dev_queue_xmit+0x410/0x1770
> [  158.803512]  ? ___slab_alloc+0x605/0x930
> [  158.807525]  ? ___slab_alloc+0x605/0x930
> [  158.811540]  ? memcpy+0x34/0x50
> [  158.814768]  ? netdev_pick_tx+0x1c0/0x1c0
> [  158.818895]  ? __skb_clone+0x2fd/0x3d0
> [  158.822712]  ? __copy_skb_header+0x270/0x270
> [  158.827079]  ? rcu_read_lock_sched_held+0x93/0xa0
> [  158.831903]  ? kmem_cache_alloc+0x344/0x4d0
> [  158.836199]  ? skb_clone+0x123/0x230
> [  158.839869]  ? skb_split+0x820/0x820
> [  158.843521]  ? tcf_mirred+0x554/0x930 [act_mirred]
> [  158.848407]  tcf_mirred+0x554/0x930 [act_mirred]
> [  158.853104]  ? tcf_mirred_act_wants_ingress.part.2+0x10/0x10 [act_mirred]
> [  158.860005]  ? __lock_acquire+0x706/0x26e0
> [  158.864162]  ? mark_lock+0x13d/0xb40
> [  158.867832]  tcf_action_exec+0xcf/0x2a0
> [  158.871736]  tcf_classify+0xfa/0x340
> [  158.875402]  __netif_receive_skb_core+0x8e1/0x1c60
> [  158.880334]  ? nf_ingress+0x500/0x500
> [  158.884059]  ? process_backlog+0x347/0x4b0
> [  158.888241]  ? lock_acquire+0xd8/0x320
> [  158.892050]  ? process_backlog+0x1b6/0x4b0
> [  158.896228]  ? process_backlog+0xc2/0x4b0
> [  158.900291]  process_backlog+0xc2/0x4b0
> [  158.904210]  net_rx_action+0x5cc/0x980
> [  158.908047]  ? napi_complete_done+0x2c0/0x2c0
> [  158.912525]  ? rcu_read_unlock+0x80/0x80
> [  158.916534]  ? __lock_is_held+0x34/0x160
> [  158.920541]  __do_softirq+0x1d4/0x9d2
> [  158.924308]  ? trace_event_raw_event_irq_handler_exit+0x140/0x140
> [  158.930515]  run_ksoftirqd+0x1d/0x40
> [  158.934152]  smpboot_thread_fn+0x32b/0x690
> [  158.938299]  ? sort_range+0x20/0x20
> [  158.941842]  ? preempt_count_sub+0xf/0xd0
> [  158.945940]  ? schedule+0x5b/0x140
> [  158.949412]  kthread+0x206/0x300
> [  158.952689]  ? sort_range+0x20/0x20
> [  158.956249]  ? kthread_stop+0x570/0x570
> [  158.960164]  ret_from_fork+0x3a/0x50
> [  158.963823] Code: 14 3e ff 8b 4b 78 55 4d 89 f9 41 56 41 55 48 c7 c7 a0 cf db 82 41 54 44 8b 44 24 2c 48 8b 54 24 30 48 8b 74 24 20 e8 16 94 13 ff <0f> 0b 48 c7 c7 60 8e 1f 85 48 83 c4 20 e8 55 ef a6 ff 89 74 24
> [  158.983235] RIP: skb_panic+0xc3/0x100 RSP: ffff8801d3f27110
> [  158.988935] ---[ end trace 5af56ee845aa6cc8 ]---
> [  158.993641] Kernel panic - not syncing: Fatal exception in interrupt
> [  159.000176] Kernel Offset: disabled
> [  159.003767] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
>
> Reproducer:
>
>         ip link add h1 type veth peer name swp1
>         ip link add h3 type veth peer name swp3
>
>         ip link set dev h1 up
>         ip address add 192.0.2.1/28 dev h1
>
>         ip link add dev vh3 type vrf table 20
>         ip link set dev h3 master vh3
>         ip link set dev vh3 up
>         ip link set dev h3 up
>
>         ip link set dev swp3 up
>         ip address add dev swp3 2001:db8:2::1/64
>
>         ip link set dev swp1 up
>         tc qdisc add dev swp1 clsact
>
>         ip link add name gt6 type ip6gretap \
>                 local 2001:db8:2::1 remote 2001:db8:2::2
>         ip link set dev gt6 up
>
>         sleep 1
>
>         tc filter add dev swp1 ingress pref 1000 matchall skip_hw \
>                 action mirred egress mirror dev gt6
>         ping -I h1 192.0.2.2
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

Thanks for the fix.
Acked-by: William Tu <u9012063@gmail.com>

>  net/ipv6/ip6_gre.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 69727bc..5d93c7c 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -698,6 +698,9 @@ static netdev_tx_t __gre6_xmit(struct sk_buff *skb,
>         else
>                 fl6->daddr = tunnel->parms.raddr;
>
> +       if (skb_cow_head(skb, dev->needed_headroom ?: tunnel->hlen))
> +               return -ENOMEM;
> +
>         /* Push GRE header. */
>         protocol = (dev->type == ARPHRD_ETHER) ? htons(ETH_P_TEB) : proto;
>
> --
> 2.4.11
>

Re: [PATCH net 2/7] net: ip6_gre: Fix headroom request in ip6erspan_tunnel_xmit()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> dev->needed_headroom is not primed until ip6_tnl_xmit(), so it starts
> out zero. Thus the call to skb_cow_head() fails to actually make sure
> there's enough headroom to push the ERSPAN headers to. That can lead to
> the panic cited below. (Reproducer below that).
>
> Fix by requesting either needed_headroom if already primed, or just the
> bare minimum needed for the header otherwise.
>
> [  190.703567] kernel BUG at net/core/skbuff.c:104!
> [  190.708384] invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
> [  190.714007] Modules linked in: act_mirred cls_matchall ip6_gre ip6_tunnel tunnel6 gre sch_ingress vrf veth x86_pkg_temp_thermal mlx_platform nfsd e1000e leds_mlxcpld
> [  190.728975] CPU: 1 PID: 959 Comm: kworker/1:2 Not tainted 4.17.0-rc4-net_master-custom-139 #10
> [  190.737647] Hardware name: Mellanox Technologies Ltd. "MSN2410-CB2F"/"SA000874", BIOS 4.6.5 03/08/2016
> [  190.747006] Workqueue: ipv6_addrconf addrconf_dad_work
> [  190.752222] RIP: 0010:skb_panic+0xc3/0x100
> [  190.756358] RSP: 0018:ffff8801d54072f0 EFLAGS: 00010282
> [  190.761629] RAX: 0000000000000085 RBX: ffff8801c1a8ecc0 RCX: 0000000000000000
> [  190.768830] RDX: 0000000000000085 RSI: dffffc0000000000 RDI: ffffed003aa80e54
> [  190.776025] RBP: ffff8801bd1ec5a0 R08: ffffed003aabce19 R09: ffffed003aabce19
> [  190.783226] R10: 0000000000000001 R11: ffffed003aabce18 R12: ffff8801bf695dbe
> [  190.790418] R13: 0000000000000084 R14: 00000000000006c0 R15: ffff8801bf695dc8
> [  190.797621] FS:  0000000000000000(0000) GS:ffff8801d5400000(0000) knlGS:0000000000000000
> [  190.805786] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  190.811582] CR2: 000055fa929aced0 CR3: 0000000003228004 CR4: 00000000001606e0
> [  190.818790] Call Trace:
> [  190.821264]  <IRQ>
> [  190.823314]  ? ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
> [  190.828940]  ? ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
> [  190.834562]  skb_push+0x78/0x90
> [  190.837749]  ip6erspan_tunnel_xmit+0x5e4/0x1982 [ip6_gre]
> [  190.843219]  ? ip6gre_tunnel_ioctl+0xd90/0xd90 [ip6_gre]
> [  190.848577]  ? debug_check_no_locks_freed+0x210/0x210
> [  190.853679]  ? debug_check_no_locks_freed+0x210/0x210
> [  190.858783]  ? print_irqtrace_events+0x120/0x120
> [  190.863451]  ? sched_clock_cpu+0x18/0x210
> [  190.867496]  ? cyc2ns_read_end+0x10/0x10
> [  190.871474]  ? skb_network_protocol+0x76/0x200
> [  190.875977]  dev_hard_start_xmit+0x137/0x770
> [  190.880317]  ? do_raw_spin_trylock+0x6d/0xa0
> [  190.884624]  sch_direct_xmit+0x2ef/0x5d0
> [  190.888589]  ? pfifo_fast_dequeue+0x3fa/0x670
> [  190.892994]  ? pfifo_fast_change_tx_queue_len+0x810/0x810
> [  190.898455]  ? __lock_is_held+0xa0/0x160
> [  190.902422]  __qdisc_run+0x39e/0xfc0
> [  190.906041]  ? _raw_spin_unlock+0x29/0x40
> [  190.910090]  ? pfifo_fast_enqueue+0x24b/0x3e0
> [  190.914501]  ? sch_direct_xmit+0x5d0/0x5d0
> [  190.918658]  ? pfifo_fast_dequeue+0x670/0x670
> [  190.923047]  ? __dev_queue_xmit+0x172/0x1770
> [  190.927365]  ? preempt_count_sub+0xf/0xd0
> [  190.931421]  __dev_queue_xmit+0x410/0x1770
> [  190.935553]  ? ___slab_alloc+0x605/0x930
> [  190.939524]  ? print_irqtrace_events+0x120/0x120
> [  190.944186]  ? memcpy+0x34/0x50
> [  190.947364]  ? netdev_pick_tx+0x1c0/0x1c0
> [  190.951428]  ? __skb_clone+0x2fd/0x3d0
> [  190.955218]  ? __copy_skb_header+0x270/0x270
> [  190.959537]  ? rcu_read_lock_sched_held+0x93/0xa0
> [  190.964282]  ? kmem_cache_alloc+0x344/0x4d0
> [  190.968520]  ? cyc2ns_read_end+0x10/0x10
> [  190.972495]  ? skb_clone+0x123/0x230
> [  190.976112]  ? skb_split+0x820/0x820
> [  190.979747]  ? tcf_mirred+0x554/0x930 [act_mirred]
> [  190.984582]  tcf_mirred+0x554/0x930 [act_mirred]
> [  190.989252]  ? tcf_mirred_act_wants_ingress.part.2+0x10/0x10 [act_mirred]
> [  190.996109]  ? __lock_acquire+0x706/0x26e0
> [  191.000239]  ? sched_clock_cpu+0x18/0x210
> [  191.004294]  tcf_action_exec+0xcf/0x2a0
> [  191.008179]  tcf_classify+0xfa/0x340
> [  191.011794]  __netif_receive_skb_core+0x8e1/0x1c60
> [  191.016630]  ? debug_check_no_locks_freed+0x210/0x210
> [  191.021732]  ? nf_ingress+0x500/0x500
> [  191.025458]  ? process_backlog+0x347/0x4b0
> [  191.029619]  ? print_irqtrace_events+0x120/0x120
> [  191.034302]  ? lock_acquire+0xd8/0x320
> [  191.038089]  ? process_backlog+0x1b6/0x4b0
> [  191.042246]  ? process_backlog+0xc2/0x4b0
> [  191.046303]  process_backlog+0xc2/0x4b0
> [  191.050189]  net_rx_action+0x5cc/0x980
> [  191.053991]  ? napi_complete_done+0x2c0/0x2c0
> [  191.058386]  ? mark_lock+0x13d/0xb40
> [  191.062001]  ? clockevents_program_event+0x6b/0x1d0
> [  191.066922]  ? print_irqtrace_events+0x120/0x120
> [  191.071593]  ? __lock_is_held+0xa0/0x160
> [  191.075566]  __do_softirq+0x1d4/0x9d2
> [  191.079282]  ? ip6_finish_output2+0x524/0x1460
> [  191.083771]  do_softirq_own_stack+0x2a/0x40
> [  191.087994]  </IRQ>
> [  191.090130]  do_softirq.part.13+0x38/0x40
> [  191.094178]  __local_bh_enable_ip+0x135/0x190
> [  191.098591]  ip6_finish_output2+0x54d/0x1460
> [  191.102916]  ? ip6_forward_finish+0x2f0/0x2f0
> [  191.107314]  ? ip6_mtu+0x3c/0x2c0
> [  191.110674]  ? ip6_finish_output+0x2f8/0x650
> [  191.114992]  ? ip6_output+0x12a/0x500
> [  191.118696]  ip6_output+0x12a/0x500
> [  191.122223]  ? ip6_route_dev_notify+0x5b0/0x5b0
> [  191.126807]  ? ip6_finish_output+0x650/0x650
> [  191.131120]  ? ip6_fragment+0x1a60/0x1a60
> [  191.135182]  ? icmp6_dst_alloc+0x26e/0x470
> [  191.139317]  mld_sendpack+0x672/0x830
> [  191.143021]  ? igmp6_mcf_seq_next+0x2f0/0x2f0
> [  191.147429]  ? __local_bh_enable_ip+0x77/0x190
> [  191.151913]  ipv6_mc_dad_complete+0x47/0x90
> [  191.156144]  addrconf_dad_completed+0x561/0x720
> [  191.160731]  ? addrconf_rs_timer+0x3a0/0x3a0
> [  191.165036]  ? mark_held_locks+0xc9/0x140
> [  191.169095]  ? __local_bh_enable_ip+0x77/0x190
> [  191.173570]  ? addrconf_dad_work+0x50d/0xa20
> [  191.177886]  ? addrconf_dad_work+0x529/0xa20
> [  191.182194]  addrconf_dad_work+0x529/0xa20
> [  191.186342]  ? addrconf_dad_completed+0x720/0x720
> [  191.191088]  ? __lock_is_held+0xa0/0x160
> [  191.195059]  ? process_one_work+0x45d/0xe20
> [  191.199302]  ? process_one_work+0x51e/0xe20
> [  191.203531]  ? rcu_read_lock_sched_held+0x93/0xa0
> [  191.208279]  process_one_work+0x51e/0xe20
> [  191.212340]  ? pwq_dec_nr_in_flight+0x200/0x200
> [  191.216912]  ? get_lock_stats+0x4b/0xf0
> [  191.220788]  ? preempt_count_sub+0xf/0xd0
> [  191.224844]  ? worker_thread+0x219/0x860
> [  191.228823]  ? do_raw_spin_trylock+0x6d/0xa0
> [  191.233142]  worker_thread+0xeb/0x860
> [  191.236848]  ? process_one_work+0xe20/0xe20
> [  191.241095]  kthread+0x206/0x300
> [  191.244352]  ? process_one_work+0xe20/0xe20
> [  191.248587]  ? kthread_stop+0x570/0x570
> [  191.252459]  ret_from_fork+0x3a/0x50
> [  191.256082] Code: 14 3e ff 8b 4b 78 55 4d 89 f9 41 56 41 55 48 c7 c7 a0 cf db 82 41 54 44 8b 44 24 2c 48 8b 54 24 30 48 8b 74 24 20 e8 16 94 13 ff <0f> 0b 48 c7 c7 60 8e 1f 85 48 83 c4 20 e8 55 ef a6 ff 89 74 24
> [  191.275327] RIP: skb_panic+0xc3/0x100 RSP: ffff8801d54072f0
> [  191.281024] ---[ end trace 7ea51094e099e006 ]---
> [  191.285724] Kernel panic - not syncing: Fatal exception in interrupt
> [  191.292168] Kernel Offset: disabled
> [  191.295697] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
>
> Reproducer:
>
>         ip link add h1 type veth peer name swp1
>         ip link add h3 type veth peer name swp3
>
>         ip link set dev h1 up
>         ip address add 192.0.2.1/28 dev h1
>
>         ip link add dev vh3 type vrf table 20
>         ip link set dev h3 master vh3
>         ip link set dev vh3 up
>         ip link set dev h3 up
>
>         ip link set dev swp3 up
>         ip address add dev swp3 2001:db8:2::1/64
>
>         ip link set dev swp1 up
>         tc qdisc add dev swp1 clsact
>
>         ip link add name gt6 type ip6erspan \
>                 local 2001:db8:2::1 remote 2001:db8:2::2 oseq okey 123
>         ip link set dev gt6 up
>
>         sleep 1
>
>         tc filter add dev swp1 ingress pref 1000 matchall skip_hw \
>                 action mirred egress mirror dev gt6
>         ping -I h1 192.0.2.2
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

Acked-by: William Tu <u9012063@gmail.com>

>  net/ipv6/ip6_gre.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 5d93c7c..53b1531 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -911,7 +911,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
>                 truncate = true;
>         }
>
> -       if (skb_cow_head(skb, dev->needed_headroom))
> +       if (skb_cow_head(skb, dev->needed_headroom ?: t->hlen))
>                 goto tx_err;
>
>         t->parms.o_flags &= ~TUNNEL_KEY;
> --
> 2.4.11
>

Re: [PATCH net 3/7] net: ip6_gre: Split up ip6gre_tnl_link_config()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> The function ip6gre_tnl_link_config() is used for setting up
> configuration of both ip6gretap and ip6erspan tunnels. Split the
> function into the common part and the route-lookup part. The latter then
> takes the calculated header length as an argument. This split will allow
> the patches down the line to sneak in a custom header length computation
> for the ERSPAN tunnel.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

LGTM.
Acked-by: William Tu <u9012063@gmail.com>

>  net/ipv6/ip6_gre.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 53b1531..78ba6b9 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1022,12 +1022,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
>         return NETDEV_TX_OK;
>  }
>
> -static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
> +static void ip6gre_tnl_link_config_common(struct ip6_tnl *t)
>  {
>         struct net_device *dev = t->dev;
>         struct __ip6_tnl_parm *p = &t->parms;
>         struct flowi6 *fl6 = &t->fl.u.ip6;
> -       int t_hlen;
>
>         if (dev->type != ARPHRD_ETHER) {
>                 memcpy(dev->dev_addr, &p->laddr, sizeof(struct in6_addr));
> @@ -1054,12 +1053,13 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
>                 dev->flags |= IFF_POINTOPOINT;
>         else
>                 dev->flags &= ~IFF_POINTOPOINT;
> +}
>
> -       t->tun_hlen = gre_calc_hlen(t->parms.o_flags);
> -
> -       t->hlen = t->encap_hlen + t->tun_hlen;
> -
> -       t_hlen = t->hlen + sizeof(struct ipv6hdr);
> +static void ip6gre_tnl_link_config_route(struct ip6_tnl *t, int set_mtu,
> +                                        int t_hlen)
> +{
> +       const struct __ip6_tnl_parm *p = &t->parms;
> +       struct net_device *dev = t->dev;
>
>         if (p->flags & IP6_TNL_F_CAP_XMIT) {
>                 int strict = (ipv6_addr_type(&p->raddr) &
> @@ -1091,6 +1091,24 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
>         }
>  }
>
> +static int ip6gre_calc_hlen(struct ip6_tnl *tunnel)
> +{
> +       int t_hlen;
> +
> +       tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
> +       tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
> +
> +       t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> +       tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +       return t_hlen;
> +}
> +
> +static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
> +{
> +       ip6gre_tnl_link_config_common(t);
> +       ip6gre_tnl_link_config_route(t, set_mtu, ip6gre_calc_hlen(t));
> +}
> +
>  static int ip6gre_tnl_change(struct ip6_tnl *t,
>         const struct __ip6_tnl_parm *p, int set_mtu)
>  {
> @@ -1384,11 +1402,7 @@ static int ip6gre_tunnel_init_common(struct net_device *dev)
>                 return ret;
>         }
>
> -       tunnel->tun_hlen = gre_calc_hlen(tunnel->parms.o_flags);
> -       tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen;
> -       t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> -
> -       dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +       t_hlen = ip6gre_calc_hlen(tunnel);
>         dev->mtu = ETH_DATA_LEN - t_hlen;
>         if (dev->type == ARPHRD_ETHER)
>                 dev->mtu -= ETH_HLEN;
> --
> 2.4.11
>

Re: [PATCH net 4/7] net: ip6_gre: Split up ip6gre_tnl_change()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> Split a reusable function ip6gre_tnl_copy_tnl_parm() from
> ip6gre_tnl_change(). This will allow ERSPAN-specific code to
> reuse the common parts while customizing the behavior for ERSPAN.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

LGTM.
Acked-by: William Tu <u9012063@gmail.com>


>  net/ipv6/ip6_gre.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 78ba6b9..307ac6d 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1109,8 +1109,8 @@ static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu)
>         ip6gre_tnl_link_config_route(t, set_mtu, ip6gre_calc_hlen(t));
>  }
>
> -static int ip6gre_tnl_change(struct ip6_tnl *t,
> -       const struct __ip6_tnl_parm *p, int set_mtu)
> +static void ip6gre_tnl_copy_tnl_parm(struct ip6_tnl *t,
> +                                    const struct __ip6_tnl_parm *p)
>  {
>         t->parms.laddr = p->laddr;
>         t->parms.raddr = p->raddr;
> @@ -1126,6 +1126,12 @@ static int ip6gre_tnl_change(struct ip6_tnl *t,
>         t->parms.o_flags = p->o_flags;
>         t->parms.fwmark = p->fwmark;
>         dst_cache_reset(&t->dst_cache);
> +}
> +
> +static int ip6gre_tnl_change(struct ip6_tnl *t, const struct __ip6_tnl_parm *p,
> +                            int set_mtu)
> +{
> +       ip6gre_tnl_copy_tnl_parm(t, p);
>         ip6gre_tnl_link_config(t, set_mtu);
>         return 0;
>  }
> --
> 2.4.11
>

Re: [PATCH net 5/7] net: ip6_gre: Split up ip6gre_newlink()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> Extract from ip6gre_newlink() a reusable function
> ip6gre_newlink_common(). The ip6gre_tnl_link_config() call needs to be
> made customizable for ERSPAN, thus reorder it with calls to
> ip6_tnl_change_mtu() and dev_hold(), and extract the whole tail to the
> caller, ip6gre_newlink(). Thus enable an ERSPAN-specific _newlink()
> function without a lot of duplicity.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

LGTM.
Acked-by: William Tu <u9012063@gmail.com>

>  net/ipv6/ip6_gre.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 307ac6d..4dfa21d 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1858,9 +1858,9 @@ static bool ip6gre_netlink_encap_parms(struct nlattr *data[],
>         return ret;
>  }
>
> -static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
> -                         struct nlattr *tb[], struct nlattr *data[],
> -                         struct netlink_ext_ack *extack)
> +static int ip6gre_newlink_common(struct net *src_net, struct net_device *dev,
> +                                struct nlattr *tb[], struct nlattr *data[],
> +                                struct netlink_ext_ack *extack)
>  {
>         struct ip6_tnl *nt;
>         struct net *net = dev_net(dev);
> @@ -1897,18 +1897,30 @@ static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
>         if (err)
>                 goto out;
>
> -       ip6gre_tnl_link_config(nt, !tb[IFLA_MTU]);
> -
>         if (tb[IFLA_MTU])
>                 ip6_tnl_change_mtu(dev, nla_get_u32(tb[IFLA_MTU]));
>
>         dev_hold(dev);
> -       ip6gre_tunnel_link(ign, nt);
>
>  out:
>         return err;
>  }
>
> +static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
> +                         struct nlattr *tb[], struct nlattr *data[],
> +                         struct netlink_ext_ack *extack)
> +{
> +       int err = ip6gre_newlink_common(src_net, dev, tb, data, extack);
> +       struct ip6_tnl *nt = netdev_priv(dev);
> +       struct net *net = dev_net(dev);
> +
> +       if (!err) {
> +               ip6gre_tnl_link_config(nt, !tb[IFLA_MTU]);
> +               ip6gre_tunnel_link(net_generic(net, ip6gre_net_id), nt);
> +       }
> +       return err;
> +}
> +
>  static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
>                              struct nlattr *data[],
>                              struct netlink_ext_ack *extack)
> --
> 2.4.11
>

Re: [PATCH net 6/7] net: ip6_gre: Split up ip6gre_changelink()

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> Extract from ip6gre_changelink() a reusable function
> ip6gre_changelink_common(). This will allow introduction of
> ERSPAN-specific _changelink() function with not a lot of code
> duplication.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

LGTM.
Acked-by: William Tu <u9012063@gmail.com>


>  net/ipv6/ip6_gre.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 4dfa21d..c17e38b 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1921,37 +1921,52 @@ static int ip6gre_newlink(struct net *src_net, struct net_device *dev,
>         return err;
>  }
>
> -static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
> -                            struct nlattr *data[],
> -                            struct netlink_ext_ack *extack)
> +static struct ip6_tnl *
> +ip6gre_changelink_common(struct net_device *dev, struct nlattr *tb[],
> +                        struct nlattr *data[], struct __ip6_tnl_parm *p_p,
> +                        struct netlink_ext_ack *extack)
>  {
>         struct ip6_tnl *t, *nt = netdev_priv(dev);
>         struct net *net = nt->net;
>         struct ip6gre_net *ign = net_generic(net, ip6gre_net_id);
> -       struct __ip6_tnl_parm p;
>         struct ip_tunnel_encap ipencap;
>
>         if (dev == ign->fb_tunnel_dev)
> -               return -EINVAL;
> +               return ERR_PTR(-EINVAL);
>
>         if (ip6gre_netlink_encap_parms(data, &ipencap)) {
>                 int err = ip6_tnl_encap_setup(nt, &ipencap);
>
>                 if (err < 0)
> -                       return err;
> +                       return ERR_PTR(err);
>         }
>
> -       ip6gre_netlink_parms(data, &p);
> +       ip6gre_netlink_parms(data, p_p);
>
> -       t = ip6gre_tunnel_locate(net, &p, 0);
> +       t = ip6gre_tunnel_locate(net, p_p, 0);
>
>         if (t) {
>                 if (t->dev != dev)
> -                       return -EEXIST;
> +                       return ERR_PTR(-EEXIST);
>         } else {
>                 t = nt;
>         }
>
> +       return t;
> +}
> +
> +static int ip6gre_changelink(struct net_device *dev, struct nlattr *tb[],
> +                            struct nlattr *data[],
> +                            struct netlink_ext_ack *extack)
> +{
> +       struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
> +       struct __ip6_tnl_parm p;
> +       struct ip6_tnl *t;
> +
> +       t = ip6gre_changelink_common(dev, tb, data, &p, extack);
> +       if (IS_ERR(t))
> +               return PTR_ERR(t);
> +
>         ip6gre_tunnel_unlink(ign, t);
>         ip6gre_tnl_change(t, &p, !tb[IFLA_MTU]);
>         ip6gre_tunnel_link(ign, t);
> --
> 2.4.11
>

Re: [PATCH net 7/7] net: ip6_gre: Fix ip6erspan hlen calculation

[William Tu]

On Thu, May 17, 2018 at 7:36 AM, Petr Machata <petrm@mellanox.com> wrote:
> Even though ip6erspan_tap_init() sets up hlen and tun_hlen according to
> what ERSPAN needs, it goes ahead to call ip6gre_tnl_link_config() which
> overwrites these settings with GRE-specific ones.
>
> Similarly for changelink callbacks, which are handled by
> ip6gre_changelink() calls ip6gre_tnl_change() calls
> ip6gre_tnl_link_config() as well.
>
> The difference ends up being 12 vs. 20 bytes, and this is generally not
> a problem, because a 12-byte request likely ends up allocating more and
> the extra 8 bytes are thus available. However correct it is not.
>
> So replace the newlink and changelink callbacks with an ERSPAN-specific
> ones, reusing the newly-introduced _common() functions.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---

Acked-by: William Tu <u9012063@gmail.com>

Thanks, using ERSPAN-specific newlink and changelink is also on
my todo list.  I'm hitting another issue related to the shared collect_md_tun
between erspan and gre, I will make my patch based on this series.


>  net/ipv6/ip6_gre.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index c17e38b..36b1669 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -81,6 +81,7 @@ static int ip6gre_tunnel_init(struct net_device *dev);
>  static void ip6gre_tunnel_setup(struct net_device *dev);
>  static void ip6gre_tunnel_link(struct ip6gre_net *ign, struct ip6_tnl *t);
>  static void ip6gre_tnl_link_config(struct ip6_tnl *t, int set_mtu);
> +static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu);
>
>  /* Tunnel hash table */
>
> @@ -1751,6 +1752,19 @@ static const struct net_device_ops ip6gre_tap_netdev_ops = {
>         .ndo_get_iflink = ip6_tnl_get_iflink,
>  };
>
> +static int ip6erspan_calc_hlen(struct ip6_tnl *tunnel)
> +{
> +       int t_hlen;
> +
> +       tunnel->tun_hlen = 8;
> +       tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
> +                      erspan_hdr_len(tunnel->parms.erspan_ver);
> +
> +       t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> +       tunnel->dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +       return t_hlen;
> +}
> +
>  static int ip6erspan_tap_init(struct net_device *dev)
>  {
>         struct ip6_tnl *tunnel;
> @@ -1774,12 +1788,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
>                 return ret;
>         }
>
> -       tunnel->tun_hlen = 8;
> -       tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
> -                      erspan_hdr_len(tunnel->parms.erspan_ver);
> -       t_hlen = tunnel->hlen + sizeof(struct ipv6hdr);
> -
> -       dev->hard_header_len = LL_MAX_HEADER + t_hlen;
> +       t_hlen = ip6erspan_calc_hlen(tunnel);
>         dev->mtu = ETH_DATA_LEN - t_hlen;
>         if (dev->type == ARPHRD_ETHER)
>                 dev->mtu -= ETH_HLEN;
> @@ -1787,7 +1796,7 @@ static int ip6erspan_tap_init(struct net_device *dev)
>                 dev->mtu -= 8;
>
>         dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> -       ip6gre_tnl_link_config(tunnel, 1);
> +       ip6erspan_tnl_link_config(tunnel, 1);
>
>         return 0;
>  }
> @@ -2118,6 +2127,53 @@ static void ip6erspan_tap_setup(struct net_device *dev)
>         netif_keep_dst(dev);
>  }
>
> +static int ip6erspan_newlink(struct net *src_net, struct net_device *dev,
> +                            struct nlattr *tb[], struct nlattr *data[],
> +                            struct netlink_ext_ack *extack)
> +{
> +       int err = ip6gre_newlink_common(src_net, dev, tb, data, extack);
> +       struct ip6_tnl *nt = netdev_priv(dev);
> +       struct net *net = dev_net(dev);
> +
> +       if (!err) {
> +               ip6erspan_tnl_link_config(nt, !tb[IFLA_MTU]);
> +               ip6gre_tunnel_link(net_generic(net, ip6gre_net_id), nt);
> +       }
> +       return err;
> +}
> +
> +static void ip6erspan_tnl_link_config(struct ip6_tnl *t, int set_mtu)
> +{
> +       ip6gre_tnl_link_config_common(t);
> +       ip6gre_tnl_link_config_route(t, set_mtu, ip6erspan_calc_hlen(t));
> +}
> +
> +static int ip6erspan_tnl_change(struct ip6_tnl *t,
> +                               const struct __ip6_tnl_parm *p, int set_mtu)
> +{
> +       ip6gre_tnl_copy_tnl_parm(t, p);
> +       ip6erspan_tnl_link_config(t, set_mtu);
> +       return 0;
> +}
> +
> +static int ip6erspan_changelink(struct net_device *dev, struct nlattr *tb[],
> +                               struct nlattr *data[],
> +                               struct netlink_ext_ack *extack)
> +{
> +       struct ip6gre_net *ign = net_generic(dev_net(dev), ip6gre_net_id);
> +       struct __ip6_tnl_parm p;
> +       struct ip6_tnl *t;
> +
> +       t = ip6gre_changelink_common(dev, tb, data, &p, extack);
> +       if (IS_ERR(t))
> +               return PTR_ERR(t);
> +
> +       ip6gre_tunnel_unlink(ign, t);
> +       ip6erspan_tnl_change(t, &p, !tb[IFLA_MTU]);
> +       ip6gre_tunnel_link(ign, t);
> +       return 0;
> +}
> +
>  static struct rtnl_link_ops ip6gre_link_ops __read_mostly = {
>         .kind           = "ip6gre",
>         .maxtype        = IFLA_GRE_MAX,
> @@ -2154,8 +2210,8 @@ static struct rtnl_link_ops ip6erspan_tap_ops __read_mostly = {
>         .priv_size      = sizeof(struct ip6_tnl),
>         .setup          = ip6erspan_tap_setup,
>         .validate       = ip6erspan_tap_validate,
> -       .newlink        = ip6gre_newlink,
> -       .changelink     = ip6gre_changelink,
> +       .newlink        = ip6erspan_newlink,
> +       .changelink     = ip6erspan_changelink,
>         .get_size       = ip6gre_get_size,
>         .fill_info      = ip6gre_fill_info,
>         .get_link_net   = ip6_tnl_get_link_net,
> --
> 2.4.11
>

Re: [PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Thu, 17 May 2018 16:36:04 +0200

> This series mends some problems in headroom management in ip6_gre
> module. The current code base has the following three closely-related
> problems:
> 
> - ip6gretap tunnels neglect to ensure there's enough writable headroom
>   before pushing GRE headers.
> 
> - ip6erspan does this, but assumes that dev->needed_headroom is primed.
>   But that doesn't happen until ip6_tnl_xmit() is called later. Thus for
>   the first packet, ip6erspan actually behaves like ip6gretap above.
> 
> - ip6erspan shares some of the code with ip6gretap, including
>   calculations of needed header length. While there is custom
>   ERSPAN-specific code for calculating the headroom, the computed
>   values are overwritten by the ip6gretap code.
> 
> The first two issues lead to a kernel panic in situations where a packet
> is mirrored from a veth device to the device in question. They are
> fixed, respectively, in patches #1 and #2, which include the full panic
> trace and a reproducer.
> 
> The rest of the patchset deals with the last issue. In patches #3 to #6,
> several functions are split up into reusable parts. Finally in patch #7
> these blocks are used to compose ERSPAN-specific callbacks where
> necessary to fix the hlen calculation.

Series applied, thank you.

Those reproducable test cases in the various commit messages are
pretty awesome.  Could you please extract them and put them somewhere
appropriate under selftests?

Thank you.

Re: [PATCH net 1/7] net: ip6_gre: Request headroom in __gre6_xmit()

[Petr Machata]

Fixes: c12b395a4664 ("gre: Support GRE over IPv6")

Re: [PATCH net 2/7] net: ip6_gre: Fix headroom request in ip6erspan_tunnel_xmit()

[Petr Machata]

Fixes: e41c7c68ea77 ("ip6erspan: make sure enough headroom at xmit.")

Re: [PATCH net 3/7] net: ip6_gre: Split up ip6gre_tnl_link_config()

[Petr Machata]

Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")

Re: [PATCH net 4/7] net: ip6_gre: Split up ip6gre_tnl_change()

[Petr Machata]

Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")

Re: [PATCH net 5/7] net: ip6_gre: Split up ip6gre_newlink()

[Petr Machata]

Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")

Re: [PATCH net 6/7] net: ip6_gre: Split up ip6gre_changelink()

[Petr Machata]

Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")

Re: [PATCH net 7/7] net: ip6_gre: Fix ip6erspan hlen calculation

[Petr Machata]

Fixes: 5a963eb61b7c ("ip6_gre: Add ERSPAN native tunnel support")

Re: [PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[Petr Machata]

David Miller <davem@davemloft.net> writes:

> Series applied, thank you.

Hi David, I forgot to add Fixes lines to the individual patches. I
replied to the e-mails with those. Let me know if you want me to send a
v2 with that and the Acked-by's.

> Those reproducable test cases in the various commit messages are
> pretty awesome.  Could you please extract them and put them somewhere
> appropriate under selftests?

The ip6gretap one is covered by the mirror_gre test if you run it
with veth devices instead of HW ports, but I can make it self-contained
if you think that would be better.

I'll add the erspan one.

Thanks,
Petr

Re: [PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[David Miller]

From: Petr Machata <petrm@mellanox.com>
Date: Fri, 18 May 2018 00:03:58 +0300

> David Miller <davem@davemloft.net> writes:
> 
>> Series applied, thank you.
> 
> Hi David, I forgot to add Fixes lines to the individual patches. I
> replied to the e-mails with those. Let me know if you want me to send a
> v2 with that and the Acked-by's.

When something is already in my tree, it can't be changed as it is committed
to the permanent record of my GIT tree and I cannot rebase since so many
people clone my tree.

Luckily for you, your Fixes: tags went out before I pushed, so I could
actually fix up the commit messages and add the tags.

>> Those reproducable test cases in the various commit messages are
>> pretty awesome.  Could you please extract them and put them somewhere
>> appropriate under selftests?
> 
> The ip6gretap one is covered by the mirror_gre test if you run it
> with veth devices instead of HW ports, but I can make it self-contained
> if you think that would be better.
> 
> I'll add the erspan one.

Thank you.

Re: [PATCH net 0/7] net: ip6_gre: Fixes in headroom handling

[Petr Machata]

David Miller <davem@davemloft.net> writes:

> Luckily for you, your Fixes: tags went out before I pushed, so I could
> actually fix up the commit messages and add the tags.

I was hoping that would be the case.

Thanks,
Petr