* [PATCH v2 13/17] cls_u32: Reinstate cyclic allocation
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Commit e7614370d6f0 ("net_sched: use idr to allocate u32 filter handles)
converted htid allocation to use the IDR. The ID allocated by this
scheme changes; it used to be cyclic, but now always allocates the
lowest available. The IDR supports cyclic allocation, so just use
the right function.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
net/sched/cls_u32.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 9d48674a70e0..e65b47483dc0 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -316,19 +316,13 @@ static void *u32_get(struct tcf_proto *tp, u32 handle)
return u32_lookup_key(ht, handle);
}
+/* Protected by rtnl lock */
static u32 gen_new_htid(struct tc_u_common *tp_c, struct tc_u_hnode *ptr)
{
- unsigned long idr_index;
- int err;
-
- /* This is only used inside rtnl lock it is safe to increment
- * without read _copy_ update semantics
- */
- err = idr_alloc_ext(&tp_c->handle_idr, ptr, &idr_index,
- 1, 0x7FF, GFP_KERNEL);
- if (err)
+ int id = idr_alloc_cyclic(&tp_c->handle_idr, ptr, 1, 0x7FF, GFP_KERNEL);
+ if (id < 0)
return 0;
- return (u32)(idr_index | 0x800) << 20;
+ return (id | 0x800U) << 20;
}
static struct hlist_head *tc_u_common_hash;
--
2.15.0
^ permalink raw reply related
* [PATCH v2 12/17] cls_flower: Convert to idr_alloc_u32
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Use the new helper which saves a temporary variable and a few lines
of code.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
net/sched/cls_flower.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index ec0dc92f6104..adee3cf30bb3 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -858,7 +858,6 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
struct cls_fl_filter *fnew;
struct nlattr **tb;
struct fl_flow_mask mask = {};
- unsigned long idr_index;
int err;
if (!tca[TCA_OPTIONS])
@@ -889,21 +888,17 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
goto errout;
if (!handle) {
- err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
- 1, 0x80000000, GFP_KERNEL);
- if (err)
- goto errout;
- fnew->handle = idr_index;
- }
-
- /* user specifies a handle and it doesn't exist */
- if (handle && !fold) {
- err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
- handle, handle + 1, GFP_KERNEL);
- if (err)
- goto errout;
- fnew->handle = idr_index;
+ handle = 1;
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ INT_MAX, GFP_KERNEL);
+ } else if (!fold) {
+ /* user specifies a handle and it doesn't exist */
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ handle, GFP_KERNEL);
}
+ if (err)
+ goto errout;
+ fnew->handle = handle;
if (tb[TCA_FLOWER_FLAGS]) {
fnew->flags = nla_get_u32(tb[TCA_FLOWER_FLAGS]);
@@ -957,7 +952,6 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
*arg = fnew;
if (fold) {
- fnew->handle = handle;
idr_replace(&head->handle_idr, fnew, fnew->handle);
list_replace_rcu(&fold->list, &fnew->list);
tcf_unbind_filter(tp, &fold->res);
--
2.15.0
^ permalink raw reply related
* [PATCH v2 06/17] idr: Delete idr_replace_ext function
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Changing idr_replace's 'id' argument to 'unsigned long' works for all
callers. Callers which passed a negative ID now get -ENOENT instead of
-EINVAL. No callers relied on this error value.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 3 +--
lib/idr.c | 15 +++------------
net/sched/act_api.c | 2 +-
net/sched/cls_basic.c | 2 +-
net/sched/cls_bpf.c | 2 +-
net/sched/cls_flower.c | 2 +-
net/sched/cls_u32.c | 2 +-
7 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 9a4042489ec6..90dbe7a3735c 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -136,8 +136,7 @@ int idr_for_each(const struct idr *,
int (*fn)(int id, void *p, void *data), void *data);
void *idr_get_next(struct idr *, int *nextid);
void *idr_get_next_ext(struct idr *idr, unsigned long *nextid);
-void *idr_replace(struct idr *, void *, int id);
-void *idr_replace_ext(struct idr *idr, void *ptr, unsigned long id);
+void *idr_replace(struct idr *, void *, unsigned long id);
void idr_destroy(struct idr *);
static inline void *idr_remove(struct idr *idr, unsigned long id)
diff --git a/lib/idr.c b/lib/idr.c
index 2593ce513a18..577bfd4fe5c2 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -147,18 +147,9 @@ EXPORT_SYMBOL(idr_get_next_ext);
* the one being replaced!).
*
* Returns: the old value on success. %-ENOENT indicates that @id was not
- * found. %-EINVAL indicates that @id or @ptr were not valid.
+ * found. %-EINVAL indicates that @ptr was not valid.
*/
-void *idr_replace(struct idr *idr, void *ptr, int id)
-{
- if (id < 0)
- return ERR_PTR(-EINVAL);
-
- return idr_replace_ext(idr, ptr, id);
-}
-EXPORT_SYMBOL(idr_replace);
-
-void *idr_replace_ext(struct idr *idr, void *ptr, unsigned long id)
+void *idr_replace(struct idr *idr, void *ptr, unsigned long id)
{
struct radix_tree_node *node;
void __rcu **slot = NULL;
@@ -175,7 +166,7 @@ void *idr_replace_ext(struct idr *idr, void *ptr, unsigned long id)
return entry;
}
-EXPORT_SYMBOL(idr_replace_ext);
+EXPORT_SYMBOL(idr_replace);
/**
* DOC: IDA description
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index bab81574a420..7e901e855d68 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -348,7 +348,7 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
struct tcf_idrinfo *idrinfo = tn->idrinfo;
spin_lock_bh(&idrinfo->lock);
- idr_replace_ext(&idrinfo->action_idr, a, a->tcfa_index);
+ idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
spin_unlock_bh(&idrinfo->lock);
}
EXPORT_SYMBOL(tcf_idr_insert);
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index d2193304bad0..147700afcf31 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -231,7 +231,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
*arg = fnew;
if (fold) {
- idr_replace_ext(&head->handle_idr, fnew, fnew->handle);
+ idr_replace(&head->handle_idr, fnew, fnew->handle);
list_replace_rcu(&fold->link, &fnew->link);
tcf_unbind_filter(tp, &fold->res);
tcf_exts_get_net(&fold->exts);
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index b017d99fd7e1..1660fc8294ef 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -526,7 +526,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
if (oldprog) {
- idr_replace_ext(&head->handle_idr, prog, handle);
+ idr_replace(&head->handle_idr, prog, handle);
list_replace_rcu(&oldprog->link, &prog->link);
tcf_unbind_filter(tp, &oldprog->res);
tcf_exts_get_net(&oldprog->exts);
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 3e89b0be1706..ca71823bee03 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -958,7 +958,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
if (fold) {
fnew->handle = handle;
- idr_replace_ext(&head->handle_idr, fnew, fnew->handle);
+ idr_replace(&head->handle_idr, fnew, fnew->handle);
list_replace_rcu(&fold->list, &fnew->list);
tcf_unbind_filter(tp, &fold->res);
tcf_exts_get_net(&fold->exts);
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 6fe4e3549ad3..9d48674a70e0 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -833,7 +833,7 @@ static void u32_replace_knode(struct tcf_proto *tp, struct tc_u_common *tp_c,
if (pins->handle == n->handle)
break;
- idr_replace_ext(&ht->handle_idr, n, n->handle);
+ idr_replace(&ht->handle_idr, n, n->handle);
RCU_INIT_POINTER(n->next, pins->next);
rcu_assign_pointer(*ins, n);
}
--
2.15.0
^ permalink raw reply related
* [PATCH v2 08/17] idr: Add idr_alloc_u32 helper
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
All current users of idr_alloc_ext() actually want to allocate a u32 and
it's a little painful for them to use idr_alloc_ext(). This convenience
function makes it simple.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 12514ec0cd28..9b2fd6f408b2 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -139,6 +139,35 @@ void *idr_get_next_ext(struct idr *idr, unsigned long *nextid);
void *idr_replace(struct idr *, void *, unsigned long id);
void idr_destroy(struct idr *);
+/**
+ * idr_alloc_u32() - Allocate an ID.
+ * @idr: IDR handle.
+ * @ptr: Pointer to be associated with the new ID.
+ * @nextid: The new ID.
+ * @max: The maximum ID to allocate (inclusive).
+ * @gfp: Memory allocation flags.
+ *
+ * Allocates an unused ID in the range [*nextid, max] and updates the @nextid
+ * pointer with the newly assigned ID. Returns -ENOSPC and does not modify
+ * @nextid if there are no unused IDs in the range.
+ *
+ * The caller should provide their own locking to ensure that two concurrent
+ * modifications to the IDR are not possible. Read-only accesses to the
+ * IDR may be done under the RCU read lock or may exclude simultaneous
+ * writers.
+ *
+ * Return: 0 on success, -ENOMEM for memory allocation errors, -ENOSPC if
+ * there are no free IDs in the range.
+ */
+static inline int __must_check idr_alloc_u32(struct idr *idr, void *ptr,
+ u32 *nextid, unsigned long max, gfp_t gfp)
+{
+ unsigned long tmp = *nextid;
+ int ret = idr_alloc_ext(idr, ptr, &tmp, tmp, max + 1, gfp);
+ *nextid = tmp;
+ return ret;
+}
+
static inline void *idr_remove(struct idr *idr, unsigned long id)
{
return radix_tree_delete_item(&idr->idr_rt, id, NULL);
--
2.15.0
^ permalink raw reply related
* [PATCH v2 07/17] idr: Delete idr_find_ext function
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Simply changing idr_remove's 'id' argument to 'unsigned long' works
for all callers.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 7 +------
net/sched/act_api.c | 2 +-
net/sched/cls_flower.c | 2 +-
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 90dbe7a3735c..12514ec0cd28 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -179,16 +179,11 @@ static inline void idr_preload_end(void)
* This function can be called under rcu_read_lock(), given that the leaf
* pointers lifetimes are correctly managed.
*/
-static inline void *idr_find_ext(const struct idr *idr, unsigned long id)
+static inline void *idr_find(const struct idr *idr, unsigned long id)
{
return radix_tree_lookup(&idr->idr_rt, id);
}
-static inline void *idr_find(const struct idr *idr, int id)
-{
- return idr_find_ext(idr, id);
-}
-
/**
* idr_for_each_entry - iterate over an idr's elements of a given type
* @idr: idr handle
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 7e901e855d68..efb90b8a3bf0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -222,7 +222,7 @@ static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo *idrinfo)
struct tc_action *p = NULL;
spin_lock_bh(&idrinfo->lock);
- p = idr_find_ext(&idrinfo->action_idr, index);
+ p = idr_find(&idrinfo->action_idr, index);
spin_unlock_bh(&idrinfo->lock);
return p;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index ca71823bee03..ec0dc92f6104 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -329,7 +329,7 @@ static void *fl_get(struct tcf_proto *tp, u32 handle)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
- return idr_find_ext(&head->handle_idr, handle);
+ return idr_find(&head->handle_idr, handle);
}
static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
--
2.15.0
^ permalink raw reply related
* [PATCH v2 15/17] idr: Rename idr_alloc_ext to idr_alloc_ul
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
idr_alloc_ul fits better with other parts of the Linux kernel where we
need to name a function based on the types it operates on.
It uses a 'nextid' pointer argument instead of separate minimum ID and
output assigned ID, (like idr_get_next), reducing the number of arguments
by one. It also takes a 'max' argument rather than an 'end' argument
(unlike idr_alloc, but the semantics of 'end' don't work for unsigned long
arguments). And its return value is an errno, so mark it as __must_check.
Includes kernel-doc for idr_alloc_ul, which idr_alloc_ext didn't have,
and I realised we were missing a test-case where idr_alloc_cyclic wraps
around INT_MAX. Chris Mi <chrism@mellanox.com> has promised to contribute
test-cases for idr_alloc_ul.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 55 ++-------------------
include/linux/radix-tree.h | 17 +------
lib/idr.c | 99 +++++++++++++++++++++++++++++--------
lib/radix-tree.c | 3 +-
net/sched/cls_u32.c | 20 ++++----
tools/testing/radix-tree/idr-test.c | 17 +++++++
6 files changed, 111 insertions(+), 100 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 9b2fd6f408b2..344380fd0887 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -13,7 +13,6 @@
#define __IDR_H__
#include <linux/radix-tree.h>
-#include <linux/bug.h>
#include <linux/gfp.h>
#include <linux/percpu.h>
@@ -82,55 +81,9 @@ static inline void idr_set_cursor(struct idr *idr, unsigned int val)
void idr_preload(gfp_t gfp_mask);
-int idr_alloc_cmn(struct idr *idr, void *ptr, unsigned long *index,
- unsigned long start, unsigned long end, gfp_t gfp,
- bool ext);
-
-/**
- * idr_alloc - allocate an id
- * @idr: idr handle
- * @ptr: pointer to be associated with the new id
- * @start: the minimum id (inclusive)
- * @end: the maximum id (exclusive)
- * @gfp: memory allocation flags
- *
- * Allocates an unused ID in the range [start, end). Returns -ENOSPC
- * if there are no unused IDs in that range.
- *
- * Note that @end is treated as max when <= 0. This is to always allow
- * using @start + N as @end as long as N is inside integer range.
- *
- * Simultaneous modifications to the @idr are not allowed and should be
- * prevented by the user, usually with a lock. idr_alloc() may be called
- * concurrently with read-only accesses to the @idr, such as idr_find() and
- * idr_for_each_entry().
- */
-static inline int idr_alloc(struct idr *idr, void *ptr,
- int start, int end, gfp_t gfp)
-{
- unsigned long id;
- int ret;
-
- if (WARN_ON_ONCE(start < 0))
- return -EINVAL;
-
- ret = idr_alloc_cmn(idr, ptr, &id, start, end, gfp, false);
-
- if (ret)
- return ret;
-
- return id;
-}
-
-static inline int idr_alloc_ext(struct idr *idr, void *ptr,
- unsigned long *index,
- unsigned long start,
- unsigned long end,
- gfp_t gfp)
-{
- return idr_alloc_cmn(idr, ptr, index, start, end, gfp, true);
-}
-
+int idr_alloc(struct idr *, void *, int start, int end, gfp_t);
+int __must_check idr_alloc_ul(struct idr *, void *, unsigned long *nextid,
+ unsigned long max, gfp_t);
int idr_alloc_cyclic(struct idr *, void *entry, int start, int end, gfp_t);
int idr_for_each(const struct idr *,
int (*fn)(int id, void *p, void *data), void *data);
@@ -163,7 +116,7 @@ static inline int __must_check idr_alloc_u32(struct idr *idr, void *ptr,
u32 *nextid, unsigned long max, gfp_t gfp)
{
unsigned long tmp = *nextid;
- int ret = idr_alloc_ext(idr, ptr, &tmp, tmp, max + 1, gfp);
+ int ret = idr_alloc_ul(idr, ptr, &tmp, max, gfp);
*nextid = tmp;
return ret;
}
diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
index 23a9c89c7ad9..fc55ff31eca7 100644
--- a/include/linux/radix-tree.h
+++ b/include/linux/radix-tree.h
@@ -356,24 +356,9 @@ int radix_tree_split(struct radix_tree_root *, unsigned long index,
int radix_tree_join(struct radix_tree_root *, unsigned long index,
unsigned new_order, void *);
-void __rcu **idr_get_free_cmn(struct radix_tree_root *root,
+void __rcu **idr_get_free(struct radix_tree_root *root,
struct radix_tree_iter *iter, gfp_t gfp,
unsigned long max);
-static inline void __rcu **idr_get_free(struct radix_tree_root *root,
- struct radix_tree_iter *iter,
- gfp_t gfp,
- int end)
-{
- return idr_get_free_cmn(root, iter, gfp, end > 0 ? end - 1 : INT_MAX);
-}
-
-static inline void __rcu **idr_get_free_ext(struct radix_tree_root *root,
- struct radix_tree_iter *iter,
- gfp_t gfp,
- unsigned long end)
-{
- return idr_get_free_cmn(root, iter, gfp, end - 1);
-}
enum {
RADIX_TREE_ITER_TAG_MASK = 0x0f, /* tag index in lower nybble */
diff --git a/lib/idr.c b/lib/idr.c
index 577bfd4fe5c2..103afb97b4bd 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -1,4 +1,5 @@
#include <linux/bitmap.h>
+#include <linux/bug.h>
#include <linux/export.h>
#include <linux/idr.h>
#include <linux/slab.h>
@@ -7,32 +8,85 @@
DEFINE_PER_CPU(struct ida_bitmap *, ida_bitmap);
static DEFINE_SPINLOCK(simple_ida_lock);
-int idr_alloc_cmn(struct idr *idr, void *ptr, unsigned long *index,
- unsigned long start, unsigned long end, gfp_t gfp,
- bool ext)
+/**
+ * idr_alloc_ul() - Allocate a large ID.
+ * @idr: IDR handle.
+ * @ptr: Pointer to be associated with the new ID.
+ * @nextid: Pointer to minimum and new ID.
+ * @max: The maximum ID to allocate (inclusive).
+ * @gfp: Memory allocation flags.
+ *
+ * Allocates an unused ID in the range [*nextid, max] and updates the @nextid
+ * pointer with the newly assigned ID. Note that @max differs by 1 from the
+ * @end parameter to idr_alloc().
+ *
+ * The caller should provide their own locking to ensure that two concurrent
+ * modifications to the IDR are not possible. Read-only accesses to the
+ * IDR may be done under the RCU read lock or may exclude simultaneous
+ * writers.
+ *
+ * Return: 0 on success, -ENOMEM for memory allocation errors, -ENOSPC if
+ * there are no free IDs in the range.
+ */
+int idr_alloc_ul(struct idr *idr, void *ptr, unsigned long *nextid,
+ unsigned long max, gfp_t gfp)
{
struct radix_tree_iter iter;
void __rcu **slot;
if (WARN_ON_ONCE(radix_tree_is_internal_node(ptr)))
return -EINVAL;
+ if (WARN_ON_ONCE(!(idr->idr_rt.gfp_mask & ROOT_IS_IDR)))
+ idr->idr_rt.gfp_mask |= IDR_RT_MARKER;
- radix_tree_iter_init(&iter, start);
- if (ext)
- slot = idr_get_free_ext(&idr->idr_rt, &iter, gfp, end);
- else
- slot = idr_get_free(&idr->idr_rt, &iter, gfp, end);
+ radix_tree_iter_init(&iter, *nextid);
+ slot = idr_get_free(&idr->idr_rt, &iter, gfp, max);
if (IS_ERR(slot))
return PTR_ERR(slot);
radix_tree_iter_replace(&idr->idr_rt, &iter, slot, ptr);
radix_tree_iter_tag_clear(&idr->idr_rt, &iter, IDR_FREE);
- if (index)
- *index = iter.index;
+ *nextid = iter.index;
return 0;
}
-EXPORT_SYMBOL_GPL(idr_alloc_cmn);
+EXPORT_SYMBOL_GPL(idr_alloc_ul);
+
+/**
+ * idr_alloc - allocate an id
+ * @idr: idr handle
+ * @ptr: pointer to be associated with the new id
+ * @start: the minimum id (inclusive)
+ * @end: the maximum id (exclusive)
+ * @gfp: memory allocation flags
+ *
+ * Allocates an unused ID in the range [start, end). Returns -ENOSPC
+ * if there are no unused IDs in that range.
+ *
+ * Note that @end is treated as max when <= 0. This is to always allow
+ * using @start + N as @end as long as N is inside integer range.
+ *
+ * Simultaneous modifications to the @idr are not allowed and should be
+ * prevented by the user, usually with a lock. idr_alloc() may be called
+ * concurrently with read-only accesses to the @idr, such as idr_find() and
+ * idr_for_each_entry().
+ */
+int idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp)
+{
+ unsigned long id = start;
+ int ret;
+
+ if (WARN_ON_ONCE(start < 0))
+ return -EINVAL;
+
+ ret = idr_alloc_ul(idr, ptr, &id, end > 0 ? end - 1 : INT_MAX, gfp);
+
+ if (ret)
+ return ret;
+
+ return id;
+}
+EXPORT_SYMBOL_GPL(idr_alloc);
/**
* idr_alloc_cyclic - allocate new idr entry in a cyclical fashion
@@ -48,18 +102,21 @@ EXPORT_SYMBOL_GPL(idr_alloc_cmn);
*/
int idr_alloc_cyclic(struct idr *idr, void *ptr, int start, int end, gfp_t gfp)
{
- int id, curr = idr->idr_next;
-
- if (curr < start)
- curr = start;
+ unsigned long id = idr->idr_next;
+ int err, max = end > 0 ? end - 1 : INT_MAX;
- id = idr_alloc(idr, ptr, curr, end, gfp);
- if ((id == -ENOSPC) && (curr > start))
- id = idr_alloc(idr, ptr, start, curr, gfp);
+ if ((int)id < start)
+ id = start;
- if (id >= 0)
- idr->idr_next = id + 1U;
+ err = idr_alloc_ul(idr, ptr, &id, max, gfp);
+ if ((err == -ENOSPC) && (id > start)) {
+ id = start;
+ err = idr_alloc_ul(idr, ptr, &id, max, gfp);
+ }
+ if (err)
+ return err;
+ idr->idr_next = id + 1;
return id;
}
EXPORT_SYMBOL(idr_alloc_cyclic);
@@ -226,7 +283,7 @@ EXPORT_SYMBOL(idr_replace);
* bitmap, which is excessive.
*/
-#define IDA_MAX (0x80000000U / IDA_BITMAP_BITS)
+#define IDA_MAX (0x80000000U / IDA_BITMAP_BITS - 1)
/**
* ida_get_new_above - allocate new ID above or equal to a start id
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index c8d55565fafa..0a7ae3288a24 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -24,6 +24,7 @@
#include <linux/bitmap.h>
#include <linux/bitops.h>
+#include <linux/bug.h>
#include <linux/cpu.h>
#include <linux/errno.h>
#include <linux/export.h>
@@ -2135,7 +2136,7 @@ int ida_pre_get(struct ida *ida, gfp_t gfp)
}
EXPORT_SYMBOL(ida_pre_get);
-void __rcu **idr_get_free_cmn(struct radix_tree_root *root,
+void __rcu **idr_get_free(struct radix_tree_root *root,
struct radix_tree_iter *iter, gfp_t gfp,
unsigned long max)
{
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index e433d1adccc8..3b4523059862 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -730,19 +730,17 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last)
static u32 gen_new_kid(struct tc_u_hnode *ht, u32 htid)
{
- unsigned long idr_index;
- u32 start = htid | 0x800;
- u32 max = htid | 0xFFF;
- u32 min = htid;
-
- if (idr_alloc_ext(&ht->handle_idr, NULL, &idr_index,
- start, max + 1, GFP_KERNEL)) {
- if (idr_alloc_ext(&ht->handle_idr, NULL, &idr_index,
- min + 1, max + 1, GFP_KERNEL))
- return max;
+ unsigned long index = htid | 0x800;
+ unsigned long max = htid | 0xFFF;
+
+ if (idr_alloc_ul(&ht->handle_idr, NULL, &index, max, GFP_KERNEL)) {
+ index = htid + 1;
+ if (idr_alloc_ul(&ht->handle_idr, NULL, &index, max,
+ GFP_KERNEL))
+ index = max;
}
- return (u32)idr_index;
+ return index;
}
static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 892ef8855b02..36437ade429c 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -215,6 +215,23 @@ void idr_checks(void)
assert(idr_is_empty(&idr));
+ idr_set_cursor(&idr, INT_MAX - 3UL);
+ for (i = INT_MAX - 3UL; i < INT_MAX + 3UL; i++) {
+ struct item *item;
+ unsigned int id;
+ if (i <= INT_MAX)
+ item = item_create(i, 0);
+ else
+ item = item_create(i - INT_MAX - 1, 0);
+
+ id = idr_alloc_cyclic(&idr, item, 0, 0, GFP_KERNEL);
+ assert(id == item->index);
+ }
+
+ idr_for_each(&idr, item_idr_free, &idr);
+ idr_destroy(&idr);
+ assert(idr_is_empty(&idr));
+
for (i = 1; i < 10000; i++) {
struct item *item = item_create(i, 0);
assert(idr_alloc(&idr, item, 1, 20000, GFP_KERNEL) == i);
--
2.15.0
^ permalink raw reply related
* [PATCH v2 16/17] idr: Rename idr_for_each_entry_ext
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Match idr_alloc_ul with idr_get_next_ul and idr_for_each_entry_ul.
Also add kernel-doc.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 17 ++++++++++++++---
lib/idr.c | 20 +++++++++++++++-----
net/sched/act_api.c | 6 +++---
3 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 344380fd0887..91d27a9bcdf4 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -88,7 +88,7 @@ int idr_alloc_cyclic(struct idr *, void *entry, int start, int end, gfp_t);
int idr_for_each(const struct idr *,
int (*fn)(int id, void *p, void *data), void *data);
void *idr_get_next(struct idr *, int *nextid);
-void *idr_get_next_ext(struct idr *idr, unsigned long *nextid);
+void *idr_get_next_ul(struct idr *, unsigned long *nextid);
void *idr_replace(struct idr *, void *, unsigned long id);
void idr_destroy(struct idr *);
@@ -178,8 +178,19 @@ static inline void *idr_find(const struct idr *idr, unsigned long id)
*/
#define idr_for_each_entry(idr, entry, id) \
for (id = 0; ((entry) = idr_get_next(idr, &(id))) != NULL; ++id)
-#define idr_for_each_entry_ext(idr, entry, id) \
- for (id = 0; ((entry) = idr_get_next_ext(idr, &(id))) != NULL; ++id)
+
+/**
+ * idr_for_each_entry_ul() - iterate over an IDR's elements of a given type.
+ * @idr: IDR handle.
+ * @entry: The type * to use as cursor.
+ * @id: Entry ID.
+ *
+ * @entry and @id do not need to be initialized before the loop, and
+ * after normal terminatinon @entry is left with the value NULL. This
+ * is convenient for a "not found" value.
+ */
+#define idr_for_each_entry_ul(idr, entry, id) \
+ for (id = 0; ((entry) = idr_get_next_ul(idr, &(id))) != NULL; ++id)
/**
* idr_for_each_entry_continue - continue iteration over an idr's elements of a given type
diff --git a/lib/idr.c b/lib/idr.c
index 103afb97b4bd..772a24513d1e 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -155,9 +155,9 @@ int idr_for_each(const struct idr *idr,
EXPORT_SYMBOL(idr_for_each);
/**
- * idr_get_next - Find next populated entry
- * @idr: idr handle
- * @nextid: Pointer to lowest possible ID to return
+ * idr_get_next() - Find next populated entry.
+ * @idr: IDR handle.
+ * @nextid: Pointer to lowest possible ID to return.
*
* Returns the next populated entry in the tree with an ID greater than
* or equal to the value pointed to by @nextid. On exit, @nextid is updated
@@ -178,7 +178,17 @@ void *idr_get_next(struct idr *idr, int *nextid)
}
EXPORT_SYMBOL(idr_get_next);
-void *idr_get_next_ext(struct idr *idr, unsigned long *nextid)
+/**
+ * idr_get_next_ul() - Find next populated entry.
+ * @idr: IDR handle.
+ * @nextid: Pointer to lowest possible ID to return.
+ *
+ * Returns the next populated entry in the tree with an ID greater than
+ * or equal to the value pointed to by @nextid. On exit, @nextid is updated
+ * to the ID of the found value. To use in a loop, the value pointed to by
+ * nextid must be incremented by the user.
+ */
+void *idr_get_next_ul(struct idr *idr, unsigned long *nextid)
{
struct radix_tree_iter iter;
void __rcu **slot;
@@ -190,7 +200,7 @@ void *idr_get_next_ext(struct idr *idr, unsigned long *nextid)
*nextid = iter.index;
return rcu_dereference_raw(*slot);
}
-EXPORT_SYMBOL(idr_get_next_ext);
+EXPORT_SYMBOL(idr_get_next_ul);
/**
* idr_replace - replace pointer for given id
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 156302c110af..4133d91b7029 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -124,7 +124,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
s_i = cb->args[0];
- idr_for_each_entry_ext(idr, p, id) {
+ idr_for_each_entry_ul(idr, p, id) {
index++;
if (index < s_i)
continue;
@@ -181,7 +181,7 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
if (nla_put_string(skb, TCA_KIND, ops->kind))
goto nla_put_failure;
- idr_for_each_entry_ext(idr, p, id) {
+ idr_for_each_entry_ul(idr, p, id) {
ret = __tcf_idr_release(p, false, true);
if (ret == ACT_P_DELETED) {
module_put(ops->owner);
@@ -351,7 +351,7 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
int ret;
unsigned long id = 1;
- idr_for_each_entry_ext(idr, p, id) {
+ idr_for_each_entry_ul(idr, p, id) {
ret = __tcf_idr_release(p, false, true);
if (ret == ACT_P_DELETED)
module_put(ops->owner);
--
2.15.0
^ permalink raw reply related
* [PATCH v2 14/17] cls_u32: Convert to idr_alloc_u32
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
No real benefit to this classifier, but since we're allocating a u32
anyway, we should use this function.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
net/sched/cls_u32.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index e65b47483dc0..e433d1adccc8 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -970,8 +970,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return -ENOMEM;
}
} else {
- err = idr_alloc_ext(&tp_c->handle_idr, ht, NULL,
- handle, handle + 1, GFP_KERNEL);
+ err = idr_alloc_u32(&tp_c->handle_idr, ht, &handle,
+ handle, GFP_KERNEL);
if (err) {
kfree(ht);
return err;
@@ -1020,8 +1020,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
if (TC_U32_HTID(handle) && TC_U32_HTID(handle^htid))
return -EINVAL;
handle = htid | TC_U32_NODE(handle);
- err = idr_alloc_ext(&ht->handle_idr, NULL, NULL,
- handle, handle + 1,
+ err = idr_alloc_u32(&ht->handle_idr, NULL, &handle, handle,
GFP_KERNEL);
if (err)
return err;
--
2.15.0
^ permalink raw reply related
* [PATCH v2 17/17] idr: Warn if old iterators see large IDs
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Now that the IDR can be used to store large IDs, it is possible somebody
might only partially convert their old code and use the iterators which
can only handle IDs up to INT_MAX. It's probably unwise to show them a
truncated ID, so settle for spewing warnings to dmesg, and terminating
the iteration.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
lib/idr.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/idr.c b/lib/idr.c
index 772a24513d1e..1aaeb5a8c426 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -145,7 +145,11 @@ int idr_for_each(const struct idr *idr,
void __rcu **slot;
radix_tree_for_each_slot(slot, &idr->idr_rt, &iter, 0) {
- int ret = fn(iter.index, rcu_dereference_raw(*slot), data);
+ int ret;
+
+ if (WARN_ON_ONCE(iter.index > INT_MAX))
+ break;
+ ret = fn(iter.index, rcu_dereference_raw(*slot), data);
if (ret)
return ret;
}
@@ -173,6 +177,9 @@ void *idr_get_next(struct idr *idr, int *nextid)
if (!slot)
return NULL;
+ if (WARN_ON_ONCE(iter.index > INT_MAX))
+ return NULL;
+
*nextid = iter.index;
return rcu_dereference_raw(*slot);
}
--
2.15.0
^ permalink raw reply related
* [PATCH v2 05/17] idr: Delete idr_remove_ext function
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Simply changing idr_remove's 'id' argument to 'unsigned long' suffices
for all callers.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 7 +------
net/sched/act_api.c | 2 +-
net/sched/cls_basic.c | 6 +++---
net/sched/cls_bpf.c | 4 ++--
net/sched/cls_flower.c | 4 ++--
net/sched/cls_u32.c | 8 ++++----
6 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index dd048cf456b7..9a4042489ec6 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -140,16 +140,11 @@ void *idr_replace(struct idr *, void *, int id);
void *idr_replace_ext(struct idr *idr, void *ptr, unsigned long id);
void idr_destroy(struct idr *);
-static inline void *idr_remove_ext(struct idr *idr, unsigned long id)
+static inline void *idr_remove(struct idr *idr, unsigned long id)
{
return radix_tree_delete_item(&idr->idr_rt, id, NULL);
}
-static inline void *idr_remove(struct idr *idr, int id)
-{
- return idr_remove_ext(idr, id);
-}
-
static inline void idr_init(struct idr *idr)
{
INIT_RADIX_TREE(&idr->idr_rt, IDR_RT_MARKER);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 4d33a50a8a6d..bab81574a420 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -78,7 +78,7 @@ static void free_tcf(struct tc_action *p)
static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, struct tc_action *p)
{
spin_lock_bh(&idrinfo->lock);
- idr_remove_ext(&idrinfo->action_idr, p->tcfa_index);
+ idr_remove(&idrinfo->action_idr, p->tcfa_index);
spin_unlock_bh(&idrinfo->lock);
gen_kill_estimator(&p->tcfa_rate_est);
free_tcf(p);
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 5f169ded347e..d2193304bad0 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -120,7 +120,7 @@ static void basic_destroy(struct tcf_proto *tp)
list_for_each_entry_safe(f, n, &head->flist, link) {
list_del_rcu(&f->link);
tcf_unbind_filter(tp, &f->res);
- idr_remove_ext(&head->handle_idr, f->handle);
+ idr_remove(&head->handle_idr, f->handle);
if (tcf_exts_get_net(&f->exts))
call_rcu(&f->rcu, basic_delete_filter);
else
@@ -137,7 +137,7 @@ static int basic_delete(struct tcf_proto *tp, void *arg, bool *last)
list_del_rcu(&f->link);
tcf_unbind_filter(tp, &f->res);
- idr_remove_ext(&head->handle_idr, f->handle);
+ idr_remove(&head->handle_idr, f->handle);
tcf_exts_get_net(&f->exts);
call_rcu(&f->rcu, basic_delete_filter);
*last = list_empty(&head->flist);
@@ -224,7 +224,7 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
err = basic_set_parms(net, tp, fnew, base, tb, tca[TCA_RATE], ovr);
if (err < 0) {
if (!fold)
- idr_remove_ext(&head->handle_idr, fnew->handle);
+ idr_remove(&head->handle_idr, fnew->handle);
goto errout;
}
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 6fe798c2df1a..b017d99fd7e1 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -299,7 +299,7 @@ static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
- idr_remove_ext(&head->handle_idr, prog->handle);
+ idr_remove(&head->handle_idr, prog->handle);
cls_bpf_stop_offload(tp, prog);
list_del_rcu(&prog->link);
tcf_unbind_filter(tp, &prog->res);
@@ -542,7 +542,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
cls_bpf_free_parms(prog);
errout_idr:
if (!oldprog)
- idr_remove_ext(&head->handle_idr, prog->handle);
+ idr_remove(&head->handle_idr, prog->handle);
errout:
tcf_exts_destroy(&prog->exts);
kfree(prog);
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 543a3e875d05..3e89b0be1706 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -283,7 +283,7 @@ static void __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
- idr_remove_ext(&head->handle_idr, f->handle);
+ idr_remove(&head->handle_idr, f->handle);
list_del_rcu(&f->list);
if (!tc_skip_hw(f->flags))
fl_hw_destroy_filter(tp, f);
@@ -972,7 +972,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
errout_idr:
if (fnew->handle)
- idr_remove_ext(&head->handle_idr, fnew->handle);
+ idr_remove(&head->handle_idr, fnew->handle);
errout:
tcf_exts_destroy(&fnew->exts);
kfree(fnew);
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index ac152b4f4247..6fe4e3549ad3 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -591,7 +591,7 @@ static void u32_clear_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht)
rtnl_dereference(n->next));
tcf_unbind_filter(tp, &n->res);
u32_remove_hw_knode(tp, n->handle);
- idr_remove_ext(&ht->handle_idr, n->handle);
+ idr_remove(&ht->handle_idr, n->handle);
if (tcf_exts_get_net(&n->exts))
call_rcu(&n->rcu, u32_delete_key_freepf_rcu);
else
@@ -617,7 +617,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht)
if (phn == ht) {
u32_clear_hw_hnode(tp, ht);
idr_destroy(&ht->handle_idr);
- idr_remove_ext(&tp_c->handle_idr, ht->handle);
+ idr_remove(&tp_c->handle_idr, ht->handle);
RCU_INIT_POINTER(*hn, ht->next);
kfree_rcu(ht, rcu);
return 0;
@@ -992,7 +992,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
err = u32_replace_hw_hnode(tp, ht, flags);
if (err) {
- idr_remove_ext(&tp_c->handle_idr, handle);
+ idr_remove(&tp_c->handle_idr, handle);
kfree(ht);
return err;
}
@@ -1120,7 +1120,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
#endif
kfree(n);
erridr:
- idr_remove_ext(&ht->handle_idr, handle);
+ idr_remove(&ht->handle_idr, handle);
return err;
}
--
2.15.0
^ permalink raw reply related
* [PATCH v2 11/17] cls_bpf: Convert to use idr_alloc_u32
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Use the new helper. This has a modest reduction in both lines of code
and compiled code size.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
net/sched/cls_bpf.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 1660fc8294ef..db1dd4de7d6a 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -472,7 +472,6 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
struct cls_bpf_prog *oldprog = *arg;
struct nlattr *tb[TCA_BPF_MAX + 1];
struct cls_bpf_prog *prog;
- unsigned long idr_index;
int ret;
if (tca[TCA_OPTIONS] == NULL)
@@ -499,21 +498,18 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
}
if (handle == 0) {
- ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
- 1, 0x7FFFFFFF, GFP_KERNEL);
- if (ret)
- goto errout;
- prog->handle = idr_index;
- } else {
- if (!oldprog) {
- ret = idr_alloc_ext(&head->handle_idr, prog, &idr_index,
- handle, handle + 1, GFP_KERNEL);
- if (ret)
- goto errout;
- }
- prog->handle = handle;
+ handle = 1;
+ ret = idr_alloc_u32(&head->handle_idr, prog, &handle,
+ INT_MAX, GFP_KERNEL);
+ } else if (!oldprog) {
+ ret = idr_alloc_u32(&head->handle_idr, prog, &handle,
+ handle, GFP_KERNEL);
}
+ if (ret)
+ goto errout;
+ prog->handle = handle;
+
ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], ovr);
if (ret < 0)
goto errout_idr;
--
2.15.0
^ permalink raw reply related
* [PATCH v2 10/17] cls_basic: Convert to use idr_alloc_u32
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
Use the new helper which saves a temporary variable and a few lines of
code.
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
net/sched/cls_basic.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 147700afcf31..c4b242fee8e4 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -182,7 +182,6 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
struct nlattr *tb[TCA_BASIC_MAX + 1];
struct basic_filter *fold = (struct basic_filter *) *arg;
struct basic_filter *fnew;
- unsigned long idr_index;
if (tca[TCA_OPTIONS] == NULL)
return -EINVAL;
@@ -205,21 +204,17 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
if (err < 0)
goto errout;
- if (handle) {
- fnew->handle = handle;
- if (!fold) {
- err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
- handle, handle + 1, GFP_KERNEL);
- if (err)
- goto errout;
- }
- } else {
- err = idr_alloc_ext(&head->handle_idr, fnew, &idr_index,
- 1, 0x7FFFFFFF, GFP_KERNEL);
- if (err)
- goto errout;
- fnew->handle = idr_index;
+ if (!handle) {
+ handle = 1;
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ INT_MAX, GFP_KERNEL);
+ } else if (!fold) {
+ err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
+ handle, GFP_KERNEL);
}
+ if (err)
+ goto errout;
+ fnew->handle = handle;
err = basic_set_parms(net, tp, fnew, base, tb, tca[TCA_RATE], ovr);
if (err < 0) {
--
2.15.0
^ permalink raw reply related
* [PATCH v2 01/17] idr: Fix build
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
The IDR calls WARN_ON without including <linux/bug.h>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
include/linux/idr.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 7c3a365f7e12..dd048cf456b7 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -13,6 +13,7 @@
#define __IDR_H__
#include <linux/radix-tree.h>
+#include <linux/bug.h>
#include <linux/gfp.h>
#include <linux/percpu.h>
--
2.15.0
^ permalink raw reply related
* [PATCH v2 03/17] idr test suite: Fix ida_test_random()
From: Matthew Wilcox @ 2017-11-29 20:19 UTC (permalink / raw)
To: willy, netdev; +Cc: Matthew Wilcox
In-Reply-To: <20171129201922.24370-1-willy@infradead.org>
From: Matthew Wilcox <mawilcox@microsoft.com>
The test was checking the wrong errno; ida_get_new_above() returns
EAGAIN, not ENOMEM on memory allocation failure. Double the number of
threads to increase the chance that we actually exercise this path
during the test suite (it was a bit sporadic before).
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
tools/testing/radix-tree/idr-test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 30cd0b296f1a..193450b29bf0 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c
@@ -380,7 +380,7 @@ void ida_check_random(void)
do {
ida_pre_get(&ida, GFP_KERNEL);
err = ida_get_new_above(&ida, bit, &id);
- } while (err == -ENOMEM);
+ } while (err == -EAGAIN);
assert(!err);
assert(id == bit);
}
@@ -489,7 +489,7 @@ static void *ida_random_fn(void *arg)
void ida_thread_tests(void)
{
- pthread_t threads[10];
+ pthread_t threads[20];
int i;
for (i = 0; i < ARRAY_SIZE(threads); i++)
--
2.15.0
^ permalink raw reply related
* Re: [PATCH] [RFC v3] packet: experimental support for 64-bit timestamps
From: Arnd Bergmann @ 2017-11-29 20:06 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David S. Miller, Willem de Bruijn, Björn Töpel,
Richard Cochran, Thomas Gleixner, Mike Maloney, Eric Dumazet,
Kees Cook, Hans Liljestrand, Andrey Konovalov, Rosen, Rami,
Reshetova, Elena, Sowmini Varadhan, Network Development, LKML
In-Reply-To: <CAF=yD-+UOPN+fJSHDp7ytiABM1aS1JD6Q-o-nvBc6d2Wzet6Qg@mail.gmail.com>
On Wed, Nov 29, 2017 at 5:51 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>> Thanks for the review! Any suggestions for how to do the testing? If you have
>> existing test cases, could you give my next version a test run to see if there
>> are any regressions and if the timestamps work as expected?
>>
>> I see that there are test cases in tools/testing/selftests/net/, but none
>> of them seem to use the time stamps so far, and I'm not overly familiar
>> with how it works in the details to extend it in a meaningful way.
>
> I could not find any good tests for this interface, either. The only
> user of the interface I found was a little tool I wrote a few years
> ago that compares timestamps at multiple points in the transmit
> path for latency measurement [1]. But it may be easier to just write
> a new test under tools/testing/selftests/net for this purpose. I can
> help with that, too, if you want.
Thanks, that would be great!
Arnd
^ permalink raw reply
* Re: [PATCH net] net: phylink: fix link state on phy-connect
From: Russell King - ARM Linux @ 2017-11-29 19:59 UTC (permalink / raw)
To: Yan Markman
Cc: Antoine Tenart, andrew@lunn.ch, f.fainelli@gmail.com,
davem@davemloft.net, gregory.clement@free-electrons.com,
thomas.petazzoni@free-electrons.com,
miquel.raynal@free-electrons.com, Nadav Haklai, mw@semihalf.com,
Stefan Chulski, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20448667430e434aad5bb8cd1b082611@IL-EXCH01.marvell.com>
[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]
On Wed, Nov 29, 2017 at 07:33:44PM +0000, Yan Markman wrote:
> Hi Russel
>
> On my board I have [Marvell 88E1510] phy working with STATUS-POLLING
> I see some inconsistencies -- first ifconfig-up is different from furthers, no "link is down" reports.
> Please refer the behavior example below.
> My patch is a "simple solution" -- always reset/clear Link-state-parameters before going UP.
> Possibly, more correct (but much more complicated) solution would be in the phy state machine and phylink resolve modification.
> I just found that
> On ifconfig-down, the phy-state-machine and phylink-resolve
> are stopped before executing before passing over full graceful down/reset state.
> The further ifconfig-up starts with old state parameters.
> Special cases not-tested but logic 2 test-cases are:
> remote side changes speed whilst link is Down or Disconnected. But local ifconfig-up starts with old speed.
Hi,
I think this is covered in my "phy" branch - but could probably do with
further testing, specifically this patch (which I've attached):
"phylink: ensure we take the link down when phylink_stop() is called"
This takes the link down on the MAC side synchronously when phylink_stop()
is called. However, I think your case might also benefit from this
patch - please test the patch referred to without this change, and let
me know if you need this change to solve your problem:
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 8f43f8779317..c90ad50204b0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -798,6 +798,7 @@ void phylink_disconnect_phy(struct phylink *pl)
mutex_lock(&pl->state_mutex);
pl->netdev->phydev = NULL;
pl->phydev = NULL;
+ pl->phy_state.link = false;
mutex_unlock(&pl->state_mutex);
mutex_unlock(&phy->lock);
flush_work(&pl->resolve);
Thanks.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
[-- Attachment #2: p21 --]
[-- Type: text/plain, Size: 909 bytes --]
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] phylink: ensure we take the link down when phylink_stop() is
called
Ensure that we tell the MAC to take the link down when phylink_stop()
is called.
Reported-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fb7ae3f925f8..cb446b8acac2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -880,6 +880,7 @@ void phylink_stop(struct phylink *pl)
sfp_upstream_stop(pl->sfp_bus);
set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
+ queue_work(system_power_efficient_wq, &pl->resolve);
flush_work(&pl->resolve);
}
EXPORT_SYMBOL_GPL(phylink_stop);
--
2.7.4
^ permalink raw reply related
* Re: [BUG] kernel stack corruption during/after Netlabel error
From: Stephen Smalley @ 2017-11-29 19:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, selinux-+05T5uksL2qpZYMLLGbcSA
In-Reply-To: <CANn89iJc=tZkN41WoCm5Zy9nPfs1tfZf9nuSXYS9EB_aem+y4g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, 2017-11-29 at 09:34 -0800, Eric Dumazet wrote:
> On Wed, Nov 29, 2017 at 9:31 AM, Stephen Smalley <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
> wrote:
> > On Wed, 2017-11-29 at 21:26 +1100, James Morris wrote:
> > > I'm seeing a kernel stack corruption bug (detected via gcc) when
> > > running
> > > the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd
> > > inet_socket
> > > test:
> > >
> > > https://github.com/SELinuxProject/selinux-testsuite/blob/master/t
> > > ests
> > > /inet_socket/test
> > >
> > > # Verify that unauthorized client cannot communicate with the
> > > server.
> > > $result = system
> > > "runcon -t test_inet_bad_client_t -- $basedir/client stream
> > > 127.0.0.1 65535 2>&1";
> > >
> > > This correctlly causes an access control error in the Netlabel
> > > code,
> > > and
> > > the bug seems to be triggered during the ICMP send:
> > >
> > > [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to
> > > parse packet
> > > [ 339.822505] Kernel panic - not syncing: stack-protector:
> > > Kernel
> > > stack is corrupted in: ffffffff81745af5
> > > [ 339.822505]
> > > [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-
> > > rc1-
> > > test #15
> > > [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS
> > > FWKT68A 01/19/2017
> > > [ 339.885060] Call Trace:
> > > [ 339.896875] <IRQ>
> > > [ 339.908103] dump_stack+0x63/0x87
> > > [ 339.920645] panic+0xe8/0x248
> > > [ 339.932668] ? ip_push_pending_frames+0x33/0x40
> > > [ 339.946328] ? icmp_send+0x525/0x530
> > > [ 339.958861] ? kfree_skbmem+0x60/0x70
> > > [ 339.971431] __stack_chk_fail+0x1b/0x20
> > > [ 339.984049] icmp_send+0x525/0x530
> > > [ 339.996205] ? netlbl_skbuff_err+0x36/0x40
> > > [ 340.008997] ? selinux_netlbl_err+0x11/0x20
> > > [ 340.021816] ? selinux_socket_sock_rcv_skb+0x211/0x230
> > > [ 340.035529] ? security_sock_rcv_skb+0x3b/0x50
> > > [ 340.048471] ? sk_filter_trim_cap+0x44/0x1c0
> > > [ 340.061246] ? tcp_v4_inbound_md5_hash+0x69/0x1b0
> > > [ 340.074562] ? tcp_filter+0x2c/0x40
> > > [ 340.086400] ? tcp_v4_rcv+0x820/0xa20
> > > [ 340.098329] ? ip_local_deliver_finish+0x71/0x1a0
> > > [ 340.111279] ? ip_local_deliver+0x6f/0xe0
> > > [ 340.123535] ? ip_rcv_finish+0x3a0/0x3a0
> > > [ 340.135523] ? ip_rcv_finish+0xdb/0x3a0
> > > [ 340.147442] ? ip_rcv+0x27c/0x3c0
> > > [ 340.158668] ? inet_del_offload+0x40/0x40
> > > [ 340.170580] ? __netif_receive_skb_core+0x4ac/0x900
> > > [ 340.183285] ? rcu_accelerate_cbs+0x5b/0x80
> > > [ 340.195282] ? __netif_receive_skb+0x18/0x60
> > > [ 340.207288] ? process_backlog+0x95/0x140
> > > [ 340.218948] ? net_rx_action+0x26c/0x3b0
> > > [ 340.230416] ? __do_softirq+0xc9/0x26a
> > > [ 340.241625] ? do_softirq_own_stack+0x2a/0x40
> > > [ 340.253368] </IRQ>
> > > [ 340.262673] ? do_softirq+0x50/0x60
> > > [ 340.273450] ? __local_bh_enable_ip+0x57/0x60
> > > [ 340.285045] ? ip_finish_output2+0x175/0x350
> > > [ 340.296403] ? ip_finish_output+0x127/0x1d0
> > > [ 340.307665] ? nf_hook_slow+0x3c/0xb0
> > > [ 340.318230] ? ip_output+0x72/0xe0
> > > [ 340.328524] ? ip_fragment.constprop.54+0x80/0x80
> > > [ 340.340070] ? ip_local_out+0x35/0x40
> > > [ 340.350497] ? ip_queue_xmit+0x15c/0x3f0
> > > [ 340.361060] ? __kmalloc_reserve.isra.40+0x31/0x90
> > > [ 340.372484] ? __skb_clone+0x2e/0x130
> > > [ 340.382633] ? tcp_transmit_skb+0x558/0xa10
> > > [ 340.393262] ? tcp_connect+0x938/0xad0
> > > [ 340.403370] ? ktime_get_with_offset+0x4c/0xb0
> > > [ 340.414206] ? tcp_v4_connect+0x457/0x4e0
> > > [ 340.424471] ? __inet_stream_connect+0xb3/0x300
> > > [ 340.435195] ? inet_stream_connect+0x3b/0x60
> > > [ 340.445607] ? SYSC_connect+0xd9/0x110
> > > [ 340.455455] ? __audit_syscall_entry+0xaf/0x100
> > > [ 340.466112] ? syscall_trace_enter+0x1d0/0x2b0
> > > [ 340.476636] ? __audit_syscall_exit+0x209/0x290
> > > [ 340.487151] ? SyS_connect+0xe/0x10
> > > [ 340.496453] ? do_syscall_64+0x67/0x1b0
> > > [ 340.506078] ? entry_SYSCALL64_slow_path+0x25/0x25
> > > [ 340.516693] Kernel Offset: disabled
> > > [ 340.526393] Rebooting in 11 seconds..
> > >
> > > This is mostly reliable, and I'm only seeing it on bare metal
> > > (not in
> > > a
> > > virtualbox vm).
> > >
> > > The SELinux skb parse error at the start only sometimes appears,
> > > and
> > > looking at the code, I suspect some kind of memory corruption
> > > being
> > > the
> > > cause at that point (basic packet header checks).
> > >
> > > I bisected the bug down to the following change:
> > >
> > > commit bffa72cf7f9df842f0016ba03586039296b4caaf
> > > Author: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > Date: Tue Sep 19 05:14:24 2017 -0700
> > >
> > > net: sk_buff rbnode reorg
> > > ...
> > >
> > >
> > > Anyone else able to reproduce this, or have any ideas on what's
> > > happening?
> >
> > So far I haven't been able to reproduce with 4.15-rc1 or -linus.
> >
>
> You might try adding KASAN in the picture ? ( CONFIG_KASAN=y )
Good idea:
==================================================================
BUG:
KASAN: stack-out-of-bounds in __ip_options_echo+0x430/0x5e0
Write of
size 44 at addr ffff8803bc1c7560 by task ksoftirqd/1/16
CPU: 1 PID: 16
Comm: ksoftirqd/1 Not tainted 4.15.0-rc1 #27
Hardware name: Dell Inc.
Latitude E7470/0VNKRJ, BIOS 1.16.4 06/02/2017
Call Trace:
dump_stack+0x5c/0x7c
print_address_description+0x6a/0x280
kasan_report+0x254/0x370
? __ip_options_echo+0x430/0x5e0
memcpy+0x34/0x50
__ip_options_echo+0x430/0x5e0
icmp_send+0x48d/0x7a0
? icmpv4_global_allow+0x50/0x50
?
selinux_netlbl_sk_security_reset+0x20/0x20
? avc_has_perm+0x238/0x260
? avc_has_perm_noaudit+0x1d0/0x1d0
? selinux_peerlbl_enabled+0x50/0x50
? deref_stack_reg+0xd0/0xd0
? __save_stack_trace+0x82/0x100
selinux_socket_sock_rcv_skb+0x341/0x350
?
selinux_sock_rcv_skb_compat+0x200/0x200
? depot_save_stack+0x12f/0x460
? pskb_expand_head+0x9d/0x4d0
? save_stack+0x92/0xa0
?
kasan_kmalloc+0xa0/0xd0
? __kmalloc_node_track_caller+0xf5/0x290
?
skb_copy_and_csum_dev+0x142/0x180
? ip_rcv_finish+0x323/0x690
?
__netif_receive_skb_core+0xe16/0x13d0
? process_backlog+0x10a/0x280
?
net_rx_action+0x3ec/0x5a0
? __do_softirq+0x13f/0x36d
?
__wake_up_common_lock+0xd7/0x130
? tcp_md5_do_lookup+0x27/0x240
security_sock_rcv_skb+0x47/0x60
sk_filter_trim_cap+0x45/0x4b0
?
tcp4_proc_exit+0x11/0x11
tcp_filter+0x5b/0x90
tcp_v4_rcv+0x108a/0x1360
ip_local_deliver_finish+0xf7/0x300
ip_local_deliver+0xf2/0x1a0
? ip_call_ra_chain+0x220/0x220
?
ip_rcv_finish+0x690/0x690
? ip_rcv_finish+0x1b9/0x690
ip_rcv+0x4a6/0x660
? ip_local_deliver+0x1a0/0x1a0
?
inet_del_offload+0x40/0x40
? cpumask_next_and+0x4e/0x70
?
ip_local_deliver+0x1a0/0x1a0
__netif_receive_skb_core+0xe16/0x13d0
?
netdev_info+0x100/0x100
? __accumulate_pelt_segments+0x47/0xd0
?
find_busiest_group+0x1100/0x1100
?
__update_load_avg_se.isra.31+0x34e/0x360
?
__update_load_avg_se.isra.31+0x201/0x360
?
__accumulate_pelt_segments+0x47/0xd0
? process_backlog+0x10a/0x280
process_backlog+0x10a/0x280
net_rx_action+0x3ec/0x5a0
?
napi_complete_done+0x180/0x180
? __schedule+0x4e0/0xd50
?
sched_clock_cpu+0x14/0xe0
__do_softirq+0x13f/0x36d
?
takeover_tasklets+0x2b0/0x2b0
run_ksoftirqd+0x25/0x40
smpboot_thread_fn+0x212/0x2b0
? sort_range+0x20/0x20
?
schedule+0x50/0xc0
kthread+0x174/0x1c0
? sort_range+0x20/0x20
?
kthread_create_worker_on_cpu+0xc0/0xc0
ret_from_fork+0x1f/0x30
The
buggy address belongs to the page:
page:ffffea000ef071c0 count:0
mapcount:0 mapping: (null) index:0x0
flags: 0x17ffffc0000000()
raw: 0017ffffc0000000 0000000000000000 0000000000000000
00000000ffffffff
raw: ffffea000ef071e0 ffffea000ef071e0
0000000000000000 0000000000000000
page dumped because: kasan: bad
access detected
Memory state around the buggy address:
ffff8803bc1c7480: 00 00 00 f2 f2 f2 f2 00 00 00 00 00 00 00 f4 f2
ffff8803bc1c7500: f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff8803bc1c7580: 00 f4 f4 00 00 00 00 00 00 00 00 00 00 00 00 00
^
ffff8803bc1c7600: 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2
f2 f2 f2 00 f4
ffff8803bc1c7680: f4 f4 f2 f2 f2 f2 00 00 00 00 f2 f2
f2 f2 00 00
==================================================================
Disabling lock debugging due to kernel taint
SELinux: failure in
selinux_parse_skb(), unable to parse packet
^ permalink raw reply
* Re: KASAN: use-after-free Read in sock_release
From: Cong Wang @ 2017-11-29 19:37 UTC (permalink / raw)
To: syzbot
Cc: David Miller, LKML, Linux Kernel Network Developers,
syzkaller-bugs, linux-fsdevel, Linus Torvalds, Al Viro
In-Reply-To: <94eb2c19e756c0119b055f1afbd0@google.com>
(Cc'ing fs people...)
On Wed, Nov 29, 2017 at 12:33 AM, syzbot
<bot+9abea25706ae35022385a41f61e579ed66e88a3f@syzkaller.appspotmail.com>
wrote:
> Hello,
>
> syzkaller hit the following crash on
> 1d3b78bbc6e983fabb3fbf91b76339bf66e4a12c
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
>
> Unfortunately, I don't have any reproducer for this bug yet.
>
>
> device syz3 left promiscuous mode
> device syz3 entered promiscuous mode
> ==================================================================
> BUG: KASAN: use-after-free in sock_release+0x1c6/0x1e0 net/socket.c:601
> Read of size 8 at addr ffff8801c8dd1d10 by task syz-executor4/31085
>
> CPU: 0 PID: 31085 Comm: syz-executor4 Not tainted 4.14.0+ #129
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x257 lib/dump_stack.c:53
> print_address_description+0x73/0x250 mm/kasan/report.c:252
> kasan_report_error mm/kasan/report.c:351 [inline]
> kasan_report+0x25b/0x340 mm/kasan/report.c:409
> __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:430
> sock_release+0x1c6/0x1e0 net/socket.c:601
> sock_close+0x16/0x20 net/socket.c:1125
> __fput+0x333/0x7f0 fs/file_table.c:210
> ____fput+0x15/0x20 fs/file_table.c:244
> task_work_run+0x199/0x270 kernel/task_work.c:113
> exit_task_work include/linux/task_work.h:22 [inline]
> do_exit+0x9bb/0x1ae0 kernel/exit.c:865
> do_group_exit+0x149/0x400 kernel/exit.c:968
> get_signal+0x73f/0x16c0 kernel/signal.c:2335
> do_signal+0x94/0x1ee0 arch/x86/kernel/signal.c:809
> exit_to_usermode_loop+0x214/0x310 arch/x86/entry/common.c:158
> prepare_exit_to_usermode arch/x86/entry/common.c:195 [inline]
> syscall_return_slowpath+0x490/0x550 arch/x86/entry/common.c:264
> entry_SYSCALL_64_fastpath+0x94/0x96
> RIP: 0033:0x452879
> RSP: 002b:00007fb1c2435ce8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
> RAX: fffffffffffffe00 RBX: 0000000000758100 RCX: 0000000000452879
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000758100
> RBP: 0000000000758100 R08: 0000000000000304 R09: 00000000007580d8
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 0000000000a6f7ff R14: 00007fb1c24369c0 R15: 000000000000000e
>
> Allocated by task 31066:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
> kmem_cache_alloc_trace+0x136/0x750 mm/slab.c:3613
> kmalloc include/linux/slab.h:499 [inline]
> sock_alloc_inode+0xb4/0x300 net/socket.c:253
> alloc_inode+0x65/0x180 fs/inode.c:208
> new_inode_pseudo+0x69/0x190 fs/inode.c:890
> sock_alloc+0x41/0x270 net/socket.c:565
> __sock_create+0x148/0x850 net/socket.c:1225
> sock_create net/socket.c:1301 [inline]
> SYSC_socket net/socket.c:1331 [inline]
> SyS_socket+0xeb/0x200 net/socket.c:1311
> entry_SYSCALL_64_fastpath+0x1f/0x96
>
> Freed by task 3039:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
> __cache_free mm/slab.c:3491 [inline]
> kfree+0xca/0x250 mm/slab.c:3806
> __rcu_reclaim kernel/rcu/rcu.h:190 [inline]
> rcu_do_batch kernel/rcu/tree.c:2758 [inline]
> invoke_rcu_callbacks kernel/rcu/tree.c:3012 [inline]
> __rcu_process_callbacks kernel/rcu/tree.c:2979 [inline]
> rcu_process_callbacks+0xe79/0x17d0 kernel/rcu/tree.c:2996
> __do_softirq+0x29d/0xbb2 kernel/softirq.c:285
This looks more like a fs issue than network, my fs knowledge
is not good enough to justify why the hell the inode could be
destroyed before we release the fd.
My _guess_ is that it is because we defer the ____fput() to a
task work. If this is the case, then fs layer is not guilty for this.
On the other hand, if we have to blame net layer, it does look
suspicious on the RCU usage in sock_release() where we
claim RCU protection but I don't see we hold any RCU lock
there. Also, the code that deferences sock->wq is pretty much
useless now, at least I don't see it catches any bug though.
diff --git a/net/socket.c b/net/socket.c
index 42d8e9c9ccd5..b2390b5591a9 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -598,9 +598,6 @@ void sock_release(struct socket *sock)
module_put(owner);
}
- if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
- pr_err("%s: fasync list not empty!\n", __func__);
-
this_cpu_sub(sockets_in_use, 1);
if (!sock->file) {
iput(SOCK_INODE(sock));
^ permalink raw reply related
* RE: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect
From: Yan Markman @ 2017-11-29 19:33 UTC (permalink / raw)
To: Russell King, Antoine Tenart
Cc: andrew@lunn.ch, f.fainelli@gmail.com, davem@davemloft.net,
gregory.clement@free-electrons.com,
thomas.petazzoni@free-electrons.com,
miquel.raynal@free-electrons.com, Nadav Haklai, mw@semihalf.com,
Stefan Chulski, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171128155611.GA8358@flint.armlinux.org.uk>
Hi Russel
On my board I have [Marvell 88E1510] phy working with STATUS-POLLING
I see some inconsistencies -- first ifconfig-up is different from furthers, no "link is down" reports.
Please refer the behavior example below.
My patch is a "simple solution" -- always reset/clear Link-state-parameters before going UP.
Possibly, more correct (but much more complicated) solution would be in the phy state machine and phylink resolve modification.
I just found that
On ifconfig-down, the phy-state-machine and phylink-resolve
are stopped before executing before passing over full graceful down/reset state.
The further ifconfig-up starts with old state parameters.
Special cases not-tested but logic 2 test-cases are:
remote side changes speed whilst link is Down or Disconnected. But local ifconfig-up starts with old speed.
Best regards
Yan Markman
----------------------------------------------------
EXAMPLE:
buildroot login: root
~# ifconfig eth1 192.169.0.81 up
[ 34.072042] mvpp2 f2000000.ethernet eth1: PHY [f212a200.mdio-mii:01] driver [Marvell 88E1510]
[ 34.080654] mvpp2 f2000000.ethernet eth1: configuring for phy/rgmii-id link mode
[ 37.220506] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
~# ifconfig eth1 down
No print "link is down"
~# ifconfig eth1 up
"Link is Up" passed twice:
[ 60.748041] mvpp2 f2000000.ethernet eth1: PHY [f212a200.mdio-mii:01] driver [Marvell 88E1510]
[ 60.756653] mvpp2 f2000000.ethernet eth1: configuring for phy/rgmii-id link mode
[ 60.764169] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
[ 63.908504] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
On Link physical disconnect/break
No print "link is down"
But link is in correct state -- ifconfig UP but not-RUNNING
On Link physical re-connect
[ 84.388501] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
-------------------------------------------------------------------------------------------
YAN's findings:
Best regards
Yan Markman
-----Original Message-----
From: Russell King [mailto:rmk@armlinux.org.uk]
Sent: Tuesday, November 28, 2017 5:56 PM
To: Antoine Tenart <antoine.tenart@free-electrons.com>
Cc: andrew@lunn.ch; f.fainelli@gmail.com; davem@davemloft.net; Yan Markman <ymarkman@marvell.com>; gregory.clement@free-electrons.com; thomas.petazzoni@free-electrons.com; miquel.raynal@free-electrons.com; Nadav Haklai <nadavh@marvell.com>; mw@semihalf.com; Stefan Chulski <stefanc@marvell.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect
External Email
----------------------------------------------------------------------
Oh, and lastly, please send patches to linux@armlinux.org.uk or the address I use in the sign-offs - sending them to rmk@armlinux.org.uk is for personal non-Linux mail only, and has resulted in _all_ of these messages ending up in my spam folder.
Thanks.
On Tue, Nov 28, 2017 at 03:53:17PM +0000, Russell King wrote:
> On Tue, Nov 28, 2017 at 02:29:32PM +0100, Antoine Tenart wrote:
> > From: Yan Markman <ymarkman@marvell.com>
>
> Hi, thanks for the patch.
>
> > When calling successively _connect, _disconnect and _connect again,
> > if the link configuration changed whilst being down from the phylink
> > perspective, the last _connect would stay in an incorrect old speed.
> > Fixes this by setting the link configuration parameters to an
> > unknown value when calling phylink_bringup_phy.
>
> Under what circumstances does this occur?
>
> >
> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > Signed-off-by: Yan Markman <ymarkman@marvell.com>
> > [Antoine: commit message]
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> > drivers/net/phy/phylink.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index e3bbc70372d3..c2cec3eef67d 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -621,6 +621,16 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
> > if (ret)
> > return ret;
> >
> > + /* On _disconnect, the phy state machine and phylink resolve
> > + * are stopped before executing full gracefull down/reset state.
> > + * The further _connect starts with incorrect init state. Let's set
> > + * init values here.
> > + */
> > + pl->phy_state.link = false;
> > + pl->link_config.pause = MLO_PAUSE_AN;
> > + pl->link_config.speed = SPEED_UNKNOWN;
> > + pl->link_config.duplex = DUPLEX_UNKNOWN;
>
> It would be much better to clean up the phy_state in
> phylink_disconnect_phy() and trigger a resolve, rather than doing that
> each time a PHY is connected - the link should be taken down when the
> PHY is removed.
>
> However, I'd like to know under what circumstances this is happening,
> since, if you're hotplugging a PHY you should be doing that via SFP
> which has additional link up/down handling. What board is this with?
>
> Also note that there's a number of patches in my "phy" branch that I'm
> intending to send as a result of working with Florian over the last
> few weeks. There's several people working fairly independently in
> this area and having everyone send patches independently of each other
> could get painful to manage.
>
> I'm intending to send patches once I know that net-next is open.
>
> --
> Russell King
> ARM architecture Linux Kernel maintainer
--
Russell King
ARM architecture Linux Kernel maintainer
^ permalink raw reply
* Re: [BUG] kernel stack corruption during/after Netlabel error
From: Paul Moore @ 2017-11-29 19:29 UTC (permalink / raw)
To: James Morris, Eric Dumazet; +Cc: Stephen Smalley, netdev, selinux
In-Reply-To: <CANn89iJc=tZkN41WoCm5Zy9nPfs1tfZf9nuSXYS9EB_aem+y4g@mail.gmail.com>
On Wed, Nov 29, 2017 at 12:34 PM, Eric Dumazet <edumazet@google.com> wrote:
> On Wed, Nov 29, 2017 at 9:31 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On Wed, 2017-11-29 at 21:26 +1100, James Morris wrote:
>>> I'm seeing a kernel stack corruption bug (detected via gcc) when
>>> running
>>> the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd inet_socket
>>> test:
>>>
>>> https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests
>>> /inet_socket/test
>>>
>>> # Verify that unauthorized client cannot communicate with the
>>> server.
>>> $result = system
>>> "runcon -t test_inet_bad_client_t -- $basedir/client stream
>>> 127.0.0.1 65535 2>&1";
>>>
>>> This correctlly causes an access control error in the Netlabel code,
>>> and
>>> the bug seems to be triggered during the ICMP send:
>>>
>>> [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to
>>> parse packet
>>> [ 339.822505] Kernel panic - not syncing: stack-protector: Kernel
>>> stack is corrupted in: ffffffff81745af5
>>> [ 339.822505]
>>> [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-rc1-
>>> test #15
>>> [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS
>>> FWKT68A 01/19/2017
>>> [ 339.885060] Call Trace:
>>> [ 339.896875] <IRQ>
>>> [ 339.908103] dump_stack+0x63/0x87
>>> [ 339.920645] panic+0xe8/0x248
>>> [ 339.932668] ? ip_push_pending_frames+0x33/0x40
>>> [ 339.946328] ? icmp_send+0x525/0x530
>>> [ 339.958861] ? kfree_skbmem+0x60/0x70
>>> [ 339.971431] __stack_chk_fail+0x1b/0x20
>>> [ 339.984049] icmp_send+0x525/0x530
...
>>> This is mostly reliable, and I'm only seeing it on bare metal (not in
>>> a
>>> virtualbox vm).
>>>
>>> The SELinux skb parse error at the start only sometimes appears, and
>>> looking at the code, I suspect some kind of memory corruption being
>>> the
>>> cause at that point (basic packet header checks).
>>>
>>> I bisected the bug down to the following change:
>>>
>>> commit bffa72cf7f9df842f0016ba03586039296b4caaf
>>> Author: Eric Dumazet <edumazet@google.com>
>>> Date: Tue Sep 19 05:14:24 2017 -0700
>>>
>>> net: sk_buff rbnode reorg
>>> ...
>>>
>>>
>>> Anyone else able to reproduce this, or have any ideas on what's
>>> happening?
>>
>> So far I haven't been able to reproduce with 4.15-rc1 or -linus.
>
> You might try adding KASAN in the picture ? ( CONFIG_KASAN=y )
As another data point, I have not hit this problem either, but I'm not
currently building my test kernels with KASAN enabled.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
From: David Daney @ 2017-11-29 19:20 UTC (permalink / raw)
To: Souptick Joarder, David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, ralf-6z/3iImG2C8G8FEW9MqTrA,
James Hogan, netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
Rob Herring, Mark Rutland, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Steven J. Hill, devicetree-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn,
Florian Fainelli, Carlos Munoz
In-Reply-To: <CAFqt6zZAPxKm663yEHD0Rx2SPye9Nvoax0RMroDQuF8BpZchsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11/29/2017 08:07 AM, Souptick Joarder wrote:
> On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder <jrdr.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Nov 29, 2017 at 6:25 AM, David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org> wrote:
>>> From: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>
>>> The Cavium OCTEON cn78xx and cn73xx SoCs have network packet I/O
>>> hardware that is significantly different from previous generations of
>>> the family.
>
>>> diff --git a/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>>> new file mode 100644
>>> index 000000000000..4dad35fa4270
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>>> @@ -0,0 +1,2033 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (c) 2017 Cavium, Inc.
>>> + *
>>> + * This file is subject to the terms and conditions of the GNU General Public
>>> + * License. See the file "COPYING" in the main directory of this archive
>>> + * for more details.
>>> + */
>>> +#include <linux/platform_device.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/etherdevice.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_mdio.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/list.h>
>>> +
>
>>> +static void bgx_port_sgmii_set_link_down(struct bgx_port_priv *priv)
>>> +{
>>> + u64 data;
>
>>> + data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));
>>> + data |= BIT(11);
>>> + oct_csr_write(data, BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));
>>> + data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, priv->index));
>>
>> Any particular reason to read immediately after write ?
>
Yes, to ensure the write is committed to hardware before the next step.
>
>
>>> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, struct port_status status)
>>> +{
>>> + u64 data;
>>> + u64 prtx;
>>> + u64 miscx;
>>> + int timeout;
>>> +
>
>>> +
>>> + switch (status.speed) {
>>> + case 10:
>>
>> In my opinion, instead of hard coding the value, is it fine to use ENUM ?
> Similar comments applicable in other places where hard coded values are used.
>
There is nothing to be gained by interposing an extra layer of
abstraction in this case. The code is more clear with the raw numbers
in this particular case.
>
>
>>> +static int bgx_port_gser_27882(struct bgx_port_priv *priv)
>>> +{
>>> + u64 data;
>>> + u64 addr;
>>
>>> + int timeout = 200;
>>> +
>>> + // timeout = 200;
> Better to initialize the timeout value
What are you talking about? It is properly initialized using valid C code.
>
>
>>> +static int bgx_port_qlm_rx_equalization(struct bgx_port_priv *priv, int qlm, int lane)
>>> +{
>>> + lmode = oct_csr_read(GSER_LANE_MODE(priv->node, qlm));
>>> + lmode &= 0xf;
>>> + addr = GSER_LANE_P_MODE_1(priv->node, qlm, lmode);
>>> + data = oct_csr_read(addr);
>>> + /* Don't complete rx equalization if in VMA manual mode */
>>> + if (data & BIT(14))
>>> + return 0;
>>> +
>>> + /* Apply rx equalization for speed > 6250 */
>>> + if (bgx_port_get_qlm_speed(priv, qlm) < 6250)
>>> + return 0;
>>> +
>>> + /* Wait until rx data is valid (CDRLOCK) */
>>> + timeout = 500;
>>
>> 500 us is the min required value or it can be further reduced ?
>
500 uS works well and is shorter than the 2000 uS from the hardware manual.
If you would like to verify shorter timeout values, we could consider
merging such a patch. But really, this doesn't matter as it is a very
short one-off action when the link is brought up.
>
>>> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
>>> +{
>
>>> +
>>> + if (use_ber) {
>>> + timeout = 10000;
>>> + do {
>>> + data =
>>> + oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, priv->bgx, priv->index));
>>> + if (data & BIT(0))
>>> + break;
>>> + timeout--;
>>> + udelay(1);
>>> + } while (timeout);
>>
>> In my opinion, it's better to implement similar kind of loops inside macros.
Ok, duly noted. I think we are in disagreement with respect to this point.
>>
>>> + if (!timeout) {
>>> + pr_debug("BGX%d:%d:%d: BLK_LOCK timeout\n",
>>> + priv->bgx, priv->index, priv->node);
>>> + return -1;
>>> + }
>>> + } else {
>>> + timeout = 10000;
>>> + do {
>>> + data =
>>> + oct_csr_read(BGX_SPU_BX_STATUS(priv->node, priv->bgx, priv->index));
>>> + if (data & BIT(12))
>>> + break;
>>> + timeout--;
>>> + udelay(1);
>>> + } while (timeout);
>> same here
--
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
* [PATCH v3] net: sched: crash on blocks with goto chain action
From: Roman Kapl @ 2017-11-29 19:20 UTC (permalink / raw)
To: netdev; +Cc: xiyou.wangcong, jiri, Roman Kapl
tcf_block_put_ext has assumed that all filters (and thus their goto
actions) are destroyed in RCU callback and so can not race with our
list iteration. However, that is not true during netns cleanup (see
tcf_exts_get_net comment). The assumption was broken by the patch series
c7e460ce5572..623859ae06b8 ("Merge branch 'net-sched-race-fix'").
Prevent the user after free by holding all chains (except 0, that one is
already held) as it was done before
822e86d997e4 ("net_sched: remove tcf_block_put_deferred()").
To reproduce, run the following in a netns and then delete the ns:
ip link add dtest type dummy
tc qdisc add dev dtest ingress
tc filter add dev dtest chain 1 parent ffff: handle 1 prio 1 flower action goto chain 2
Fixes: 623859ae06b8 ("Merge branch 'net-sched-race-fix'")
Signed-off-by: Roman Kapl <code@rkapl.cz>
---
v1 -> v2: Hold all chains instead of just the currently iterated one,
the code should be more clear this way.
v2 -> v3: Point out where the chains will be released.
Blame the correct patch series (the one that broke the assumption).
--
net/sched/cls_api.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 7d97f612c9b9..24a3593ed88a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -336,7 +336,8 @@ static void tcf_block_put_final(struct work_struct *work)
struct tcf_chain *chain, *tmp;
rtnl_lock();
- /* Only chain 0 should be still here. */
+
+ /* At this point, all the chains should have refcnt == 1. */
list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
tcf_chain_put(chain);
rtnl_unlock();
@@ -344,15 +345,22 @@ static void tcf_block_put_final(struct work_struct *work)
}
/* XXX: Standalone actions are not allowed to jump to any chain, and bound
- * actions should be all removed after flushing. However, filters are now
- * destroyed in tc filter workqueue with RTNL lock, they can not race here.
+ * actions should be all removed after flushing.
*/
void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q,
struct tcf_block_ext_info *ei)
{
- struct tcf_chain *chain, *tmp;
+ struct tcf_chain *chain;
- list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
+ /* Hold a refcnt for all chains, except 0 (which is held anyway), so
+ * that they don't disappear while we are iterating. We will release
+ * them in tcf_block_put_final, including finally releasing chain 0.
+ */
+ list_for_each_entry(chain, &block->chain_list, list)
+ if (chain->index)
+ tcf_chain_hold(chain);
+
+ list_for_each_entry(chain, &block->chain_list, list)
tcf_chain_flush(chain);
tcf_block_offload_unbind(block, q, ei);
--
2.15.0
^ permalink raw reply related
* Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.
From: Dan Carpenter @ 2017-11-29 19:11 UTC (permalink / raw)
To: Souptick Joarder
Cc: Mark Rutland, linux-mips, devel, devicetree, netdev, David Daney,
linux-kernel, ralf, Carlos Munoz, Rob Herring, Andrew Lunn,
Steven J. Hill, Greg Kroah-Hartman, Florian Fainelli, James Hogan,
David S. Miller
In-Reply-To: <CAFqt6zZAPxKm663yEHD0Rx2SPye9Nvoax0RMroDQuF8BpZchsA@mail.gmail.com>
On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote:
> >> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, struct port_status status)
> >> +{
> >> + u64 data;
> >> + u64 prtx;
> >> + u64 miscx;
> >> + int timeout;
> >> +
>
> >> +
> >> + switch (status.speed) {
> >> + case 10:
> >
> > In my opinion, instead of hard coding the value, is it fine to use ENUM ?
> Similar comments applicable in other places where hard coded values are used.
>
10 means 10M right? That's not really a magic number. It's fine.
> >> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
> >> +{
>
> >> +
> >> + if (use_ber) {
> >> + timeout = 10000;
> >> + do {
> >> + data =
> >> + oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, priv->bgx, priv->index));
> >> + if (data & BIT(0))
> >> + break;
> >> + timeout--;
> >> + udelay(1);
> >> + } while (timeout);
> >
> > In my opinion, it's better to implement similar kind of loops inside macros.
I don't understand what you mean here. For what it's worth this code
seems clear enough to me (except for the bad indenting of oct_csr_read().
It should be something like:
data = oct_csr_read(BGX_SPU_BR_STATUS1(priv->node,
priv->bgx, priv->index));
That's over the 80 char limit but so is the original code.
regards,
dan carpenter
^ permalink raw reply
* Re: [RFC PATCH] net_sched: bulk free tcf_block
From: Paolo Abeni @ 2017-11-29 18:47 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller
In-Reply-To: <CAKgT0UeE+TE95cCTPSJQ_yN21jikRtKV3rNc=EDOB5XkWUsnKQ@mail.gmail.com>
Hi,
On Wed, 2017-11-29 at 08:14 -0800, Alexander Duyck wrote:
> On Wed, Nov 29, 2017 at 6:25 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > Currently deleting qdisc with a large number of children and filters
> > can take a lot of time:
> >
> > tc qdisc add dev lo root htb
> > for I in `seq 1 1000`; do
> > tc class add dev lo parent 1: classid 1:$I htb rate 100kbit
> > tc qdisc add dev lo parent 1:$I handle $((I + 1)): htb
> > for J in `seq 1 10`; do
> > tc filter add dev lo parent $((I + 1)): u32 match ip src 1.1.1.$J
> > done
> > done
> > time tc qdisc del dev root
> >
> > real 0m54.764s
> > user 0m0.023s
> > sys 0m0.000s
> >
> > This is due to the multiple rcu_barrier() calls, one for each tcf_block
> > freed, invoked with the rtnl lock held. Most other network related
> > tasks will block in this timespan.
> >
> > This change implements bulk free of tcf_block() at destroy() time:
> > when deleting a qdisc hierarchy, the to-be-deleted blocks are queued
> > in a rtnl_lock protected list, and a single rcu_barrier is invoked
> > for each burst.
> >
> > The burst is flushed after the deletion of the topmost qdisc of the
> > destroyed hierarchy and all the queued blocks are deleted with a
> > single delayed work call.
> >
> > After this change, the previous example gives:
> >
> > real 0m0.193s
> > user 0m0.000s
> > sys 0m0.016s
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> While I agree there is an issue I don't think this is being approached
> quite the right way. The question I have is why aren't we using the
> standard RCU approach for this and simply using either call_rcu or
> kfree_rcu to free the object?
>
> > ---
> > This patch adds 2 new list_head fields to tcf_block, that could be
> > replaced with a single pointer, open coding single linked list
> > manipulation in cls_api.c, if a lower memory impact is required.
> > ---
> > include/net/pkt_cls.h | 1 +
> > include/net/sch_generic.h | 5 +++
> > net/sched/cls_api.c | 78 +++++++++++++++++++++++++++++++++++------------
> > net/sched/sch_api.c | 1 +
> > net/sched/sch_generic.c | 17 +++++++++++
> > net/sched/sch_ingress.c | 1 +
> > 6 files changed, 83 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> > index 0105445cab83..12777cfae77c 100644
> > --- a/include/net/pkt_cls.h
> > +++ b/include/net/pkt_cls.h
> > @@ -45,6 +45,7 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
> > void tcf_block_put(struct tcf_block *block);
> > void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q,
> > struct tcf_block_ext_info *ei);
> > +void tcf_flush_blocks(void);
> >
> > static inline struct Qdisc *tcf_block_q(struct tcf_block *block)
> > {
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index 65d0d25f2648..99846ee644a8 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -71,6 +71,7 @@ struct Qdisc {
> > * qdisc_tree_decrease_qlen() should stop.
> > */
> > #define TCQ_F_INVISIBLE 0x80 /* invisible by default in dump */
> > +#define TCQ_F_DELETING 0x100
> > u32 limit;
> > const struct Qdisc_ops *ops;
> > struct qdisc_size_table __rcu *stab;
> > @@ -279,6 +280,10 @@ struct tcf_block {
> > struct Qdisc *q;
> > struct list_head cb_list;
> > struct work_struct work;
> > +
> > + /* TODO: use a single list, do avoid wasting too much memory */
> > + struct list_head del_list;
> > + struct list_head del_head;
> > };
> >
>
> This is just adding yet another layer of deferred freeing. We already
> have RCU why don't we just use that?
>
> > static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index 7d97f612c9b9..446b16c1f532 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -37,6 +37,61 @@ static LIST_HEAD(tcf_proto_base);
> > /* Protects list of registered TC modules. It is pure SMP lock. */
> > static DEFINE_RWLOCK(cls_mod_lock);
> >
> > +/* List of tcf_blocks queued for deletion. Bulk freeing them we avoid the
> > + * rcu_barrier() storm at root_qdisc->destroy() time
> > + */
> > +static LIST_HEAD(queued_blocks);
> > +
> > +static void queue_for_deletion(struct tcf_block *block)
> > +{
> > + if (WARN_ON(!list_empty(&block->del_list)))
> > + return;
> > +
> > + ASSERT_RTNL();
> > + list_add(&block->del_list, &queued_blocks);
> > +}
> > +
> > +static void flush_blocks(struct work_struct *work)
> > +{
> > + struct tcf_block *block, *tmp_block;
> > + struct tcf_chain *chain, *tmp;
> > + struct list_head *head;
> > +
> > + head = &container_of(work, struct tcf_block, work)->del_head;
> > + rtnl_lock();
> > + list_for_each_entry(block, head, del_list)
> > + /* Only chain 0 should be still here. */
> > + list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
> > + tcf_chain_put(chain);
> > + rtnl_unlock();
> > +
> > + list_for_each_entry_safe(block, tmp_block, head, del_list)
> > + kfree(block);
> > +}
> > +
> > +void tcf_flush_blocks(void)
> > +{
> > + struct tcf_block *head;
> > + LIST_HEAD(flush_burst);
> > +
> > + ASSERT_RTNL();
> > + if (list_empty(&queued_blocks))
> > + return;
> > +
> > + head = list_first_entry(&queued_blocks, struct tcf_block, del_list);
> > + INIT_LIST_HEAD(&head->del_head);
> > + list_splice_init(&queued_blocks, &head->del_head);
> > + INIT_WORK(&head->work, flush_blocks);
> > +
> > + /* Wait for existing RCU callbacks to cool down, make sure their works
> > + * have been queued before this. We can not flush pending works here
> > + * because we are holding the RTNL lock.
> > + */
> > + rcu_barrier();
> > + schedule_work(&head->work);
> > +}
> > +EXPORT_SYMBOL_GPL(tcf_flush_blocks);
> > +
> > /* Find classifier type by string name */
> >
> > static const struct tcf_proto_ops *tcf_proto_lookup_ops(const char *kind)
> > @@ -288,6 +343,7 @@ int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
> > return -ENOMEM;
> > INIT_LIST_HEAD(&block->chain_list);
> > INIT_LIST_HEAD(&block->cb_list);
> > + INIT_LIST_HEAD(&block->del_list);
> >
> > /* Create chain 0 by default, it has to be always present. */
> > chain = tcf_chain_create(block, 0);
> > @@ -330,19 +386,6 @@ int tcf_block_get(struct tcf_block **p_block,
> > }
> > EXPORT_SYMBOL(tcf_block_get);
> >
> > -static void tcf_block_put_final(struct work_struct *work)
> > -{
> > - struct tcf_block *block = container_of(work, struct tcf_block, work);
> > - struct tcf_chain *chain, *tmp;
> > -
> > - rtnl_lock();
> > - /* Only chain 0 should be still here. */
> > - list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
> > - tcf_chain_put(chain);
>
> So it seems like the heart of the problem is right here. The
> tcf_chain_put call is updating the reference count and I would assume
> that is the only bit that really needs you to still be holding onto
> the rtnl_lock.
>
> The question I would have is if there is anything accessing the
> reference count or manipulating the list itself without holding the
> rtnl lock? If not you could look at converting this whole thing from
> using a list to an rculist and it seems like it would save you a lot
> of trouble. As far as I can tell the only thing you would then really
> have to worry about would be the freeing of the chain itself which
> would happen in an rcu callback instead of with the rtnl lock held.
Thank you for the feedback!
Big fat disclaimer: I'm not 100% sure I fully understood the locking
schema currently in use (ence the RFC tag), so please Jamal/Cong/Jiri
correct me if/where needed.
AFAIU, there are some more context information: tcf_block_put_final()
and the like must be invoked after that the related filters are freed.
Filter themself must be destroyed synchronously at namespace deletion
time, but must respect RCU grace period elsewhere - see comments in
tcf_exts_get_net().
The rcu_barrier() enforces the proper ordering in the latter case, and
the rtnl_lock protects vs concurrency in the first case.
Cheers,
Paolo
^ permalink raw reply
* Re: [Patch net] act_sample: get rid of tcf_sample_cleanup_rcu()
From: Cong Wang @ 2017-11-29 18:34 UTC (permalink / raw)
To: Linux Kernel Network Developers
Cc: Eric Dumazet, Cong Wang, Jamal Hadi Salim, Jiri Pirko, Yotam Gigi
In-Reply-To: <20171129183234.2541-1-xiyou.wangcong@gmail.com>
On Wed, Nov 29, 2017 at 10:32 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Similar to commit d7fb60b9cafb ("net_sched: get rid of tcfa_rcu"),
> TC actions don't need to respect RCU grace period, because it
> is either just detached from tc filter (standalone case) or
> it is removed together with tc filter (bound case) in which case
> RCU grace period is already respected at filter layer.
The rcu_barrier() now can be removed too...
I will send v2 shortly.
^ 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