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.wangc...@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
