From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Miller Subject: Re: [PATCH net 1/1] net: sched: cls_u32: fix hnode refcounting Date: Sun, 07 Oct 2018 21:03:08 -0700 (PDT) Message-ID: <20181007.210308.786227727970174514.davem@davemloft.net> References: <20181007114017.26087-1-jhs@emojatatu.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, netdev@vger.kernel.org, viro@zeniv.linux.org.uk To: jhs@mojatatu.com Return-path: Received: from shards.monkeyblade.net ([23.128.96.9]:47356 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725760AbeJHLMs (ORCPT ); Mon, 8 Oct 2018 07:12:48 -0400 In-Reply-To: <20181007114017.26087-1-jhs@emojatatu.com> Sender: netdev-owner@vger.kernel.org List-ID: From: Jamal Hadi Salim Date: Sun, 7 Oct 2018 07:40:17 -0400 > From: Al Viro > > cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references > via ->hlist and via ->tp_root together. u32_destroy() drops the former > and, in case when there had been links, leaves the sucker on the list. > As the result, there's nothing to protect it from getting freed once links > are dropped. > That also makes the "is it busy" check incapable of catching the root > hnode - it *is* busy (there's a reference from tp), but we don't see it as > something separate. "Is it our root?" check partially covers that, but > the problem exists for others' roots as well. > > AFAICS, the minimal fix preserving the existing behaviour (where it doesn't > include oopsen, that is) would be this: > * count tp->root and tp_c->hlist as separate references. I.e. > have u32_init() set refcount to 2, not 1. > * in u32_destroy() we always drop the former; > in u32_destroy_hnode() - the latter. > > That way we have *all* references contributing to refcount. List > removal happens in u32_destroy_hnode() (called only when ->refcnt is 1) > an in u32_destroy() in case of tc_u_common going away, along with > everything reachable from it. IOW, that way we know that > u32_destroy_key() won't free something still on the list (or pointed to by > someone's ->root). > > Reproducer: ... > Signed-off-by: Al Viro > Signed-off-by: Jamal Hadi Salim Applied and queued up for -stable.