idr: fix overflow case for idr_for_each_entry_ul()

idr_for_each_entry_ul() is buggy as it can't handle overflow
case correctly. When we have an ID == UINT_MAX, it becomes an
infinite loop. This happens when running on 32-bit CPU where
unsigned long has the same size with unsigned int.

There is no better way to fix this than casting it to a larger
integer, but we can't just 64 bit integer on 32 bit CPU. Instead
we could just use an additional integer to help us to detect this
overflow case, that is, adding a new parameter to this macro.
Fortunately tc action is its only user right now.

Fixes: 65a206c01e8e ("net/sched: Change act_api and act_xxx modules to use IDR")
Reported-by: Li Shuang <shuali@redhat.com>
Tested-by: Davide Caratti <dcaratti@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Chris Mi <chrism@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/idr.h b/include/linux/idr.h
index ee7abae143d39a41bba5cfadc82f6aade8c1584f..68528a72d10d4dfa41ed7aff407875436795130e 100644 (file)
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -191,14 +191,17 @@ static inline void idr_preload_end(void)
  * 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.
+ * @tmp: A temporary placeholder for ID.
  * @id: Entry ID.
  *
  * @entry and @id do not need to be initialized before the loop, and
  * after normal termination @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)
+#define idr_for_each_entry_ul(idr, entry, tmp, id)                     \
+       for (tmp = 0, id = 0;                                           \
+            tmp <= id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+            tmp = id, ++id)
 
 /**
  * idr_for_each_entry_continue() - Continue iteration over an IDR's elements of a given type

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 4e5d2e9ace5de537e383f3781003041556b4c102..3397122961642f8dacc5f95a2c9e3dd4d6feddf0 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -221,12 +221,13 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
        struct idr *idr = &idrinfo->action_idr;
        struct tc_action *p;
        unsigned long id = 1;
+       unsigned long tmp;
 
        mutex_lock(&idrinfo->lock);
 
        s_i = cb->args[0];
 
-       idr_for_each_entry_ul(idr, p, id) {
+       idr_for_each_entry_ul(idr, p, tmp, id) {
                index++;
                if (index < s_i)
                        continue;
@@ -292,6 +293,7 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
        struct idr *idr = &idrinfo->action_idr;
        struct tc_action *p;
        unsigned long id = 1;
+       unsigned long tmp;
 
        nest = nla_nest_start_noflag(skb, 0);
        if (nest == NULL)
@@ -300,7 +302,7 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
                goto nla_put_failure;
 
        mutex_lock(&idrinfo->lock);
-       idr_for_each_entry_ul(idr, p, id) {
+       idr_for_each_entry_ul(idr, p, tmp, id) {
                ret = tcf_idr_release_unsafe(p);
                if (ret == ACT_P_DELETED) {
                        module_put(ops->owner);
@@ -533,8 +535,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
        struct tc_action *p;
        int ret;
        unsigned long id = 1;
+       unsigned long tmp;
 
-       idr_for_each_entry_ul(idr, p, id) {
+       idr_for_each_entry_ul(idr, p, tmp, id) {
                ret = __tcf_idr_release(p, false, true);
                if (ret == ACT_P_DELETED)
                        module_put(ops->owner);