Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()

[Cong Wang <xiyou.wangcong@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Tue, Jun 25, 2019 at 11:07 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On one hand, its callers should not need to worry about details
> like overflow. On the other hand, in fact it does exactly what its
> callers tell it to do, the problematic part is actually the
> incremented id. On 64bit, it is fairly easy, we can just simply
> know 'long' is longer than 32bit and leverage this to detect overflow,
> but on 32bit this clearly doesn't work.
>
> Let me think about it.

Davide, do you mind to try the attached patch?

It should handle this overflow case more gracefully, I hope.

Thanks.

[-- Attachment #2: idr_get_next_ul.diff --]
[-- Type: application/octet-stream, Size: 2903 bytes --]

commit 685934f9eed9b50a46d33a9ec9671800397d20cc
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date:   Tue Jun 25 12:23:18 2019 -0700

    idr: fix idr_get_next_ul() usage
    
    Reported-by: Li Shuang <shuali@redhat.com>
    Cc: Davide Caratti <dcaratti@redhat.com>
    Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
index c6c28f56aa29..c73f80bddde4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c
@@ -105,10 +105,11 @@ static struct list_head *mlx5_fc_counters_lookup_next(struct mlx5_core_dev *dev,
 
 	rcu_read_lock();
 	/* skip counters that are in idr, but not yet in counters list */
-	while ((counter = idr_get_next_ul(&fc_stats->counters_idr,
-					  &next_id)) != NULL &&
-	       list_empty(&counter->list))
-		next_id++;
+	idr_for_each_entry_continue_ul(&fc_stats->counters_idr,
+				       counter, next_id) {
+		if (list_empty(&counter->list))
+			continue;
+	}
 	rcu_read_unlock();
 
 	return counter ? &counter->list : &fc_stats->counters;
diff --git a/include/linux/idr.h b/include/linux/idr.h
index ee7abae143d3..af7a67e65c1c 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -198,7 +198,21 @@ static inline void idr_preload_end(void)
  * 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)
+	for (id = 0;						\
+	     (u32)id + 1 > id && ((entry) = idr_get_next_ul(idr, &(id))) != NULL; \
+	     ++id)
+
+/**
+ * idr_for_each_entry_continue_ul() - Continue iteration over an IDR's elements of a given type
+ * @idr: IDR handle.
+ * @entry: The type * to use as a cursor.
+ * @id: Entry ID.
+ *
+ * Continue to iterate over entries, continuing after the current position.
+ */
+#define idr_for_each_entry_continue_ul(idr, entry, id)			\
+	for (; (u32)id + 1 > id && ((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/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eedd5786c084..06338dadd5e4 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -528,17 +528,18 @@ static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
 						unsigned long *handle)
 {
 	struct cls_fl_head *head = fl_head_dereference(tp);
+	unsigned long id = *handle;
 	struct cls_fl_filter *f;
 
 	rcu_read_lock();
-	while ((f = idr_get_next_ul(&head->handle_idr, handle))) {
+	idr_for_each_entry_continue_ul(&head->handle_idr, f, id) {
 		/* don't return filters that are being deleted */
 		if (refcount_inc_not_zero(&f->refcnt))
 			break;
-		++(*handle);
 	}
 	rcu_read_unlock();
 
+	*handle = id;
 	return f;
 }