| Message ID | 20180318182239.28455-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: nf_tables: permit second nat hook if colliding hook is going away | expand |
Hi Florian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Florian-Westphal/netfilter-nf_tables-permit-second-nat-hook-if-colliding-hook-is-going-away/20180319-161221
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> net/netfilter/nf_tables_api.c:92:6: sparse: symbol 'nf_tables_allow_nat_conflict' was not declared. Should it be static?
net/netfilter/nf_tables_api.c:1242:31: sparse: incorrect type in return expression (different address spaces) @@ expected struct nft_stats [noderef] <asn:3>* @@ got sn:3>* @@
net/netfilter/nf_tables_api.c:1242:31: expected struct nft_stats [noderef] <asn:3>*
net/netfilter/nf_tables_api.c:1242:31: got void *
net/netfilter/nf_tables_api.c:1245:31: sparse: incorrect type in return expression (different address spaces) @@ expected struct nft_stats [noderef] <asn:3>* @@ got sn:3>* @@
net/netfilter/nf_tables_api.c:1245:31: expected struct nft_stats [noderef] <asn:3>*
net/netfilter/nf_tables_api.c:1245:31: got void *
net/netfilter/nf_tables_api.c:1249:31: sparse: incorrect type in return expression (different address spaces) @@ expected struct nft_stats [noderef] <asn:3>* @@ got sn:3>* @@
net/netfilter/nf_tables_api.c:1249:31: expected struct nft_stats [noderef] <asn:3>*
net/netfilter/nf_tables_api.c:1249:31: got void *
net/netfilter/nf_tables_api.c:1272:28: sparse: cast between address spaces (<asn:3>-><asn:4>)
net/netfilter/nf_tables_api.c:1272:28: sparse: incompatible types in comparison expression (different address spaces)
net/netfilter/nf_tables_api.c:1526:23: sparse: incorrect type in assignment (different address spaces) @@ expected struct nft_stats *stats @@ got struct nft_stats struct nft_stats *stats @@
net/netfilter/nf_tables_api.c:1534:29: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] <asn:3>*__pdata @@ got [noderef] <asn:3>*__pdata @@
net/netfilter/nf_tables_api.c:1538:38: sparse: incorrect type in assignment (different address spaces) @@ expected struct nft_stats [noderef] <asn:3>*stats @@ got [noderef] <asn:3>*stats @@
net/netfilter/nf_tables_api.c:1552:37: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] <asn:3>*__pdata @@ got [noderef] <asn:3>*__pdata @@
net/netfilter/nf_tables_api.c:4393:19: sparse: symbol 'nf_tables_obj_lookup_byhandle' was not declared. Should it be static?
net/netfilter/nf_tables_api.c:4422:41: sparse: Variable length array is used.
net/netfilter/nf_tables_api.c:4915:22: sparse: symbol 'nf_tables_flowtable_lookup_byhandle' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 18, 2018 at 07:22:39PM +0100, Florian Westphal wrote: > Sergei Trofimovich reported that restoring an nft ruleset doesn't work > anymore unless old rule content is flushed first. > > The problem stems from a recent change designed to prevent multiple nat > hooks at the same hook point locations and nftables transaction model. > > A 'flush ruleset' won't take effect until the entire transaction has > completed. > > So, if one has a nft.rules file that contains a 'flush ruleset', > followed by a nat hook register request, then 'nft -f file' will work, > but running 'nft -f file' again will fail with -EBUSY. > > Reason is that nftables will place the flush/removal requests in the > transaction list, but it will not act on the removal until after all new > rules are in place. > > The netfilter core will therefore get request to register a new nat > hook before the old one is removed -- this now fails as the netfilter > core can't know the existing hook is staged for removal. > > To fix this, we can search the transaction log when a hook collision > is detected. The collision is okay if > > 1. there is a delete request pending for the nat hook that is already > registered. > 2. there is no second add request for a matching nat hook. > This is required to only apply the exception once. Also applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 36f69acaf51f..c2ad333dce1e 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -74,15 +74,77 @@ static void nft_trans_destroy(struct nft_trans *trans) kfree(trans); } +/* removal requests are queued in the commit_list, but not acted upon + * until after all new rules are in place. + * + * Therefore, nf_register_net_hook(net, &nat_hook) runs before pending + * nf_unregister_net_hook(). + * + * nf_register_net_hook thus fails if a nat hook is already in place + * even if the conflicting hook is about to be removed. + * + * If collision is detected, search commit_log for DELCHAIN matching + * the new nat hooknum; if we find one collision is temporary: + * + * Either transaction is aborted (new/colliding hook is removed), or + * transaction is committed (old hook is removed). + */ +bool nf_tables_allow_nat_conflict(const struct net *net, + const struct nf_hook_ops *ops) +{ + const struct nft_trans *trans; + bool ret = false; + + if (!ops->nat_hook) + return false; + + list_for_each_entry(trans, &net->nft.commit_list, list) { + const struct nf_hook_ops *pending_ops; + const struct nft_chain *pending; + + if (trans->msg_type != NFT_MSG_NEWCHAIN && + trans->msg_type != NFT_MSG_DELCHAIN) + continue; + + pending = trans->ctx.chain; + if (!nft_is_base_chain(pending)) + continue; + + pending_ops = &nft_base_chain(pending)->ops; + if (pending_ops->nat_hook && + pending_ops->pf == ops->pf && + pending_ops->hooknum == ops->hooknum) { + /* other hook registration already pending? */ + if (trans->msg_type == NFT_MSG_NEWCHAIN) + return false; + + ret = true; + } + } + + return ret; +} + static int nf_tables_register_hook(struct net *net, const struct nft_table *table, struct nft_chain *chain) { + struct nf_hook_ops *ops; + int ret; + if (table->flags & NFT_TABLE_F_DORMANT || !nft_is_base_chain(chain)) return 0; - return nf_register_net_hook(net, &nft_base_chain(chain)->ops); + ops = &nft_base_chain(chain)->ops; + ret = nf_register_net_hook(net, ops); + if (ret == -EBUSY && nf_tables_allow_nat_conflict(net, ops)) { + ops->nat_hook = false; + ret = nf_register_net_hook(net, ops); + ops->nat_hook = true; + } + + return ret; } static void nf_tables_unregister_hook(struct net *net,
Sergei Trofimovich reported that restoring an nft ruleset doesn't work anymore unless old rule content is flushed first. The problem stems from a recent change designed to prevent multiple nat hooks at the same hook point locations and nftables transaction model. A 'flush ruleset' won't take effect until the entire transaction has completed. So, if one has a nft.rules file that contains a 'flush ruleset', followed by a nat hook register request, then 'nft -f file' will work, but running 'nft -f file' again will fail with -EBUSY. Reason is that nftables will place the flush/removal requests in the transaction list, but it will not act on the removal until after all new rules are in place. The netfilter core will therefore get request to register a new nat hook before the old one is removed -- this now fails as the netfilter core can't know the existing hook is staged for removal. To fix this, we can search the transaction log when a hook collision is detected. The collision is okay if 1. there is a delete request pending for the nat hook that is already registered. 2. there is no second add request for a matching nat hook. This is required to only apply the exception once. Fixes: f92b40a8b2645 ("netfilter: core: only allow one nat hook per hook point") Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/nf_tables_api.c | 64 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)