* INFO: rcu detected stall in corrupted
From: syzbot @ 2018-05-21 18:05 UTC (permalink / raw)
To: davem, kuznet, linux-kernel, netdev, syzkaller-bugs, yoshfuji
Hello,
syzbot found the following crash on:
HEAD commit: 771c577c23ba Linux 4.17-rc6
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17134357800000
kernel config: https://syzkaller.appspot.com/x/.config?x=982e2df1b9e60b02
dashboard link: https://syzkaller.appspot.com/bug?extid=f116bc1994efe725d51b
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
userspace arch: i386
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=14e5a7cf800000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+f116bc1994efe725d51b@syzkaller.appspotmail.com
IPv6: ADDRCONF(NETDEV_UP): veth1: link is not ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth1: link becomes ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
8021q: adding VLAN 0 to HW filter on device team0
8021q: adding VLAN 0 to HW filter on device team0
INFO: rcu_sched self-detected stall on CPU
INFO: rcu_sched detected stalls on CPUs/tasks:
0-...!: (124975 ticks this GP) idle=a36/1/4611686018427387906
softirq=14002/14002 fqs=10
0-...!: (124975 ticks this GP) idle=a36/1/4611686018427387906
softirq=14002/14002 fqs=10
(t=125002 jiffies g=7347 c=7346 q=349000)
(detected by 1, t=125002 jiffies, g=7347, c=7346, q=349000)
rcu_sched kthread starved for 124927 jiffies! g7347 c7346 f0x2
RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=1
Sending NMI from CPU 1 to CPUs 0:
RCU grace-period kthread stack dump:
NMI backtrace for cpu 0
CPU: 0 PID: 8 Comm: ksoftirqd/0 Not tainted 4.17.0-rc6+ #86
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:get_current arch/x86/include/asm/current.h:15 [inline]
RIP: 0010:write_comp_data+0xa/0x70 kernel/kcov.c:121
RSP: 0018:ffff8801dae06d30 EFLAGS: 00000006
RAX: 0000000000010105 RBX: 0000000000000006 RCX: ffffffff876bdc58
RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000001
RBP: ffff8801dae06d68 R08: ffff8801d9a9c200 R09: fffffbfff14da4bc
R10: fffffbfff14da4bc R11: ffffffff8a6d25e0 R12: ffffffff88644220
R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000008
FS: 0000000000000000(0000) GS:ffff8801dae00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f595e194270 CR3: 00000001b09c2000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<IRQ>
vsnprintf+0x1b8/0x1b40 lib/vsprintf.c:2252
sprintf+0xa7/0xd0 lib/vsprintf.c:2498
print_time kernel/printk/printk.c:1223 [inline]
print_prefix+0x26a/0x3f0 kernel/printk/printk.c:1246
msg_print_text+0xca/0x1c0 kernel/printk/printk.c:1273
console_unlock+0x4f5/0x1100 kernel/printk/printk.c:2369
vprintk_emit+0x6ad/0xdd0 kernel/printk/printk.c:1907
vprintk_default+0x28/0x30 kernel/printk/printk.c:1947
vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:379
printk+0x9e/0xba kernel/printk/printk.c:1980
rcu_check_gp_kthread_starvation+0x325/0x3a4 kernel/rcu/tree.c:1353
print_cpu_stall kernel/rcu/tree.c:1523 [inline]
check_cpu_stall.isra.61.cold.80+0x364/0x59a kernel/rcu/tree.c:1593
__rcu_pending kernel/rcu/tree.c:3356 [inline]
rcu_pending kernel/rcu/tree.c:3401 [inline]
rcu_check_callbacks+0x21b/0xad0 kernel/rcu/tree.c:2763
update_process_times+0x2d/0x70 kernel/time/timer.c:1636
tick_sched_handle+0x9f/0x180 kernel/time/tick-sched.c:164
tick_sched_timer+0x45/0x130 kernel/time/tick-sched.c:1274
__run_hrtimer kernel/time/hrtimer.c:1398 [inline]
__hrtimer_run_queues+0x3e3/0x10a0 kernel/time/hrtimer.c:1460
hrtimer_interrupt+0x2f3/0x750 kernel/time/hrtimer.c:1518
local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1025 [inline]
smp_apic_timer_interrupt+0x15d/0x710 arch/x86/kernel/apic/apic.c:1050
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:863
</IRQ>
RIP: 0010:__sanitizer_cov_trace_pc+0x1/0x50 kernel/kcov.c:94
RSP: 0018:ffff8801d9aad680 EFLAGS: 00000293 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000103 RBX: 0000000000000002 RCX: ffffffff867e02e0
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000000005
RBP: ffff8801d9aad7e0 R08: ffff8801d9a9c200 R09: ffff8801d9aadaf0
R10: ffffed003b5c46c2 R11: ffff8801dae23613 R12: ffff8801ce597c40
R13: 0000000000000000 R14: 0000000000000002 R15: 0000000000000000
find_match+0x244/0x13a0 net/ipv6/route.c:691
find_rr_leaf net/ipv6/route.c:729 [inline]
rt6_select net/ipv6/route.c:779 [inline]
ip6_pol_route+0x946/0x3d40 net/ipv6/route.c:1705
ip6_pol_route_output+0x54/0x70 net/ipv6/route.c:1969
fib6_rule_lookup+0x211/0x6d0 net/ipv6/fib6_rules.c:89
ip6_route_output_flags+0x2c5/0x350 net/ipv6/route.c:1997
ip6_dst_lookup_tail+0x47b/0x1b30 net/ipv6/ip6_output.c:995
ip6_dst_lookup_flow+0xc1/0x260 net/ipv6/ip6_output.c:1096
sctp_v6_get_dst+0x16b4/0x20b0 net/sctp/ipv6.c:327
sctp_transport_route+0xad/0x450 net/sctp/transport.c:293
sctp_packet_config+0xb89/0xfd0 net/sctp/output.c:123
sctp_outq_flush+0x79c/0x4370 net/sctp/outqueue.c:894
? trace_hardirqs_off
Lost 148 message(s)!
rcu_sched kthread starved for 124927 jiffies! g7347 c7346 f0x2
RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=1
rcu_sched R
RCU grace-period kthread stack dump:
rcu_sched R
running task
running task 23896 9 2 0x80000000
23896 9 2 0x80000000
Call Trace:
Call Trace:
context_switch kernel/sched/core.c:2859 [inline]
__schedule+0x801/0x1e30 kernel/sched/core.c:3501
context_switch kernel/sched/core.c:2859 [inline]
__schedule+0x801/0x1e30 kernel/sched/core.c:3501
schedule+0xef/0x430 kernel/sched/core.c:3545
schedule+0xef/0x430 kernel/sched/core.c:3545
schedule_timeout+0x138/0x240 kernel/time/timer.c:1801
schedule_timeout+0x138/0x240 kernel/time/timer.c:1801
rcu_gp_kthread+0x6b5/0x1940 kernel/rcu/tree.c:2231
rcu_gp_kthread+0x6b5/0x1940 kernel/rcu/tree.c:2231
kthread+0x345/0x410 kernel/kthread.c:240
kthread+0x345/0x410 kernel/kthread.c:240
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
NMI backtrace for cpu 0
CPU: 0 PID: 8 Comm: ksoftirqd/0 Not tainted 4.17.0-rc6+ #86
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1b9/0x294 lib/dump_stack.c:113
nmi_cpu_backtrace.cold.4+0x19/0xce lib/nmi_backtrace.c:103
nmi_trigger_cpumask_backtrace+0x151/0x192 lib/nmi_backtrace.c:62
arch_trigger_cpumask_backtrace+0x14/0x20 arch/x86/kernel/apic/hw_nmi.c:38
trigger_single_cpu_backtrace include/linux/nmi.h:156 [inline]
rcu_dump_cpu_stacks+0x175/0x1c2 kernel/rcu/tree.c:1376
print_cpu_stall kernel/rcu/tree.c:1525 [inline]
check_cpu_stall.isra.61.cold.80+0x36c/0x59a kernel/rcu/tree.c:1593
__rcu_pending kernel/rcu/tree.c:3356 [inline]
rcu_pending kernel/rcu/tree.c:3401 [inline]
rcu_check_callbacks+0x21b/0xad0 kernel/rcu/tree.c:2763
update_process_times+0x2d/0x70 kernel/time/timer.c:1636
tick_sched_handle+0x9f/0x180 kernel/time/tick-sched.c:164
tick_sched_timer+0x45/0x130 kernel/time/tick-sched.c:1274
__run_hrtimer kernel/time/hrtimer.c:1398 [inline]
__hrtimer_run_queues+0x3e3/0x10a0 kernel/time/hrtimer.c:1460
hrtimer_interrupt+0x2f3/0x750 kernel/time/hrtimer.c:1518
local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1025 [inline]
smp_apic_timer_interrupt+0x15d/0x710 arch/x86/kernel/apic/apic.c:1050
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:863
</IRQ>
RIP: 0010:__sanitizer_cov_trace_pc+0x1/0x50 kernel/kcov.c:94
RSP: 0018:ffff8801d9aad680 EFLAGS: 00000293 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000103 RBX: 0000000000000002 RCX: ffffffff867e02e0
RDX: 0000000000000000 RSI: 0000000000000002 RDI: 0000000000000005
RBP: ffff8801d9aad7e0 R08: ffff8801d9a9c200 R09: ffff8801d9aadaf0
R10: ffffed003b5c46c2 R11: ffff8801dae23613 R12: ffff8801ce597c40
R13: 0000000000000000 R14: 0000000000000002 R15: 0000000000000000
find_match+0x244/0x13a0 net/ipv6/route.c:691
find_rr_leaf net/ipv6/route.c:729 [inline]
rt6_select net/ipv6/route.c:779 [inline]
ip6_pol_route+0x946/0x3d40 net/ipv6/route.c:1705
ip6_pol_route_output+0x54/0x70 net/ipv6/route.c:1969
fib6_rule_lookup+0x211/0x6d0 net/ipv6/fib6_rules.c:89
ip6_route_output_flags+0x2c5/0x350 net/ipv6/route.c:1997
ip6_dst_lookup_tail+0x47b/0x1b30 net/ipv6/ip6_output.c:995
ip6_dst_lookup_flow+0xc1/0x260 net/ipv6/ip6_output.c:1096
sctp_v6_get_dst+0x16b4/0x20b0 net/sctp/ipv6.c:327
sctp_transport_route+0xad/0x450 net/sctp/transport.c:293
sctp_packet_config+0xb89/0xfd0 net/sctp/output.c:123
sctp_outq_flush+0x79c/0x4370 net/sctp/outqueue.c:894
sctp_outq_uncork+0x6a/0x80 net/sctp/outqueue.c:776
sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1820 [inline]
sctp_side_effects net/sctp/sm_sideeffect.c:1220 [inline]
sctp_do_sm+0x596/0x7160 net/sctp/sm_sideeffect.c:1191
sctp_generate_heartbeat_event+0x218/0x450 net/sctp/sm_sideeffect.c:406
call_timer_fn+0x230/0x940 kernel/time/timer.c:1326
expire_timers kernel/time/timer.c:1363 [inline]
__run_timers+0x79e/0xc50 kernel/time/timer.c:1666
run_timer_softirq+0x4c/0x70 kernel/time/timer.c:1692
__do_softirq+0x2e0/0xaf5 kernel/softirq.c:285
run_ksoftirqd+0x86/0x100 kernel/softirq.c:646
smpboot_thread_fn+0x417/0x870 kernel/smpboot.c:164
kthread+0x345/0x410 kernel/kthread.c:240
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
INFO: NMI handler (nmi_cpu_backtrace_handler) took too long to run: 1.929
msecs
BUG: workqueue lockup - pool cpus=0-1 flags=0x4 nice=0 stuck for 127s!
Showing busy workqueues and worker pools:
workqueue events: flags=0x0
pwq 0: cpus=0 node=0 flags=0x0 nice=0 active=10/256
pending: pcpu_balance_workfn, defense_work_handler,
defense_work_handler, defense_work_handler, defense_work_handler,
defense_work_handler, defense_work_handler, check_corruption,
vmstat_shepherd, cache_reap
workqueue events_power_efficient: flags=0x80
pwq 0: cpus=0 node=0 flags=0x0 nice=0 active=1/256
pending: check_lifetime
workqueue mm_percpu_wq: flags=0x8
pwq 0: cpus=0 node=0 flags=0x0 nice=0 active=1/256
pending: vmstat_update
workqueue writeback: flags=0x4e
pwq 4: cpus=0-1 flags=0x4 nice=0 active=4/256
pending: wb_workfn, wb_workfn, wb_workfn, wb_workfn
workqueue kblockd: flags=0x18
pwq 1: cpus=0 node=0 flags=0x0 nice=-20 active=1/256
pending: blk_mq_timeout_work
workqueue ib_addr: flags=0xa0002
pwq 4: cpus=0-1 flags=0x4 nice=0 active=1/1
pending: process_req
workqueue gid-cache-wq: flags=0xa0002
pwq 4: cpus=0-1 flags=0x4 nice=0 active=1/1
pending: update_gid_event_work_handler
delayed: netdevice_event_work_handler
workqueue ipv6_addrconf: flags=0x40008
pwq 0: cpus=0 node=0 flags=0x0 nice=0 active=1/1
pending: addrconf_dad_work
delayed: addrconf_dad_work, addrconf_dad_work, addrconf_dad_work,
addrconf_dad_work, addrconf_dad_work, addrconf_dad_work, addrconf_dad_work,
addrconf_dad_work, addrconf_dad_work, addrconf_dad_work, addrconf_dad_work,
addrconf_dad_work, addrconf_dad_work, addrconf_dad_work, addrconf_dad_work,
addrconf_dad_work, addrconf_dad_work, addrconf_dad_work
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* [PATCH net] ipmr: properly check rhltable_init() return value
From: Eric Dumazet @ 2018-05-21 17:51 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Nikolay Aleksandrov,
Yuval Mintz
commit 8fb472c09b9d ("ipmr: improve hash scalability")
added a call to rhltable_init() without checking its return value.
This problem was then later copied to IPv6 and factorized in commit
0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 31552 Comm: syz-executor7 Not tainted 4.17.0-rc5+ #60
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:rht_key_hashfn include/linux/rhashtable.h:277 [inline]
RIP: 0010:__rhashtable_lookup include/linux/rhashtable.h:630 [inline]
RIP: 0010:rhltable_lookup include/linux/rhashtable.h:716 [inline]
RIP: 0010:mr_mfc_find_parent+0x2ad/0xbb0 net/ipv4/ipmr_base.c:63
RSP: 0018:ffff8801826aef70 EFLAGS: 00010203
RAX: 0000000000000001 RBX: 0000000000000001 RCX: ffffc90001ea0000
RDX: 0000000000000079 RSI: ffffffff8661e859 RDI: 000000000000000c
RBP: ffff8801826af1c0 R08: ffff8801b2212000 R09: ffffed003b5e46c2
R10: ffffed003b5e46c2 R11: ffff8801daf23613 R12: dffffc0000000000
R13: ffff8801826af198 R14: ffff8801cf8225c0 R15: ffff8801826af658
FS: 00007ff7fa732700(0000) GS:ffff8801daf00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000003ffffff9c CR3: 00000001b0210000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
ip6mr_cache_find_parent net/ipv6/ip6mr.c:981 [inline]
ip6mr_mfc_delete+0x1fe/0x6b0 net/ipv6/ip6mr.c:1221
ip6_mroute_setsockopt+0x15c6/0x1d70 net/ipv6/ip6mr.c:1698
do_ipv6_setsockopt.isra.9+0x422/0x4660 net/ipv6/ipv6_sockglue.c:163
ipv6_setsockopt+0xbd/0x170 net/ipv6/ipv6_sockglue.c:922
rawv6_setsockopt+0x59/0x140 net/ipv6/raw.c:1060
sock_common_setsockopt+0x9a/0xe0 net/core/sock.c:3039
__sys_setsockopt+0x1bd/0x390 net/socket.c:1903
__do_sys_setsockopt net/socket.c:1914 [inline]
__se_sys_setsockopt net/socket.c:1911 [inline]
__x64_sys_setsockopt+0xbe/0x150 net/socket.c:1911
do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
Fixes: 0bbbf0e7d0e7 ("ipmr, ip6mr: Unite creation of new mr_table")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Cc: Yuval Mintz <yuvalm@mellanox.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
net/ipv4/ipmr_base.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
index 4fe97723b53f4deb5ebbb1d7633ceb1b4efc3af3..30221701614c599d380229c69b83558537c69de8 100644
--- a/net/ipv4/ipmr_base.c
+++ b/net/ipv4/ipmr_base.c
@@ -43,7 +43,10 @@ mr_table_alloc(struct net *net, u32 id,
write_pnet(&mrt->net, net);
mrt->ops = *ops;
- rhltable_init(&mrt->mfc_hash, mrt->ops.rht_params);
+ if (rhltable_init(&mrt->mfc_hash, mrt->ops.rht_params)) {
+ kfree(mrt);
+ return NULL;
+ }
INIT_LIST_HEAD(&mrt->mfc_cache_list);
INIT_LIST_HEAD(&mrt->mfc_unres_queue);
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related
* Re: [PATCH] bpf: fix mem leak in error path of lwt bpf setup
From: Martin KaFai Lau @ 2018-05-21 17:44 UTC (permalink / raw)
To: Mathieu Xhonneux; +Cc: netdev, daniel, alexei.starovoitov
In-Reply-To: <20180520130857.1278-1-m.xhonneux@gmail.com>
On Sun, May 20, 2018 at 02:08:57PM +0100, Mathieu Xhonneux wrote:
> In bpf_parse_prog, if bpf_prog_get_type fails, the function is
> immediately terminated without freeing the previously allocated
> prog->name.
> This patch adds a kfree before the return.
>
> Signed-off-by: Mathieu Xhonneux <m.xhonneux@gmail.com>
> ---
> net/core/lwt_bpf.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index e7e626fb87bb..e142a7a32e46 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -223,8 +223,10 @@ static int bpf_parse_prog(struct nlattr *attr, struct bpf_lwt_prog *prog,
>
> fd = nla_get_u32(tb[LWT_BPF_PROG_FD]);
> p = bpf_prog_get_type(fd, type);
> - if (IS_ERR(p))
> + if (IS_ERR(p)) {
> + kfree(prog->name);
I don't think it is needed.
The caller, "bpf_build_state()", does bpf_destroy_state() during error
out and it will eventually free up "name".
> return PTR_ERR(p);
> + }
>
> prog->prog = p;
>
> --
> 2.16.1
>
^ permalink raw reply
* pull request Cavium liquidio vswitch firmware v1.7.2
From: Felix Manlunas @ 2018-05-21 17:39 UTC (permalink / raw)
To: linux-firmware
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
felix.manlunas, manish.awasthi, manojkumar.panicker
The following changes since commit 2a9b2cf50fb32e36e4fc1586c2f6f1421913b553:
Merge branch 'for-upstreaming-v1.7.2' of https://github.com/felix-cavium/linux-firmware (2018-05-18 08:35:22 -0400)
are available in the git repository at:
https://github.com/felix-cavium/linux-firmware.git for-upstreaming-v1.7.2-vsw
for you to fetch changes up to 0e193ca65d8b064502d61163597bf14eef81710f:
linux-firmware: liquidio: update vswitch firmware to v1.7.2 (2018-05-19 23:29:03 -0700)
Signed-off-by: Manish Awasthi <manish.awasthi@cavium.com>
Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
----------------------------------------------------------------
Felix Manlunas (1):
linux-firmware: liquidio: update vswitch firmware to v1.7.2
WHENCE | 2 +-
liquidio/lio_23xx_vsw.bin | Bin 19922416 -> 20434408 bytes
2 files changed, 1 insertion(+), 1 deletion(-)
^ permalink raw reply
* Re: [PATCH net-next v13 3/7] sch_cake: Add optional ACK filter
From: Toke Høiland-Jørgensen @ 2018-05-21 17:35 UTC (permalink / raw)
To: Eric Dumazet, netdev, cake
In-Reply-To: <f7a7917b-9d81-b192-514b-3861321a4892@gmail.com>
Eric Dumazet <eric.dumazet@gmail.com> writes:
> On 05/21/2018 09:24 AM, Toke Høiland-Jørgensen wrote:
>
>> + while (oplen_tmp >= 8) {
>> + u32 right_b = get_unaligned_be32(sack_tmp + 4);
>> + u32 left_b = get_unaligned_be32(sack_tmp);
>> +
>> + if (left_b >= right_b)
>> + continue;
>> +
>> + if (first)
>> + bytes_b += right_b - left_b;
>> +
>> + if (left_b <= left_a && right_a <= right_b) {
>> + found = true;
>> + if (!first)
>> + break;
>> + }
>> + oplen_tmp -= 8;
>> + sack_tmp += 8;
>> + }
>>
>
> This is obviously incorrect.
>
> Please take a look at TCP stack, and how it handles sequence wrapping
> with following macros :
>
> after(), before()
Ah yes, sequence number wrapping. I was thinking I needed to deal with
that, and then got sidetracked and forgot about it. Will fix.
Other than that, do you agree that this approach to SACK and header
handling can work?
> Quite frankly O wonder how this was really tested.
Getting it wrong results in a few drop decisions being wrong every 2**32
bytes; easy to miss. If you have a "mis-behaving middle-box test suite"
or something I'll be happy to hear about it :)
-Toke
^ permalink raw reply
* [PATCH] ipvs: drop templates for never established TCP connections
From: Michal Koutný @ 2018-05-21 17:27 UTC (permalink / raw)
To: wensong, horms, ja; +Cc: mkoutny, mkubecek, netdev, lvs-devel
IPVS includes protection against filling the ip_vs_conn_tab by dropping 1/32 of
feasible entries every second. The template entries (for persistent services)
are never directly deleted by this mechanism but when a picked TCP connection
entry is being dropped (1), the respective template entry is dropped too
(realized by expiring 60 seconds after the connection entry being dropped).
There is another mechanism that removes connection entries when they
time out (2), in this case the associated template entry is not deleted.
Under SYN flood template entries would accumulate (due to their entry
longer timeout).
The accumulation takes place also with drop_entry being enabled. Roughly
15% ((31/32)^60) of SYN_RECV connections survive the dropping mechanism
(1) and are removed by the timeout mechanism (2)(defaults to 60 seconds
for SYN_RECV), thus template entries would still accumulate.
The patch ensures that when a connection entry times out, we also remove the
template entry from the table. To prevent breaking persistent services (since
the connection may time out in already established state) we add a new entry
flag to protect templates what spawned at least one established TCP connection.
Cc: Michal Kubeček <mkubecek@suse.com>
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
include/uapi/linux/ip_vs.h | 33 +++++++++++++++++----------------
net/netfilter/ipvs/ip_vs_conn.c | 10 +++++++++-
net/netfilter/ipvs/ip_vs_core.c | 15 ++++++++++++++-
net/netfilter/ipvs/ip_vs_proto_tcp.c | 6 ++++++
4 files changed, 46 insertions(+), 18 deletions(-)
diff --git a/include/uapi/linux/ip_vs.h b/include/uapi/linux/ip_vs.h
index 1c916b2f89dc..ef3bbc001fcd 100644
--- a/include/uapi/linux/ip_vs.h
+++ b/include/uapi/linux/ip_vs.h
@@ -79,22 +79,23 @@
* IPVS Connection Flags
* Only flags 0..15 are sent to backup server
*/
-#define IP_VS_CONN_F_FWD_MASK 0x0007 /* mask for the fwd methods */
-#define IP_VS_CONN_F_MASQ 0x0000 /* masquerading/NAT */
-#define IP_VS_CONN_F_LOCALNODE 0x0001 /* local node */
-#define IP_VS_CONN_F_TUNNEL 0x0002 /* tunneling */
-#define IP_VS_CONN_F_DROUTE 0x0003 /* direct routing */
-#define IP_VS_CONN_F_BYPASS 0x0004 /* cache bypass */
-#define IP_VS_CONN_F_SYNC 0x0020 /* entry created by sync */
-#define IP_VS_CONN_F_HASHED 0x0040 /* hashed entry */
-#define IP_VS_CONN_F_NOOUTPUT 0x0080 /* no output packets */
-#define IP_VS_CONN_F_INACTIVE 0x0100 /* not established */
-#define IP_VS_CONN_F_OUT_SEQ 0x0200 /* must do output seq adjust */
-#define IP_VS_CONN_F_IN_SEQ 0x0400 /* must do input seq adjust */
-#define IP_VS_CONN_F_SEQ_MASK 0x0600 /* in/out sequence mask */
-#define IP_VS_CONN_F_NO_CPORT 0x0800 /* no client port set yet */
-#define IP_VS_CONN_F_TEMPLATE 0x1000 /* template, not connection */
-#define IP_VS_CONN_F_ONE_PACKET 0x2000 /* forward only one packet */
+#define IP_VS_CONN_F_FWD_MASK 0x0007 /* mask for the fwd methods */
+#define IP_VS_CONN_F_MASQ 0x0000 /* masquerading/NAT */
+#define IP_VS_CONN_F_LOCALNODE 0x0001 /* local node */
+#define IP_VS_CONN_F_TUNNEL 0x0002 /* tunneling */
+#define IP_VS_CONN_F_DROUTE 0x0003 /* direct routing */
+#define IP_VS_CONN_F_BYPASS 0x0004 /* cache bypass */
+#define IP_VS_CONN_F_SYNC 0x0020 /* entry created by sync */
+#define IP_VS_CONN_F_HASHED 0x0040 /* hashed entry */
+#define IP_VS_CONN_F_NOOUTPUT 0x0080 /* no output packets */
+#define IP_VS_CONN_F_INACTIVE 0x0100 /* not established */
+#define IP_VS_CONN_F_OUT_SEQ 0x0200 /* must do output seq adjust */
+#define IP_VS_CONN_F_IN_SEQ 0x0400 /* must do input seq adjust */
+#define IP_VS_CONN_F_SEQ_MASK 0x0600 /* in/out sequence mask */
+#define IP_VS_CONN_F_NO_CPORT 0x0800 /* no client port set yet */
+#define IP_VS_CONN_F_TEMPLATE 0x1000 /* template, not connection */
+#define IP_VS_CONN_F_ONE_PACKET 0x2000 /* forward only one packet */
+#define IP_VS_CONN_F_TMPL_PERSISTED 0x4000 /* template, confirmed persistent */
/* Initial bits allowed in backup server */
#define IP_VS_CONN_F_BACKUP_MASK (IP_VS_CONN_F_FWD_MASK | \
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 370abbf6f421..6afc606a388c 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -820,6 +820,7 @@ static void ip_vs_conn_rcu_free(struct rcu_head *head)
static void ip_vs_conn_expire(struct timer_list *t)
{
struct ip_vs_conn *cp = from_timer(cp, t, timer);
+ struct ip_vs_conn *cp_c;
struct netns_ipvs *ipvs = cp->ipvs;
/*
@@ -834,8 +835,15 @@ static void ip_vs_conn_expire(struct timer_list *t)
del_timer(&cp->timer);
/* does anybody control me? */
- if (cp->control)
+ cp_c = cp->control;
+ if (cp_c) {
ip_vs_control_del(cp);
+ if (cp_c->flags & IP_VS_CONN_F_TEMPLATE &&
+ !(cp_c->flags & IP_VS_CONN_F_TMPL_PERSISTED)) {
+ IP_VS_DBG(4, "del conn template\n");
+ ip_vs_conn_expire_now(cp_c);
+ }
+ }
if ((cp->flags & IP_VS_CONN_F_NFCT) &&
!(cp->flags & IP_VS_CONN_F_ONE_PACKET)) {
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 5f6f73cf2174..8e4ba69a09e1 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -221,6 +221,17 @@ ip_vs_conn_fill_param_persist(const struct ip_vs_service *svc,
return 0;
}
+static inline bool
+is_new_template_persisted(u16 protocol)
+{
+ switch (protocol) {
+ case IPPROTO_TCP:
+ return false;
+ default:
+ return true;
+ }
+}
+
/*
* IPVS persistent scheduling function
* It creates a connection entry according to its template if exists,
@@ -347,8 +358,10 @@ ip_vs_sched_persist(struct ip_vs_service *svc,
* This adds param.pe_data to the template,
* and thus param.pe_data will be destroyed
* when the template expires */
+ flags = IP_VS_CONN_F_TEMPLATE;
+ flags |= is_new_template_persisted(param.protocol) ? IP_VS_CONN_F_TMPL_PERSISTED : 0;
ct = ip_vs_conn_new(¶m, dest->af, &dest->addr, dport,
- IP_VS_CONN_F_TEMPLATE, dest, skb->mark);
+ flags, dest, skb->mark);
if (ct == NULL) {
kfree(param.pe_data);
*ignored = -1;
diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c b/net/netfilter/ipvs/ip_vs_proto_tcp.c
index 569631d2b2a1..ede6e0d31926 100644
--- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
@@ -538,6 +538,7 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct ip_vs_conn *cp,
tcp_state_out:
if (new_state != cp->state) {
struct ip_vs_dest *dest = cp->dest;
+ struct ip_vs_conn *ct = cp->control;
IP_VS_DBG_BUF(8, "%s %s [%c%c%c%c] %s:%d->"
"%s:%d state: %s->%s conn->refcnt:%d\n",
@@ -569,6 +570,11 @@ set_tcp_state(struct ip_vs_proto_data *pd, struct ip_vs_conn *cp,
cp->flags &= ~IP_VS_CONN_F_INACTIVE;
}
}
+
+ if (ct && ct->flags & IP_VS_CONN_F_TEMPLATE) {
+ if (new_state == IP_VS_TCP_S_ESTABLISHED)
+ ct->flags |= IP_VS_CONN_F_TMPL_PERSISTED;
+ }
}
if (likely(pd))
--
2.13.6
^ permalink raw reply related
* [PATCH net-next 6/7] selftests: fib_tests: Add ipv6 route add append replace tests
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Add IPv6 route tests covering add, append and replace permutations.
Assumes the ability to add a basic single path route works; this is
required for example when adding an address to an interface.
$ fib_tests.sh -t ipv6_rt
IPv6 route add / append tests
TEST: Attempt to add duplicate route - gw [ OK ]
TEST: Attempt to add duplicate route - dev only [ OK ]
TEST: Attempt to add duplicate route - reject route [ OK ]
TEST: Add new route for existing prefix (w/o NLM_F_EXCL) [ OK ]
TEST: Append nexthop to existing route - gw [ OK ]
TEST: Append nexthop to existing route - dev only [ OK ]
TEST: Append nexthop to existing route - reject route [ OK ]
TEST: Append nexthop to existing reject route - gw [ OK ]
TEST: Append nexthop to existing reject route - dev only [ OK ]
TEST: Add multipath route [ OK ]
TEST: Attempt to add duplicate multipath route [ OK ]
TEST: Route add with different metrics [ OK ]
TEST: Route delete with metric [ OK ]
IPv6 route replace tests
TEST: Single path with single path [ OK ]
TEST: Single path with multipath [ OK ]
TEST: Single path with reject route [ OK ]
TEST: Single path with single path via multipath attribute [ OK ]
TEST: Invalid nexthop [ OK ]
TEST: Single path - replace of non-existent route [ OK ]
TEST: Multipath with multipath [ OK ]
TEST: Multipath with single path [ OK ]
TEST: Multipath with single path via multipath attribute [ OK ]
TEST: Multipath with reject route [ OK ]
TEST: Multipath - invalid first nexthop [ OK ]
TEST: Multipath - invalid second nexthop [ OK ]
TEST: Multipath - replace of non-existent route [ OK ]
Signed-off-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/fib_tests.sh | 335 ++++++++++++++++++++++++++++++-
1 file changed, 334 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 12b648826151..384828f28372 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -6,7 +6,7 @@
ret=0
-TESTS="unregister down carrier nexthop"
+TESTS="unregister down carrier nexthop ipv6_rt"
VERBOSE=0
PAUSE_ON_FAIL=no
PAUSE=no
@@ -574,6 +574,338 @@ fib_nexthop_test()
}
################################################################################
+# Tests on route add and replace
+
+run_cmd()
+{
+ local cmd="$1"
+ local out
+ local stderr="2>/dev/null"
+
+ if [ "$VERBOSE" = "1" ]; then
+ printf " COMMAND: $cmd\n"
+ stderr=
+ fi
+
+ out=$(eval $cmd $stderr)
+ rc=$?
+ if [ "$VERBOSE" = "1" -a -n "$out" ]; then
+ echo " $out"
+ fi
+
+ [ "$VERBOSE" = "1" ] && echo
+
+ return $rc
+}
+
+# add route for a prefix, flushing any existing routes first
+# expected to be the first step of a test
+add_route6()
+{
+ local pfx="$1"
+ local nh="$2"
+ local out
+
+ if [ "$VERBOSE" = "1" ]; then
+ echo
+ echo " ##################################################"
+ echo
+ fi
+
+ run_cmd "$IP -6 ro flush ${pfx}"
+ [ $? -ne 0 ] && exit 1
+
+ out=$($IP -6 ro ls match ${pfx})
+ if [ -n "$out" ]; then
+ echo "Failed to flush routes for prefix used for tests."
+ exit 1
+ fi
+
+ run_cmd "$IP -6 ro add ${pfx} ${nh}"
+ if [ $? -ne 0 ]; then
+ echo "Failed to add initial route for test."
+ exit 1
+ fi
+}
+
+# add initial route - used in replace route tests
+add_initial_route6()
+{
+ add_route6 "2001:db8:104::/64" "$1"
+}
+
+check_route6()
+{
+ local pfx="2001:db8:104::/64"
+ local expected="$1"
+ local out
+ local rc=0
+
+ out=$($IP -6 ro ls match ${pfx} | sed -e 's/ pref medium//')
+ if [ -z "${out}" ]; then
+ if [ "$VERBOSE" = "1" ]; then
+ printf "\nNo route entry found\n"
+ printf "Expected:\n"
+ printf " ${expected}\n"
+ fi
+ return 1
+ fi
+
+ # tricky way to convert output to 1-line without ip's
+ # messy '\'; this drops all extra white space
+ out=$(echo ${out})
+ if [ "${out}" != "${expected}" ]; then
+ rc=1
+ if [ "${VERBOSE}" = "1" ]; then
+ printf " Unexpected route entry. Have:\n"
+ printf " ${out}\n"
+ printf " Expected:\n"
+ printf " ${expected}\n\n"
+ fi
+ fi
+
+ return $rc
+}
+
+route_cleanup()
+{
+ $IP li del red 2>/dev/null
+ $IP li del dummy1 2>/dev/null
+ $IP li del veth1 2>/dev/null
+ $IP li del veth3 2>/dev/null
+
+ cleanup &> /dev/null
+}
+
+route_setup()
+{
+ route_cleanup
+ setup
+
+ [ "${VERBOSE}" = "1" ] && set -x
+ set -e
+
+ $IP li add red up type vrf table 101
+ $IP li add veth1 type veth peer name veth2
+ $IP li add veth3 type veth peer name veth4
+
+ $IP li set veth1 up
+ $IP li set veth3 up
+ $IP li set veth2 vrf red up
+ $IP li set veth4 vrf red up
+ $IP li add dummy1 type dummy
+ $IP li set dummy1 vrf red up
+
+ $IP -6 addr add 2001:db8:101::1/64 dev veth1
+ $IP -6 addr add 2001:db8:101::2/64 dev veth2
+ $IP -6 addr add 2001:db8:103::1/64 dev veth3
+ $IP -6 addr add 2001:db8:103::2/64 dev veth4
+ $IP -6 addr add 2001:db8:104::1/64 dev dummy1
+
+ set +ex
+}
+
+# assumption is that basic add of a single path route works
+# otherwise just adding an address on an interface is broken
+ipv6_rt_add()
+{
+ local rc
+
+ echo
+ echo "IPv6 route add / append tests"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro add 2001:db8:104::/64 via 2001:db8:103::2"
+ log_test $? 2 "Attempt to add duplicate route - gw"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro add 2001:db8:104::/64 dev veth3"
+ log_test $? 2 "Attempt to add duplicate route - dev only"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro add unreachable 2001:db8:104::/64"
+ log_test $? 2 "Attempt to add duplicate route - reject route"
+
+ # iproute2 prepend only sets NLM_F_CREATE
+ # - adds a new route; does NOT convert existing route to ECMP
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro prepend 2001:db8:104::/64 via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 via 2001:db8:101::2 dev veth1 metric 1024 2001:db8:104::/64 via 2001:db8:103::2 dev veth3 metric 1024"
+ log_test $? 0 "Add new route for existing prefix (w/o NLM_F_EXCL)"
+
+ # route append with same prefix adds a new route
+ # - iproute2 sets NLM_F_CREATE | NLM_F_APPEND
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro append 2001:db8:104::/64 via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::2 dev veth1 weight 1 nexthop via 2001:db8:103::2 dev veth3 weight 1"
+ log_test $? 0 "Append nexthop to existing route - gw"
+
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro append 2001:db8:104::/64 dev veth3"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::2 dev veth1 weight 1 nexthop dev veth3 weight 1"
+ log_test $? 0 "Append nexthop to existing route - dev only"
+
+ # multipath route can not have a nexthop that is a reject route
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro append unreachable 2001:db8:104::/64"
+ log_test $? 2 "Append nexthop to existing route - reject route"
+
+ # reject route can not be converted to multipath route
+ run_cmd "$IP -6 ro flush 2001:db8:104::/64"
+ run_cmd "$IP -6 ro add unreachable 2001:db8:104::/64"
+ run_cmd "$IP -6 ro append 2001:db8:104::/64 via 2001:db8:103::2"
+ log_test $? 2 "Append nexthop to existing reject route - gw"
+
+ run_cmd "$IP -6 ro flush 2001:db8:104::/64"
+ run_cmd "$IP -6 ro add unreachable 2001:db8:104::/64"
+ run_cmd "$IP -6 ro append 2001:db8:104::/64 dev veth3"
+ log_test $? 2 "Append nexthop to existing reject route - dev only"
+
+ # insert mpath directly
+ add_route6 "2001:db8:104::/64" "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::2 dev veth1 weight 1 nexthop via 2001:db8:103::2 dev veth3 weight 1"
+ log_test $? 0 "Add multipath route"
+
+ add_route6 "2001:db8:104::/64" "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro add 2001:db8:104::/64 nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ log_test $? 2 "Attempt to add duplicate multipath route"
+
+ # insert of a second route without append but different metric
+ add_route6 "2001:db8:104::/64" "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro add 2001:db8:104::/64 via 2001:db8:103::2 metric 512"
+ rc=$?
+ if [ $rc -eq 0 ]; then
+ run_cmd "$IP -6 ro add 2001:db8:104::/64 via 2001:db8:103::3 metric 256"
+ rc=$?
+ fi
+ log_test $rc 0 "Route add with different metrics"
+
+ run_cmd "$IP -6 ro del 2001:db8:104::/64 metric 512"
+ rc=$?
+ if [ $rc -eq 0 ]; then
+ check_route6 "2001:db8:104::/64 via 2001:db8:103::3 dev veth3 metric 256 2001:db8:104::/64 via 2001:db8:101::2 dev veth1 metric 1024"
+ rc=$?
+ fi
+ log_test $rc 0 "Route delete with metric"
+}
+
+ipv6_rt_replace_single()
+{
+ # single path with single path
+ #
+ add_initial_route6 "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 via 2001:db8:103::2 dev veth3 metric 1024"
+ log_test $? 0 "Single path with single path"
+
+ # single path with multipath
+ #
+ add_initial_route6 "nexthop via 2001:db8:101::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:101::3 nexthop via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::3 dev veth1 weight 1 nexthop via 2001:db8:103::2 dev veth3 weight 1"
+ log_test $? 0 "Single path with multipath"
+
+ # single path with reject
+ #
+ add_initial_route6 "nexthop via 2001:db8:101::2"
+ run_cmd "$IP -6 ro replace unreachable 2001:db8:104::/64"
+ check_route6 "unreachable 2001:db8:104::/64 dev lo metric 1024"
+ log_test $? 0 "Single path with reject route"
+
+ # single path with single path using MULTIPATH attribute
+ #
+ add_initial_route6 "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:103::2"
+ check_route6 "2001:db8:104::/64 via 2001:db8:103::2 dev veth3 metric 1024"
+ log_test $? 0 "Single path with single path via multipath attribute"
+
+ # route replace fails - invalid nexthop
+ add_initial_route6 "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 via 2001:db8:104::2"
+ if [ $? -eq 0 ]; then
+ # previous command is expected to fail so if it returns 0
+ # that means the test failed.
+ log_test 0 1 "Invalid nexthop"
+ else
+ check_route6 "2001:db8:104::/64 via 2001:db8:101::2 dev veth1 metric 1024"
+ log_test $? 0 "Invalid nexthop"
+ fi
+
+ # replace non-existent route
+ # - note use of change versus replace since ip adds NLM_F_CREATE
+ # for replace
+ add_initial_route6 "via 2001:db8:101::2"
+ run_cmd "$IP -6 ro change 2001:db8:105::/64 via 2001:db8:101::2"
+ log_test $? 2 "Single path - replace of non-existent route"
+}
+
+ipv6_rt_replace_mpath()
+{
+ # multipath with multipath
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:101::3 nexthop via 2001:db8:103::3"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::3 dev veth1 weight 1 nexthop via 2001:db8:103::3 dev veth3 weight 1"
+ log_test $? 0 "Multipath with multipath"
+
+ # multipath with single
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 via 2001:db8:101::3"
+ check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
+ log_test $? 0 "Multipath with single path"
+
+ # multipath with single
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:101::3"
+ check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
+ log_test $? 0 "Multipath with single path via multipath attribute"
+
+ # multipath with reject
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace unreachable 2001:db8:104::/64"
+ check_route6 "unreachable 2001:db8:104::/64 dev lo metric 1024"
+ log_test $? 0 "Multipath with reject route"
+
+ # route replace fails - invalid nexthop 1
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:111::3 nexthop via 2001:db8:103::3"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::2 dev veth1 weight 1 nexthop via 2001:db8:103::2 dev veth3 weight 1"
+ log_test $? 0 "Multipath - invalid first nexthop"
+
+ # route replace fails - invalid nexthop 2
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:101::3 nexthop via 2001:db8:113::3"
+ check_route6 "2001:db8:104::/64 metric 1024 nexthop via 2001:db8:101::2 dev veth1 weight 1 nexthop via 2001:db8:103::2 dev veth3 weight 1"
+ log_test $? 0 "Multipath - invalid second nexthop"
+
+ # multipath non-existent route
+ add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
+ run_cmd "$IP -6 ro change 2001:db8:105::/64 nexthop via 2001:db8:101::3 nexthop via 2001:db8:103::3"
+ log_test $? 2 "Multipath - replace of non-existent route"
+}
+
+ipv6_rt_replace()
+{
+ echo
+ echo "IPv6 route replace tests"
+
+ ipv6_rt_replace_single
+ ipv6_rt_replace_mpath
+}
+
+ipv6_route_test()
+{
+ route_setup
+
+ ipv6_rt_add
+ ipv6_rt_replace
+
+ route_cleanup
+}
+
+################################################################################
# usage
usage()
@@ -635,6 +967,7 @@ do
fib_down_test|down) fib_down_test;;
fib_carrier_test|carrier) fib_carrier_test;;
fib_nexthop_test|nexthop) fib_nexthop_test;;
+ ipv6_route_test|ipv6_rt) ipv6_route_test;;
help) echo "Test names: $TESTS"; exit 0;;
esac
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 7/7] selftests: fib_tests: Add ipv4 route add append replace tests
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Add IPv4 route tests covering add, append and replace permutations.
Assumes the ability to add a basic single path route works; this is
required for example when adding an address to an interface.
$ fib_tests.sh -t ipv4_rt
IPv4 route add / append tests
TEST: Attempt to add duplicate route - gw [ OK ]
TEST: Attempt to add duplicate route - dev only [ OK ]
TEST: Attempt to add duplicate route - reject route [ OK ]
TEST: Add new nexthop for existing prefix [ OK ]
TEST: Append nexthop to existing route - gw [ OK ]
TEST: Append nexthop to existing route - dev only [ OK ]
TEST: Append nexthop to existing route - reject route [ OK ]
TEST: Append nexthop to existing reject route - gw [ OK ]
TEST: Append nexthop to existing reject route - dev only [ OK ]
TEST: add multipath route [ OK ]
TEST: Attempt to add duplicate multipath route [ OK ]
TEST: Route add with different metrics [ OK ]
TEST: Route delete with metric [ OK ]
IPv4 route replace tests
TEST: Single path with single path [ OK ]
TEST: Single path with multipath [ OK ]
TEST: Single path with reject route [ OK ]
TEST: Single path with single path via multipath attribute [ OK ]
TEST: Invalid nexthop [ OK ]
TEST: Single path - replace of non-existent route [ OK ]
TEST: Multipath with multipath [ OK ]
TEST: Multipath with single path [ OK ]
TEST: Multipath with single path via multipath attribute [ OK ]
TEST: Multipath with reject route [ OK ]
TEST: Multipath - invalid first nexthop [ OK ]
TEST: Multipath - invalid second nexthop [ OK ]
TEST: Multipath - replace of non-existent route [ OK ]
Signed-off-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/fib_tests.sh | 279 ++++++++++++++++++++++++++++++-
1 file changed, 278 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 384828f28372..e7d76fbc36e9 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -6,7 +6,7 @@
ret=0
-TESTS="unregister down carrier nexthop ipv6_rt"
+TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt"
VERBOSE=0
PAUSE_ON_FAIL=no
PAUSE=no
@@ -702,6 +702,12 @@ route_setup()
$IP -6 addr add 2001:db8:103::2/64 dev veth4
$IP -6 addr add 2001:db8:104::1/64 dev dummy1
+ $IP addr add 172.16.101.1/24 dev veth1
+ $IP addr add 172.16.101.2/24 dev veth2
+ $IP addr add 172.16.103.1/24 dev veth3
+ $IP addr add 172.16.103.2/24 dev veth4
+ $IP addr add 172.16.104.1/24 dev dummy1
+
set +ex
}
@@ -905,6 +911,276 @@ ipv6_route_test()
route_cleanup
}
+# add route for a prefix, flushing any existing routes first
+# expected to be the first step of a test
+add_route()
+{
+ local pfx="$1"
+ local nh="$2"
+ local out
+
+ if [ "$VERBOSE" = "1" ]; then
+ echo
+ echo " ##################################################"
+ echo
+ fi
+
+ run_cmd "$IP ro flush ${pfx}"
+ [ $? -ne 0 ] && exit 1
+
+ out=$($IP ro ls match ${pfx})
+ if [ -n "$out" ]; then
+ echo "Failed to flush routes for prefix used for tests."
+ exit 1
+ fi
+
+ run_cmd "$IP ro add ${pfx} ${nh}"
+ if [ $? -ne 0 ]; then
+ echo "Failed to add initial route for test."
+ exit 1
+ fi
+}
+
+# add initial route - used in replace route tests
+add_initial_route()
+{
+ add_route "172.16.104.0/24" "$1"
+}
+
+check_route()
+{
+ local pfx="172.16.104.0/24"
+ local expected="$1"
+ local out
+ local rc=0
+
+ out=$($IP ro ls match ${pfx})
+ if [ -z "${out}" ]; then
+ if [ "$VERBOSE" = "1" ]; then
+ printf "\nNo route entry found\n"
+ printf "Expected:\n"
+ printf " ${expected}\n"
+ fi
+ return 1
+ fi
+
+ # tricky way to convert output to 1-line without ip's
+ # messy '\'; this drops all extra white space
+ out=$(echo ${out})
+ if [ "${out}" != "${expected}" ]; then
+ rc=1
+ if [ "${VERBOSE}" = "1" ]; then
+ printf " Unexpected route entry. Have:\n"
+ printf " ${out}\n"
+ printf " Expected:\n"
+ printf " ${expected}\n\n"
+ fi
+ fi
+
+ return $rc
+}
+
+# assumption is that basic add of a single path route works
+# otherwise just adding an address on an interface is broken
+ipv4_rt_add()
+{
+ local rc
+
+ echo
+ echo "IPv4 route add / append tests"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro add 172.16.104.0/24 via 172.16.103.2"
+ log_test $? 2 "Attempt to add duplicate route - gw"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro add 172.16.104.0/24 dev veth3"
+ log_test $? 2 "Attempt to add duplicate route - dev only"
+
+ # route add same prefix - fails with EEXISTS b/c ip adds NLM_F_EXCL
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro add unreachable 172.16.104.0/24"
+ log_test $? 2 "Attempt to add duplicate route - reject route"
+
+ # iproute2 prepend only sets NLM_F_CREATE
+ # - adds a new route; does NOT convert existing route to ECMP
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro prepend 172.16.104.0/24 via 172.16.103.2"
+ check_route "172.16.104.0/24 via 172.16.103.2 dev veth3 172.16.104.0/24 via 172.16.101.2 dev veth1"
+ log_test $? 0 "Add new nexthop for existing prefix"
+
+ # route append with same prefix adds a new route
+ # - iproute2 sets NLM_F_CREATE | NLM_F_APPEND
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro append 172.16.104.0/24 via 172.16.103.2"
+ check_route "172.16.104.0/24 via 172.16.101.2 dev veth1 172.16.104.0/24 via 172.16.103.2 dev veth3"
+ log_test $? 0 "Append nexthop to existing route - gw"
+
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro append 172.16.104.0/24 dev veth3"
+ check_route "172.16.104.0/24 via 172.16.101.2 dev veth1 172.16.104.0/24 dev veth3 scope link"
+ log_test $? 0 "Append nexthop to existing route - dev only"
+
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro append unreachable 172.16.104.0/24"
+ check_route "172.16.104.0/24 via 172.16.101.2 dev veth1 unreachable 172.16.104.0/24"
+ log_test $? 0 "Append nexthop to existing route - reject route"
+
+ run_cmd "$IP ro flush 172.16.104.0/24"
+ run_cmd "$IP ro add unreachable 172.16.104.0/24"
+ run_cmd "$IP ro append 172.16.104.0/24 via 172.16.103.2"
+ check_route "unreachable 172.16.104.0/24 172.16.104.0/24 via 172.16.103.2 dev veth3"
+ log_test $? 0 "Append nexthop to existing reject route - gw"
+
+ run_cmd "$IP ro flush 172.16.104.0/24"
+ run_cmd "$IP ro add unreachable 172.16.104.0/24"
+ run_cmd "$IP ro append 172.16.104.0/24 dev veth3"
+ check_route "unreachable 172.16.104.0/24 172.16.104.0/24 dev veth3 scope link"
+ log_test $? 0 "Append nexthop to existing reject route - dev only"
+
+ # insert mpath directly
+ add_route "172.16.104.0/24" "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ check_route "172.16.104.0/24 nexthop via 172.16.101.2 dev veth1 weight 1 nexthop via 172.16.103.2 dev veth3 weight 1"
+ log_test $? 0 "add multipath route"
+
+ add_route "172.16.104.0/24" "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro add 172.16.104.0/24 nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ log_test $? 2 "Attempt to add duplicate multipath route"
+
+ # insert of a second route without append but different metric
+ add_route "172.16.104.0/24" "via 172.16.101.2"
+ run_cmd "$IP ro add 172.16.104.0/24 via 172.16.103.2 metric 512"
+ rc=$?
+ if [ $rc -eq 0 ]; then
+ run_cmd "$IP ro add 172.16.104.0/24 via 172.16.103.3 metric 256"
+ rc=$?
+ fi
+ log_test $rc 0 "Route add with different metrics"
+
+ run_cmd "$IP ro del 172.16.104.0/24 metric 512"
+ rc=$?
+ if [ $rc -eq 0 ]; then
+ check_route "172.16.104.0/24 via 172.16.101.2 dev veth1 172.16.104.0/24 via 172.16.103.3 dev veth3 metric 256"
+ rc=$?
+ fi
+ log_test $rc 0 "Route delete with metric"
+}
+
+ipv4_rt_replace_single()
+{
+ # single path with single path
+ #
+ add_initial_route "via 172.16.101.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 via 172.16.103.2"
+ check_route "172.16.104.0/24 via 172.16.103.2 dev veth3"
+ log_test $? 0 "Single path with single path"
+
+ # single path with multipath
+ #
+ add_initial_route "nexthop via 172.16.101.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.101.3 nexthop via 172.16.103.2"
+ check_route "172.16.104.0/24 nexthop via 172.16.101.3 dev veth1 weight 1 nexthop via 172.16.103.2 dev veth3 weight 1"
+ log_test $? 0 "Single path with multipath"
+
+ # single path with reject
+ #
+ add_initial_route "nexthop via 172.16.101.2"
+ run_cmd "$IP ro replace unreachable 172.16.104.0/24"
+ check_route "unreachable 172.16.104.0/24"
+ log_test $? 0 "Single path with reject route"
+
+ # single path with single path using MULTIPATH attribute
+ #
+ add_initial_route "via 172.16.101.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.103.2"
+ check_route "172.16.104.0/24 via 172.16.103.2 dev veth3"
+ log_test $? 0 "Single path with single path via multipath attribute"
+
+ # route replace fails - invalid nexthop
+ add_initial_route "via 172.16.101.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 via 2001:db8:104::2"
+ if [ $? -eq 0 ]; then
+ # previous command is expected to fail so if it returns 0
+ # that means the test failed.
+ log_test 0 1 "Invalid nexthop"
+ else
+ check_route "172.16.104.0/24 via 172.16.101.2 dev veth1"
+ log_test $? 0 "Invalid nexthop"
+ fi
+
+ # replace non-existent route
+ # - note use of change versus replace since ip adds NLM_F_CREATE
+ # for replace
+ add_initial_route "via 172.16.101.2"
+ run_cmd "$IP ro change 172.16.105.0/24 via 172.16.101.2"
+ log_test $? 2 "Single path - replace of non-existent route"
+}
+
+ipv4_rt_replace_mpath()
+{
+ # multipath with multipath
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.101.3 nexthop via 172.16.103.3"
+ check_route "172.16.104.0/24 nexthop via 172.16.101.3 dev veth1 weight 1 nexthop via 172.16.103.3 dev veth3 weight 1"
+ log_test $? 0 "Multipath with multipath"
+
+ # multipath with single
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 via 172.16.101.3"
+ check_route "172.16.104.0/24 via 172.16.101.3 dev veth1"
+ log_test $? 0 "Multipath with single path"
+
+ # multipath with single
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.101.3"
+ check_route "172.16.104.0/24 via 172.16.101.3 dev veth1"
+ log_test $? 0 "Multipath with single path via multipath attribute"
+
+ # multipath with reject
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace unreachable 172.16.104.0/24"
+ check_route "unreachable 172.16.104.0/24"
+ log_test $? 0 "Multipath with reject route"
+
+ # route replace fails - invalid nexthop 1
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.111.3 nexthop via 172.16.103.3"
+ check_route "172.16.104.0/24 nexthop via 172.16.101.2 dev veth1 weight 1 nexthop via 172.16.103.2 dev veth3 weight 1"
+ log_test $? 0 "Multipath - invalid first nexthop"
+
+ # route replace fails - invalid nexthop 2
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro replace 172.16.104.0/24 nexthop via 172.16.101.3 nexthop via 172.16.113.3"
+ check_route "172.16.104.0/24 nexthop via 172.16.101.2 dev veth1 weight 1 nexthop via 172.16.103.2 dev veth3 weight 1"
+ log_test $? 0 "Multipath - invalid second nexthop"
+
+ # multipath non-existent route
+ add_initial_route "nexthop via 172.16.101.2 nexthop via 172.16.103.2"
+ run_cmd "$IP ro change 172.16.105.0/24 nexthop via 172.16.101.3 nexthop via 172.16.103.3"
+ log_test $? 2 "Multipath - replace of non-existent route"
+}
+
+ipv4_rt_replace()
+{
+ echo
+ echo "IPv4 route replace tests"
+
+ ipv4_rt_replace_single
+ ipv4_rt_replace_mpath
+}
+
+ipv4_route_test()
+{
+ route_setup
+
+ ipv4_rt_add
+ ipv4_rt_replace
+
+ route_cleanup
+}
+
################################################################################
# usage
@@ -968,6 +1244,7 @@ do
fib_carrier_test|carrier) fib_carrier_test;;
fib_nexthop_test|nexthop) fib_nexthop_test;;
ipv6_route_test|ipv6_rt) ipv6_route_test;;
+ ipv4_route_test|ipv4_rt) ipv4_route_test;;
help) echo "Test names: $TESTS"; exit 0;;
esac
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 3/7] selftests: fib_tests: Add success-fail counts
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
As more tests are added, it is convenient to have a tally at the end.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/fib_tests.sh | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 9164e60d4b66..7e2291161e15 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -18,8 +18,10 @@ log_test()
if [ ${rc} -eq ${expected} ]; then
printf " TEST: %-60s [ OK ]\n" "${msg}"
+ nsuccess=$((nsuccess+1))
else
ret=1
+ nfail=$((nfail+1))
printf " TEST: %-60s [FAIL]\n" "${msg}"
if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
echo
@@ -598,4 +600,9 @@ cleanup &> /dev/null
fib_test
+if [ "$TESTS" != "none" ]; then
+ printf "\nTests passed: %3d\n" ${nsuccess}
+ printf "Tests failed: %3d\n" ${nfail}
+fi
+
exit $ret
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 2/7] net/ipv6: Simplify route replace and appending into multipath route
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Bring consistency to ipv6 route replace and append semantics.
Remove rt6_qualify_for_ecmp which is just guess work. It fails in 2 cases:
1. can not replace a route with a reject route. Existing code appends
a new route instead of replacing the existing one.
2. can not have a multipath route where a leg uses a dev only nexthop
Existing use cases affected by this change:
1. adding a route with existing prefix and metric using NLM_F_CREATE
without NLM_F_APPEND or NLM_F_EXCL (ie., what iproute2 calls
'prepend'). Existing code auto-determines that the new nexthop can
be appended to an existing route to create a multipath route. This
change breaks that by requiring the APPEND flag for the new route
to be added to an existing one. Instead the prepend just adds another
route entry.
2. route replace. Existing code replaces first matching multipath route
if new route is multipath capable and fallback to first matching
non-ECMP route (reject or dev only route) in case one isn't available.
New behavior replaces first matching route. (Thanks to Ido for spotting
this one)
Note: Newer iproute2 is needed to display multipath routes with a dev-only
nexthop. This is due to a bug in iproute2 and parsing nexthops.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
include/net/ip6_route.h | 6 --
net/ipv6/ip6_fib.c | 157 ++++++++++++++++++++++--------------------------
net/ipv6/route.c | 3 +-
3 files changed, 73 insertions(+), 93 deletions(-)
diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 4cf1ef935ed9..9e4d0f0aeb6d 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -66,12 +66,6 @@ static inline bool rt6_need_strict(const struct in6_addr *daddr)
(IPV6_ADDR_MULTICAST | IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
}
-static inline bool rt6_qualify_for_ecmp(const struct fib6_info *f6i)
-{
- return (f6i->fib6_flags & (RTF_GATEWAY|RTF_ADDRCONF|RTF_DYNAMIC)) ==
- RTF_GATEWAY;
-}
-
void ip6_route_input(struct sk_buff *skb);
struct dst_entry *ip6_route_input_lookup(struct net *net,
struct net_device *dev,
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index d1dc6017f5a6..f9132a6de917 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -934,19 +934,19 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
{
struct fib6_info *leaf = rcu_dereference_protected(fn->leaf,
lockdep_is_held(&rt->fib6_table->tb6_lock));
- struct fib6_info *iter = NULL;
+ struct fib6_info *iter = NULL, *match = NULL;
struct fib6_info __rcu **ins;
- struct fib6_info __rcu **fallback_ins = NULL;
int replace = (info->nlh &&
(info->nlh->nlmsg_flags & NLM_F_REPLACE));
+ int append = (info->nlh &&
+ (info->nlh->nlmsg_flags & NLM_F_APPEND));
int add = (!info->nlh ||
(info->nlh->nlmsg_flags & NLM_F_CREATE));
int found = 0;
- bool rt_can_ecmp = rt6_qualify_for_ecmp(rt);
u16 nlflags = NLM_F_EXCL;
int err;
- if (info->nlh && (info->nlh->nlmsg_flags & NLM_F_APPEND))
+ if (append)
nlflags |= NLM_F_APPEND;
ins = &fn->leaf;
@@ -968,13 +968,8 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
nlflags &= ~NLM_F_EXCL;
if (replace) {
- if (rt_can_ecmp == rt6_qualify_for_ecmp(iter)) {
- found++;
- break;
- }
- if (rt_can_ecmp)
- fallback_ins = fallback_ins ?: ins;
- goto next_iter;
+ found++;
+ break;
}
if (rt6_duplicate_nexthop(iter, rt)) {
@@ -989,86 +984,67 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
fib6_metric_set(iter, RTAX_MTU, rt->fib6_pmtu);
return -EEXIST;
}
- /* If we have the same destination and the same metric,
- * but not the same gateway, then the route we try to
- * add is sibling to this route, increment our counter
- * of siblings, and later we will add our route to the
- * list.
- * Only static routes (which don't have flag
- * RTF_EXPIRES) are used for ECMPv6.
- *
- * To avoid long list, we only had siblings if the
- * route have a gateway.
- */
- if (rt_can_ecmp &&
- rt6_qualify_for_ecmp(iter))
- rt->fib6_nsiblings++;
+
+ /* first route that matches */
+ if (!match)
+ match = iter;
}
if (iter->fib6_metric > rt->fib6_metric)
break;
-next_iter:
ins = &iter->fib6_next;
}
- if (fallback_ins && !found) {
- /* No ECMP-able route found, replace first non-ECMP one */
- ins = fallback_ins;
- iter = rcu_dereference_protected(*ins,
- lockdep_is_held(&rt->fib6_table->tb6_lock));
- found++;
- }
-
/* Reset round-robin state, if necessary */
if (ins == &fn->leaf)
fn->rr_ptr = NULL;
/* Link this route to others same route. */
- if (rt->fib6_nsiblings) {
- unsigned int fib6_nsiblings;
+ if (append && match) {
struct fib6_info *sibling, *temp_sibling;
- /* Find the first route that have the same metric */
- sibling = leaf;
- while (sibling) {
- if (sibling->fib6_metric == rt->fib6_metric &&
- rt6_qualify_for_ecmp(sibling)) {
- list_add_tail(&rt->fib6_siblings,
- &sibling->fib6_siblings);
- break;
- }
- sibling = rcu_dereference_protected(sibling->fib6_next,
- lockdep_is_held(&rt->fib6_table->tb6_lock));
+ if (rt->fib6_flags & RTF_REJECT) {
+ NL_SET_ERR_MSG(extack,
+ "Can not append a REJECT route");
+ return -EINVAL;
+ } else if (match->fib6_flags & RTF_REJECT) {
+ NL_SET_ERR_MSG(extack,
+ "Can not append to a REJECT route");
+ return -EINVAL;
}
+ rt->fib6_nsiblings = match->fib6_nsiblings;
+ list_add_tail(&rt->fib6_siblings, &match->fib6_siblings);
+ match->fib6_nsiblings++;
+
/* For each sibling in the list, increment the counter of
* siblings. BUG() if counters does not match, list of siblings
* is broken!
*/
- fib6_nsiblings = 0;
list_for_each_entry_safe(sibling, temp_sibling,
- &rt->fib6_siblings, fib6_siblings) {
+ &match->fib6_siblings, fib6_siblings) {
sibling->fib6_nsiblings++;
- BUG_ON(sibling->fib6_nsiblings != rt->fib6_nsiblings);
- fib6_nsiblings++;
+ BUG_ON(sibling->fib6_nsiblings != match->fib6_nsiblings);
}
- BUG_ON(fib6_nsiblings != rt->fib6_nsiblings);
- rt6_multipath_rebalance(temp_sibling);
+
+ rt6_multipath_rebalance(match);
}
/*
* insert node
*/
if (!replace) {
+ enum fib_event_type event;
+
if (!add)
pr_warn("NLM_F_CREATE should be set when creating new route\n");
add:
nlflags |= NLM_F_CREATE;
- err = call_fib6_entry_notifiers(info->nl_net,
- FIB_EVENT_ENTRY_ADD,
- rt, extack);
+ event = append ? FIB_EVENT_ENTRY_APPEND : FIB_EVENT_ENTRY_ADD;
+ err = call_fib6_entry_notifiers(info->nl_net, event, rt,
+ extack);
if (err)
return err;
@@ -1086,7 +1062,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
}
} else {
- int nsiblings;
+ struct fib6_info *tmp;
if (!found) {
if (add)
@@ -1101,48 +1077,57 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
if (err)
return err;
+ /* if route being replaced has siblings, set tmp to
+ * last one, otherwise tmp is current route. this is
+ * used to set fib6_next for new route
+ */
+ if (iter->fib6_nsiblings)
+ tmp = list_last_entry(&iter->fib6_siblings,
+ struct fib6_info,
+ fib6_siblings);
+ else
+ tmp = iter;
+
+ /* insert new route */
atomic_inc(&rt->fib6_ref);
rcu_assign_pointer(rt->fib6_node, fn);
- rt->fib6_next = iter->fib6_next;
+ rt->fib6_next = tmp->fib6_next;
rcu_assign_pointer(*ins, rt);
+
if (!info->skip_notify)
inet6_rt_notify(RTM_NEWROUTE, rt, info, NLM_F_REPLACE);
if (!(fn->fn_flags & RTN_RTINFO)) {
info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
fn->fn_flags |= RTN_RTINFO;
}
- nsiblings = iter->fib6_nsiblings;
- iter->fib6_node = NULL;
- fib6_purge_rt(iter, fn, info->nl_net);
- if (rcu_access_pointer(fn->rr_ptr) == iter)
- fn->rr_ptr = NULL;
- fib6_info_release(iter);
- if (nsiblings) {
+ /* delete old route */
+ rt = iter;
+
+ if (rt->fib6_nsiblings) {
+ struct fib6_info *tmp;
+
/* Replacing an ECMP route, remove all siblings */
- ins = &rt->fib6_next;
- iter = rcu_dereference_protected(*ins,
- lockdep_is_held(&rt->fib6_table->tb6_lock));
- while (iter) {
- if (iter->fib6_metric > rt->fib6_metric)
- break;
- if (rt6_qualify_for_ecmp(iter)) {
- *ins = iter->fib6_next;
- iter->fib6_node = NULL;
- fib6_purge_rt(iter, fn, info->nl_net);
- if (rcu_access_pointer(fn->rr_ptr) == iter)
- fn->rr_ptr = NULL;
- fib6_info_release(iter);
- nsiblings--;
- info->nl_net->ipv6.rt6_stats->fib_rt_entries--;
- } else {
- ins = &iter->fib6_next;
- }
- iter = rcu_dereference_protected(*ins,
- lockdep_is_held(&rt->fib6_table->tb6_lock));
+ list_for_each_entry_safe(iter, tmp, &rt->fib6_siblings,
+ fib6_siblings) {
+ iter->fib6_node = NULL;
+ fib6_purge_rt(iter, fn, info->nl_net);
+ if (rcu_access_pointer(fn->rr_ptr) == iter)
+ fn->rr_ptr = NULL;
+ fib6_info_release(iter);
+
+ rt->fib6_nsiblings--;
+ info->nl_net->ipv6.rt6_stats->fib_rt_entries--;
}
- WARN_ON(nsiblings != 0);
}
+
+ WARN_ON(rt->fib6_nsiblings != 0);
+
+ rt->fib6_node = NULL;
+ fib6_purge_rt(rt, fn, info->nl_net);
+ if (rcu_access_pointer(fn->rr_ptr) == rt)
+ fn->rr_ptr = NULL;
+ fib6_info_release(rt);
}
return 0;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cc24ed3bc334..bcb8785c0451 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3791,7 +3791,7 @@ static struct fib6_info *rt6_multipath_first_sibling(const struct fib6_info *rt)
lockdep_is_held(&rt->fib6_table->tb6_lock));
while (iter) {
if (iter->fib6_metric == rt->fib6_metric &&
- rt6_qualify_for_ecmp(iter))
+ iter->fib6_nsiblings)
return iter;
iter = rcu_dereference_protected(iter->fib6_next,
lockdep_is_held(&rt->fib6_table->tb6_lock));
@@ -4381,6 +4381,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
*/
cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL |
NLM_F_REPLACE);
+ cfg->fc_nlinfo.nlh->nlmsg_flags |= NLM_F_APPEND;
nhn++;
}
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 4/7] selftests: fib_tests: Add command line options
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Add command line options for controlling pause on fail, controlling
specific tests to run and verbose mode rather than relying on environment
variables.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/fib_tests.sh | 53 ++++++++++++++++++++++++--------
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 7e2291161e15..8c99f0689efc 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -6,8 +6,9 @@
ret=0
-VERBOSE=${VERBOSE:=0}
-PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
+TESTS="unregister down carrier nexthop"
+VERBOSE=0
+PAUSE_ON_FAIL=no
IP="ip -netns testns"
log_test()
@@ -565,20 +566,36 @@ fib_nexthop_test()
}
################################################################################
-#
+# usage
-fib_test()
+usage()
{
- if [ -n "$TEST" ]; then
- eval $TEST
- else
- fib_unreg_test
- fib_down_test
- fib_carrier_test
- fib_nexthop_test
- fi
+ cat <<EOF
+usage: ${0##*/} OPTS
+
+ -t <test> Test(s) to run (default: all)
+ (options: $TESTS)
+ -p Pause on fail
+ -v verbose mode (show commands and output)
+EOF
}
+################################################################################
+# main
+
+while getopts :t:pPhv o
+do
+ case $o in
+ t) TESTS=$OPTARG;;
+ p) PAUSE_ON_FAIL=yes;;
+ v) VERBOSE=$(($VERBOSE + 1));;
+ h) usage; exit 0;;
+ *) usage; exit 1;;
+ esac
+done
+
+PEER_CMD="ip netns exec ${PEER_NS}"
+
if [ "$(id -u)" -ne 0 ];then
echo "SKIP: Need root privileges"
exit 0
@@ -598,7 +615,17 @@ fi
# start clean
cleanup &> /dev/null
-fib_test
+for t in $TESTS
+do
+ case $t in
+ fib_unreg_test|unregister) fib_unreg_test;;
+ fib_down_test|down) fib_down_test;;
+ fib_carrier_test|carrier) fib_carrier_test;;
+ fib_nexthop_test|nexthop) fib_nexthop_test;;
+
+ help) echo "Test names: $TESTS"; exit 0;;
+ esac
+done
if [ "$TESTS" != "none" ]; then
printf "\nTests passed: %3d\n" ${nsuccess}
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 5/7] selftests: fib_tests: Add option to pause after each test
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Add option to pause after each test before cleanup is done. Allows
user to do manual inspection or more ad-hoc testing after each test
with the setup in tact.
Signed-off-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 8c99f0689efc..12b648826151 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -9,6 +9,7 @@ ret=0
TESTS="unregister down carrier nexthop"
VERBOSE=0
PAUSE_ON_FAIL=no
+PAUSE=no
IP="ip -netns testns"
log_test()
@@ -31,6 +32,13 @@ log_test()
[ "$a" = "q" ] && exit 1
fi
fi
+
+ if [ "${PAUSE}" = "yes" ]; then
+ echo
+ echo "hit enter to continue, 'q' to quit"
+ read a
+ [ "$a" = "q" ] && exit 1
+ fi
}
setup()
@@ -576,6 +584,7 @@ usage: ${0##*/} OPTS
-t <test> Test(s) to run (default: all)
(options: $TESTS)
-p Pause on fail
+ -P Pause after each test before cleanup
-v verbose mode (show commands and output)
EOF
}
@@ -588,6 +597,7 @@ do
case $o in
t) TESTS=$OPTARG;;
p) PAUSE_ON_FAIL=yes;;
+ P) PAUSE=yes;;
v) VERBOSE=$(($VERBOSE + 1));;
h) usage; exit 0;;
*) usage; exit 1;;
@@ -596,6 +606,9 @@ done
PEER_CMD="ip netns exec ${PEER_NS}"
+# make sure we don't pause twice
+[ "${PAUSE}" = "yes" ] && PAUSE_ON_FAIL=no
+
if [ "$(id -u)" -ne 0 ];then
echo "SKIP: Need root privileges"
exit 0
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 0/7] net/ipv6: Fix route append and replace use cases
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
From: David Ahern <dsahern@gmail.com>
This patch set fixes a few append and replace uses cases for IPv6 and
adds test cases that codifies the expectations of how append and replace
are expected to work. In paricular it allows a multipath route to have
a dev-only nexthop, something Thomas tried to accomplish with commit
edd7ceb78296 ("ipv6: Allow non-gateway ECMP for IPv6") which had to be
reverted because of breakage, and to replace an existing FIB entry
with a reject route.
There are a number of inconsistent and surprising aspects to the Linux
API for adding, deleting, replacing and changing FIB entries. For example,
with IPv4 NLM_F_APPEND means insert the route after any existing entries
with the same key (prefix + priority + TOS for IPv4) and NLM_F_CREATE
without the append flag inserts the new route before any existing entries.
IPv6 on the other hand attempts to guess whether a new route should be
appended to an existing one, possibly creating a multipath route, or to
add a new entry after any existing ones. This applies to both the 'append'
(NLM_F_CREATE + NLM_F_APPEND) and 'prepend' (NLM_F_CREATE only) cases
meaning for IPv6 the NLM_F_APPEND is basically ignored. This guessing
whether the route should be added to a multipath route (gateway routes)
or inserted after existing entries (non-gateway based routes) means a
multipath route can not have a dev only nexthop (potentially required in
some cases - tunnels or VRF route leaking for example) and route 'replace'
is a bit adhoc treating gateway based routes and dev-only / reject routes
differently.
This has led to frustration with developers working on routing suites
such as FRR where workarounds such as delete and add are used instead of
replace.
After this patch set there are 2 differences between IPv4 and IPv6:
1. 'ip ro prepend' = NLM_F_CREATE only
IPv4 adds the new route before any existing ones
IPv6 adds new route after any existing ones
2. 'ip ro append' = NLM_F_CREATE|NLM_F_APPEND
IPv4 adds the new route after any existing ones
IPv6 adds the nexthop to existing routes converting to multipath
For the former, there are cases where we want same prefix routes added
after existing ones (e.g., multicast, prefix routes for macvlan when used
for virtual router redundancy). Requiring the APPEND flag to add a new
route to an existing one helps here but is a slight change in behavior
since prepend with gateway routes now create a separate entry.
For the latter IPv6 behavior is preferred - appending a route for the same
prefix and metric to make a multipath route, so really IPv4 not allowing an
existing route to be updated is the limiter. This will be fixed when
nexthops become separate objects - a future patch set.
Thank you to Thomas and Ido for testing earlier versions of this set, and
to Ido for providing an update to the mlxsw driver.
Changes since RFC
- cleanup wording in test script; add comments about expected failures
and why
David Ahern (7):
mlxsw: spectrum_router: Add support for route append
net/ipv6: Simplify route replace and appending into multipath route
selftests: fib_tests: Add success-fail counts
selftests: fib_tests: Add command line options
selftests: fib_tests: Add option to pause after each test
selftests: fib_tests: Add ipv6 route add append replace tests
selftests: fib_tests: Add ipv4 route add append replace tests
.../net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +
include/net/ip6_route.h | 6 -
net/ipv6/ip6_fib.c | 157 +++--
net/ipv6/route.c | 3 +-
tools/testing/selftests/net/fib_tests.sh | 679 ++++++++++++++++++++-
5 files changed, 743 insertions(+), 104 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH net-next 1/7] mlxsw: spectrum_router: Add support for route append
From: dsahern @ 2018-05-21 17:26 UTC (permalink / raw)
To: netdev; +Cc: Thomas.Winter, idosch, sharpd, roopa, David Ahern
In-Reply-To: <20180521172658.7389-1-dsahern@kernel.org>
From: David Ahern <dsahern@gmail.com>
Handle append for gateway based routes. Dev-only multipath routes will
be handled by a follow on patch.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 8028d221aece..77b2adb29341 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -5725,6 +5725,7 @@ static void mlxsw_sp_router_fib6_event_work(struct work_struct *work)
switch (fib_work->event) {
case FIB_EVENT_ENTRY_REPLACE: /* fall through */
+ case FIB_EVENT_ENTRY_APPEND: /* fall through */
case FIB_EVENT_ENTRY_ADD:
replace = fib_work->event == FIB_EVENT_ENTRY_REPLACE;
err = mlxsw_sp_router_fib6_add(mlxsw_sp,
@@ -5831,6 +5832,7 @@ static void mlxsw_sp_router_fib6_event(struct mlxsw_sp_fib_event_work *fib_work,
switch (fib_work->event) {
case FIB_EVENT_ENTRY_REPLACE: /* fall through */
+ case FIB_EVENT_ENTRY_APPEND: /* fall through */
case FIB_EVENT_ENTRY_ADD: /* fall through */
case FIB_EVENT_ENTRY_DEL:
fen6_info = container_of(info, struct fib6_entry_notifier_info,
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next v13 3/7] sch_cake: Add optional ACK filter
From: Eric Dumazet @ 2018-05-21 17:18 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, netdev, cake
In-Reply-To: <152691985736.4083.3416150561524698541.stgit@alrua-kau>
On 05/21/2018 09:24 AM, Toke Høiland-Jørgensen wrote:
> + while (oplen_tmp >= 8) {
> + u32 right_b = get_unaligned_be32(sack_tmp + 4);
> + u32 left_b = get_unaligned_be32(sack_tmp);
> +
> + if (left_b >= right_b)
> + continue;
> +
> + if (first)
> + bytes_b += right_b - left_b;
> +
> + if (left_b <= left_a && right_a <= right_b) {
> + found = true;
> + if (!first)
> + break;
> + }
> + oplen_tmp -= 8;
> + sack_tmp += 8;
> + }
>
This is obviously incorrect.
Please take a look at TCP stack, and how it handles sequence wrapping with following macros :
after(), before()
Quite frankly O wonder how this was really tested.
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] selftests/bpf: test_sockmap, fix test timeout
From: John Fastabend @ 2018-05-21 17:13 UTC (permalink / raw)
To: Prashant Bhole, Alexei Starovoitov, Daniel Borkmann
Cc: David S . Miller, Shuah Khan, netdev
In-Reply-To: <615c089e-47a6-e36c-9ad4-2c86a8edeaa1@lab.ntt.co.jp>
On 05/20/2018 10:15 PM, Prashant Bhole wrote:
>
>
> On 5/19/2018 1:47 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> In order to reduce runtime of tests, recently timout for select() call
>>> was reduced from 1sec to 10usec. This was causing many tests failures.
>>> It was caught with failure handling commits in this series.
>>>
>>> Restoring the timeout from 10usec to 1sec
>>>
>>> Fixes: a18fda1a62c3 ("bpf: reduce runtime of test_sockmap tests")
>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>> ---
>>
>> Whats the runtime for the entire test suite after this? I agree
>> I was probably to aggressive in setting this but on the other
>> hand I was trying to avoid letting the test run for minutes.
>>
>
> Currently any failure stops further tests.
> So I made local change to force cork=0. With this change, it takes
> around 60 seconds for all 648 tests. It will change after fixing cork
> tests.
>
> -Prashant
>
OK, lets see how long it takes with the cork tests fixed. Maybe if
the cork flag is set we should lower the timeout to 100us.
.John
^ permalink raw reply
* Re: [PATCH bpf-next 2/5] selftests/bpf: test_sockmap, join cgroup in selftest mode
From: John Fastabend @ 2018-05-21 17:10 UTC (permalink / raw)
To: Prashant Bhole, Alexei Starovoitov, Daniel Borkmann
Cc: David S . Miller, Shuah Khan, netdev
In-Reply-To: <f8320fed-076d-9a93-ef93-ea7f3cdbf88f@lab.ntt.co.jp>
On 05/20/2018 10:15 PM, Prashant Bhole wrote:
>
>
> On 5/19/2018 1:45 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> In case of selftest mode, temporary cgroup environment is created but
>>> cgroup is not joined. It causes test failures. Fixed by joining the
>>> cgroup
>>>
>>> Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>> --
>>
>> Thanks, LGTM. Should this be the first patch in the series though?
>> I wonder if after patch 1 if you would get failures without this
>> patch.
>
> Patch 1 fixes selftest mode as well as manual mode. This patch 2 is
> specifically for selftest mode, hence the sequence.
>
> - Prashant
>
>
OK.
^ permalink raw reply
* Re: [PATCH bpf-next 0/5] fix test_sockmap
From: John Fastabend @ 2018-05-21 17:08 UTC (permalink / raw)
To: Prashant Bhole, Alexei Starovoitov, Daniel Borkmann
Cc: David S . Miller, Shuah Khan, netdev
In-Reply-To: <5e08aa3f-acfb-f6a9-e918-bca2393ea95f@lab.ntt.co.jp>
On 05/20/2018 10:13 PM, Prashant Bhole wrote:
>
>
> On 5/19/2018 1:42 AM, John Fastabend wrote:
>> On 05/18/2018 12:17 AM, Prashant Bhole wrote:
>>> This series fixes bugs in test_sockmap code. They weren't caught
>>> previously because failure in RX/TX thread was not notified to the
>>> main thread.
>>>
>>> Also fixed data verification logic and slightly improved test output
>>> such that parameters values (cork, apply, start, end) of failed test
>>> can be easily seen.
>>>
>>
>> Great, this was on my list so thanks for taking care of it.
>>
>>> Note: Even after fixing above problems there are issues with tests
>>> which set cork parameter. Tests fail (RX thread timeout) when cork
>>> value is non-zero and overall data sent by TX thread isn't multiples
>>> of cork value.
>>
>>
>> This is expected. When 'cork' is set the sender should only xmit
>> the data when 'cork' bytes are available. If the user doesn't
>> provide the N bytes the data is cork'ed waiting for the bytes and
>> if the socket is closed the state is cleaned up. What these tests
>> are testing is the cleanup path when a user doesn't provide the
>> N bytes. In practice this is used to validate headers and prevent
>> users from sending partial headers. We want to keep these tests because
>> they verify a tear-down path in the code.
>
> Ok.
>
>>
>> After your changes do these get reported as failures? If so we
>> need to account for the above in the calculations.
>
> Yes, cork related test are reported as failures because of RX thread
> timeout.
>
> So with your above description, I think we need to differentiate cork
> tests with partial data and full data. In partial data test we can have
> something like "timeout_expected" flag. Any other way to fix it?
>
Adding a flag seems reasonable to me. Lets do this for now. Also I
plan to add more negative tests so we can either use the same
flag or a new one for those cases as well.
Thanks,
John
^ permalink raw reply
* Re: [PATCH v2] packet: track ring entry use using a shadow ring to prevent RX ring overrun
From: Willem de Bruijn @ 2018-05-21 17:07 UTC (permalink / raw)
To: Jon Rosen (jrosen)
Cc: David S. Miller, Willem de Bruijn, Eric Dumazet, Kees Cook,
David Windsor, Rosen, Rami, Reshetova, Elena, Mike Maloney,
Benjamin Poirier, Thomas Gleixner, Greg Kroah-Hartman,
open list:NETWORKING [GENERAL], open list
In-Reply-To: <b7f6cc54a9f04132a279b15de0187ddd@XCH-RTP-016.cisco.com>
On Mon, May 21, 2018 at 8:57 AM, Jon Rosen (jrosen) <jrosen@cisco.com> wrote:
> On Sunday, May 20, 2018 7:22 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Sun, May 20, 2018 at 6:51 PM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>> On Sat, May 19, 2018 at 8:07 AM, Jon Rosen <jrosen@cisco.com> wrote:
>>>> Fix PACKET_RX_RING bug for versions TPACKET_V1 and TPACKET_V2 which
>>>> casues the ring to get corrupted by allowing multiple kernel threads
>>>> to claim ownership of the same ring entry. Track ownership in a shadow
>>>> ring structure to prevent other kernel threads from reusing the same
>>>> entry before it's fully filled in, passed to user space, and then
>>>> eventually passed back to the kernel for use with a new packet.
>>>>
>>>> Signed-off-by: Jon Rosen <jrosen@cisco.com>
>>>> ---
>>>>
>>>> There is a bug in net/packet/af_packet.c:tpacket_rcv in how it manages
>>>> the PACKET_RX_RING for versions TPACKET_V1 and TPACKET_V2. This bug makes
>>>> it possible for multiple kernel threads to claim ownership of the same
>>>> ring entry, corrupting the ring and the corresponding packet(s).
>>>>
>>>> These diffs are the second proposed solution, previous proposal was described
>>>> in https://www.mail-archive.com/netdev@vger.kernel.org/msg227468.html
>>>> subject [RFC PATCH] packet: mark ring entry as in-use inside spin_lock
>>>> to prevent RX ring overrun
>>>>
>>>> Those diffs would have changed the binary interface and have broken certain
>>>> applications. Consensus was that such a change would be inappropriate.
>>>>
>>>> These new diffs use a shadow ring in kernel space for tracking intermediate
>>>> state of an entry and prevent more than one kernel thread from simultaneously
>>>> allocating a ring entry. This avoids any impact to the binary interface
>>>> between kernel and userspace but comes at the additional cost of requiring a
>>>> second spin_lock when passing ownership of a ring entry to userspace.
>>>>
>>>> Jon Rosen (1):
>>>> packet: track ring entry use using a shadow ring to prevent RX ring
>>>> overrun
>>>>
>>>> net/packet/af_packet.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> net/packet/internal.h | 14 +++++++++++
>>>> 2 files changed, 78 insertions(+)
>>>>
>>>
>>>> @@ -2383,7 +2412,11 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>>>> #endif
>>>>
>>>> if (po->tp_version <= TPACKET_V2) {
>>>> + spin_lock(&sk->sk_receive_queue.lock);
>>>> __packet_set_status(po, h.raw, status);
>>>> + packet_rx_shadow_release(rx_shadow_ring_entry);
>>>> + spin_unlock(&sk->sk_receive_queue.lock);
>>>> +
>>>> sk->sk_data_ready(sk);
>>>
>>> Thanks for continuing to look at this. I spent some time on it last time
>>> around but got stuck, too.
>>>
>>> This version takes an extra spinlock in the hot path. That will be very
>>> expensive. Once we need to accept that, we could opt for a simpler
>>> implementation akin to the one discussed in the previous thread:
>>>
>>> stash a value in tp_padding or similar while tp_status remains
>>> TP_STATUS_KERNEL to signal ownership to concurrent kernel
>>> threads. The issue previously was that that field could not atomically
>>> be cleared together with __packet_set_status. This is no longer
>>> an issue when holding the queue lock.
>>>
>>> With a field like tp_padding, unlike tp_len, it is arguably also safe to
>>> clear it after flipping status (userspace should treat it as undefined).
>>>
>>> With v1 tpacket_hdr, no explicit padding field is defined but due to
>>> TPACKET_HDRLEN alignment it exists on both 32 and 64 bit
>>> platforms.
>>>
>>> The danger with using padding is that a process may write to it
>>> and cause deadlock, of course. There is no logical reason for doing
>>> so.
>>
>> For the ring, there is no requirement to allocate exactly the amount
>> specified by the user request. Safer than relying on shared memory
>> and simpler than the extra allocation in this patch would be to allocate
>> extra shadow memory at the end of the ring (and not mmap that).
>>
>> That still leaves an extra cold cacheline vs using tp_padding.
>
> Given my lack of experience and knowledge in writing kernel code
> it was easier for me to allocate the shadow ring as a separate
> structure. Of course it's not about me and my skills so if it's
> more appropriate to allocate at the tail of the existing ring
> then certainly I can look at doing that.
>
> I think the bigger issues as you've pointed out are the cost of
> the additional spin lock and should the additional state be
> stored in-band (fewer cache lines) or out-of band (less risk of
> breaking due to unpredictable application behavior).
We don't need the spinlock if clearing the shadow byte after
setting the status to user.
Worst case, user will set it back to kernel while the shadow
byte is not cleared yet and the next producer will drop a packet.
But next producers will make progress, so there is no deadlock
or corruption.
It probably does require a shadow structure as opposed to a
padding byte to work with the long tail of (possibly broken)
applications, sadly.
A setsockopt for userspace to signal a stricter interpretation of
tp_status to elide the shadow hack could then be considered.
It's not pretty. Either way, no full new version is required.
> As much as I would like to find a solution that doesn't require
> the spin lock I have yet to do so. Maybe the answer is that
> existing applications will need to suffer the performance impact
> but a new version or option for TPACKET_V1/V2 could be added to
> indicate strict adherence of the TP_STATUS_USER bit and then the
> original diffs could be used.
>
> There is another option I was considering but have yet to try
> which would avoid needing a shadow ring by using counter(s) to
> track maximum sequence number queued to userspace vs. the next
> sequence number to be allocated in the ring. If the difference
> is greater than the size of the ring then the ring can be
> considered full and the allocation would fail. Of course this may
> create an additional hotspot between cores, not sure if that
> would be significant or not.
Please do have a look, but I don't think that this will work in this
case in practice. It requires tracking the producer tail. Updating
the slowest writer requires probing each subsequent slot's status
byte to find the new tail, which is a lot of (by then cold) cacheline
reads.
^ permalink raw reply
* [PATCH net-next] r8169: perform reset synchronously in __rtl8169_resume
From: Heiner Kallweit @ 2018-05-21 17:01 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev@vger.kernel.org
The driver uses pm_runtime_get_sync() in few places and relies on the
device being fully runtime-resumed after this call. So far however
the runtime resume callback triggers an asynchronous reset.
Avoid this and perform the reset synchronously.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 75dfac024..1eb4f625a 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7327,9 +7327,9 @@ static void __rtl8169_resume(struct net_device *dev)
rtl_lock_work(tp);
napi_enable(&tp->napi);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
+ if (netif_running(dev))
+ rtl_reset_work(tp);
rtl_unlock_work(tp);
-
- rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
}
static int rtl8169_resume(struct device *device)
--
2.17.0
^ permalink raw reply related
* [PATCH bpf-next 6/7] bpf: btf: Sync bpf.h and btf.h to tools/include/uapi/linux/
From: Martin KaFai Lau @ 2018-05-21 16:57 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20180519001650.4043980-1-kafai@fb.com>
This patch sync the uapi's bpf.h and btf.h to tools/.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/include/uapi/linux/bpf.h | 8 ++++----
tools/include/uapi/linux/btf.h | 28 +++++++---------------------
2 files changed, 11 insertions(+), 25 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d94d333a8225..123ebe4b3662 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -284,8 +284,8 @@ union bpf_attr {
char map_name[BPF_OBJ_NAME_LEN];
__u32 map_ifindex; /* ifindex of netdev to create on */
__u32 btf_fd; /* fd pointing to a BTF type data */
- __u32 btf_key_id; /* BTF type_id of the key */
- __u32 btf_value_id; /* BTF type_id of the value */
+ __u32 btf_key_type_id; /* BTF type_id of the key */
+ __u32 btf_value_type_id; /* BTF type_id of the value */
};
struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
@@ -2211,8 +2211,8 @@ struct bpf_map_info {
__u64 netns_dev;
__u64 netns_ino;
__u32 btf_id;
- __u32 btf_key_id;
- __u32 btf_value_id;
+ __u32 btf_key_type_id;
+ __u32 btf_value_type_id;
} __attribute__((aligned(8)));
struct bpf_btf_info {
diff --git a/tools/include/uapi/linux/btf.h b/tools/include/uapi/linux/btf.h
index bcb56ee47014..b89b56f2b099 100644
--- a/tools/include/uapi/linux/btf.h
+++ b/tools/include/uapi/linux/btf.h
@@ -12,42 +12,29 @@ struct btf_header {
__u16 magic;
__u8 version;
__u8 flags;
-
- __u32 parent_label;
- __u32 parent_name;
+ __u32 hdr_len;
/* All offsets are in bytes relative to the end of this header */
- __u32 label_off; /* offset of label section */
- __u32 object_off; /* offset of data object section*/
- __u32 func_off; /* offset of function section */
__u32 type_off; /* offset of type section */
+ __u32 type_len; /* length of type section */
__u32 str_off; /* offset of string section */
__u32 str_len; /* length of string section */
};
/* Max # of type identifier */
-#define BTF_MAX_TYPE 0x7fffffff
+#define BTF_MAX_TYPE 0x0000ffff
/* Max offset into the string section */
-#define BTF_MAX_NAME_OFFSET 0x7fffffff
+#define BTF_MAX_NAME_OFFSET 0x0000ffff
/* Max # of struct/union/enum members or func args */
#define BTF_MAX_VLEN 0xffff
-/* The type id is referring to a parent BTF */
-#define BTF_TYPE_PARENT(id) (((id) >> 31) & 0x1)
-#define BTF_TYPE_ID(id) ((id) & BTF_MAX_TYPE)
-
-/* String is in the ELF string section */
-#define BTF_STR_TBL_ELF_ID(ref) (((ref) >> 31) & 0x1)
-#define BTF_STR_OFFSET(ref) ((ref) & BTF_MAX_NAME_OFFSET)
-
struct btf_type {
__u32 name_off;
/* "info" bits arrangement
* bits 0-15: vlen (e.g. # of struct's members)
* bits 16-23: unused
- * bits 24-28: kind (e.g. int, ptr, array...etc)
- * bits 29-30: unused
- * bits 31: root
+ * bits 24-27: kind (e.g. int, ptr, array...etc)
+ * bits 28-31: unused
*/
__u32 info;
/* "size" is used by INT, ENUM, STRUCT and UNION.
@@ -62,8 +49,7 @@ struct btf_type {
};
};
-#define BTF_INFO_KIND(info) (((info) >> 24) & 0x1f)
-#define BTF_INFO_ISROOT(info) (!!(((info) >> 24) & 0x80))
+#define BTF_INFO_KIND(info) (((info) >> 24) & 0x0f)
#define BTF_INFO_VLEN(info) ((info) & 0xffff)
#define BTF_KIND_UNKN 0 /* Unknown */
--
2.9.5
^ permalink raw reply related
* Re: [RFC PATCH net-next 10/12] vhost_net: build xdp buff
From: Jesse Brandeburg @ 2018-05-21 16:56 UTC (permalink / raw)
To: Jason Wang
Cc: mst, kvm, virtualization, netdev, linux-kernel, jesse.brandeburg
In-Reply-To: <1526893473-20128-11-git-send-email-jasowang@redhat.com>
On Mon, 21 May 2018 17:04:31 +0800 Jason wrote:
> This patch implement build XDP buffers in vhost_net. The idea is do
> userspace copy in vhost_net and build XDP buff based on the
> page. Vhost_net can then submit one or an array of XDP buffs to
> underlayer socket (e.g TUN). TUN can choose to do XDP or call
> build_skb() to build skb. To support build skb, vnet header were also
> stored into the header of the XDP buff.
>
> This userspace copy and XDP buffs building is key to achieve XDP
> batching in TUN, since TUN does not need to care about userspace copy
> and then can disable premmption for several XDP buffs to achieve
> batching from XDP.
>
> TODO: reserve headroom based on the TUN XDP.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index f0639d7..1209e84 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,6 +492,80 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
> likely(!vhost_exceeds_maxpend(net));
> }
>
> +#define VHOST_NET_HEADROOM 256
> +#define VHOST_NET_RX_PAD (NET_IP_ALIGN + NET_SKB_PAD)
> +
> +static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
> + struct iov_iter *from,
> + struct xdp_buff *xdp)
> +{
> + struct vhost_virtqueue *vq = &nvq->vq;
> + struct page_frag *alloc_frag = ¤t->task_frag;
> + struct virtio_net_hdr *gso;
> + size_t len = iov_iter_count(from);
> + int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + int pad = SKB_DATA_ALIGN(VHOST_NET_RX_PAD + VHOST_NET_HEADROOM
> + + nvq->sock_hlen);
> + int sock_hlen = nvq->sock_hlen;
> + void *buf;
> + int copied;
> +
> + if (len < nvq->sock_hlen)
> + return -EFAULT;
> +
> + if (SKB_DATA_ALIGN(len + pad) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) > PAGE_SIZE)
> + return -ENOSPC;
> +
> + buflen += SKB_DATA_ALIGN(len + pad);
maybe store the result of SKB_DATA_ALIGN in a local instead of doing
the work twice?
> + alloc_frag->offset = ALIGN((u64)alloc_frag->offset, SMP_CACHE_BYTES);
> + if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, GFP_KERNEL)))
> + return -ENOMEM;
> +
> + buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> +
> + /* We store two kinds of metadata in the header which will be
> + * used for XDP_PASS to do build_skb():
> + * offset 0: buflen
> + * offset sizeof(int): vnet header
> + */
> + copied = copy_page_from_iter(alloc_frag->page,
> + alloc_frag->offset + sizeof(int), sock_hlen, from);
> + if (copied != sock_hlen)
> + return -EFAULT;
> +
> + gso = (struct virtio_net_hdr *)(buf + sizeof(int));
> +
> + if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> + vhost16_to_cpu(vq, gso->csum_start) +
> + vhost16_to_cpu(vq, gso->csum_offset) + 2 >
> + vhost16_to_cpu(vq, gso->hdr_len)) {
> + gso->hdr_len = cpu_to_vhost16(vq,
> + vhost16_to_cpu(vq, gso->csum_start) +
> + vhost16_to_cpu(vq, gso->csum_offset) + 2);
> +
> + if (vhost16_to_cpu(vq, gso->hdr_len) > len)
> + return -EINVAL;
> + }
> +
> + len -= sock_hlen;
> + copied = copy_page_from_iter(alloc_frag->page,
> + alloc_frag->offset + pad,
> + len, from);
> + if (copied != len)
> + return -EFAULT;
> +
> + xdp->data_hard_start = buf;
> + xdp->data = buf + pad;
> + xdp->data_end = xdp->data + len;
> + *(int *)(xdp->data_hard_start)= buflen;
space before =
> +
> + get_page(alloc_frag->page);
> + alloc_frag->offset += buflen;
> +
> + return 0;
> +}
> +
> static void handle_tx_copy(struct vhost_net *net)
> {
> struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
^ permalink raw reply
* Re: [RFC PATCH net-next 04/12] vhost_net: split out datacopy logic
From: Jesse Brandeburg @ 2018-05-21 16:46 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, mst, netdev, virtualization, linux-kernel
In-Reply-To: <1526893473-20128-5-git-send-email-jasowang@redhat.com>
On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
> Instead of mixing zerocopy and datacopy logics, this patch tries to
> split datacopy logic out. This results for a more compact code and
> specific optimization could be done on top more easily.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 102 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 4ebac76..4682fcc 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
> likely(!vhost_exceeds_maxpend(net));
> }
>
> +static void handle_tx_copy(struct vhost_net *net)
> +{
> + struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> + struct vhost_virtqueue *vq = &nvq->vq;
> + unsigned out, in;
move "out" and "in" down to inside the for loop as well.
> + int head;
move this "head" declaration inside for-loop below.
> + struct msghdr msg = {
> + .msg_name = NULL,
> + .msg_namelen = 0,
> + .msg_control = NULL,
> + .msg_controllen = 0,
> + .msg_flags = MSG_DONTWAIT,
> + };
> + size_t len, total_len = 0;
> + int err;
> + size_t hdr_size;
> + struct socket *sock;
> + struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> + int sent_pkts = 0;
why do we need so many locals?
> +
> + mutex_lock(&vq->mutex);
> + sock = vq->private_data;
> + if (!sock)
> + goto out;
> +
> + if (!vq_iotlb_prefetch(vq))
> + goto out;
> +
> + vhost_disable_notify(&net->dev, vq);
> + vhost_net_disable_vq(net, vq);
> +
> + hdr_size = nvq->vhost_hlen;
> +
> + for (;;) {
> + head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
> + ARRAY_SIZE(vq->iov),
> + &out, &in);
> + /* On error, stop handling until the next kick. */
> + if (unlikely(head < 0))
> + break;
> + /* Nothing new? Wait for eventfd to tell us they refilled. */
> + if (head == vq->num) {
> + if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> + vhost_disable_notify(&net->dev, vq);
> + continue;
> + }
> + break;
> + }
> + if (in) {
> + vq_err(vq, "Unexpected descriptor format for TX: "
> + "out %d, int %d\n", out, in);
don't break strings, keep all the bits between " " together, even if it
is longer than 80 chars.
> + break;
> + }
> +
> + len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
> + if (len < 0)
> + break;
same comment as previous patch, len is a size_t which is unsigned.
> +
> + total_len += len;
> + if (total_len < VHOST_NET_WEIGHT &&
> + vhost_has_more_pkts(net, vq)) {
> + msg.msg_flags |= MSG_MORE;
> + } else {
> + msg.msg_flags &= ~MSG_MORE;
> + }
don't need { } here.
> +
> + /* TODO: Check specific error and bomb out unless ENOBUFS? */
> + err = sock->ops->sendmsg(sock, &msg, len);
> + if (unlikely(err < 0)) {
> + vhost_discard_vq_desc(vq, 1);
> + vhost_net_enable_vq(net, vq);
> + break;
> + }
> + if (err != len)
> + pr_debug("Truncated TX packet: "
> + " len %d != %zd\n", err, len);
single line " " string please
^ permalink raw reply
* Re: [PATCH v2 net] stmmac: strip vlan tag on reception only for 8021q tagged frames
From: Florian Fainelli @ 2018-05-21 16:42 UTC (permalink / raw)
To: David Miller, eladv6, Jose.Abreu
Cc: makita.toshiaki, netdev, peppe.cavallaro, alexandre.torgue
In-Reply-To: <20180521.114829.1456236827847875133.davem@davemloft.net>
On 05/21/2018 08:48 AM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 17 May 2018 12:43:56 -0400 (EDT)
>
>> Giuseppe and Alexandre, please review this patch.
>
> If nobody thinks this patch is important enough to actually
> review, I'm tossing it.
>
> Sorry.
>
How about looping in Jose?
--
Florian
^ permalink raw reply
* Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts()
From: Jesse Brandeburg @ 2018-05-21 16:39 UTC (permalink / raw)
To: Jason Wang
Cc: mst, netdev, linux-kernel, kvm, virtualization, jesse.brandeburg
In-Reply-To: <1526893473-20128-4-git-send-email-jasowang@redhat.com>
On Mon, 21 May 2018 17:04:24 +0800 Jason wrote:
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index de544ee..4ebac76 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -485,6 +485,13 @@ static bool vhost_exceeds_weight(int pkts, int total_len)
> unlikely(pkts >= VHOST_NET_PKT_WEIGHT);
> }
>
> +static bool vhost_has_more_pkts(struct vhost_net *net,
> + struct vhost_virtqueue *vq)
> +{
> + return !vhost_vq_avail_empty(&net->dev, vq) &&
> + likely(!vhost_exceeds_maxpend(net));
This really seems like mis-use of likely/unlikely, in the middle of a
sequence of operations that will always be run when this function is
called. I think you should remove the likely from this helper,
especially, and control the branch from the branch point.
> +}
> +
> /* Expects to be always run from workqueue - which acts as
> * read-size critical section for our kind of RCU. */
> static void handle_tx(struct vhost_net *net)
> @@ -578,8 +585,7 @@ static void handle_tx(struct vhost_net *net)
> }
> total_len += len;
> if (total_len < VHOST_NET_WEIGHT &&
> - !vhost_vq_avail_empty(&net->dev, vq) &&
> - likely(!vhost_exceeds_maxpend(net))) {
> + vhost_has_more_pkts(net, vq)) {
Yes, I know it came from here, but likely/unlikely are for branch
control, so they should encapsulate everything inside the if, unless
I'm mistaken.
> msg.msg_flags |= MSG_MORE;
> } else {
> msg.msg_flags &= ~MSG_MORE;
> @@ -605,7 +611,7 @@ static void handle_tx(struct vhost_net *net)
> else
> vhost_zerocopy_signal_used(net, vq);
> vhost_net_tx_packet(net);
> - if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
> + if (vhost_exceeds_weight(++sent_pkts, total_len)) {
You should have kept the unlikely here, and not had it inside the
helper (as per the previous patch. Also, why wasn't this change part
of the previous patch?
> vhost_poll_queue(&vq->poll);
> break;
> }
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox