From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 84243C4321A for ; Thu, 27 Jun 2019 22:10:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5B10E208CB for ; Thu, 27 Jun 2019 22:10:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726561AbfF0WKS (ORCPT ); Thu, 27 Jun 2019 18:10:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36398 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726513AbfF0WKS (ORCPT ); Thu, 27 Jun 2019 18:10:18 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E42E8308FC4A; Thu, 27 Jun 2019 22:10:17 +0000 (UTC) Received: from ovpn-204-56.brq.redhat.com (ovpn-204-56.brq.redhat.com [10.40.204.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id F38185D719; Thu, 27 Jun 2019 22:10:13 +0000 (UTC) Message-ID: <94260c8898834cfa8e5421933a2b5ea59680b970.camel@redhat.com> Subject: Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk() From: Davide Caratti To: Cong Wang Cc: Vlad Buslov , "David S. Miller" , Linux Kernel Network Developers , Lucas Bates In-Reply-To: References: <9068475730862e1d9014c16cee0ad2734a4dd1f9.1560978242.git.dcaratti@redhat.com> <53b8c3118900b31536594e98952640c03a4456e0.camel@redhat.com> <6650f0da68982ffa5bb71a773c5a3d588bd972c4.camel@redhat.com> Organization: red hat Content-Type: text/plain; charset="UTF-8" Date: Fri, 28 Jun 2019 00:10:12 +0200 Mime-Version: 1.0 User-Agent: Evolution 3.30.3 (3.30.3-1.fc29) Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Thu, 27 Jun 2019 22:10:18 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 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