From patchwork Mon Dec 31 00:10:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Phil Sutter X-Patchwork-Id: 1019494 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netfilter-devel-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=nwl.cc Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 43Sd4y5rBmz9s2P for ; Mon, 31 Dec 2018 11:10:58 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725975AbeLaAK4 (ORCPT ); Sun, 30 Dec 2018 19:10:56 -0500 Received: from orbyte.nwl.cc ([151.80.46.58]:55562 "EHLO orbyte.nwl.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725937AbeLaAK4 (ORCPT ); Sun, 30 Dec 2018 19:10:56 -0500 Received: from localhost ([::1]:40420 helo=tatos) by orbyte.nwl.cc with esmtp (Exim 4.91) (envelope-from ) id 1gdlAU-0004tW-8I; Mon, 31 Dec 2018 01:10:54 +0100 From: Phil Sutter To: Pablo Neira Ayuso Cc: Florian Westphal , netfilter-devel@vger.kernel.org Subject: [PATCH v2] net: nf_tables: Fix speedup of selective rule dumps Date: Mon, 31 Dec 2018 01:10:47 +0100 Message-Id: <20181231001047.17849-1-phil@nwl.cc> X-Mailer: git-send-email 2.19.0 MIME-Version: 1.0 Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org Fixed commit had two problems. The significant one being an endless loop when dumping a ruleset containing multiple non-empty chains: __nf_tables_dump_rules() stores the current idx value into cb->args[0] before returning to caller. With multiple chains present, cb->args[0] is therefore updated after each chain's rules have been traversed. This though causes the final nf_tables_dump_rules() run (which should return an skb->len of zero since no rules are left to dump) to continue dumping rules for each but the first chain. Fix this by moving the cb->args[0] update to nf_tables_dump_rules(). With no final action to be performed anymore in __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' variable - instead return the appropriate value directly. The second problem was inability to find given chain under certain circumstances: If no table but only chain name was specified in user's request, nf_tables_dump_rules() returns after not finding the given chain in the first table instead of trying other ones. Fix this by continuing the table traversal, but make sure the loop is still left immediately if a table name was given. Fixes: 241faeceb849c ("netfilter: nf_tables: Speed up selective rule dumps") Signed-off-by: Phil Sutter --- Changes since v1: - Fix second potential issue as well, adjust commit message accordingly. In fact, the second problem fixed here is for a use-case nf_tables shouldn't even support: Dumping rules of a chain without specifying the table may lead to unexpected results if multiple chains with same name in different tables are present. Which chain's rules are dumped depends entirely on the ordering of tables in kernel's table list. The latter is subject to change if e.g. the data structure is changed from list to rhltable. --- net/netfilter/nf_tables_api.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 09ad796d9904e..d6be77618d556 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2304,7 +2304,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, struct net *net = sock_net(skb->sk); unsigned int s_idx = cb->args[0]; const struct nft_rule *rule; - int rc = 1; list_for_each_entry_rcu(rule, &chain->rules, list) { if (!nft_is_active(net, rule)) @@ -2321,16 +2320,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, NLM_F_MULTI | NLM_F_APPEND, table->family, table, chain, rule) < 0) - goto out_unfinished; + return 1; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); cont: (*idx)++; } - rc = 0; -out_unfinished: - cb->args[0] = *idx; - return rc; + return 0; } static int nf_tables_dump_rules(struct sk_buff *skb, @@ -2360,7 +2356,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb, list = rhltable_lookup(&table->chains_ht, ctx->chain, nft_chain_ht_params); if (!list) - goto done; + goto next_table; rhl_for_each_entry_rcu(chain, tmp, list, rhlhead) { if (!nft_is_active(net, chain)) @@ -2376,12 +2372,14 @@ static int nf_tables_dump_rules(struct sk_buff *skb, if (__nf_tables_dump_rules(skb, &idx, cb, table, chain)) goto done; } - +next_table: if (ctx && ctx->table) break; } done: rcu_read_unlock(); + + cb->args[0] = idx; return skb->len; }