* Re: [bpf-next v3 02/12] selftests/bpf: Avoid a clobbering of errno
From: Andrii Nakryiko @ 2019-07-10 23:51 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: open list, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
Networking, bpf, xdp-newbies
In-Reply-To: <20190708163121.18477-3-krzesimir@kinvolk.io>
On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> Save errno right after bpf_prog_test_run returns, so we later check
> the error code actually set by bpf_prog_test_run, not by some libcap
> function.
>
> Changes since v1:
> - Fix the "Fixes:" tag to mention actual commit that introduced the
> bug
>
> Changes since v2:
> - Move the declaration so it fits the reverse christmas tree style.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Fixes: 832c6f2c29ec ("bpf: test make sure to run unpriv test cases in test_verifier")
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index b8d065623ead..3fe126e0083b 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -823,16 +823,18 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> __u8 tmp[TEST_DATA_LEN << 2];
> __u32 size_tmp = sizeof(tmp);
> uint32_t retval;
> + int saved_errno;
> int err;
>
> if (unpriv)
> set_admin(true);
> err = bpf_prog_test_run(fd_prog, 1, data, size_data,
> tmp, &size_tmp, &retval, NULL);
Given err is either 0 or -1, how about instead making err useful right
here without extra variable?
if (bpf_prog_test_run(...))
err = errno;
> + saved_errno = errno;
> if (unpriv)
> set_admin(false);
> if (err) {
> - switch (errno) {
> + switch (saved_errno) {
> case 524/*ENOTSUPP*/:
ENOTSUPP is defined in include/linux/errno.h, is there any problem
with using this in selftests?
> printf("Did not run the program (not supported) ");
> return 0;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [bpf-next v3 01/12] selftests/bpf: Print a message when tester could not run a program
From: Andrii Nakryiko @ 2019-07-10 23:44 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: open list, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
Networking, bpf, xdp-newbies
In-Reply-To: <20190708163121.18477-2-krzesimir@kinvolk.io>
On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@kinvolk.io> wrote:
>
> This prints a message when the error is about program type being not
> supported by the test runner or because of permissions problem. This
> is to see if the program we expected to run was actually executed.
>
> The messages are open-coded because strerror(ENOTSUPP) returns
> "Unknown error 524".
>
> Changes since v2:
> - Also print "FAIL" on an unexpected bpf_prog_test_run error, so there
> is a corresponding "FAIL" message for each failed test.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..b8d065623ead 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -831,11 +831,20 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> tmp, &size_tmp, &retval, NULL);
> if (unpriv)
> set_admin(false);
> - if (err && errno != 524/*ENOTSUPP*/ && errno != EPERM) {
> - printf("Unexpected bpf_prog_test_run error ");
> - return err;
> + if (err) {
> + switch (errno) {
> + case 524/*ENOTSUPP*/:
> + printf("Did not run the program (not supported) ");
> + return 0;
> + case EPERM:
> + printf("Did not run the program (no permission) ");
Let's add "SKIP: " prefix to these?
> + return 0;
> + default:
> + printf("FAIL: Unexpected bpf_prog_test_run error (%s) ", strerror(saved_errno));
> + return err;
> + }
> }
> - if (!err && retval != expected_val &&
> + if (retval != expected_val &&
> expected_val != POINTER_VALUE) {
> printf("FAIL retval %d != %d ", retval, expected_val);
> return 1;
> --
> 2.20.1
>
^ permalink raw reply
* bpf_prog_free_deferred bug. WAS: Re: WARNING in mark_chain_precision
From: Andrii Nakryiko @ 2019-07-10 23:29 UTC (permalink / raw)
To: Hillf Danton
Cc: syzbot, aaron.f.brown, Alexei Starovoitov, bpf, Daniel Borkmann,
David S. Miller, hawk, intel-wired-lan, Jakub Kicinski,
jeffrey.t.kirsher, john fastabend, Martin Lau, open list,
Networking, sasha.neftin, Song Liu, syzkaller-bugs, xdp-newbies,
Yonghong Song
In-Reply-To: <000000000000a94981058d37f1a4@google.com>
On Tue, Jul 9, 2019 at 9:08 PM Hillf Danton <hdanton@sina.com> wrote:
>
>
> Mon, 08 Jul 2019 21:25:00 -0700 (PDT)
> > Hello,
> >
> > syzbot has tested the proposed patch but the reproducer still triggered
> > crash:
> > WARNING in bpf_jit_free
> >
> > WARNING: CPU: 0 PID: 9077 at kernel/bpf/core.c:851 bpf_jit_free+0x157/0x1b0
> > Kernel panic - not syncing: panic_on_warn set ...
> > CPU: 0 PID: 9077 Comm: kworker/0:3 Not tainted 5.2.0-rc6+ #1
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > Google 01/01/2011
> > Workqueue: events bpf_prog_free_deferred
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> > panic+0x2cb/0x744 kernel/panic.c:219
> > __warn.cold+0x20/0x4d kernel/panic.c:576
> > report_bug+0x263/0x2b0 lib/bug.c:186
> > fixup_bug arch/x86/kernel/traps.c:179 [inline]
> > fixup_bug arch/x86/kernel/traps.c:174 [inline]
> > do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
> > do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
> > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986
> > RIP: 0010:bpf_jit_free+0x157/0x1b0
> > Code: 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c 02 00 75 5d 48 b8 00 02 00 00
> > 00 00 ad de 48 39 43 70 0f 84 05 ff ff ff e8 09 7f f4 ff <0f> 0b e9 f9 fe
> > ff ff e8 2d 02 2e 00 e9 d9 fe ff ff 48 89 7d e0 e8
> > RSP: 0018:ffff888084affcb0 EFLAGS: 00010293
> > RAX: ffff88808a622100 RBX: ffff88809639d580 RCX: ffffffff817b0b0d
> > RDX: 0000000000000000 RSI: ffffffff817c4557 RDI: ffff88809639d5f0
> > RBP: ffff888084affcd0 R08: 1ffffffff150daa8 R09: fffffbfff150daa9
> > R10: fffffbfff150daa8 R11: ffffffff8a86d547 R12: ffffc90001921000
> > R13: ffff88809639d5e8 R14: ffff8880a0589800 R15: ffff8880ae834d40
> > bpf_prog_free_deferred+0x27a/0x350 kernel/bpf/core.c:1982
> > process_one_work+0x989/0x1790 kernel/workqueue.c:2269
> > worker_thread+0x98/0xe40 kernel/workqueue.c:2415
> > kthread+0x354/0x420 kernel/kthread.c:255
> > ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> > Kernel Offset: disabled
> > Rebooting in 86400 seconds..
> >
> >
> > Tested on:
> >
> > commit: b9321614 bpf: fix precision bit propagation for BPF_ST ins..
> > git tree: https://github.com/anakryiko/linux bpf-fix-precise-bpf_st
> > console output: https://syzkaller.appspot.com/x/log.txt?x=112f0dfda00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=6bb3e6e7997c14f9
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> >
>
> 1, currently, bpf_prog_put(), the put helper, deletes all kallsyms before
> invoking the free helper, bpf_prog_free(); the latter complains if kallsyms
> are detected not all off.
>
> 2, before commit 538950a1b752 ("soreuseport: setsockopt
> SO_ATTACH_REUSEPORT_[CE]BPF"), __bpf_prog_release() already called the put
> helper or the free one for a given prog depending on its type: put for
> BPF_PROG_TYPE_SOCKET_FILTER.
>
> In the commit we can see bpf_prog_destroy(), __bpf_prog_free(),
> bpf_prog_put(), here and then; Note in __get_bpf() the put for
> !BPF_PROG_TYPE_SOCKET_FILTER.
>
> 3, in commit 113214be7f6c ("bpf: refactor bpf_prog_get and type check into
> helper") bpf_prog_get_type() was added and put in __get_bpf().
>
> In the commit we can see other types like BPF_PROG_TYPE_SCHED_ACT and
> BPF_PROG_TYPE_SCHED_CLS.
>
> 4, in commit 8217ca653ec6 ("bpf: Enable BPF_PROG_TYPE_SK_REUSEPORT bpf prog
> in reuseport selection") sk_reuseport_prog_free() was added without a word
> in the log for it.
>
> 5, enrich prog type in __bpf_prog_release().
Thanks for investigation!
I'm curious what makes BPF_PROG_TYPE_SOCKET_FILTER (and
BPF_PROG_TYPE_SK_REUSEPORT) special, compared to all the other types
of BPF programs. If it's something to do with sockets specifically,
there are a bunch of other programs that deal with sockets, so should
they be handled specially as well (e.g., BPF_PROG_TYPE_CGROUP_SOCK)?
Daniel, do you have any insight here?
>
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1142,11 +1142,15 @@ static void bpf_release_orig_filter(stru
>
> static void __bpf_prog_release(struct bpf_prog *prog)
> {
> - if (prog->type == BPF_PROG_TYPE_SOCKET_FILTER) {
> + switch (prog->type) {
> + case BPF_PROG_TYPE_SOCKET_FILTER:
> + case BPF_PROG_TYPE_SK_REUSEPORT:
> bpf_prog_put(prog);
> - } else {
> + break;
> + default:
> bpf_release_orig_filter(prog);
> bpf_prog_free(prog);
> + break;
> }
> }
>
> --
>
^ permalink raw reply
* Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"
From: Andi Kleen @ 2019-07-10 23:14 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Paolo Pisati, Yonghong Song, Daniel Borkmann, Alexei Starovoitov,
Martin Lau, Song Liu, bpf@vger.kernel.org, Networking
In-Reply-To: <CAEf4Bzbtpqk-9ELnHFsHo278b5T4Z-2CgNnNbOqbD5Ocbuc-fg@mail.gmail.com>
> > Reading csum_partial/csum_fold, seems like after calculation of
> > checksum (so-called unfolded checksum), it is supposed to be passed
> > into csum_fold() to convert it into 16-bit one and invert.
Yes, you always need to fold at the end.
The low level code does fold sometimes, but not always.
-Andi
^ permalink raw reply
* Re: [PATCH net-next] net/mlx5e: Provide cb_list pointer when setting up tc block on rep
From: Pablo Neira Ayuso @ 2019-07-10 23:15 UTC (permalink / raw)
To: Vlad Buslov; +Cc: netdev, jhs, xiyou.wangcong, jiri, davem, saeedm
In-Reply-To: <20190710182554.2988-1-vladbu@mellanox.com>
On Wed, Jul 10, 2019 at 09:25:54PM +0300, Vlad Buslov wrote:
> Recent refactoring of tc block offloads infrastructure introduced new
> flow_block_cb_setup_simple() method intended to be used as unified way for
> all drivers to register offload callbacks. However, commit that actually
> extended all users (drivers) with block cb list and provided it to
> flow_block infra missed mlx5 en_rep. This leads to following NULL-pointer
> dereference when creating Qdisc:
>
> [ 278.385175] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [ 278.393233] #PF: supervisor read access in kernel mode
> [ 278.399446] #PF: error_code(0x0000) - not-present page
> [ 278.405847] PGD 8000000850e73067 P4D 8000000850e73067 PUD 8620cd067 PMD 0
> [ 278.414141] Oops: 0000 [#1] SMP PTI
> [ 278.419019] CPU: 7 PID: 3369 Comm: tc Not tainted 5.2.0-rc6+ #492
> [ 278.426580] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
> [ 278.435853] RIP: 0010:flow_block_cb_setup_simple+0xc4/0x190
> [ 278.442953] Code: 10 48 89 42 08 48 89 10 48 b8 00 01 00 00 00 00 ad de 49 89 00 48 05 00 01 00 00 49 89 40 08 31 c0 c3 b8 a1 ff ff ff c3 f3 c3 <48> 8b 06 48 39 c6 75 0a eb 1a 48 8b 00 48 39 c6 74 12
> 48 3b 50 28
> [ 278.464829] RSP: 0018:ffffaf07c3f97990 EFLAGS: 00010246
> [ 278.471648] RAX: 0000000000000000 RBX: ffff9b43ed4c7680 RCX: ffff9b43d5f80840
> [ 278.480408] RDX: ffffffffc0491650 RSI: 0000000000000000 RDI: ffffaf07c3f97998
> [ 278.489110] RBP: ffff9b43ddff9000 R08: ffff9b43d5f80840 R09: 0000000000000001
> [ 278.497838] R10: 0000000000000009 R11: 00000000000003ad R12: ffffaf07c3f97c08
> [ 278.506595] R13: ffff9b43d5f80000 R14: ffff9b43ed4c7680 R15: ffff9b43dfa20b40
> [ 278.515374] FS: 00007f796be1b400(0000) GS:ffff9b43ef840000(0000) knlGS:0000000000000000
> [ 278.525099] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 278.532453] CR2: 0000000000000000 CR3: 0000000840398002 CR4: 00000000001606e0
> [ 278.541197] Call Trace:
> [ 278.545252] tcf_block_offload_cmd.isra.52+0x7e/0xb0
> [ 278.551871] tcf_block_get_ext+0x365/0x3e0
> [ 278.557569] qdisc_create+0x15c/0x4e0
> [ 278.562859] ? kmem_cache_alloc_trace+0x1a2/0x1c0
> [ 278.569235] tc_modify_qdisc+0x1c8/0x780
> [ 278.574761] rtnetlink_rcv_msg+0x291/0x340
> [ 278.580518] ? _cond_resched+0x15/0x40
> [ 278.585856] ? rtnl_calcit.isra.29+0x120/0x120
> [ 278.591868] netlink_rcv_skb+0x4a/0x110
> [ 278.597198] netlink_unicast+0x1a0/0x250
> [ 278.602601] netlink_sendmsg+0x2c1/0x3c0
> [ 278.608022] sock_sendmsg+0x5b/0x60
> [ 278.612969] ___sys_sendmsg+0x289/0x310
> [ 278.618231] ? do_wp_page+0x99/0x730
> [ 278.623216] ? page_add_new_anon_rmap+0xbe/0x140
> [ 278.629298] ? __handle_mm_fault+0xc84/0x1360
> [ 278.635113] ? __sys_sendmsg+0x5e/0xa0
> [ 278.640285] __sys_sendmsg+0x5e/0xa0
> [ 278.645239] do_syscall_64+0x5b/0x1b0
> [ 278.650274] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 278.656697] RIP: 0033:0x7f796abdeb87
> [ 278.661628] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53
> 48 89 f3 48
> [ 278.683248] RSP: 002b:00007ffde213ba48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> [ 278.692245] RAX: ffffffffffffffda RBX: 000000005d261e6f RCX: 00007f796abdeb87
> [ 278.700862] RDX: 0000000000000000 RSI: 00007ffde213bab0 RDI: 0000000000000003
> [ 278.709527] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000006
> [ 278.718167] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
> [ 278.726743] R13: 000000000067b580 R14: 0000000000000000 R15: 0000000000000000
> [ 278.735302] Modules linked in: dummy vxlan ip6_udp_tunnel udp_tunnel sch_ingress nfsv3 nfs_acl nfs lockd grace fscache bridge stp llc sunrpc mlx5_ib ib_uverbs intel_rapl ib_core sb_edac x86_pkg_temp_
> thermal intel_powerclamp coretemp kvm_intel kvm mlx5_core irqbypass crct10dif_pclmul crc32_pclmul crc32c_intel igb ghash_clmulni_intel ses mei_me enclosure mlxfw ipmi_ssif intel_cstate iTCO_wdt ptp mei
> pps_core iTCO_vendor_support pcspkr joydev intel_uncore i2c_i801 ipmi_si lpc_ich intel_rapl_perf ioatdma wmi dca pcc_cpufreq ipmi_devintf ipmi_msghandler acpi_power_meter acpi_pad ast i2c_algo_bit drm_k
> ms_helper ttm drm mpt3sas raid_class scsi_transport_sas
> [ 278.802263] CR2: 0000000000000000
> [ 278.807170] ---[ end trace b1f0a442a279e66f ]---
>
> Extend en_rep with new static mlx5e_rep_block_cb_list list and pass it to
> flow_block_cb_setup_simple() function instead of hardcoded NULL pointer.
>
> Fixes: 955bcb6ea0df ("drivers: net: use flow block API")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
There's a similar patch from wenxu BTW.
^ permalink raw reply
* Re: [PATCH net-next] net/sched: Fix kernel NULL pointer dereference
From: Pablo Neira Ayuso @ 2019-07-10 23:13 UTC (permalink / raw)
To: wenxu; +Cc: davem, netfilter-devel, netdev
In-Reply-To: <20190710220337.tbflwdku4332ewo5@salvia>
On Thu, Jul 11, 2019 at 12:03:37AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 10, 2019 at 09:45:04PM +0800, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> >
> > [ 697.665184] BUG: kernel NULL pointer dereference, address: 0000000000000030
> > [ 697.665550] #PF: supervisor read access in kernel mode
> > [ 697.665906] #PF: error_code(0x0000) - not-present page
> > [ 697.666297] PGD 800000104e636067 P4D 800000104e636067 PUD ff4b02067 PMD 0
> > [ 697.666710] Oops: 0000 [#1] SMP PTI
> > [ 697.667115] CPU: 31 PID: 24466 Comm: modprobe Kdump: loaded Tainted: G O 5.2.0-rc6+ #1
> > [ 697.667867] Hardware name: Huawei Technologies Co., Ltd. RH1288 V3/BC11HGSC0, BIOS 3.57 02/26/2017
> > [ 697.668620] RIP: 0010:tc_indr_block_ing_cmd.isra.52+0x4c/0xb0
> > [ 697.669029] Code: 83 ec 40 65 48 8b 04 25 28 00 00 00 48 89 45 e8 31 c0 f3 48 ab 48 8b 06 49 8b b3 e8 04 00 00 44 89 45 b0 c7 45 b4 01 00 00 00 <8b> 48 30 48 89 75 c0 85 c9 48 8d 4d b0 0f 95 45 b8 48 85 c0 4c 8d
> > [ 697.670132] RSP: 0018:ffffc90007bf7958 EFLAGS: 00010246
> > [ 697.670537] RAX: 0000000000000000 RBX: ffff88905e2cbae8 RCX: 0000000000000000
> > [ 697.670938] RDX: ffff88905e2cbcd8 RSI: ffffffff823a8480 RDI: ffffc90007bf7990
> > [ 697.671352] RBP: ffffc90007bf79a8 R08: 0000000000000000 R09: ffff88905e2cbcc0
> > [ 697.671761] R10: ffff888107c07780 R11: ffff88902c249000 R12: ffff88905e2cbcd0
> > [ 697.672173] R13: ffff88905e2cbac0 R14: ffff88885596bc00 R15: ffff88905e2cbcc0
> > [ 697.672582] FS: 00007fe0b4095740(0000) GS:ffff88905fbc0000(0000) knlGS:0000000000000000
> > [ 697.673335] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 697.673746] CR2: 0000000000000030 CR3: 0000000ff46b4005 CR4: 00000000001606e0
> > [ 697.674156] Call Trace:
> > [ 697.674563] __tc_indr_block_cb_register+0x11e/0x3c0
> > [ 697.674998] mlx5e_nic_rep_netdevice_event+0x9e/0x110 [mlx5_core]
> > [ 697.675411] notifier_call_chain+0x53/0xa0
> > [ 697.675812] raw_notifier_call_chain+0x16/0x20
> > [ 697.676223] call_netdevice_notifiers_info+0x2d/0x60
> > [ 697.676633] register_netdevice+0x3fa/0x500
> >
> > get indr_dev->block after check it.
> >
> > Fixes: 955bcb6ea0df ("drivers: net: use flow block API")
> > Signed-off-by: wenxu <wenxu@ucloud.cn>
>
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
Please, toss this patch.
Vlad's patch provides a more complete fix.
^ permalink raw reply
* Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"
From: Andrii Nakryiko @ 2019-07-10 22:54 UTC (permalink / raw)
To: Paolo Pisati
Cc: Yonghong Song, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Song Liu, bpf@vger.kernel.org, Networking, ak
In-Reply-To: <CAEf4BzayQ+bEKFHcs8cUDcVnwPpQ2_2gzPaxX-j38r=AWDzVvg@mail.gmail.com>
On Wed, Jul 10, 2019 at 3:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> cc Andi Kleen re: refactoring, do you have any insight here?
OK, let's try again...
>
> On Wed, Jul 10, 2019 at 3:12 AM Paolo Pisati <p.pisati@gmail.com> wrote:
> >
> > On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
> > >
> > > Below is the test case.
> > > {
> > > "valid read map access into a read-only array 2",
> > > .insns = {
> > > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > > BPF_LD_MAP_FD(BPF_REG_1, 0),
> > > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > > BPF_FUNC_map_lookup_elem),
> > > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
> > >
> > > BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> > > BPF_MOV64_IMM(BPF_REG_2, 4),
> > > BPF_MOV64_IMM(BPF_REG_3, 0),
> > > BPF_MOV64_IMM(BPF_REG_4, 0),
> > > BPF_MOV64_IMM(BPF_REG_5, 0),
> > > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > > BPF_FUNC_csum_diff),
> > > BPF_EXIT_INSN(),
> > > },
> > > .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > > .fixup_map_array_ro = { 3 },
> > > .result = ACCEPT,
> > > .retval = -29,
> > > },
> > >
> > > The issue may be with helper bpf_csum_diff().
> > > Maybe you can check bpf_csum_diff() helper return value
> > > to confirm and take a further look at bpf_csum_diff implementations
> > > between x64 and amd64.
> >
> > Indeed, the different result comes from csum_partial() or, more precisely,
> > do_csum().
>
> I assume this is checksum used for ipv4 header checksum ([0]), right?
> It's defined as "16-bit one's complement of the one's complement sum
> of all 16-bit words", so at the end it has to be folded into 16-bit
> anyways.
>
> Reading csum_partial/csum_fold, seems like after calculation of
> checksum (so-called unfolded checksum), it is supposed to be passed
> into csum_fold() to convert it into 16-bit one and invert.
>
> So maybe that's what is missing from bpf_csum_diff()? Though I've
> never used it and don't even know exactly what is its purpose, so
> might be (and probably am) totally wrong here (e.g., it might be that
> BPF app is supposed to do that or something).
>
> [0] https://en.wikipedia.org/wiki/IPv4_header_checksum
>
> >
> > x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
> > while the generic version is in lib/checksum.c.
> >
> > I replaced the x86-64 csum_partial() / do_csum() code, with the one in
> > lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
> > not an arch dependent issue).
> >
> > I added some debugging to bpf_csum_diff(), and here are the results with different
> > checksum implementation code:
> >
> > http://paste.debian.net/1091037/
> >
> > lib/checksum.c:
> > ...
> > [ 206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
> > [ 206.085274] ____bpf_csum_diff from[0]: 28
> > [ 206.085276] ____bpf_csum_diff diff[0]: 4294967267
> > [ 206.085277] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [ 206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3
> >
> > arch/x86/lib/csum-partial_64.c
> > ...
> > [ 40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
> > [ 40.468141] ____bpf_csum_diff from[0]: 28
> > [ 40.468143] ____bpf_csum_diff diff[0]: 4294967267
> > [ 40.468144] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [ 40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3
> >
> > One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
> > calculated checksum to 16bit before returning it (unless the input value is
> > odd - *):
> >
> > arch/x86/lib/csum-partial_64.c::do_csum()
> > ...
> > if (unlikely(odd)) {
> > result = from32to16(result);
> > result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> > }
> > return result;
> > }
> >
> > contrary to all the other do_csum() implementations (that i could understand):
> >
> > lib/checksum.c::do_csum()
> > arch/alpha/lib/checksum.c::do_csum()
> > arch/parisc/lib/checksum.c::do_csum()
> >
> > Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
> > before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
> > arch/c6x/lib/csum_64plus.S).
> >
> > Funnily enough, if i change do_csum() for x86-64, folding the
> > checksum to 16 bit (following all the other implementations):
> >
> > --- a/arch/x86/lib/csum-partial_64.c
> > +++ b/arch/x86/lib/csum-partial_64.c
> > @@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
> > len)
> > if (len & 1)
> > result += *buff;
> > result = add32_with_carry(result>>32, result & 0xffffffff);
> > + result = from32to16(result);
> > if (unlikely(odd)) {
> > - result = from32to16(result);
> > result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> > }
> > return result;
> >
> > then, the x86-64 result match the others: 65507 or 0xffe3.
> >
> > As a last attempt, i tried running the bpf test_verifier on an armhf platform,
> > and i got a completely different number:
> >
> > [ 57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
> > [ 57.668016] ____bpf_csum_diff from[0]: 28
> > [ 57.668028] ____bpf_csum_diff diff[0]: 4294967267
> > [ 57.668039] ____bpf_csum_diff diff_size: 4 seed: 0
> >
> > After csum_partial() call:
> >
> > [ 57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2
> >
> > Not sure what to make of these number, but i have a question: whats is the
> > correct checksum of the memory chunk passed to csum_partial()? Is it really -29?
> >
> > Because, at least 2 other implementations i tested (the arm assembly code and
> > the c implementation in lib/checksum.c) computes a different value, so either
> > there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
> > returned value from csum_partial() somehow wrongly.
> >
> > *: originally, the x86-64 did the 16bit folding, but the logic was changed to
> > what we have today during a big rewrite - search for:
> >
> > commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
> > Author: Andi Kleen <ak@suse.de>
> > Date: Fri Jun 13 04:27:34 2003 -0700
> >
> > [PATCH] x86-64 merge
> >
> > in this historic repo:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> > --
> > bye,
> > p.
^ permalink raw reply
* Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"
From: Andrii Nakryiko @ 2019-07-10 22:52 UTC (permalink / raw)
To: Paolo Pisati
Cc: Yonghong Song, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Song Liu, bpf@vger.kernel.org, Networking, ak
In-Reply-To: <20190710100248.GA32281@harukaze>
cc Andi Kleen re: refactoring, do you have any insight here?
On Wed, Jul 10, 2019 at 3:12 AM Paolo Pisati <p.pisati@gmail.com> wrote:
>
> On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
> >
> > Below is the test case.
> > {
> > "valid read map access into a read-only array 2",
> > .insns = {
> > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > BPF_LD_MAP_FD(BPF_REG_1, 0),
> > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > BPF_FUNC_map_lookup_elem),
> > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
> >
> > BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> > BPF_MOV64_IMM(BPF_REG_2, 4),
> > BPF_MOV64_IMM(BPF_REG_3, 0),
> > BPF_MOV64_IMM(BPF_REG_4, 0),
> > BPF_MOV64_IMM(BPF_REG_5, 0),
> > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> > BPF_FUNC_csum_diff),
> > BPF_EXIT_INSN(),
> > },
> > .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > .fixup_map_array_ro = { 3 },
> > .result = ACCEPT,
> > .retval = -29,
> > },
> >
> > The issue may be with helper bpf_csum_diff().
> > Maybe you can check bpf_csum_diff() helper return value
> > to confirm and take a further look at bpf_csum_diff implementations
> > between x64 and amd64.
>
> Indeed, the different result comes from csum_partial() or, more precisely,
> do_csum().
I assume this is checksum used for ipv4 header checksum ([0]), right?
It's defined as "16-bit one's complement of the one's complement sum
of all 16-bit words", so at the end it has to be folded into 16-bit
anyways.
Reading csum_partial/csum_fold, seems like after calculation of
checksum (so-called unfolded checksum), it is supposed to be passed
into csum_fold() to convert it into 16-bit one and invert.
So maybe that's what is missing from bpf_csum_diff()? Though I've
never used it and don't even know exactly what is its purpose, so
might be (and probably am) totally wrong here (e.g., it might be that
BPF app is supposed to do that or something).
[0] https://en.wikipedia.org/wiki/IPv4_header_checksum
>
> x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
> while the generic version is in lib/checksum.c.
>
> I replaced the x86-64 csum_partial() / do_csum() code, with the one in
> lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
> not an arch dependent issue).
>
> I added some debugging to bpf_csum_diff(), and here are the results with different
> checksum implementation code:
>
> http://paste.debian.net/1091037/
>
> lib/checksum.c:
> ...
> [ 206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
> [ 206.085274] ____bpf_csum_diff from[0]: 28
> [ 206.085276] ____bpf_csum_diff diff[0]: 4294967267
> [ 206.085277] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [ 206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3
>
> arch/x86/lib/csum-partial_64.c
> ...
> [ 40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
> [ 40.468141] ____bpf_csum_diff from[0]: 28
> [ 40.468143] ____bpf_csum_diff diff[0]: 4294967267
> [ 40.468144] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [ 40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3
>
> One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
> calculated checksum to 16bit before returning it (unless the input value is
> odd - *):
>
> arch/x86/lib/csum-partial_64.c::do_csum()
> ...
> if (unlikely(odd)) {
> result = from32to16(result);
> result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> }
> return result;
> }
>
> contrary to all the other do_csum() implementations (that i could understand):
>
> lib/checksum.c::do_csum()
> arch/alpha/lib/checksum.c::do_csum()
> arch/parisc/lib/checksum.c::do_csum()
>
> Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
> before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
> arch/c6x/lib/csum_64plus.S).
>
> Funnily enough, if i change do_csum() for x86-64, folding the
> checksum to 16 bit (following all the other implementations):
>
> --- a/arch/x86/lib/csum-partial_64.c
> +++ b/arch/x86/lib/csum-partial_64.c
> @@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
> len)
> if (len & 1)
> result += *buff;
> result = add32_with_carry(result>>32, result & 0xffffffff);
> + result = from32to16(result);
> if (unlikely(odd)) {
> - result = from32to16(result);
> result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
> }
> return result;
>
> then, the x86-64 result match the others: 65507 or 0xffe3.
>
> As a last attempt, i tried running the bpf test_verifier on an armhf platform,
> and i got a completely different number:
>
> [ 57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
> [ 57.668016] ____bpf_csum_diff from[0]: 28
> [ 57.668028] ____bpf_csum_diff diff[0]: 4294967267
> [ 57.668039] ____bpf_csum_diff diff_size: 4 seed: 0
>
> After csum_partial() call:
>
> [ 57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2
>
> Not sure what to make of these number, but i have a question: whats is the
> correct checksum of the memory chunk passed to csum_partial()? Is it really -29?
>
> Because, at least 2 other implementations i tested (the arm assembly code and
> the c implementation in lib/checksum.c) computes a different value, so either
> there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
> returned value from csum_partial() somehow wrongly.
>
> *: originally, the x86-64 did the 16bit folding, but the logic was changed to
> what we have today during a big rewrite - search for:
>
> commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
> Author: Andi Kleen <ak@suse.de>
> Date: Fri Jun 13 04:27:34 2003 -0700
>
> [PATCH] x86-64 merge
>
> in this historic repo:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> --
> bye,
> p.
^ permalink raw reply
* Re: [PATCH net] net: fix use-after-free in __netif_receive_skb_core
From: Sabrina Dubroca @ 2019-07-10 22:47 UTC (permalink / raw)
To: Edward Cree; +Cc: netdev, Andreas Steinmetz
In-Reply-To: <62ad16f6-c33a-407e-2f55-9be382b7ec52@solarflare.com>
2019-07-10, 16:07:43 +0100, Edward Cree wrote:
> On 10/07/2019 14:52, Sabrina Dubroca wrote:
> > -static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
> > +static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
> > struct packet_type **ppt_prev)
> > {
> > struct packet_type *ptype, *pt_prev;
> > rx_handler_func_t *rx_handler;
> > + struct sk_buff *skb = *pskb;
> Would it not be simpler just to change all users of skb to *pskb?
> Then you avoid having to keep doing "*pskb = skb;" whenever skb changes
> (with concomitant risk of bugs if one gets missed).
Yes, that would be less risky. I wrote a version of the patch that did
exactly that, but found it really too ugly (both the patch and the
resulting code). We have more than 50 occurences of skb, including
things like:
atomic_long_inc(&skb->dev->rx_dropped);
--
Sabrina
^ permalink raw reply
* Re: Fw: [Bug 204099] New: systemd-networkd fails on 5.2 - same version works on 5.1.16
From: David Ahern @ 2019-07-10 22:43 UTC (permalink / raw)
To: Stephen Hemminger, netdev
In-Reply-To: <20190709074344.76049d02@hermes.lan>
On 7/9/19 8:43 AM, Stephen Hemminger wrote:
> Looks like the stricter netlink validation broke userspace.
> This is bad.
I believe other reports have traced this to
commit 7dc2bccab0ee37ac28096b8fcdc390a679a15841
Author: Maxim Mikityanskiy <maximmi@mellanox.com>
Date: Tue May 21 06:40:04 2019 +0000
Validate required parameters in inet6_validate_link_af
^ permalink raw reply
* Re: [PATCH V2 0/1] tools/dtrace: initial implementation of DTrace
From: David Miller @ 2019-07-10 22:32 UTC (permalink / raw)
To: kris.van.hees
Cc: netdev, bpf, dtrace-devel, linux-kernel, rostedt, mhiramat, acme,
ast, daniel, peterz, clm
In-Reply-To: <201907101537.x6AFboMR015946@aserv0122.oracle.com>
From: Kris Van Hees <kris.van.hees@oracle.com>
Date: Wed, 10 Jul 2019 08:37:50 -0700 (PDT)
> This is version 2 of the patch, incorporating feedback from Peter
> Zijlstra and Arnaldo Carvalho de Melo.
No way, NACK.
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: David Miller @ 2019-07-10 22:31 UTC (permalink / raw)
To: brendan.d.gregg
Cc: kris.van.hees, daniel, corbet, netdev, bpf, dtrace-devel,
linux-kernel, rostedt, mhiramat, acme, ast, peterz, clm
In-Reply-To: <CAE40pdeSVN+QhhUeQ4sEbsyzJ+NWkQA5XU5X0FrKAbRMHPzBsw@mail.gmail.com>
From: Brendan Gregg <brendan.d.gregg@gmail.com>
Date: Wed, 10 Jul 2019 14:49:52 -0700
> Hey Kris -- so you're referring to me, and I've used DTrace more than
> anyone over the past 15 years, and I don't think anyone has used all
> the different Linux tracers more than I have. I think my opinion has a
> lot of value.
+1
I seriously am against starting to merge all of these userland tracing
tools into the tree.
They belong as separate independant projects, outside of the kernel
tree.
^ permalink raw reply
* Re: NEIGH: BUG, double timer add, state is 8
From: David Ahern @ 2019-07-10 22:18 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Marek Majkowski, David Miller, netdev, kernel-team
In-Reply-To: <20190705173011.GA2882@localhost.localdomain>
On 7/5/19 11:30 AM, Lorenzo Bianconi wrote:
> looking at the reproducer it seems to me the issue is due to the use of
> 'NTF_USE' from userspace.
> Should we unschedule the neigh timer if we are in IN_TIMER receiving this
> flag from userspace? (taking appropriate locking)
I think you are right. Do you want to send a patch?
^ permalink raw reply
* Re: [PATCH nf-next] net/mlx5e: Fix kernel NULL pointer dereference
From: Pablo Neira Ayuso @ 2019-07-10 22:04 UTC (permalink / raw)
To: wenxu; +Cc: davem, netdev
In-Reply-To: <1562768310-20468-1-git-send-email-wenxu@ucloud.cn>
On Wed, Jul 10, 2019 at 10:18:30PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> [ 3444.666552] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [ 3444.666631] #PF: supervisor read access in kernel mode
> [ 3444.666701] #PF: error_code(0x0000) - not-present page
> [ 3444.666769] PGD 8000000812dd7067 P4D 8000000812dd7067 PUD 8207cc067 PMD 0
> [ 3444.666843] Oops: 0000 [#1] SMP PTI
> [ 3444.666910] CPU: 17 PID: 27387 Comm: nft Kdump: loaded Tainted: G O 5.2.0-rc6+ #1
> [ 3444.666987] Hardware name: Huawei Technologies Co., Ltd. RH1288 V3/BC11HGSC0, BIOS 3.57 02/26/2017
> [ 3444.667071] RIP: 0010:flow_block_cb_setup_simple+0x127/0x240
> [ 3444.667141] Code: 02 48 89 43 08 31 c0 48 83 c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 83 c4 10 b8 a1 ff ff ff 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <49> 8b 04 24 49 39 c4 75 0a eb 2f 48 8b 00 49 39 c4 74 27 4c 3b 68
> [ 3444.668201] RSP: 0018:ffffc90007b7b888 EFLAGS: 00010246
> [ 3444.668595] RAX: 0000000000000000 RBX: ffff8890439a9b40 RCX: ffff88904d5008c0
> [ 3444.668992] RDX: ffffffffa0879850 RSI: 0000000000000000 RDI: ffffc90007b7b908
> [ 3444.669389] RBP: ffffc90007b7b8c0 R08: ffff88904d5008c0 R09: 0000000000000001
> [ 3444.669787] R10: ffff88885a797d00 R11: ffff8890439a9b00 R12: 0000000000000000
> [ 3444.670186] R13: ffffffffa0879850 R14: ffffc90007b7b908 R15: ffffffff823a8480
> [ 3444.670588] FS: 00007f357c2fa740(0000) GS:ffff88885fe40000(0000) knlGS:0000000000000000
> [ 3444.671313] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 3444.671705] CR2: 0000000000000000 CR3: 00000001a1600002 CR4: 00000000001626e0
> [ 3444.672103] Call Trace:
> [ 3444.672505] ? jump_label_update+0x5f/0xc0
> [ 3444.672933] mlx5e_rep_setup_tc+0x32/0x40 [mlx5_core]
> [ 3444.673335] nft_flow_offload_chain+0xd0/0x1d0 [nf_tables]
> [ 3444.673729] nft_flow_rule_offload_commit+0x91/0x11b [nf_tables]
> [ 3444.674129] nf_tables_commit+0x90/0xe30 [nf_tables]
> [ 3444.674529] nfnetlink_rcv_batch+0x3b9/0x750 [nfnetlink]
>
> Init the driver_block_list parameter
>
> Fixes: 955bcb6ea0df ("drivers: net: use flow block API")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
^ permalink raw reply
* Re: [PATCH net-next] net/sched: Fix kernel NULL pointer dereference
From: Pablo Neira Ayuso @ 2019-07-10 22:03 UTC (permalink / raw)
To: wenxu; +Cc: davem, netfilter-devel, netdev
In-Reply-To: <1562766304-20272-1-git-send-email-wenxu@ucloud.cn>
On Wed, Jul 10, 2019 at 09:45:04PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> [ 697.665184] BUG: kernel NULL pointer dereference, address: 0000000000000030
> [ 697.665550] #PF: supervisor read access in kernel mode
> [ 697.665906] #PF: error_code(0x0000) - not-present page
> [ 697.666297] PGD 800000104e636067 P4D 800000104e636067 PUD ff4b02067 PMD 0
> [ 697.666710] Oops: 0000 [#1] SMP PTI
> [ 697.667115] CPU: 31 PID: 24466 Comm: modprobe Kdump: loaded Tainted: G O 5.2.0-rc6+ #1
> [ 697.667867] Hardware name: Huawei Technologies Co., Ltd. RH1288 V3/BC11HGSC0, BIOS 3.57 02/26/2017
> [ 697.668620] RIP: 0010:tc_indr_block_ing_cmd.isra.52+0x4c/0xb0
> [ 697.669029] Code: 83 ec 40 65 48 8b 04 25 28 00 00 00 48 89 45 e8 31 c0 f3 48 ab 48 8b 06 49 8b b3 e8 04 00 00 44 89 45 b0 c7 45 b4 01 00 00 00 <8b> 48 30 48 89 75 c0 85 c9 48 8d 4d b0 0f 95 45 b8 48 85 c0 4c 8d
> [ 697.670132] RSP: 0018:ffffc90007bf7958 EFLAGS: 00010246
> [ 697.670537] RAX: 0000000000000000 RBX: ffff88905e2cbae8 RCX: 0000000000000000
> [ 697.670938] RDX: ffff88905e2cbcd8 RSI: ffffffff823a8480 RDI: ffffc90007bf7990
> [ 697.671352] RBP: ffffc90007bf79a8 R08: 0000000000000000 R09: ffff88905e2cbcc0
> [ 697.671761] R10: ffff888107c07780 R11: ffff88902c249000 R12: ffff88905e2cbcd0
> [ 697.672173] R13: ffff88905e2cbac0 R14: ffff88885596bc00 R15: ffff88905e2cbcc0
> [ 697.672582] FS: 00007fe0b4095740(0000) GS:ffff88905fbc0000(0000) knlGS:0000000000000000
> [ 697.673335] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 697.673746] CR2: 0000000000000030 CR3: 0000000ff46b4005 CR4: 00000000001606e0
> [ 697.674156] Call Trace:
> [ 697.674563] __tc_indr_block_cb_register+0x11e/0x3c0
> [ 697.674998] mlx5e_nic_rep_netdevice_event+0x9e/0x110 [mlx5_core]
> [ 697.675411] notifier_call_chain+0x53/0xa0
> [ 697.675812] raw_notifier_call_chain+0x16/0x20
> [ 697.676223] call_netdevice_notifiers_info+0x2d/0x60
> [ 697.676633] register_netdevice+0x3fa/0x500
>
> get indr_dev->block after check it.
>
> Fixes: 955bcb6ea0df ("drivers: net: use flow block API")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: Brendan Gregg @ 2019-07-10 21:49 UTC (permalink / raw)
To: Kris Van Hees
Cc: Daniel Borkmann, Jonathan Corbet, netdev, bpf, dtrace-devel, LKML,
Steven Rostedt, Masami Hiramatsu, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Peter Zijlstra, Chris Mason, David S. Miller
In-Reply-To: <20190710213637.GB13962@oracle.com>
On Wed, Jul 10, 2019 at 2:36 PM Kris Van Hees <kris.van.hees@oracle.com> wrote:
>
> On Wed, Jul 10, 2019 at 11:19:43PM +0200, Daniel Borkmann wrote:
> > On 07/10/2019 10:30 PM, Jonathan Corbet wrote:
> > > On Wed, 10 Jul 2019 21:32:25 +0200
> > > Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > >> Looks like you missed Brendan Gregg's prior feedback from v1 [0]. I haven't
> > >> seen a strong compelling argument for why this needs to reside in the kernel
> > >> tree given we also have all the other tracing tools and many of which also
> > >> rely on BPF such as bcc, bpftrace, ply, systemtap, sysdig, lttng to just name
> > >> a few.
> > >
> > > So I'm just watching from the sidelines here, but I do feel the need to
> > > point out that Kris appears to be trying to follow the previous feedback
> > > he got from Alexei, where creating tools/dtrace is exactly what he was
> > > told to do:
> > >
> > > https://lwn.net/ml/netdev/20190521175617.ipry6ue7o24a2e6n@ast-mbp.dhcp.thefacebook.com/
> > >
> > > Now he's being told the exact opposite. Not the best experience for
> > > somebody who is trying to make the kernel better.
> >
> > Ugh, agree, sorry for the misleading direction. Alexei is currently offgrid
> > this week, he might comment later.
> >
> > It has nothing to do with making the _kernel_ better, it's a /user space/ front
> > end for the existing kernel infrastructure like many of the other tracers out
> > there. Don't get me wrong, adding the missing /kernel parts/ for it is a totally
> > different subject [and _that_ is what is making the kernel better, not the former].
>
> I disagree. Yes, the current patch obviously isn't making the kernel better
> because it doesn't touch the kernel. But DTrace as a whole is not just a
> /front end/ to the existing kernel infrastructure, and I did make that point
> at LPC 2018 and in my emails. Some of its more advanced features will lead
> to contributions to the kernel that (by virtue of being developed as part of
> this DTrace re-implementation) will more often than not be able to benefit
> other tracers as well. I do think that aspect qualifies as working towards
> making the kenrel better.
>
> > Hypothetical question: does it make the _kernel_ better if we suddenly add a huge
> > and complex project like tools/mysql/ to the kernel tree? Nope.
> >
> > > There are still people interested in DTrace out there. How would you
> > > recommend that Kris proceed at this point?
> >
> > My recommendation to proceed is to maintain the dtrace user space tooling in
> > its own separate project like the vast majority of all the other tracing projects
> > (see also the other advantages that Steven pointed out from his experience), and
> > extend the kernel bits whenever needed.
>
> I wish that would have been the initial recommendation because it certainly
> would have avoided me going down a path that was going to lead to rejection.
>
> Either way, I do hope that as work progresses and contributions to the kernel
> code are submitted in support of advancing tracing on Linux, those patches
> will receive a fair review and consideration. I can appreciate that some
> people do not like DTrace or feel that it is not necessary, but personal
> opinions about tools should not be a deciding factor in whether a contribution
> has merit or not.
Hey Kris -- so you're referring to me, and I've used DTrace more than
anyone over the past 15 years, and I don't think anyone has used all
the different Linux tracers more than I have. I think my opinion has a
lot of value.
Brendan
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: Kris Van Hees @ 2019-07-10 21:36 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jonathan Corbet, Kris Van Hees, netdev, bpf, dtrace-devel,
linux-kernel, rostedt, mhiramat, acme, ast, Peter Zijlstra,
Chris Mason, brendan.d.gregg, davem
In-Reply-To: <1de27d29-65bb-89d3-9fca-7c452cd66934@iogearbox.net>
On Wed, Jul 10, 2019 at 11:19:43PM +0200, Daniel Borkmann wrote:
> On 07/10/2019 10:30 PM, Jonathan Corbet wrote:
> > On Wed, 10 Jul 2019 21:32:25 +0200
> > Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> >> Looks like you missed Brendan Gregg's prior feedback from v1 [0]. I haven't
> >> seen a strong compelling argument for why this needs to reside in the kernel
> >> tree given we also have all the other tracing tools and many of which also
> >> rely on BPF such as bcc, bpftrace, ply, systemtap, sysdig, lttng to just name
> >> a few.
> >
> > So I'm just watching from the sidelines here, but I do feel the need to
> > point out that Kris appears to be trying to follow the previous feedback
> > he got from Alexei, where creating tools/dtrace is exactly what he was
> > told to do:
> >
> > https://lwn.net/ml/netdev/20190521175617.ipry6ue7o24a2e6n@ast-mbp.dhcp.thefacebook.com/
> >
> > Now he's being told the exact opposite. Not the best experience for
> > somebody who is trying to make the kernel better.
>
> Ugh, agree, sorry for the misleading direction. Alexei is currently offgrid
> this week, he might comment later.
>
> It has nothing to do with making the _kernel_ better, it's a /user space/ front
> end for the existing kernel infrastructure like many of the other tracers out
> there. Don't get me wrong, adding the missing /kernel parts/ for it is a totally
> different subject [and _that_ is what is making the kernel better, not the former].
I disagree. Yes, the current patch obviously isn't making the kernel better
because it doesn't touch the kernel. But DTrace as a whole is not just a
/front end/ to the existing kernel infrastructure, and I did make that point
at LPC 2018 and in my emails. Some of its more advanced features will lead
to contributions to the kernel that (by virtue of being developed as part of
this DTrace re-implementation) will more often than not be able to benefit
other tracers as well. I do think that aspect qualifies as working towards
making the kenrel better.
> Hypothetical question: does it make the _kernel_ better if we suddenly add a huge
> and complex project like tools/mysql/ to the kernel tree? Nope.
>
> > There are still people interested in DTrace out there. How would you
> > recommend that Kris proceed at this point?
>
> My recommendation to proceed is to maintain the dtrace user space tooling in
> its own separate project like the vast majority of all the other tracing projects
> (see also the other advantages that Steven pointed out from his experience), and
> extend the kernel bits whenever needed.
I wish that would have been the initial recommendation because it certainly
would have avoided me going down a path that was going to lead to rejection.
Either way, I do hope that as work progresses and contributions to the kernel
code are submitted in support of advancing tracing on Linux, those patches
will receive a fair review and consideration. I can appreciate that some
people do not like DTrace or feel that it is not necessary, but personal
opinions about tools should not be a deciding factor in whether a contribution
has merit or not.
Kris
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: Brendan Gregg @ 2019-07-10 21:35 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Daniel Borkmann, Kris Van Hees, netdev, bpf, dtrace-devel, LKML,
Steven Rostedt, Masami Hiramatsu, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Peter Zijlstra, Chris Mason, David S. Miller
In-Reply-To: <20190710143048.3923d1d9@lwn.net>
On Wed, Jul 10, 2019 at 1:30 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> On Wed, 10 Jul 2019 21:32:25 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> > Looks like you missed Brendan Gregg's prior feedback from v1 [0]. I haven't
> > seen a strong compelling argument for why this needs to reside in the kernel
> > tree given we also have all the other tracing tools and many of which also
> > rely on BPF such as bcc, bpftrace, ply, systemtap, sysdig, lttng to just name
> > a few.
>
> So I'm just watching from the sidelines here, but I do feel the need to
> point out that Kris appears to be trying to follow the previous feedback
> he got from Alexei, where creating tools/dtrace is exactly what he was
> told to do:
>
> https://lwn.net/ml/netdev/20190521175617.ipry6ue7o24a2e6n@ast-mbp.dhcp.thefacebook.com/
From what I saw, the discussion was about kernel and user bits, where
the user bit was a "little tool is currently hardcoded to process a
single test case". I missed it first time around, but was going to
make the case that such tests belong in tools/testing/selftests/bpf
rather than tools/dtrace, since we have other similar test cases to
ensure bits of BPF work.
This patchset pivoted from a single test case to the entire DTrace
front end. If this was Kris's intent all along, it wasn't clear to me
(and maybe Alexei either) until now.
>
> Now he's being told the exact opposite. Not the best experience for
> somebody who is trying to make the kernel better.
>
> There are still people interested in DTrace out there.
Yes, they can:
apt-get install bpftrace (or snap, yum, whatever)
and start solving production problems today, like we are.
> How would you
> recommend that Kris proceed at this point?
You may not be asking me, but I don't think it's best for Linux to
split our tracing expertise among 13 different tracers (SystemTap,
LTTng, ftrace, perf, dtrace4linux, OEL DTrace, ktap, sysdig, Intel
PIN, bcc, shark, ply, and bpftrace). bpftrace is already far ahead and
in use in production, and Kris is just starting building a new one. I
actually think we need to consolidate our expertise on fewer tracers,
which includes asking developers to jump the fence and work on other
projects (like I did myself). But I also recognize's Kris's need to
support legacy users, so I'll stop short of saying that it shouldn't
exist at all.
Brendan
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: Daniel Borkmann @ 2019-07-10 21:19 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Kris Van Hees, netdev, bpf, dtrace-devel, linux-kernel, rostedt,
mhiramat, acme, ast, Peter Zijlstra, Chris Mason, brendan.d.gregg,
davem
In-Reply-To: <20190710143048.3923d1d9@lwn.net>
On 07/10/2019 10:30 PM, Jonathan Corbet wrote:
> On Wed, 10 Jul 2019 21:32:25 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> Looks like you missed Brendan Gregg's prior feedback from v1 [0]. I haven't
>> seen a strong compelling argument for why this needs to reside in the kernel
>> tree given we also have all the other tracing tools and many of which also
>> rely on BPF such as bcc, bpftrace, ply, systemtap, sysdig, lttng to just name
>> a few.
>
> So I'm just watching from the sidelines here, but I do feel the need to
> point out that Kris appears to be trying to follow the previous feedback
> he got from Alexei, where creating tools/dtrace is exactly what he was
> told to do:
>
> https://lwn.net/ml/netdev/20190521175617.ipry6ue7o24a2e6n@ast-mbp.dhcp.thefacebook.com/
>
> Now he's being told the exact opposite. Not the best experience for
> somebody who is trying to make the kernel better.
Ugh, agree, sorry for the misleading direction. Alexei is currently offgrid
this week, he might comment later.
It has nothing to do with making the _kernel_ better, it's a /user space/ front
end for the existing kernel infrastructure like many of the other tracers out
there. Don't get me wrong, adding the missing /kernel parts/ for it is a totally
different subject [and _that_ is what is making the kernel better, not the former].
Hypothetical question: does it make the _kernel_ better if we suddenly add a huge
and complex project like tools/mysql/ to the kernel tree? Nope.
> There are still people interested in DTrace out there. How would you
> recommend that Kris proceed at this point?
My recommendation to proceed is to maintain the dtrace user space tooling in
its own separate project like the vast majority of all the other tracing projects
(see also the other advantages that Steven pointed out from his experience), and
extend the kernel bits whenever needed.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH AUTOSEL 4.19 14/60] mwifiex: Abort at too short BSS descriptor element
From: Brian Norris @ 2019-07-10 21:12 UTC (permalink / raw)
To: Sasha Levin
Cc: Linux Kernel, stable, Takashi Iwai, Kalle Valo, linux-wireless,
<netdev@vger.kernel.org>
In-Reply-To: <20190710145112.GX10104@sasha-vm>
On Wed, Jul 10, 2019 at 7:51 AM Sasha Levin <sashal@kernel.org> wrote:
> I see that 63d7ef36103d didn't make it into 5.2, so I'll just drop this
> for now.
Yeah, I think it's stuck at net/master. Presumably it'll get into
5.3-rc somewhere.
Brian
^ permalink raw reply
* Re: [PATCH iproute2-next v2 0/3] add interface to TC MPLS actions
From: David Ahern @ 2019-07-10 21:08 UTC (permalink / raw)
To: John Hurley, netdev
Cc: davem, jiri, xiyou.wangcong, willemdebruijn.kernel, stephen,
simon.horman, jakub.kicinski, oss-drivers
In-Reply-To: <1562762440-25656-1-git-send-email-john.hurley@netronome.com>
On 7/10/19 6:40 AM, John Hurley wrote:
> Recent kernel additions to TC allows the manipulation of MPLS headers as
> filter actions.
>
> The following patchset creates an iproute2 interface to the new actions
> and includes documentation on how to use it.
>
> v1->v2:
> - change error from print_string() to fprintf(strerr,) (Stephen Hemminger)
> - split long line in explain() message (David Ahern)
> - use _SL_ instead of /n in print message (David Ahern)
>
> John Hurley (3):
> lib: add mpls_uc and mpls_mc as link layer protocol names
> tc: add mpls actions
> man: update man pages for TC MPLS actions
>
> lib/ll_proto.c | 2 +
> man/man8/tc-mpls.8 | 156 ++++++++++++++++++++++++++++++
> tc/Makefile | 1 +
> tc/m_mpls.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 435 insertions(+)
> create mode 100644 man/man8/tc-mpls.8
> create mode 100644 tc/m_mpls.c
>
applied to iproute2-next. Thanks
^ permalink raw reply
* Re: [PATCH net-next iproute2 v2 2/2] devlink: Introduce PCI PF and VF port flavour and attribute
From: David Ahern @ 2019-07-10 21:01 UTC (permalink / raw)
To: Parav Pandit, netdev; +Cc: stephen, dsahern, jiri
In-Reply-To: <20190710123952.6877-2-parav@mellanox.com>
On 7/10/19 6:39 AM, Parav Pandit wrote:
> Introduce PCI PF and VF port flavour and port attributes such as PF
> number and VF number.
>
> $ devlink port show
> pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
> pci/0000:05:00.0/1: type eth netdev eth1 flavour pcivf pfnum 0 vfnum 0
> pci/0000:05:00.0/2: type eth netdev eth2 flavour pcivf pfnum 0 vfnum 1
>
> Signed-off-by: Parav Pandit <parav@mellanox.com>
> ---
> Changelog:
> v1->v2:
> - Instead of if-else using switch-case.
> - Split patch to two patches to have kernel header update in dedicated
> patch.
> ---
> devlink/devlink.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
applied to iproute2-next. Thanks
^ permalink raw reply
* Re: [PATCH] tipc: ensure skb->lock is initialised
From: Chris Packham @ 2019-07-10 20:58 UTC (permalink / raw)
To: Jon Maloy, Eric Dumazet, ying.xue@windriver.com,
davem@davemloft.net
Cc: netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
linux-kernel@vger.kernel.org
In-Reply-To: <MN2PR15MB3581E1D6D56D6AA7DE8E357E9AF00@MN2PR15MB3581.namprd15.prod.outlook.com>
On 11/07/19 1:10 AM, Jon Maloy wrote:
>> -----Original Message-----
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Sent: 10-Jul-19 04:00
>> To: Jon Maloy <jon.maloy@ericsson.com>; Eric Dumazet
>> <eric.dumazet@gmail.com>; Chris Packham
>> <Chris.Packham@alliedtelesis.co.nz>; ying.xue@windriver.com;
>> davem@davemloft.net
>> Cc: netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH] tipc: ensure skb->lock is initialised
>>
>>
>>
>> On 7/9/19 10:15 PM, Jon Maloy wrote:
>>>
>>> It is not only for lockdep purposes, -it is essential. But please provide details
>> about where you see that more fixes are needed.
>>>
>>
>> Simple fact that you detect a problem only when skb_queue_purge() is called
>> should talk by itself.
>>
>> As I stated, there are many places where the list is manipulated _without_ its
>> spinlock being held.
>
> Yes, and that is the way it should be on the send path.
>
>>
>> You want consistency, then
>>
>> - grab the spinlock all the time.
>> - Or do not ever use it.
>
> That is exactly what we are doing.
> - The send path doesn't need the spinlock, and never grabs it.
> - The receive path does need it, and always grabs it.
>
> However, since we don't know from the beginning which path a created
> message will follow, we initialize the queue spinlock "just in case"
> when it is created, even though it may never be used later.
> You can see this as a violation of the principle you are stating
> above, but it is a prize that is worth paying, given savings in code
> volume, complexity and performance.
>
>>
>> Do not initialize the spinlock just in case a path will use skb_queue_purge()
>> (instead of using __skb_queue_purge())
>
> I am ok with that. I think we can agree that Chris goes for that
> solution, so we can get this bug fixed.
So would you like a v2 with an improved commit message? I note that I
said skb->lock instead of head->lock in the subject line so at least
that should be corrected.
^ permalink raw reply
* Re: [PATCH iproute2-next] ip: bond: add peer notification delay support
From: David Ahern @ 2019-07-10 20:54 UTC (permalink / raw)
To: Vincent Bernat, Stephen Hemminger, netdev
In-Reply-To: <20190707175115.3704-1-vincent@bernat.ch>
On 7/7/19 11:51 AM, Vincent Bernat wrote:
> Ability to tweak the delay between gratuitous ND/ARP packets has been
> added in kernel commit 07a4ddec3ce9 ("bonding: add an option to
> specify a delay between peer notifications"), through
> IFLA_BOND_PEER_NOTIF_DELAY attribute. Add support to set and show this
> value.
>
> Example:
>
> $ ip -d link set bond0 type bond peer_notify_delay 1000
> $ ip -d link l dev bond0
> 2: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue
> state UP mode DEFAULT group default qlen 1000
> link/ether 50:54:33:00:00:01 brd ff:ff:ff:ff:ff:ff
> bond mode active-backup active_slave eth0 miimon 100 updelay 0
> downdelay 0 peer_notify_delay 1000 use_carrier 1 arp_interval 0
> arp_validate none arp_all_targets any primary eth0
> primary_reselect always fail_over_mac active xmit_hash_policy
> layer2 resend_igmp 1 num_grat_arp 5 all_slaves_active 0 min_links
> 0 lp_interval 1 packets_per_slave 1 lacp_rate slow ad_select
> stable tlb_dynamic_lb 1 addrgenmode eu
>
> Signed-off-by: Vincent Bernat <vincent@bernat.ch>
> ---
> include/uapi/linux/if_link.h | 1 +
> ip/iplink_bond.c | 14 +++++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
applied to iproute2-next. Thanks
^ permalink raw reply
* Re: [PATCH V2 1/1 (was 0/1 by accident)] tools/dtrace: initial implementation of DTrace
From: Jonathan Corbet @ 2019-07-10 20:30 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Kris Van Hees, netdev, bpf, dtrace-devel, linux-kernel, rostedt,
mhiramat, acme, ast, Peter Zijlstra, Chris Mason, brendan.d.gregg,
davem
In-Reply-To: <c7f15d1d-1696-4d95-1729-4c4e97bdc43e@iogearbox.net>
On Wed, 10 Jul 2019 21:32:25 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> Looks like you missed Brendan Gregg's prior feedback from v1 [0]. I haven't
> seen a strong compelling argument for why this needs to reside in the kernel
> tree given we also have all the other tracing tools and many of which also
> rely on BPF such as bcc, bpftrace, ply, systemtap, sysdig, lttng to just name
> a few.
So I'm just watching from the sidelines here, but I do feel the need to
point out that Kris appears to be trying to follow the previous feedback
he got from Alexei, where creating tools/dtrace is exactly what he was
told to do:
https://lwn.net/ml/netdev/20190521175617.ipry6ue7o24a2e6n@ast-mbp.dhcp.thefacebook.com/
Now he's being told the exact opposite. Not the best experience for
somebody who is trying to make the kernel better.
There are still people interested in DTrace out there. How would you
recommend that Kris proceed at this point?
Thanks,
jon
^ 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