* Re: KASAN: slab-out-of-bounds Read in tls_push_record
From: Dmitry Vyukov @ 2019-02-20 16:55 UTC (permalink / raw)
To: syzbot
Cc: aviadye, borisp, davejwatson, David Miller, LKML, netdev,
syzkaller-bugs
In-Reply-To: <000000000000ccb25d0576c17446@google.com>
On Wed, Sep 26, 2018 at 9:49 AM syzbot
<syzbot+942a1909765f0413329b@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 739d0def85ca Merge branch 'hv_netvsc-Support-LRO-RSC-in-th..
> git tree: net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=13aa179e400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=e79b9299baeb2298
> dashboard link: https://syzkaller.appspot.com/bug?extid=942a1909765f0413329b
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+942a1909765f0413329b@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in sg_mark_end
> include/linux/scatterlist.h:195 [inline]
> BUG: KASAN: slab-out-of-bounds in tls_push_record+0x1243/0x1330
> net/tls/tls_sw.c:521
> Read of size 8 at addr ffff8801a4d235f8 by task syz-executor1/7668
This is probably:
#syz dup: KASAN: out-of-bounds Write in tls_push_record
> CPU: 1 PID: 7668 Comm: syz-executor1 Not tainted 4.19.0-rc4+ #228
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
> print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
> kasan_report_error mm/kasan/report.c:354 [inline]
> kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
> __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
> sg_mark_end include/linux/scatterlist.h:195 [inline]
> tls_push_record+0x1243/0x1330 net/tls/tls_sw.c:521
> tls_sw_push_pending_record+0x22/0x30 net/tls/tls_sw.c:558
> tls_handle_open_record net/tls/tls_main.c:155 [inline]
> tls_sk_proto_close+0x439/0x750 net/tls/tls_main.c:272
> inet_release+0x104/0x1f0 net/ipv4/af_inet.c:428
> inet6_release+0x50/0x70 net/ipv6/af_inet6.c:458
> __sock_release+0xd7/0x250 net/socket.c:579
> sock_close+0x19/0x20 net/socket.c:1141
> __fput+0x385/0xa30 fs/file_table.c:278
> ____fput+0x15/0x20 fs/file_table.c:309
> task_work_run+0x1e8/0x2a0 kernel/task_work.c:113
> tracehook_notify_resume include/linux/tracehook.h:193 [inline]
> exit_to_usermode_loop+0x318/0x380 arch/x86/entry/common.c:166
> prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
> syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
> do_syscall_64+0x6be/0x820 arch/x86/entry/common.c:293
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x411151
> Code: 75 14 b8 03 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 34 19 00 00 c3 48
> 83 ec 08 e8 0a fc ff ff 48 89 04 24 b8 03 00 00 00 0f 05 <48> 8b 3c 24 48
> 89 c2 e8 53 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01
> RSP: 002b:00007fff8ba9e200 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
> RAX: 0000000000000000 RBX: 0000000000000005 RCX: 0000000000411151
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000004
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000001
> R10: 00007fff8ba9e130 R11: 0000000000000293 R12: 000000000000000f
> R13: 0000000000086e3d R14: 0000000000000412 R15: badc0ffeebadface
>
> Allocated by task 0:
> (stack is not available)
>
> Freed by task 0:
> (stack is not available)
>
> The buggy address belongs to the object at ffff8801a4d22d80
> which belongs to the cache kmalloc-2048 of size 2048
> The buggy address is located 120 bytes to the right of
> 2048-byte region [ffff8801a4d22d80, ffff8801a4d23580)
> The buggy address belongs to the page:
> page:ffffea0006934880 count:1 mapcount:0 mapping:ffff8801da800c40 index:0x0
> compound_mapcount: 0
> flags: 0x2fffc0000008100(slab|head)
> raw: 02fffc0000008100 ffffea000717fd08 ffff8801da801948 ffff8801da800c40
> raw: 0000000000000000 ffff8801a4d22500 0000000100000003 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8801a4d23480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8801a4d23500: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff8801a4d23580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ^
> ffff8801a4d23600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ffff8801a4d23680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/000000000000ccb25d0576c17446%40google.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH iproute2] ss: Render buffer to output every time a number of chunks are allocated
From: Stefano Brivio @ 2019-02-20 16:58 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Eric Dumazet, Phil Sutter, David Ahern, Sabrina Dubroca, netdev
In-Reply-To: <03dd56e5161a3c1270a21c4ba3f6e695793dbb74.1550105375.git.sbrivio@redhat.com>
Hi Stephen,
I think this patch is reasonably tested now. Eric, who reported the
original issue, is also satisfied with it. Is there any issue with it?
--
Stefano
On Thu, 14 Feb 2019 01:58:32 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:
> Eric reported that, with 10 million sockets, ss -emoi (about 1000 bytes
> output per socket) can easily lead to OOM (buffer would grow to 10GB of
> memory).
>
> Limit the maximum size of the buffer to five chunks, 1M each. Render and
> flush buffers whenever we reach that.
>
> This might make the resulting blocks slightly unaligned between them, with
> occasional loss of readability on lines occurring every 5k to 50k sockets
> approximately. Something like (from ss -tu):
>
> [...]
> CLOSE-WAIT 32 0 192.168.1.50:35232 10.0.0.1:https
> ESTAB 0 0 192.168.1.50:53820 10.0.0.1:https
> ESTAB 0 0 192.168.1.50:46924 10.0.0.1:https
> CLOSE-WAIT 32 0 192.168.1.50:35228 10.0.0.1:https
> [...]
>
> However, I don't actually expect any human user to scroll through that
> amount of sockets, so readability should be preserved when it matters.
>
> The bulk of the diffstat comes from moving field_next() around, as we now
> call render() from it. Functionally, this is implemented by six lines of
> code, most of them in field_next().
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: 691bd854bf4a ("ss: Buffer raw fields first, then render them as a table")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> Eric, it would be nice if you could test this with your bazillion sockets,
> I checked this with -emoi and "only" 500,000 sockets.
>
> misc/ss.c | 68 ++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/misc/ss.c b/misc/ss.c
> index 9e821faf0d31..28bdcba72d73 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -52,7 +52,8 @@
> #include <linux/tipc_sockets_diag.h>
>
> #define MAGIC_SEQ 123456
> -#define BUF_CHUNK (1024 * 1024)
> +#define BUF_CHUNK (1024 * 1024) /* Buffer chunk allocation size */
> +#define BUF_CHUNKS_MAX 5 /* Maximum number of allocated buffer chunks */
> #define LEN_ALIGN(x) (((x) + 1) & ~1)
>
> #define DIAG_REQUEST(_req, _r) \
> @@ -176,6 +177,7 @@ static struct {
> struct buf_token *cur; /* Position of current token in chunk */
> struct buf_chunk *head; /* First chunk */
> struct buf_chunk *tail; /* Current chunk */
> + int chunks; /* Number of allocated chunks */
> } buffer;
>
> static const char *TCP_PROTO = "tcp";
> @@ -936,6 +938,8 @@ static struct buf_chunk *buf_chunk_new(void)
>
> new->end = buffer.cur->data;
>
> + buffer.chunks++;
> +
> return new;
> }
>
> @@ -1080,33 +1084,6 @@ static int field_is_last(struct column *f)
> return f - columns == COL_MAX - 1;
> }
>
> -static void field_next(void)
> -{
> - field_flush(current_field);
> -
> - if (field_is_last(current_field))
> - current_field = columns;
> - else
> - current_field++;
> -}
> -
> -/* Walk through fields and flush them until we reach the desired one */
> -static void field_set(enum col_id id)
> -{
> - while (id != current_field - columns)
> - field_next();
> -}
> -
> -/* Print header for all non-empty columns */
> -static void print_header(void)
> -{
> - while (!field_is_last(current_field)) {
> - if (!current_field->disabled)
> - out("%s", current_field->header);
> - field_next();
> - }
> -}
> -
> /* Get the next available token in the buffer starting from the current token */
> static struct buf_token *buf_token_next(struct buf_token *cur)
> {
> @@ -1132,6 +1109,7 @@ static void buf_free_all(void)
> free(tmp);
> }
> buffer.head = NULL;
> + buffer.chunks = 0;
> }
>
> /* Get current screen width, default to 80 columns if TIOCGWINSZ fails */
> @@ -1294,6 +1272,40 @@ static void render(void)
> current_field = columns;
> }
>
> +/* Move to next field, and render buffer if we reached the maximum number of
> + * chunks, at the last field in a line.
> + */
> +static void field_next(void)
> +{
> + if (field_is_last(current_field) && buffer.chunks >= BUF_CHUNKS_MAX) {
> + render();
> + return;
> + }
> +
> + field_flush(current_field);
> + if (field_is_last(current_field))
> + current_field = columns;
> + else
> + current_field++;
> +}
> +
> +/* Walk through fields and flush them until we reach the desired one */
> +static void field_set(enum col_id id)
> +{
> + while (id != current_field - columns)
> + field_next();
> +}
> +
> +/* Print header for all non-empty columns */
> +static void print_header(void)
> +{
> + while (!field_is_last(current_field)) {
> + if (!current_field->disabled)
> + out("%s", current_field->header);
> + field_next();
> + }
> +}
> +
> static void sock_state_print(struct sockstat *s)
> {
> const char *sock_name;
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Alexei Starovoitov @ 2019-02-20 17:07 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, netdev
In-Reply-To: <20190220110629.21646-1-daniel@iogearbox.net>
On Wed, Feb 20, 2019 at 12:06:29PM +0100, Daniel Borkmann wrote:
> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> a check was added for BPF_PROG_RUN() that for every invocation preemption has
> to be disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this
> does not count for seccomp because only cBPF -> eBPF is loaded here and it does
> not make use of any functionality that would require this assertion. Fix this
> false positive by adding and using __BPF_PROG_RUN() variant that does not have
> the cant_sleep(); check.
>
> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> include/linux/filter.h | 9 ++++++++-
> kernel/seccomp.c | 2 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f32b3ec..2648fd7 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -533,7 +533,14 @@ struct sk_filter {
> struct bpf_prog *prog;
> };
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +#define bpf_prog_run__non_preempt(prog, ctx) \
> + ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
> +#define BPF_PROG_RUN(prog, ctx) \
> + bpf_prog_run__non_preempt(prog, ctx)
> +/* Direct use for cBPF -> eBPF only, but not for native eBPF. */
I think the comment is too abstract.
May be it should say that this is seccomp cBPF only ?
And macro name should be explicit as well ?
> +#define __BPF_PROG_RUN(prog, ctx) \
> + (*(prog)->bpf_func)(ctx, (prog)->insnsi)
>
> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index e815781..826d4e4 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
> * value always takes priority (ignoring the DATA).
> */
> for (; f; f = f->prev) {
> - u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
> + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd);
>
> if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) {
> ret = cur_ret;
> --
> 2.9.5
>
^ permalink raw reply
* [PATCH net 0/2] ipv6: route: enforce RCU protection for fib6_info->from
From: Paolo Abeni @ 2019-02-20 17:10 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David S. Miller
This series addresses a couple of RCU left-over dating back to rt6_info->from
conversion to RCU
Paolo Abeni (2):
ipv6: route: enforce RCU protection in rt6_update_exception_stamp_rt()
ipv6: route: enforce RCU protection in ip6_route_check_nh_onlink()
net/ipv6/route.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net 1/2] ipv6: route: enforce RCU protection in rt6_update_exception_stamp_rt()
From: Paolo Abeni @ 2019-02-20 17:10 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David S. Miller
In-Reply-To: <cover.1550682516.git.pabeni@redhat.com>
We must access rt6_info->from under RCU read lock: move the
dereference under such lock, with proper annotation, and use
rcu_access_pointer() to check for null value outside the lock.
Fixes: a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv6/route.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index bd09abd1fb22..cbaa8745d9ff 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1610,15 +1610,15 @@ static int rt6_remove_exception_rt(struct rt6_info *rt)
static void rt6_update_exception_stamp_rt(struct rt6_info *rt)
{
struct rt6_exception_bucket *bucket;
- struct fib6_info *from = rt->from;
struct in6_addr *src_key = NULL;
struct rt6_exception *rt6_ex;
+ struct fib6_info *from;
- if (!from ||
- !(rt->rt6i_flags & RTF_CACHE))
+ if (!rcu_access_pointer(rt->from) || !(rt->rt6i_flags & RTF_CACHE))
return;
rcu_read_lock();
+ from = rcu_dereference(rt->from);
bucket = rcu_dereference(from->rt6i_exception_bucket);
#ifdef CONFIG_IPV6_SUBTREES
--
2.20.1
^ permalink raw reply related
* [PATCH net 2/2] ipv6: route: enforce RCU protection in ip6_route_check_nh_onlink()
From: Paolo Abeni @ 2019-02-20 17:10 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David S. Miller
In-Reply-To: <cover.1550682516.git.pabeni@redhat.com>
We need a RCU critical section around rt6_info->from deference, and
proper annotation.
Fixes: 4ed591c8ab44 ("net/ipv6: Allow onlink routes to have a device mismatch if it is the default route")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv6/route.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cbaa8745d9ff..3b526a070299 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2753,20 +2753,24 @@ static int ip6_route_check_nh_onlink(struct net *net,
u32 tbid = l3mdev_fib_table(dev) ? : RT_TABLE_MAIN;
const struct in6_addr *gw_addr = &cfg->fc_gateway;
u32 flags = RTF_LOCAL | RTF_ANYCAST | RTF_REJECT;
+ struct fib6_info *from;
struct rt6_info *grt;
int err;
err = 0;
grt = ip6_nh_lookup_table(net, cfg, gw_addr, tbid, 0);
if (grt) {
+ rcu_read_lock();
+ from = rcu_dereference(grt->from);
if (!grt->dst.error &&
/* ignore match if it is the default route */
- grt->from && !ipv6_addr_any(&grt->from->fib6_dst.addr) &&
+ from && !ipv6_addr_any(&from->fib6_dst.addr) &&
(grt->rt6i_flags & flags || dev != grt->dst.dev)) {
NL_SET_ERR_MSG(extack,
"Nexthop has invalid gateway or device mismatch");
err = -EINVAL;
}
+ rcu_read_unlock();
ip6_rt_put(grt);
}
--
2.20.1
^ permalink raw reply related
* [PATCH net v2] ipv6: route: purge exception on removal
From: Paolo Abeni @ 2019-02-20 17:18 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David S. Miller
When a netdevice is unregistered, we flush the relevant exception
via rt6_sync_down_dev() -> fib6_ifdown() -> fib6_del() -> fib6_del_route().
Finally, we end-up calling rt6_remove_exception(), where we release
the relevant dst, while we keep the references to the related fib6_info and
dev. Such references should be released later when the dst will be
destroyed.
There are a number of caches that can keep the exception around for an
unlimited amount of time - namely dst_cache, possibly even socket cache.
As a result device registration may hang, as demonstrated by this script:
ip netns add cl
ip netns add rt
ip netns add srv
ip netns exec rt sysctl -w net.ipv6.conf.all.forwarding=1
ip link add name cl_veth type veth peer name cl_rt_veth
ip link set dev cl_veth netns cl
ip -n cl link set dev cl_veth up
ip -n cl addr add dev cl_veth 2001::2/64
ip -n cl route add default via 2001::1
ip -n cl link add tunv6 type ip6tnl mode ip6ip6 local 2001::2 remote 2002::1 hoplimit 64 dev cl_veth
ip -n cl link set tunv6 up
ip -n cl addr add 2013::2/64 dev tunv6
ip link set dev cl_rt_veth netns rt
ip -n rt link set dev cl_rt_veth up
ip -n rt addr add dev cl_rt_veth 2001::1/64
ip link add name rt_srv_veth type veth peer name srv_veth
ip link set dev srv_veth netns srv
ip -n srv link set dev srv_veth up
ip -n srv addr add dev srv_veth 2002::1/64
ip -n srv route add default via 2002::2
ip -n srv link add tunv6 type ip6tnl mode ip6ip6 local 2002::1 remote 2001::2 hoplimit 64 dev srv_veth
ip -n srv link set tunv6 up
ip -n srv addr add 2013::1/64 dev tunv6
ip link set dev rt_srv_veth netns rt
ip -n rt link set dev rt_srv_veth up
ip -n rt addr add dev rt_srv_veth 2002::2/64
ip netns exec srv netserver & sleep 0.1
ip netns exec cl ping6 -c 4 2013::1
ip netns exec cl netperf -H 2013::1 -t TCP_STREAM -l 3 & sleep 1
ip -n rt link set dev rt_srv_veth mtu 1400
wait %2
ip -n cl link del cl_veth
This commit addresses the issue purging all the references held by the
exception at time, as we currently do for e.g. ipv6 pcpu dst entries.
v1 -> v2:
- re-order the code to avoid accessing dst and net after dst_dev_put()
Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv6/route.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 964491cf3672..bd09abd1fb22 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1274,18 +1274,29 @@ static DEFINE_SPINLOCK(rt6_exception_lock);
static void rt6_remove_exception(struct rt6_exception_bucket *bucket,
struct rt6_exception *rt6_ex)
{
+ struct fib6_info *from;
struct net *net;
if (!bucket || !rt6_ex)
return;
net = dev_net(rt6_ex->rt6i->dst.dev);
+ net->ipv6.rt6_stats->fib_rt_cache--;
+
+ /* purge completely the exception to allow releasing the held resources:
+ * some [sk] cache may keep the dst around for unlimited time
+ */
+ from = rcu_dereference_protected(rt6_ex->rt6i->from,
+ lockdep_is_held(&rt6_exception_lock));
+ rcu_assign_pointer(rt6_ex->rt6i->from, NULL);
+ fib6_info_release(from);
+ dst_dev_put(&rt6_ex->rt6i->dst);
+
hlist_del_rcu(&rt6_ex->hlist);
dst_release(&rt6_ex->rt6i->dst);
kfree_rcu(rt6_ex, rcu);
WARN_ON_ONCE(!bucket->depth);
bucket->depth--;
- net->ipv6.rt6_stats->fib_rt_cache--;
}
/* Remove oldest rt6_ex in bucket and free the memory
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
From: Florian Fainelli @ 2019-02-20 17:22 UTC (permalink / raw)
To: Russell King, Andrew Lunn, Vivien Didelot; +Cc: David S. Miller, netdev
In-Reply-To: <E1gwPBM-0008SQ-CM@rmk-PC.armlinux.org.uk>
On 2/20/19 2:32 AM, Russell King wrote:
> When a DSA port is added to a bridge and brought up, the resulting STP
> state programmed into the hardware depends on the order that these
> operations are performed. However, the Linux bridge code believes that
> the port is in disabled mode.
>
> If the DSA port is first added to a bridge and then brought up, it will
> be in blocking mode. If it is brought up and then added to the bridge,
> it will be in disabled mode.
>
> This difference is caused by DSA always setting the STP mode in
> dsa_port_enable() whether or not this port is part of a bridge. Since
> bridge always sets the STP state when the port is added, brought up or
> taken down, it is unnecessary for us to manipulate the STP state.
>
> Apparently, this code was copied from Rocker, and the very next day a
> similar fix for Rocker was merged but was not propagated to DSA. See
> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>
> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Nice example of cargo cult programming, thanks for fixing this!
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH net 1/2] ipv6: route: enforce RCU protection in rt6_update_exception_stamp_rt()
From: Paolo Abeni @ 2019-02-20 17:22 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, David S. Miller
In-Reply-To: <1ac484e7626a5d5e7c22de13485575de30ad7fe0.1550682516.git.pabeni@redhat.com>
On Wed, 2019-02-20 at 18:10 +0100, Paolo Abeni wrote:
> We must access rt6_info->from under RCU read lock: move the
> dereference under such lock, with proper annotation, and use
> rcu_access_pointer() to check for null value outside the lock.
>
> Fixes: a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/ipv6/route.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index bd09abd1fb22..cbaa8745d9ff 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1610,15 +1610,15 @@ static int rt6_remove_exception_rt(struct rt6_info *rt)
> static void rt6_update_exception_stamp_rt(struct rt6_info *rt)
> {
> struct rt6_exception_bucket *bucket;
> - struct fib6_info *from = rt->from;
> struct in6_addr *src_key = NULL;
> struct rt6_exception *rt6_ex;
> + struct fib6_info *from;
>
> - if (!from ||
> - !(rt->rt6i_flags & RTF_CACHE))
> + if (!rcu_access_pointer(rt->from) || !(rt->rt6i_flags & RTF_CACHE))
> return;
>
> rcu_read_lock();
> + from = rcu_dereference(rt->from);
-ELOWONCOFFEE: even this one is racy, as rt->from can go away due to
underlying device removal between the two fetch operation.
I'll send a v2.
Again, I'm sorry for the noise,
Paolo
^ permalink raw reply
* Re: [PATCH v2 net-next 2/4] net: dsa: microchip: add MIB counter reading support
From: Florian Fainelli @ 2019-02-20 17:22 UTC (permalink / raw)
To: Andrew Lunn, Tristram.Ha
Cc: Sergio Paracuellos, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <20190220150826.GI13075@lunn.ch>
On 2/20/19 7:08 AM, Andrew Lunn wrote:
>> +static void mib_monitor(struct timer_list *t)
>> +{
>> + struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
>> + const struct dsa_port *dp;
>> + struct net_device *netdev;
>> + struct ksz_port_mib *mib;
>> + struct ksz_port *p;
>> + int i;
>> +
>> + mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
>> +
>> + /* Check which port needs to read MIB counters. */
>> + for (i = 0; i < dev->mib_port_cnt; i++) {
>> + p = &dev->ports[i];
>> + if (!p->on)
>> + continue;
>> + dp = dsa_to_port(dev->ds, i);
>> + netdev = dp->slave;
>> +
>> + mib = &p->mib;
>> + mutex_lock(&mib->cnt_mutex);
>> +
>> + /* Read only dropped counters when link is not up. */
>> + if (netdev && netdev->phydev && !netdev->phydev->link)
>> + mib->cnt_ptr = dev->reg_mib_cnt;
>> + mutex_unlock(&mib->cnt_mutex);
>> + p->read = true;
>> + }
>> + schedule_work(&dev->mib_read);
>> +}
>
> Hi Tristram
>
> This is much easier to understand. Thanks for making the change.
>
> However, i suspect Florian was suggesting you use
> netif_carrier_ok(netdev), not poke around inside the phydev structure.
Yes indeed.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] net: dsa: enable flooding for bridge ports
From: Vivien Didelot @ 2019-02-20 17:23 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <E1gwR7T-0003iw-Cq@rmk-PC.armlinux.org.uk>
Hi Russell,
On Wed, 20 Feb 2019 12:36:59 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> Switches work by learning the MAC address for each attached station by
> monitoring traffic from each station. When a station sends a packet,
> the switch records which port the MAC address is connected to.
>
> With IPv4 networking, before communication commences with a neighbour,
> an ARP packet is broadcasted to all stations asking for the MAC address
> corresponding with the IPv4. The desired station responds with an ARP
> reply, and the ARP reply causes the switch to learn which port the
> station is connected to.
>
> With IPv6 networking, the situation is rather different. Rather than
> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> rather than broadcasted. This multicast needs to reach the intended
> station in order for the neighbour to be discovered.
>
> Once a neighbour has been discovered, and entered into the sending
> stations neighbour cache, communication can restart at a point later
> without sending a new neighbour solicitation, even if the entry in
> the neighbour cache is marked as stale. This can be after the MAC
> address has expired from the forwarding cache of the DSA switch -
> when that occurs, there is a long pause in communication.
>
> Our DSA implementation for mv88e6xxx switches disables flooding of
> multicast and unicast frames for bridged ports. As per the above
> description, this is fine for IPv4 networking, since the broadcasted
> ARP queries will be sent to and received by all stations on the same
> network. However, this breaks IPv6 very badly - blocking neighbour
> solicitations and later causing connections to stall.
>
> The defaults that the Linux bridge code expect from bridges are for
> unknown unicast and unknown multicast frames to be flooded to all ports
> on the bridge, which is at odds to the defaults adopted by our DSA
> implementation for mv88e6xxx switches.
>
> This commit enables by default flooding of both unknown unicast and
> unknown multicast frames whenever a port is added to a bridge, and
> disables the flooding when a port leaves the bridge. This means that
> mv88e6xxx DSA switches now behave as per the bridge(8) man page, and
> IPv6 works flawlessly through such a switch.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> net/dsa/port.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index b84d010fb165..9e7aab13957e 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -105,6 +105,11 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> };
> int err;
>
> + /* Set the flooding mode before joining */
Note that as stated by the comment just below, the port has already joined
the bridge here.
> + err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD, NULL);
> + if (err)
> + return err;
> +
> /* Here the port is already bridged. Reflect the current configuration
> * so that drivers can program their chips accordingly.
> */
> @@ -113,8 +118,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> err = dsa_port_notify(dp, DSA_NOTIFIER_BRIDGE_JOIN, &info);
>
> /* The bridging is rolled back on error */
> - if (err)
> + if (err) {
> + dsa_port_bridge_flags(dp, 0, NULL);
> dp->bridge_dev = NULL;
> + }
>
> return err;
> }
> @@ -137,6 +144,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
> if (err)
> pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
>
> + /* Port is leaving the bridge, disable flooding */
> + dsa_port_bridge_flags(dp, BR_LEARNING, NULL);
> +
> /* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
> * so allow it to be in BR_STATE_FORWARDING to be kept functional
> */
This makes it clear that we must add this logic which sets the expected
default flags into the bridge code itself. But this can be done later.
Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v3 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-20 17:26 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <E1gwR7O-0003ig-0P@rmk-PC.armlinux.org.uk>
Hi Russell,
On Wed, 20 Feb 2019 12:36:54 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> Add support for the bridge flags to Marvell 88e6xxx bridges, allowing
> the multicast and unicast flood properties to be controlled. These
> can be controlled on a per-port basis via commands such as:
>
> bridge link set dev lan1 flood on|off
> bridge link set dev lan1 mcast_flood on|off
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 32e7af5caa69..937370639fe4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4692,6 +4692,24 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
> return err;
> }
>
> +static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
> + bool unicast, bool multicast)
> +{
> + struct mv88e6xxx_chip *chip = ds->priv;
> + int ret = -EOPNOTSUPP;
I'd prefer "err" like in the rest of the driver.
> +
> + WARN_ON_ONCE(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port));
No need to warn, the driver does not need to care about which port is targeted
by a call.
> +
> + mutex_lock(&chip->reg_lock);
> + if (chip->info->ops->port_set_egress_floods)
> + ret = chip->info->ops->port_set_egress_floods(chip, port,
> + unicast,
> + multicast);
> + mutex_unlock(&chip->reg_lock);
> +
> + return ret;
> +}
> +
> static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
> #if IS_ENABLED(CONFIG_NET_DSA_LEGACY)
> .probe = mv88e6xxx_drv_probe,
> @@ -4719,6 +4737,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
> .set_ageing_time = mv88e6xxx_set_ageing_time,
> .port_bridge_join = mv88e6xxx_port_bridge_join,
> .port_bridge_leave = mv88e6xxx_port_bridge_leave,
> + .port_egress_floods = mv88e6xxx_port_egress_floods,
> .port_stp_state_set = mv88e6xxx_port_stp_state_set,
> .port_fast_age = mv88e6xxx_port_fast_age,
> .port_vlan_filtering = mv88e6xxx_port_vlan_filtering,
Otherwise it looks good to me.
^ permalink raw reply
* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
From: Andrew Lunn @ 2019-02-20 17:27 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Russell King, Vivien Didelot, David S. Miller, netdev
In-Reply-To: <b7f73796-8545-c953-d71b-c0b0adb85965@gmail.com>
On Wed, Feb 20, 2019 at 09:22:30AM -0800, Florian Fainelli wrote:
> On 2/20/19 2:32 AM, Russell King wrote:
> > When a DSA port is added to a bridge and brought up, the resulting STP
> > state programmed into the hardware depends on the order that these
> > operations are performed. However, the Linux bridge code believes that
> > the port is in disabled mode.
> >
> > If the DSA port is first added to a bridge and then brought up, it will
> > be in blocking mode. If it is brought up and then added to the bridge,
> > it will be in disabled mode.
> >
> > This difference is caused by DSA always setting the STP mode in
> > dsa_port_enable() whether or not this port is part of a bridge. Since
> > bridge always sets the STP state when the port is added, brought up or
> > taken down, it is unnecessary for us to manipulate the STP state.
> >
> > Apparently, this code was copied from Rocker, and the very next day a
> > similar fix for Rocker was merged but was not propagated to DSA. See
> > e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
> >
> > Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>
> Nice example of cargo cult programming, thanks for fixing this!
Maybe now would be a good time to look at other drivers. Does the
Microsemi Ocelot driver have the same issue?
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] net: dsa: add support for bridge flags
From: Vivien Didelot @ 2019-02-20 17:30 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <E1gwR7I-0003iS-Kg@rmk-PC.armlinux.org.uk>
Hi Russell,
On Wed, 20 Feb 2019 12:36:48 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> The Linux bridge implementation allows various properties of the bridge
> to be controlled, such as flooding unknown unicast and multicast frames.
> This patch adds the necessary DSA infrastructure to allow the Linux
> bridge support to control these properties for DSA switches.
>
> We implement this by providing two new methods: one to get the switch-
> wide support bitmask, and another to set the properties.
This is not true anymore ;-)
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> include/net/dsa.h | 2 ++
> net/dsa/dsa_priv.h | 2 ++
> net/dsa/port.c | 16 ++++++++++++++++
> net/dsa/slave.c | 6 ++++++
> 4 files changed, 26 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7f2a668ef2cc..2c2c10812814 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -400,6 +400,8 @@ struct dsa_switch_ops {
> void (*port_stp_state_set)(struct dsa_switch *ds, int port,
> u8 state);
> void (*port_fast_age)(struct dsa_switch *ds, int port);
> + int (*port_egress_floods)(struct dsa_switch *ds, int port,
> + bool unicast, bool multicast);
>
> /*
> * VLAN support
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 1f4972dab9f2..f4f99ec29f5d 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -160,6 +160,8 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
> struct switchdev_trans *trans);
> int dsa_port_mdb_del(const struct dsa_port *dp,
> const struct switchdev_obj_port_mdb *mdb);
> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> + struct switchdev_trans *trans);
> int dsa_port_vlan_add(struct dsa_port *dp,
> const struct switchdev_obj_port_vlan *vlan,
> struct switchdev_trans *trans);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 2d7e01b23572..b84d010fb165 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -177,6 +177,22 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
> return dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
> }
>
> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> + struct switchdev_trans *trans)
> +{
> + struct dsa_switch *ds = dp->ds;
> + int port = dp->index;
> +
> + if (switchdev_trans_ph_prepare(trans))
> + return 0;
> +
> + if (ds->ops->port_egress_floods)
> + ds->ops->port_egress_floods(ds, port, flags & BR_FLOOD,
> + flags & BR_MCAST_FLOOD);
Even though we're not supposed to fail in the switchdev commit phase, I'd
prefer not to ignore the return code.
> +
> + return 0;
> +}
> +
> int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
> u16 vid)
> {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 2e5e7c04821b..f99161c3b1ea 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -295,6 +295,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
> case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
> ret = dsa_port_ageing_time(dp, attr->u.ageing_time, trans);
> break;
> + case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> + ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans);
> + break;
> default:
> ret = -EOPNOTSUPP;
> break;
> @@ -384,6 +387,9 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
> switch (attr->id) {
> case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
> attr->u.brport_flags_support = 0;
> + if (ds->ops->port_egress_floods)
> + attr->u.brport_flags_support |= BR_FLOOD |
> + BR_MCAST_FLOOD;
> break;
> default:
> return -EOPNOTSUPP;
Otherwise, LGTM:
Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] net: dsa: enable flooding for bridge ports
From: Russell King - ARM Linux admin @ 2019-02-20 17:33 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190220122355.GB32192@t480s.localdomain>
On Wed, Feb 20, 2019 at 12:23:55PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Wed, 20 Feb 2019 12:36:59 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > Switches work by learning the MAC address for each attached station by
> > monitoring traffic from each station. When a station sends a packet,
> > the switch records which port the MAC address is connected to.
> >
> > With IPv4 networking, before communication commences with a neighbour,
> > an ARP packet is broadcasted to all stations asking for the MAC address
> > corresponding with the IPv4. The desired station responds with an ARP
> > reply, and the ARP reply causes the switch to learn which port the
> > station is connected to.
> >
> > With IPv6 networking, the situation is rather different. Rather than
> > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > rather than broadcasted. This multicast needs to reach the intended
> > station in order for the neighbour to be discovered.
> >
> > Once a neighbour has been discovered, and entered into the sending
> > stations neighbour cache, communication can restart at a point later
> > without sending a new neighbour solicitation, even if the entry in
> > the neighbour cache is marked as stale. This can be after the MAC
> > address has expired from the forwarding cache of the DSA switch -
> > when that occurs, there is a long pause in communication.
> >
> > Our DSA implementation for mv88e6xxx switches disables flooding of
> > multicast and unicast frames for bridged ports. As per the above
> > description, this is fine for IPv4 networking, since the broadcasted
> > ARP queries will be sent to and received by all stations on the same
> > network. However, this breaks IPv6 very badly - blocking neighbour
> > solicitations and later causing connections to stall.
> >
> > The defaults that the Linux bridge code expect from bridges are for
> > unknown unicast and unknown multicast frames to be flooded to all ports
> > on the bridge, which is at odds to the defaults adopted by our DSA
> > implementation for mv88e6xxx switches.
> >
> > This commit enables by default flooding of both unknown unicast and
> > unknown multicast frames whenever a port is added to a bridge, and
> > disables the flooding when a port leaves the bridge. This means that
> > mv88e6xxx DSA switches now behave as per the bridge(8) man page, and
> > IPv6 works flawlessly through such a switch.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > net/dsa/port.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index b84d010fb165..9e7aab13957e 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -105,6 +105,11 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> > };
> > int err;
> >
> > + /* Set the flooding mode before joining */
>
> Note that as stated by the comment just below, the port has already joined
> the bridge here.
The software interface has joined at this point, but not the physical
port itself. I actually find that the statement in the comment below
this code is the confusing statement.
>
> > + err = dsa_port_bridge_flags(dp, BR_FLOOD | BR_MCAST_FLOOD, NULL);
> > + if (err)
> > + return err;
> > +
> > /* Here the port is already bridged. Reflect the current configuration
> > * so that drivers can program their chips accordingly.
This is the confusing statement: the switch port is not bridged at
this point since we haven't programmed the hardware to make that happen.
The Linux interface corresponding to the switch port is what has been
bridged.
> > */
> > @@ -113,8 +118,10 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br)
> > err = dsa_port_notify(dp, DSA_NOTIFIER_BRIDGE_JOIN, &info);
> >
> > /* The bridging is rolled back on error */
> > - if (err)
> > + if (err) {
> > + dsa_port_bridge_flags(dp, 0, NULL);
> > dp->bridge_dev = NULL;
> > + }
> >
> > return err;
> > }
> > @@ -137,6 +144,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
> > if (err)
> > pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
> >
> > + /* Port is leaving the bridge, disable flooding */
> > + dsa_port_bridge_flags(dp, BR_LEARNING, NULL);
Hmm, that should've been 0 not BR_LEARNING since we are not dealing
with that flag yet. I'll send a v4 later this evening.
> > +
> > /* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
> > * so allow it to be in BR_STATE_FORWARDING to be kept functional
> > */
>
>
> This makes it clear that we must add this logic which sets the expected
> default flags into the bridge code itself. But this can be done later.
Note that the above is carefully chosen to ensure that the port is
configured for flooding whenever traffic may pass in bridge mode,
which I suggest is something that is kept in future.
>
>
> Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>
>
--
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
* Re: [PATCH][next] rtlwifi: rtl8192ce: fix typo, "PairwiseENcAlgorithm" -> "PairwiseEncAlgorithm"
From: Kalle Valo @ 2019-02-20 17:55 UTC (permalink / raw)
To: Colin King
Cc: Ping-Ke Shih, David S . Miller, Larry Finger, linux-wireless,
netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190220092727.17119-1-colin.king@canonical.com>
Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is an uppercase 'N' that should be a lowercase 'n', fix this.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Patch applied to wireless-drivers-next.git, thanks.
0421dd4167ec rtlwifi: rtl8192ce: fix typo, "PairwiseENcAlgorithm" -> "PairwiseEncAlgorithm"
--
https://patchwork.kernel.org/patch/10821751/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
From: Nathan Chancellor @ 2019-02-20 17:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, Kalle Valo, linux-wireless, Networking,
Linux Kernel Mailing List, Nick Desaulniers
In-Reply-To: <CAK8P3a1d6Zksdv-Or+paawyn4N0u4EpVw9QgyDGHYvX_1s3JrQ@mail.gmail.com>
On Wed, Feb 20, 2019 at 11:51:34AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 19, 2019 at 7:22 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
>
> > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> > index e9822a3ec373..92b22250eb7d 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> > @@ -462,7 +462,7 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
> > {
> > s64 rtt_avg = res->ftm.rtt_avg * 100;
> >
> > - do_div(rtt_avg, 6666);
> > + div64_s64(rtt_avg, 6666);
>
> This is wrong: div64_s64 does not modify its argument like do_div(), but
> it returns the result instead. You also don't want to divide by a 64-bit
> value when the second argument is a small constant.
>
> I think the correct way should be
>
> s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, 6666);
>
> If you know that the value is positive, using unsigned types
> and div_u64() would be slightly faster.
>
> Arnd
Thanks for the review and explanation, Arnd.
Luca, could you drop this version so I can resend it?
Nathan
^ permalink raw reply
* Re: [RESEND PATCH 0/7] Add FOLL_LONGTERM to GUP fast and use it
From: Ira Weiny @ 2019-02-20 18:02 UTC (permalink / raw)
To: Christoph Hellwig
Cc: John Hubbard, Andrew Morton, Michal Hocko, Kirill A. Shutemov,
Peter Zijlstra, Jason Gunthorpe, Benjamin Herrenschmidt,
Paul Mackerras, David S. Miller, Martin Schwidefsky,
Heiko Carstens, Rich Felker, Yoshinori Sato, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Ralf Baechle, Paul Burton,
James Hogan, linux-kernel, linux-mm, linux-mips, linuxppc-dev,
linux-s390, linux-sh, sparclinux, kvm-ppc, kvm, linux-fpga,
dri-devel, linux-rdma, linux-media, linux-scsi, devel,
virtualization, netdev, linux-fbdev, xen-devel, devel, ceph-devel,
rds-devel
In-Reply-To: <20190220151930.GB11695@infradead.org>
On Wed, Feb 20, 2019 at 07:19:30AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 19, 2019 at 09:30:33PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > Resending these as I had only 1 minor comment which I believe we have covered
> > in this series. I was anticipating these going through the mm tree as they
> > depend on a cleanup patch there and the IB changes are very minor. But they
> > could just as well go through the IB tree.
> >
> > NOTE: This series depends on my clean up patch to remove the write parameter
> > from gup_fast_permitted()[1]
> >
> > HFI1, qib, and mthca, use get_user_pages_fast() due to it performance
> > advantages. These pages can be held for a significant time. But
> > get_user_pages_fast() does not protect against mapping of FS DAX pages.
>
> This I don't get - if you do lock down long term mappings performance
> of the actual get_user_pages call shouldn't matter to start with.
>
> What do I miss?
A couple of points.
First "longterm" is a relative thing and at this point is probably a misnomer.
This is really flagging a pin which is going to be given to hardware and can't
move. I've thought of a couple of alternative names but I think we have to
settle on if we are going to use FL_LAYOUT or something else to solve the
"longterm" problem. Then I think we can change the flag to a better name.
Second, It depends on how often you are registering memory. I have spoken with
some RDMA users who consider MR in the performance path... For the overall
application performance. I don't have the numbers as the tests for HFI1 were
done a long time ago. But there was a significant advantage. Some of which is
probably due to the fact that you don't have to hold mmap_sem.
Finally, architecturally I think it would be good for everyone to use *_fast.
There are patches submitted to the RDMA list which would allow the use of
*_fast (they reworking the use of mmap_sem) and as soon as they are accepted
I'll submit a patch to convert the RDMA core as well. Also to this point
others are looking to use *_fast.[2]
As an asside, Jasons pointed out in my previous submission that *_fast and
*_unlocked look very much the same. I agree and I think further cleanup will
be coming. But I'm focused on getting the final solution for DAX at the
moment.
Ira
^ permalink raw reply
* [PATCH] net: dsa: add missing phy address offset
From: Marcel Reichmuth @ 2019-02-20 18:15 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc: andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
davem@davemloft.net, Marcel Reichmuth
When phys do not start at address 0 like on the mv88e6341 the wrong
phy address is used and therefore the slave ports can not be
initialized. This patch adds the proper offset to the phy address.
Signed-off-by: Marcel Reichmuth <marcel.reichmuth@netmodule.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 3 +++
include/net/dsa.h | 1 +
net/dsa/slave.c | 3 ++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 12fd7ce3f1ff..0ca649f784d2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2198,12 +2198,15 @@ static int mv88e6xxx_setup_upstream_port(struct mv88e6xxx_chip *chip, int port)
static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
{
struct dsa_switch *ds = chip->ds;
+ struct dsa_port *dp = &ds->ports[port];
int err;
u16 reg;
chip->ports[port].chip = chip;
chip->ports[port].port = port;
+ dp->phy_base_addr = chip->info->phy_base_addr;
+
/* MAC Forcing register: don't force link, speed, duplex or flow control
* state to any particular values on physical ports, but force the CPU
* port and all DSA ports to their maximum bandwidth and full duplex.
diff --git a/include/net/dsa.h b/include/net/dsa.h
index b3eefe8e18fd..f9c9dc1f6d21 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -196,6 +196,7 @@ struct dsa_port {
struct dsa_switch *ds;
unsigned int index;
+ unsigned int phy_base_addr;
const char *name;
const struct dsa_port *cpu_dp;
struct device_node *dn;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a1c9fe155057..4f67dff34a3b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1221,7 +1221,8 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
/* We could not connect to a designated PHY or SFP, so use the
* switch internal MDIO bus instead
*/
- ret = dsa_slave_phy_connect(slave_dev, dp->index);
+ ret = dsa_slave_phy_connect(slave_dev, dp->phy_base_addr +
+ dp->index);
if (ret) {
netdev_err(slave_dev,
"failed to connect to port %d: %d\n",
--
2.11.0
^ permalink raw reply related
* Re: BUG: assuming atomic context at kernel/seccomp.c:LINE
From: Kees Cook @ 2019-02-20 18:23 UTC (permalink / raw)
To: Daniel Borkmann
Cc: syzbot, Alexei Starovoitov, kafai, LKML, Andy Lutomirski,
Network Development, Song Liu, syzkaller-bugs, Will Drewry,
Yonghong Song
In-Reply-To: <69ff36f9-8729-9b58-5595-1b35aa4a7825@iogearbox.net>
On Wed, Feb 20, 2019 at 2:00 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/20/2019 10:32 AM, syzbot wrote:
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit: abf446c90405 Add linux-next specific files for 20190220
> > git tree: linux-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=17f250d8c00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=463cb576ac40e350
> > dashboard link: https://syzkaller.appspot.com/bug?extid=8bf19ee2aa580de7a2a7
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> >
> > Unfortunately, I don't have any reproducer for this crash yet.
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> >
> > BUG: assuming atomic context at kernel/seccomp.c:271
> > in_atomic(): 0, irqs_disabled(): 0, pid: 12803, name: syz-executor.5
> > no locks held by syz-executor.5/12803.
> > CPU: 1 PID: 12803 Comm: syz-executor.5 Not tainted 5.0.0-rc7-next-20190220 #39
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> > __cant_sleep kernel/sched/core.c:6218 [inline]
> > __cant_sleep.cold+0xa3/0xbb kernel/sched/core.c:6195
> > seccomp_run_filters kernel/seccomp.c:271 [inline]
> > __seccomp_filter+0x12b/0x12b0 kernel/seccomp.c:801
> > __secure_computing+0x101/0x360 kernel/seccomp.c:932
> > syscall_trace_enter+0x5bf/0xe10 arch/x86/entry/common.c:120
> > do_syscall_64+0x479/0x610 arch/x86/entry/common.c:280
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> False positive; bpf-next only. Pushing this out in a bit:
>
> From d56547070162a105ff666f3324e558fa6492aedd Mon Sep 17 00:00:00 2001
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Wed, 20 Feb 2019 10:51:17 +0100
> Subject: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for
> cbpf->ebpf progs
>
> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> a check was added for BPF_PROG_RUN() that for every invocation preemption is
> disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this does
> not count for seccomp because only cBPF -> eBPF is loaded here and it does not
> make use of any functionality that would require this assertion. Fix this false
> positive by adding and using __BPF_PROG_RUN() variant that does not have the
> cant_sleep(); check.
>
> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> include/linux/filter.h | 9 ++++++++-
> kernel/seccomp.c | 2 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index f32b3ec..2f3e29a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -533,7 +533,14 @@ struct sk_filter {
> struct bpf_prog *prog;
> };
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +#define bpf_prog_run__non_preempt(prog, ctx) \
> + ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
> +#define BPF_PROG_RUN(prog, ctx) \
> + bpf_prog_run__non_preempt(prog, ctx)
> +/* cBPF -> eBPF only, but not for native eBPF. */
> +#define __BPF_PROG_RUN(prog, ctx) \
> + (*(prog)->bpf_func)(ctx, (prog)->insnsi)
>
> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index e815781..826d4e4 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
> * value always takes priority (ignoring the DATA).
> */
> for (; f; f = f->prev) {
> - u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
> + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd);
>
> if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) {
> ret = cur_ret;
> --
> 2.9.5
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Daniel Borkmann @ 2019-02-20 18:27 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: ast, netdev
In-Reply-To: <20190220170723.bbcj7bipsa6r7oy6@ast-mbp>
On 02/20/2019 06:07 PM, Alexei Starovoitov wrote:
> On Wed, Feb 20, 2019 at 12:06:29PM +0100, Daniel Borkmann wrote:
>> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
>> a check was added for BPF_PROG_RUN() that for every invocation preemption has
>> to be disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this
>> does not count for seccomp because only cBPF -> eBPF is loaded here and it does
>> not make use of any functionality that would require this assertion. Fix this
>> false positive by adding and using __BPF_PROG_RUN() variant that does not have
>> the cant_sleep(); check.
>>
>> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
>> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> include/linux/filter.h | 9 ++++++++-
>> kernel/seccomp.c | 2 +-
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index f32b3ec..2648fd7 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -533,7 +533,14 @@ struct sk_filter {
>> struct bpf_prog *prog;
>> };
>>
>> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
>> +#define bpf_prog_run__non_preempt(prog, ctx) \
>> + ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
>> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
>> +#define BPF_PROG_RUN(prog, ctx) \
>> + bpf_prog_run__non_preempt(prog, ctx)
>> +/* Direct use for cBPF -> eBPF only, but not for native eBPF. */
>
> I think the comment is too abstract.
> May be it should say that this is seccomp cBPF only ?
> And macro name should be explicit as well ?
I think macro naming is probably okay imho as used internally as
well from BPF_PROG_RUN(), but I'll improve the comment to state
seccomp specifically as an example there and providing some more
background.
>> +#define __BPF_PROG_RUN(prog, ctx) \
>> + (*(prog)->bpf_func)(ctx, (prog)->insnsi)
>>
>> #define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index e815781..826d4e4 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -268,7 +268,7 @@ static u32 seccomp_run_filters(const struct seccomp_data *sd,
>> * value always takes priority (ignoring the DATA).
>> */
>> for (; f; f = f->prev) {
>> - u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
>> + u32 cur_ret = __BPF_PROG_RUN(f->prog, sd);
>>
>> if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) {
>> ret = cur_ret;
>> --
>> 2.9.5
>>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, seccomp: fix false positive preemption splat for cbpf->ebpf progs
From: Alexei Starovoitov @ 2019-02-20 18:34 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, Network Development
In-Reply-To: <15b7c010-0635-35e4-dac8-0d811a496cd7@iogearbox.net>
On Wed, Feb 20, 2019 at 10:27 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/20/2019 06:07 PM, Alexei Starovoitov wrote:
> > On Wed, Feb 20, 2019 at 12:06:29PM +0100, Daniel Borkmann wrote:
> >> In 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> a check was added for BPF_PROG_RUN() that for every invocation preemption has
> >> to be disabled to not break eBPF assumptions (e.g. per-cpu map). Of course this
> >> does not count for seccomp because only cBPF -> eBPF is loaded here and it does
> >> not make use of any functionality that would require this assertion. Fix this
> >> false positive by adding and using __BPF_PROG_RUN() variant that does not have
> >> the cant_sleep(); check.
> >>
> >> Fixes: 568f196756ad ("bpf: check that BPF programs run with preemption disabled")
> >> Reported-by: syzbot+8bf19ee2aa580de7a2a7@syzkaller.appspotmail.com
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >> include/linux/filter.h | 9 ++++++++-
> >> kernel/seccomp.c | 2 +-
> >> 2 files changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >> index f32b3ec..2648fd7 100644
> >> --- a/include/linux/filter.h
> >> +++ b/include/linux/filter.h
> >> @@ -533,7 +533,14 @@ struct sk_filter {
> >> struct bpf_prog *prog;
> >> };
> >>
> >> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> >> +#define bpf_prog_run__non_preempt(prog, ctx) \
> >> + ({ cant_sleep(); __BPF_PROG_RUN(prog, ctx); })
> >> +/* Native eBPF or cBPF -> eBPF transitions. Preemption must be disabled. */
> >> +#define BPF_PROG_RUN(prog, ctx) \
> >> + bpf_prog_run__non_preempt(prog, ctx)
> >> +/* Direct use for cBPF -> eBPF only, but not for native eBPF. */
> >
> > I think the comment is too abstract.
> > May be it should say that this is seccomp cBPF only ?
> > And macro name should be explicit as well ?
>
> I think macro naming is probably okay imho as used internally as
> well from BPF_PROG_RUN(), but I'll improve the comment to state
> seccomp specifically as an example there and providing some more
> background.
I'm worried about misuse of the macro.
If there was a word seccomp in it it would made people
think much harder before calling it.
^ permalink raw reply
* Re: [RFC PATCH net-next v3 13/21] ethtool: provide timestamping information in GET_INFO request
From: Jakub Kicinski @ 2019-02-20 18:37 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, David Miller, Andrew Lunn, Jiri Pirko, linux-kernel
In-Reply-To: <20190220130007.GI23151@unicorn.suse.cz>
On Wed, 20 Feb 2019 14:00:07 +0100, Michal Kubecek wrote:
> On Tue, Feb 19, 2019 at 07:00:48PM -0800, Jakub Kicinski wrote:
> > On Mon, 18 Feb 2019 19:22:29 +0100 (CET), Michal Kubecek wrote:
> > > Add timestamping information as provided by ETHTOOL_GET_TS_INFO ioctl
> > > command in GET_INFO reply if ETH_INFO_IM_TSINFO flag is set in the request.
> > >
> > > Add constants for counts of HWTSTAMP_TX_* and HWTSTAM_FILTER_* constants
> > > and provide symbolic names for timestamping related values so that they can
> > > be retrieved in GET_STRSET and GET_INFO requests.
> >
> > What's the reason for providing the symbolic names?
>
> One of the the goals I had was to reduce the need to keep the lists of
> possible values in sync between kernel and userspace ethtool and other
> users of the interface so that when a new value is added, we don't have
> to update all userspace tools to be able to use or present it.
>
> This already works in ethtool for some newer commands (e.g. features)
> and obviously for those where the list of available options depends on
> the device (e.g. private flags or statistics). I would like to extend
> the principle also to older commands and new ones which do not work like
> this (e.g. device reset).
Let me try to argue that's the wrong direction. People should learn to
update their user space tooling if they want access to new features.
In my (limited) experience trying to solve forward compatibility leads
to short term gains, and long term warts in the APIs and increased
maintenance burden in the kernel.
^ permalink raw reply
* Re: [RFC] net: dsa: qca8k: CPU port broken with commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
From: Heiner Kallweit @ 2019-02-20 18:37 UTC (permalink / raw)
To: Michal Vokáč, Andrew Lunn, Vinod Koul
Cc: David S. Miller, Florian Fainelli, netdev
In-Reply-To: <d127618b-873e-e4aa-ea75-f747da5574ee@ysoft.com>
On 20.02.2019 16:02, Michal Vokáč wrote:
> Hi,
>
> Another issue in a row with networking on imx6dl-yapp4 platform [1]
> that uses QCA8334 Ethernet switch.
>
> Very recently, with Vinod and Andrew, we solved an issue with
> RGMII_ID mode by patch[2][3]. I tested those with next-20190215
> and it worked just fine.
>
> The patch[2] was merged into next-20190220 so I tested the latest version.
> Now the cpu port does not work again. I tracked it down to this commit
> 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in
> genphy_read_status") [4]
>
> If I revert the offending commit, cpu port works fine. I suspect the
> problem is on the qca8k driver side but I am not really sure.
> AFAICT autonegotiation is not available on the QCA833x cpu port (MAC0).
>
Patch[4] only affects a code path with AUTONEG_ENABLE == phydev->autoneg.
IMO this shouldn't be the case for the cpu port (like you state).
Seems like somewhere a phydev->autoneg = AUTONEG_DISABLE is missing.
Not clear to me is where the difference with this patch comes from.
It would be helpful to know (w/o patch[4]):
- which values the register reads in genphy_read_status() return
- which bits are set in phydev->advertising
> Any ideas what may be the root cause of the problem?
>
> Thank you,
> Michal
>
Heiner
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=87489ec3a77f3e01bcf0d46e353ae7112ec8c4f0
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=a968b5e9d5879f9535d6099505f9e14abcafb623
> [3] https://lore.kernel.org/patchwork/patch/1043817/
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=5502b218e001
>
^ permalink raw reply
* Re: [PATCH net] net: dsa: fix unintended change of bridge interface STP state
From: Florian Fainelli @ 2019-02-20 18:38 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Russell King, Vivien Didelot, David S. Miller, netdev
In-Reply-To: <20190220172715.GL13075@lunn.ch>
On 2/20/19 9:27 AM, Andrew Lunn wrote:
> On Wed, Feb 20, 2019 at 09:22:30AM -0800, Florian Fainelli wrote:
>> On 2/20/19 2:32 AM, Russell King wrote:
>>> When a DSA port is added to a bridge and brought up, the resulting STP
>>> state programmed into the hardware depends on the order that these
>>> operations are performed. However, the Linux bridge code believes that
>>> the port is in disabled mode.
>>>
>>> If the DSA port is first added to a bridge and then brought up, it will
>>> be in blocking mode. If it is brought up and then added to the bridge,
>>> it will be in disabled mode.
>>>
>>> This difference is caused by DSA always setting the STP mode in
>>> dsa_port_enable() whether or not this port is part of a bridge. Since
>>> bridge always sets the STP state when the port is added, brought up or
>>> taken down, it is unnecessary for us to manipulate the STP state.
>>>
>>> Apparently, this code was copied from Rocker, and the very next day a
>>> similar fix for Rocker was merged but was not propagated to DSA. See
>>> e47172ab7e41 ("rocker: put port in FORWADING state after leaving bridge")
>>>
>>> Fixes: b73adef67765 ("net: dsa: integrate with SWITCHDEV for HW bridging")
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>
>> Nice example of cargo cult programming, thanks for fixing this!
>
> Maybe now would be a good time to look at other drivers. Does the
> Microsemi Ocelot driver have the same issue?
Checked fsl-dpaa2/ethsw and ocelot and neither of those seem to be
affected by this problem.
--
Florian
^ 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