From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH net-next 00/11] net: sched: cls_u32 Various improvements Date: Mon, 8 Oct 2018 06:45:15 +0100 Message-ID: <20181008054515.GC32577@ZenIV.linux.org.uk> References: <20181007163811.18453-1-jhs@emojatatu.com> <20181007.212501.1606549212155525845.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: jhs@mojatatu.com, jiri@resnulli.us, xiyou.wangcong@gmail.com, netdev@vger.kernel.org To: David Miller Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:43634 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725842AbeJHMzS (ORCPT ); Mon, 8 Oct 2018 08:55:18 -0400 Content-Disposition: inline In-Reply-To: <20181007.212501.1606549212155525845.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Oct 07, 2018 at 09:25:01PM -0700, David Miller wrote: > From: Jamal Hadi Salim > Date: Sun, 7 Oct 2018 12:38:00 -0400 > > > Various improvements from Al. > > Please submit changes that actually are compile tested: > > CC [M] net/sched/cls_u32.o > net/sched/cls_u32.c: In function ‘u32_delete’: > net/sched/cls_u32.c:674:6: error: ‘root_ht’ undeclared (first use in this function); did you mean ‘root_user’? > if (root_ht == ht) { > ^~~~~~~ > root_user > net/sched/cls_u32.c:674:6: note: each undeclared identifier is reported only once for each function it appears in > net/sched/cls_u32.c: In function ‘u32_set_parms’: > net/sched/cls_u32.c:746:15: error: ‘struct tc_u_hnode’ has no member named ‘is_root’ > if (ht_down->is_root) { > ^~ Er... Both are due to missing in the very beginning of the series (well, on top of "net: sched: cls_u32: fix hnode refcounting") commit Author: Al Viro Date: Mon Sep 3 14:39:02 2018 -0400 net: sched: cls_u32: mark root hnode explicitly ... and produce consistent error on attempt to delete such. Existing check in u32_delete() is inconsistent - after tc qdisc add dev eth0 ingress tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: u32 divisor 1 tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: u32 divisor 1 both tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 801: u32 and tc filter delete dev eth0 parent ffff: protocol ip prio 100 handle 800: u32 will fail (at least with refcounting fixes), but the former will complain about an attempt to remove a busy table, while the latter will recognize it as root and yield "Not allowed to delete root node" instead. The problem with the existing check is that several tcf_proto instances might share the same tp->data and handle-to-hnode lookup will be the same for all of them. So comparing an hnode to be deleted with tp->root won't catch the case when one tp is used to try deleting the root of another. Solution is trivial - mark the root hnodes explicitly upon allocation and check for that. Signed-off-by: Al Viro diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index b2c3406a2cf2..c4782aa808c7 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -84,6 +84,7 @@ struct tc_u_hnode { int refcnt; unsigned int divisor; struct idr handle_idr; + bool is_root; struct rcu_head rcu; u32 flags; /* The 'ht' field MUST be the last field in structure to allow for @@ -377,6 +378,7 @@ static int u32_init(struct tcf_proto *tp) root_ht->refcnt++; root_ht->handle = tp_c ? gen_new_htid(tp_c, root_ht) : 0x80000000; root_ht->prio = tp->prio; + root_ht->is_root = true; idr_init(&root_ht->handle_idr); if (tp_c == NULL) { @@ -693,7 +695,7 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last, goto out; } - if (root_ht == ht) { + if (ht->is_root) { NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node"); return -EINVAL; }