* [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: David Ahern @ 2017-04-19 21:19 UTC (permalink / raw)
To: netdev; +Cc: kafai, andreyknvl, David Ahern
Andrey reported a fault in the IPv6 route code:
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
Modules linked in:
CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff880069809600 task.stack: ffff880062dc8000
RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
RSP: 0018:ffff880062dced30 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: ffff8800670561c0 RCX: 0000000000000006
RDX: 0000000000000003 RSI: ffff880062dcfb28 RDI: 0000000000000018
RBP: ffff880062dced68 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff880062dcfb28 R14: dffffc0000000000 R15: 0000000000000000
FS: 00007feebe37e7c0(0000) GS:ffff88006cb00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000205a0fe4 CR3: 000000006b5c9000 CR4: 00000000000006e0
Call Trace:
ip6_pol_route+0x1512/0x1f20 net/ipv6/route.c:1128
ip6_pol_route_output+0x4c/0x60 net/ipv6/route.c:1212
...
Andrey's syzkaller program passes rtmsg.rtmsg_flags with the RTF_PCPU bit
set. Flags passed to the kernel are blindly copied to the allocated
rt6_info by ip6_route_info_create making a newly inserted route appear
as though it is a per-cpu route. ip6_rt_cache_alloc sees the flag set
and expects rt->dst.from to be set - which it is not since it is not
really a per-cpu copy. The subsequent call to __ip6_dst_alloc then
generates the fault.
Fix by checking for the flag and failing with EINVAL.
Fixes: d52d3997f843f ("ipv6: Create percpu rt6_info")
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
include/uapi/linux/ipv6_route.h | 2 +-
net/ipv6/route.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/ipv6_route.h b/include/uapi/linux/ipv6_route.h
index 85bbb1799df3..d496c02e14bc 100644
--- a/include/uapi/linux/ipv6_route.h
+++ b/include/uapi/linux/ipv6_route.h
@@ -35,7 +35,7 @@
#define RTF_PREF(pref) ((pref) << 27)
#define RTF_PREF_MASK 0x18000000
-#define RTF_PCPU 0x40000000
+#define RTF_PCPU 0x40000000 /* read-only: can not be set by user */
#define RTF_LOCAL 0x80000000
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4ba7c49872ff..a1bf426c959b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1854,6 +1854,10 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg)
int addr_type;
int err = -EINVAL;
+ /* RTF_PCPU is an internal flag; can not be set by userspace */
+ if (cfg->fc_flags & RTF_PCPU)
+ goto out;
+
if (cfg->fc_dst_len > 128 || cfg->fc_src_len > 128)
goto out;
#ifndef CONFIG_IPV6_SUBTREES
--
2.9.3
^ permalink raw reply related
* [Patch net-next v4 0/2] net_sched: clean up tc filter destroy and delete logic
From: Cong Wang @ 2017-04-19 21:21 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang
The first patch fixes a potenial race condition, the second one
is pure cleanup.
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
v4: split the patch and update changelog
v3: fix a compiler warning
v2: rebase
Cong Wang (2):
net_sched: move the empty tp check from ->destroy() to ->delete()
net_sched: remove useless NULL to tp->root
include/net/sch_generic.h | 4 +--
net/sched/cls_api.c | 27 +++++++++---------
net/sched/cls_basic.c | 10 +++----
net/sched/cls_bpf.c | 11 ++++----
net/sched/cls_cgroup.c | 8 ++----
net/sched/cls_flow.c | 10 +++----
net/sched/cls_flower.c | 10 ++-----
net/sched/cls_fw.c | 30 +++++++++++---------
net/sched/cls_matchall.c | 7 ++---
net/sched/cls_route.c | 44 ++++++++++-------------------
net/sched/cls_rsvp.h | 36 ++++++++++++------------
net/sched/cls_tcindex.c | 14 +++++-----
net/sched/cls_u32.c | 71 +++++++++++++++++++++++++++--------------------
13 files changed, 134 insertions(+), 148 deletions(-)
--
2.5.5
^ permalink raw reply
* [Patch net-next v4 1/2] net_sched: move the empty tp check from ->destroy() to ->delete()
From: Cong Wang @ 2017-04-19 21:21 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, Roi Dayan, Daniel Borkmann, John Fastabend,
Jamal Hadi Salim
In-Reply-To: <1492636882-19571-1-git-send-email-xiyou.wangcong@gmail.com>
We could have a race condition where in ->classify() path we
dereference tp->root and meanwhile a parallel ->destroy() makes it
a NULL. Daniel cured this bug in commit d936377414fa
("net, sched: respect rcu grace period on cls destruction").
This happens when ->destroy() is called for deleting a filter to
check if we are the last one in tp, this tp is still linked and
visible at that time. The root cause of this problem is the semantic
of ->destroy(), it does two things (for non-force case):
1) check if tp is empty
2) if tp is empty we could really destroy it
and its caller, if cares, needs to check its return value to see if it
is really destroyed. Therefore we can't unlink tp unless we know it is
empty.
As suggested by Daniel, we could actually move the test logic to ->delete()
so that we can safely unlink tp after ->delete() tells us the last one is
just deleted and before ->destroy().
Fixes: 1e052be69d04 ("net_sched: destroy proto tp when all filters are gone")
Cc: Roi Dayan <roid@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/sch_generic.h | 4 +--
net/sched/cls_api.c | 27 +++++++++---------
net/sched/cls_basic.c | 10 +++----
net/sched/cls_bpf.c | 11 ++++----
net/sched/cls_cgroup.c | 8 ++----
net/sched/cls_flow.c | 10 +++----
net/sched/cls_flower.c | 10 ++-----
net/sched/cls_fw.c | 29 +++++++++++--------
net/sched/cls_matchall.c | 7 ++---
net/sched/cls_route.c | 29 +++++++++----------
net/sched/cls_rsvp.h | 32 +++++++++++----------
net/sched/cls_tcindex.c | 14 +++++-----
net/sched/cls_u32.c | 71 +++++++++++++++++++++++++++--------------------
13 files changed, 134 insertions(+), 128 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 65d5026..22e5209 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -204,14 +204,14 @@ struct tcf_proto_ops {
const struct tcf_proto *,
struct tcf_result *);
int (*init)(struct tcf_proto*);
- bool (*destroy)(struct tcf_proto*, bool);
+ void (*destroy)(struct tcf_proto*);
unsigned long (*get)(struct tcf_proto*, u32 handle);
int (*change)(struct net *net, struct sk_buff *,
struct tcf_proto*, unsigned long,
u32 handle, struct nlattr **,
unsigned long *, bool);
- int (*delete)(struct tcf_proto*, unsigned long);
+ int (*delete)(struct tcf_proto*, unsigned long, bool*);
void (*walk)(struct tcf_proto*, struct tcf_walker *arg);
/* rtnetlink specific */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a8da383..22f88b3 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -178,14 +178,11 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
return ERR_PTR(err);
}
-static bool tcf_proto_destroy(struct tcf_proto *tp, bool force)
+static void tcf_proto_destroy(struct tcf_proto *tp)
{
- if (tp->ops->destroy(tp, force)) {
- module_put(tp->ops->owner);
- kfree_rcu(tp, rcu);
- return true;
- }
- return false;
+ tp->ops->destroy(tp);
+ module_put(tp->ops->owner);
+ kfree_rcu(tp, rcu);
}
void tcf_destroy_chain(struct tcf_proto __rcu **fl)
@@ -194,7 +191,7 @@ void tcf_destroy_chain(struct tcf_proto __rcu **fl)
while ((tp = rtnl_dereference(*fl)) != NULL) {
RCU_INIT_POINTER(*fl, tp->next);
- tcf_proto_destroy(tp, true);
+ tcf_proto_destroy(tp);
}
}
EXPORT_SYMBOL(tcf_destroy_chain);
@@ -361,7 +358,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
RCU_INIT_POINTER(*back, next);
tfilter_notify(net, skb, n, tp, fh,
RTM_DELTFILTER, false);
- tcf_proto_destroy(tp, true);
+ tcf_proto_destroy(tp);
err = 0;
goto errout;
}
@@ -372,24 +369,28 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
goto errout;
}
} else {
+ bool last;
+
switch (n->nlmsg_type) {
case RTM_NEWTFILTER:
if (n->nlmsg_flags & NLM_F_EXCL) {
if (tp_created)
- tcf_proto_destroy(tp, true);
+ tcf_proto_destroy(tp);
err = -EEXIST;
goto errout;
}
break;
case RTM_DELTFILTER:
- err = tp->ops->delete(tp, fh);
+ err = tp->ops->delete(tp, fh, &last);
if (err)
goto errout;
next = rtnl_dereference(tp->next);
tfilter_notify(net, skb, n, tp, t->tcm_handle,
RTM_DELTFILTER, false);
- if (tcf_proto_destroy(tp, false))
+ if (last) {
RCU_INIT_POINTER(*back, next);
+ tcf_proto_destroy(tp);
+ }
goto errout;
case RTM_GETTFILTER:
err = tfilter_notify(net, skb, n, tp, fh,
@@ -411,7 +412,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tfilter_notify(net, skb, n, tp, fh, RTM_NEWTFILTER, false);
} else {
if (tp_created)
- tcf_proto_destroy(tp, true);
+ tcf_proto_destroy(tp);
}
errout:
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 422414f..c4fd63a 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -93,30 +93,28 @@ static void basic_delete_filter(struct rcu_head *head)
kfree(f);
}
-static bool basic_destroy(struct tcf_proto *tp, bool force)
+static void basic_destroy(struct tcf_proto *tp)
{
struct basic_head *head = rtnl_dereference(tp->root);
struct basic_filter *f, *n;
- if (!force && !list_empty(&head->flist))
- return false;
-
list_for_each_entry_safe(f, n, &head->flist, link) {
list_del_rcu(&f->link);
tcf_unbind_filter(tp, &f->res);
call_rcu(&f->rcu, basic_delete_filter);
}
kfree_rcu(head, rcu);
- return true;
}
-static int basic_delete(struct tcf_proto *tp, unsigned long arg)
+static int basic_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
+ struct basic_head *head = rtnl_dereference(tp->root);
struct basic_filter *f = (struct basic_filter *) arg;
list_del_rcu(&f->link);
tcf_unbind_filter(tp, &f->res);
call_rcu(&f->rcu, basic_delete_filter);
+ *last = list_empty(&head->flist);
return 0;
}
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 7ddd08e..5ebeae9 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -274,25 +274,24 @@ static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog)
call_rcu(&prog->rcu, cls_bpf_delete_prog_rcu);
}
-static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg)
+static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
+ struct cls_bpf_head *head = rtnl_dereference(tp->root);
+
__cls_bpf_delete(tp, (struct cls_bpf_prog *) arg);
+ *last = list_empty(&head->plist);
return 0;
}
-static bool cls_bpf_destroy(struct tcf_proto *tp, bool force)
+static void cls_bpf_destroy(struct tcf_proto *tp)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
struct cls_bpf_prog *prog, *tmp;
- if (!force && !list_empty(&head->plist))
- return false;
-
list_for_each_entry_safe(prog, tmp, &head->plist, link)
__cls_bpf_delete(tp, prog);
kfree_rcu(head, rcu);
- return true;
}
static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index b5e7c1b..12ce547 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -131,20 +131,16 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static bool cls_cgroup_destroy(struct tcf_proto *tp, bool force)
+static void cls_cgroup_destroy(struct tcf_proto *tp)
{
struct cls_cgroup_head *head = rtnl_dereference(tp->root);
- if (!force)
- return false;
/* Head can still be NULL due to cls_cgroup_init(). */
if (head)
call_rcu(&head->rcu, cls_cgroup_destroy_rcu);
-
- return true;
}
-static int cls_cgroup_delete(struct tcf_proto *tp, unsigned long arg)
+static int cls_cgroup_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
return -EOPNOTSUPP;
}
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 008ba7e..3065752 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -562,12 +562,14 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static int flow_delete(struct tcf_proto *tp, unsigned long arg)
+static int flow_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
+ struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f = (struct flow_filter *)arg;
list_del_rcu(&f->list);
call_rcu(&f->rcu, flow_destroy_filter);
+ *last = list_empty(&head->filters);
return 0;
}
@@ -583,20 +585,16 @@ static int flow_init(struct tcf_proto *tp)
return 0;
}
-static bool flow_destroy(struct tcf_proto *tp, bool force)
+static void flow_destroy(struct tcf_proto *tp)
{
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f, *next;
- if (!force && !list_empty(&head->filters))
- return false;
-
list_for_each_entry_safe(f, next, &head->filters, list) {
list_del_rcu(&f->list);
call_rcu(&f->rcu, flow_destroy_filter);
}
kfree_rcu(head, rcu);
- return true;
}
static unsigned long flow_get(struct tcf_proto *tp, u32 handle)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 3e7bd78..31ee340 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -328,21 +328,16 @@ static void fl_destroy_rcu(struct rcu_head *rcu)
schedule_work(&head->work);
}
-static bool fl_destroy(struct tcf_proto *tp, bool force)
+static void fl_destroy(struct tcf_proto *tp)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct cls_fl_filter *f, *next;
- if (!force && !list_empty(&head->filters))
- return false;
-
list_for_each_entry_safe(f, next, &head->filters, list)
__fl_delete(tp, f);
__module_get(THIS_MODULE);
call_rcu(&head->rcu, fl_destroy_rcu);
-
- return true;
}
static unsigned long fl_get(struct tcf_proto *tp, u32 handle)
@@ -947,7 +942,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static int fl_delete(struct tcf_proto *tp, unsigned long arg)
+static int fl_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct cls_fl_filter *f = (struct cls_fl_filter *) arg;
@@ -956,6 +951,7 @@ static int fl_delete(struct tcf_proto *tp, unsigned long arg)
rhashtable_remove_fast(&head->ht, &f->ht_node,
head->ht_params);
__fl_delete(tp, f);
+ *last = list_empty(&head->filters);
return 0;
}
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 9962090..78ccb4a 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -127,20 +127,14 @@ static void fw_delete_filter(struct rcu_head *head)
kfree(f);
}
-static bool fw_destroy(struct tcf_proto *tp, bool force)
+static void fw_destroy(struct tcf_proto *tp)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f;
int h;
if (head == NULL)
- return true;
-
- if (!force) {
- for (h = 0; h < HTSIZE; h++)
- if (rcu_access_pointer(head->ht[h]))
- return false;
- }
+ return;
for (h = 0; h < HTSIZE; h++) {
while ((f = rtnl_dereference(head->ht[h])) != NULL) {
@@ -152,15 +146,16 @@ static bool fw_destroy(struct tcf_proto *tp, bool force)
}
RCU_INIT_POINTER(tp->root, NULL);
kfree_rcu(head, rcu);
- return true;
}
-static int fw_delete(struct tcf_proto *tp, unsigned long arg)
+static int fw_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f = (struct fw_filter *)arg;
struct fw_filter __rcu **fp;
struct fw_filter *pfp;
+ int ret = -EINVAL;
+ int h;
if (head == NULL || f == NULL)
goto out;
@@ -173,11 +168,21 @@ static int fw_delete(struct tcf_proto *tp, unsigned long arg)
RCU_INIT_POINTER(*fp, rtnl_dereference(f->next));
tcf_unbind_filter(tp, &f->res);
call_rcu(&f->rcu, fw_delete_filter);
- return 0;
+ ret = 0;
+ break;
}
}
+
+ *last = true;
+ for (h = 0; h < HTSIZE; h++) {
+ if (rcu_access_pointer(head->ht[h])) {
+ *last = false;
+ break;
+ }
+ }
+
out:
- return -EINVAL;
+ return ret;
}
static const struct nla_policy fw_policy[TCA_FW_MAX + 1] = {
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 0dbcca6..2efb36c 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -90,19 +90,18 @@ static void mall_destroy_hw_filter(struct tcf_proto *tp,
&offload);
}
-static bool mall_destroy(struct tcf_proto *tp, bool force)
+static void mall_destroy(struct tcf_proto *tp)
{
struct cls_mall_head *head = rtnl_dereference(tp->root);
struct net_device *dev = tp->q->dev_queue->dev;
if (!head)
- return true;
+ return;
if (tc_should_offload(dev, tp, head->flags))
mall_destroy_hw_filter(tp, head, (unsigned long) head);
call_rcu(&head->rcu, mall_destroy_rcu);
- return true;
}
static unsigned long mall_get(struct tcf_proto *tp, u32 handle)
@@ -216,7 +215,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static int mall_delete(struct tcf_proto *tp, unsigned long arg)
+static int mall_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
return -EOPNOTSUPP;
}
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index a371075..f4d687e 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -276,20 +276,13 @@ static void route4_delete_filter(struct rcu_head *head)
kfree(f);
}
-static bool route4_destroy(struct tcf_proto *tp, bool force)
+static void route4_destroy(struct tcf_proto *tp)
{
struct route4_head *head = rtnl_dereference(tp->root);
int h1, h2;
if (head == NULL)
- return true;
-
- if (!force) {
- for (h1 = 0; h1 <= 256; h1++) {
- if (rcu_access_pointer(head->table[h1]))
- return false;
- }
- }
+ return;
for (h1 = 0; h1 <= 256; h1++) {
struct route4_bucket *b;
@@ -314,10 +307,9 @@ static bool route4_destroy(struct tcf_proto *tp, bool force)
}
RCU_INIT_POINTER(tp->root, NULL);
kfree_rcu(head, rcu);
- return true;
}
-static int route4_delete(struct tcf_proto *tp, unsigned long arg)
+static int route4_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct route4_head *head = rtnl_dereference(tp->root);
struct route4_filter *f = (struct route4_filter *)arg;
@@ -325,7 +317,7 @@ static int route4_delete(struct tcf_proto *tp, unsigned long arg)
struct route4_filter *nf;
struct route4_bucket *b;
unsigned int h = 0;
- int i;
+ int i, h1;
if (!head || !f)
return -EINVAL;
@@ -356,16 +348,25 @@ static int route4_delete(struct tcf_proto *tp, unsigned long arg)
rt = rtnl_dereference(b->ht[i]);
if (rt)
- return 0;
+ goto out;
}
/* OK, session has no flows */
RCU_INIT_POINTER(head->table[to_hash(h)], NULL);
kfree_rcu(b, rcu);
+ break;
+ }
+ }
- return 0;
+out:
+ *last = true;
+ for (h1 = 0; h1 <= 256; h1++) {
+ if (rcu_access_pointer(head->table[h1])) {
+ *last = false;
+ break;
}
}
+
return 0;
}
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index d7f2923..18a9470 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -302,20 +302,13 @@ static void rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
call_rcu(&f->rcu, rsvp_delete_filter_rcu);
}
-static bool rsvp_destroy(struct tcf_proto *tp, bool force)
+static void rsvp_destroy(struct tcf_proto *tp)
{
struct rsvp_head *data = rtnl_dereference(tp->root);
int h1, h2;
if (data == NULL)
- return true;
-
- if (!force) {
- for (h1 = 0; h1 < 256; h1++) {
- if (rcu_access_pointer(data->ht[h1]))
- return false;
- }
- }
+ return;
RCU_INIT_POINTER(tp->root, NULL);
@@ -337,10 +330,9 @@ static bool rsvp_destroy(struct tcf_proto *tp, bool force)
}
}
kfree_rcu(data, rcu);
- return true;
}
-static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
+static int rsvp_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct rsvp_head *head = rtnl_dereference(tp->root);
struct rsvp_filter *nfp, *f = (struct rsvp_filter *)arg;
@@ -348,7 +340,7 @@ static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
unsigned int h = f->handle;
struct rsvp_session __rcu **sp;
struct rsvp_session *nsp, *s = f->sess;
- int i;
+ int i, h1;
fp = &s->ht[(h >> 8) & 0xFF];
for (nfp = rtnl_dereference(*fp); nfp;
@@ -361,7 +353,7 @@ static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
for (i = 0; i <= 16; i++)
if (s->ht[i])
- return 0;
+ goto out;
/* OK, session has no flows */
sp = &head->ht[h & 0xFF];
@@ -370,13 +362,23 @@ static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
if (nsp == s) {
RCU_INIT_POINTER(*sp, s->next);
kfree_rcu(s, rcu);
- return 0;
+ goto out;
}
}
- return 0;
+ break;
}
}
+
+out:
+ *last = true;
+ for (h1 = 0; h1 < 256; h1++) {
+ if (rcu_access_pointer(head->ht[h1])) {
+ *last = false;
+ break;
+ }
+ }
+
return 0;
}
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 2ab0013..8a8a583 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -150,7 +150,7 @@ static void tcindex_destroy_fexts(struct rcu_head *head)
kfree(f);
}
-static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
+static int tcindex_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
struct tcindex_filter_result *r = (struct tcindex_filter_result *) arg;
@@ -186,6 +186,8 @@ static int tcindex_delete(struct tcf_proto *tp, unsigned long arg)
call_rcu(&f->rcu, tcindex_destroy_fexts);
else
call_rcu(&r->rcu, tcindex_destroy_rexts);
+
+ *last = false;
return 0;
}
@@ -193,7 +195,9 @@ static int tcindex_destroy_element(struct tcf_proto *tp,
unsigned long arg,
struct tcf_walker *walker)
{
- return tcindex_delete(tp, arg);
+ bool last;
+
+ return tcindex_delete(tp, arg, &last);
}
static void __tcindex_destroy(struct rcu_head *head)
@@ -529,14 +533,11 @@ static void tcindex_walk(struct tcf_proto *tp, struct tcf_walker *walker)
}
}
-static bool tcindex_destroy(struct tcf_proto *tp, bool force)
+static void tcindex_destroy(struct tcf_proto *tp)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
struct tcf_walker walker;
- if (!force)
- return false;
-
pr_debug("tcindex_destroy(tp %p),p %p\n", tp, p);
walker.count = 0;
walker.skip = 0;
@@ -544,7 +545,6 @@ static bool tcindex_destroy(struct tcf_proto *tp, bool force)
tcindex_walk(tp, &walker);
call_rcu(&p->rcu, __tcindex_destroy);
- return true;
}
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9e2f330..d20e72a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -585,37 +585,13 @@ static bool ht_empty(struct tc_u_hnode *ht)
return true;
}
-static bool u32_destroy(struct tcf_proto *tp, bool force)
+static void u32_destroy(struct tcf_proto *tp)
{
struct tc_u_common *tp_c = tp->data;
struct tc_u_hnode *root_ht = rtnl_dereference(tp->root);
WARN_ON(root_ht == NULL);
- if (!force) {
- if (root_ht) {
- if (root_ht->refcnt > 1)
- return false;
- if (root_ht->refcnt == 1) {
- if (!ht_empty(root_ht))
- return false;
- }
- }
-
- if (tp_c->refcnt > 1)
- return false;
-
- if (tp_c->refcnt == 1) {
- struct tc_u_hnode *ht;
-
- for (ht = rtnl_dereference(tp_c->hlist);
- ht;
- ht = rtnl_dereference(ht->next))
- if (!ht_empty(ht))
- return false;
- }
- }
-
if (root_ht && --root_ht->refcnt == 0)
u32_destroy_hnode(tp, root_ht);
@@ -640,20 +616,22 @@ static bool u32_destroy(struct tcf_proto *tp, bool force)
}
tp->data = NULL;
- return true;
}
-static int u32_delete(struct tcf_proto *tp, unsigned long arg)
+static int u32_delete(struct tcf_proto *tp, unsigned long arg, bool *last)
{
struct tc_u_hnode *ht = (struct tc_u_hnode *)arg;
struct tc_u_hnode *root_ht = rtnl_dereference(tp->root);
+ struct tc_u_common *tp_c = tp->data;
+ int ret = 0;
if (ht == NULL)
- return 0;
+ goto out;
if (TC_U32_KEY(ht->handle)) {
u32_remove_hw_knode(tp, ht->handle);
- return u32_delete_key(tp, (struct tc_u_knode *)ht);
+ ret = u32_delete_key(tp, (struct tc_u_knode *)ht);
+ goto out;
}
if (root_ht == ht)
@@ -666,7 +644,40 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
return -EBUSY;
}
- return 0;
+out:
+ *last = true;
+ if (root_ht) {
+ if (root_ht->refcnt > 1) {
+ *last = false;
+ goto ret;
+ }
+ if (root_ht->refcnt == 1) {
+ if (!ht_empty(root_ht)) {
+ *last = false;
+ goto ret;
+ }
+ }
+ }
+
+ if (tp_c->refcnt > 1) {
+ *last = false;
+ goto ret;
+ }
+
+ if (tp_c->refcnt == 1) {
+ struct tc_u_hnode *ht;
+
+ for (ht = rtnl_dereference(tp_c->hlist);
+ ht;
+ ht = rtnl_dereference(ht->next))
+ if (!ht_empty(ht)) {
+ *last = false;
+ break;
+ }
+ }
+
+ret:
+ return ret;
}
#define NR_U32_NODE (1<<12)
--
2.5.5
^ permalink raw reply related
* [Patch net-next v4 2/2] net_sched: remove useless NULL to tp->root
From: Cong Wang @ 2017-04-19 21:21 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Daniel Borkmann, John Fastabend, Jamal Hadi Salim
In-Reply-To: <1492636882-19571-1-git-send-email-xiyou.wangcong@gmail.com>
There is no need to NULL tp->root in ->destroy(), since tp is
going to be freed very soon, and existing readers are still
safe to read them.
For cls_route, we always init its tp->root, so it can't be NULL,
we can drop more useless code.
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_fw.c | 1 -
net/sched/cls_route.c | 15 ---------------
net/sched/cls_rsvp.h | 4 ----
3 files changed, 20 deletions(-)
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 78ccb4a..d388536 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -144,7 +144,6 @@ static void fw_destroy(struct tcf_proto *tp)
call_rcu(&f->rcu, fw_delete_filter);
}
}
- RCU_INIT_POINTER(tp->root, NULL);
kfree_rcu(head, rcu);
}
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index f4d687e..d63d550 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -140,8 +140,6 @@ static int route4_classify(struct sk_buff *skb, const struct tcf_proto *tp,
goto failure;
id = dst->tclassid;
- if (head == NULL)
- goto old_method;
iif = inet_iif(skb);
@@ -194,15 +192,6 @@ static int route4_classify(struct sk_buff *skb, const struct tcf_proto *tp,
route4_set_fastmap(head, id, iif, ROUTE4_FAILURE);
failure:
return -1;
-
-old_method:
- if (id && (TC_H_MAJ(id) == 0 ||
- !(TC_H_MAJ(id^tp->q->handle)))) {
- res->classid = id;
- res->class = 0;
- return 0;
- }
- return -1;
}
static inline u32 to_hash(u32 id)
@@ -234,9 +223,6 @@ static unsigned long route4_get(struct tcf_proto *tp, u32 handle)
struct route4_filter *f;
unsigned int h1, h2;
- if (!head)
- return 0;
-
h1 = to_hash(handle);
if (h1 > 256)
return 0;
@@ -305,7 +291,6 @@ static void route4_destroy(struct tcf_proto *tp)
kfree_rcu(b, rcu);
}
}
- RCU_INIT_POINTER(tp->root, NULL);
kfree_rcu(head, rcu);
}
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 18a9470..0d9d077 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -152,8 +152,6 @@ static int rsvp_classify(struct sk_buff *skb, const struct tcf_proto *tp,
return -1;
nhptr = ip_hdr(skb);
#endif
- if (unlikely(!head))
- return -1;
restart:
#if RSVP_DST_LEN == 4
@@ -310,8 +308,6 @@ static void rsvp_destroy(struct tcf_proto *tp)
if (data == NULL)
return;
- RCU_INIT_POINTER(tp->root, NULL);
-
for (h1 = 0; h1 < 256; h1++) {
struct rsvp_session *s;
--
2.5.5
^ permalink raw reply related
* [PATCH v2 net-next]smsc911x: Adding support for Micochip LAN9250 Ethernet controller
From: David.Cai @ 2017-04-19 21:26 UTC (permalink / raw)
To: netdev, davem; +Cc: UNGLinuxDriver, steve.glendinning
Adding support for Microchip LAN9250 Ethernet controller.
Signed-off-by: David Cai <david.cai@microchip.com>
---
Changes
V2
- email format changed
- remove unnecessary text in commit log
drivers/net/ethernet/smsc/smsc911x.c | 32 +++++++++++++++++++++++++++-----
drivers/net/ethernet/smsc/smsc911x.h | 3 +++
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index fa5ca09..22b1951 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -25,7 +25,7 @@
* LAN9215, LAN9216, LAN9217, LAN9218
* LAN9210, LAN9211
* LAN9220, LAN9221
- * LAN89218
+ * LAN89218,LAN9250
*
*/
@@ -104,6 +104,9 @@ struct smsc911x_data {
/* used to decide which workarounds apply */
unsigned int generation;
+ /* used to decide which sub generation product work arounds to apply */
+ unsigned int sub_generation;
+
/* device configuration (copied from platform_data during probe) */
struct smsc911x_platform_config config;
@@ -1450,6 +1453,8 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
unsigned int timeout;
unsigned int temp;
int ret;
+ unsigned int reset_offset = HW_CFG;
+ unsigned int reset_mask = HW_CFG_SRST_;
/*
* Make sure to power-up the PHY chip before doing a reset, otherwise
@@ -1476,15 +1481,23 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
}
}
+ if (pdata->sub_generation) {
+ /* special reset for LAN9250 */
+ reset_offset = RESET_CTL;
+ reset_mask = RESET_CTL_DIGITAL_RST_;
+ }
+
/* Reset the LAN911x */
- smsc911x_reg_write(pdata, HW_CFG, HW_CFG_SRST_);
+ smsc911x_reg_write(pdata, reset_offset, reset_mask);
+
+ /* verify reset bit is cleared */
timeout = 10;
do {
udelay(10);
- temp = smsc911x_reg_read(pdata, HW_CFG);
- } while ((--timeout) && (temp & HW_CFG_SRST_));
+ temp = smsc911x_reg_read(pdata, reset_offset);
+ } while ((--timeout) && (temp & reset_mask));
- if (unlikely(temp & HW_CFG_SRST_)) {
+ if (unlikely(temp & reset_mask)) {
SMSC_WARN(pdata, drv, "Failed to complete reset");
return -EIO;
}
@@ -2251,6 +2264,9 @@ static int smsc911x_init(struct net_device *dev)
/* Default generation to zero (all workarounds apply) */
pdata->generation = 0;
+ /* Default sub_generation to zero */
+ pdata->sub_generation = 0;
+
pdata->idrev = smsc911x_reg_read(pdata, ID_REV);
switch (pdata->idrev & 0xFFFF0000) {
case 0x01180000:
@@ -2278,6 +2294,12 @@ static int smsc911x_init(struct net_device *dev)
pdata->generation = 4;
break;
+ case 0x92500000:
+ /* LAN9250 */
+ pdata->generation = 4;
+ pdata->sub_generation = 1;
+ break;
+
default:
SMSC_WARN(pdata, probe, "LAN911x not identified, idrev: 0x%08X",
pdata->idrev);
diff --git a/drivers/net/ethernet/smsc/smsc911x.h b/drivers/net/ethernet/smsc/smsc911x.h
index 54d6489..6b2d479 100644
--- a/drivers/net/ethernet/smsc/smsc911x.h
+++ b/drivers/net/ethernet/smsc/smsc911x.h
@@ -303,6 +303,9 @@
#define E2P_DATA_EEPROM_DATA_ 0x000000FF
#define LAN_REGISTER_EXTENT 0x00000100
+#define RESET_CTL 0x1F8
+#define RESET_CTL_DIGITAL_RST_ 0x00000001
+
/*
* MAC Control and Status Register (Indirect Address)
* Offset (through the MAC_CSR CMD and DATA port)
--
2.7.4
^ permalink raw reply related
* __sk_buff.data_end
From: Johannes Berg @ 2017-04-19 21:31 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev
Hi Alexei, Daniel,
I'm looking at adding the __wifi_sk_buff I talked about, and I notice
that it uses CB space to store data_end. Unfortunately, in a lot of
cases, we don't have any CB space to spare in wifi.
Is there any way to generate a series of instructions that instead does
the necessary calculations? I don't actually *see* such a way, because
I don't see how I could have a scratch register or scratch stack space,
but perhaps there's a way to do it?
johannes
^ permalink raw reply
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Cong Wang @ 2017-04-19 21:32 UTC (permalink / raw)
To: Wolfgang Bumiller
Cc: Jamal Hadi Salim, Linux Kernel Network Developers,
David S. Miller
In-Reply-To: <1244443915.139.1492614192777@webmail.proxmox.com>
On Wed, Apr 19, 2017 at 8:03 AM, Wolfgang Bumiller
<w.bumiller@proxmox.com> wrote:
>> On April 19, 2017 at 1:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> My suggestion:
>> If we move the cookie allocation before init we can save it and
>> only when init succeeds do we attach it to the action, otherwise
>> we free it on error path.
>
> Shouldn't the old code have freed an old a->act_cookie as well before
> replacing it then? nla_memdup_cookie() starts off by assigning a->act_cookie.
>
Yeah. Looks even better, we can totally avoid rollback the action modification
rollback on failure.
^ permalink raw reply
* Re: [PATCH v2] sh_eth: unmap DMA buffers when freeing rings
From: David Miller @ 2017-04-19 21:36 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: netdev, linux-renesas-soc, horms
In-Reply-To: <de18ee2d-b401-5542-7901-c35cee0adc70@cogentembedded.com>
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Wed, 19 Apr 2017 22:09:51 +0300
> On 04/17/2017 11:10 PM, David Miller wrote:
>
>>> The DMA API debugging (when enabled) causes:
>>>
>>> WARNING: CPU: 0 PID: 1445 at lib/dma-debug.c:519
>>> add_dma_entry+0xe0/0x12c
>>> DMA-API: exceeded 7 overlapping mappings of cacheline 0x01b2974d
>>>
>>> to be printed after repeated initialization of the Ether device, e.g.
>>> suspend/resume or 'ifconfig' up/down. This is because DMA buffers
>>> mapped
>>> using dma_map_single() in sh_eth_ring_format() and sh_eth_start_xmit()
>>> are
>>> never unmapped. Resolve this problem by unmapping the buffers when
>>> freeing
>>> the descriptor rings; in order to do it right, we'd have to add an
>>> extra
>>> parameter to sh_eth_txfree() (we rename this function to
>>> sh_eth_tx_free(),
>>> while at it).
>>>
>>> Based on the commit a47b70ea86bd ("ravb: unmap descriptors when
>>> freeing
>>> rings").
>>>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> Applied, thanks.
>
> Please don;t forget to send it to -stable.
> The bug seems to be there from the very beginning.
Ok, queued up.
^ permalink raw reply
* Re: XDP question: best API for returning/setting egress port?
From: Daniel Borkmann @ 2017-04-19 21:42 UTC (permalink / raw)
To: Andy Gospodarek, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, Alexei Starovoitov,
netdev@vger.kernel.org, xdp-newbies@vger.kernel.org,
John Fastabend
In-Reply-To: <20170419200259.GK4730@C02RW35GFVH8.dhcp.broadcom.net>
On 04/19/2017 10:02 PM, Andy Gospodarek wrote:
> On Tue, Apr 18, 2017 at 09:58:56PM +0200, Jesper Dangaard Brouer wrote:
>>
>> As I argued in NetConf presentation[1] (from slide #9) we need a port
>> mapping table (instead of using ifindex'es). Both for supporting
>> other "port" types than net_devices (think sockets), and for
>> sandboxing what XDP can bypass.
>>
>> I want to create a new XDP action called XDP_REDIRECT, that instruct
>> XDP to send the xdp_buff to another "port" (get translated into a
>> net_device, or something else depending on internal port type).
>>
>> Looking at the userspace/eBPF interface, I'm wondering what is the
>> best API for "returning" this port number from eBPF?
>>
>> The options I see is:
>>
>> 1) Split-up the u32 action code, and e.g let the high-16-bit be the
>> port number and lower-16bit the (existing) action verdict.
>>
>> Pros: Simple API
>> Cons: Number of ports limited to 64K
>
> Practically speaking this may be seem reserving 64k for the port number
> might be enough space, but I would also like to see a new return option
> for flags so I start to get concerned about space. Daniel also
> hightlights the fact that encoding the port in the action may does not
> leave room for flags and could get confusing.
>
> One unfortunate side-effect of dropping or transmitting frames with XDP
> is that we lose the opportunity to statistically sample in netfilter
> since the frames were dropped so early and I'd like to bring that back
> with a call to parse flags and possibly call psample_sample_packet()
> after the xdp action. Packet sampling cannot simply be an action
> since there are times when a frame should be dropped but should also be
> sampled, so it seems logical to add this as a flag.
Hmm, you can do that already today with bpf_xdp_event_output() helper
and the program decides when to sample and what custom meta data to
prepend along with the (partial or full) xdp packet, see also [1], slide
11 for the "XDP dump" part.
No need to change drivers for this, psample_sample_packet() would also
be slower.
[1] http://netdevconf.org/2.1/slides/apr6/zhou-netdev-xdp-2017.pdf
^ permalink raw reply
* Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an assert step mechanism
From: Mickaël Salaün @ 2017-04-19 21:51 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5j+D-FP8Kt9unNOqKrQJP4DYTpmgkJxWykZyrYiVPz3Y3Q@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3006 bytes --]
On 19/04/2017 02:02, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> This is useful to return an information about the error without being
>> able to write to TH_LOG_STREAM.
>>
>> Helpers from test_harness.h may be useful outside of the seccomp
>> directory.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: Will Drewry <wad@chromium.org>
>> ---
>> tools/testing/selftests/seccomp/test_harness.h | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/seccomp/test_harness.h b/tools/testing/selftests/seccomp/test_harness.h
>> index a786c69c7584..77e407663e06 100644
>> --- a/tools/testing/selftests/seccomp/test_harness.h
>> +++ b/tools/testing/selftests/seccomp/test_harness.h
>> @@ -397,7 +397,7 @@ struct __test_metadata {
>> const char *name;
>> void (*fn)(struct __test_metadata *);
>> int termsig;
>> - int passed;
>> + __s8 passed;
>
> Why the reduction here? int is signed too?
Because the return code of a process is capped to 8 bits and I use a
negative value to not mess with the current interpretation of 0 (error)
and 1 (OK) for the "passed" variable.
>
>> int trigger; /* extra handler after the evaluation */
>> struct __test_metadata *prev, *next;
>> };
>> @@ -476,6 +476,12 @@ void __run_test(struct __test_metadata *t)
>> "instead of by signal (code: %d)\n",
>> t->name,
>> WEXITSTATUS(status));
>> + } else if (t->passed < 0) {
>> + fprintf(TH_LOG_STREAM,
>> + "%s: Failed at step #%d\n",
>> + t->name,
>> + t->passed * -1);
>> + t->passed = 0;
>> }
>
> Instead of creating an overloaded mechanism here, perhaps have an
> option reporting mechanism that can be enabled. Like adding to
> __test_metadata "bool no_stream; int test_number;" and adding
> test_number++ to each ASSERT/EXCEPT call, and doing something like:
>
> if (t->no_stream) {
> fprintf(TH_LOG_STREAM,
> "%s: Failed at step #%d\n",
> t->name,
> t->test_number);
> }
>
> It'd be a cleaner approach, maybe?
Good idea, we will then be able to use 255 steps!
Do you want me to send this as a separate patch?
Can we move test_harness.h outside of the seccomp directory to be
available to other subsystems as well?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: Martin KaFai Lau @ 2017-04-19 21:52 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, andreyknvl
In-Reply-To: <1492636783-29756-1-git-send-email-dsa@cumulusnetworks.com>
On Wed, Apr 19, 2017 at 02:19:43PM -0700, David Ahern wrote:
> Andrey reported a fault in the IPv6 route code:
>
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff880069809600 task.stack: ffff880062dc8000
> RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
> RSP: 0018:ffff880062dced30 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff8800670561c0 RCX: 0000000000000006
> RDX: 0000000000000003 RSI: ffff880062dcfb28 RDI: 0000000000000018
> RBP: ffff880062dced68 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff880062dcfb28 R14: dffffc0000000000 R15: 0000000000000000
> FS: 00007feebe37e7c0(0000) GS:ffff88006cb00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000205a0fe4 CR3: 000000006b5c9000 CR4: 00000000000006e0
> Call Trace:
> ip6_pol_route+0x1512/0x1f20 net/ipv6/route.c:1128
> ip6_pol_route_output+0x4c/0x60 net/ipv6/route.c:1212
> ...
>
> Andrey's syzkaller program passes rtmsg.rtmsg_flags with the RTF_PCPU bit
> set. Flags passed to the kernel are blindly copied to the allocated
> rt6_info by ip6_route_info_create making a newly inserted route appear
> as though it is a per-cpu route. ip6_rt_cache_alloc sees the flag set
> and expects rt->dst.from to be set - which it is not since it is not
> really a per-cpu copy. The subsequent call to __ip6_dst_alloc then
> generates the fault.
>
> Fix by checking for the flag and failing with EINVAL.
Thanks for the fix.
Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> Fixes: d52d3997f843f ("ipv6: Create percpu rt6_info")
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
> include/uapi/linux/ipv6_route.h | 2 +-
> net/ipv6/route.c | 4 ++++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/ipv6_route.h b/include/uapi/linux/ipv6_route.h
> index 85bbb1799df3..d496c02e14bc 100644
> --- a/include/uapi/linux/ipv6_route.h
> +++ b/include/uapi/linux/ipv6_route.h
> @@ -35,7 +35,7 @@
> #define RTF_PREF(pref) ((pref) << 27)
> #define RTF_PREF_MASK 0x18000000
>
> -#define RTF_PCPU 0x40000000
> +#define RTF_PCPU 0x40000000 /* read-only: can not be set by user */
> #define RTF_LOCAL 0x80000000
>
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4ba7c49872ff..a1bf426c959b 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1854,6 +1854,10 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg)
> int addr_type;
> int err = -EINVAL;
>
> + /* RTF_PCPU is an internal flag; can not be set by userspace */
> + if (cfg->fc_flags & RTF_PCPU)
> + goto out;
> +
> if (cfg->fc_dst_len > 128 || cfg->fc_src_len > 128)
> goto out;
> #ifndef CONFIG_IPV6_SUBTREES
> --
> 2.9.3
>
^ permalink raw reply
* Re: [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
From: Martin KaFai Lau @ 2017-04-19 21:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Saeed Mahameed, kernel-team
In-Reply-To: <1492633478.22296.14.camel@edumazet-glaptop3.roam.corp.google.com>
On Wed, Apr 19, 2017 at 01:24:38PM -0700, Eric Dumazet wrote:
> On Wed, 2017-04-19 at 11:29 -0700, Martin KaFai Lau wrote:
> > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
> > reported under /proc/net/dev. It seems there is a race in
> > mlx5e_update_stats() and some of the get-stats functions (the
> > one that we hit is the mlx5e_get_stats() which is called
> > by ndo_get_stats64()).
> >
> > In particular, the very first thing mlx5e_update_sw_counters()
> > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats()
> > is unlucky at one point, rx_bytes and rx_packets could be 0. One second
> > later, a normal (and much bigger than 0) value will be reported.
> >
> > This patch is not meant to be a proper fix. It merely tries
> > to show what I have suspected and start the discussion.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 +++++--
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > index a004a5a1a4c2..d24916f720bb 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > @@ -313,7 +313,6 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > mutex_lock(&priv->state_lock);
> > if (test_bit(MLX5E_STATE_OPENED, &priv->state))
> > mlx5e_update_stats(priv);
> > - mutex_unlock(&priv->state_lock);
> >
> > for (i = 0; i < NUM_SW_COUNTERS; i++)
> > data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
> > @@ -378,8 +377,10 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > data[idx++] = MLX5E_READ_CTR64_CPU(mlx5_priv->pme_stats.error_counters,
> > mlx5e_pme_error_desc, i);
> >
> > - if (!test_bit(MLX5E_STATE_OPENED, &priv->state))
> > + if (!test_bit(MLX5E_STATE_OPENED, &priv->state)) {
> > + mutex_unlock(&priv->state_lock);
> > return;
> > + }
> >
> > /* per channel counters */
> > for (i = 0; i < priv->params.num_channels; i++)
> > @@ -393,6 +394,8 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > for (j = 0; j < NUM_SQ_STATS; j++)
> > data[idx++] = MLX5E_READ_CTR64_CPU(&priv->channel[i]->sq[tc].stats,
> > sq_stats_desc, j);
> > +
> > + mutex_unlock(&priv->state_lock);
> > }
> >
> > static u32 mlx5e_rx_wqes_to_packets(struct mlx5e_priv *priv, int rq_wq_type,
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index 66c133757a5e..a4c100bea541 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -2748,6 +2748,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
> > struct mlx5e_vport_stats *vstats = &priv->stats.vport;
> > struct mlx5e_pport_stats *pstats = &priv->stats.pport;
> >
> > + mutex_lock(&priv->state_lock);
> > +
>
> We can not sleep from ndo_get_stats() ( look at bonding driver )
Thanks for pointing out the bond_get_stats().
>
> What about the following ?
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 66c133757a5ee8daae122e93322306b1c5c44336..b9fea146a0ca18498a8dfa5698dca7dea06e3c5e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work)
>
> static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> {
> - struct mlx5e_sw_stats *s = &priv->stats.sw;
> + struct mlx5e_sw_stats temp, *s = &temp;
> struct mlx5e_rq_stats *rq_stats;
> struct mlx5e_sq_stats *sq_stats;
> u64 tx_offload_none = 0;
> @@ -229,6 +229,8 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> s->link_down_events_phy = MLX5_GET(ppcnt_reg,
> priv->stats.pport.phy_counters,
> counter_set.phys_layer_cntrs.link_down_events);
> + /* A bit racy (depending on memcpy() sanity...) , we probably should use a spinlock */
> + memcpy(&priv->stats.sw, s, sizeof(*s));
Right, a temp and a memcpy should be enough to solve our spike problem.
It may be the right fix for net.
Agree that using a spinlock is better (likely changing state_lock
to spinlock). A quick grep shows 80 line changes. Saeed, thoughts?
> }
>
> static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
>
>
^ permalink raw reply
* Re: [PATCH 2/4] net: macb: Add tsu_clk to device tree
From: Rob Herring @ 2017-04-19 21:57 UTC (permalink / raw)
To: Rafal Ozieblo
Cc: David Miller, nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
harinikatakamlinux-Re5JQEeQqe8AvxtiuMwx3w,
harini.katakam-gjFFaj9aHVfQT0dZR+AlfA,
richardcochran-Re5JQEeQqe8AvxtiuMwx3w,
Andrei.Pistirica-UWL1GkI3JZL3oGB3hsPCZA
In-Reply-To: <1492090686-14778-1-git-send-email-rafalo-vna1KIf7WgpBDgjK7y7TUQ@public.gmane.org>
On Thu, Apr 13, 2017 at 02:38:06PM +0100, Rafal Ozieblo wrote:
> Signed-off-by: Rafal Ozieblo <rafalo-vna1KIf7WgpBDgjK7y7TUQ@public.gmane.org>
> ---
> Documentation/devicetree/bindings/net/macb.txt | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 net-next]smsc911x: Adding support for Micochip LAN9250 Ethernet controller
From: Andrew Lunn @ 2017-04-19 22:01 UTC (permalink / raw)
To: David.Cai; +Cc: netdev, davem, UNGLinuxDriver, steve.glendinning
In-Reply-To: <C3C28FB10418274EB7FD7C2B85C796A4412465FE@CHN-SV-EXMX02.mchp-main.com>
On Wed, Apr 19, 2017 at 09:26:43PM +0000, David.Cai@microchip.com wrote:
> Adding support for Microchip LAN9250 Ethernet controller.
>
> Signed-off-by: David Cai <david.cai@microchip.com>
> ---
> Changes
> V2
> - email format changed
> - remove unnecessary text in commit log
Much better, thanks.
> drivers/net/ethernet/smsc/smsc911x.c | 32 +++++++++++++++++++++++++++-----
> drivers/net/ethernet/smsc/smsc911x.h | 3 +++
> 2 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index fa5ca09..22b1951 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -25,7 +25,7 @@
> * LAN9215, LAN9216, LAN9217, LAN9218
> * LAN9210, LAN9211
> * LAN9220, LAN9221
> - * LAN89218
> + * LAN89218,LAN9250
> *
> */
>
> @@ -104,6 +104,9 @@ struct smsc911x_data {
> /* used to decide which workarounds apply */
> unsigned int generation;
>
> + /* used to decide which sub generation product work arounds to apply */
> + unsigned int sub_generation;
> +
> /* device configuration (copied from platform_data during probe) */
> struct smsc911x_platform_config config;
>
> @@ -1450,6 +1453,8 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
> unsigned int timeout;
> unsigned int temp;
> int ret;
> + unsigned int reset_offset = HW_CFG;
> + unsigned int reset_mask = HW_CFG_SRST_;
>
> /*
> * Make sure to power-up the PHY chip before doing a reset, otherwise
> @@ -1476,15 +1481,23 @@ static int smsc911x_soft_reset(struct smsc911x_data *pdata)
> }
> }
>
> + if (pdata->sub_generation) {
I know it does not make a difference at the moment, but shouldn't this
be
if (pdata>generation == 4 && pdata->sub_generation) {
at some point other sub-generations might become important, at which
point your simpler code breaks.
> + /* special reset for LAN9250 */
> + reset_offset = RESET_CTL;
> + reset_mask = RESET_CTL_DIGITAL_RST_;
> + }
> @@ -2251,6 +2264,9 @@ static int smsc911x_init(struct net_device *dev)
> /* Default generation to zero (all workarounds apply) */
> pdata->generation = 0;
>
> + /* Default sub_generation to zero */
> + pdata->sub_generation = 0;
> +
alloc_etherdev() uses kzalloc, so sub_generation is guaranteed to be
0. No need to set it here.
> pdata->idrev = smsc911x_reg_read(pdata, ID_REV);
> switch (pdata->idrev & 0xFFFF0000) {
> case 0x01180000:
> @@ -2278,6 +2294,12 @@ static int smsc911x_init(struct net_device *dev)
> pdata->generation = 4;
> break;
>
> + case 0x92500000:
> + /* LAN9250 */
> + pdata->generation = 4;
> + pdata->sub_generation = 1;
> + break;
>
Maybe it would be more readable in general to add some #defines for
0x92500000 and all the other hex values, and then in smsc911x_init()
look at the pdata->idrev?
Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an assert step mechanism
From: Kees Cook @ 2017-04-19 22:02 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry <wa
In-Reply-To: <94ac6ddc-eaac-8548-f83f-826ddf05ac69-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
On Wed, Apr 19, 2017 at 2:51 PM, Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org> wrote:
>
> On 19/04/2017 02:02, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> This is useful to return an information about the error without being
>>> able to write to TH_LOG_STREAM.
>>>
>>> Helpers from test_harness.h may be useful outside of the seccomp
>>> directory.
>>>
>>> Signed-off-by: Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
>>> Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>>> Cc: Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> Cc: Shuah Khan <shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> ---
>>> tools/testing/selftests/seccomp/test_harness.h | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/seccomp/test_harness.h b/tools/testing/selftests/seccomp/test_harness.h
>>> index a786c69c7584..77e407663e06 100644
>>> --- a/tools/testing/selftests/seccomp/test_harness.h
>>> +++ b/tools/testing/selftests/seccomp/test_harness.h
>>> @@ -397,7 +397,7 @@ struct __test_metadata {
>>> const char *name;
>>> void (*fn)(struct __test_metadata *);
>>> int termsig;
>>> - int passed;
>>> + __s8 passed;
>>
>> Why the reduction here? int is signed too?
>
> Because the return code of a process is capped to 8 bits and I use a
> negative value to not mess with the current interpretation of 0 (error)
> and 1 (OK) for the "passed" variable.
>
>>
>>> int trigger; /* extra handler after the evaluation */
>>> struct __test_metadata *prev, *next;
>>> };
>>> @@ -476,6 +476,12 @@ void __run_test(struct __test_metadata *t)
>>> "instead of by signal (code: %d)\n",
>>> t->name,
>>> WEXITSTATUS(status));
>>> + } else if (t->passed < 0) {
>>> + fprintf(TH_LOG_STREAM,
>>> + "%s: Failed at step #%d\n",
>>> + t->name,
>>> + t->passed * -1);
>>> + t->passed = 0;
>>> }
>>
>> Instead of creating an overloaded mechanism here, perhaps have an
>> option reporting mechanism that can be enabled. Like adding to
>> __test_metadata "bool no_stream; int test_number;" and adding
>> test_number++ to each ASSERT/EXCEPT call, and doing something like:
>>
>> if (t->no_stream) {
>> fprintf(TH_LOG_STREAM,
>> "%s: Failed at step #%d\n",
>> t->name,
>> t->test_number);
>> }
>>
>> It'd be a cleaner approach, maybe?
>
> Good idea, we will then be able to use 255 steps!
>
> Do you want me to send this as a separate patch?
>
> Can we move test_harness.h outside of the seccomp directory to be
> available to other subsystems as well?
Yeah, I would do two patches, and send them out separately (to shuah
with lkml and me in cc at least), one to move test_hardness.h into
some include/ directory, and then to add the new logic for streamless
reporting.
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem
From: Mickaël Salaün @ 2017-04-19 22:03 UTC (permalink / raw)
To: Kees Cook, Casey Schaufler
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Daniel Borkmann, David Drysdale,
David S . Miller, Eric W . Biederman, James Morris, Jann Horn,
Jonathan Corbet, Matthew Garrett, Michael Kerrisk, Paul Moore,
Sargun Dhillon, Serge E . Hallyn, Shuah Khan, Tejun Heo,
Thomas Graf, Will Drewry,
"kernel-hardening@lists.openwall.com" <k
In-Reply-To: <CAGXu5jKCr1qPqJ8YWAjK1Lj-fThAhMnrxFW8twFcj6C9ODBpsA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2384 bytes --]
On 19/04/2017 01:40, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>>> On 19/04/2017 00:17, Kees Cook wrote:
>>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>> +void __init landlock_add_hooks(void)
>>>>> +{
>>>>> + pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>>> + landlock_add_hooks_fs();
>>>>> + security_add_hooks(NULL, 0, "landlock");
>>>>> + bpf_register_prog_type(&bpf_landlock_type);
>>>> I'm confused by the separation of hook registration here. The call to
>>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>>> just a single call with security_add_hooks(landlock_hooks,
>>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>>> Yes, this is ugly with the new security_add_hooks() with three arguments
>>> but I wanted to split the hooks definition in multiple files.
>>
>> Why? I'll buy a good argument, but there are dangers in
>> allowing multiple calls to security_add_hooks().
I prefer to have one file per hook "family" (e.g. filesystem, network,
ptrace…). This reduce the mess with all the included files (needed for
LSM hook argument types) and make the files easier to read, understand
and maintain.
>>
>>>
>>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>>> is not exported. Unfortunately, calling multiple security_add_hooks()
>>> with the same LSM name would register multiple names for the same LSM…
>>> Is it OK if I modify this function to not add duplicated entries?
>>
>> It may seem absurd, but it's conceivable that a module might
>> have two hooks it wants called. My example is a module that
>> counts the number of times SELinux denies a process access to
>> things (which needs to be called before and after SELinux in
>> order to detect denials) and takes "appropriate action" if
>> too many denials occur. It would be weird, wonky and hackish,
>> but that never stopped anybody before.
Right, but now, with the new lsm_append(), module names are concatenated
("%s,%s") in the lsm_names variable. It would be nice to not pollute
this string with multiple time the same module name.
>
> If ends up being sane and clear, I'm fine with allowing multiple calls.
>
> -Kees
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v6 09/11] seccomp: Enhance test_harness with an assert step mechanism
From: Mickaël Salaün @ 2017-04-19 22:05 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <CAGXu5jJfnA=XeUNc4eAsUvXCja725riA7Vaod=BmM2ZbXdfQ4A@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3597 bytes --]
On 20/04/2017 00:02, Kees Cook wrote:
> On Wed, Apr 19, 2017 at 2:51 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 19/04/2017 02:02, Kees Cook wrote:
>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> This is useful to return an information about the error without being
>>>> able to write to TH_LOG_STREAM.
>>>>
>>>> Helpers from test_harness.h may be useful outside of the seccomp
>>>> directory.
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Shuah Khan <shuah@kernel.org>
>>>> Cc: Will Drewry <wad@chromium.org>
>>>> ---
>>>> tools/testing/selftests/seccomp/test_harness.h | 8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/seccomp/test_harness.h b/tools/testing/selftests/seccomp/test_harness.h
>>>> index a786c69c7584..77e407663e06 100644
>>>> --- a/tools/testing/selftests/seccomp/test_harness.h
>>>> +++ b/tools/testing/selftests/seccomp/test_harness.h
>>>> @@ -397,7 +397,7 @@ struct __test_metadata {
>>>> const char *name;
>>>> void (*fn)(struct __test_metadata *);
>>>> int termsig;
>>>> - int passed;
>>>> + __s8 passed;
>>>
>>> Why the reduction here? int is signed too?
>>
>> Because the return code of a process is capped to 8 bits and I use a
>> negative value to not mess with the current interpretation of 0 (error)
>> and 1 (OK) for the "passed" variable.
>>
>>>
>>>> int trigger; /* extra handler after the evaluation */
>>>> struct __test_metadata *prev, *next;
>>>> };
>>>> @@ -476,6 +476,12 @@ void __run_test(struct __test_metadata *t)
>>>> "instead of by signal (code: %d)\n",
>>>> t->name,
>>>> WEXITSTATUS(status));
>>>> + } else if (t->passed < 0) {
>>>> + fprintf(TH_LOG_STREAM,
>>>> + "%s: Failed at step #%d\n",
>>>> + t->name,
>>>> + t->passed * -1);
>>>> + t->passed = 0;
>>>> }
>>>
>>> Instead of creating an overloaded mechanism here, perhaps have an
>>> option reporting mechanism that can be enabled. Like adding to
>>> __test_metadata "bool no_stream; int test_number;" and adding
>>> test_number++ to each ASSERT/EXCEPT call, and doing something like:
>>>
>>> if (t->no_stream) {
>>> fprintf(TH_LOG_STREAM,
>>> "%s: Failed at step #%d\n",
>>> t->name,
>>> t->test_number);
>>> }
>>>
>>> It'd be a cleaner approach, maybe?
>>
>> Good idea, we will then be able to use 255 steps!
>>
>> Do you want me to send this as a separate patch?
>>
>> Can we move test_harness.h outside of the seccomp directory to be
>> available to other subsystems as well?
>
> Yeah, I would do two patches, and send them out separately (to shuah
> with lkml and me in cc at least), one to move test_hardness.h into
> some include/ directory, and then to add the new logic for streamless
> reporting.
>
> Thanks!
>
> -Kees
>
>
Good, in which place and name would it fit better?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH net-next] brcmfmac: fix build without CONFIG_BRCMFMAC_PROTO_BCDC
From: Arnd Bergmann @ 2017-04-19 22:10 UTC (permalink / raw)
To: Arend van Spriel
Cc: Arnd Bergmann, Franky Lin, Hante Meuleman, Kalle Valo,
Pieter-Paul Giesberts, Michael S. Tsirkin, linux-wireless,
brcm80211-dev-list.pdl, netdev, linux-kernel
With CONFIG_BRCMFMAC_PROTO_BCDC unset, we cannot build the fwsignal.c file:
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c: In function 'brcmf_fws_notify_credit_map':
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c:1590:31: error: implicit declaration of function 'drvr_to_fws'; did you mean 'dev_to_psd'? [-Werror=implicit-function-declaration]
struct brcmf_fws_info *fws = drvr_to_fws(ifp->drvr);
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c:1590:31: error: initialization makes pointer from integer without a cast [-Werror=int-conversion]
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c:1621:31: error: initialization makes pointer from integer without a cast [-Werror=int-conversion]
However, as pointed out in the changeset description for the patch that caused
the problem, fwsignal.c is only required when CONFIG_BRCMFMAC_PROTO_BCDC is
enabled, so we can simply change the Makefile to build it conditionally.
Fixes: acf8ac41dd73 ("brcmfmac: remove reference to fwsignal data from struct brcmf_pub")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
index 0383ba559edc..1f5a9b948abf 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
@@ -25,7 +25,6 @@ brcmfmac-objs += \
chip.o \
fwil.o \
fweh.o \
- fwsignal.o \
p2p.o \
proto.o \
common.o \
@@ -36,7 +35,8 @@ brcmfmac-objs += \
vendor.o \
pno.o
brcmfmac-$(CONFIG_BRCMFMAC_PROTO_BCDC) += \
- bcdc.o
+ bcdc.o \
+ fwsignal.o
brcmfmac-$(CONFIG_BRCMFMAC_PROTO_MSGBUF) += \
commonring.o \
flowring.o \
--
2.9.0
^ permalink raw reply related
* Re: [PATCH net-next v6 05/11] seccomp: Split put_seccomp_filter() with put_seccomp()
From: Mickaël Salaün @ 2017-04-19 22:18 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Shuah Khan, Tejun Heo, Thomas Graf, Will Drewry
In-Reply-To: <9b420ecc-f062-8ab3-0b18-e09c60a00c6a@digikod.net>
[-- Attachment #1.1: Type: text/plain, Size: 4752 bytes --]
On 19/04/2017 00:47, Mickaël Salaün wrote:
>
> On 19/04/2017 00:23, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> The semantic is unchanged. This will be useful for the Landlock
>>> integration with seccomp (next commit).
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>> Cc: Will Drewry <wad@chromium.org>
>>> ---
>>> include/linux/seccomp.h | 4 ++--
>>> kernel/fork.c | 2 +-
>>> kernel/seccomp.c | 18 +++++++++++++-----
>>> 3 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>>> index ecc296c137cd..e25aee2cdfc0 100644
>>> --- a/include/linux/seccomp.h
>>> +++ b/include/linux/seccomp.h
>>> @@ -77,10 +77,10 @@ static inline int seccomp_mode(struct seccomp *s)
>>> #endif /* CONFIG_SECCOMP */
>>>
>>> #ifdef CONFIG_SECCOMP_FILTER
>>> -extern void put_seccomp_filter(struct task_struct *tsk);
>>> +extern void put_seccomp(struct task_struct *tsk);
>>> extern void get_seccomp_filter(struct task_struct *tsk);
>>> #else /* CONFIG_SECCOMP_FILTER */
>>> -static inline void put_seccomp_filter(struct task_struct *tsk)
>>> +static inline void put_seccomp(struct task_struct *tsk)
>>> {
>>> return;
>>> }
>>> diff --git a/kernel/fork.c b/kernel/fork.c
>>> index 6c463c80e93d..a27d8e67ce33 100644
>>> --- a/kernel/fork.c
>>> +++ b/kernel/fork.c
>>> @@ -363,7 +363,7 @@ void free_task(struct task_struct *tsk)
>>> #endif
>>> rt_mutex_debug_task_free(tsk);
>>> ftrace_graph_exit_task(tsk);
>>> - put_seccomp_filter(tsk);
>>> + put_seccomp(tsk);
>>> arch_release_task_struct(tsk);
>>> if (tsk->flags & PF_KTHREAD)
>>> free_kthread_struct(tsk);
>>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>>> index 65f61077ad50..326f79e32127 100644
>>> --- a/kernel/seccomp.c
>>> +++ b/kernel/seccomp.c
>>> @@ -64,6 +64,8 @@ struct seccomp_filter {
>>> /* Limit any path through the tree to 256KB worth of instructions. */
>>> #define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
>>>
>>> +static void put_seccomp_filter(struct seccomp_filter *filter);
>>
>> Can this be reorganized easily to avoid a forward-declaration?
>
> I didn't want to move too much code but I will.
>
>>
>>> +
>>> /*
>>> * Endianness is explicitly ignored and left for BPF program authors to manage
>>> * as per the specific architecture.
>>> @@ -314,7 +316,7 @@ static inline void seccomp_sync_threads(void)
>>> * current's path will hold a reference. (This also
>>> * allows a put before the assignment.)
>>> */
>>> - put_seccomp_filter(thread);
>>> + put_seccomp_filter(thread->seccomp.filter);
>>> smp_store_release(&thread->seccomp.filter,
>>> caller->seccomp.filter);
>>>
>>> @@ -476,10 +478,11 @@ static inline void seccomp_filter_free(struct seccomp_filter *filter)
>>> }
>>> }
>>>
>>> -/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
>>> -void put_seccomp_filter(struct task_struct *tsk)
>>> +/* put_seccomp_filter - decrements the ref count of a filter */
>>> +static void put_seccomp_filter(struct seccomp_filter *filter)
>>> {
>>> - struct seccomp_filter *orig = tsk->seccomp.filter;
>>> + struct seccomp_filter *orig = filter;
>>> +
>>> /* Clean up single-reference branches iteratively. */
>>> while (orig && atomic_dec_and_test(&orig->usage)) {
>>> struct seccomp_filter *freeme = orig;
>>> @@ -488,6 +491,11 @@ void put_seccomp_filter(struct task_struct *tsk)
>>> }
>>> }
>>>
>>> +void put_seccomp(struct task_struct *tsk)
>>> +{
>>> + put_seccomp_filter(tsk->seccomp.filter);
>>> +}
>>> +
>>> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
>>> {
>>> memset(info, 0, sizeof(*info));
>>> @@ -914,7 +922,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
>>> ret = -EFAULT;
>>>
>>> - put_seccomp_filter(task);
>>> + put_seccomp_filter(task->seccomp.filter);
>>> return ret;
>>
>> I don't like that the arguments to get_seccomp_filter() and
>> put_seccomp_filter() are now different. I think they should match for
>> readability.
>
> OK, I can do that.
>
Kees, can I send this as a separate patch?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: __sk_buff.data_end
From: Johannes Berg @ 2017-04-19 22:20 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann; +Cc: netdev
In-Reply-To: <1492637460.22185.6.camel@sipsolutions.net>
On Wed, 2017-04-19 at 23:31 +0200, Johannes Berg wrote:
> Hi Alexei, Daniel,
>
> I'm looking at adding the __wifi_sk_buff I talked about, and I notice
> that it uses CB space to store data_end. Unfortunately, in a lot of
> cases, we don't have any CB space to spare in wifi.
I guess I can work around this, would this seem reasonable?
struct bpf_skb_data_end {
struct qdisc_skb_cb qdisc_cb;
- void *data_end;
+ /*
+ * The alignment here is for mac80211, since that doesn't use
+ * a pointer but a u64 value and needs to save/restore that
+ * across running its BPF programs.
+ */
+ void *data_end __aligned(sizeof(u64));
};
johannes
^ permalink raw reply
* [PATCH net-next] netvsc: fix use after free on module removal
From: Stephen Hemminger @ 2017-04-19 22:22 UTC (permalink / raw)
To: davem; +Cc: netdev, Stephen Hemminger
The NAPI data structure is embedded in the netvsc_device structure
and is freed when device is closed. There is still a reference
(in NAPI list) to this which causes a crash in netif_napi_del
when device is removed. Fix by managing NAPI instances correctly.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
Note: resend with correct list address
drivers/net/hyperv/netvsc.c | 9 +++++----
drivers/net/hyperv/rndis_filter.c | 9 ++++++---
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index fd21d5aab580..e282bdff7aa0 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -568,8 +568,9 @@ void netvsc_device_remove(struct hv_device *device)
/* Now, we can close the channel safely */
vmbus_close(device->channel);
+ /* And dissassociate NAPI context from device */
for (i = 0; i < net_device->num_chn; i++)
- napi_disable(&net_device->chan_table[i].napi);
+ netif_napi_del(&net_device->chan_table[i].napi);
/* Release all resources */
free_netvsc_device_rcu(net_device);
@@ -1304,8 +1305,6 @@ int netvsc_device_add(struct hv_device *device,
struct netvsc_channel *nvchan = &net_device->chan_table[i];
nvchan->channel = device->channel;
- netif_napi_add(ndev, &nvchan->napi,
- netvsc_poll, NAPI_POLL_WEIGHT);
}
/* Open the channel */
@@ -1323,6 +1322,8 @@ int netvsc_device_add(struct hv_device *device,
netdev_dbg(ndev, "hv_netvsc channel opened successfully\n");
/* Enable NAPI handler for init callbacks */
+ netif_napi_add(ndev, &net_device->chan_table[0].napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
napi_enable(&net_device->chan_table[0].napi);
/* Writing nvdev pointer unlocks netvsc_send(), make sure chn_table is
@@ -1341,7 +1342,7 @@ int netvsc_device_add(struct hv_device *device,
return ret;
close:
- napi_disable(&net_device->chan_table[0].napi);
+ netif_napi_del(&net_device->chan_table[0].napi);
/* Now, we can close the channel safely */
vmbus_close(device->channel);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 1e9445bc4539..ab92c3c95951 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1009,13 +1009,16 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
+ netif_napi_add(ndev, &nvchan->napi,
+ netvsc_poll, NAPI_POLL_WEIGHT);
ret = vmbus_open(new_sc, nvscdev->ring_size * PAGE_SIZE,
nvscdev->ring_size * PAGE_SIZE, NULL, 0,
netvsc_channel_cb, nvchan);
-
-
- napi_enable(&nvchan->napi);
+ if (ret == 0)
+ napi_enable(&nvchan->napi);
+ else
+ netdev_err(ndev, "sub channel open failed (%d)\n", ret);
if (refcount_dec_and_test(&nvscdev->sc_offered))
complete(&nvscdev->channel_init_wait);
--
2.11.0
^ permalink raw reply related
* Re: XDP question: best API for returning/setting egress port?
From: Daniel Borkmann @ 2017-04-19 22:51 UTC (permalink / raw)
To: Andy Gospodarek, Jesper Dangaard Brouer
Cc: Daniel Borkmann, Alexei Starovoitov, Alexei Starovoitov,
netdev@vger.kernel.org, xdp-newbies@vger.kernel.org,
John Fastabend
In-Reply-To: <20170419200259.GK4730@C02RW35GFVH8.dhcp.broadcom.net>
On 04/19/2017 10:02 PM, Andy Gospodarek wrote:
[...]
> and then lookup this dest in a table we have the option to make that
> dest an ifindex/socket/other.
>
> I did also look at JohnF's patch and I do like the simplicity of the redirect
> action and new ndo_xdp_xmit and how it moves towards a way to transmit the
> frame. The downside is that it presumes an ifindex, so it might not be ideal
> we want the lookup to return something other than an ifindex.
>
[...]
> would be handled. If we are ultimately going to need a new netdev op to
> handle the redirect then what may be the issue with not providing the
> destination port the return code and the option proposed by JohnF looks
> good to me with maybe a small tweak to not presume ifindex in some manner.
Is there a concrete reason that all the proposed future cases like sockets
have to be handled within the very same XDP_REDIRECT return code? F.e. why
not XDP_TX_NIC that only assumes ifindex as proposed in the patch, and future
ones would get a different return code f.e. XDP_TX_SK only handling sockets
when we get there implementation-wise?
^ permalink raw reply
* Re: [PATCH] xen/9pfs: select CONFIG_XEN_XENBUS_FRONTEND
From: Stefano Stabellini @ 2017-04-19 23:10 UTC (permalink / raw)
To: jgross
Cc: Arnd Bergmann, Stefano Stabellini, boris.ostrovsky, groug,
xen-devel, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
David S. Miller, v9fs-developer, netdev, linux-kernel
In-Reply-To: <alpine.DEB.2.10.1704191020310.31486@sstabellini-ThinkPad-X260>
Juergen, I have committed this patch to for-linus-4.12 and linux-next, I
hope that's OK.
Og Wed, 19 Apr 2017, Stefano Stabellini wrote:
> On Wed, 19 Apr 2017, Arnd Bergmann wrote:
> > All Xen frontends need to select this symbol to avoid a link error:
> >
> > net/built-in.o: In function `p9_trans_xen_init':
> > :(.text+0x149e9c): undefined reference to `__xenbus_register_frontend'
> >
> > Fixes: d4b40a02f837 ("xen/9pfs: build 9pfs Xen transport driver")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Thank you for the fix!
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > net/9p/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/9p/Kconfig b/net/9p/Kconfig
> > index 3f286f1bd1d3..e6014e0e51f7 100644
> > --- a/net/9p/Kconfig
> > +++ b/net/9p/Kconfig
> > @@ -24,6 +24,7 @@ config NET_9P_VIRTIO
> >
> > config NET_9P_XEN
> > depends on XEN
> > + select XEN_XENBUS_FRONTEND
> > tristate "9P Xen Transport"
> > help
> > This builds support for a transport for 9pfs between
> > --
> > 2.9.0
> >
>
^ permalink raw reply
* Re: FEC on i.MX 7 transmit queue timeout
From: Stefan Agner @ 2017-04-19 23:15 UTC (permalink / raw)
To: Andy Duan; +Cc: fugang.duan, festevam, netdev, netdev-owner
In-Reply-To: <2bdd64ab-5644-e0a0-9bfe-b8dd2fca7abb@nxp.com>
On 2017-04-19 01:45, Andy Duan wrote:
> On 2017年04月19日 13:56, Stefan Agner wrote:
>> On 2017-04-18 22:28, Andy Duan wrote:
>>> From: Stefan Agner <stefan@agner.ch> Sent: Wednesday, April 19, 2017 1:02 PM
>>>> To: Andy Duan <fugang.duan@nxp.com>
>>>> Cc: fugang.duan@freescale.com; festevam@gmail.com;
>>>> netdev@vger.kernel.org; netdev-owner@vger.kernel.org
>>>> Subject: Re: FEC on i.MX 7 transmit queue timeout
>>>>
>>>> Hi Andy,
>>>>
>>>> On 2017-04-18 19:24, Andy Duan wrote:
>>>>> On 2017年04月19日 03:46, Stefan Agner wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I noticed last week on upstream (v4.11-rc6) on a Colibri iMX7 board
>>>>>> that after a while (~10 minutes) the detdev wachdog prints a
>>>>>> stacktrace and the driver then continuously dumps the TX ring. I then
>>>>>> did a quick test with 4.10, and realized it actually suffers the same
>>>>>> issue, so it seems not to be a regression. I use a rootfs mounted over NFS...
>>>>>>
>>>>>> ------------[ cut here ]------------
>>>>>> WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316
>>>>>> dev_watchdog+0x240/0x244
>>>>>> NETDEV WATCHDOG: eth0 (fec): transmit queue 2 timed out Modules
>>>>>> linked in:
>>>>>> CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>>>> 4.11.0-rc7-00030-g2c4e6bd0c4f0-dirty #330 Hardware name: Freescale
>>>>>> i.MX7 Dual (Device Tree) [<c02293f0>] (unwind_backtrace) from
>>>>>> [<c0225820>] (show_stack+0x10/0x14) [<c0225820>] (show_stack) from
>>>>>> [<c050db6c>] (dump_stack+0x90/0xa0) [<c050db6c>] (dump_stack) from
>>>>>> [<c023ae68>] (__warn+0xac/0x11c) [<c023ae68>] (__warn) from
>>>>>> [<c023af10>] (warn_slowpath_fmt+0x38/0x48) [<c023af10>]
>>>>>> (warn_slowpath_fmt) from [<c088bb8c>]
>>>>>> (dev_watchdog+0x240/0x244)
>>>>>> [<c088bb8c>] (dev_watchdog) from [<c0294798>]
>>>>>> (run_timer_softirq+0x24c/0x708)
>>>>>> [<c0294798>] (run_timer_softirq) from [<c023f584>]
>>>>>> (__do_softirq+0x12c/0x2a8)
>>>>>> [<c023f584>] (__do_softirq) from [<c023f8c4>] (irq_exit+0xdc/0x13c)
>>>>>> [<c023f8c4>] (irq_exit) from [<c02818ac>]
>>>>>> (__handle_domain_irq+0xa4/0xf8)
>>>>>> [<c02818ac>] (__handle_domain_irq) from [<c0201624>]
>>>>>> (gic_handle_irq+0x34/0xa4)
>>>>>> [<c0201624>] (gic_handle_irq) from [<c0226338>] (__irq_svc+0x58/0x8c)
>>>>>> Exception stack(0xc1201f30 to 0xc1201f78)
>>>>>> 1f20: c0233320 00000000 00000000
>>>>>> 01400000
>>>>>> 1f40: c1203d80 ffffe000 00000000 00000000 c107bf10 c0e055b5 c1203d34
>>>>>> 00000001
>>>>>> 1f60: c07d2324 c1201f80 c0222ac8 c0222acc 60000013 ffffffff
>>>>>> [<c0226338>] (__irq_svc) from [<c0222acc>] (arch_cpu_idle+0x38/0x3c)
>>>>>> [<c0222acc>] (arch_cpu_idle) from [<c0275f24>] (do_idle+0xa8/0x250)
>>>>>> [<c0275f24>] (do_idle) from [<c02760e4>]
>>>>>> (cpu_startup_entry+0x18/0x1c) [<c02760e4>] (cpu_startup_entry) from
>>>>>> [<c1000aa0>]
>>>>>> (start_kernel+0x3fc/0x45c)
>>>>>> ---[ end trace 5b0c6dc3466a7918 ]---
>>>>>> fec 30be0000.ethernet eth0: TX ring dump
>>>>>> Nr SC addr len SKB
>>>>>> 0 0x1c00 0x00000000 590 (null)
>>>>>> 1 0x1c00 0x00000000 590 (null)
>>>>>> 2 0x1c00 0x00000000 42 (null)
>>>>>> 3 H 0x1c00 0x00000000 42 (null)
>>>>>> 4 S 0x0000 0x00000000 0 (null)
>>>>>> 5 0x0000 0x00000000 0 (null)
>>>>>> 6 0x0000 0x00000000 0 (null)
>>>>>> 7 0x0000 0x00000000 0 (null)
>>>>>> 8 0x0000 0x00000000 0 (null)
>>>>>> 9 0x0000 0x00000000 0 (null)
>>>>>> 10 0x0000 0x00000000 0 (null)
>>>>>> 11 0x0000 0x00000000 0 (null)
>>>>>> 12 0x0000 0x00000000 0 (null)
>>>>>> 13 0x0000 0x00000000 0 (null)
>>>>>> 14 0x0000 0x00000000 0 (null)
>>>>>> 15 0x0000 0x00000000 0 (null)
>>>>>> 16 0x0000 0x00000000 0 (null)
>>>>>> 17 0x0000 0x00000000 0 (null)
>>>>>> 18 0x0000 0x00000000 0 (null)
>>>>>> ...
>>>>>>
>>>>>>
>>>>>> A second TX ring dump from 4.10:
>>>>>> fec 30be0000.ethernet eth0: TX ring dump
>>>>>> Nr SC addr len SKB
>>>>>> 0 0x1c00 0x00000000 42 (null)
>>>>>> 1 0x1c00 0x00000000 42 (null)
>>>>>> 2 0x1c00 0x00000000 90 (null)
>>>>>> 3 0x1c00 0x00000000 90 (null)
>>>>>> 4 0x1c00 0x00000000 90 (null)
>>>>>> 5 0x1c00 0x00000000 218 (null)
>>>>>> 6 0x1c00 0x00000000 218 (null)
>>>>>> 7 0x1c00 0x00000000 218 (null)
>>>>>> 8 0x1c00 0x00000000 90 (null)
>>>>>> 9 0x1c00 0x00000000 206 (null)
>>>>>> 10 0x1c00 0x00000000 216 (null)
>>>>>> 11 0x1c00 0x00000000 216 (null)
>>>>>> 12 0x1c00 0x00000000 216 (null)
>>>>>> 13 0x1c00 0x00000000 311 (null)
>>>>>> 14 0x1c00 0x00000000 178 (null)
>>>>>> 15 0x1c00 0x00000000 311 (null)
>>>>>> 16 0x1c00 0x00000000 206 (null)
>>>>>> 17 H 0x1c00 0x00000000 311 (null)
>>>>>> 18 S 0x0000 0x00000000 0 (null)
>>>>>> 19 0x0000 0x00000000 0 (null)
>>>>> The dump show tx ring is fine.
>>>>>
>>>>>> The ring dump prints continously, but I can access console every now
>>>>>> and then. I noticed that the second interrupt seems static (66441, TX
>>>>>> interrupt?):
>>>>>> 58: 18 GIC-0 150 Level 30be0000.ethernet
>>>>>> 59: 66441 GIC-0 151 Level 30be0000.ethernet
>>>>>> 60: 70477 GIC-0 152 Level 30be0000.ethernet
>>>>> 150 irq number is for tx/rx queue 1 receive/transmit buffer/frame done.
>>>>> 151 irq number is for tx/rx queue 2 receive/transmit buffer/frame done.
>>>>> 152 irq number is for tx/rx queue 0 receive/transmit buffer/frame
>>>>> done, mii interrupt and others.
>>>>>
>>>>> i.MX7D enet has three queues for tx and rx.
>>>>> It seems netdev pick tx queue 1 rate is very rare by __netdev_pick_tx().
>>>> Oh ok I see, and it seems to choose queue 2 fairly often...
>>>>
>>>>>> Anybody else seen this? Any idea?
>>>>>>
>>>>>> In 4.10 as well as 4.11-rc6 the interrupt counts were just over 65536...
>>>>>> pure chance?
>>>>>>
>>>>>>
>>>>> you can use ethtool to set the irq coalesce like:
>>>>> ethtool -c eth0 rx-frames 80
>>>>> ethtool -c eth0 rx-usecs 600
>>>>> ethtool -c eth0 tx-frames 64
>>>>> ethtool -c eth0 tx-usenc 700
>>>>>
>>>>>
>>>>> You don't run any test case, just nfs mount rootfs ?
>>>>> I will setup one imx7d sdb board to run it.
>>>> I noticed it without doing anything, just boot via NFS. There was always a little
>>>> bit of activity, at least according to the link (blinks every ~5s).
>>>>
>>>> It seemd that it happened a bit earlier when using iperf to exacerbate the
>>>> problem...
>>>>
>>>> I noticed that errata 7885 is not mentioned in the i.MX 7 errata, so I created a
>>>> new devtype:
>>>>
>>>> }, {
>>>> .name = "imx7d-fec",
> This is added by you, we never added the platform_device_id.
>
>>>> .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
>>>> FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
>>>> FEC_QUIRK_HAS_VLAN | FEC_QUIRK_BUG_CAPTURE |
>>>> FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE,
>>>> }, {
>>>>
>>> Upstreaming driver doesn't have the platform_device_id for
>>> "imx7d-fec", imx7d enet still use imx6sx-fec device id driver.
>>> It lost FEC_QUIRK_ERR007885 and FEC_QUIRK_HAS_AVB quirk flags.
>> Also downstream uses imx6sx-fec, at least 4.1.15 GA 2.0.0 release:
>> http://git.freescale.com/git/cgit.cgi/imx/linux-imx.git/tree/arch/arm/boot/dts/imx7d.dtsi?h=imx_4.1.15_2.0.0_ga#n1380
>>
>> However, with downstream Linux 4.1 the kernel seems to only use queue 0:
>> 292: 0 GPCV2 118 Edge 30be0000.ethernet
>> 293: 0 GPCV2 119 Edge 30be0000.ethernet
>> 294: 204929 GPCV2 120 Edge 30be0000.ethernet
>>
> yes, queue 0 is for best effort, queue 1 and 2 are for audio/video.
>
>>> You can add these.
>> I guess if i.MX 7 does not suffer ERR007885 it would be good to add a
>> new devtype, correct? This also needs a device tree change, since
>> imx6sx-fec is still in the compatible list... I saw that you sent a
>> patch to add ERR007885 for imx6ul as well ("net: fec: add ERR007885 for
>> i.MX6ul enet IP").
> ERR007885 just to add some cycles before set TDAR that don't take side
> effort.
> I will confirm the hw issue is fixed or not.
>
>> My earlier run which showed the stack trace again actually still had
>> imx6sx-fec in the device tree compatible string, and hence used
>> ERR007885! So I need to test again...
>>
> pls use compatible string "imx6sx-fec" and test again.
>
I tested again with imx6sx-fec compatible string. I could reproduce it
on a Colibri with i.MX 7Dual. But not always: It really depends whether
queue 2 is counting up or not. Just after boot, I check /proc/interrupts
twice, if queue 2 is counting it will happen!
But if only queue 0 is mostly in use, then it seems to work just fine.
I also tried i.MX 7Dual SabreSD here, and the same thing. I had to
reboot 3 times, then queue 2 was counting:
57: 8 GIC-0 150 Level 30be0000.ethernet
58: 20137 GIC-0 151 Level 30be0000.ethernet
59: 9269 GIC-0 152 Level 30be0000.ethernet
It took me about 40 minutes on Sabre until it happened, and I had to
force it using iperf, but then I got the ring dumps:
(I do not have the first dump unfortunately)
fec 30be0000.ethernet eth0: TX ring dump
Nr SC addr len SKB
0 H 0x1c00 0x00000000 78 (null)
1 S 0x0000 0x00000000 66 (null)
2 0x0000 0x00000000 78 (null)
3 0x0000 0x00000000 78 (null)
4 0x0000 0x00000000 78 (null)
...
>>> I validate imx7d sdb board with 4.11.0-rc6, no such problem after nfs
>>> mount more than 3.5 hours
>> Hm, the Colibri iMX7 uses a different PHY and only supports fast
>> ethernet. Also, I do tests on a i.MX 7Solo actually, but I can do test
>> on a i.MX 7Dual tomorrow. But again, with downstream which only uses
>> queue 0 the issue did never appear.
>>
>> --
> no, my imx7d sdb board running upstreaming kernel 4.11.0-rc6 with three
> queues.
> So far so good (about 6.5 hours).
^ permalink raw reply
* [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats
From: Martin KaFai Lau @ 2017-04-19 23:32 UTC (permalink / raw)
To: netdev; +Cc: Saeed Mahameed, Eric Dumazet, kernel-team
We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
reported under /proc/net/dev. There is a race in mlx5e_update_stats()
and some of the get-stats functions (the one that we hit is the
mlx5e_get_stats() which is called by ndo_get_stats64()).
In particular, the very first thing mlx5e_update_sw_counters()
does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats()
is unlucky at one point, rx_bytes and rx_packets could be 0. One second
later, a normal (and much bigger than 0) value will be reported.
This patch is to use a 'struct mlx5e_sw_stats temp' to avoid
a direct memset zero on priv->stats.sw.
mlx5e_update_vport_counters() has a similar race. Hence, addressed
together.
I am lucky enough to catch this 0-reset in rx multicast:
eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0
eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0
eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0
eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0
eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0
Cc: Saeed Mahameed <saeedm@mellanox.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 66c133757a5e..246786bb861b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work)
static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
{
- struct mlx5e_sw_stats *s = &priv->stats.sw;
+ struct mlx5e_sw_stats temp, *s = &temp;
struct mlx5e_rq_stats *rq_stats;
struct mlx5e_sq_stats *sq_stats;
u64 tx_offload_none = 0;
@@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
s->link_down_events_phy = MLX5_GET(ppcnt_reg,
priv->stats.pport.phy_counters,
counter_set.phys_layer_cntrs.link_down_events);
+ memcpy(&priv->stats.sw, s, sizeof(*s));
}
static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
{
+ struct mlx5e_vport_stats temp;
int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out);
- u32 *out = (u32 *)priv->stats.vport.query_vport_out;
+ u32 *out = (u32 *)temp.query_vport_out;
u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0};
struct mlx5_core_dev *mdev = priv->mdev;
@@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
memset(out, 0, outlen);
mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen);
+ memcpy(priv->stats.vport.query_vport_out, out, outlen);
}
static void mlx5e_update_pport_counters(struct mlx5e_priv *priv)
--
2.9.3
^ permalink raw reply related
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