On Thu, 2019-06-20 at 10:33 -0700, Cong Wang wrote:
> On Thu, Jun 20, 2019 at 5:52 AM Davide Caratti <dcara...@redhat.com> wrote:
> > hello Cong, thanks for reading.
> >
> > On Wed, 2019-06-19 at 15:04 -0700, Cong Wang wrote:
> > > On Wed, Jun 19, 2019 at 2:10 PM Davide Caratti <dcara...@redhat.com>
> > > wrote:
> > > > on some CPUs (e.g. i686), tcf_walker.cookie has the same size as the
> > > > IDR.
> > > > In this situation, the following script:
> > > >
> > > > # tc filter add dev eth0 ingress handle 0xffffffff flower action ok
> > > > # tc filter show dev eth0 ingress
> > > >
> > > > results in an infinite loop.
[...]
> I am not sure it is better to handle this overflow inside idr_get_next_ul()
> or just let its callers to handle it. According to the comments above
> idr_get_next_ul() it sounds like it is not expected to overflow, so...
>
> diff --git a/lib/idr.c b/lib/idr.c
> index c34e256d2f01..a38f5e391cec 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -267,6 +267,9 @@ void *idr_get_next_ul(struct idr *idr, unsigned
> long *nextid)
> if (!slot)
> return NULL;
>
> + /* overflow */
> + if (iter.index < id)
> + return NULL;
> *nextid = iter.index + base;
> return rcu_dereference_raw(*slot);
> }
hello Cong,
I tested the above patch, but I still see the infinite loop on kernel
5.2.0-0.rc5.git0.1.fc31.i686 .
idr_get_next_ul() returns the entry in the radix tree which is greater or
equal to '*nextid' (which has the same value as 'id' in the above hunk).
So, when the radix tree contains one slot with index equal to ULONG_MAX,
whatever can be the value of 'id', the condition in that if() will always
be false (and the function will keep returning non-NULL, hence the
infinite loop).
I also tried this:
if (iter.index == id && id == ULONG_MAX) {
return NULL;
}
it fixes the infinite loop, but it clearly breaks the function semantic
(and anyway, it's not sufficient to fix my test, at least with cls_flower
it still dumps the entry with id 0xffffffff several times). I'm for
fixing the callers of idr_get_next_ul(), and in details:
- apply this patch for cls_flower
- change tcf_dump_walker() in act_api.c as follows, and add a TDC testcase
for 'gact'.
index 4e5d2e9ace5d..f34888c8a952 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -228,8 +228,11 @@ static int tcf_dump_walker(struct tcf_idrinfo
*idrinfo, struct sk_buff *skb,
idr_for_each_entry_ul(idr, p, id) {
index++;
- if (index < s_i)
+ if (index < s_i) {
+ if (id == ULONG_MAX)
+ break;
continue;
+ }
if (jiffy_since &&
time_after(jiffy_since,
WDYT?
thanks a lot,
--
davide
