On Thu, Jun 27, 2019 at 3:10 PM Davide Caratti <dcara...@redhat.com> wrote: > > 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.
Yes, thanks for testing it with tc actions. > 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. Good catch, it is actually the arg->cookie++ which causes the trouble here. > 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? I think we can just fold the nested loops into one for cls_flower and remove the arg->cookie++. What's more, arg->cookie could overflow too,seems we have to switch back to arg->skip. I am not sure, if this is really a problem we can fix it separately. Thanks.
