* Re: [PATCH v5 0/5] M_CAN Framework re-write
From: Wolfgang Grandegger @ 2019-02-22 9:38 UTC (permalink / raw)
To: Dan Murphy, mkl, davem; +Cc: linux-can, netdev, linux-kernel
In-Reply-To: <43e01edf-9b9a-c68d-ec1a-218a8117f59e@grandegger.com>
Hello Dan,
what kernel version is that patch series for. I have problems to apply it!
Wolfgang.
Am 21.02.19 um 17:41 schrieb Wolfgang Grandegger:
> Hello Dan,
>
> I will have a closer look end of this week!
>
> Wolfgang.
>
> Am 21.02.19 um 17:24 schrieb Dan Murphy:
>> Bump
>>
>> On 2/14/19 12:27 PM, Dan Murphy wrote:
>>> Hello
>>>
>>> OK I did not give up on this patch series just got a little preoccupied with
>>> some other kernel work. But here is the update per the comments.
>>>
>>> It should be understood I broke these out for reviewability.
>>> For instance the first patch does not compile on its own as including this
>>> patch should not change the current functionality and it pulls all the io-mapped
>>> code from the m_can base file to a platfrom file.
>>>
>>> The next patch "Migrate the m_can code to use the framework"
>>> is the change to the kernel for the io-mapped conversion from a flat file to use
>>> the framework. Finally the rename patch just renames the m_can_priv to
>>> m_can_classdev. I broke this change out specifically for readability of the
>>> migration patch per comments on the code.
>>>
>>> AFAIC the first 3 patches can all be squashed into a single patch. Or the
>>> first 2 patches in the series can be re-arranged but then m_can functionality is
>>> affected in the migration patch.
>>>
>>> Again the first 3 patches here are all just for readability and review purposes.
>>>
>>> Dan
>>>
>>> Dan Murphy (5):
>>> can: m_can: Create a m_can platform framework
>>> can: m_can: Migrate the m_can code to use the framework
>>> can: m_can: Rename m_can_priv to m_can_classdev
>>> dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver
>>> can: tcan4x5x: Add tcan4x5x driver to the kernel
>>>
>>> .../devicetree/bindings/net/can/tcan4x5x.txt | 37 +
>>> drivers/net/can/m_can/Kconfig | 14 +-
>>> drivers/net/can/m_can/Makefile | 2 +
>>> drivers/net/can/m_can/m_can.c | 788 +++++++++---------
>>> drivers/net/can/m_can/m_can.h | 159 ++++
>>> drivers/net/can/m_can/m_can_platform.c | 198 +++++
>>> drivers/net/can/m_can/tcan4x5x.c | 531 ++++++++++++
>>> 7 files changed, 1320 insertions(+), 409 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
>>> create mode 100644 drivers/net/can/m_can/m_can.h
>>> create mode 100644 drivers/net/can/m_can/m_can_platform.c
>>> create mode 100644 drivers/net/can/m_can/tcan4x5x.c
>>>
>>
>>
^ permalink raw reply
* [PATCH] cfg80211: reg: Fix use-after-free in call_crda
From: Yue Haibing @ 2019-02-22 9:42 UTC (permalink / raw)
To: johannes, davem; +Cc: linux-kernel, netdev, linux-wireless, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
KASAN report this:
BUG: KASAN: use-after-free in kobject_uevent_env+0xedb/0xf20 lib/kobject_uevent.c:474
Read of size 8 at addr ffff8881e52d5dc0 by task kworker/0:2/1066
CPU: 0 PID: 1066 Comm: kworker/0:2 Not tainted 5.0.0-rc7+ #45
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Workqueue: events reg_todo [cfg80211]
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xfa/0x1ce lib/dump_stack.c:113
print_address_description+0x65/0x270 mm/kasan/report.c:187
kasan_report+0x149/0x18d mm/kasan/report.c:317
kobject_uevent_env+0xedb/0xf20 lib/kobject_uevent.c:474
reg_query_database+0x23e/0x390 [cfg80211]
reg_process_hint+0x34a/0xc00 [cfg80211]
reg_todo+0x3f0/0xa80 [cfg80211]
process_one_work+0xc4d/0x1b40 kernel/workqueue.c:2173
worker_thread+0x171/0x1130 kernel/workqueue.c:2319
kthread+0x302/0x3c0 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
Allocated by task 3500:
set_track mm/kasan/common.c:85 [inline]
__kasan_kmalloc.constprop.3+0xa0/0xd0 mm/kasan/common.c:496
kmalloc include/linux/slab.h:550 [inline]
kzalloc include/linux/slab.h:740 [inline]
platform_device_alloc+0x2a/0xf0 drivers/base/platform.c:268
platform_device_register_full+0x6d/0x4d0 drivers/base/platform.c:509
regulatory_init+0x13c/0x5e0 [cfg80211]
cfg80211_init+0x7a/0x10f [cfg80211]
do_one_initcall+0xfa/0x5ca init/main.c:887
do_init_module+0x204/0x5f6 kernel/module.c:3460
load_module+0x66b2/0x8570 kernel/module.c:3808
__do_sys_finit_module+0x238/0x2a0 kernel/module.c:3902
do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 2957:
set_track mm/kasan/common.c:85 [inline]
__kasan_slab_free+0x130/0x180 mm/kasan/common.c:458
slab_free_hook mm/slub.c:1409 [inline]
slab_free_freelist_hook mm/slub.c:1436 [inline]
slab_free mm/slub.c:2986 [inline]
kfree+0xe1/0x270 mm/slub.c:3938
device_release+0x78/0x200 drivers/base/core.c:919
kobject_cleanup lib/kobject.c:662 [inline]
kobject_release lib/kobject.c:691 [inline]
kref_put include/linux/kref.h:67 [inline]
kobject_put+0x146/0x240 lib/kobject.c:708
put_device+0x1c/0x30 drivers/base/core.c:2060
request_firmware_work_func+0x16d/0x290 drivers/base/firmware_loader/main.c:786
process_one_work+0xc4d/0x1b40 kernel/workqueue.c:2173
worker_thread+0x171/0x1130 kernel/workqueue.c:2319
kthread+0x302/0x3c0 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
The buggy address belongs to the object at ffff8881e52d5d80
which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 64 bytes inside of
2048-byte region [ffff8881e52d5d80, ffff8881e52d6580)
The buggy address belongs to the page:
page:ffffea000794b400 count:1 mapcount:0 mapping:ffff8881f6c02800 index:0xffff8881e52d3300 compound_mapcount: 0
flags: 0x2fffc0000010200(slab|head)
raw: 02fffc0000010200 ffffea00079d5200 0000000400000004 ffff8881f6c02800
raw: ffff8881e52d3300 00000000800f000e 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8881e52d5c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff8881e52d5d00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff8881e52d5d80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8881e52d5e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8881e52d5e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
In function reg_query_database, query_regdb_file call
request_firmware_nowait to do request_firmware asynchronously,
which need the caller hold the reference of dev, otherwise it will
do put_device freeing '®_pdev->dev'. After that, call_crda access
the dev will trigger use-after-free bug.
This patch fix this by holding a reference of dev in regulatory_init
after platform_device_register_simple registered successly, which
releasing in platform_device_unregister.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 007f6c5e6eb4 ("cfg80211: support loading regulatory database as firmware file")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/wireless/reg.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index adfa58f..3de568f 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -3884,6 +3884,8 @@ int __init regulatory_init(void)
if (IS_ERR(reg_pdev))
return PTR_ERR(reg_pdev);
+ get_device(®_pdev->dev);
+
spin_lock_init(®_requests_lock);
spin_lock_init(®_pending_beacons_lock);
spin_lock_init(®_indoor_lock);
--
2.7.4
^ permalink raw reply related
* [PATCH][net-next] ipv6: sanitize RCU usage on fib6_next
From: Li RongQing @ 2019-02-22 9:44 UTC (permalink / raw)
To: netdev
using rcu_assign_pointer when setting, which has a memory
barrier to ensure the initialization is seen first.
using rcu_dereference when dereference this pointer
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
net/ipv6/ip6_fib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 6613d8dbb0e5..b73d40d68178 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1143,7 +1143,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
atomic_inc(&rt->fib6_ref);
rcu_assign_pointer(rt->fib6_node, fn);
- rt->fib6_next = iter->fib6_next;
+ rcu_assign_pointer(rt->fib6_next, iter->fib6_next);
rcu_assign_pointer(*ins, rt);
if (!info->skip_notify)
inet6_rt_notify(RTM_NEWROUTE, rt, info, NLM_F_REPLACE);
@@ -1761,7 +1761,9 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
RT6_TRACE("fib6_del_route\n");
/* Unlink it */
- *rtp = rt->fib6_next;
+ *rtp = rcu_dereference_protected(rt->fib6_next,
+ lockdep_is_held(&rt->fib6_table->tb6_lock));
+
rt->fib6_node = NULL;
net->ipv6.rt6_stats->fib_rt_entries--;
net->ipv6.rt6_stats->fib_discarded_routes++;
--
2.16.2
^ permalink raw reply related
* Re: [PATCH net-next 2/2] xdp: Add devmap_idx map type for looking up devices by ifindex
From: Toke Høiland-Jørgensen @ 2019-02-22 9:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, netdev, Jesper Dangaard Brouer, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20190221163218.72905325@cakuba.netronome.com>
Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> On Fri, 22 Feb 2019 00:02:23 +0100, Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
>>
>> > On Thu, 21 Feb 2019 12:56:54 +0100, Toke Høiland-Jørgensen wrote:
>> >> A common pattern when using xdp_redirect_map() is to create a device map
>> >> where the lookup key is simply ifindex. Because device maps are arrays,
>> >> this leaves holes in the map, and the map has to be sized to fit the
>> >> largest ifindex, regardless of how many devices actually are actually
>> >> needed in the map.
>> >>
>> >> This patch adds a second type of device map where the key is interpreted as
>> >> an ifindex and looked up using a hashmap, instead of being used as an array
>> >> index. This leads to maps being densely packed, so they can be smaller.
>> >>
>> >> The default maps used by xdp_redirect() are changed to use the new map
>> >> type, which means that xdp_redirect() is no longer limited to ifindex < 64,
>> >> but instead to 64 total simultaneous interfaces per network namespace. This
>> >> also provides an easy way to compare the performance of devmap and
>> >> devmap_idx:
>> >>
>> >> xdp_redirect_map (devmap): 8394560 pkt/s
>> >> xdp_redirect (devmap_idx): 8179480 pkt/s
>> >>
>> >> Difference: 215080 pkt/s or 3.1 nanoseconds per packet.
>> >
>> > Could you share what the ifindex mix was here, to arrive at these
>> > numbers? How does it compare to using an array but not keying with
>> > ifindex?
>>
>> Just the standard set on my test machine; ifindex 1 through 9, except 8
>> in this case. So certainly no more than 1 ifindex in each hash bucket
>> for those numbers.
>
> Oh, I clearly misread your numbers, it's still slower than array, you
> just don't need the size limit.
Yeah, this is not about speeding up devmap, it's about lifting the size
restriction.
>> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> >> +static int dev_map_idx_update_elem(struct bpf_map *map, void *key, void *value,
>> >> + u64 map_flags)
>> >> +{
>> >> + struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
>> >> + struct bpf_dtab_netdev *dev, *old_dev;
>> >> + u32 idx = *(u32 *)key;
>> >> + u32 val = *(u32 *)value;
>> >> + u32 bit;
>> >> +
>> >> + if (unlikely(map_flags > BPF_EXIST))
>> >> + return -EINVAL;
>> >> + if (unlikely(map_flags == BPF_NOEXIST))
>> >> + return -EEXIST;
>> >> +
>> >> + old_dev = __dev_map_idx_lookup_elem(map, idx);
>> >> + if (!val) {
>> >> + if (!old_dev)
>> >> + return 0;
>> >
>> > IMHO this is a fairly strange mix of array and hashmap semantics. I
>> > think you should stick to hashmap behaviour AFA flags and
>> > update/delete goes.
>>
>> Yeah, the double book-keeping is a bit strange, but it allows the actual
>> forwarding and flush code to be reused between both types of maps. I
>> think this is worth the slight semantic confusion :)
>
> I'm not sure I was clear, let me try again :) Your get_next_key only
> reports existing indexes if I read the code right, so that's not an
> array - in an array indexes always exist. What follows inserting 0
> should not be equivalent to delete and BPF_NOEXIST should be handled
> appropriately.
Ah, I see what you mean. Yeah, sure, I guess I can restrict deletion to
only working through explicit delete.
I could also add a fail on NOEXIST, but since each index is tied to a
particular value, you can't actually change the contents of each index,
only insert and remove. So why would you ever set that flag?
> Different maps behave differently, I think it's worth trying to limit
> the divergence in how things behave to the basic array and a hashmap
> models when possible.
So I don't actually think of this as a hashmap in the general sense;
after all, you can only store ifindexes in it, and key and value are
tied to one another. So it's an ifindex'ed devmap (which is also why I
named it devmap_idx and not devmap_hash); the fact that it's implemented
as a hashmap is just incidental.
So I guess it's a choice between being consistent with the other devmap
type, or with a general hashmap. I'm not actually sure that the latter
is less surprising? :)
-Toke
^ permalink raw reply
* [PATCH 0/3] Allow checking SYN cookies from XDP and tc cls act
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
This series adds a helper bpf_sk_check_syncookie, which allows checking
whether a packet is a valid SYN cookie for a given socket. This is
useful for implementing load-balancing approaches like glb-director [1]
or Beamer [2] in pure eBPF.
Specifically, we'd like to replace the functionality of the glb-redirect
kernel module by an XDP program or tc classifier.
1: https://github.com/github/glb-director
2: https://www.usenix.org/conference/nsdi18/presentation/olteanu
Lorenz Bauer (3):
bpf: add helper to check for a valid SYN cookie
tools: sync changes to uapi/linux/bpf.h
selftests/bpf: add tests for bpf_sk_check_syncookie
include/uapi/linux/bpf.h | 18 +-
net/core/filter.c | 68 ++++++
tools/include/uapi/linux/bpf.h | 18 +-
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 5 +-
tools/testing/selftests/bpf/bpf_helpers.h | 3 +
.../bpf/progs/test_sk_syncookie_kern.c | 119 ++++++++++
.../selftests/bpf/test_sk_syncookie.sh | 81 +++++++
.../selftests/bpf/test_sk_syncookie_user.c | 212 ++++++++++++++++++
9 files changed, 521 insertions(+), 4 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c
--
2.19.1
^ permalink raw reply
* [PATCH 1/3] bpf: add helper to check for a valid SYN cookie
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
to a known connection. However, there is one corner case: no sockets are
created if SYN cookies are active. This means that the final ACK in the
3WHS is misclassified.
Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
and then check whether a packet is a valid SYN cookie ACK.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 18 ++++++++++-
net/core/filter.c | 68 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * Description
+ * Check whether iph and th contain a valid SYN cookie ACK for
+ * the listening socket in sk.
+ *
+ * iph points to the start of the IPv4 or IPv6 header, while
+ * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * th points to the start of the TCP header, while th_len contains
+ * sizeof(struct tcphdr).
+ *
+ * Return
+ * 0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * otherwise.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2472,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(sk_check_syncookie),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 85749f6ec789..9e68897cc7ed 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
.arg1_type = ARG_PTR_TO_SOCK_COMMON,
};
+BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
+ struct tcphdr *, th, u32, th_len)
+{
+#if IS_ENABLED(CONFIG_SYN_COOKIES)
+ u32 cookie;
+ int ret;
+
+ if (unlikely(th_len < sizeof(*th)))
+ return -EINVAL;
+
+ /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
+ if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
+ return -EINVAL;
+
+ if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
+ return -EINVAL;
+
+ if (!th->ack || th->rst)
+ return -ENOENT;
+
+ cookie = ntohl(th->ack_seq) - 1;
+
+ switch (sk->sk_family) {
+ case AF_INET:
+ if (unlikely(iph_len < sizeof(struct iphdr)))
+ return -EINVAL;
+
+ ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
+ break;
+
+#if IS_ENABLED(CONFIG_IPV6)
+ case AF_INET6:
+ if (unlikely(iph_len < sizeof(struct ipv6hdr)))
+ return -EINVAL;
+
+ ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
+ break;
+#endif /* CONFIG_IPV6 */
+
+ default:
+ return -EPROTONOSUPPORT;
+ }
+
+ if (ret > 0)
+ return 0;
+
+ return -ENOENT;
+#else
+ return -ENOTSUP;
+#endif
+}
+
+static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
+ .func = bpf_sk_check_syncookie,
+ .gpl_only = true,
+ .pkt_access = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_SOCKET,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_PTR_TO_MEM,
+ .arg5_type = ARG_CONST_SIZE,
+};
+
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
@@ -5678,6 +5742,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_release_proto;
case BPF_FUNC_tcp_sock:
return &bpf_tcp_sock_proto;
+ case BPF_FUNC_sk_check_syncookie:
+ return &bpf_sk_check_syncookie_proto;
#endif
default:
return bpf_base_func_proto(func_id);
@@ -5713,6 +5779,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_xdp_sk_lookup_tcp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+ case BPF_FUNC_sk_check_syncookie:
+ return &bpf_sk_check_syncookie_proto;
#endif
default:
return bpf_base_func_proto(func_id);
--
2.19.1
^ permalink raw reply related
* [PATCH 2/3] tools: sync changes to uapi/linux/bpf.h
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Pull in definitions for bpf_sk_check_syncookie.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
* Return
* A **struct bpf_tcp_sock** pointer on success, or NULL in
* case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * Description
+ * Check whether iph and th contain a valid SYN cookie ACK for
+ * the listening socket in sk.
+ *
+ * iph points to the start of the IPv4 or IPv6 header, while
+ * iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * th points to the start of the TCP header, while th_len contains
+ * sizeof(struct tcphdr).
+ *
+ * Return
+ * 0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * otherwise.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2457,7 +2472,8 @@ union bpf_attr {
FN(spin_lock), \
FN(spin_unlock), \
FN(sk_fullsock), \
- FN(tcp_sock),
+ FN(tcp_sock), \
+ FN(sk_check_syncookie),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
--
2.19.1
^ permalink raw reply related
* [PATCH 3/3] selftests/bpf: add tests for bpf_sk_check_syncookie
From: Lorenz Bauer @ 2019-02-22 9:50 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: linux-api, Lorenz Bauer
In-Reply-To: <20190222095057.9442-1-lmb@cloudflare.com>
Add tests which verify that the new helper works for both IPv4 and
IPv6, by forcing SYN cookies to always on. Use a new network namespace
to avoid clobbering the global SYN cookie settings.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 5 +-
tools/testing/selftests/bpf/bpf_helpers.h | 3 +
.../bpf/progs/test_sk_syncookie_kern.c | 119 ++++++++++
.../selftests/bpf/test_sk_syncookie.sh | 81 +++++++
.../selftests/bpf/test_sk_syncookie_user.c | 212 ++++++++++++++++++
6 files changed, 419 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..44b2853b8534 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -29,4 +29,5 @@ test_netcnt
test_section_names
test_tcpnotify_user
test_libbpf
+test_sk_syncookie_user
alu32
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ccffaa0a0787..4ad1f7a915b3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -51,7 +51,8 @@ TEST_PROGS := test_kmod.sh \
test_skb_cgroup_id.sh \
test_flow_dissector.sh \
test_xdp_vlan.sh \
- test_lwt_ip_encap.sh
+ test_lwt_ip_encap.sh \
+ test_sk_syncookie.sh
TEST_PROGS_EXTENDED := with_addr.sh \
with_tunnels.sh \
@@ -60,7 +61,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
# Compile but not part of 'make run_tests'
TEST_GEN_PROGS_EXTENDED = test_libbpf_open test_sock_addr test_skb_cgroup_id_user \
- flow_dissector_load test_flow_dissector
+ flow_dissector_load test_flow_dissector test_sk_syncookie_user
include ../lib.mk
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index d9999f1ed1d2..b80cec7af445 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -166,6 +166,9 @@ static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
(void *) BPF_FUNC_sk_lookup_udp;
static int (*bpf_sk_release)(struct bpf_sock *sk) =
(void *) BPF_FUNC_sk_release;
+static int (*bpf_sk_check_syncookie)(struct bpf_sock *sk,
+ void *ip, int ip_len, void *tcp, int tcp_len) =
+ (void *) BPF_FUNC_sk_check_syncookie;
static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
(void *) BPF_FUNC_skb_vlan_push;
static int (*bpf_skb_vlan_pop)(void *ctx) =
diff --git a/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
new file mode 100644
index 000000000000..4918457efc1e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+
+#include <linux/bpf.h>
+#include <linux/pkt_cls.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <sys/socket.h>
+#include <linux/tcp.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+struct bpf_map_def SEC("maps") results = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(__u32),
+ .value_size = sizeof(__u64),
+ .max_entries = 1,
+};
+
+static __always_inline void check_syncookie(void *ctx, void *data,
+ void *data_end)
+{
+ struct bpf_sock_tuple tup;
+ struct bpf_sock *sk;
+ struct ethhdr *ethh;
+ struct iphdr *ipv4h;
+ struct ipv6hdr *ipv6h;
+ struct tcphdr *tcph;
+ int ret;
+ __u32 key = 0;
+ __u64 value = 1;
+
+ ethh = data;
+ if (ethh + 1 > data_end)
+ return;
+
+ switch (bpf_ntohs(ethh->h_proto)) {
+ case ETH_P_IP:
+ ipv4h = data + sizeof(struct ethhdr);
+ if (ipv4h + 1 > data_end)
+ return;
+
+ tcph = data + sizeof(struct ethhdr) + sizeof(struct iphdr);
+ if (tcph + 1 > data_end)
+ return;
+
+ tup.ipv4.saddr = ipv4h->saddr;
+ tup.ipv4.daddr = ipv4h->daddr;
+ tup.ipv4.sport = tcph->source;
+ tup.ipv4.dport = tcph->dest;
+
+ sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv4),
+ BPF_F_CURRENT_NETNS, 0);
+ if (!sk)
+ return;
+
+ ret = bpf_sk_check_syncookie(sk, ipv4h, sizeof(*ipv4h),
+ tcph, sizeof(*tcph));
+ break;
+
+ case ETH_P_IPV6:
+ ipv6h = data + sizeof(struct ethhdr);
+ if (ipv6h + 1 > data_end)
+ return;
+
+ if (ipv6h->nexthdr != IPPROTO_TCP)
+ return;
+
+ tcph = data + sizeof(struct ethhdr) + sizeof(struct ipv6hdr);
+ if (tcph + 1 > data_end)
+ return;
+
+ memcpy(tup.ipv6.saddr, &ipv6h->saddr, sizeof(tup.ipv6.saddr));
+ memcpy(tup.ipv6.daddr, &ipv6h->daddr, sizeof(tup.ipv6.daddr));
+ tup.ipv6.sport = tcph->source;
+ tup.ipv6.dport = tcph->dest;
+
+ sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv6),
+ BPF_F_CURRENT_NETNS, 0);
+ if (!sk)
+ return;
+
+ ret = bpf_sk_check_syncookie(sk, ipv6h, sizeof(*ipv6h),
+ tcph, sizeof(*tcph));
+ break;
+
+ default:
+ return;
+ }
+
+ if (ret == 0)
+ bpf_map_update_elem(&results, &key, &value, 0);
+
+ bpf_sk_release(sk);
+}
+
+SEC("clsact/check_syncookie")
+int check_syncookie_clsact(struct __sk_buff *skb)
+{
+ check_syncookie(skb, (void *)(long)skb->data,
+ (void *)(long)skb->data_end);
+ return TC_ACT_OK;
+}
+
+SEC("xdp/check_syncookie")
+int check_syncookie_xdp(struct xdp_md *ctx)
+{
+ check_syncookie(ctx, (void *)(long)ctx->data,
+ (void *)(long)ctx->data_end);
+ return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie.sh b/tools/testing/selftests/bpf/test_sk_syncookie.sh
new file mode 100755
index 000000000000..429ca8c04c5e
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Facebook
+# Copyright (c) 2019 Cloudflare
+
+set -eu
+
+wait_for_ip()
+{
+ local _i
+ printf "Wait for IP %s to become available " "$1"
+ for _i in $(seq ${MAX_PING_TRIES}); do
+ printf "."
+ if ns1_exec ping -c 1 -W 1 "$1" >/dev/null 2>&1; then
+ echo " OK"
+ return
+ fi
+ sleep 1
+ done
+ echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
+ exit 1
+}
+
+get_prog_id()
+{
+ awk '/ id / {sub(/.* id /, "", $0); print($1)}'
+}
+
+ns1_exec()
+{
+ ip netns exec ns1 "$@"
+}
+
+setup()
+{
+ ip netns add ns1
+ ns1_exec ip link set lo up
+
+ ns1_exec sysctl -w net.ipv4.tcp_syncookies=2
+
+ wait_for_ip 127.0.0.1
+ wait_for_ip ::1
+}
+
+cleanup()
+{
+ ip netns del ns1 2>/dev/null || :
+}
+
+main()
+{
+ trap cleanup EXIT 2 3 6 15
+ setup
+
+ printf "Testing clsact..."
+ ns1_exec tc qdisc add dev "${TEST_IF}" clsact
+ ns1_exec tc filter add dev "${TEST_IF}" ingress \
+ bpf obj "${BPF_PROG_OBJ}" sec "${CLSACT_SECTION}" da
+
+ BPF_PROG_ID=$(ns1_exec tc filter show dev "${TEST_IF}" ingress | \
+ get_prog_id)
+ ns1_exec "${PROG}" "${BPF_PROG_ID}"
+ ns1_exec tc qdisc del dev "${TEST_IF}" clsact
+
+ printf "Testing XDP..."
+ ns1_exec ip link set "${TEST_IF}" xdp \
+ object "${BPF_PROG_OBJ}" section "${XDP_SECTION}"
+ BPF_PROG_ID=$(ns1_exec ip link show "${TEST_IF}" | get_prog_id)
+ ns1_exec "${PROG}" "${BPF_PROG_ID}"
+}
+
+DIR=$(dirname $0)
+TEST_IF=lo
+MAX_PING_TRIES=5
+BPF_PROG_OBJ="${DIR}/test_sk_syncookie_kern.o"
+CLSACT_SECTION="clsact/check_syncookie"
+XDP_SECTION="xdp/check_syncookie"
+BPF_PROG_ID=0
+PROG="${DIR}/test_sk_syncookie_user"
+
+main
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie_user.c b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
new file mode 100644
index 000000000000..87829c86c746
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "cgroup_helpers.h"
+
+static int start_server(const struct sockaddr *addr, socklen_t len)
+{
+ int fd;
+
+ fd = socket(addr->sa_family, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create server socket");
+ goto out;
+ }
+
+ if (bind(fd, addr, len) == -1) {
+ log_err("Failed to bind server socket");
+ goto close_out;
+ }
+
+ if (listen(fd, 128) == -1) {
+ log_err("Failed to listen on server socket");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = -1;
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ fd = socket(addr.ss_family, SOCK_STREAM, 0);
+ if (fd == -1) {
+ log_err("Failed to create client socket");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
+ log_err("Fail to connect to server");
+ goto close_out;
+ }
+
+ goto out;
+
+close_out:
+ close(fd);
+ fd = -1;
+out:
+ return fd;
+}
+
+static int get_map_fd_by_prog_id(int prog_id)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ __u32 map_ids[1];
+ int prog_fd = -1;
+ int map_fd = -1;
+
+ prog_fd = bpf_prog_get_fd_by_id(prog_id);
+ if (prog_fd < 0) {
+ log_err("Failed to get fd by prog id %d", prog_id);
+ goto err;
+ }
+
+ info.nr_map_ids = 1;
+ info.map_ids = (__u64)(unsigned long)map_ids;
+
+ if (bpf_obj_get_info_by_fd(prog_fd, &info, &info_len)) {
+ log_err("Failed to get info by prog fd %d", prog_fd);
+ goto err;
+ }
+
+ if (!info.nr_map_ids) {
+ log_err("No maps found for prog fd %d", prog_fd);
+ goto err;
+ }
+
+ map_fd = bpf_map_get_fd_by_id(map_ids[0]);
+ if (map_fd < 0)
+ log_err("Failed to get fd by map id %d", map_ids[0]);
+err:
+ if (prog_fd >= 0)
+ close(prog_fd);
+ return map_fd;
+}
+
+static int run_test(int server_fd, int results_fd)
+{
+ int client = -1, srv_client = -1;
+ int ret = 0;
+ __u32 key = 0;
+ __u64 value = 0;
+
+ if (bpf_map_update_elem(results_fd, &key, &value, 0) < 0) {
+ log_err("Can't clear results");
+ goto err;
+ }
+
+ client = connect_to_server(server_fd);
+ if (client == -1)
+ goto err;
+
+ srv_client = accept(server_fd, NULL, 0);
+ if (srv_client == -1) {
+ log_err("Can't accept connection");
+ goto err;
+ }
+
+ if (bpf_map_lookup_elem(results_fd, &key, &value) < 0) {
+ log_err("Can't lookup result");
+ goto err;
+ }
+
+ if (value != 1) {
+ log_err("Didn't match syncookie: %llu", value);
+ goto err;
+ }
+
+ goto out;
+
+err:
+ ret = 1;
+out:
+ close(client);
+ close(srv_client);
+ return ret;
+}
+
+int main(int argc, char **argv)
+{
+ struct sockaddr_in addr4;
+ struct sockaddr_in6 addr6;
+ int server = -1;
+ int server_v6 = -1;
+ int results = -1;
+ int err = 0;
+
+ if (argc < 2) {
+ fprintf(stderr, "Usage: %s prog_id\n", argv[0]);
+ exit(1);
+ }
+
+ results = get_map_fd_by_prog_id(atoi(argv[1]));
+ if (results < 0) {
+ log_err("Can't get map");
+ goto err;
+ }
+
+ memset(&addr4, 0, sizeof(addr4));
+ addr4.sin_family = AF_INET;
+ addr4.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ addr4.sin_port = 0;
+
+ memset(&addr6, 0, sizeof(addr6));
+ addr6.sin6_family = AF_INET6;
+ addr6.sin6_addr = in6addr_loopback;
+ addr6.sin6_port = 0;
+
+ server = start_server((const struct sockaddr *)&addr4, sizeof(addr4));
+ if (server == -1)
+ goto err;
+
+ server_v6 = start_server((const struct sockaddr *)&addr6,
+ sizeof(addr6));
+ if (server_v6 == -1)
+ goto err;
+
+ if (run_test(server, results))
+ goto err;
+
+ if (run_test(server_v6, results))
+ goto err;
+
+ printf("ok\n");
+ goto out;
+err:
+ err = 1;
+out:
+ close(server);
+ close(server_v6);
+ close(results);
+ return err;
+}
--
2.19.1
^ permalink raw reply related
* Re: [PATCH net-next 3/5] devlink: create a special NDO for getting the devlink instance
From: Michal Kubecek @ 2019-02-22 9:51 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, jiri, andrew, f.fainelli, netdev, oss-drivers
In-Reply-To: <20190221174620.12144-4-jakub.kicinski@netronome.com>
On Thu, Feb 21, 2019 at 09:46:18AM -0800, Jakub Kicinski wrote:
> Instead of iterating over all devlink ports add a NDO which
> will return the devlink instance from the driver.
>
> Suggested-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> include/linux/netdevice.h | 6 ++++
> net/core/devlink.c | 62 +++++++++++++--------------------------
> 2 files changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index aab4d9f6613d..eebcef6b8191 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -940,6 +940,8 @@ struct dev_ifalias {
> char ifalias[];
> };
>
> +struct devlink;
> +
> /*
> * This structure defines the management hooks for network devices.
> * The following hooks can be defined; unless noted otherwise, they are
> @@ -1248,6 +1250,9 @@ struct dev_ifalias {
> * that got dropped are freed/returned via xdp_return_frame().
> * Returns negative number, means general error invoking ndo, meaning
> * no frames were xmit'ed and core-caller will free all frames.
> + * struct devlink *(*ndo_get_devlink)(struct net_device *dev);
> + * Get devlink instance associated with a given netdev.
> + * Called with a reference on the device only, rtnl_lock is not held.
The formulation is a bit ambiguous. Do I understand correctly that what
it says is that device reference is sufficient and rtnl_lock is not
necessary (but there is no harm if caller holds rtnl_lock)?
Michal
^ permalink raw reply
* Re: [PATCH] sctp: don't compare hb_timer expire date before starting it
From: Sverdlin, Alexander (Nokia - DE/Ulm) @ 2019-02-22 10:03 UTC (permalink / raw)
To: kwiecienmaciek@gmail.com, linux-sctp@vger.kernel.org
Cc: netdev@vger.kernel.org, Kwiecien, Maciej (Nokia - PL/Wroclaw)
In-Reply-To: <20190222084526.8214-1-maciej.kwiecien@nokia.com>
On 22/02/2019 09:45, kwiecienmaciek@gmail.com wrote:
> From: Maciej Kwiecien <maciej.kwiecien@nokia.com>
>
> hb_timer might not start at all for a particular transport because its
> start is conditional. In a result a node is not sending heartbeats.
>
> Function sctp_transport_reset_hb_timer has two roles:
> - initial start of hb_timer for a given transport,
> - update expire date of hb_timer for a given transport.
> The function is optimized to update timer's expire only if it is before
> a new calculated one but this comparison is invalid for a timer which
> has not yet started. Such a timer has expire == 0 and if a new expire
> value is bigger than (MAX_JIFFIES / 2 + 2) then "time_before" macro will
> fail and timer will not start resulting in no heartbeat packets send by
> the node.
>
> This was found when association was initialized within first 5 mins
> after system boot due to jiffies init value which is near to MAX_JIFFIES.
>
> Test kernel version: 4.9.154 (ARCH=arm)
> hb_timer.expire = 0; //initialized, not started timer
> new_expire = MAX_JIFFIES / 2 + 2; //or more
> time_before(hb_timer.expire, new_expire) == false
>
> Fixes: ba6f5e33bdbb ("sctp: avoid refreshing heartbeat timer too often")
> Reported-by: Marcin Stojek <marcin.stojek@nokia.com>
> Tested-by: Marcin Stojek <marcin.stojek@nokia.com>
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Signed-off-by: Maciej Kwiecien <maciej.kwiecien@nokia.com>
> ---
> net/sctp/transport.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 033696e6f74f..ad158d311ffa 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -207,7 +207,8 @@ void sctp_transport_reset_hb_timer(struct sctp_transport *transport)
>
> /* When a data chunk is sent, reset the heartbeat interval. */
> expires = jiffies + sctp_transport_timeout(transport);
> - if (time_before(transport->hb_timer.expires, expires) &&
> + if ((time_before(transport->hb_timer.expires, expires) ||
> + !timer_pending(&transport->hb_timer)) &&
> !mod_timer(&transport->hb_timer,
> expires + prandom_u32_max(transport->rto)))
> sctp_transport_hold(transport);
--
Best regards,
Alexander Sverdlin.
^ permalink raw reply
* Re: [PATCH net-next 4/5] nfp: add .ndo_get_devlink
From: Michal Kubecek @ 2019-02-22 10:04 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, jiri, andrew, f.fainelli, netdev, oss-drivers
In-Reply-To: <20190221174620.12144-5-jakub.kicinski@netronome.com>
On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> Support getting devlink instance from a new NDO.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> drivers/net/ethernet/netronome/nfp/nfp_app.h | 2 ++
> drivers/net/ethernet/netronome/nfp/nfp_devlink.c | 11 +++++++++++
> drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 1 +
> drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 1 +
> 4 files changed, 15 insertions(+)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> index d578d856a009..f8d422713705 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> @@ -433,4 +433,6 @@ int nfp_app_nic_vnic_alloc(struct nfp_app *app, struct nfp_net *nn,
> int nfp_app_nic_vnic_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
> struct nfp_net *nn, unsigned int id);
>
> +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev);
> +
> #endif
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> index db2da99f6aa7..e9eca99cf493 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> @@ -376,3 +376,14 @@ void nfp_devlink_port_unregister(struct nfp_port *port)
> {
> devlink_port_unregister(&port->dl_port);
> }
> +
> +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev)
> +{
> + struct nfp_app *app;
> +
> + app = nfp_app_from_netdev(netdev);
> + if (!app)
> + return NULL;
> +
> + return priv_to_devlink(app->pf);
> +}
AFAICS this would return a pointer to zero initialized struct devlink
when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
would execute
if (!dev->netdev_ops->ndo_get_devlink)
return;
devlink = dev->netdev_ops->ndo_get_devlink(dev);
if (!devlink || !devlink->ops->info_get)
return;
with non-null devlink but null devlink->ops so that it dereferences null
pointer (and so does devlink_compat_flash_update()).
Maybe it would be safer not to call ndo_get_devlink directly and have
an inline wrapper like
#if IS_ENABLED(CONFIG_NET_DEVLINK)
static inline struct devlink *dev_get_devlink(struct net_device *dev)
{
if (dev->netdev_ops->ndo_get_devlink)
return dev->netdev_ops->ndo_get_devlink();
else
retrurn NULL;
}
#else
static inline struct devlink *dev_get_devlink(struct net_device *dev)
{
return NULL;
}
#endif
so that one can simply call the wrapper and check return value for NULL.
Michal
^ permalink raw reply
* Re: [PATCH net-next 1/2] xdp: Always use a devmap for XDP_REDIRECT to a device
From: Toke Høiland-Jørgensen @ 2019-02-22 10:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, netdev, Jesper Dangaard Brouer, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20190221163627.7b8aa2ce@cakuba.netronome.com>
Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> On Thu, 21 Feb 2019 12:56:54 +0100, Toke Høiland-Jørgensen wrote:
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index b63bc77af2d1..629661db36ee 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -7527,6 +7527,12 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>> prog->dst_needed = 1;
>> if (insn->imm == BPF_FUNC_get_prandom_u32)
>> bpf_user_rnd_init_once();
>> + if (insn->imm == BPF_FUNC_redirect) {
>> + int err = dev_map_alloc_default_map();
>> +
>> + if (err)
>> + return err;
>> + }
>> if (insn->imm == BPF_FUNC_override_return)
>> prog->kprobe_override = 1;
>> if (insn->imm == BPF_FUNC_tail_call) {
>
>> +int dev_map_alloc_default_map(void)
>> +{
>> + struct net *net = current->nsproxy->net_ns;
>> + struct bpf_dtab *dtab, *old_dtab;
>> + struct net_device *netdev;
>> + union bpf_attr attr = {};
>> + u32 idx;
>> + int err;
>
> BPF programs don't obey by netns boundaries. The fact the program is
> verified in one ns doesn't mean this is the only ns it will be used in :(
> Meaning if any program is using the redirect map you may need a secret
> map in every ns.. no?
Ah, yes, good point. Totally didn't think about the fact that load and
attach are decoupled. Hmm, guess I'll just have to move the call to
alloc_default_map() to the point where the program is attached to an
interface, then...
I trust it's safe to skip the allocation in case the program is
offloaded to hardware, right? I.e., an offloaded program will never need
to fall back to the kernel helper?
-Toke
^ permalink raw reply
* Re: [PATCH net-next v2] route: Add a new fib_multipath_hash_policy base on cpu id for tunnel packet
From: Nikolay Aleksandrov @ 2019-02-22 10:26 UTC (permalink / raw)
To: wenxu, davem; +Cc: netdev
In-Reply-To: <1550827209-23440-1-git-send-email-wenxu@ucloud.cn>
On 22/02/2019 11:20, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> Current fib_multipath_hash_policy can make hash based on the L3 or
> L4. But it only work on the outer IP. So a specific tunnel always
> has the same hash value. But a specific tunnel may contain so many
> inner connections. However there is no good way for tunnel packet.
> A specific tunnel route based on the percpu dst_cache. It will not
> lookup route table for each packet.
>
> This patch provide a based cpu id hash policy. The different
> connection run on different cpu and there will be different hash
> value for percpu dst_cache.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> net/ipv4/route.c | 6 ++++++
> net/ipv4/sysctl_net_ipv4.c | 2 +-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
Hi,
When we had the same issue in the bonding, we added a new mode which used
the flow dissector to get the inner headers and hash on them.
I believe that is even easier nowadays, but I think people wanted to
use different hash algorithms as well and last we discussed this we
were talking about yet another bpf use to generate the hash.
Now we got bpf_set_hash() which can be used to achieve this from various
places, if you use that with hash_policy=1 (L4) then skb->hash will
be used and you can achieve any hashing that you desire.
Cheers,
Nik
^ permalink raw reply
* [PATCH] selftests: fib_tests: sleep after changing carrier. again.
From: Thadeu Lima de Souza Cascardo @ 2019-02-22 10:27 UTC (permalink / raw)
To: netdev; +Cc: linux-kselftest, Thadeu Lima de Souza Cascardo, David Ahern
Just like commit e2ba732a1681 ("selftests: fib_tests: sleep after
changing carrier"), wait one second to allow linkwatch to propagate the
carrier change to the stack.
There are two sets of carrier tests. The first slept after the carrier
was set to off, and when the second set ran, it was likely that the
linkwatch would be able to run again without much delay, reducing the
likelihood of a race. However, if you run 'fib_tests.sh -t carrier' on a
loop, you will quickly notice the failures.
Sleeping on the second set of tests make the failures go away.
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
tools/testing/selftests/net/fib_tests.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 802b4af18729..1080ff55a788 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -388,6 +388,7 @@ fib_carrier_unicast_test()
set -e
$IP link set dev dummy0 carrier off
+ sleep 1
set +e
echo " Carrier down"
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/5] net: dsa: fix a leaked reference by adding a missing of_node_put
From: Linus Walleij @ 2019-02-22 10:38 UTC (permalink / raw)
To: Wen Yang
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
netdev, linux-kernel@vger.kernel.org, Alexandre Belloni,
UNGLinuxDriver, Felix Fietkau, Lorenzo Bianconi, Kalle Valo,
Matthias Brugger, linux-wireless, Linux ARM,
moderated list:ARM/Mediatek SoC support, anirudh, John Linn,
Michal Simek, wang.yi59
In-Reply-To: <1550819742-32155-1-git-send-email-wen.yang99@zte.com.cn>
On Fri, Feb 22, 2019 at 8:15 AM Wen Yang <wen.yang99@zte.com.cn> wrote:
> The call to of_get_child_by_name returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
>
> Detected by coccinelle with the following warnings:
> ./drivers/net/dsa/rtl8366rb.c:510:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
> ./drivers/net/dsa/rtl8366rb.c:518:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
> ./drivers/net/dsa/rtl8366rb.c:540:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
> ./drivers/net/dsa/rtl8366rb.c:548:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
> ./drivers/net/dsa/rtl8366rb.c:556:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
> ./drivers/net/dsa/rtl8366rb.c:561:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 501, but without a corresponding object release within this function.
>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Vivien Didelot <vivien.didelot@gmail.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries
From: Nikolay Aleksandrov @ 2019-02-22 11:16 UTC (permalink / raw)
To: Hangbin Liu
Cc: Linus Lüssing, netdev, roopa, bridge, davem, yinxu,
Sebastian Gottschall
In-Reply-To: <20190222075705.GN10051@dhcp-12-139.nay.redhat.com>
On 22/02/2019 09:57, Hangbin Liu wrote:
> Hi Nikolay,
>
> On Thu, Feb 21, 2019 at 03:20:14PM +0200, Nikolay Aleksandrov wrote:
>>>
>>> Yes, I agree. But this "regression" could be fixed by setting up correct
>>> switch configuration. See more explains below.
>>>
>>
>> That is irrelevant, if the setup once worked we should not break it unless
>> it's in RFC requirement violation and RFC 4541 is only suggestive, not required.
>
> Thanks for your reply. I just noticed the RFC4541 category is informational.
>
>>> "because this would cause the Queries to be seen as coming from a newly
>>> elected Querier" means other address could be elected as a Querier but
>>> "0.0.0.0" should not.
>>>
>>
>> But this change hasn't been incorporated, has it ? A 0.0.0.0 address currently
>> will always win the election and silence all of the rest. Current bridge state
>> is simply broken for some cases because of that.
>
> Yes. I agree. I realized linus also said
>
> """
> However, one of the two options seems to be necessary. Either
> reverting the patch for the IGMP part, too. Or Ignoring 0.0.0.0
> sources for querier eletcion and presence detection.
> """
>
>>
>> Removing 0.0.0.0 from the election will effectively disable snooping even if there's
>> a configured bridge unless it has an address. You can see that this will end up in
>> people having suddenly their multicast flooded with current setups, right ?
>
> Yes
>
>> Any big behaviour change like that should be optional, but I don't think we need
>> another option as this is not so big of a deal because we're not breaking any
>> required behaviour.
>
> Just a little curious, RFC 3376 said the General Queries are sent from multicast
> routers. I think a router *should* has a IP address, isn't it?
>
> RFC 4541 also suggested:
>
> If the switch is not the Querier, it should use the 'all-zeros' IP
> Source Address in these proxy queries (even though some hosts may
> elect to not process queries with a 0.0.0.0 IP Source Address).
> When such proxy queries are received, they must not be included in
> the Querier election process.
>
> And what I got is most vendors apply this suggestion.
>
>> In case you decide to follow the option path, please use the new boolopt api to avoid
>> adding new fields to the bridge, this should be an on/off thing. I still vote for a
>> revert though.
>
> For consistency with other vendors and rfc, I would prefer to remove zero address election.
> For compatibility with previous users, I'm also OK to revert it.
> > Regards
> Hangbin
>
Hi,
In this case I'd suggest the following course of action:
- For -net/-stable revert the change since backporting new options is a
no-go and we need to restore the bridge state
- After -net is merged in net-next, for net-next if you'd like add it
as an option and also exclude it from elections when the option is
enabled (for example something like multicast_nonzero_src_querier).
Just please use the boolopt api and don't add new fields/attr ids.
Obviously by default this option will be off to be backwards
compatible and avoid surprise mcast flood.
Or just leave it reverted. :)
Thanks,
Nik
^ permalink raw reply
* [PATCH net-next] net: dsa: add missing of_node_put
From: Himadri Pandya @ 2019-02-22 11:18 UTC (permalink / raw)
To: andrew
Cc: vivien.didelot, f.fainelli, davem, netdev, linux-kernel,
Himadri Pandya
Decrement the reference count on port while returning out of the
loop. Issue identified by Coccinelle.
Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
---
net/dsa/dsa2.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index a1917025e155..396e7433dd8f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -624,19 +624,25 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
for_each_available_child_of_node(ports, port) {
err = of_property_read_u32(port, "reg", ®);
if (err)
- return err;
+ goto put_port;
- if (reg >= ds->num_ports)
- return -EINVAL;
+ if (reg >= ds->num_ports) {
+ err = -EINVAL;
+ goto put_port;
+ }
dp = &ds->ports[reg];
err = dsa_port_parse_of(dp, port);
if (err)
- return err;
+ goto put_port;
}
return 0;
+
+put_port:
+ of_node_put(port);
+ return err;
}
static int dsa_switch_parse_member_of(struct dsa_switch *ds,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 2/2] doc: add phylink documentation to the networking book
From: Russell King - ARM Linux admin @ 2019-02-22 11:20 UTC (permalink / raw)
To: Randy Dunlap; +Cc: linux-doc, netdev, David S. Miller, Jonathan Corbet
In-Reply-To: <f002402d-fb27-f697-f07d-de3cdff41f40@infradead.org>
Hi Randy,
Sorry for the delay.
On Wed, Feb 13, 2019 at 08:00:29PM -0800, Randy Dunlap wrote:
> On 2/5/19 7:58 AM, Russell King wrote:
> > +uses in-band mode, where results from the PHYs negotiation are passed
>
> PHY's
...
> > + Manipulation of the PHY's supported/advertised happens within phylink
>
> PHYs
Either one way or the other. "negotiation of the PHY", "supported/
advertised (link modes) of the PHY". Both are posessive, so the first
is a mistake but I don't agree with removing the posessive apostrophy
in the second case.
> > +For information describing the SFP cage in DT, please see the binding
> > +documentation in the kernel source tree
> > +``Documentation/devicetree/bindings/net/sff,sfp.txt``
> oh, so SFP means "Small Form-factor Pluggable".
>
> I see that this source file:
> ./drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:1902:
>
> seems to imply that SFP means "single function per port (SFP) mode":
>
> dev_err(&pf->pdev->dev,
> "VF %d requested polling mode: this feature is supported only when the device is running in single function per port (SFP) mode\n",
> vf->vf_id);
Software engineers endlessly like to create TLAs, so it could mean
both, but in this case it's the official definition from the SFF/
SNIA documents, e.g.:
INF-8074i Specification for
SFP (Small Formfactor Pluggable) Transceiver
note, no hyphen!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH net-next v2 1/2] net: phylink: update mac_config() documentation
From: Russell King @ 2019-02-22 11:31 UTC (permalink / raw)
To: linux-doc, netdev
A detail for mac_config() had been missed in the documentation for the
method - it is expected that the method will update the MAC to the
settings, rather than completely reprogram the MAC on each call.
Update the documentation for this method for this detail.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
include/linux/phylink.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index f57059e4353f..6411c624f63a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -149,6 +149,13 @@ int mac_link_state(struct net_device *ndev,
* configuration word. Nothing is advertised by the MAC. The MAC is
* responsible for reading the configuration word and configuring
* itself accordingly.
+ *
+ * Implementations are expected to update the MAC to reflect the
+ * requested settings - i.o.w., if nothing has changed between two
+ * calls, no action is expected. If only flow control settings have
+ * changed, flow control should be updated *without* taking the link
+ * down. This "update" behaviour is critical to avoid bouncing the
+ * link up status.
*/
void mac_config(struct net_device *ndev, unsigned int mode,
const struct phylink_link_state *state);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v2 2/2] doc: add phylink documentation to the networking book
From: Russell King @ 2019-02-22 11:31 UTC (permalink / raw)
To: linux-doc, netdev; +Cc: David S. Miller, Jonathan Corbet
Add some phylink documentation to the networking book detailing how
to convert network drivers from phylib to phylink.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
v2: updated with comments from Randy and updated to apply to net-next
Documentation/networking/index.rst | 1 +
Documentation/networking/sfp-phylink.rst | 268 +++++++++++++++++++++++++++++++
2 files changed, 269 insertions(+)
create mode 100644 Documentation/networking/sfp-phylink.rst
diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
index 9a32451cd201..b08cf145d5eb 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst
@@ -31,6 +31,7 @@ Linux Networking Documentation
failover
net_failover
phy
+ sfp-phylink
alias
bridge
snmp_counter
diff --git a/Documentation/networking/sfp-phylink.rst b/Documentation/networking/sfp-phylink.rst
new file mode 100644
index 000000000000..5bd26cb07244
--- /dev/null
+++ b/Documentation/networking/sfp-phylink.rst
@@ -0,0 +1,268 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=======
+phylink
+=======
+
+Overview
+========
+
+phylink is a mechanism to support hot-pluggable networking modules
+without needing to re-initialise the adapter on hot-plug events.
+
+phylink supports conventional phylib-based setups, fixed link setups
+and SFP (Small Formfactor Pluggable) modules at present.
+
+Modes of operation
+==================
+
+phylink has several modes of operation, which depend on the firmware
+settings.
+
+1. PHY mode
+
+ In PHY mode, we use phylib to read the current link settings from
+ the PHY, and pass them to the MAC driver. We expect the MAC driver
+ to configure exactly the modes that are specified without any
+ negotiation being enabled on the link.
+
+2. Fixed mode
+
+ Fixed mode is the same as PHY mode as far as the MAC driver is
+ concerned.
+
+3. In-band mode
+
+ In-band mode is used with 802.3z, SGMII and similar interface modes,
+ and we are expecting to use and honor the in-band negotiation or
+ control word sent across the serdes channel.
+
+By example, what this means is that:
+
+.. code-block:: none
+
+ ð {
+ phy = <&phy>;
+ phy-mode = "sgmii";
+ };
+
+does not use in-band SGMII signalling. The PHY is expected to follow
+exactly the settings given to it in its :c:func:`mac_config` function.
+The link should be forced up or down appropriately in the
+:c:func:`mac_link_up` and :c:func:`mac_link_down` functions.
+
+.. code-block:: none
+
+ ð {
+ managed = "in-band-status";
+ phy = <&phy>;
+ phy-mode = "sgmii";
+ };
+
+uses in-band mode, where results from the PHY's negotiation are passed
+to the MAC through the SGMII control word, and the MAC is expected to
+acknowledge the control word. The :c:func:`mac_link_up` and
+:c:func:`mac_link_down` functions must not force the MAC side link
+up and down.
+
+Rough guide to converting a network driver to sfp/phylink
+=========================================================
+
+This guide briefly describes how to convert a network driver from
+phylib to the sfp/phylink support. Please send patches to improve
+this documentation.
+
+1. Optionally split the network driver's phylib update function into
+ three parts dealing with link-down, link-up and reconfiguring the
+ MAC settings. This can be done as a separate preparation commit.
+
+ An example of this preparation can be found in git commit fc548b991fb0.
+
+2. Replace::
+
+ select FIXED_PHY
+ select PHYLIB
+
+ with::
+
+ select PHYLINK
+
+ in the driver's Kconfig stanza.
+
+3. Add::
+
+ #include <linux/phylink.h>
+
+ to the driver's list of header files.
+
+4. Add::
+
+ struct phylink *phylink;
+
+ to the driver's private data structure. We shall refer to the
+ driver's private data pointer as ``priv`` below, and the driver's
+ private data structure as ``struct foo_priv``.
+
+5. Replace the following functions:
+
+ .. flat-table::
+ :header-rows: 1
+ :widths: 1 1
+ :stub-columns: 0
+
+ * - Original function
+ - Replacement function
+ * - phy_start(phydev)
+ - phylink_start(priv->phylink)
+ * - phy_stop(phydev)
+ - phylink_stop(priv->phylink)
+ * - phy_mii_ioctl(phydev, ifr, cmd)
+ - phylink_mii_ioctl(priv->phylink, ifr, cmd)
+ * - phy_ethtool_get_wol(phydev, wol)
+ - phylink_ethtool_get_wol(priv->phylink, wol)
+ * - phy_ethtool_set_wol(phydev, wol)
+ - phylink_ethtool_set_wol(priv->phylink, wol)
+ * - phy_disconnect(phydev)
+ - phylink_disconnect_phy(priv->phylink)
+
+ Please note that some of these functions must be called under the
+ rtnl lock, and will warn if not. This will normally be the case,
+ except if these are called from the driver suspend/resume paths.
+
+6. Add/replace ksettings get/set methods with:
+
+ .. code-block:: c
+
+ static int foo_ethtool_set_link_ksettings(struct net_device *dev,
+ const struct ethtool_link_ksettings *cmd)
+ {
+ struct foo_priv *priv = netdev_priv(dev);
+
+ return phylink_ethtool_ksettings_set(priv->phylink, cmd);
+ }
+
+ static int foo_ethtool_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *cmd)
+ {
+ struct foo_priv *priv = netdev_priv(dev);
+
+ return phylink_ethtool_ksettings_get(priv->phylink, cmd);
+ }
+
+7. Replace the call to:
+
+ phy_dev = of_phy_connect(dev, node, link_func, flags, phy_interface);
+
+ and associated code with a call to:
+
+ err = phylink_of_phy_connect(priv->phylink, node, flags);
+
+ For the most part, ``flags`` can be zero; these flags are passed to
+ the of_phy_attach() inside this function call if a PHY is specified
+ in the DT node ``node``.
+
+ ``node`` should be the DT node which contains the network phy property,
+ fixed link properties, and will also contain the sfp property.
+
+ The setup of fixed links should also be removed; these are handled
+ internally by phylink.
+
+ of_phy_connect() was also passed a function pointer for link updates.
+ This function is replaced by a different form of MAC updates
+ described below in (8).
+
+ Manipulation of the PHY's supported/advertised happens within phylink
+ based on the validate callback, see below in (8).
+
+ Note that the driver no longer needs to store the ``phy_interface``,
+ and also note that ``phy_interface`` becomes a dynamic property,
+ just like the speed, duplex etc. settings.
+
+ Finally, note that the MAC driver has no direct access to the PHY
+ anymore; that is because in the phylink model, the PHY can be
+ dynamic.
+
+8. Add a :c:type:`struct phylink_mac_ops <phylink_mac_ops>` instance to
+ the driver, which is a table of function pointers, and implement
+ these functions. The old link update function for
+ :c:func:`of_phy_connect` becomes three methods: :c:func:`mac_link_up`,
+ :c:func:`mac_link_down`, and :c:func:`mac_config`. If step 1 was
+ performed, then the functionality will have been split there.
+
+ It is important that if in-band negotiation is used,
+ :c:func:`mac_link_up` and :c:func:`mac_link_down` do not prevent the
+ in-band negotiation from completing, since these functions are called
+ when the in-band link state changes - otherwise the link will never
+ come up.
+
+ The :c:func:`validate` method should mask the supplied supported mask,
+ and ``state->advertising`` with the supported ethtool link modes.
+ These are the new ethtool link modes, so bitmask operations must be
+ used. For an example, see drivers/net/ethernet/marvell/mvneta.c.
+
+ The :c:func:`mac_link_state` method is used to read the link state
+ from the MAC, and report back the settings that the MAC is currently
+ using. This is particularly important for in-band negotiation
+ methods such as 1000base-X and SGMII.
+
+ The :c:func:`mac_config` method is used to update the MAC with the
+ requested state, and must avoid unnecessarily taking the link down
+ when making changes to the MAC configuration. This means the
+ function should modify the state and only take the link down when
+ absolutely necessary to change the MAC configuration. An example
+ of how to do this can be found in :c:func:`mvneta_mac_config` in
+ drivers/net/ethernet/marvell/mvneta.c.
+
+ For further information on these methods, please see the inline
+ documentation in :c:type:`struct phylink_mac_ops <phylink_mac_ops>`.
+
+9. Remove calls to of_parse_phandle() for the PHY,
+ of_phy_register_fixed_link() for fixed links etc. from the probe
+ function, and replace with:
+
+ .. code-block:: c
+
+ struct phylink *phylink;
+
+ phylink = phylink_create(dev, node, phy_mode, &phylink_ops);
+ if (IS_ERR(phylink)) {
+ err = PTR_ERR(phylink);
+ fail probe;
+ }
+
+ priv->phylink = phylink;
+
+ and arrange to destroy the phylink in the probe failure path as
+ appropriate and the removal path too by calling:
+
+ .. code-block:: c
+
+ phylink_destroy(priv->phylink);
+
+10. Arrange for MAC link state interrupts to be forwarded into
+ phylink, via:
+
+ .. code-block:: c
+
+ phylink_mac_change(priv->phylink, link_is_up);
+
+ where ``link_is_up`` is true if the link is currently up or false
+ otherwise.
+
+11. Verify that the driver does not call::
+
+ netif_carrier_on()
+ netif_carrier_off()
+
+ as these will interfere with phylink's tracking of the link state,
+ and cause phylink to omit calls via the :c:func:`mac_link_up` and
+ :c:func:`mac_link_down` methods.
+
+Network drivers should call phylink_stop() and phylink_start() via their
+suspend/resume paths, which ensures that the appropriate
+:c:type:`struct phylink_mac_ops <phylink_mac_ops>` methods are called
+as necessary.
+
+For information describing the SFP cage in DT, please see the binding
+documentation in the kernel source tree
+``Documentation/devicetree/bindings/net/sff,sfp.txt``
--
2.7.4
^ permalink raw reply related
* [PATCH net] net/sched: act_ipt: fix refcount leak when replace fails
From: Davide Caratti @ 2019-02-22 11:33 UTC (permalink / raw)
To: Vlad Buslov, Cong Wang, Jamal Hadi Salim, Jiri Pirko,
David S. Miller
Cc: netdev
After commit 4e8ddd7f1758 ("net: sched: don't release reference on action
overwrite"), the error path of all actions was converted to drop refcount
also when the action was being overwritten. But we forgot act_ipt_init(),
in case allocation of 'tname' was not successful:
# tc action add action xt -j LOG --log-prefix hello index 100
tablename: mangle hook: NF_IP_POST_ROUTING
target: LOG level warning prefix "hello" index 100
# tc action show action xt
total acts 1
action order 0: tablename: mangle hook: NF_IP_POST_ROUTING
target LOG level warning prefix "hello"
index 100 ref 1 bind 0
# tc action replace action xt -j LOG --log-prefix world index 100
tablename: mangle hook: NF_IP_POST_ROUTING
target: LOG level warning prefix "world" index 100
RTNETLINK answers: Cannot allocate memory
We have an error talking to the kernel
# tc action show action xt
total acts 1
action order 0: tablename: mangle hook: NF_IP_POST_ROUTING
target LOG level warning prefix "hello"
index 100 ref 2 bind 0
Ensure we call tcf_idr_release(), in case 'tname' allocation failed, also
when the action is being replaced.
Fixes: 4e8ddd7f1758 ("net: sched: don't release reference on action overwrite")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_ipt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 8af6c11d2482..faa1addf89b3 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -199,8 +199,7 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
err2:
kfree(tname);
err1:
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return err;
}
--
2.20.1
^ permalink raw reply related
* [PATCH net] net/sched: act_skbedit: fix refcount leak when replace fails
From: Davide Caratti @ 2019-02-22 11:33 UTC (permalink / raw)
To: Vlad Buslov, Cong Wang, Jamal Hadi Salim, Jiri Pirko,
David S. Miller
Cc: netdev
when act_skbedit was converted to use RCU in the data plane, we added an
error path, but we forgot to drop the action refcount in case of failure
during a 'replace' operation:
# tc actions add action skbedit ptype otherhost pass index 100
# tc action show action skbedit
total acts 1
action order 0: skbedit ptype otherhost pass
index 100 ref 1 bind 0
# tc actions replace action skbedit ptype otherhost drop index 100
RTNETLINK answers: Cannot allocate memory
We have an error talking to the kernel
# tc action show action skbedit
total acts 1
action order 0: skbedit ptype otherhost pass
index 100 ref 2 bind 0
Ensure we call tcf_idr_release(), in case 'params_new' allocation failed,
also when the action is being replaced.
Fixes: c749cdda9089 ("net/sched: act_skbedit: don't use spinlock in the data path")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_skbedit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 64dba3708fce..cfceed28c333 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -189,8 +189,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return -ENOMEM;
}
--
2.20.1
^ permalink raw reply related
* RE: [PATCH net-next v2 1/7] net: Don't set transport offset to invalid value
From: Maxim Mikityanskiy @ 2019-02-22 12:30 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <CAF=yD-KhWPL5JewXMxsikRNYCL88GZqdwrGvNNAm10N-NNaocw@mail.gmail.com>
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 21 February, 2019 19:28
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to
> invalid value
>
> On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
> > skb->protocol will be unset, __skb_flow_dissect() will fail, and
> > skb_probe_transport_header() will fall back to the offset_hint, making
> > the resulting skb_transport_offset incorrect.
> >
> > If, however, there is no transport header in the packet,
> > transport_header shouldn't be set to an arbitrary value.
> >
> > Fix it by leaving the transport offset unset if it couldn't be found, to
> > be explicit rather than to fill it with some wrong value. It changes the
> > behavior, but if some code relied on the old behavior, it would be
> > broken anyway, as the old one is incorrect.
> >
> > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
>
> qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
> set for gso packets.
>
> Once net is merged into net-next, commit d5be7f632bad ("net: validate
This commit is already in net-next, isn't it?
> untrusted gso packets without csum offload") will ensure that packets
> that fail flow dissection do not make it into the stack. But we have
> to skip dissection in some cases, like tun [1].
OK, got you. However, is everything OK with patch [1]? It fixes false
positives, when a packet was dropped because network_header had not been
set yet for dissection to succeed, but what about evil packets that have
no network_offset at the moment of calling virtio_net_hdr_to_skb? Why
are all of them considered valid?
> I think we need to add a check in qdisc_pkt_len_init to skip the gso
> size estimation branch if !skb_transport_header_was_set(skb).
>
> Otherwise this patch set looks good to me. To avoid resubmitting
> everything we can fix up the qdisc_pkt_len_init in a follow-up, in
> which case I'm happy to add my Acked-by to this series.
I'll add this check and submit the patch soon. Thanks for reviewing!
> [1] http://patchwork.ozlabs.org/patch/1044429/
^ permalink raw reply
* Re: [Bridge] [PATCH net] net: bridge: remove ipv6 zero address check in mcast queries
From: Hangbin Liu @ 2019-02-22 12:49 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Linus Lüssing, netdev, roopa, bridge, davem, yinxu,
Sebastian Gottschall
In-Reply-To: <2f60dc0a-f6ee-06ad-f60c-15e035574a75@cumulusnetworks.com>
On Fri, Feb 22, 2019 at 01:16:53PM +0200, Nikolay Aleksandrov wrote:
> >
> > For consistency with other vendors and rfc, I would prefer to remove zero address election.
> > For compatibility with previous users, I'm also OK to revert it.
> > > Regards
> > Hangbin
> >
>
> Hi,
> In this case I'd suggest the following course of action:
> - For -net/-stable revert the change since backporting new options is a
> no-go and we need to restore the bridge state
> - After -net is merged in net-next, for net-next if you'd like add it
> as an option and also exclude it from elections when the option is
> enabled (for example something like multicast_nonzero_src_querier).
> Just please use the boolopt api and don't add new fields/attr ids.
> Obviously by default this option will be off to be backwards
> compatible and avoid surprise mcast flood.
>
> Or just leave it reverted. :)
This makes sense to me. Thanks for your suggestions.
Regards
Hangbin
^ permalink raw reply
* Re: [PATCH] sctp: don't compare hb_timer expire date before starting it
From: Marcelo Ricardo Leitner @ 2019-02-22 12:49 UTC (permalink / raw)
To: kwiecienmaciek; +Cc: linux-sctp, netdev, alexander.sverdlin, Maciej Kwiecien
In-Reply-To: <20190222084526.8214-1-maciej.kwiecien@nokia.com>
On Fri, Feb 22, 2019 at 09:45:26AM +0100, kwiecienmaciek@gmail.com wrote:
> From: Maciej Kwiecien <maciej.kwiecien@nokia.com>
>
> hb_timer might not start at all for a particular transport because its
> start is conditional. In a result a node is not sending heartbeats.
>
> Function sctp_transport_reset_hb_timer has two roles:
> - initial start of hb_timer for a given transport,
> - update expire date of hb_timer for a given transport.
> The function is optimized to update timer's expire only if it is before
> a new calculated one but this comparison is invalid for a timer which
> has not yet started. Such a timer has expire == 0 and if a new expire
> value is bigger than (MAX_JIFFIES / 2 + 2) then "time_before" macro will
> fail and timer will not start resulting in no heartbeat packets send by
> the node.
>
> This was found when association was initialized within first 5 mins
> after system boot due to jiffies init value which is near to MAX_JIFFIES.
>
> Test kernel version: 4.9.154 (ARCH=arm)
> hb_timer.expire = 0; //initialized, not started timer
> new_expire = MAX_JIFFIES / 2 + 2; //or more
> time_before(hb_timer.expire, new_expire) == false
>
> Fixes: ba6f5e33bdbb ("sctp: avoid refreshing heartbeat timer too often")
> Reported-by: Marcin Stojek <marcin.stojek@nokia.com>
> Tested-by: Marcin Stojek <marcin.stojek@nokia.com>
> Signed-off-by: Maciej Kwiecien <maciej.kwiecien@nokia.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/transport.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 033696e6f74f..ad158d311ffa 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -207,7 +207,8 @@ void sctp_transport_reset_hb_timer(struct sctp_transport *transport)
>
> /* When a data chunk is sent, reset the heartbeat interval. */
> expires = jiffies + sctp_transport_timeout(transport);
> - if (time_before(transport->hb_timer.expires, expires) &&
> + if ((time_before(transport->hb_timer.expires, expires) ||
> + !timer_pending(&transport->hb_timer)) &&
> !mod_timer(&transport->hb_timer,
> expires + prandom_u32_max(transport->rto)))
> sctp_transport_hold(transport);
> --
> 2.14.1
>
^ 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