* Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support
From: Andy Gospodarek @ 2019-07-08 15:24 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Andy Gospodarek, Michael Chan, davem, netdev, hawk, ast,
ivan.khoronzhuk
In-Reply-To: <20190708145137.GA21894@apalos>
On Mon, Jul 08, 2019 at 05:51:37PM +0300, Ilias Apalodimas wrote:
> Hi Andy,
>
> > On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> > > Thanks Andy, Michael
> > >
> > > > + if (event & BNXT_REDIRECT_EVENT)
> > > > + xdp_do_flush_map();
> > > > +
> > > > if (event & BNXT_TX_EVENT) {
> > > > struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> > > > u16 prod = txr->tx_prod;
> > > > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> > > >
> > > > for (j = 0; j < max_idx;) {
> > > > struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > > > - struct sk_buff *skb = tx_buf->skb;
> > > > + struct sk_buff *skb;
> > > > int k, last;
> > > >
> > > > + if (i < bp->tx_nr_rings_xdp &&
> > > > + tx_buf->action == XDP_REDIRECT) {
> > > > + dma_unmap_single(&pdev->dev,
> > > > + dma_unmap_addr(tx_buf, mapping),
> > > > + dma_unmap_len(tx_buf, len),
> > > > + PCI_DMA_TODEVICE);
> > > > + xdp_return_frame(tx_buf->xdpf);
> > > > + tx_buf->action = 0;
> > > > + tx_buf->xdpf = NULL;
> > > > + j++;
> > > > + continue;
> > > > + }
> > > > +
> > >
> > > Can't see the whole file here and maybe i am missing something, but since you
> > > optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> > > not remapped) buffer that can be returned to the pool and resynced for
> > > device usage.
> > > Is that happening later on the tx clean function?
> >
> > Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
> > receive them and then in bnxt_tx_int_xdp() when the transmits have
> > completed (for XDP_TX and XDP_REDIRECT). I think we are doing what is
> > proper with respect to mapping vs sync for both cases, but I would be
> > fine to be corrected.
> >
>
> Yea seems to be doing the right thing,
> XDP_TX syncs correctly and reuses with bnxt_reuse_rx_data() right?
>
> This might be a bit confusing for someone reading the driver on the first time,
> probably because you'll end up with 2 ways of recycling buffers.
>
> Once a buffers get freed on the XDP path it's either fed back to the pool, so
> the next requested buffer get served from the pools cache (ndo_xdp_xmit case in
> the patch). If the buffer is used for XDP_TX is's synced correctly but recycled
> via bnxt_reuse_rx_data() right? Since you are moving to page pool please
> consider having a common approach towards the recycling path. I understand that
> means tracking buffers types and make sure you do the right thing on 'tx clean'.
> I've done something similar on the netsec driver and i do think this might be a
> good thing to add on page_pool API
>
> Again this isn't a blocker at least for me but you already have the buffer type
> (via tx_buf->action)
Thanks for the confirmation. I agree things are not totally clear as I
had to learn how all of it worked to do this. We can work on that.
>
> > >
> > > > + skb = tx_buf->skb;
> > > > if (!skb) {
> > > > j++;
> > > > continue;
> > > > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > > > if (rc < 0)
> > > > return rc;
> > > >
> > > > + rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > > > + MEM_TYPE_PAGE_SHARED, NULL);
> > > > + if (rc) {
> > > > + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> > >
> > > I think you can use page_pool_free directly here (and pge_pool_destroy once
> > > Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> > > common please?
> >
> > That's an easy change, I can do that.
> >
I'll reply to myself here and note that you are correct that we need to
fixup the error case, but it actually belongs in patch 4 in the series
since that is the patch that adds page_pool support. I'll reply to that
one in just a min once I've tested my patch.
> > >
> > > If Ivan's patch get merged please note you'll have to explicitly
> > > page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
> > > case (not the error habdling here). Sorry for the confusion this might bring!
> >
> > Funny enough the driver was basically doing that until page_pool_destroy
> > was removed (these patches are not new). I saw last week there was
> > discussion to add it back, but I did not want to wait to get this on the
> > list before that was resolved.
>
> Fair enough
>
> >
> > This path works as expected with the code in the tree today so it seemed
> > like the correct approach to post something that is working, right? :-)
>
> Yes.
>
> It will continue to work even if you dont change the call in the future.
> This is more a 'let's not spread the code' attempt, but removing and re-adding
> page_pool_destroy() was/is our mess. We might as well live with the
> consequences!
So as someone who ends up doing some of this work after the trail has
already been blazed upstream on other drivers, etc, I definitely
understand the desire to keep things more common. I think the page_pool
bits are a nice step in that direction, so I enjoy any attempts to help
make repeated tasks easier for everyone.
>
> >
> > >
> > > > + return rc;
> > > > + }
> > > > +
> > > > rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> > > > if (rc)
> > > > return rc;
> > > > @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> > > [...]
> > >
>
> Thanks!
> /Ilias
^ permalink raw reply
* Re: [PATCH bpf-next v3 0/3] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Daniel Borkmann @ 2019-07-08 15:25 UTC (permalink / raw)
To: Stanislav Fomichev, netdev, bpf; +Cc: davem, ast
In-Reply-To: <20190701173841.32249-1-sdf@google.com>
On 07/01/2019 07:38 PM, Stanislav Fomichev wrote:
> Clang can generate 8-byte stores for user_ip6 & msg_src_ip6,
> let's support that on the verifier side.
>
> v3:
> * fix comments spelling an -> and (Andrii Nakryiko)
>
> v2:
> * Add simple cover letter (Yonghong Song)
> * Update comments (Yonghong Song)
> * Remove [4] selftests (Yonghong Song)
>
> Stanislav Fomichev (3):
> bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
> bpf: sync bpf.h to tools/
> selftests/bpf: add verifier tests for wide stores
>
> include/linux/filter.h | 6 ++++
> include/uapi/linux/bpf.h | 6 ++--
> net/core/filter.c | 22 +++++++-----
> tools/include/uapi/linux/bpf.h | 6 ++--
> tools/testing/selftests/bpf/test_verifier.c | 17 +++++++--
> .../selftests/bpf/verifier/wide_store.c | 36 +++++++++++++++++++
> 6 files changed, 76 insertions(+), 17 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix test_attach_probe map definition
From: Daniel Borkmann @ 2019-07-08 15:25 UTC (permalink / raw)
To: Andrii Nakryiko, andrii.nakryiko, bpf, netdev, ast, kernel-team
In-Reply-To: <20190706044420.1582763-1-andriin@fb.com>
On 07/06/2019 06:44 AM, Andrii Nakryiko wrote:
> ef99b02b23ef ("libbpf: capture value in BTF type info for BTF-defined map
> defs") changed BTF-defined maps syntax, while independently merged
> 1e8611bbdfc9 ("selftests/bpf: add kprobe/uprobe selftests") added new
> test using outdated syntax of maps. This patch fixes this test after
> corresponding patch sets were merged.
>
> Fixes: ef99b02b23ef ("libbpf: capture value in BTF type info for BTF-defined map defs")
> Fixes: 1e8611bbdfc9 ("selftests/bpf: add kprobe/uprobe selftests")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: cgroup: Fix build error without CONFIG_NET
From: Daniel Borkmann @ 2019-07-08 15:26 UTC (permalink / raw)
To: YueHaibing, ast, kafai, songliubraving, yhs, sdf
Cc: linux-kernel, netdev, bpf
In-Reply-To: <20190703082630.51104-1-yuehaibing@huawei.com>
On 07/03/2019 10:26 AM, YueHaibing wrote:
> If CONFIG_NET is not set and CONFIG_CGROUP_BPF=y,
> gcc building fails:
>
> kernel/bpf/cgroup.o: In function `cg_sockopt_func_proto':
> cgroup.c:(.text+0x237e): undefined reference to `bpf_sk_storage_get_proto'
> cgroup.c:(.text+0x2394): undefined reference to `bpf_sk_storage_delete_proto'
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_getsockopt':
> (.text+0x2a1f): undefined reference to `lock_sock_nested'
> (.text+0x2ca2): undefined reference to `release_sock'
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_setsockopt':
> (.text+0x3006): undefined reference to `lock_sock_nested'
> (.text+0x32bb): undefined reference to `release_sock'
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Suggested-by: Stanislav Fomichev <sdf@fomichev.me>
> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next 1/2] bpf: skip sockopt hooks without CONFIG_NET
From: Daniel Borkmann @ 2019-07-08 15:26 UTC (permalink / raw)
To: Yonghong Song, Arnd Bergmann, Alexei Starovoitov
Cc: Andrii Nakryiko, Martin Lau, Stanislav Fomichev, Song Liu,
Mauricio Vasquez B, Roman Gushchin, Matt Mullins,
Willem de Bruijn, Andrey Ignatov, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <0e7cf1b5-579f-5fcd-0966-8760148b00de@fb.com>
On 07/08/2019 05:06 PM, Yonghong Song wrote:
> On 7/8/19 5:57 AM, Arnd Bergmann wrote:
>> When CONFIG_NET is disabled, we get a link error:
>>
>> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_setsockopt':
>> cgroup.c:(.text+0x3010): undefined reference to `lock_sock_nested'
>> cgroup.c:(.text+0x3258): undefined reference to `release_sock'
>> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_getsockopt':
>> cgroup.c:(.text+0x3568): undefined reference to `lock_sock_nested'
>> cgroup.c:(.text+0x3870): undefined reference to `release_sock'
>> kernel/bpf/cgroup.o: In function `cg_sockopt_func_proto':
>> cgroup.c:(.text+0x41d8): undefined reference to `bpf_sk_storage_delete_proto'
>>
>> None of this code is useful in this configuration anyway, so we can
>> simply hide it in an appropriate #ifdef.
>>
>> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> FYI.
>
> There is already a patch to fix the same issue,
> https://lore.kernel.org/bpf/e9e489fe-feec-a211-82aa-5df0c6a308d1@huawei.com/T/#t
>
> which has been acked and not merged yet.
Done now, and I've also applied patch 2/2 from here, thanks.
^ permalink raw reply
* Re: [PATCH bpf-next 0/3] xdp: Add devmap_hash map type
From: Toke Høiland-Jørgensen @ 2019-07-08 15:40 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <53906C87-8AF9-4048-8CA0-AE38C023AEF7@flugsvamp.com>
"Jonathan Lemon" <jlemon@flugsvamp.com> writes:
> On 5 Jul 2019, at 10:56, Toke Høiland-Jørgensen wrote:
>
>> This series adds a new map type, devmap_hash, that works like the
>> existing
>> devmap type, but using a hash-based indexing scheme. This is useful
>> for the use
>> case where a devmap is indexed by ifindex (for instance for use with
>> the routing
>> table lookup helper). For this use case, the regular devmap needs to
>> be sized
>> after the maximum ifindex number, not the number of devices in it. A
>> hash-based
>> indexing scheme makes it possible to size the map after the number of
>> devices it
>> should contain instead.
>
> This device hash map is sized at NETDEV_HASHENTRIES == 2^8 == 256. Is
> this actually smaller than an array? What ifindex values are you
> seeing?
Well, not in all cases, certainly. But machines with lots of virtual
interfaces (e.g., container hosts) can easily exceed that. Also, for a
devmap we charge the full size of max_entries * struct bpf_dtab_netdev
towards the locked memory cost on map creation. And since sizeof(struct
bpf_dtab_netdev) is 64, the size of the hashmap only corresponds to 32
entries...
But more importantly, it's a UI issue: Say you want to create a simple
program that uses the fib_lookup helper (something like the xdp_fwd
example under samples/bpf/). You know that you only want to route
between a couple of interfaces, so you naturally create a devmap that
can hold, say, 8 entries (just to be sure). This works fine on your
initial test, where the machine only has a couple of physical interfaces
brought up at boot. But then you try to run the same program on your
production server, where the interfaces you need to use just happen to
have ifindexes higher than 8, and now it breaks for no discernible
reason. Or even worse, if you remove and re-add an interface, you may no
longer be able to insert it into your map because the ifindex changed...
-Toke
^ permalink raw reply
* WARNING in __mark_chain_precision
From: syzbot @ 2019-07-08 15:27 UTC (permalink / raw)
To: ast, bcrl, bpf, daniel, davem, hawk, jakub.kicinski,
john.fastabend, kafai, linux-aio, linux-fsdevel, linux-kernel,
netdev, songliubraving, syzkaller-bugs, torvalds, viro,
xdp-newbies, yhs
Hello,
syzbot found the following crash on:
HEAD commit: e5a3e259 Merge branch 'bpf-tcp-rtt-hook'
git tree: bpf-next
console output: https://syzkaller.appspot.com/x/log.txt?x=14190c2da00000
kernel config: https://syzkaller.appspot.com/x/.config?x=dd16b8dc9d0d210c
dashboard link: https://syzkaller.appspot.com/bug?extid=4da3ff23081bafe74fc2
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1409ce0da00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17226a0da00000
The bug was bisected to:
commit b53119f13a04879c3bf502828d99d13726639ead
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu Mar 7 01:22:54 2019 +0000
pin iocb through aio.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11427b8ba00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=13427b8ba00000
console output: https://syzkaller.appspot.com/x/log.txt?x=15427b8ba00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4da3ff23081bafe74fc2@syzkaller.appspotmail.com
Fixes: b53119f13a04 ("pin iocb through aio.")
------------[ cut here ]------------
verifier backtracking bug
WARNING: CPU: 0 PID: 9104 at kernel/bpf/verifier.c:1785
__mark_chain_precision+0x19bb/0x1ee0 kernel/bpf/verifier.c:1785
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 9104 Comm: syz-executor284 Not tainted 5.2.0-rc5+ #34
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
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:__mark_chain_precision+0x19bb/0x1ee0 kernel/bpf/verifier.c:1785
Code: 08 31 ff 89 de e8 95 ba f2 ff 84 db 0f 85 ce fe ff ff e8 48 b9 f2 ff
48 c7 c7 e0 44 91 87 c6 05 1c 15 1f 08 01 e8 03 f1 c4 ff <0f> 0b 41 bc f2
ff ff ff e9 af fe ff ff e8 d3 3c 2c 00 e9 c2 e7 ff
RSP: 0018:ffff88809f04f380 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815ad926 RDI: ffffed1013e09e62
RBP: ffff88809f04f4d0 R08: ffff88809987a540 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff88809af0c280 R14: 0000000000000001 R15: ffff88809f65cb00
mark_chain_precision kernel/bpf/verifier.c:1822 [inline]
check_cond_jmp_op+0xcd8/0x3c30 kernel/bpf/verifier.c:5842
do_check+0x60f4/0x8a20 kernel/bpf/verifier.c:7782
bpf_check+0x6f99/0x9950 kernel/bpf/verifier.c:9293
bpf_prog_load+0xe68/0x1670 kernel/bpf/syscall.c:1698
__do_sys_bpf+0xa20/0x42d0 kernel/bpf/syscall.c:2849
__se_sys_bpf kernel/bpf/syscall.c:2808 [inline]
__x64_sys_bpf+0x73/0xb0 kernel/bpf/syscall.c:2808
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440369
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffca85a2fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440369
RDX: 0000000000000048 RSI: 0000000020000200 RDI: 0000000000000005
RBP: 00000000006ca018 R08: 0000000000000000 R09: 0000000000000000
R10: 00000000ffffffff R11: 0000000000000246 R12: 0000000000401bf0
R13: 0000000000401c80 R14: 0000000000000000 R15: 0000000000000000
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
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#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH net-next v5 1/4] net/sched: Introduce action ct
From: Florian Westphal @ 2019-07-08 15:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Paul Blakey, Jiri Pirko, Roi Dayan, Yossi Kuperman, Oz Shlomo,
netdev, David Miller, Aaron Conole, Zhike Wang, Rony Efraim,
nst-kernel, John Hurley, Simon Horman, Justin Pettit
In-Reply-To: <20190708134208.GD3390@localhost.localdomain>
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> > + } else { /* NFPROTO_IPV6 */
> > + enum ip6_defrag_users user = IP6_DEFRAG_CONNTRACK_IN + zone;
> > +
> > + memset(IP6CB(skb), 0, sizeof(struct inet6_skb_parm));
> > + err = nf_ct_frag6_gather(net, skb, user);
>
> This doesn't build without IPv6 enabled.
> ERROR: "nf_ct_frag6_gather" [net/sched/act_ct.ko] undefined!
>
> We need to (copy and pasted):
>
> @@ -179,7 +179,9 @@ static int tcf_ct_handle_fragments(struct net *net, struct sk_buff *skb,
> local_bh_enable();
> if (err && err != -EINPROGRESS)
> goto out_free;
> - } else { /* NFPROTO_IPV6 */
> + }
> +#if IS_ENABLED(IPV6)
> + else { /* NFPROTO_IPV6 */
> enum ip6_defrag_users user = IP6_DEFRAG_CONNTRACK_IN + zone;
Good catch, but it should be
#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
just like ovs conntrack.c ,
^ permalink raw reply
* Re: [PATCH bpf-next] tools: bpftool: add completion for bpftool prog "loadall"
From: Daniel Borkmann @ 2019-07-08 15:27 UTC (permalink / raw)
To: Quentin Monnet, Alexei Starovoitov; +Cc: bpf, netdev, oss-drivers
In-Reply-To: <20190708130546.7518-1-quentin.monnet@netronome.com>
On 07/08/2019 03:05 PM, Quentin Monnet wrote:
> Bash completion for proposing the "loadall" subcommand is missing. Let's
> add it to the completion script.
>
> Add a specific case to propose "load" and "loadall" for completing:
>
> $ bpftool prog load
> ^ cursor is here
>
> Otherwise, completion considers that $command is in load|loadall and
> starts making related completions (file or directory names, as the
> number of words on the command line is below 6), when the only suggested
> keywords should be "load" and "loadall" until one has been picked and a
> space entered after that to move to the next word.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied, thanks!
^ permalink raw reply
* [ANNOUNCE] iproute2 5.2
From: Stephen Hemminger @ 2019-07-08 15:26 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
The 5.2 kernel has been released, and therefore time for another
update to iproute2.
Not a lot of big new features in this release. Just the usual array of
small fixes across the board.
Download:
https://www.kernel.org/pub/linux/utils/net/iproute2/iproute2-5.2.0.tar.gz
Repository for upcoming release:
git://git.kernel.org/pub/scm/network/iproute2/iproute2.git
And future release (net-next):
git://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git
Thanks for all the contributions.
Report problems (or enhancements) to the netdev@vger.kernel.org mailing list.
---
Andrea Claudi (6):
Makefile: use make -C
ip address: do not set nodad option for IPv4 addresses
ip address: do not set home option for IPv4 addresses
ip address: do not set mngtmpaddr option for IPv4 addresses
man: tc-netem.8: fix URL for netem page
tc: netem: fix r parameter in Bernoulli loss model
Baruch Siach (2):
devlink: fix format string warning for 32bit targets
devlink: fix libc and kernel headers collision
David Ahern (5):
Update kernel headers
Update kernel headers
Update kernel headers
uapi: wrap SIOCGSTAMP and SIOCGSTAMPNS in ifndef
Update kernel headers
Davide Caratti (2):
man: tc-skbedit.8: document 'inheritdsfield'
tc: simple: don't hardcode the control action
Eyal Birger (1):
tc: adjust xtables_match and xtables_target to changes in recent iptables
Gal Pressman (1):
rdma: Update node type strings
Hangbin Liu (1):
ip/iptoken: fix dump error when ipv6 disabled
Hoang Le (3):
tipc: add link broadcast set method and ratio
tipc: add link broadcast get
tipc: add link broadcast man page
Ido Schimmel (2):
ipneigh: Print neighbour offload indication
devlink: Increase column size for larger shared buffers
Josh Hunt (1):
ss: add option to print socket information on one line
Kristian Evensen (1):
ip fou: Support binding FOU ports
Lucas Siba 2019-04-20 11:40 UTC (1):
Update tc-bpf.8 man page examples
Lukasz Czapnik (1):
tc: flower: fix port value truncation
Mahesh Bandewar (1):
ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds
Matteo Croce (4):
ip: reset netns after each command in batch mode
netns: switch netns in the child when executing commands
ip vrf: use hook to change VRF in the child
netns: make netns_{save,restore} static
Michael Forney (1):
ipmroute: Prevent overlapping storage of `filter` global
Mike Manning (1):
iplink_vlan: add support for VLAN bridge binding flag
Moshe Shemesh (1):
devlink: mnlg: Catch returned error value of dumpit commands
Nicolas Dichtel (3):
lib: suppress error msg when filling the cache
iplink: don't try to get ll addr len when creating an iface
ip monitor: display interfaces from all groups
Nikolay Aleksandrov (2):
iplink: bridge: add support for vlan_stats_per_port
bridge: mdb: restore text output format
Paolo Abeni (2):
tc: add support for plug qdisc
m_mirred: don't bail if the control action is missing
Parav Pandit (1):
devlink: Increase bus, device buffer size to 64 bytes
Pete Morici (1):
Add support for configuring MACsec gcm-aes-256 cipher type.
Roman Mashak (1):
tc: Fix binding of gact action by index.
Stefano Brivio (1):
iproute: Set flags and attributes on dump to get IPv6 cached routes to be flushed
Stephen Hemminger (14):
uapi: update to elf-em header
uapi: add include/linux/net.h
uapi: update headers to import asm-generic/sockios.h
mailmap: add myself
mailmap: map David's mail address
uapi: add sockios.h
uapi: merge bpf.h from 5.2
rdma: update uapi headers
man: fix macaddr section of ip-link
uapi: minor upstream btf.h header change
testsuite: intent if/else in Makefile
uapi: update headers and add if_link.h and if_infiniband.h
devlink: replace print macros with functions
vv5.2.0
Steve Wise (4):
Add .mailmap file
rdma: add helper rd_sendrecv_msg()
rdma: add 'link add/delete' commands
rdma: man page update for link add/delete
Tomasz Torcz (1):
ss: in --numeric mode, print raw numbers for data rates
Vinicius Costa Gomes (2):
taprio: Add support for changing schedules
taprio: Add support for cycle_time and cycle_time_extension
^ permalink raw reply
* Re: [PATCH v7 bpf-next 0/5] libbpf: add perf buffer abstraction and API
From: Daniel Borkmann @ 2019-07-08 15:23 UTC (permalink / raw)
To: Andrii Nakryiko, andrii.nakryiko, bpf, netdev, ast, kernel-team
In-Reply-To: <20190706180628.3919653-1-andriin@fb.com>
On 07/06/2019 08:06 PM, Andrii Nakryiko wrote:
> This patchset adds a high-level API for setting up and polling perf buffers
> associated with BPF_MAP_TYPE_PERF_EVENT_ARRAY map. Details of APIs are
> described in corresponding commit.
>
> Patch #1 adds a set of APIs to set up and work with perf buffer.
> Patch #2 enhances libbpf to support auto-setting PERF_EVENT_ARRAY map size.
> Patch #3 adds test.
> Patch #4 converts bpftool map event_pipe to new API.
> Patch #5 updates README to mention perf_buffer_ prefix.
>
> v6->v7:
> - __x64_ syscall prefix (Yonghong);
> v5->v6:
> - fix C99 for loop variable initialization usage (Yonghong);
> v4->v5:
> - initialize perf_buffer_raw_opts in bpftool map event_pipe (Jakub);
> - add perf_buffer_ to README;
> v3->v4:
> - fixed bpftool event_pipe cmd error handling (Jakub);
> v2->v3:
> - added perf_buffer__new_raw for more low-level control;
> - converted bpftool map event_pipe to new API (Daniel);
> - fixed bug with error handling in create_maps (Song);
> v1->v2:
> - add auto-sizing of PERF_EVENT_ARRAY maps;
>
> Andrii Nakryiko (5):
> libbpf: add perf buffer API
> libbpf: auto-set PERF_EVENT_ARRAY size to number of CPUs
> selftests/bpf: test perf buffer API
> tools/bpftool: switch map event_pipe to libbpf's perf_buffer
> libbpf: add perf_buffer_ prefix to README
>
> tools/bpf/bpftool/map_perf_ring.c | 201 +++------
> tools/lib/bpf/README.rst | 3 +-
> tools/lib/bpf/libbpf.c | 397 +++++++++++++++++-
> tools/lib/bpf/libbpf.h | 49 +++
> tools/lib/bpf/libbpf.map | 4 +
> .../selftests/bpf/prog_tests/perf_buffer.c | 100 +++++
> .../selftests/bpf/progs/test_perf_buffer.c | 25 ++
> 7 files changed, 634 insertions(+), 145 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/perf_buffer.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_perf_buffer.c
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] xdp: Refactor devmap allocation code for reuse
From: Toke Høiland-Jørgensen @ 2019-07-08 15:19 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <A2ABED10-8475-4878-93DF-F16D106FC33D@flugsvamp.com>
"Jonathan Lemon" <jlemon@flugsvamp.com> writes:
> On 5 Jul 2019, at 10:56, Toke Høiland-Jørgensen wrote:
>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> The subsequent patch to add a new devmap sub-type can re-use much of
>> the
>> initialisation and allocation code, so refactor it into separate
>> functions.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>> kernel/bpf/devmap.c | 137
>> +++++++++++++++++++++++++++++++--------------------
>> 1 file changed, 84 insertions(+), 53 deletions(-)
>>
>> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>> index d83cf8ccc872..a2fe16362129 100644
>> --- a/kernel/bpf/devmap.c
>> +++ b/kernel/bpf/devmap.c
>> @@ -60,7 +60,7 @@ struct xdp_bulk_queue {
>> struct bpf_dtab_netdev {
>> struct net_device *dev; /* must be first member, due to tracepoint
>> */
>> struct bpf_dtab *dtab;
>> - unsigned int bit;
>> + unsigned int idx; /* keep track of map index for tracepoint */
>> struct xdp_bulk_queue __percpu *bulkq;
>> struct rcu_head rcu;
>> };
>> @@ -75,28 +75,22 @@ struct bpf_dtab {
>> static DEFINE_SPINLOCK(dev_map_lock);
>> static LIST_HEAD(dev_map_list);
>>
>> -static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
>> +static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr
>> *attr,
>> + bool check_memlock)
>
> This check_memlock parameter appears to be unused.
Ah yes, good catch! That was left over from when the patch set also
contained the "default map" stuff. Will fix.
-Toke
^ permalink raw reply
* Re: [PATCH iproute2 1/2] tc: added mask parameter in skbedit action
From: Stephen Hemminger @ 2019-07-08 15:15 UTC (permalink / raw)
To: Roman Mashak; +Cc: netdev, kernel, jhs, xiyou.wangcong, jiri
In-Reply-To: <1562195132-9829-1-git-send-email-mrv@mojatatu.com>
On Wed, 3 Jul 2019 19:05:31 -0400
Roman Mashak <mrv@mojatatu.com> wrote:
> + if (tb[TCA_SKBEDIT_MASK]) {
> + print_uint(PRINT_ANY, "mask", "/0x%x",
> + rta_getattr_u32(tb[TCA_SKBEDIT_MASK]));
Why not print in hex with.
print_hex(PRINT_ANY, "mask", "/%#x",
rta_getattr_u32(tb[TCA_SKBEDIT_MASK]));
^ permalink raw reply
* Re: [PATCH iproute2] ip-route: fix json formatting for metrics
From: Andrea Claudi @ 2019-07-08 15:14 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger, David Ahern
In-Reply-To: <f1535e547aa6da8216ca2a0da7c06b645a132929.1562578533.git.aclaudi@redhat.com>
On Mon, Jul 8, 2019 at 11:38 AM Andrea Claudi <aclaudi@redhat.com> wrote:
>
> Setting metrics for routes currently lead to non-parsable
> json output. For example:
>
> $ ip link add type dummy
> $ ip route add 192.168.2.0 dev dummy0 metric 100 mtu 1000 rto_min 3
> $ ip -j route | jq
> parse error: ':' not as part of an object at line 1, column 319
>
> Fixing this opening a json object in the metrics array and using
> print_string() instead of fprintf().
>
> This is the output for the above commands applying this patch:
>
> $ ip -j route | jq
> [
> {
> "dst": "192.168.2.0",
> "dev": "dummy0",
> "scope": "link",
> "metric": 100,
> "flags": [],
> "metrics": [
> {
> "mtu": 1000,
> "rto_min": 3
> }
> ]
> }
> ]
>
> Fixes: 663c3cb23103f ("iproute: implement JSON and color output")
> Fixes: 968272e791710 ("iproute: refactor metrics print")
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> ---
> ip/iproute.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 1669e0138259e..2f9b612b0b506 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -578,6 +578,7 @@ static void print_rta_metrics(FILE *fp, const struct rtattr *rta)
> int i;
>
> open_json_array(PRINT_JSON, "metrics");
> + open_json_object(NULL);
>
> parse_rtattr(mxrta, RTAX_MAX, RTA_DATA(rta), RTA_PAYLOAD(rta));
>
> @@ -611,7 +612,7 @@ static void print_rta_metrics(FILE *fp, const struct rtattr *rta)
> print_rtax_features(fp, val);
> break;
> default:
> - fprintf(fp, "%u ", val);
> + print_uint(PRINT_ANY, mx_names[i], "%u ", val);
> break;
>
> case RTAX_RTT:
> @@ -639,6 +640,7 @@ static void print_rta_metrics(FILE *fp, const struct rtattr *rta)
> }
> }
>
> + close_json_object();
> close_json_array(PRINT_JSON, NULL);
> }
>
> --
> 2.20.1
>
Sorry, I forgot to add:
Reported-by: Frank Hofmann <fhofmann@cloudflare.com>
Regards,
Andrea
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()
From: Anton Protopopov @ 2019-07-08 15:11 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
netdev, bpf, linux-kernel, andriin
In-Reply-To: <734dd45a-95b0-a7fd-9e1d-0535ef4d3e12@iogearbox.net>
пт, 5 июл. 2019 г. в 17:44, Daniel Borkmann <daniel@iogearbox.net>:
>
> On 07/05/2019 10:44 PM, Anton Protopopov wrote:
> > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in
> > an object by maps pinned to a directory provided in the path argument. Namely,
> > each map M in the object will be replaced by a map pinned to path/M.name.
> >
> > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > ---
> > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 2 ++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 37 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 4907997289e9..84c9e8f7bfd3 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> > return 0;
> > }
> >
> > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path)
> > +{
> > + struct bpf_map *map;
> > +
> > + if (!obj)
> > + return -ENOENT;
> > +
> > + if (!path)
> > + return -EINVAL;
> > +
> > + bpf_object__for_each_map(map, obj) {
> > + int len, err;
> > + int pinned_map_fd;
> > + char buf[PATH_MAX];
>
> We'd need to skip the case of bpf_map__is_internal(map) since they are always
> recreated for the given object.
>
> > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> > + if (len < 0) {
> > + return -EINVAL;
> > + } else if (len >= PATH_MAX) {
> > + return -ENAMETOOLONG;
> > + }
> > +
> > + pinned_map_fd = bpf_obj_get(buf);
> > + if (pinned_map_fd < 0)
> > + return pinned_map_fd;
>
> Should we rather have a new map definition attribute that tells to reuse
> the map if it's pinned in bpf fs, and if not, we create it and later on
> pin it? This is what iproute2 is doing and which we're making use of heavily.
What do you think about adding a new generic field, say load_flags,
to the bpf_map_def structure and a particular flag, say LOAD_F_STICKY
for this purpose? And it will be cleared for internal maps, so we will skip
them as well.
> In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps
> too limiting for a generic API as new version of an object file may contain
> new maps which are not yet present in bpf fs at that point.
How permissive should it be? Is it ok to just print a warning on any
bpf_obj_get()
failure? Or does it make sense to skip some specific error (ENOENT) and reject
on other errors?
>
> > + err = bpf_map__reuse_fd(map, pinned_map_fd);
> > + if (err)
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
> > {
> > struct bpf_program *prog;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index d639f47e3110..7fe465a1be76 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
> > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
> > const char *path);
> > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj,
> > + const char *path);
> > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
> > const char *path);
> > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 2c6d835620d2..66a30be6696c 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 {
> > btf_dump__new;
> > btf__parse_elf;
> > bpf_object__load_xattr;
> > + bpf_object__reuse_maps;
> > libbpf_num_possible_cpus;
> > } LIBBPF_0.0.3;
> >
>
^ permalink raw reply
* Re: [PATCH net-next 1/2] bpf: skip sockopt hooks without CONFIG_NET
From: Yonghong Song @ 2019-07-08 15:06 UTC (permalink / raw)
To: Arnd Bergmann, Alexei Starovoitov, Daniel Borkmann
Cc: Andrii Nakryiko, Martin Lau, Stanislav Fomichev, Song Liu,
Mauricio Vasquez B, Roman Gushchin, Matt Mullins,
Willem de Bruijn, Andrey Ignatov, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190708125733.3944836-1-arnd@arndb.de>
On 7/8/19 5:57 AM, Arnd Bergmann wrote:
> When CONFIG_NET is disabled, we get a link error:
>
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_setsockopt':
> cgroup.c:(.text+0x3010): undefined reference to `lock_sock_nested'
> cgroup.c:(.text+0x3258): undefined reference to `release_sock'
> kernel/bpf/cgroup.o: In function `__cgroup_bpf_run_filter_getsockopt':
> cgroup.c:(.text+0x3568): undefined reference to `lock_sock_nested'
> cgroup.c:(.text+0x3870): undefined reference to `release_sock'
> kernel/bpf/cgroup.o: In function `cg_sockopt_func_proto':
> cgroup.c:(.text+0x41d8): undefined reference to `bpf_sk_storage_delete_proto'
>
> None of this code is useful in this configuration anyway, so we can
> simply hide it in an appropriate #ifdef.
>
> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
FYI.
There is already a patch to fix the same issue,
https://lore.kernel.org/bpf/e9e489fe-feec-a211-82aa-5df0c6a308d1@huawei.com/T/#t
which has been acked and not merged yet.
> ---
> include/linux/bpf_types.h | 2 ++
> kernel/bpf/cgroup.c | 6 ++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index eec5aeeeaf92..3c7222b2db96 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -30,8 +30,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, raw_tracepoint_writable)
> #ifdef CONFIG_CGROUP_BPF
> BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_DEVICE, cg_dev)
> BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SYSCTL, cg_sysctl)
> +#ifdef CONFIG_NET
> BPF_PROG_TYPE(BPF_PROG_TYPE_CGROUP_SOCKOPT, cg_sockopt)
> #endif
> +#endif
> #ifdef CONFIG_BPF_LIRC_MODE2
> BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
> #endif
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 76fa0076f20d..7be44460bd93 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -590,6 +590,7 @@ int cgroup_bpf_prog_query(const union bpf_attr *attr,
> return ret;
> }
>
> +#ifdef CONFIG_NET
> /**
> * __cgroup_bpf_run_filter_skb() - Run a program for packet filtering
> * @sk: The socket sending or receiving traffic
> @@ -750,6 +751,7 @@ int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
> return ret == 1 ? 0 : -EPERM;
> }
> EXPORT_SYMBOL(__cgroup_bpf_run_filter_sock_ops);
> +#endif
>
> int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> short access, enum bpf_attach_type type)
> @@ -939,6 +941,7 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
> }
> EXPORT_SYMBOL(__cgroup_bpf_run_filter_sysctl);
>
> +#ifdef CONFIG_NET
> static bool __cgroup_bpf_prog_array_is_empty(struct cgroup *cgrp,
> enum bpf_attach_type attach_type)
> {
> @@ -1120,6 +1123,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
> return ret;
> }
> EXPORT_SYMBOL(__cgroup_bpf_run_filter_getsockopt);
> +#endif
>
> static ssize_t sysctl_cpy_dir(const struct ctl_dir *dir, char **bufp,
> size_t *lenp)
> @@ -1382,6 +1386,7 @@ const struct bpf_verifier_ops cg_sysctl_verifier_ops = {
> const struct bpf_prog_ops cg_sysctl_prog_ops = {
> };
>
> +#ifdef CONFIG_NET
> static const struct bpf_func_proto *
> cg_sockopt_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> {
> @@ -1531,3 +1536,4 @@ const struct bpf_verifier_ops cg_sockopt_verifier_ops = {
>
> const struct bpf_prog_ops cg_sockopt_prog_ops = {
> };
> +#endif
>
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Matt Hart @ 2019-07-08 15:11 UTC (permalink / raw)
To: andriin; +Cc: andrii.nakryiko, ast, bpf, daniel, kernel-team, netdev, sdf
Hi all,
I bisected a perf build error on ARMv7 to this patch:
libbpf.c: In function ‘perf_event_open_probe’:
libbpf.c:4112:17: error: cast from pointer to integer of different
size [-Werror=pointer-to-int-cast]
attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
^
Is this a known issue?
^ permalink raw reply
* Re: [PATCH bpf-next 0/3] xdp: Add devmap_hash map type
From: Jonathan Lemon @ 2019-07-08 15:10 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <156234940798.2378.9008707939063611210.stgit@alrua-x1>
On 5 Jul 2019, at 10:56, Toke Høiland-Jørgensen wrote:
> This series adds a new map type, devmap_hash, that works like the
> existing
> devmap type, but using a hash-based indexing scheme. This is useful
> for the use
> case where a devmap is indexed by ifindex (for instance for use with
> the routing
> table lookup helper). For this use case, the regular devmap needs to
> be sized
> after the maximum ifindex number, not the number of devices in it. A
> hash-based
> indexing scheme makes it possible to size the map after the number of
> devices it
> should contain instead.
This device hash map is sized at NETDEV_HASHENTRIES == 2^8 == 256. Is
this actually
smaller than an array? What ifindex values are you seeing?
--
Jonathan
>
> This was previously part of my patch series that also turned the
> regular
> bpf_redirect() helper into a map-based one; for this series I just
> pulled out
> the patches that introduced the new map type.
>
> Changelog:
>
> Changes to these patches since the previous series:
>
> - Rebase on top of the other devmap changes (makes this one simpler!)
> - Don't enforce key==val, but allow arbitrary indexes.
> - Rename the type to devmap_hash to reflect the fact that it's just a
> hashmap now.
>
> ---
>
> Toke Høiland-Jørgensen (3):
> include/bpf.h: Remove map_insert_ctx() stubs
> xdp: Refactor devmap allocation code for reuse
> xdp: Add devmap_hash map type for looking up devices by hashed
> index
>
>
> include/linux/bpf.h | 11 -
> include/linux/bpf_types.h | 1
> include/trace/events/xdp.h | 3
> include/uapi/linux/bpf.h | 7 -
> kernel/bpf/devmap.c | 325
> ++++++++++++++++++++++++++-----
> kernel/bpf/verifier.c | 2
> net/core/filter.c | 9 +
> tools/bpf/bpftool/map.c | 1
> tools/include/uapi/linux/bpf.h | 7 -
> tools/lib/bpf/libbpf_probes.c | 1
> tools/testing/selftests/bpf/test_maps.c | 16 ++
> 11 files changed, 316 insertions(+), 67 deletions(-)
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] xdp: Refactor devmap allocation code for reuse
From: Jonathan Lemon @ 2019-07-08 15:06 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <156234940841.2378.6629890565300526702.stgit@alrua-x1>
On 5 Jul 2019, at 10:56, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The subsequent patch to add a new devmap sub-type can re-use much of
> the
> initialisation and allocation code, so refactor it into separate
> functions.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> kernel/bpf/devmap.c | 137
> +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 84 insertions(+), 53 deletions(-)
>
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index d83cf8ccc872..a2fe16362129 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -60,7 +60,7 @@ struct xdp_bulk_queue {
> struct bpf_dtab_netdev {
> struct net_device *dev; /* must be first member, due to tracepoint
> */
> struct bpf_dtab *dtab;
> - unsigned int bit;
> + unsigned int idx; /* keep track of map index for tracepoint */
> struct xdp_bulk_queue __percpu *bulkq;
> struct rcu_head rcu;
> };
> @@ -75,28 +75,22 @@ struct bpf_dtab {
> static DEFINE_SPINLOCK(dev_map_lock);
> static LIST_HEAD(dev_map_list);
>
> -static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
> +static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr
> *attr,
> + bool check_memlock)
This check_memlock parameter appears to be unused.
--
Jonathan
> {
> - struct bpf_dtab *dtab;
> int err, cpu;
> u64 cost;
>
> - if (!capable(CAP_NET_ADMIN))
> - return ERR_PTR(-EPERM);
> -
> /* check sanity of attributes */
> if (attr->max_entries == 0 || attr->key_size != 4 ||
> attr->value_size != 4 || attr->map_flags &
> ~DEV_CREATE_FLAG_MASK)
> - return ERR_PTR(-EINVAL);
> + return -EINVAL;
>
> /* Lookup returns a pointer straight to dev->ifindex, so make sure
> the
> * verifier prevents writes from the BPF side
> */
> attr->map_flags |= BPF_F_RDONLY_PROG;
>
> - dtab = kzalloc(sizeof(*dtab), GFP_USER);
> - if (!dtab)
> - return ERR_PTR(-ENOMEM);
>
> bpf_map_init_from_attr(&dtab->map, attr);
>
> @@ -107,9 +101,7 @@ static struct bpf_map *dev_map_alloc(union
> bpf_attr *attr)
> /* if map size is larger than memlock limit, reject it */
> err = bpf_map_charge_init(&dtab->map.memory, cost);
> if (err)
> - goto free_dtab;
> -
> - err = -ENOMEM;
> + return -EINVAL;
>
> dtab->flush_list = alloc_percpu(struct list_head);
> if (!dtab->flush_list)
> @@ -124,19 +116,38 @@ static struct bpf_map *dev_map_alloc(union
> bpf_attr *attr)
> if (!dtab->netdev_map)
> goto free_percpu;
>
> - spin_lock(&dev_map_lock);
> - list_add_tail_rcu(&dtab->list, &dev_map_list);
> - spin_unlock(&dev_map_lock);
> -
> - return &dtab->map;
> + return 0;
>
> free_percpu:
> free_percpu(dtab->flush_list);
> free_charge:
> bpf_map_charge_finish(&dtab->map.memory);
> -free_dtab:
> - kfree(dtab);
> - return ERR_PTR(err);
> + return -ENOMEM;
> +}
> +
> +static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
> +{
> + struct bpf_dtab *dtab;
> + int err;
> +
> + if (!capable(CAP_NET_ADMIN))
> + return ERR_PTR(-EPERM);
> +
> + dtab = kzalloc(sizeof(*dtab), GFP_USER);
> + if (!dtab)
> + return ERR_PTR(-ENOMEM);
> +
> + err = dev_map_init_map(dtab, attr, true);
> + if (err) {
> + kfree(dtab);
> + return ERR_PTR(err);
> + }
> +
> + spin_lock(&dev_map_lock);
> + list_add_tail_rcu(&dtab->list, &dev_map_list);
> + spin_unlock(&dev_map_lock);
> +
> + return &dtab->map;
> }
>
> static void dev_map_free(struct bpf_map *map)
> @@ -235,7 +246,7 @@ static int bq_xmit_all(struct xdp_bulk_queue *bq,
> u32 flags,
> out:
> bq->count = 0;
>
> - trace_xdp_devmap_xmit(&obj->dtab->map, obj->bit,
> + trace_xdp_devmap_xmit(&obj->dtab->map, obj->idx,
> sent, drops, bq->dev_rx, dev, err);
> bq->dev_rx = NULL;
> __list_del_clearprev(&bq->flush_node);
> @@ -412,17 +423,52 @@ static int dev_map_delete_elem(struct bpf_map
> *map, void *key)
> return 0;
> }
>
> -static int dev_map_update_elem(struct bpf_map *map, void *key, void
> *value,
> - u64 map_flags)
> +static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
> + struct bpf_dtab *dtab,
> + u32 ifindex,
> + unsigned int idx)
> {
> - struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> - struct net *net = current->nsproxy->net_ns;
> gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
> + struct bpf_dtab_netdev *dev;
> + struct xdp_bulk_queue *bq;
> + int cpu;
> +
> + dev = kmalloc_node(sizeof(*dev), gfp, dtab->map.numa_node);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev->bulkq = __alloc_percpu_gfp(sizeof(*dev->bulkq),
> + sizeof(void *), gfp);
> + if (!dev->bulkq) {
> + kfree(dev);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + for_each_possible_cpu(cpu) {
> + bq = per_cpu_ptr(dev->bulkq, cpu);
> + bq->obj = dev;
> + }
> +
> + dev->dev = dev_get_by_index(net, ifindex);
> + if (!dev->dev) {
> + free_percpu(dev->bulkq);
> + kfree(dev);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + dev->idx = idx;
> + dev->dtab = dtab;
> +
> + return dev;
> +}
> +
> +static int __dev_map_update_elem(struct net *net, struct bpf_map
> *map,
> + void *key, void *value, u64 map_flags)
> +{
> + struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> struct bpf_dtab_netdev *dev, *old_dev;
> u32 ifindex = *(u32 *)value;
> - struct xdp_bulk_queue *bq;
> u32 i = *(u32 *)key;
> - int cpu;
>
> if (unlikely(map_flags > BPF_EXIST))
> return -EINVAL;
> @@ -434,31 +480,9 @@ static int dev_map_update_elem(struct bpf_map
> *map, void *key, void *value,
> if (!ifindex) {
> dev = NULL;
> } else {
> - dev = kmalloc_node(sizeof(*dev), gfp, map->numa_node);
> - if (!dev)
> - return -ENOMEM;
> -
> - dev->bulkq = __alloc_percpu_gfp(sizeof(*dev->bulkq),
> - sizeof(void *), gfp);
> - if (!dev->bulkq) {
> - kfree(dev);
> - return -ENOMEM;
> - }
> -
> - for_each_possible_cpu(cpu) {
> - bq = per_cpu_ptr(dev->bulkq, cpu);
> - bq->obj = dev;
> - }
> -
> - dev->dev = dev_get_by_index(net, ifindex);
> - if (!dev->dev) {
> - free_percpu(dev->bulkq);
> - kfree(dev);
> - return -EINVAL;
> - }
> -
> - dev->bit = i;
> - dev->dtab = dtab;
> + dev = __dev_map_alloc_node(net, dtab, ifindex, i);
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> }
>
> /* Use call_rcu() here to ensure rcu critical sections have
> completed
> @@ -472,6 +496,13 @@ static int dev_map_update_elem(struct bpf_map
> *map, void *key, void *value,
> return 0;
> }
>
> +static int dev_map_update_elem(struct bpf_map *map, void *key, void
> *value,
> + u64 map_flags)
> +{
> + return __dev_map_update_elem(current->nsproxy->net_ns,
> + map, key, value, map_flags);
> +}
> +
> const struct bpf_map_ops dev_map_ops = {
> .map_alloc = dev_map_alloc,
> .map_free = dev_map_free,
^ permalink raw reply
* Re: [PATCH bpf-next v2 4/6] xdp: Add devmap_hash map type for looking up devices by hashed index
From: Y Song @ 2019-07-08 15:01 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Daniel Borkmann, Alexei Starovoitov, netdev, David Miller,
Jesper Dangaard Brouer, Jakub Kicinski, Björn Töpel
In-Reply-To: <874l3w26lb.fsf@toke.dk>
On Mon, Jul 8, 2019 at 2:55 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Y Song <ys114321@gmail.com> writes:
>
> > On Sat, Jul 6, 2019 at 1:48 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> A common pattern when using xdp_redirect_map() is to create a device map
> >> where the lookup key is simply ifindex. Because device maps are arrays,
> >> this leaves holes in the map, and the map has to be sized to fit the
> >> largest ifindex, regardless of how many devices actually are actually
> >> needed in the map.
> >>
> >> This patch adds a second type of device map where the key is looked up
> >> using a hashmap, instead of being used as an array index. This allows maps
> >> to be densely packed, so they can be smaller.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >> include/linux/bpf.h | 7 ++
> >> include/linux/bpf_types.h | 1
> >> include/trace/events/xdp.h | 3 -
> >> kernel/bpf/devmap.c | 192 ++++++++++++++++++++++++++++++++++++++++++++
> >> kernel/bpf/verifier.c | 2
> >> net/core/filter.c | 9 ++
> >> 6 files changed, 211 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index bfdb54dd2ad1..f9a506147c8a 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -713,6 +713,7 @@ struct xdp_buff;
> >> struct sk_buff;
> >>
> >> struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> >> +struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key);
> >> void __dev_map_flush(struct bpf_map *map);
> >> int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
> >> struct net_device *dev_rx);
> >> @@ -799,6 +800,12 @@ static inline struct net_device *__dev_map_lookup_elem(struct bpf_map *map,
> >> return NULL;
> >> }
> >>
> >> +static inline struct net_device *__dev_map_hash_lookup_elem(struct bpf_map *map,
> >> + u32 key)
> >> +{
> >> + return NULL;
> >> +}
> >> +
> >> static inline void __dev_map_flush(struct bpf_map *map)
> >> {
> >> }
> >> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> >> index eec5aeeeaf92..36a9c2325176 100644
> >> --- a/include/linux/bpf_types.h
> >> +++ b/include/linux/bpf_types.h
> >> @@ -62,6 +62,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY_OF_MAPS, array_of_maps_map_ops)
> >> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS, htab_of_maps_map_ops)
> >> #ifdef CONFIG_NET
> >> BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
> >> +BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops)
> >> BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops)
> >> #if defined(CONFIG_BPF_STREAM_PARSER)
> >> BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
> >> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> >> index 68899fdc985b..8c8420230a10 100644
> >> --- a/include/trace/events/xdp.h
> >> +++ b/include/trace/events/xdp.h
> >> @@ -175,7 +175,8 @@ struct _bpf_dtab_netdev {
> >> #endif /* __DEVMAP_OBJ_TYPE */
> >>
> >> #define devmap_ifindex(fwd, map) \
> >> - ((map->map_type == BPF_MAP_TYPE_DEVMAP) ? \
> >> + ((map->map_type == BPF_MAP_TYPE_DEVMAP || \
> >> + map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) ? \
> >> ((struct _bpf_dtab_netdev *)fwd)->dev->ifindex : 0)
> >>
> >> #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx) \
> >> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> >> index a2fe16362129..341af02f049d 100644
> >> --- a/kernel/bpf/devmap.c
> >> +++ b/kernel/bpf/devmap.c
> >> @@ -37,6 +37,12 @@
> >> * notifier hook walks the map we know that new dev references can not be
> >> * added by the user because core infrastructure ensures dev_get_by_index()
> >> * calls will fail at this point.
> >> + *
> >> + * The devmap_hash type is a map type which interprets keys as ifindexes and
> >> + * indexes these using a hashmap. This allows maps that use ifindex as key to be
> >> + * densely packed instead of having holes in the lookup array for unused
> >> + * ifindexes. The setup and packet enqueue/send code is shared between the two
> >> + * types of devmap; only the lookup and insertion is different.
> >> */
> >> #include <linux/bpf.h>
> >> #include <net/xdp.h>
> >> @@ -59,6 +65,7 @@ struct xdp_bulk_queue {
> >>
> >> struct bpf_dtab_netdev {
> >> struct net_device *dev; /* must be first member, due to tracepoint */
> >> + struct hlist_node index_hlist;
> >> struct bpf_dtab *dtab;
> >> unsigned int idx; /* keep track of map index for tracepoint */
> >> struct xdp_bulk_queue __percpu *bulkq;
> >> @@ -70,11 +77,29 @@ struct bpf_dtab {
> >> struct bpf_dtab_netdev **netdev_map;
> >> struct list_head __percpu *flush_list;
> >> struct list_head list;
> >> +
> >> + /* these are only used for DEVMAP_HASH type maps */
> >> + unsigned int items;
> >> + struct hlist_head *dev_index_head;
> >> + spinlock_t index_lock;
> >> };
> >>
> >> static DEFINE_SPINLOCK(dev_map_lock);
> >> static LIST_HEAD(dev_map_list);
> >>
> >> +static struct hlist_head *dev_map_create_hash(void)
> >> +{
> >> + int i;
> >> + struct hlist_head *hash;
> >> +
> >> + hash = kmalloc_array(NETDEV_HASHENTRIES, sizeof(*hash), GFP_KERNEL);
> >> + if (hash != NULL)
> >> + for (i = 0; i < NETDEV_HASHENTRIES; i++)
> >> + INIT_HLIST_HEAD(&hash[i]);
> >> +
> >> + return hash;
> >> +}
> >> +
> >> static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
> >> bool check_memlock)
> >> {
> >> @@ -98,6 +123,9 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
> >> cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
> >> cost += sizeof(struct list_head) * num_possible_cpus();
> >>
> >> + if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH)
> >> + cost += sizeof(struct hlist_head) * NETDEV_HASHENTRIES;
> >> +
> >> /* if map size is larger than memlock limit, reject it */
> >> err = bpf_map_charge_init(&dtab->map.memory, cost);
> >> if (err)
> >> @@ -116,8 +144,18 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
> >> if (!dtab->netdev_map)
> >> goto free_percpu;
> >>
> >> + if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
> >> + dtab->dev_index_head = dev_map_create_hash();
> >> + if (!dtab->dev_index_head)
> >> + goto free_map_area;
> >> +
> >> + spin_lock_init(&dtab->index_lock);
> >> + }
> >> +
> >> return 0;
> >>
> >> +free_map_area:
> >> + bpf_map_area_free(dtab->netdev_map);
> >> free_percpu:
> >> free_percpu(dtab->flush_list);
> >> free_charge:
> >> @@ -199,6 +237,7 @@ static void dev_map_free(struct bpf_map *map)
> >>
> >> free_percpu(dtab->flush_list);
> >> bpf_map_area_free(dtab->netdev_map);
> >> + kfree(dtab->dev_index_head);
> >> kfree(dtab);
> >> }
> >>
> >> @@ -219,6 +258,70 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
> >> return 0;
> >> }
> >>
> >> +static inline struct hlist_head *dev_map_index_hash(struct bpf_dtab *dtab,
> >> + int idx)
> >> +{
> >> + return &dtab->dev_index_head[idx & (NETDEV_HASHENTRIES - 1)];
> >> +}
> >> +
> >> +struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
> >> +{
> >> + struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> >> + struct hlist_head *head = dev_map_index_hash(dtab, key);
> >> + struct bpf_dtab_netdev *dev;
> >> +
> >> + hlist_for_each_entry_rcu(dev, head, index_hlist)
> >> + if (dev->idx == key)
> >> + return dev;
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static int dev_map_hash_get_next_key(struct bpf_map *map, void *key,
> >> + void *next_key)
> >> +{
> >> + struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> >> + u32 idx, *next = next_key;
> >> + struct bpf_dtab_netdev *dev, *next_dev;
> >> + struct hlist_head *head;
> >> + int i = 0;
> >> +
> >> + if (!key)
> >> + goto find_first;
> >> +
> >> + idx = *(u32 *)key;
> >> +
> >> + dev = __dev_map_hash_lookup_elem(map, idx);
> >> + if (!dev)
> >> + goto find_first;
> >> +
> >> + next_dev = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(&dev->index_hlist)),
> >> + struct bpf_dtab_netdev, index_hlist);
> >
> > Just want to get a better understanding. Why do you want
> > hlist_entry_safe instead of hlist_entry?
>
> Erm, because the entry might not exist? The _safe variant just checks
> the list pointer before casting to the containing struct.
hlist_entry_safe is certainly correct. Just want to better understand when
the entry might not exist. By looking at hashtab.c implementation, it has
the same possible-null-entry assumption, so we should be ok here.
>
> > Also, maybe rcu_dereference instead of rcu_dereference_raw?
>
> Well, the _raw() variant comes from the equivalent function in
> hashtab.c, where I originally nicked most of this function, so think it
> makes more sense to keep them the same.
Okay.
>
> > dev_map_hash_get_next_key() is called in syscall.c within rcu_read_lock region.
> >
> >> +
> >> + if (next_dev) {
> >> + *next = next_dev->idx;
> >> + return 0;
> >> + }
> >> +
> >> + i = idx & (NETDEV_HASHENTRIES - 1);
> >> + i++;
> >> +
> >> + find_first:
> >> + for (; i < NETDEV_HASHENTRIES; i++) {
> >> + head = dev_map_index_hash(dtab, i);
> >> +
> >> + next_dev = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),
> >> + struct bpf_dtab_netdev,
> >> + index_hlist);
> >
> > ditto. The same question as the above.
> >
> >> + if (next_dev) {
> >> + *next = next_dev->idx;
> >> + return 0;
> >> + }
> >> + }
> >> +
> >> + return -ENOENT;
> >> +}
> >> +
> >> static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags,
> >> bool in_napi_ctx)
> >> {
> >> @@ -374,6 +477,15 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
> >> return dev ? &dev->ifindex : NULL;
> >> }
> >>
> >> +static void *dev_map_hash_lookup_elem(struct bpf_map *map, void *key)
> >> +{
> >> + struct bpf_dtab_netdev *obj = __dev_map_hash_lookup_elem(map,
> >> + *(u32 *)key);
> >> + struct net_device *dev = obj ? obj->dev : NULL;
> >
> > When obj->dev could be NULL?
>
> Never. This could just as well be written as
>
> return obj ? obj->dev->ifindex : NULL;
>
> but writing it this way keeps it consistent with the existing
> dev_map_lookup_elem() function.
Okay then in order to be consistent with the existing code base.
Thanks for all the explanation!
^ permalink raw reply
* [PATCH net,v2] nfc: fix potential illegal memory access
From: Yang Wei @ 2019-07-08 14:57 UTC (permalink / raw)
To: netdev, linux-kernel; +Cc: davem, tglx, albin_yang
The frags_q is not properly initialized, it may result in illegal memory
access when conn_info is NULL.
The "goto free_exit" should be replaced by "goto exit".
Signed-off-by: Yang Wei <albin_yang@163.com>
---
net/nfc/nci/data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index 0a0c265..ce3382b 100644
--- a/net/nfc/nci/data.c
+++ b/net/nfc/nci/data.c
@@ -107,7 +107,7 @@ static int nci_queue_tx_data_frags(struct nci_dev *ndev,
conn_info = nci_get_conn_info_by_conn_id(ndev, conn_id);
if (!conn_info) {
rc = -EPROTO;
- goto free_exit;
+ goto exit;
}
__skb_queue_head_init(&frags_q);
--
2.7.4
^ permalink raw reply related
* Re: i.mx6ul with DSA in multi chip addressing mode - no MDIO access
From: Andrew Lunn @ 2019-07-08 14:57 UTC (permalink / raw)
To: Benjamin Beckmeyer; +Cc: netdev
In-Reply-To: <5e35a41c-be0e-efd4-cb69-cf5c860b872e@eks-engel.de>
> Hi Andrew,
> I got it working a little bit better. When I'm fast enough I can read
> the registers I want but it isn't a solution.
Why do you need to read registers?
What you actually might be interested in is the debugfs patches in
Viviens github tree.
> Here is an output of the tracing even with my custom accesses.
> mii -i 2 0 0x9b60; mii -i 2 1
> phyid:2, reg:0x01 -> 0xc801
>
> Do you know how to delete EEInt bit? It is always one. And now all
> accesses coming from the kworker thread. Maybe this is your polling
> function?
EEInt should clear on read for older chips. For 6390, it might be
necessary to read global 2, register 0x13, index 03.
> I view the INT pin on an oscilloscope but it never changed. So maybe
> this is the problem. We just soldered a pull-up to that pin but it
> still never changend. Maybe you have an idea?
The EEInt bit is probably masked. So it will not generate in
interrupt.
> So what I think is, because of the EEInt bit is never set back to one
> i will poll it as fast as possible.
Is it forever looping in mv88e6xxx_g1_irq_thread_work? Or is it the
polling code, mv88e6xxx_irq_poll() firing every 100ms?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support
From: Ilias Apalodimas @ 2019-07-08 14:51 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: Michael Chan, davem, netdev, hawk, ast, ivan.khoronzhuk
In-Reply-To: <20190708142606.GF87269@C02RW35GFVH8.dhcp.broadcom.net>
Hi Andy,
> On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> > Thanks Andy, Michael
> >
> > > + if (event & BNXT_REDIRECT_EVENT)
> > > + xdp_do_flush_map();
> > > +
> > > if (event & BNXT_TX_EVENT) {
> > > struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> > > u16 prod = txr->tx_prod;
> > > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> > >
> > > for (j = 0; j < max_idx;) {
> > > struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > > - struct sk_buff *skb = tx_buf->skb;
> > > + struct sk_buff *skb;
> > > int k, last;
> > >
> > > + if (i < bp->tx_nr_rings_xdp &&
> > > + tx_buf->action == XDP_REDIRECT) {
> > > + dma_unmap_single(&pdev->dev,
> > > + dma_unmap_addr(tx_buf, mapping),
> > > + dma_unmap_len(tx_buf, len),
> > > + PCI_DMA_TODEVICE);
> > > + xdp_return_frame(tx_buf->xdpf);
> > > + tx_buf->action = 0;
> > > + tx_buf->xdpf = NULL;
> > > + j++;
> > > + continue;
> > > + }
> > > +
> >
> > Can't see the whole file here and maybe i am missing something, but since you
> > optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> > not remapped) buffer that can be returned to the pool and resynced for
> > device usage.
> > Is that happening later on the tx clean function?
>
> Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
> receive them and then in bnxt_tx_int_xdp() when the transmits have
> completed (for XDP_TX and XDP_REDIRECT). I think we are doing what is
> proper with respect to mapping vs sync for both cases, but I would be
> fine to be corrected.
>
Yea seems to be doing the right thing,
XDP_TX syncs correctly and reuses with bnxt_reuse_rx_data() right?
This might be a bit confusing for someone reading the driver on the first time,
probably because you'll end up with 2 ways of recycling buffers.
Once a buffers get freed on the XDP path it's either fed back to the pool, so
the next requested buffer get served from the pools cache (ndo_xdp_xmit case in
the patch). If the buffer is used for XDP_TX is's synced correctly but recycled
via bnxt_reuse_rx_data() right? Since you are moving to page pool please
consider having a common approach towards the recycling path. I understand that
means tracking buffers types and make sure you do the right thing on 'tx clean'.
I've done something similar on the netsec driver and i do think this might be a
good thing to add on page_pool API
Again this isn't a blocker at least for me but you already have the buffer type
(via tx_buf->action)
> >
> > > + skb = tx_buf->skb;
> > > if (!skb) {
> > > j++;
> > > continue;
> > > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > > if (rc < 0)
> > > return rc;
> > >
> > > + rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > > + MEM_TYPE_PAGE_SHARED, NULL);
> > > + if (rc) {
> > > + xdp_rxq_info_unreg(&rxr->xdp_rxq);
> >
> > I think you can use page_pool_free directly here (and pge_pool_destroy once
> > Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> > common please?
>
> That's an easy change, I can do that.
>
> >
> > If Ivan's patch get merged please note you'll have to explicitly
> > page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
> > case (not the error habdling here). Sorry for the confusion this might bring!
>
> Funny enough the driver was basically doing that until page_pool_destroy
> was removed (these patches are not new). I saw last week there was
> discussion to add it back, but I did not want to wait to get this on the
> list before that was resolved.
Fair enough
>
> This path works as expected with the code in the tree today so it seemed
> like the correct approach to post something that is working, right? :-)
Yes.
It will continue to work even if you dont change the call in the future.
This is more a 'let's not spread the code' attempt, but removing and re-adding
page_pool_destroy() was/is our mess. We might as well live with the
consequences!
>
> >
> > > + return rc;
> > > + }
> > > +
> > > rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> > > if (rc)
> > > return rc;
> > > @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> > [...]
> >
Thanks!
/Ilias
^ permalink raw reply
* Re: [PATCH] ath10k: work around uninitialized vht_pfr variable
From: Nathan Chancellor @ 2019-07-08 14:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kalle Valo, Miaoqing Pan, David S. Miller, Rakesh Pillai,
Brian Norris, Balaji Pothunoori, Wen Gong, Pradeep kumar Chitrapu,
Sriram R, ath10k, linux-wireless, netdev, linux-kernel,
clang-built-linux
In-Reply-To: <20190708125050.3689133-1-arnd@arndb.de>
On Mon, Jul 08, 2019 at 02:50:06PM +0200, Arnd Bergmann wrote:
> As clang points out, the vht_pfr is assigned to a struct member
> without being initialized in one case:
>
> drivers/net/wireless/ath/ath10k/mac.c:7528:7: error: variable 'vht_pfr' is used uninitialized whenever 'if' condition
> is false [-Werror,-Wsometimes-uninitialized]
> if (!ath10k_mac_can_set_bitrate_mask(ar, band, mask,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7551:20: note: uninitialized use occurs here
> arvif->vht_pfr = vht_pfr;
> ^~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7528:3: note: remove the 'if' if its condition is always true
> if (!ath10k_mac_can_set_bitrate_mask(ar, band, mask,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/ath/ath10k/mac.c:7483:12: note: initialize the variable 'vht_pfr' to silence this warning
> u8 vht_pfr;
>
> Add an explicit but probably incorrect initialization here.
> I suspect we want a better fix here, but chose this approach to
> illustrate the issue.
Yup, I reached out to the maintainers when this issue first cropped up,
should have taken your approach though.
https://lore.kernel.org/lkml/20190702181837.GA118849@archlinux-epyc/
Initializing to zero is better than uninitialized.
>
> Fixes: 8b97b055dc9d ("ath10k: fix failure to set multiple fixed rate")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v5 2/5] devlink: Return physical port fields only for applicable port flavours
From: Jiri Pirko @ 2019-07-08 14:44 UTC (permalink / raw)
To: Parav Pandit; +Cc: netdev, jiri, saeedm, jakub.kicinski
In-Reply-To: <20190708041549.56601-3-parav@mellanox.com>
Mon, Jul 08, 2019 at 06:15:46AM CEST, parav@mellanox.com wrote:
>Physical port number and split group fields are applicable only to
>physical port flavours such as PHYSICAL, CPU and DSA.
>Hence limit returning those values in netlink response to such port
>flavours.
>
>Signed-off-by: Parav Pandit <parav@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ 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