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

[Davide Caratti <dcaratti@redhat.com>]

On Wed, 2019-06-26 at 14:15 -0700, Cong Wang wrote:
> Hi, Davide
> 
> On Tue, Jun 25, 2019 at 12:29 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > It should handle this overflow case more gracefully, I hope.
> > 
> 
> Please try this attached one and let me know if it works.
> Hope I get it right this time.
> 
> Thanks!

hello Cong, and thanks a lot for the patch!
I see it uses 

    (tmp <= id)

as the condition to detect the overflow, and at each iteration it does

    tmp = id, ++id

so that 'tmp' contains the last IDR found in the tree and 'id' is the next
tentative value to be searched for. When 'id' overflows, (tmp <= id)
becomes false, and the 'for' loop exits.
I tested it successfully with TC actions having the highest possible
index: 'tc actions show' doesn't loop anymore. But with cls_flower (that
uses idr_for_each_entry_continue_ul() ) I still see the infinite loop:
even when idr_for_each_entry_continue_ul() is used, fl_get_next_filter() 
never returns NULL, because 

    (tmp <= id) && (((entry) = idr_get_next_ul(idr, &(id))) != NULL)

calls idr_get_next_ul(idr, &(id)) at least once. So, even if
idr_for_each_entry_continue_ul() detected the overflow of 'id' after the
first iteration, and bailouts the for loop, fl_get_next_filter()
repeatedly returns a pointer to the idr slot with index equal to
0xffffffff. Because of that, the while() loop in fl_walk() keeps dumping
the same rule.
In my original patch I found easier to check for the overflow of
arg->cookie in fl_walk(), before the self-increment, so I was sure that 

    arg->fn(tp, f, arg)

was already called once when 'f' was the slot having the highest possible
IDR. Now, I didn't check it, but I guess 

    refcount_inc_not_zero(&f->refcnt))

in fl_get_next_filter() is always true during my test, so the inner
while() loop is not endless, even when the idr has a slot with id equal to
ULONG_MAX. Probably, to stay on the safe side, cls_flower needs both tests
to be in place, what do you think?  

-- 
davide