* [PATCH] net: stmmac: make "snps,reset-delays-us" optional again
From: Martin Blumenstingl @ 2019-07-01 22:42 UTC (permalink / raw)
To: davem, netdev, colin.king
Cc: peppe.cavallaro, alexandre.torgue, joabreu, linux-arm-kernel,
linux-kernel, Martin Blumenstingl
Commit 760f1dc2958022 ("net: stmmac: add sanity check to
device_property_read_u32_array call") introduced error checking of the
device_property_read_u32_array() call in stmmac_mdio_reset().
This results in the following error when the "snps,reset-delays-us"
property is not defined in devicetree:
invalid property snps,reset-delays-us
This sanity check made sense until commit 84ce4d0f9f55b4 ("net: stmmac:
initialize the reset delay array") ensured that there are fallback
values for the reset delay if the "snps,reset-delays-us" property is
absent. That was at the cost of making that property mandatory though.
Drop the sanity check for device_property_read_u32_array() and thus make
the "snps,reset-delays-us" property optional again (avoiding the error
message while loading the stmmac driver with a .dtb where the property
is absent).
Fixes: 760f1dc2958022 ("net: stmmac: add sanity check to device_property_read_u32_array call")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
This is a fix for a patch in net-next and should either go into net-next
or 5.3-rcX.
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index f8061e34122f..18cadf0b0d66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -242,7 +242,6 @@ int stmmac_mdio_reset(struct mii_bus *bus)
if (priv->device->of_node) {
struct gpio_desc *reset_gpio;
u32 delays[3] = { 0, 0, 0 };
- int ret;
reset_gpio = devm_gpiod_get_optional(priv->device,
"snps,reset",
@@ -250,15 +249,9 @@ int stmmac_mdio_reset(struct mii_bus *bus)
if (IS_ERR(reset_gpio))
return PTR_ERR(reset_gpio);
- ret = device_property_read_u32_array(priv->device,
- "snps,reset-delays-us",
- delays,
- ARRAY_SIZE(delays));
- if (ret) {
- dev_err(ndev->dev.parent,
- "invalid property snps,reset-delays-us\n");
- return -EINVAL;
- }
+ device_property_read_u32_array(priv->device,
+ "snps,reset-delays-us",
+ delays, ARRAY_SIZE(delays));
if (delays[0])
msleep(DIV_ROUND_UP(delays[0], 1000));
--
2.22.0
^ permalink raw reply related
* Re: Use-after-free in br_multicast_rcv
From: Nikolay Aleksandrov @ 2019-07-01 22:37 UTC (permalink / raw)
To: Martin Weinelt, bridge, Roopa Prabhu; +Cc: netdev
In-Reply-To: <5e43ba82-de32-e419-efc3-5dfca8291973@linuxlounge.net>
[-- Attachment #1: Type: text/plain, Size: 6979 bytes --]
On 7/2/19 1:17 AM, Martin Weinelt wrote:
> Hi again,
>
> On 7/1/19 7:37 PM, Nikolay Aleksandrov wrote:
>> I see, thanks for clarifying this. So on the KASAN could you please try the attached patch ?
>> Also could you please run the br_multicast_rcv+xxx addresses through
>> linux/scripts/faddr2line for your kernel/bridge:
>> usage: faddr2line [--list] <object file> <func+offset> <func+offset>...
>>
>> Thanks,
>> Nik
>>
>
> back with a new report. This is 5.2.0-rc7 + your patch.
>
> Best,
> Martin
>
Thanks! Aaargh.. I made a stupid mistake hurrying to send the patch, apologies.
Here's the fixed version, please give it a go. This report is because
of my change, not because of the previous bug that should've been fixed.
> $ ./faddr2line /usr/lib/debug/lib/modules/5.2.0-rc7+/kernel/net/bridge/bridge.ko br_multicast_rcv+0x4d0/0x4b00
> br_multicast_rcv+0x4d0/0x4b00:
> __skb_header_pointer at /home/hexa/git/linux/./include/linux/skbuff.h:3476
> (inlined by) skb_header_pointer at /home/hexa/git/linux/./include/linux/skbuff.h:3486
> (inlined by) br_ip6_multicast_mld2_report at /home/hexa/git/linux/net/bridge/br_multicast.c:998
> (inlined by) br_multicast_ipv6_rcv at /home/hexa/git/linux/net/bridge/br_multicast.c:1694
> (inlined by) br_multicast_rcv at /home/hexa/git/linux/net/bridge/br_multicast.c:1729
>
>
> [ 329.723036] ==================================================================
> [ 329.732244] BUG: KASAN: stack-out-of-bounds in skb_copy_bits+0x33e/0x730
> [ 329.738974] Write of size 8 at addr ffff888050f09860 by task swapper/4/0
> [ 329.745754]
> [ 329.749528] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G OE 5.2.0-rc7+ #2
> [ 329.756304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [ 329.764062] Call Trace:
> [ 329.768281] <IRQ>
> [ 329.772037] dump_stack+0x71/0xab
> [ 329.776015] print_address_description+0x6a/0x280
> [ 329.780840] ? skb_copy_bits+0x33e/0x730
> [ 329.784817] __kasan_report+0x152/0x1aa
> [ 329.788623] ? skb_copy_bits+0x33e/0x730
> [ 329.792398] ? skb_copy_bits+0x33e/0x730
> [ 329.796231] kasan_report+0xe/0x20
> [ 329.800250] memcpy+0x34/0x50
> [ 329.803716] skb_copy_bits+0x33e/0x730
> [ 329.807736] br_multicast_rcv+0x4d0/0x4b00 [bridge]
> [ 329.811579] ? netif_receive_skb_internal+0x84/0x1a0
> [ 329.815197] ? br_multicast_disable_port+0x150/0x150 [bridge]
> [ 329.819164] ? netif_receive_skb+0x1b/0x1e0
> [ 329.823374] ? br_pass_frame_up+0x25b/0x3a0 [bridge]
> [ 329.828084] ? br_handle_local_finish+0x20/0x20 [bridge]
> [ 329.832960] ? br_fdb_update+0x10e/0x6e0 [bridge]
> [ 329.837599] ? br_handle_frame_finish+0x3c6/0x11d0 [bridge]
> [ 329.843090] br_handle_frame_finish+0x3c6/0x11d0 [bridge]
> [ 329.848091] ? br_pass_frame_up+0x3a0/0x3a0 [bridge]
> [ 329.853063] ? _raw_write_trylock+0x100/0x100
> [ 329.857660] ? update_load_avg+0x1c4/0x1890
> [ 329.861863] ? virtnet_probe+0x1c80/0x1c80 [virtio_net]
> [ 329.866355] br_handle_frame+0x731/0xd90 [bridge]
> [ 329.870343] ? rcu_irq_exit+0x72/0x1c0
> [ 329.873708] ? br_handle_frame_finish+0x11d0/0x11d0 [bridge]
> [ 329.878333] ? do_IRQ+0x71/0x160
> [ 329.881667] ? __update_load_avg_cfs_rq+0x2aa/0x980
> [ 329.885859] ? common_interrupt+0xa/0xf
> [ 329.889588] ? __update_load_avg_cfs_rq+0x2aa/0x980
> [ 329.894482] __netif_receive_skb_core+0xced/0x2d70
> [ 329.900915] ? napi_complete_done+0x10/0x360
> [ 329.905743] ? virtqueue_get_buf_ctx+0x271/0x1130 [virtio_ring]
> [ 329.909920] ? do_xdp_generic+0x20/0x20
> [ 329.912854] ? virtqueue_napi_complete+0x39/0x70 [virtio_net]
> [ 329.916242] ? virtnet_poll+0x94d/0xc78 [virtio_net]
> [ 329.919606] ? receive_buf+0x5120/0x5120 [virtio_net]
> [ 329.924068] ? __netif_receive_skb_one_core+0x97/0x1d0
> [ 329.929248] ? account_entity_enqueue+0x340/0x4c0
> [ 329.933515] __netif_receive_skb_one_core+0x97/0x1d0
> [ 329.937323] ? __netif_receive_skb_core+0x2d70/0x2d70
> [ 329.941076] ? _raw_write_trylock+0x100/0x100
> [ 329.944515] process_backlog+0x19c/0x650
> [ 329.947618] ? update_cfs_group+0x10b/0x380
> [ 329.950863] net_rx_action+0x71e/0xbc0
> [ 329.953899] ? napi_complete_done+0x360/0x360
> [ 329.957240] ? handle_irq_event_percpu+0xeb/0x140
> [ 329.960485] ? _raw_spin_lock+0x7a/0xd0
> [ 329.962980] ? _raw_write_trylock+0x100/0x100
> [ 329.965598] __do_softirq+0x1db/0x5f9
> [ 329.968022] irq_exit+0x123/0x150
> [ 329.970338] do_IRQ+0x71/0x160
> [ 329.972599] common_interrupt+0xf/0xf
> [ 329.975034] </IRQ>
> [ 329.977167] RIP: 0010:native_safe_halt+0xe/0x10
> [ 329.979783] Code: 09 f9 fe 48 8b 04 24 e9 12 ff ff ff e9 07 00 00 00 0f 00 2d d4 60 52 00 f4 c3 66 90 e9 07 00 00 00 0f 00 2d c4 60 52 00 fb f4 <c3> 90 66 66 66 66 90 41 56 41 55 41 54 55 53 e8 7e 05 ba fe 65 44
> [ 329.987611] RSP: 0018:ffff888050347d98 EFLAGS: 00000246 ORIG_RAX: ffffffffffffffd1
> [ 329.991311] RAX: ffffffffa3d1ad00 RBX: 0000000000000004 RCX: ffffffffa28bbbd6
> [ 329.994903] RDX: 1ffff1100a05c5b8 RSI: 0000000000000004 RDI: ffff888050f33f38
> [ 329.998432] RBP: ffffed100a05c5b8 R08: ffffed100a1e67e8 R09: ffffed100a1e67e7
> [ 330.002025] R10: ffff888050f33f3b R11: ffffed100a1e67e8 R12: ffffffffa4c604c0
> [ 330.005736] R13: 0000000000000004 R14: 0000000000000000 R15: ffff8880502e2dc0
> [ 330.010074] ? ldsem_down_write+0x590/0x590
> [ 330.012905] ? rcu_idle_enter+0x106/0x150
> [ 330.016130] ? tsc_verify_tsc_adjust+0x96/0x2a0
> [ 330.019068] default_idle+0x1f/0x280
> [ 330.021710] do_idle+0x2d8/0x3e0
> [ 330.024246] ? arch_cpu_idle_exit+0x40/0x40
> [ 330.027089] cpu_startup_entry+0x19/0x20
> [ 330.030120] start_secondary+0x316/0x3f0
> [ 330.032770] ? set_cpu_sibling_map+0x19c0/0x19c0
> [ 330.035625] secondary_startup_64+0xa4/0xb0
> [ 330.038485]
> [ 330.040559] The buggy address belongs to the page:
> [ 330.043525] page:ffffea000143c240 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0
> [ 330.047223] flags: 0xffffc000001000(reserved)
> [ 330.050063] raw: 00ffffc000001000 ffffea000143c248 ffffea000143c248 0000000000000000
> [ 330.053806] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
> [ 330.057765] page dumped because: kasan: bad access detected
> [ 330.060927]
> [ 330.063115] Memory state around the buggy address:
> [ 330.066036] ffff888050f09700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 330.069509] ffff888050f09780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 330.073066] >ffff888050f09800: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 02 f4 f4 f4
> [ 330.076586] ^
> [ 330.079861] ffff888050f09880: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 00 04 f4
> [ 330.083397] ffff888050f09900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 330.086923] ==================================================================
> [ 330.090465] Disabling lock debugging due to kernel taint
>
[-- Attachment #2: 0001-net-bridge-mcast-fix-possible-uses-of-stale-pointers.patch --]
[-- Type: text/x-patch, Size: 3465 bytes --]
From c99a71f05ec8643745f435c179c21dda6079c56a Mon Sep 17 00:00:00 2001
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Mon, 1 Jul 2019 20:31:14 +0300
Subject: [PATCH TEST v2] net: bridge: mcast: fix possible uses of stale pointers
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_multicast.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index de22c8fbbb15..d3bb841942b0 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -917,6 +917,8 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
len = skb_transport_offset(skb) + sizeof(*ih);
for (i = 0; i < num; i++) {
+ u16 nsrcs;
+
len += sizeof(*grec);
if (!ip_mc_may_pull(skb, len))
return -EINVAL;
@@ -924,8 +926,9 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
grec = (void *)(skb->data + len - sizeof(*grec));
group = grec->grec_mca;
type = grec->grec_type;
+ nsrcs = ntohs(grec->grec_nsrcs);
- len += ntohs(grec->grec_nsrcs) * 4;
+ len += nsrcs * 4;
if (!ip_mc_may_pull(skb, len))
return -EINVAL;
@@ -946,7 +949,7 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
src = eth_hdr(skb)->h_source;
if ((type == IGMPV3_CHANGE_TO_INCLUDE ||
type == IGMPV3_MODE_IS_INCLUDE) &&
- ntohs(grec->grec_nsrcs) == 0) {
+ nsrcs == 0) {
br_ip4_multicast_leave_group(br, port, group, vid, src);
} else {
err = br_ip4_multicast_add_group(br, port, group, vid,
@@ -983,20 +986,22 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
len = skb_transport_offset(skb) + sizeof(*icmp6h);
for (i = 0; i < num; i++) {
- __be16 *nsrcs, _nsrcs;
+ __be16 *_nsrcs, __nsrcs;
+ u16 nsrcs;
nsrcs_offset = len + offsetof(struct mld2_grec, grec_nsrcs);
if (skb_transport_offset(skb) + ipv6_transport_len(skb) <
- nsrcs_offset + sizeof(_nsrcs))
+ nsrcs_offset + sizeof(__nsrcs))
return -EINVAL;
- nsrcs = skb_header_pointer(skb, nsrcs_offset,
- sizeof(_nsrcs), &_nsrcs);
- if (!nsrcs)
+ _nsrcs = skb_header_pointer(skb, nsrcs_offset,
+ sizeof(__nsrcs), &__nsrcs);
+ if (!_nsrcs)
return -EINVAL;
- grec_len = struct_size(grec, grec_src, ntohs(*nsrcs));
+ nsrcs = ntohs(*_nsrcs);
+ grec_len = struct_size(grec, grec_src, nsrcs);
if (!ipv6_mc_may_pull(skb, len + grec_len))
return -EINVAL;
@@ -1021,7 +1026,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
src = eth_hdr(skb)->h_source;
if ((grec->grec_type == MLD2_CHANGE_TO_INCLUDE ||
grec->grec_type == MLD2_MODE_IS_INCLUDE) &&
- ntohs(*nsrcs) == 0) {
+ nsrcs == 0) {
br_ip6_multicast_leave_group(br, port, &grec->grec_mca,
vid, src);
} else {
@@ -1275,7 +1280,6 @@ static int br_ip6_multicast_query(struct net_bridge *br,
u16 vid)
{
unsigned int transport_len = ipv6_transport_len(skb);
- const struct ipv6hdr *ip6h = ipv6_hdr(skb);
struct mld_msg *mld;
struct net_bridge_mdb_entry *mp;
struct mld2_query *mld2q;
@@ -1319,7 +1323,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
if (is_general_query) {
saddr.proto = htons(ETH_P_IPV6);
- saddr.u.ip6 = ip6h->saddr;
+ saddr.u.ip6 = ipv6_hdr(skb)->saddr;
br_multicast_query_received(br, port, &br->ip6_other_query,
&saddr, max_delay);
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4 bpf-next 7/9] selftests/bpf: switch test to new attach_perf_event API
From: Andrii Nakryiko @ 2019-07-01 22:32 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf@vger.kernel.org, netdev@vger.kernel.org,
Alexei Starovoitov, daniel@iogearbox.net, Kernel Team,
sdf@fomichev.me, Song Liu
In-Reply-To: <60e7bee0-0ab9-dacb-0211-6b93c94f603c@fb.com>
On Mon, Jul 1, 2019 at 10:16 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 8:49 PM, Andrii Nakryiko wrote:
> > Use new bpf_program__attach_perf_event() in test previously relying on
> > direct ioctl manipulations.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > Reviewed-by: Stanislav Fomichev <sdf@google.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > ---
> > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 31 +++++++++----------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > index 1c1a2f75f3d8..9557b7dfb782 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
> > @@ -17,6 +17,7 @@ static __u64 read_perf_max_sample_freq(void)
> > void test_stacktrace_build_id_nmi(void)
> > {
> > int control_map_fd, stackid_hmap_fd, stackmap_fd, stack_amap_fd;
> > + const char *prog_name = "tracepoint/random/urandom_read";
> > const char *file = "./test_stacktrace_build_id.o";
> > int err, pmu_fd, prog_fd;
> > struct perf_event_attr attr = {
> > @@ -25,7 +26,9 @@ void test_stacktrace_build_id_nmi(void)
> > .config = PERF_COUNT_HW_CPU_CYCLES,
> > };
> > __u32 key, previous_key, val, duration = 0;
> > + struct bpf_program *prog;
> > struct bpf_object *obj;
> > + struct bpf_link *link;
> > char buf[256];
> > int i, j;
> > struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> > @@ -39,6 +42,10 @@ void test_stacktrace_build_id_nmi(void)
> > if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
> > return;
> >
> > + prog = bpf_object__find_program_by_title(obj, prog_name);
> > + if (CHECK(!prog, "find_prog", "prog '%s' not found\n", prog_name))
> > + goto close_prog;
> > +
> > pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
> > 0 /* cpu 0 */, -1 /* group id */,
> > 0 /* flags */);
> > @@ -47,15 +54,12 @@ void test_stacktrace_build_id_nmi(void)
> > pmu_fd, errno))
> > goto close_prog;
> >
> > - err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> > - if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n",
> > - err, errno))
> > - goto close_pmu;
> > -
> > - err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> > - if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n",
> > - err, errno))
> > - goto disable_pmu;
> > + link = bpf_program__attach_perf_event(prog, pmu_fd);
> > + if (CHECK(IS_ERR(link), "attach_perf_event",
> > + "err %ld\n", PTR_ERR(link))) {
> > + close(pmu_fd);
> > + goto close_prog;
> > + }
> >
> > /* find map fds */
> > control_map_fd = bpf_find_map(__func__, obj, "control_map");
> > @@ -134,8 +138,7 @@ void test_stacktrace_build_id_nmi(void)
> > * try it one more time.
> > */
> > if (build_id_matches < 1 && retry--) {
> > - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> > - close(pmu_fd);
> > + bpf_link__destroy(link);
> > bpf_object__close(obj);
> > printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> > __func__);
> > @@ -154,11 +157,7 @@ void test_stacktrace_build_id_nmi(void)
> > */
> >
> > disable_pmu:
> > - ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> > -
> > -close_pmu:
> > - close(pmu_fd);
> > -
> > + bpf_link__destroy(link);
>
> There is a problem in bpf_link__destroy(link).
> The "link = bpf_program__attach_perf_event(prog, pmu_fd)"
> may be an error pointer (IS_ERR(link) is true), in which
> case, link should be reset to NULL and then call
> bpf_link__destroy(link). Otherwise, the program may
> segfault or function incorrectly.
Not really, if bpf_program__attach_perf_event fails and IS_ERR(link)
is true, we'll close pmu_fd explicitly and `goto close_prog` bypassing
bpf_link__destroy. `goto disable_pmu` is done only after we
successfully established attached link.
So unless I still miss something, I think this will work reliably.
>
> > close_prog:
> > bpf_object__close(obj);
> > }
> >
^ permalink raw reply
* Re: [PATCH v4 bpf-next 6/9] libbpf: add raw tracepoint attach API
From: Andrii Nakryiko @ 2019-07-01 22:26 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf@vger.kernel.org, netdev@vger.kernel.org,
Alexei Starovoitov, daniel@iogearbox.net, Kernel Team,
sdf@fomichev.me, Song Liu
In-Reply-To: <e6be6907-4587-6106-9868-e76fbf38a3f5@fb.com>
On Mon, Jul 1, 2019 at 10:13 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 8:49 PM, Andrii Nakryiko wrote:
> > Add a wrapper utilizing bpf_link "infrastructure" to allow attaching BPF
> > programs to raw tracepoints.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > Acked-by: Song Liu <songliubraving@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 37 +++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 3 +++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 41 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8ad4f915df38..f8c7a7ecb35e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4263,6 +4263,43 @@ struct bpf_link *bpf_program__attach_tracepoint(struct bpf_program *prog,
> > return link;
> > }
> >
> > +static int bpf_link__destroy_fd(struct bpf_link *link)
> > +{
> > + struct bpf_link_fd *l = (void *)link;
> > +
> > + return close(l->fd);
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> > + const char *tp_name)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link_fd *link;
> > + int prog_fd, pfd;
> > +
> > + prog_fd = bpf_program__fd(prog);
> > + if (prog_fd < 0) {
> > + pr_warning("program '%s': can't attach before loaded\n",
> > + bpf_program__title(prog, false));
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + link = malloc(sizeof(*link));
> > + link->link.destroy = &bpf_link__destroy_fd;
>
> You can move the "link = malloc(...)" etc. after
> bpf_raw_tracepoint_open(). That way, you do not need to free(link)
> in the error case.
It's either `free(link)` here, or `close(pfd)` if malloc fails after
we attached program. Either way extra clean up is needed. I went with
the first one.
>
> > +
> > + pfd = bpf_raw_tracepoint_open(tp_name, prog_fd);
> > + if (pfd < 0) {
> > + pfd = -errno;
> > + free(link);
> > + pr_warning("program '%s': failed to attach to raw tracepoint '%s': %s\n",
> > + bpf_program__title(prog, false), tp_name,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link->fd = pfd;
> > + return (struct bpf_link *)link;
> > +}
> > +
> > enum bpf_perf_event_ret
> > bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> > void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 60611f4b4e1d..f55933784f95 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -182,6 +182,9 @@ LIBBPF_API struct bpf_link *
> > bpf_program__attach_tracepoint(struct bpf_program *prog,
> > const char *tp_category,
> > const char *tp_name);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> > + const char *tp_name);
> >
> > struct bpf_insn;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 3c618b75ef65..e6b7d4edbc93 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -171,6 +171,7 @@ LIBBPF_0.0.4 {
> > bpf_object__load_xattr;
> > bpf_program__attach_kprobe;
> > bpf_program__attach_perf_event;
> > + bpf_program__attach_raw_tracepoint;
> > bpf_program__attach_tracepoint;
> > bpf_program__attach_uprobe;
> > btf_dump__dump_type;
> >
^ permalink raw reply
* Re: Use-after-free in br_multicast_rcv
From: Martin Weinelt @ 2019-07-01 22:17 UTC (permalink / raw)
To: Nikolay Aleksandrov, bridge, Roopa Prabhu; +Cc: netdev
In-Reply-To: <908e9e90-70cc-7bbe-f83f-0810c9ef3925@cumulusnetworks.com>
Hi again,
On 7/1/19 7:37 PM, Nikolay Aleksandrov wrote:
> I see, thanks for clarifying this. So on the KASAN could you please try the attached patch ?
> Also could you please run the br_multicast_rcv+xxx addresses through
> linux/scripts/faddr2line for your kernel/bridge:
> usage: faddr2line [--list] <object file> <func+offset> <func+offset>...
>
> Thanks,
> Nik
>
back with a new report. This is 5.2.0-rc7 + your patch.
Best,
Martin
$ ./faddr2line /usr/lib/debug/lib/modules/5.2.0-rc7+/kernel/net/bridge/bridge.ko br_multicast_rcv+0x4d0/0x4b00
br_multicast_rcv+0x4d0/0x4b00:
__skb_header_pointer at /home/hexa/git/linux/./include/linux/skbuff.h:3476
(inlined by) skb_header_pointer at /home/hexa/git/linux/./include/linux/skbuff.h:3486
(inlined by) br_ip6_multicast_mld2_report at /home/hexa/git/linux/net/bridge/br_multicast.c:998
(inlined by) br_multicast_ipv6_rcv at /home/hexa/git/linux/net/bridge/br_multicast.c:1694
(inlined by) br_multicast_rcv at /home/hexa/git/linux/net/bridge/br_multicast.c:1729
[ 329.723036] ==================================================================
[ 329.732244] BUG: KASAN: stack-out-of-bounds in skb_copy_bits+0x33e/0x730
[ 329.738974] Write of size 8 at addr ffff888050f09860 by task swapper/4/0
[ 329.745754]
[ 329.749528] CPU: 4 PID: 0 Comm: swapper/4 Tainted: G OE 5.2.0-rc7+ #2
[ 329.756304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 329.764062] Call Trace:
[ 329.768281] <IRQ>
[ 329.772037] dump_stack+0x71/0xab
[ 329.776015] print_address_description+0x6a/0x280
[ 329.780840] ? skb_copy_bits+0x33e/0x730
[ 329.784817] __kasan_report+0x152/0x1aa
[ 329.788623] ? skb_copy_bits+0x33e/0x730
[ 329.792398] ? skb_copy_bits+0x33e/0x730
[ 329.796231] kasan_report+0xe/0x20
[ 329.800250] memcpy+0x34/0x50
[ 329.803716] skb_copy_bits+0x33e/0x730
[ 329.807736] br_multicast_rcv+0x4d0/0x4b00 [bridge]
[ 329.811579] ? netif_receive_skb_internal+0x84/0x1a0
[ 329.815197] ? br_multicast_disable_port+0x150/0x150 [bridge]
[ 329.819164] ? netif_receive_skb+0x1b/0x1e0
[ 329.823374] ? br_pass_frame_up+0x25b/0x3a0 [bridge]
[ 329.828084] ? br_handle_local_finish+0x20/0x20 [bridge]
[ 329.832960] ? br_fdb_update+0x10e/0x6e0 [bridge]
[ 329.837599] ? br_handle_frame_finish+0x3c6/0x11d0 [bridge]
[ 329.843090] br_handle_frame_finish+0x3c6/0x11d0 [bridge]
[ 329.848091] ? br_pass_frame_up+0x3a0/0x3a0 [bridge]
[ 329.853063] ? _raw_write_trylock+0x100/0x100
[ 329.857660] ? update_load_avg+0x1c4/0x1890
[ 329.861863] ? virtnet_probe+0x1c80/0x1c80 [virtio_net]
[ 329.866355] br_handle_frame+0x731/0xd90 [bridge]
[ 329.870343] ? rcu_irq_exit+0x72/0x1c0
[ 329.873708] ? br_handle_frame_finish+0x11d0/0x11d0 [bridge]
[ 329.878333] ? do_IRQ+0x71/0x160
[ 329.881667] ? __update_load_avg_cfs_rq+0x2aa/0x980
[ 329.885859] ? common_interrupt+0xa/0xf
[ 329.889588] ? __update_load_avg_cfs_rq+0x2aa/0x980
[ 329.894482] __netif_receive_skb_core+0xced/0x2d70
[ 329.900915] ? napi_complete_done+0x10/0x360
[ 329.905743] ? virtqueue_get_buf_ctx+0x271/0x1130 [virtio_ring]
[ 329.909920] ? do_xdp_generic+0x20/0x20
[ 329.912854] ? virtqueue_napi_complete+0x39/0x70 [virtio_net]
[ 329.916242] ? virtnet_poll+0x94d/0xc78 [virtio_net]
[ 329.919606] ? receive_buf+0x5120/0x5120 [virtio_net]
[ 329.924068] ? __netif_receive_skb_one_core+0x97/0x1d0
[ 329.929248] ? account_entity_enqueue+0x340/0x4c0
[ 329.933515] __netif_receive_skb_one_core+0x97/0x1d0
[ 329.937323] ? __netif_receive_skb_core+0x2d70/0x2d70
[ 329.941076] ? _raw_write_trylock+0x100/0x100
[ 329.944515] process_backlog+0x19c/0x650
[ 329.947618] ? update_cfs_group+0x10b/0x380
[ 329.950863] net_rx_action+0x71e/0xbc0
[ 329.953899] ? napi_complete_done+0x360/0x360
[ 329.957240] ? handle_irq_event_percpu+0xeb/0x140
[ 329.960485] ? _raw_spin_lock+0x7a/0xd0
[ 329.962980] ? _raw_write_trylock+0x100/0x100
[ 329.965598] __do_softirq+0x1db/0x5f9
[ 329.968022] irq_exit+0x123/0x150
[ 329.970338] do_IRQ+0x71/0x160
[ 329.972599] common_interrupt+0xf/0xf
[ 329.975034] </IRQ>
[ 329.977167] RIP: 0010:native_safe_halt+0xe/0x10
[ 329.979783] Code: 09 f9 fe 48 8b 04 24 e9 12 ff ff ff e9 07 00 00 00 0f 00 2d d4 60 52 00 f4 c3 66 90 e9 07 00 00 00 0f 00 2d c4 60 52 00 fb f4 <c3> 90 66 66 66 66 90 41 56 41 55 41 54 55 53 e8 7e 05 ba fe 65 44
[ 329.987611] RSP: 0018:ffff888050347d98 EFLAGS: 00000246 ORIG_RAX: ffffffffffffffd1
[ 329.991311] RAX: ffffffffa3d1ad00 RBX: 0000000000000004 RCX: ffffffffa28bbbd6
[ 329.994903] RDX: 1ffff1100a05c5b8 RSI: 0000000000000004 RDI: ffff888050f33f38
[ 329.998432] RBP: ffffed100a05c5b8 R08: ffffed100a1e67e8 R09: ffffed100a1e67e7
[ 330.002025] R10: ffff888050f33f3b R11: ffffed100a1e67e8 R12: ffffffffa4c604c0
[ 330.005736] R13: 0000000000000004 R14: 0000000000000000 R15: ffff8880502e2dc0
[ 330.010074] ? ldsem_down_write+0x590/0x590
[ 330.012905] ? rcu_idle_enter+0x106/0x150
[ 330.016130] ? tsc_verify_tsc_adjust+0x96/0x2a0
[ 330.019068] default_idle+0x1f/0x280
[ 330.021710] do_idle+0x2d8/0x3e0
[ 330.024246] ? arch_cpu_idle_exit+0x40/0x40
[ 330.027089] cpu_startup_entry+0x19/0x20
[ 330.030120] start_secondary+0x316/0x3f0
[ 330.032770] ? set_cpu_sibling_map+0x19c0/0x19c0
[ 330.035625] secondary_startup_64+0xa4/0xb0
[ 330.038485]
[ 330.040559] The buggy address belongs to the page:
[ 330.043525] page:ffffea000143c240 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0
[ 330.047223] flags: 0xffffc000001000(reserved)
[ 330.050063] raw: 00ffffc000001000 ffffea000143c248 ffffea000143c248 0000000000000000
[ 330.053806] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
[ 330.057765] page dumped because: kasan: bad access detected
[ 330.060927]
[ 330.063115] Memory state around the buggy address:
[ 330.066036] ffff888050f09700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 330.069509] ffff888050f09780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 330.073066] >ffff888050f09800: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 02 f4 f4 f4
[ 330.076586] ^
[ 330.079861] ffff888050f09880: f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2 00 00 04 f4
[ 330.083397] ffff888050f09900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 330.086923] ==================================================================
[ 330.090465] Disabling lock debugging due to kernel taint
^ permalink raw reply
* Re: [PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E
From: Matthias Kaehlcke @ 2019-07-01 22:11 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <8adbb2b8-6747-b876-f85d-75e54f1978cb@gmail.com>
On Mon, Jul 01, 2019 at 10:49:45PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> > By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
> > Enable it if the device tree property 'realtek,enable-ssc' exists.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
> > 1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index dfc2e20ef335..b617169ccc8c 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -9,8 +9,10 @@
> > * Copyright (c) 2004 Freescale Semiconductor, Inc.
> > */
> > #include <linux/bitops.h>
> > -#include <linux/phy.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > #include <linux/module.h>
> > +#include <linux/phy.h>
> >
> > #define RTL821x_PHYSR 0x11
> > #define RTL821x_PHYSR_DUPLEX BIT(13)
> > @@ -28,6 +30,8 @@
> >
> > #define RTL8211E_EXT_PAGE 7
> > #define RTL8211E_EPAGSR 0x1e
> > +#define RTL8211E_SCR 0x1a
> > +#define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
> >
> > #define RTL8211F_INSR 0x1d
> >
> > @@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> > return ret;
> > }
> >
> > -static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> > - int page, u32 regnum, u16 mask, u16 set)
> > +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> > + u32 regnum, u16 mask, u16 set)
> > {
> > int ret = 0;
> > int oldpage;
> > @@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> > return rtl821e_restore_page(phydev, oldpage, ret);
> > }
> >
> > +static int rtl8211e_probe(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + int err;
> > +
> > + if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
> > + err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
> > + RTL8211E_SCR_DISABLE_RXC_SSC,
> > + 0);
> > + if (err)
> > + dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int rtl8201_ack_interrupt(struct phy_device *phydev)
> > {
> > int err;
> > @@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
> > .config_init = &rtl8211e_config_init,
> > .ack_interrupt = &rtl821x_ack_interrupt,
> > .config_intr = &rtl8211e_config_intr,
> > + .probe = rtl8211e_probe,
>
> I'm not sure whether this setting survives soft reset and power-down.
> Maybe it should be better applied in the config_init callback.
Sounds reasonable, I'll change it in the next revision, thanks!
^ permalink raw reply
* Re: [PATCH v4 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Andrii Nakryiko @ 2019-07-01 22:08 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf@vger.kernel.org, netdev@vger.kernel.org,
Alexei Starovoitov, daniel@iogearbox.net, Kernel Team,
sdf@fomichev.me, Song Liu
In-Reply-To: <fbc744f2-74c9-7fca-b9bc-76353b1cf7b5@fb.com>
On Mon, Jul 1, 2019 at 10:09 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 8:49 PM, Andrii Nakryiko wrote:
> > Add ability to attach to kernel and user probes and retprobes.
> > Implementation depends on perf event support for kprobes/uprobes.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 165 +++++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 7 ++
> > tools/lib/bpf/libbpf.map | 2 +
> > 3 files changed, 174 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 98c155ec3bfa..2f79e9563db9 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4019,6 +4019,171 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> > return (struct bpf_link *)link;
> > }
> >
> > +static int parse_value_from_file(const char *file, const char *fmt)
>
> Here, the value from the file must be positive int values to avoid
> confusion between valid value vs. error code.
> Could you add a comment to state this fact for the current
> uprobe/kprobe/tracepoint support?
You are right. I'll name it parse_uint_from_file to make it more
obvious (though even that is not strictly true, we expect value in the
range of [0, 2^32-1]. I'll leave a comment.
>
> > +{
> > + char buf[STRERR_BUFSIZE];
> > + int err, ret;
> > + FILE *f;
> > +
> > + f = fopen(file, "r");
> > + if (!f) {
> > + err = -errno;
> > + pr_debug("failed to open '%s': %s\n", file,
> > + libbpf_strerror_r(err, buf, sizeof(buf)));
> > + fclose(f);
>
> fclose(f) is not needed. fopen has failed.
*facepalm*, of course, thanks!
>
> > + return err;
> > + }
> > + err = fscanf(f, fmt, &ret);
> > + if (err != 1) {
> > + err = err == EOF ? -EIO : -errno;
> > + pr_debug("failed to parse '%s': %s\n", file,
> > + libbpf_strerror_r(err, buf, sizeof(buf)));
> > + fclose(f);
> > + return err;
> > + }
> > + fclose(f);
> > + return ret;
> > +}
> > +
> > +static int determine_kprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> > +
> > + return parse_value_from_file(file, "%d\n");
> > +}
> > +
> > +static int determine_uprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > +
> > + return parse_value_from_file(file, "%d\n");
> > +}
> > +
> > +static int determine_kprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> > +
> > + return parse_value_from_file(file, "config:%d\n");
> > +}
> > +
> > +static int determine_uprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > +
> > + return parse_value_from_file(file, "config:%d\n");
> > +}
> > +
> > +static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > + uint64_t offset, int pid)
> > +{
> > + struct perf_event_attr attr = {};
> > + char errmsg[STRERR_BUFSIZE];
> > + int type, pfd, err;
> > +
> > + type = uprobe ? determine_uprobe_perf_type()
> > + : determine_kprobe_perf_type();
> > + if (type < 0) {
> > + pr_warning("failed to determine %s perf type: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> > + return type;
> > + }
> > + if (retprobe) {
> > + int bit = uprobe ? determine_uprobe_retprobe_bit()
> > + : determine_kprobe_retprobe_bit();
> > +
> > + if (bit < 0) {
> > + pr_warning("failed to determine %s retprobe bit: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(bit, errmsg,
> > + sizeof(errmsg)));
> > + return bit;
> > + }
> > + attr.config |= 1 << bit;
> > + }
> > + attr.size = sizeof(attr);
> > + attr.type = type;
> > + attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> > + attr.config2 = offset; /* kprobe_addr or probe_offset */
> > +
> > + /* pid filter is meaningful only for uprobes */
> > + pfd = syscall(__NR_perf_event_open, &attr,
> > + pid < 0 ? -1 : pid /* pid */,
> > + pid == -1 ? 0 : -1 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + if (pfd < 0) {
> > + err = -errno;
> > + pr_warning("%s perf_event_open() failed: %s\n",
> > + uprobe ? "uprobe" : "kprobe",
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return err;
> > + }
> > + return pfd;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> > + bool retprobe,
> > + const char *func_name)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link *link;
> > + int pfd, err;
> > +
> > + pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> > + 0 /* offset */, -1 /* pid */);
> > + if (pfd < 0) {
> > + pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "kretprobe" : "kprobe", func_name,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link = bpf_program__attach_perf_event(prog, pfd);
> > + if (IS_ERR(link)) {
> > + close(pfd);
> > + err = PTR_ERR(link);
> > + pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "kretprobe" : "kprobe", func_name,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return link;
> > + }
> > + return link;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> > + bool retprobe, pid_t pid,
> > + const char *binary_path,
> > + size_t func_offset)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link *link;
> > + int pfd, err;
> > +
> > + pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> > + binary_path, func_offset, pid);
> > + if (pfd < 0) {
> > + pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "uretprobe" : "uprobe",
> > + binary_path, func_offset,
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
> > + }
> > + link = bpf_program__attach_perf_event(prog, pfd);
> > + if (IS_ERR(link)) {
> > + close(pfd);
> > + err = PTR_ERR(link);
> > + pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> > + bpf_program__title(prog, false),
> > + retprobe ? "uretprobe" : "uprobe",
> > + binary_path, func_offset,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return link;
> > + }
> > + return link;
> > +}
> > +
> > enum bpf_perf_event_ret
> > bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> > void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 1bf66c4a9330..bd767cc11967 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
> >
> > LIBBPF_API struct bpf_link *
> > bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> > + const char *func_name);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> > + pid_t pid, const char *binary_path,
> > + size_t func_offset);
> >
> > struct bpf_insn;
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 756f5aa802e9..57a40fb60718 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
> > global:
> > bpf_link__destroy;
> > bpf_object__load_xattr;
> > + bpf_program__attach_kprobe;
> > bpf_program__attach_perf_event;
> > + bpf_program__attach_uprobe;
> > btf_dump__dump_type;
> > btf_dump__free;
> > btf_dump__new;
> >
^ permalink raw reply
* Re: [PATCH v4 bpf-next 4/4] tools/bpftool: switch map event_pipe to libbpf's perf_buffer
From: Jakub Kicinski @ 2019-07-01 22:04 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: andrii.nakryiko, ast, daniel, kernel-team, bpf, netdev
In-Reply-To: <20190630065109.1794420-5-andriin@fb.com>
On Sat, 29 Jun 2019 23:51:09 -0700, Andrii Nakryiko wrote:
> Switch event_pipe implementation to rely on new libbpf perf buffer API
> (it's raw low-level variant).
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> -int do_event_pipe(int argc, char **argv)
> -{
> - int i, nfds, map_fd, index = -1, cpu = -1;
> struct bpf_map_info map_info = {};
> - struct event_ring_info *rings;
> - size_t tmp_buf_sz = 0;
> - void *tmp_buf = NULL;
> - struct pollfd *pfds;
> + struct perf_buffer_raw_opts opts;
I'm slightly worried we don't init the ops, but we can wait and see if
it bites us or not.
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Thanks!
^ permalink raw reply
* Re: [PATCH v4 bpf-next 3/9] libbpf: add ability to attach/detach BPF program to perf event
From: Andrii Nakryiko @ 2019-07-01 21:57 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf@vger.kernel.org, netdev@vger.kernel.org,
Alexei Starovoitov, daniel@iogearbox.net, Kernel Team,
sdf@fomichev.me, Song Liu
In-Reply-To: <964c51ff-2b83-98e8-4b20-aaa7336a5536@fb.com>
On Mon, Jul 1, 2019 at 10:03 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 8:49 PM, Andrii Nakryiko wrote:
> > bpf_program__attach_perf_event allows to attach BPF program to existing
> > perf event hook, providing most generic and most low-level way to attach BPF
> > programs. It returns struct bpf_link, which should be passed to
> > bpf_link__destroy to detach and free resources, associated with a link.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 61 ++++++++++++++++++++++++++++++++++++++++
> > tools/lib/bpf/libbpf.h | 3 ++
> > tools/lib/bpf/libbpf.map | 1 +
> > 3 files changed, 65 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 455795e6f8af..98c155ec3bfa 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -32,6 +32,7 @@
> > #include <linux/limits.h>
> > #include <linux/perf_event.h>
> > #include <linux/ring_buffer.h>
> > +#include <sys/ioctl.h>
> > #include <sys/stat.h>
> > #include <sys/types.h>
> > #include <sys/vfs.h>
> > @@ -3958,6 +3959,66 @@ int bpf_link__destroy(struct bpf_link *link)
> > return err;
> > }
> >
> > +struct bpf_link_fd {
> > + struct bpf_link link; /* has to be at the top of struct */
> > + int fd; /* hook FD */
> > +};
> > +
> > +static int bpf_link__destroy_perf_event(struct bpf_link *link)
> > +{
> > + struct bpf_link_fd *l = (void *)link;
> > + int err;
> > +
> > + if (l->fd < 0)
> > + return 0;
> > +
> > + err = ioctl(l->fd, PERF_EVENT_IOC_DISABLE, 0);
> > + if (err)
> > + err = -errno;
> > +
> > + close(l->fd);
> > + return err;
> > +}
> > +
> > +struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> > + int pfd)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link_fd *link;
> > + int prog_fd, err;
> > +
> > + prog_fd = bpf_program__fd(prog);
> > + if (prog_fd < 0) {
> > + pr_warning("program '%s': can't attach before loaded\n",
> > + bpf_program__title(prog, false));
> > + return ERR_PTR(-EINVAL);
> > + }
>
> should we check validity of pfd here?
> If pfd < 0, we just return ERR_PTR(-EINVAL)?
I can add that. I didn't do it, because in general, you can provide fd
>= 0 which is still not a valid FD for PERF_EVENT_IOC_SET_BPF and
PERF_EVENT_IOC_ENABLE, so in general we can't detect this reliably.
> This way, in bpf_link__destroy_perf_event(), we do not need to check
> l->fd < 0 since it will be always nonnegative.
That check is not needed anyway, because even if pfd < 0, ioctl should
fail and return error. I'll remove that check.
>
> > +
> > + link = malloc(sizeof(*link));
> > + if (!link)
> > + return ERR_PTR(-ENOMEM);
> > + link->link.destroy = &bpf_link__destroy_perf_event;
> > + link->fd = pfd;
> > +
> > + if (ioctl(pfd, PERF_EVENT_IOC_SET_BPF, prog_fd) < 0) {
> > + err = -errno;
> > + free(link);
> > + pr_warning("program '%s': failed to attach to pfd %d: %s\n",
> > + bpf_program__title(prog, false), pfd,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(err);
> > + }
> > + if (ioctl(pfd, PERF_EVENT_IOC_ENABLE, 0) < 0) {
> > + err = -errno;
> > + free(link);
> > + pr_warning("program '%s': failed to enable pfd %d: %s\n",
> > + bpf_program__title(prog, false), pfd,
> > + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(err);
> > + }
> > + return (struct bpf_link *)link;
> > +}
> > +
> > enum bpf_perf_event_ret
> > bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> > void **copy_mem, size_t *copy_size,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 5082a5ebb0c2..1bf66c4a9330 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -169,6 +169,9 @@ struct bpf_link;
> >
> > LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
> >
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> > +
> > struct bpf_insn;
> >
> > /*
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 3cde850fc8da..756f5aa802e9 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -169,6 +169,7 @@ LIBBPF_0.0.4 {
> > global:
> > bpf_link__destroy;
> > bpf_object__load_xattr;
> > + bpf_program__attach_perf_event;
> > btf_dump__dump_type;
> > btf_dump__free;
> > btf_dump__new;
> >
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:51 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701214649.GE17978@ZenIV.linux.org.uk>
On Mon, Jul 01, 2019 at 10:46:49PM +0100, Al Viro wrote:
> already done cpu_to_le32 to the containing 32bit word. So the
le32_to_cpu, sorry
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:46 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701211356.GD17978@ZenIV.linux.org.uk>
On Mon, Jul 01, 2019 at 10:13:56PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
>
> > > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > > third argument treats it as a host-endian integer. So... Has anyone
> > > tested that code on b-e host? Should that ntohs() actually be swab16(),
> > > yielding (on any host) the same value we currently get for l-e hosts only?
> > >
> > I haven't seen any b-e host with a Realtek network chip yet.
>
> Ever tried to google for realtek 8169 pcie card? The first hit is this:
> https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
> and certainly does look like it should fit into at least some G5 Macs.
> What's more, googling for realtek 8169 PCI card brings quite a bit (top
> hit happens to be on ebay for ~$8). That certainly shall fit into
> any number of big-endian motherboards...
>
> Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
> but these beasts do exist on discrete cards. I'm fairly certain that I've
> got one or two somewhere in the detritus pile and they are fairly cheap
> these days.
>
> So it wouldn't cost too much to put together a mixed network, with
> r8169 both on l-e and b-e hosts and play with VLAN setups there...
FWIW, looking at r8169 docs, they say that Rx descriptor has, at offset
4, a 32bit value (l-e) with
bits 17..31 reserved
bit 16: TAVA (Tag Available)
bits 8..15: VIDL (lower 8 bits of VLAN ID)
bits 5..7: PRIO (priority)
bit 4: CFI (Canonical Format Indicator)
bits 0..3: VIDH (upper 4 bits of VLAN ID)
AFAICS, in kernel-side representation we want VIDL in bits 0..7, VIDH - 8..11,
CFI - 12 and PRIO - 13..15, so that VLAN ID is simply value & 0xfff.
IOW, we do want 256 * octet4 + octet5, i.e.
swab16(le32_to_cpu(desc->opts2) & 0xffff).
Regardless of the host endianness. Some cards might be possible to set up to
byteswap the 32bit values on the way to/from host, but even in that case we
would have wanted
swab16(desc->opts2 & 0xffff),
not
be16_to_cpu(le32_to_cpu(desc->opts2 & 0xffff)
and if it *is* set that way, the current code will break anyway.
Again, all of that is from RTFS alone - I have _not_ tested that, but
it does look like your original patch has missed that while these
16 bits are stored in network order _in_ _card_ _memory_, we'd
already done cpu_to_le32 to the containing 32bit word. So the
value is host-endian, and the lower 16 bits are consistently in
the wrong order, be it b-e or l-e host. IOW, swab16 is the right
thing to do and sparse warning has been correct.
^ permalink raw reply
* Re: [PATCHv3 next 2/3] blackhole_netdev: use blackhole_netdev to invalidate dst entries
From: Michael Chan @ 2019-07-01 21:42 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: Netdev, Eric Dumazet, David Miller, Daniel Axtens,
Mahesh Bandewar
In-Reply-To: <20190701213857.103511-1-maheshb@google.com>
On Mon, Jul 1, 2019 at 2:39 PM Mahesh Bandewar <maheshb@google.com> wrote:
>
> Use blackhole_netdev instead of 'lo' device with lower MTU when marking
> dst "dead".
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Tested-by: Michael Chan <michael.chan@broadcom.com>
^ permalink raw reply
* Re: [PATCHv2 next 3/3] blackhole_dev: add a selftest
From: Mahesh Bandewar (महेश बंडेवार) @ 2019-07-01 21:40 UTC (permalink / raw)
To: David Miller; +Cc: linux-netdev, Eric Dumazet, michael.chan, dja, mahesh
In-Reply-To: <20190629.122847.986644252948714439.davem@davemloft.net>
On Sat, Jun 29, 2019 at 12:28 PM David Miller <davem@davemloft.net> wrote:
>
> From: Mahesh Bandewar <maheshb@google.com>
> Date: Thu, 27 Jun 2019 12:43:09 -0700
>
> > +config TEST_BLACKHOLE_DEV
> > + tristate "Test BPF filter functionality"
>
> I think the tristate string needs to be changed :-)
side effects of copy-paste :(
sending v3
^ permalink raw reply
* [PATCHv3 next 3/3] blackhole_dev: add a selftest
From: Mahesh Bandewar @ 2019-07-01 21:39 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Since this is not really a device with all capabilities, this test
ensures that it has *enough* to make it through the data path
without causing unwanted side-effects (read crash!).
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1 -> v2
fixed the conflict resolution in selftests Makefile
v2 -> v3
fixed lib/Kconfig.debug tristate text / string.
lib/Kconfig.debug | 9 ++
lib/Makefile | 1 +
lib/test_blackhole_dev.c | 100 ++++++++++++++++++
tools/testing/selftests/net/Makefile | 2 +-
tools/testing/selftests/net/config | 1 +
.../selftests/net/test_blackhole_dev.sh | 11 ++
6 files changed, 123 insertions(+), 1 deletion(-)
create mode 100644 lib/test_blackhole_dev.c
create mode 100755 tools/testing/selftests/net/test_blackhole_dev.sh
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index cbdfae379896..99272b5dd980 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1909,6 +1909,15 @@ config TEST_BPF
If unsure, say N.
+config TEST_BLACKHOLE_DEV
+ tristate "Test blackhole netdev functionality"
+ depends on m && NET
+ help
+ This builds the "test_blackhole_dev" module that validates the
+ data path through this blackhole netdev.
+
+ If unsure, say N.
+
config FIND_BIT_BENCHMARK
tristate "Test find_bit functions"
help
diff --git a/lib/Makefile b/lib/Makefile
index dcb558c7554d..6ac44fe2a37f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
+obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
diff --git a/lib/test_blackhole_dev.c b/lib/test_blackhole_dev.c
new file mode 100644
index 000000000000..4c40580a99a3
--- /dev/null
+++ b/lib/test_blackhole_dev.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module tests the blackhole_dev that is created during the
+ * net subsystem initialization. The test this module performs is
+ * by injecting an skb into the stack with skb->dev as the
+ * blackhole_dev and expects kernel to behave in a sane manner
+ * (in other words, *not crash*)!
+ *
+ * Copyright (c) 2018, Mahesh Bandewar <maheshb@google.com>
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/skbuff.h>
+#include <linux/netdevice.h>
+#include <linux/udp.h>
+#include <linux/ipv6.h>
+
+#include <net/dst.h>
+
+#define SKB_SIZE 256
+#define HEAD_SIZE (14+40+8) /* Ether + IPv6 + UDP */
+#define TAIL_SIZE 32 /* random tail-room */
+
+#define UDP_PORT 1234
+
+static int __init test_blackholedev_init(void)
+{
+ struct ipv6hdr *ip6h;
+ struct sk_buff *skb;
+ struct ethhdr *ethh;
+ struct udphdr *uh;
+ int data_len;
+ int ret;
+
+ skb = alloc_skb(SKB_SIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ /* Reserve head-room for the headers */
+ skb_reserve(skb, HEAD_SIZE);
+
+ /* Add data to the skb */
+ data_len = SKB_SIZE - (HEAD_SIZE + TAIL_SIZE);
+ memset(__skb_put(skb, data_len), 0xf, data_len);
+
+ /* Add protocol data */
+ /* (Transport) UDP */
+ uh = (struct udphdr *)skb_push(skb, sizeof(struct udphdr));
+ skb_set_transport_header(skb, 0);
+ uh->source = uh->dest = htons(UDP_PORT);
+ uh->len = htons(data_len);
+ uh->check = 0;
+ /* (Network) IPv6 */
+ ip6h = (struct ipv6hdr *)skb_push(skb, sizeof(struct ipv6hdr));
+ skb_set_network_header(skb, 0);
+ ip6h->hop_limit = 32;
+ ip6h->payload_len = data_len + sizeof(struct udphdr);
+ ip6h->nexthdr = IPPROTO_UDP;
+ ip6h->saddr = in6addr_loopback;
+ ip6h->daddr = in6addr_loopback;
+ /* Ether */
+ ethh = (struct ethhdr *)skb_push(skb, sizeof(struct ethhdr));
+ skb_set_mac_header(skb, 0);
+
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = blackhole_netdev;
+
+ /* Now attempt to send the packet */
+ ret = dev_queue_xmit(skb);
+
+ switch (ret) {
+ case NET_XMIT_SUCCESS:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_SUCCESS\n");
+ break;
+ case NET_XMIT_DROP:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_DROP\n");
+ break;
+ case NET_XMIT_CN:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_CN\n");
+ break;
+ default:
+ pr_err("dev_queue_xmit() returned UNKNOWN(%d)\n", ret);
+ }
+
+ return 0;
+}
+
+static void __exit test_blackholedev_exit(void)
+{
+ pr_warn("test_blackholedev module terminating.\n");
+}
+
+module_init(test_blackholedev_init);
+module_exit(test_blackholedev_exit);
+
+MODULE_AUTHOR("Mahesh Bandewar <maheshb@google.com>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 9a275d932fd5..1b24e36b4047 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -5,7 +5,7 @@ CFLAGS = -Wall -Wl,--no-as-needed -O2 -g
CFLAGS += -I../../../../usr/include/
TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \
- rtnetlink.sh xfrm_policy.sh
+ rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh
TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh
TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index 89f84b5118bf..e4b878d95ba0 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -27,3 +27,4 @@ CONFIG_NFT_CHAIN_NAT_IPV6=m
CONFIG_NFT_CHAIN_NAT_IPV4=m
CONFIG_NET_SCH_FQ=m
CONFIG_NET_SCH_ETF=m
+CONFIG_TEST_BLACKHOLE_DEV=m
diff --git a/tools/testing/selftests/net/test_blackhole_dev.sh b/tools/testing/selftests/net/test_blackhole_dev.sh
new file mode 100755
index 000000000000..3119b80e711f
--- /dev/null
+++ b/tools/testing/selftests/net/test_blackhole_dev.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Runs blackhole-dev test using blackhole-dev kernel module
+
+if /sbin/modprobe -q test_blackhole_dev ; then
+ /sbin/modprobe -q -r test_blackhole_dev;
+ echo "test_blackhole_dev: ok";
+else
+ echo "test_blackhole_dev: [FAIL]";
+ exit 1;
+fi
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 2/3] blackhole_netdev: use blackhole_netdev to invalidate dst entries
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Use blackhole_netdev instead of 'lo' device with lower MTU when marking
dst "dead".
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1->v2->v3
no change
net/core/dst.c | 2 +-
net/ipv4/route.c | 3 +--
net/ipv6/route.c | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/core/dst.c b/net/core/dst.c
index e46366228eaf..1325316d9eab 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -160,7 +160,7 @@ void dst_dev_put(struct dst_entry *dst)
dst->ops->ifdown(dst, dev, true);
dst->input = dst_discard;
dst->output = dst_discard_out;
- dst->dev = dev_net(dst->dev)->loopback_dev;
+ dst->dev = blackhole_netdev;
dev_hold(dst->dev);
dev_put(dev);
}
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index bbd55c7f6b2e..dc1f510a7c81 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1532,7 +1532,6 @@ static void ipv4_dst_destroy(struct dst_entry *dst)
void rt_flush_dev(struct net_device *dev)
{
- struct net *net = dev_net(dev);
struct rtable *rt;
int cpu;
@@ -1543,7 +1542,7 @@ void rt_flush_dev(struct net_device *dev)
list_for_each_entry(rt, &ul->head, rt_uncached) {
if (rt->dst.dev != dev)
continue;
- rt->dst.dev = net->loopback_dev;
+ rt->dst.dev = blackhole_netdev;
dev_hold(rt->dst.dev);
dev_put(dev);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 7556275b1cef..39361f57351a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -176,7 +176,7 @@ static void rt6_uncached_list_flush_dev(struct net *net, struct net_device *dev)
}
if (rt_dev == dev) {
- rt->dst.dev = loopback_dev;
+ rt->dst.dev = blackhole_netdev;
dev_hold(rt->dst.dev);
dev_put(rt_dev);
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 1/3] loopback: create blackhole net device similar to loopack.
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Create a blackhole net device that can be used for "dead"
dst entries instead of loopback device. This blackhole device differs
from loopback in few aspects: (a) It's not per-ns. (b) MTU on this
device is ETH_MIN_MTU (c) The xmit function is essentially kfree_skb().
and (d) since it's not registered it won't have ifindex.
Lower MTU effectively make the device not pass the MTU check during
the route check when a dst associated with the skb is dead.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1->v2->v3
no change
drivers/net/loopback.c | 76 ++++++++++++++++++++++++++++++++++-----
include/linux/netdevice.h | 2 ++
2 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 87d361666cdd..3b39def5471e 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -55,6 +55,13 @@
#include <net/net_namespace.h>
#include <linux/u64_stats_sync.h>
+/* blackhole_netdev - a device used for dsts that are marked expired!
+ * This is global device (instead of per-net-ns) since it's not needed
+ * to be per-ns and gets initialized at boot time.
+ */
+struct net_device *blackhole_netdev;
+EXPORT_SYMBOL(blackhole_netdev);
+
/* The higher levels take care of making this non-reentrant (it's
* called with bh's disabled).
*/
@@ -150,12 +157,14 @@ static const struct net_device_ops loopback_ops = {
.ndo_set_mac_address = eth_mac_addr,
};
-/* The loopback device is special. There is only one instance
- * per network namespace.
- */
-static void loopback_setup(struct net_device *dev)
+static void gen_lo_setup(struct net_device *dev,
+ unsigned int mtu,
+ const struct ethtool_ops *eth_ops,
+ const struct header_ops *hdr_ops,
+ const struct net_device_ops *dev_ops,
+ void (*dev_destructor)(struct net_device *dev))
{
- dev->mtu = 64 * 1024;
+ dev->mtu = mtu;
dev->hard_header_len = ETH_HLEN; /* 14 */
dev->min_header_len = ETH_HLEN; /* 14 */
dev->addr_len = ETH_ALEN; /* 6 */
@@ -174,11 +183,20 @@ static void loopback_setup(struct net_device *dev)
| NETIF_F_NETNS_LOCAL
| NETIF_F_VLAN_CHALLENGED
| NETIF_F_LOOPBACK;
- dev->ethtool_ops = &loopback_ethtool_ops;
- dev->header_ops = ð_header_ops;
- dev->netdev_ops = &loopback_ops;
+ dev->ethtool_ops = eth_ops;
+ dev->header_ops = hdr_ops;
+ dev->netdev_ops = dev_ops;
dev->needs_free_netdev = true;
- dev->priv_destructor = loopback_dev_free;
+ dev->priv_destructor = dev_destructor;
+}
+
+/* The loopback device is special. There is only one instance
+ * per network namespace.
+ */
+static void loopback_setup(struct net_device *dev)
+{
+ gen_lo_setup(dev, (64 * 1024), &loopback_ethtool_ops, ð_header_ops,
+ &loopback_ops, loopback_dev_free);
}
/* Setup and register the loopback device. */
@@ -213,3 +231,43 @@ static __net_init int loopback_net_init(struct net *net)
struct pernet_operations __net_initdata loopback_net_ops = {
.init = loopback_net_init,
};
+
+/* blackhole netdevice */
+static netdev_tx_t blackhole_netdev_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ kfree_skb(skb);
+ net_warn_ratelimited("%s(): Dropping skb.\n", __func__);
+ return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops blackhole_netdev_ops = {
+ .ndo_start_xmit = blackhole_netdev_xmit,
+};
+
+/* This is a dst-dummy device used specifically for invalidated
+ * DSTs and unlike loopback, this is not per-ns.
+ */
+static void blackhole_netdev_setup(struct net_device *dev)
+{
+ gen_lo_setup(dev, ETH_MIN_MTU, NULL, NULL, &blackhole_netdev_ops, NULL);
+}
+
+/* Setup and register the blackhole_netdev. */
+static int __init blackhole_netdev_init(void)
+{
+ blackhole_netdev = alloc_netdev(0, "blackhole_dev", NET_NAME_UNKNOWN,
+ blackhole_netdev_setup);
+ if (!blackhole_netdev)
+ return -ENOMEM;
+
+ dev_init_scheduler(blackhole_netdev);
+ dev_activate(blackhole_netdev);
+
+ blackhole_netdev->flags |= IFF_UP | IFF_RUNNING;
+ dev_net_set(blackhole_netdev, &init_net);
+
+ return 0;
+}
+
+device_initcall(blackhole_netdev_init);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eeacebd7debb..88292953aa6f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4870,4 +4870,6 @@ do { \
#define PTYPE_HASH_SIZE (16)
#define PTYPE_HASH_MASK (PTYPE_HASH_SIZE - 1)
+extern struct net_device *blackhole_netdev;
+
#endif /* _LINUX_NETDEVICE_H */
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 0/3] blackhole device to invalidate dst
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
When we invalidate dst or mark it "dead", we assign 'lo' to
dst->dev. First of all this assignment is racy and more over,
it has MTU implications.
The standard dev MTU is 1500 while the Loopback MTU is 64k. TCP
code when dereferencing the dst don't check if the dst is valid
or not. TCP when dereferencing a dead-dst while negotiating a
new connection, may use dst device which is 'lo' instead of
using the correct device. Consider the following scenario:
A SYN arrives on an interface and tcp-layer while processing
SYNACK finds a dst and associates it with SYNACK skb. Now before
skb gets passed to L3 for processing, if that dst gets "dead"
(because of the virtual device getting disappeared & then reappeared),
the 'lo' gets assigned to that dst (lo MTU = 64k). Let's assume
the SYN has ADV_MSS set as 9k while the output device through
which this SYNACK is going to go out has standard MTU of 1500.
The MTU check during the route check passes since MIN(9K, 64K)
is 9k and TCP successfully negotiates 9k MSS. The subsequent
data packet; bigger in size gets passed to the device and it
won't be marked as GSO since the assumed MTU of the device is
9k.
This either crashes the NIC and we have seen fixes that went
into drivers to handle this scenario. 8914a595110a ('bnx2x:
disable GSO where gso_size is too big for hardware') and
2b16f048729b ('net: create skb_gso_validate_mac_len()') and
with those fixes TCP eventually recovers but not before
few dropped segments.
Well, I'm not a TCP expert and though we have experienced
these corner cases in our environment, I could not reproduce
this case reliably in my test setup to try this fix myself.
However, Michael Chan <michael.chan@broadcom.com> had a setup
where these fixes helped him mitigate the issue and not cause
the crash.
The idea here is to not alter the data-path with additional
locks or smb()/rmb() barriers to avoid racy assignments but
to create a new device that has really low MTU that has
.ndo_start_xmit essentially a kfree_skb(). Make use of this
device instead of 'lo' when marking the dst dead.
First patch implements the blackhole device and second
patch uses it in IPv4 and IPv6 stack while the third patch
is the self test that ensures the sanity of this device.
v1->v2
fixed the self-test patch to handle the conflict
v2 -> v3
fixed Kconfig text/string.
Mahesh Bandewar (3):
loopback: create blackhole net device similar to loopack.
blackhole_netdev: use blackhole_netdev to invalidate dst entries
blackhole_dev: add a selftest
drivers/net/loopback.c | 76 +++++++++++--
include/linux/netdevice.h | 2 +
lib/Kconfig.debug | 9 ++
lib/Makefile | 1 +
lib/test_blackhole_dev.c | 100 ++++++++++++++++++
net/core/dst.c | 2 +-
net/ipv4/route.c | 3 +-
net/ipv6/route.c | 2 +-
tools/testing/selftests/net/Makefile | 2 +-
tools/testing/selftests/net/config | 1 +
.../selftests/net/test_blackhole_dev.sh | 11 ++
11 files changed, 195 insertions(+), 14 deletions(-)
create mode 100644 lib/test_blackhole_dev.c
create mode 100755 tools/testing/selftests/net/test_blackhole_dev.sh
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Matthias Kaehlcke @ 2019-07-01 21:32 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <d2386f7d-b4bc-d983-1b83-cc2aa4aec38b@gmail.com>
On Mon, Jul 01, 2019 at 10:43:12PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> > The RTL8211E has extension pages, which can be accessed after
> > selecting a page through a custom method. Add a function to
> > modify bits in a register of an extension page and a few
> > helpers for dealing with ext pages.
> >
> > rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> > inspired by their counterparts phy_modify_paged() and
> > phy_restore_page().
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > This code might be applicable to other Realtek PHYs, but I don't
> > have access to the datasheets to confirm it, so for now it's just
> > for the RTL8211E.
> >
> This extended page mechanism exists on a number of older Realtek
> PHY's. For most extended pages however Realtek releases no public
> documentation.
> Considering that we use these helpers in one place only, I don't
> really see a need for them.
I see it as self-documenting code, that may be reused, rather than
inline code with comments.
In any case I'm looking into another patch that would write registers
on extented pages rather than doing a modify, if that materializes I
think we would want the helpers.
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Matthias Kaehlcke @ 2019-07-01 21:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, David S . Miller, Rob Herring, Mark Rutland,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <35db1bff-f48e-5372-06b7-3140cb7cbb71@gmail.com>
On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> >
> > Hi Matthias
> >
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> >
> AFAIK: no
From my observations it looks like registers 0x00 to 0x0f are still
accessible, but not the ones above. IIUC 0x00-0x0f are standard
registers, the others are vendor specific.
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jakub Kicinski @ 2019-07-01 21:20 UTC (permalink / raw)
To: Laatz, Kevin
Cc: Jonathan Lemon, netdev, ast, daniel, bjorn.topel, magnus.karlsson,
bpf, intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <07e404eb-f712-b15a-4884-315aff3f7c7d@intel.com>
On Mon, 1 Jul 2019 15:44:29 +0100, Laatz, Kevin wrote:
> On 28/06/2019 21:29, Jonathan Lemon wrote:
> > On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
> >> On 27/06/2019 22:25, Jakub Kicinski wrote:
> >>> I think that's very limiting. What is the challenge in providing
> >>> aligned addresses, exactly?
> >> The challenges are two-fold:
> >> 1) it prevents using arbitrary buffer sizes, which will be an issue
> >> supporting e.g. jumbo frames in future.
> >> 2) higher level user-space frameworks which may want to use AF_XDP,
> >> such as DPDK, do not currently support having buffers with 'fixed'
> >> alignment.
> >> The reason that DPDK uses arbitrary placement is that:
> >> - it would stop things working on certain NICs which need the
> >> actual writable space specified in units of 1k - therefore we need 2k
> >> + metadata space.
> >> - we place padding between buffers to avoid constantly
> >> hitting the same memory channels when accessing memory.
> >> - it allows the application to choose the actual buffer size
> >> it wants to use.
> >> We make use of the above to allow us to speed up processing
> >> significantly and also reduce the packet buffer memory size.
> >>
> >> Not having arbitrary buffer alignment also means an AF_XDP driver
> >> for DPDK cannot be a drop-in replacement for existing drivers in
> >> those frameworks. Even with a new capability to allow an arbitrary
> >> buffer alignment, existing apps will need to be modified to use that
> >> new capability.
> >
> > Since all buffers in the umem are the same chunk size, the original
> > buffer
> > address can be recalculated with some multiply/shift math. However,
> > this is
> > more expensive than just a mask operation.
>
> Yes, we can do this.
That'd be best, can DPDK reasonably guarantee the slicing is uniform?
E.g. it's not desperate buffer pools with different bases?
> Another option we have is to add a socket option for querying the
> metadata length from the driver (assuming it doesn't vary per packet).
> We can use that information to get back to the original address using
> subtraction.
Unfortunately the metadata depends on the packet and how much info
the device was able to extract. So it's variable length.
> Alternatively, we can change the Rx descriptor format to include the
> metadata length. We could do this in a couple of ways, for example,
> rather than returning the address as the start of the packet, instead
> return the buffer address that was passed in, and adding another 16-bit
> field to specify the start of packet offset with that buffer. If using
> another 16-bits of the descriptor space is not desirable, an alternative
> could be to limit umem sizes to e.g. 2^48 bits (256 terabytes should be
> enough, right :-) ) and use the remaining 16 bits of the address as a
> packet offset. Other variations on these approach are obviously possible
> too.
Seems reasonable to me..
^ permalink raw reply
* Re: [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Yuchung Cheng @ 2019-07-01 21:15 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, David Miller, ast, Daniel Borkmann, Eric Dumazet,
Priyaranjan Jha, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
On Mon, Jul 1, 2019 at 1:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Congestion control team would like to have a periodic callback to
> track some TCP statistics. Let's add a sock_ops callback that can be
> selectively enabled on a socket by socket basis and is executed for
> every RTT. BPF program frequency can be further controlled by calling
> bpf_ktime_get_ns and bailing out early.
>
> I run neper tcp_stream and tcp_rr tests with the sample program
> from the last patch and didn't observe any noticeable performance
> difference.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Thanks!
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
>
> Stanislav Fomichev (8):
> bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
> bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
> bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
> bpf: add icsk_retransmits to bpf_tcp_sock
> bpf/tools: sync bpf.h
> selftests/bpf: test BPF_SOCK_OPS_RTT_CB
> samples/bpf: add sample program that periodically dumps TCP stats
> samples/bpf: fix tcp_bpf.readme detach command
>
> include/net/tcp.h | 8 +
> include/uapi/linux/bpf.h | 12 +-
> net/core/filter.c | 207 +++++++++++-----
> net/ipv4/tcp_input.c | 4 +
> samples/bpf/Makefile | 1 +
> samples/bpf/tcp_bpf.readme | 2 +-
> samples/bpf/tcp_dumpstats_kern.c | 65 +++++
> tools/include/uapi/linux/bpf.h | 12 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
> 11 files changed, 570 insertions(+), 58 deletions(-)
> create mode 100644 samples/bpf/tcp_dumpstats_kern.c
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:13 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <81c45b3c-bbaa-c619-981c-8b8f4b73d5c5@gmail.com>
On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
> > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > third argument treats it as a host-endian integer. So... Has anyone
> > tested that code on b-e host? Should that ntohs() actually be swab16(),
> > yielding (on any host) the same value we currently get for l-e hosts only?
> >
> I haven't seen any b-e host with a Realtek network chip yet.
Ever tried to google for realtek 8169 pcie card? The first hit is this:
https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
and certainly does look like it should fit into at least some G5 Macs.
What's more, googling for realtek 8169 PCI card brings quite a bit (top
hit happens to be on ebay for ~$8). That certainly shall fit into
any number of big-endian motherboards...
Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
but these beasts do exist on discrete cards. I'm fairly certain that I've
got one or two somewhere in the detritus pile and they are fairly cheap
these days.
So it wouldn't cost too much to put together a mixed network, with
r8169 both on l-e and b-e hosts and play with VLAN setups there...
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Andrew Lunn @ 2019-07-01 21:09 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <35db1bff-f48e-5372-06b7-3140cb7cbb71@gmail.com>
On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> >
> > Hi Matthias
> >
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> >
> AFAIK: no
This it would be better to make use of the core paged access support,
so that locking is done correctly.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: Gerd Rausch @ 2019-07-01 21:06 UTC (permalink / raw)
To: santosh.shilimkar, netdev; +Cc: David Miller
In-Reply-To: <b5669540-3892-9d79-85ba-79e96ddd3a81@oracle.com>
Hi Santosh,
On 01/07/2019 14.00, santosh.shilimkar@oracle.com wrote:
>>
> Look for command timeout in CX3 sources. 60 second is upper bound in
> CX3. Its not standard in specs(at least not that I know) though
> and may vary from vendor to vendor.
>
I am not seeing it. Can you point me to the right place?
% grep -ni timeout drivers/net/ethernet/mellanox/mlx4/*.[ch]
drivers/net/ethernet/mellanox/mlx4/cmd.c:116: GO_BIT_TIMEOUT_MSECS = 10000
[...]
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:101:#define MLX4_EN_WATCHDOG_TIMEOUT (15 * HZ)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:155:#define MLX4_EN_TX_POLL_TIMEOUT (HZ / 4)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:171:#define MLX4_EN_LOOPBACK_TIMEOUT 100
[...]
drivers/net/ethernet/mellanox/mlx4/reset.c:61:#define MLX4_SEM_TIMEOUT_JIFFIES (10 * HZ)
drivers/net/ethernet/mellanox/mlx4/reset.c:62:#define MLX4_RESET_TIMEOUT_JIFFIES (2 * HZ)
% grep -i timeout drivers/infiniband/hw/mlx4/*.[ch]
drivers/infiniband/hw/mlx4/cm.c:42:#define CM_CLEANUP_CACHE_TIMEOUT (30 * HZ)
[...]
drivers/infiniband/hw/mlx4/mcg.c:46:#define MAD_TIMEOUT_MS 2000
[...]
drivers/infiniband/hw/mlx4/qp.c:4358: while (wait_for_completion_timeout(&sdrain->done, HZ / 10) <= 0)
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: Add support for fq's EDT to HBM
From: Yonghong Song @ 2019-07-01 21:04 UTC (permalink / raw)
To: Lawrence Brakmo, netdev
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eric Dumazet, Kernel Team
In-Reply-To: <20190628194133.2831708-1-brakmo@fb.com>
On 6/28/19 12:41 PM, brakmo wrote:
> Adds support for fq's Earliest Departure Time to HBM (Host Bandwidth
> Manager). Includes a new BPF program supporting EDT, and also updates
> corresponding programs.
>
> It will drop packets with an EDT of more than 500us in the future
> unless the packet belongs to a flow with less than 2 packets in flight.
> This is done so each flow has at least 2 packets in flight, so they
> will not starve, and also to help prevent delayed ACK timeouts.
>
> It will also work with ECN enabled traffic, where the packets will be
> CE marked if their EDT is more than 50us in the future.
>
> The table below shows some performance numbers. The flows are back to
> back RPCS. One server sending to another, either 2 or 4 flows.
> One flow is a 10KB RPC, the rest are 1MB RPCs. When there are more
> than one flow of a given RPC size, the numbers represent averages.
>
> The rate limit applies to all flows (they are in the same cgroup).
> Tests ending with "-edt" ran with the new BPF program supporting EDT.
> Tests ending with "-hbt" ran on top HBT qdisc with the specified rate
> (i.e. no HBM). The other tests ran with the HBM BPF program included
> in the HBM patch-set.
>
> EDT has limited value when using DCTCP, but it helps in many cases when
> using Cubic. It usually achieves larger link utilization and lower
> 99% latencies for the 1MB RPCs.
> HBM ends up queueing a lot of packets with its default parameter values,
> reducing the goodput of the 10KB RPCs and increasing their latency. Also,
> the RTTs seen by the flows are quite large.
>
> Aggr 10K 10K 10K 1MB 1MB 1MB
> Limit rate drops RTT rate P90 P99 rate P90 P99
> Test rate Flows Mbps % us Mbps us us Mbps ms ms
> -------- ---- ----- ---- ----- --- ---- ---- ---- ---- ---- ----
> cubic 1G 2 904 0.02 108 257 511 539 647 13.4 24.5
> cubic-edt 1G 2 982 0.01 156 239 656 967 743 14.0 17.2
> dctcp 1G 2 977 0.00 105 324 408 744 653 14.5 15.9
> dctcp-edt 1G 2 981 0.01 142 321 417 811 660 15.7 17.0
> cubic-htb 1G 2 919 0.00 1825 40 2822 4140 879 9.7 9.9
>
> cubic 200M 2 155 0.30 220 81 532 655 74 283 450
> cubic-edt 200M 2 188 0.02 222 87 1035 1095 101 84 85
> dctcp 200M 2 188 0.03 111 77 912 939 111 76 325
> dctcp-edt 200M 2 188 0.03 217 74 1416 1738 114 76 79
> cubic-htb 200M 2 188 0.00 5015 8 14ms 15ms 180 48 50
>
> cubic 1G 4 952 0.03 110 165 516 546 262 38 154
> cubic-edt 1G 4 973 0.01 190 111 1034 1314 287 65 79
> dctcp 1G 4 951 0.00 103 180 617 905 257 37 38
> dctcp-edt 1G 4 967 0.00 163 151 732 1126 272 43 55
> cubic-htb 1G 4 914 0.00 3249 13 7ms 8ms 300 29 34
>
> cubic 5G 4 4236 0.00 134 305 490 624 1310 10 17
> cubic-edt 5G 4 4865 0.00 156 306 425 759 1520 10 16
> dctcp 5G 4 4936 0.00 128 485 221 409 1484 7 9
> dctcp-edt 5G 4 4924 0.00 148 390 392 623 1508 11 26
>
> v1 -> v2: Incorporated Andrii's suggestions
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> samples/bpf/Makefile | 2 +
> samples/bpf/do_hbm_test.sh | 22 ++---
> samples/bpf/hbm.c | 18 +++-
> samples/bpf/hbm_edt_kern.c | 171 +++++++++++++++++++++++++++++++++++++
> samples/bpf/hbm_kern.h | 40 +++++++--
> 5 files changed, 234 insertions(+), 19 deletions(-)
> create mode 100644 samples/bpf/hbm_edt_kern.c
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 0917f8cf4fab..35640414ebb3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -168,6 +168,7 @@ always += task_fd_query_kern.o
> always += xdp_sample_pkts_kern.o
> always += ibumad_kern.o
> always += hbm_out_kern.o
> +always += hbm_edt_kern.o
>
> KBUILD_HOSTCFLAGS += -I$(objtree)/usr/include
> KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/bpf/
> @@ -272,6 +273,7 @@ $(src)/*.c: verify_target_bpf $(LIBBPF)
> $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
> $(obj)/hbm_out_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
> $(obj)/hbm.o: $(src)/hbm.h
> +$(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
>
> # asm/sysreg.h - inline assembly used by it is incompatible with llvm.
> # But, there is no easy way to fix it, so just exclude it since it is
> diff --git a/samples/bpf/do_hbm_test.sh b/samples/bpf/do_hbm_test.sh
> index e48b047d4646..ffe4c0607341 100755
> --- a/samples/bpf/do_hbm_test.sh
> +++ b/samples/bpf/do_hbm_test.sh
> @@ -14,7 +14,7 @@ Usage() {
> echo "loads. The output is the goodput in Mbps (unless -D was used)."
> echo ""
> echo "USAGE: $name [out] [-b=<prog>|--bpf=<prog>] [-c=<cc>|--cc=<cc>]"
> - echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E]"
> + echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E] [--edt]"
> echo " [-f=<#flows>|--flows=<#flows>] [-h] [-i=<id>|--id=<id >]"
> echo " [-l] [-N] [--no_cn] [-p=<port>|--port=<port>] [-P]"
> echo " [-q=<qdisc>] [-R] [-s=<server>|--server=<server]"
> @@ -30,6 +30,7 @@ Usage() {
> echo " other detailed information. This information is"
> echo " test dependent (i.e. iperf3 or netperf)."
> echo " -E enable ECN (not required for dctcp)"
> + echo " --edt use fq's Earliest Departure Time (requires fq)"
> echo " -f or --flows number of concurrent flows (default=1)"
> echo " -i or --id cgroup id (an integer, default is 1)"
> echo " -N use netperf instead of iperf3"
> @@ -130,13 +131,12 @@ processArgs () {
> details=1
> ;;
> -E)
> - ecn=1
> + ecn=1
> + ;;
> + --edt)
> + flags="$flags --edt"
> + qdisc="fq"
> ;;
> - # Support for upcomming fq Early Departure Time egress rate limiting
> - #--edt)
> - # prog="hbm_out_edt_kern.o"
> - # qdisc="fq"
> - # ;;
> -f=*|--flows=*)
> flows="${i#*=}"
> ;;
> @@ -228,8 +228,8 @@ if [ "$netem" -ne "0" ] ; then
> tc qdisc del dev lo root > /dev/null 2>&1
> tc qdisc add dev lo root netem delay $netem\ms > /dev/null 2>&1
> elif [ "$qdisc" != "" ] ; then
> - tc qdisc del dev lo root > /dev/null 2>&1
> - tc qdisc add dev lo root $qdisc > /dev/null 2>&1
> + tc qdisc del dev eth0 root > /dev/null 2>&1
> + tc qdisc add dev eth0 root $qdisc > /dev/null 2>&1
> fi
>
> n=0
> @@ -399,7 +399,9 @@ fi
> if [ "$netem" -ne "0" ] ; then
> tc qdisc del dev lo root > /dev/null 2>&1
> fi
> -
> +if [ "$qdisc" != "" ] ; then
> + tc qdisc del dev eth0 root > /dev/null 2>&1
> +fi
> sleep 2
>
> hbmPid=`ps ax | grep "hbm " | grep --invert-match "grep" | awk '{ print $1 }'`
> diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c
> index b905b32ff185..e0fbab9bec83 100644
> --- a/samples/bpf/hbm.c
> +++ b/samples/bpf/hbm.c
> @@ -62,6 +62,7 @@ bool loopback_flag;
> bool debugFlag;
> bool work_conserving_flag;
> bool no_cn_flag;
> +bool edt_flag;
>
> static void Usage(void);
> static void read_trace_pipe2(void);
> @@ -372,9 +373,14 @@ static int run_bpf_prog(char *prog, int cg_id)
> fprintf(fout, "avg rtt:%d\n",
> (int)(qstats.sum_rtt / (qstats.pkts_total + 1)));
> // Average credit
> - fprintf(fout, "avg credit:%d\n",
> - (int)(qstats.sum_credit /
> - (1500 * ((int)qstats.pkts_total) + 1)));
> + if (edt_flag)
> + fprintf(fout, "avg credit_ms:%.03f\n",
> + (qstats.sum_credit /
> + (qstats.pkts_total + 1.0)) / 1000000.0);
> + else
> + fprintf(fout, "avg credit:%d\n",
> + (int)(qstats.sum_credit /
> + (1500 * ((int)qstats.pkts_total ) + 1)));
>
> // Return values stats
> for (k = 0; k < RET_VAL_COUNT; k++) {
> @@ -408,6 +414,7 @@ static void Usage(void)
> " Where:\n"
> " -o indicates egress direction (default)\n"
> " -d print BPF trace debug buffer\n"
> + " --edt use fq's Earliest Departure Time\n"
> " -l also limit flows using loopback\n"
> " -n <#> to create cgroup \"/hbm#\" and attach prog\n"
> " Default is /hbm1\n"
> @@ -433,6 +440,7 @@ int main(int argc, char **argv)
> char *optstring = "iodln:r:st:wh";
> struct option loptions[] = {
> {"no_cn", 0, NULL, 1},
> + {"edt", 0, NULL, 2},
> {NULL, 0, NULL, 0}
> };
>
> @@ -441,6 +449,10 @@ int main(int argc, char **argv)
> case 1:
> no_cn_flag = true;
> break;
> + case 2:
> + prog = "hbm_edt_kern.o";
> + edt_flag = true;
> + break;
> case'o':
> break;
> case 'd':
> diff --git a/samples/bpf/hbm_edt_kern.c b/samples/bpf/hbm_edt_kern.c
> new file mode 100644
> index 000000000000..004a44a83e1e
> --- /dev/null
> +++ b/samples/bpf/hbm_edt_kern.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * Sample Host Bandwidth Manager (HBM) BPF program.
> + *
> + * A cgroup skb BPF egress program to limit cgroup output bandwidth.
> + * It uses a modified virtual token bucket queue to limit average
> + * egress bandwidth. The implementation uses credits instead of tokens.
> + * Negative credits imply that queueing would have happened (this is
> + * a virtual queue, so no queueing is done by it. However, queueing may
> + * occur at the actual qdisc (which is not used for rate limiting).
> + *
> + * This implementation uses 3 thresholds, one to start marking packets and
> + * the other two to drop packets:
> + * CREDIT
> + * - <--------------------------|------------------------> +
> + * | | | 0
> + * | Large pkt |
> + * | drop thresh |
> + * Small pkt drop Mark threshold
> + * thresh
> + *
> + * The effect of marking depends on the type of packet:
> + * a) If the packet is ECN enabled and it is a TCP packet, then the packet
> + * is ECN marked.
> + * b) If the packet is a TCP packet, then we probabilistically call tcp_cwr
> + * to reduce the congestion window. The current implementation uses a linear
> + * distribution (0% probability at marking threshold, 100% probability
> + * at drop threshold).
> + * c) If the packet is not a TCP packet, then it is dropped.
> + *
> + * If the credit is below the drop threshold, the packet is dropped. If it
> + * is a TCP packet, then it also calls tcp_cwr since packets dropped by
> + * by a cgroup skb BPF program do not automatically trigger a call to
> + * tcp_cwr in the current kernel code.
> + *
> + * This BPF program actually uses 2 drop thresholds, one threshold
> + * for larger packets (>= 120 bytes) and another for smaller packets. This
> + * protects smaller packets such as SYNs, ACKs, etc.
> + *
> + * The default bandwidth limit is set at 1Gbps but this can be changed by
> + * a user program through a shared BPF map. In addition, by default this BPF
> + * program does not limit connections using loopback. This behavior can be
> + * overwritten by the user program. There is also an option to calculate
> + * some statistics, such as percent of packets marked or dropped, which
> + * a user program, such as hbm, can access.
> + */
> +
> +#include "hbm_kern.h"
> +
> +SEC("cgroup_skb/egress")
> +int _hbm_out_cg(struct __sk_buff *skb)
> +{
> + signed long long delta = 0, delta_send;
"signed" is not needed here.
> + unsigned long long curtime, sendtime;
> + struct hbm_queue_stats *qsp = NULL;
> + unsigned int queue_index = 0;
> + bool congestion_flag = false;
> + bool ecn_ce_flag = false;
> + struct hbm_pkt_info pkti;
> + struct hbm_vqueue *qdp;
> + bool drop_flag = false;
> + bool cwr_flag = false;
> + int len = skb->len;
> + int rv = ALLOW_PKT;
> +
> + qsp = bpf_map_lookup_elem(&queue_stats, &queue_index);
> + if (qsp != NULL && !qsp->loopback && (skb->ifindex == 1))
The skb->ifindex = 1 is skipped. Why? Some comments will
be helpful to understand.
> + return ALLOW_PKT;
> +
> + hbm_get_pkt_info(skb, &pkti);
> +
> + // We may want to account for the length of headers in len
> + // calculation, like ETH header + overhead, specially if it
> + // is a gso packet. But I am not doing it right now.
> +
> + qdp = bpf_get_local_storage(&queue_state, 0);
> + if (!qdp)
> + return ALLOW_PKT;
> + if (qdp->lasttime == 0)
> + hbm_init_edt_vqueue(qdp, 1024);
> +
> + curtime = bpf_ktime_get_ns();
> +
> + // Begin critical section
> + bpf_spin_lock(&qdp->lock);
> + delta = qdp->lasttime - curtime;
> + // bound bursts to 100us
> + if (delta < -BURST_SIZE_NS) {
> + // negative delta is a credit that allows bursts
> + qdp->lasttime = curtime - BURST_SIZE_NS;
> + delta = -BURST_SIZE_NS;
> + }
> + sendtime = qdp->lasttime;
> + delta_send = BYTES_TO_NS(len, qdp->rate);
> + qdp->lasttime += delta_send;
> + bpf_spin_unlock(&qdp->lock);
> + // End critical section
> +
> + // Set EDT of packet
> + skb->tstamp = sendtime;
> +
> + // Check if we should update rate
> + if (qsp != NULL && (qsp->rate * 128) != qdp->rate) {
> + qdp->rate = qsp->rate * 128;
> + bpf_printk("Updating rate: %d (1sec:%llu bits)\n",
> + (int)qdp->rate,
> + CREDIT_PER_NS(1000000000, qdp->rate) * 8);
How often this bpf_printk will fire at runtime?
If it fires relatively frequently, maybe worth to put it
under debug?
> + }
> +
> + // Set flags (drop, congestion, cwr)
> + // last packet will be sent in the future, bound latency
> + if (delta > DROP_THRESH_NS || (delta > LARGE_PKT_DROP_THRESH_NS &&
> + len > LARGE_PKT_THRESH)) {
> + drop_flag = true;
> + if (pkti.is_tcp && pkti.ecn == 0)
> + cwr_flag = true;
> + } else if (delta > MARK_THRESH_NS) {
> + if (pkti.is_tcp)
> + congestion_flag = true;
> + else
> + drop_flag = true;
> + }
> +
> + if (congestion_flag) {
> + if (bpf_skb_ecn_set_ce(skb)) {
> + ecn_ce_flag = true;
> + } else {
> + if (pkti.is_tcp) {
> + unsigned int rand = bpf_get_prandom_u32();
> +
> + if (delta >= MARK_THRESH_NS +
> + (rand % MARK_REGION_SIZE_NS)) {
> + // Do congestion control
> + cwr_flag = true;
> + }
> + } else if (len > LARGE_PKT_THRESH) {
> + // Problem if too many small packets?
> + drop_flag = true;
> + congestion_flag = false;
> + }
> + }
> + }
> +
> + if (pkti.is_tcp && drop_flag && pkti.packets_out <= 1) {
> + drop_flag = false;
> + cwr_flag = true;
> + congestion_flag = false;
> + }
> +
> + if (qsp != NULL && qsp->no_cn)
> + cwr_flag = false;
> +
> + hbm_update_stats(qsp, len, curtime, congestion_flag, drop_flag,
> + cwr_flag, ecn_ce_flag, &pkti, (int) delta);
> +
> + if (drop_flag) {
> + __sync_add_and_fetch(&(qdp->credit), len);
> + __sync_add_and_fetch(&(qdp->lasttime), -delta_send);
We have race here. Updating qdp->lasttime may happen concurrently with
the update in bpf_spin_lock region. Do we have any issue here?
> + rv = DROP_PKT;
> + }
> +
> + if (cwr_flag)
> + rv |= CWR;
> + return rv;
> +}
> +char _license[] SEC("license") = "GPL";
> diff --git a/samples/bpf/hbm_kern.h b/samples/bpf/hbm_kern.h
> index be19cf1d5cd5..aa207a2eebbd 100644
> --- a/samples/bpf/hbm_kern.h
> +++ b/samples/bpf/hbm_kern.h
> @@ -29,6 +29,7 @@
> #define DROP_PKT 0
> #define ALLOW_PKT 1
> #define TCP_ECN_OK 1
> +#define CWR 2
>
> #ifndef HBM_DEBUG // Define HBM_DEBUG to enable debugging
> #undef bpf_printk
> @@ -45,8 +46,18 @@
> #define MAX_CREDIT (100 * MAX_BYTES_PER_PACKET)
> #define INIT_CREDIT (INITIAL_CREDIT_PACKETS * MAX_BYTES_PER_PACKET)
>
> +// Time base accounting for fq's EDT
> +#define BURST_SIZE_NS 100000 // 100us
> +#define MARK_THRESH_NS 50000 // 50us
> +#define DROP_THRESH_NS 500000 // 500us
> +// Reserve 20us of queuing for small packets (less than 120 bytes)
> +#define LARGE_PKT_DROP_THRESH_NS (DROP_THRESH_NS - 20000)
> +#define MARK_REGION_SIZE_NS (LARGE_PKT_DROP_THRESH_NS - MARK_THRESH_NS)
> +
> // rate in bytes per ns << 20
> #define CREDIT_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
> +#define BYTES_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
> +#define BYTES_TO_NS(bytes, rate) div64_u64(((u64)(bytes)) << 20, (u64)(rate))
>
> struct bpf_map_def SEC("maps") queue_state = {
> .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> @@ -67,6 +78,7 @@ BPF_ANNOTATE_KV_PAIR(queue_stats, int, struct hbm_queue_stats);
> struct hbm_pkt_info {
> int cwnd;
> int rtt;
> + int packets_out;
> bool is_ip;
> bool is_tcp;
> short ecn;
> @@ -86,16 +98,20 @@ static int get_tcp_info(struct __sk_buff *skb, struct hbm_pkt_info *pkti)
> if (tp) {
> pkti->cwnd = tp->snd_cwnd;
> pkti->rtt = tp->srtt_us >> 3;
> + pkti->packets_out = tp->packets_out;
> return 0;
> }
> }
> }
> }
> + pkti->cwnd = 0;
> + pkti->rtt = 0;
> + pkti->packets_out = 0;
> return 1;
> }
>
> -static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
> - struct hbm_pkt_info *pkti)
> +static void hbm_get_pkt_info(struct __sk_buff *skb,
> + struct hbm_pkt_info *pkti)
> {
> struct iphdr iph;
> struct ipv6hdr *ip6h;
> @@ -123,10 +139,22 @@ static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
>
> static __always_inline void hbm_init_vqueue(struct hbm_vqueue *qdp, int rate)
> {
> - bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> - qdp->lasttime = bpf_ktime_get_ns();
> - qdp->credit = INIT_CREDIT;
> - qdp->rate = rate * 128;
> + bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> + qdp->lasttime = bpf_ktime_get_ns();
> + qdp->credit = INIT_CREDIT;
> + qdp->rate = rate * 128;
> +}
> +
> +static __always_inline void hbm_init_edt_vqueue(struct hbm_vqueue *qdp,
> + int rate)
> +{
> + unsigned long long curtime;
> +
> + curtime = bpf_ktime_get_ns();
> + bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> + qdp->lasttime = curtime - BURST_SIZE_NS; // support initial burst
> + qdp->credit = 0; // not used
> + qdp->rate = rate * 128;
> }
>
> static __always_inline void hbm_update_stats(struct hbm_queue_stats *qsp,
>
^ 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