| Message ID | 20210929230500.811946-3-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net,1/5] netfilter: conntrack: fix boot failure with nf_conntrack.enable_hooks=1 | expand |
On Thu, 30 Sep 2021 01:04:57 +0200 Pablo Neira Ayuso wrote: > Add position handle to allow to identify the rule location from netlink > events. Otherwise, userspace cannot incrementally update a userspace > cache through monitoring events. > > Skip handle dump if the rule has been either inserted (at the beginning > of the ruleset) or appended (at the end of the ruleset), the > NLM_F_APPEND netlink flag is sufficient in these two cases. > > Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement > expansion appends it after the specified rule handle. > > Fixes: 96518518cc41 ("netfilter: add nftables") > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Let me defer to Dave on this one. Krzysztof K recently provided us with this quote: "One thing that does bother [Linus] is developers who send him fixes in the -rc2 or -rc3 time frame for things that never worked in the first place. If something never worked, then the fact that it doesn't work now is not a regression, so the fixes should just wait for the next merge window. Those fixes are, after all, essentially development work." https://lwn.net/Articles/705245/ Maybe the thinking has evolved since, but this patch strikes me as odd. We forgot to put an attribute in netlink 8 years ago, and suddenly it's urgent to fill it in? Something does not connect for me, certainly the commit message should have explained things better...
Hi Jakub, On Wed, Sep 29, 2021 at 07:19:53PM -0700, Jakub Kicinski wrote: > On Thu, 30 Sep 2021 01:04:57 +0200 Pablo Neira Ayuso wrote: > > Add position handle to allow to identify the rule location from netlink > > events. Otherwise, userspace cannot incrementally update a userspace > > cache through monitoring events. > > > > Skip handle dump if the rule has been either inserted (at the beginning > > of the ruleset) or appended (at the end of the ruleset), the > > NLM_F_APPEND netlink flag is sufficient in these two cases. > > > > Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement > > expansion appends it after the specified rule handle. > > > > Fixes: 96518518cc41 ("netfilter: add nftables") > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > Let me defer to Dave on this one. Krzysztof K recently provided us with > this quote: > > "One thing that does bother [Linus] is developers who send him fixes in the > -rc2 or -rc3 time frame for things that never worked in the first place. > If something never worked, then the fact that it doesn't work now is not > a regression, so the fixes should just wait for the next merge window. > Those fixes are, after all, essentially development work." > > https://lwn.net/Articles/705245/ > > Maybe the thinking has evolved since, but this patch strikes me as odd. > We forgot to put an attribute in netlink 8 years ago, and suddenly it's > urgent to fill it in? Something does not connect for me, certainly the > commit message should have explained things better... Reasonable, but in this particular case I cannot fix userspace monitor mode without this patch. A user reported that 'nft insert rule...' shows as 'nft add rule...' in 'nft monitor'. Then if 'nft add rule x y position 10...' is used to add a rule at a given position, then it does not show the 'position 10' so the user is just getting a 'add rule x y' which means append it at the end. Same thing happens with 'create table x', it shows as 'add table x'. Noone noticed the missing flags in the event notification path so far. I can place this into net-next, yes, but this is only going to delay things before I can ask for including this in -stable, meanwhile users will keep getting misleading event notification for these cases. Thanks.
From: Jakub Kicinski <kuba@kernel.org> Date: Wed, 29 Sep 2021 19:19:53 -0700 > On Thu, 30 Sep 2021 01:04:57 +0200 Pablo Neira Ayuso wrote: >> Add position handle to allow to identify the rule location from netlink >> events. Otherwise, userspace cannot incrementally update a userspace >> cache through monitoring events. >> >> Skip handle dump if the rule has been either inserted (at the beginning >> of the ruleset) or appended (at the end of the ruleset), the >> NLM_F_APPEND netlink flag is sufficient in these two cases. >> >> Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement >> expansion appends it after the specified rule handle. >> >> Fixes: 96518518cc41 ("netfilter: add nftables") >> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > Let me defer to Dave on this one. Krzysztof K recently provided us with > this quote: > > "One thing that does bother [Linus] is developers who send him fixes in the > -rc2 or -rc3 time frame for things that never worked in the first place. > If something never worked, then the fact that it doesn't work now is not > a regression, so the fixes should just wait for the next merge window. > Those fixes are, after all, essentially development work." > > https://lwn.net/Articles/705245/ > > Maybe the thinking has evolved since, but this patch strikes me as odd. > We forgot to put an attribute in netlink 8 years ago, and suddenly it's > urgent to fill it in? Something does not connect for me, certainly the > commit message should have explained things better... Agreed.
On Thu, Sep 30, 2021 at 01:35:22PM +0100, David Miller wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Wed, 29 Sep 2021 19:19:53 -0700 > > > On Thu, 30 Sep 2021 01:04:57 +0200 Pablo Neira Ayuso wrote: > >> Add position handle to allow to identify the rule location from netlink > >> events. Otherwise, userspace cannot incrementally update a userspace > >> cache through monitoring events. > >> > >> Skip handle dump if the rule has been either inserted (at the beginning > >> of the ruleset) or appended (at the end of the ruleset), the > >> NLM_F_APPEND netlink flag is sufficient in these two cases. > >> > >> Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement > >> expansion appends it after the specified rule handle. > >> > >> Fixes: 96518518cc41 ("netfilter: add nftables") > >> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > Let me defer to Dave on this one. Krzysztof K recently provided us with > > this quote: > > > > "One thing that does bother [Linus] is developers who send him fixes in the > > -rc2 or -rc3 time frame for things that never worked in the first place. > > If something never worked, then the fact that it doesn't work now is not > > a regression, so the fixes should just wait for the next merge window. > > Those fixes are, after all, essentially development work." > > > > https://lwn.net/Articles/705245/ > > > > Maybe the thinking has evolved since, but this patch strikes me as odd. > > We forgot to put an attribute in netlink 8 years ago, and suddenly it's > > urgent to fill it in? Something does not connect for me, certainly the > > commit message should have explained things better... > > Agreed. The aforementioned article says: "In general, he said, if a fix applies to a feature that is not currently being used, it should wait for the next development cycle" This feature is being used by 'nft monitor', which is not representing: - insert rule - add/insert rule with position handle - create table/chain/set/map commands in the correct way via netlink notifications. I can rework the commit message to clarify this and resubmit.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index b9546defdc28..085783b14075 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2866,8 +2866,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net, u32 flags, int family, const struct nft_table *table, const struct nft_chain *chain, - const struct nft_rule *rule, - const struct nft_rule *prule) + const struct nft_rule *rule, u64 handle) { struct nlmsghdr *nlh; const struct nft_expr *expr, *next; @@ -2887,9 +2886,8 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net, NFTA_RULE_PAD)) goto nla_put_failure; - if (event != NFT_MSG_DELRULE && prule) { - if (nla_put_be64(skb, NFTA_RULE_POSITION, - cpu_to_be64(prule->handle), + if (event != NFT_MSG_DELRULE && handle) { + if (nla_put_be64(skb, NFTA_RULE_POSITION, cpu_to_be64(handle), NFTA_RULE_PAD)) goto nla_put_failure; } @@ -2925,7 +2923,10 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx, const struct nft_rule *rule, int event) { struct nftables_pernet *nft_net = nft_pernet(ctx->net); + const struct nft_rule *prule; struct sk_buff *skb; + u64 handle = 0; + u16 flags = 0; int err; if (!ctx->report && @@ -2936,9 +2937,18 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx, if (skb == NULL) goto err; + if (event == NFT_MSG_NEWRULE && + !list_is_first(&rule->list, &ctx->chain->rules) && + !list_is_last(&rule->list, &ctx->chain->rules)) { + prule = list_prev_entry(rule, list); + handle = prule->handle; + } + if (ctx->flags & (NLM_F_APPEND | NLM_F_REPLACE)) + flags |= NLM_F_APPEND; + err = nf_tables_fill_rule_info(skb, ctx->net, ctx->portid, ctx->seq, - event, 0, ctx->family, ctx->table, - ctx->chain, rule, NULL); + event, flags, ctx->family, ctx->table, + ctx->chain, rule, handle); if (err < 0) { kfree_skb(skb); goto err; @@ -2964,6 +2974,7 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, struct net *net = sock_net(skb->sk); const struct nft_rule *rule, *prule; unsigned int s_idx = cb->args[0]; + u64 handle; prule = NULL; list_for_each_entry_rcu(rule, &chain->rules, list) { @@ -2975,12 +2986,17 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, memset(&cb->args[1], 0, sizeof(cb->args) - sizeof(cb->args[0])); } + if (prule) + handle = prule->handle; + else + handle = 0; + if (nf_tables_fill_rule_info(skb, net, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, NFT_MSG_NEWRULE, NLM_F_MULTI | NLM_F_APPEND, table->family, - table, chain, rule, prule) < 0) + table, chain, rule, handle) < 0) return 1; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); @@ -3143,7 +3159,7 @@ static int nf_tables_getrule(struct sk_buff *skb, const struct nfnl_info *info, err = nf_tables_fill_rule_info(skb2, net, NETLINK_CB(skb).portid, info->nlh->nlmsg_seq, NFT_MSG_NEWRULE, 0, - family, table, chain, rule, NULL); + family, table, chain, rule, 0); if (err < 0) goto err_fill_rule_info;
Add position handle to allow to identify the rule location from netlink events. Otherwise, userspace cannot incrementally update a userspace cache through monitoring events. Skip handle dump if the rule has been either inserted (at the beginning of the ruleset) or appended (at the end of the ruleset), the NLM_F_APPEND netlink flag is sufficient in these two cases. Handle NLM_F_REPLACE as NLM_F_APPEND since the rule replacement expansion appends it after the specified rule handle. Fixes: 96518518cc41 ("netfilter: add nftables") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nf_tables_api.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)