| Message ID | 1461664793-22342-1-git-send-email-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
Florian Westphal <fw@strlen.de> wrote: [..] Just to clarify, this targets nf-next tree of course. -- 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 Tue, Apr 26, 2016 at 11:59:53AM +0200, Florian Westphal wrote: > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c > index 25998fa..4ef41a8 100644 > --- a/net/netfilter/nft_ct.c > +++ b/net/netfilter/nft_ct.c > @@ -198,9 +198,22 @@ static void nft_ct_set_eval(const struct nft_expr *expr, > } > break; > #endif > +#ifdef CONFIG_NF_CONNTRACK_LABELS > + case NFT_CT_LABELS: > + if (nf_connlabels_replace(ct, > + ®s->data[priv->sreg], > + ®s->data[priv->sreg], > + NF_CT_LABELS_MAX_SIZE / sizeof(u32))) > + goto err; > + break; > +#endif > default: > break; > } > + > + return; > +err: > + regs->verdict.code = NFT_BREAK; This will trigger a warning when CONFIG_NF_CONNTRACK_LABELS is disabled (the err: label will be unused). I have fixed this here with: +#ifdef CONFIG_NF_CONNTRACK_LABELS + case NFT_CT_LABELS: + if (nf_connlabels_replace(ct, + ®s->data[priv->sreg], + ®s->data[priv->sreg], + NF_CT_LABELS_MAX_SIZE / sizeof(u32))) { + regs->verdict.code = NFT_BREAK; + return; + } + break; +#endif But still I'm unsure we should stop evaluating the rule. How can we reach this error situation? Let me know, I can fix this locally, no need to resend a new patch. 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
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Apr 26, 2016 at 11:59:53AM +0200, Florian Westphal wrote: > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c > > index 25998fa..4ef41a8 100644 > > --- a/net/netfilter/nft_ct.c > > +++ b/net/netfilter/nft_ct.c > > @@ -198,9 +198,22 @@ static void nft_ct_set_eval(const struct nft_expr *expr, > > } > > break; > > #endif > > +#ifdef CONFIG_NF_CONNTRACK_LABELS > > + case NFT_CT_LABELS: > > + if (nf_connlabels_replace(ct, > > + ®s->data[priv->sreg], > > + ®s->data[priv->sreg], > > + NF_CT_LABELS_MAX_SIZE / sizeof(u32))) > > + goto err; > > + break; > > +#endif > > default: > > break; > > } > > + > > + return; > > +err: > > + regs->verdict.code = NFT_BREAK; > > This will trigger a warning when CONFIG_NF_CONNTRACK_LABELS is > disabled (the err: label will be unused). > > I have fixed this here with: Thanks, fix looks good! > But still I'm unsure we should stop evaluating the rule. How can we > reach this error situation? It happens when you hit a conntrack that doesn't have the connlabel extension attached because it predates the nft label set rule. I don't mind changing this to not break and continue with evaluation (i followed what xt_connlabel does but we don't need to follow that example). -- 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 Thu, May 05, 2016 at 03:51:22PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > But still I'm unsure we should stop evaluating the rule. How can we > > reach this error situation? > > It happens when you hit a conntrack that doesn't have the connlabel > extension attached because it predates the nft label set rule. > > I don't mind changing this to not break and continue with evaluation > (i followed what xt_connlabel does but we don't need to follow that > example). OK, then I'm going to simplify this to make it look like: +#ifdef CONFIG_NF_CONNTRACK_LABELS + case NFT_CT_LABELS: + nf_connlabels_replace(ct, + ®s->data[priv->sreg], + ®s->data[priv->sreg], + NF_CT_LABELS_MAX_SIZE / sizeof(u32)); + break; +#endif -- 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
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > OK, then I'm going to simplify this to make it look like: > > +#ifdef CONFIG_NF_CONNTRACK_LABELS > + case NFT_CT_LABELS: > + nf_connlabels_replace(ct, > + ®s->data[priv->sreg], > + ®s->data[priv->sreg], > + NF_CT_LABELS_MAX_SIZE / sizeof(u32)); > + break; > +#endif I'm OK with this. -- 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 Thu, May 05, 2016 at 04:35:43PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > OK, then I'm going to simplify this to make it look like: > > > > +#ifdef CONFIG_NF_CONNTRACK_LABELS > > + case NFT_CT_LABELS: > > + nf_connlabels_replace(ct, > > + ®s->data[priv->sreg], > > + ®s->data[priv->sreg], > > + NF_CT_LABELS_MAX_SIZE / sizeof(u32)); > > + break; > > +#endif > > I'm OK with this. OK, pushed it out. Thanks for reviewing. -- 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/nft_ct.c b/net/netfilter/nft_ct.c index 25998fa..4ef41a8 100644 --- a/net/netfilter/nft_ct.c +++ b/net/netfilter/nft_ct.c @@ -198,9 +198,22 @@ static void nft_ct_set_eval(const struct nft_expr *expr, } break; #endif +#ifdef CONFIG_NF_CONNTRACK_LABELS + case NFT_CT_LABELS: + if (nf_connlabels_replace(ct, + ®s->data[priv->sreg], + ®s->data[priv->sreg], + NF_CT_LABELS_MAX_SIZE / sizeof(u32))) + goto err; + break; +#endif default: break; } + + return; +err: + regs->verdict.code = NFT_BREAK; } static const struct nla_policy nft_ct_policy[NFTA_CT_MAX + 1] = { @@ -365,6 +378,16 @@ static int nft_ct_set_init(const struct nft_ctx *ctx, len = FIELD_SIZEOF(struct nf_conn, mark); break; #endif +#ifdef CONFIG_NF_CONNTRACK_LABELS + case NFT_CT_LABELS: + if (tb[NFTA_CT_DIRECTION]) + return -EINVAL; + len = NF_CT_LABELS_MAX_SIZE; + err = nf_connlabels_get(ctx->net, (len * BITS_PER_BYTE) - 1); + if (err) + return err; + break; +#endif default: return -EOPNOTSUPP; } @@ -384,6 +407,18 @@ static int nft_ct_set_init(const struct nft_ctx *ctx, static void nft_ct_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) { + struct nft_ct *priv = nft_expr_priv(expr); + + switch (priv->key) { +#ifdef CONFIG_NF_CONNTRACK_LABELS + case NFT_CT_LABELS: + nf_connlabels_put(ctx->net); + break; +#endif + default: + break; + } + nft_ct_l3proto_module_put(ctx->afi->family); }
Conntrack labels are currently sized depending on the iptables ruleset, i.e. if we're asked to test or set bits 1, 2, and 65 then we would allocate enough room to store at least bit 65. However, with nft, the input is just a register with arbitrary runtime content. We therefore ask for the upper ceiling we currently have, which is enough room to store 128 bits. Alternatively, we could alter nf_connlabel_replace to increase net->ct.label_words at run time, but since 128 bits is not that big we'd only save sizeof(long) so it doesn't seem worth it for now. This follows a similar approach that xtables 'connlabel' match uses, so when user inputs ct label set bar then we will set the bit used by the 'bar' label and leave the rest alone. This is done by passing the sreg content to nf_connlabels_replace as both value and mask argument. Labels (bits) already set thus cannot be re-set to zero, but this is not supported by xtables connlabel match either. Signed-off-by: Florian Westphal <fw@strlen.de> --- Changes since v6: - revert back to using an sreg, like in V1 Changes vs. V1: - pass sreg also as a mask so existing labels are left alone net/netfilter/nft_ct.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)