[RFC PATCH 0/3] connlabel set support using extra setter attr

[RFC PATCH 0/3] connlabel set support using extra setter attr

[Florian Westphal]

Hi Pablo

This attempts to implement the set support via your proposed
setter attribute.

IOW instead of taking an sreg and replacing the entire label
area with whatever is contained therein take a bit number
passed in from userspace and set only that one bit.

So ct label set foo

will really mean 'set the foo bit in the label area', just like
iptables -m connlabel --label foo --set

I'm sure that there are better ways to implement the expr <-> attr
conversion, see patch #3.

Comments welcome, I'll respin it once -next is open again.

Thanks,
Florian

[PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Instead of taking the value to set from a source register, userspace
passes the bit that we should set as a netlink attribute.

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.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Pablo, I know its too late for -next, I'm resending this now while
 this stuff is fresh on my plate -- its unlikely that this will cause
 merge issues.

 So if everything is fine I can resubmit once -next opens up again.

 This takes the different approach you suggested -- pass in the bit we want
 to set via an extra attribute (no sreg).

 include/uapi/linux/netfilter/nf_tables.h |  2 ++
 net/netfilter/nft_ct.c                   | 53 ++++++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index eeffde1..12bc116 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -770,6 +770,7 @@ enum nft_ct_keys {
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
  * @NFTA_CT_SREG: source register (NLA_U32)
+ * @NFTA_CT_LABEL: label bit number to set (NLA_U32)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
@@ -777,6 +778,7 @@ enum nft_ct_attributes {
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
 	NFTA_CT_SREG,
+	NFTA_CT_LABEL,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index d4a4619..76da69d 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -29,6 +29,7 @@ struct nft_ct {
 		enum nft_registers	dreg:8;
 		enum nft_registers	sreg:8;
 	};
+	u8			set_label_bit;
 };
 
 static u64 nft_ct_get_eval_counter(const struct nf_conn_counter *c,
@@ -198,6 +199,11 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 		}
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		nf_connlabel_set(ct, priv->set_label_bit);
+		break;
+#endif
 	default:
 		break;
 	}
@@ -276,6 +282,9 @@ static int nft_ct_get_init(const struct nft_ctx *ctx,
 		if (tb[NFTA_CT_DIRECTION] != NULL)
 			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
 	case NFT_CT_HELPER:
@@ -355,16 +364,39 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 			   const struct nlattr * const tb[])
 {
 	struct nft_ct *priv = nft_expr_priv(expr);
-	unsigned int len;
+	unsigned int len = 0;
 	int err;
 
 	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
 	switch (priv->key) {
 #ifdef CONFIG_NF_CONNTRACK_MARK
 	case NFT_CT_MARK:
+		if (tb[NFTA_CT_DIRECTION])
+			return -EINVAL;
 		len = FIELD_SIZEOF(struct nf_conn, mark);
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS: {
+		u32 bit;
+
+		if (tb[NFTA_CT_DIRECTION] || tb[NFTA_CT_SREG] ||
+		    tb[NFTA_CT_LABEL] == NULL)
+			return -EINVAL;
+
+		bit = ntohl(nla_get_be32(tb[NFTA_CT_LABEL]));
+		priv->set_label_bit = (u8)bit;
+
+		if (bit > priv->set_label_bit)
+			return -ERANGE;
+
+		err = nf_connlabels_get(ctx->net, bit + 1);
+		if (err < 0)
+			return err;
+
+		return nft_ct_l3proto_try_module_get(ctx->afi->family);
+	}
+#endif
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -384,6 +416,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);
 }
 
@@ -430,6 +474,11 @@ static int nft_ct_set_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		goto nla_put_failure;
 	if (nla_put_be32(skb, NFTA_CT_KEY, htonl(priv->key)))
 		goto nla_put_failure;
+
+	if (priv->key == NFT_CT_LABELS &&
+	    nla_put_be32(skb, NFTA_CT_LABEL, htonl(priv->set_label_bit)))
+		goto nla_put_failure;
+
 	return 0;
 
 nla_put_failure:
@@ -468,7 +517,7 @@ nft_ct_select_ops(const struct nft_ctx *ctx,
 	if (tb[NFTA_CT_DREG])
 		return &nft_ct_get_ops;
 
-	if (tb[NFTA_CT_SREG])
+	if (tb[NFTA_CT_SREG] || tb[NFTA_CT_LABEL])
 		return &nft_ct_set_ops;
 
 	return ERR_PTR(-EINVAL);
-- 
2.4.10

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Tue, Mar 15, 2016 at 05:10:09PM +0100, Florian Westphal wrote:
> Instead of taking the value to set from a source register, userspace
> passes the bit that we should set as a netlink attribute.
> 
> This follows a similar approach that xtables 'connlabel'
> match uses, so when user inputs
> 
>     ct label set bar

I think we can introduce:

        ct label bitset bar

so this is clear to the user this is just setting the bit at that
position.

This is just more infrastructure for this single case, and we've been
trying to avoid adding more complexity for only one single use-case
at this stage.

> then we will set the bit used by the 'bar' label and leave the rest alone.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Pablo, I know its too late for -next, I'm resending this now while
>  this stuff is fresh on my plate -- its unlikely that this will cause
>  merge issues.
> 
>  So if everything is fine I can resubmit once -next opens up again.
> 
>  This takes the different approach you suggested -- pass in the bit we want
>  to set via an extra attribute (no sreg).
> 
>  include/uapi/linux/netfilter/nf_tables.h |  2 ++
>  net/netfilter/nft_ct.c                   | 53 ++++++++++++++++++++++++++++++--
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index eeffde1..12bc116 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -770,6 +770,7 @@ enum nft_ct_keys {
>   * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
>   * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
>   * @NFTA_CT_SREG: source register (NLA_U32)
> + * @NFTA_CT_LABEL: label bit number to set (NLA_U32)
>   */
>  enum nft_ct_attributes {
>  	NFTA_CT_UNSPEC,
> @@ -777,6 +778,7 @@ enum nft_ct_attributes {
>  	NFTA_CT_KEY,
>  	NFTA_CT_DIRECTION,
>  	NFTA_CT_SREG,
> +	NFTA_CT_LABEL,

We can probably add:

        NFTA_CT_IMM

This would be a nested attribute, then inside there we can define the:

        NFTA_CT_LABEL

We'll have NFTA_CT_HELPER soon, we can place this attribute in the
imm nest, or any other one following up.

>  	__NFTA_CT_MAX
>  };
>  #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index d4a4619..76da69d 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -29,6 +29,7 @@ struct nft_ct {
>  		enum nft_registers	dreg:8;
>  		enum nft_registers	sreg:8;
>  	};
> +	u8			set_label_bit;

I understand a specific nft_ct_label is too much at this stage, so:

        union {
                u8                      set_label_bit;
        } imm;

or use something like imm_label_bit for readability.

>  };
>  
>  static u64 nft_ct_get_eval_counter(const struct nf_conn_counter *c,
> @@ -198,6 +199,11 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
>  		}
>  		break;
>  #endif
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	case NFT_CT_LABELS:
> +		nf_connlabel_set(ct, priv->set_label_bit);
> +		break;
> +#endif
>  	default:
>  		break;
>  	}
> @@ -276,6 +282,9 @@ static int nft_ct_get_init(const struct nft_ctx *ctx,
>  		if (tb[NFTA_CT_DIRECTION] != NULL)
>  			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
>  	case NFT_CT_HELPER:
> @@ -355,16 +364,39 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
>  			   const struct nlattr * const tb[])
>  {
>  	struct nft_ct *priv = nft_expr_priv(expr);
> -	unsigned int len;
> +	unsigned int len = 0;
>  	int err;
>  
>  	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
>  	switch (priv->key) {
>  #ifdef CONFIG_NF_CONNTRACK_MARK
>  	case NFT_CT_MARK:
> +		if (tb[NFTA_CT_DIRECTION])
> +			return -EINVAL;
>  		len = FIELD_SIZEOF(struct nf_conn, mark);
>  		break;
>  #endif
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	case NFT_CT_LABELS: {
> +		u32 bit;
> +
> +		if (tb[NFTA_CT_DIRECTION] || tb[NFTA_CT_SREG] ||
> +		    tb[NFTA_CT_LABEL] == NULL)
> +			return -EINVAL;
> +
> +		bit = ntohl(nla_get_be32(tb[NFTA_CT_LABEL]));
> +		priv->set_label_bit = (u8)bit;
> +
> +		if (bit > priv->set_label_bit)
> +			return -ERANGE;
> +
> +		err = nf_connlabels_get(ctx->net, bit + 1);
> +		if (err < 0)
> +			return err;
> +
> +		return nft_ct_l3proto_try_module_get(ctx->afi->family);
> +	}
> +#endif
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -384,6 +416,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);
>  }
>  
> @@ -430,6 +474,11 @@ static int nft_ct_set_dump(struct sk_buff *skb, const struct nft_expr *expr)
>  		goto nla_put_failure;
>  	if (nla_put_be32(skb, NFTA_CT_KEY, htonl(priv->key)))
>  		goto nla_put_failure;
> +
> +	if (priv->key == NFT_CT_LABELS &&
> +	    nla_put_be32(skb, NFTA_CT_LABEL, htonl(priv->set_label_bit)))
> +		goto nla_put_failure;
> +
>  	return 0;
>  
>  nla_put_failure:
> @@ -468,7 +517,7 @@ nft_ct_select_ops(const struct nft_ctx *ctx,
>  	if (tb[NFTA_CT_DREG])
>  		return &nft_ct_get_ops;
>  
> -	if (tb[NFTA_CT_SREG])
> +	if (tb[NFTA_CT_SREG] || tb[NFTA_CT_LABEL])
>  		return &nft_ct_set_ops;
>  
>  	return ERR_PTR(-EINVAL);
> -- 
> 2.4.10
> 
> --
> 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

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Mar 15, 2016 at 05:10:09PM +0100, Florian Westphal wrote:
> > Instead of taking the value to set from a source register, userspace
> > passes the bit that we should set as a netlink attribute.
> > 
> > This follows a similar approach that xtables 'connlabel'
> > match uses, so when user inputs
> > 
> >     ct label set bar
> 
> I think we can introduce:
> 
>         ct label bitset bar
> 
> so this is clear to the user this is just setting the bit at that
> position.

I don't like this one bit ;)

Seriously, I think "label set bar" is fine.

We already treat "ct label foo" correctly without
using "ct label testbit foo", so I like it better
without the extra special-case "bitset" keyword.

> > @@ -777,6 +778,7 @@ enum nft_ct_attributes {
> >  	NFTA_CT_KEY,
> >  	NFTA_CT_DIRECTION,
> >  	NFTA_CT_SREG,
> > +	NFTA_CT_LABEL,
> 
> We can probably add:
> 
>         NFTA_CT_IMM

Right, we can derive the exact type based on the key.

> This would be a nested attribute, then inside there we can define the:
> 
>         NFTA_CT_LABEL

Hmm, why a nested attribute?
What do we gain from that?

Do you want the nested attr so that we can define policies
for the "immediate subtypes"...?

If its the latter, then ok.

I'd then suggest adding a new enum, for instance:

NFTA_CT_IMM_LABEL

That appear as nested attrs inside the NFTA_CT_IMM one.

> > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > index d4a4619..76da69d 100644
> > --- a/net/netfilter/nft_ct.c
> > +++ b/net/netfilter/nft_ct.c
> > @@ -29,6 +29,7 @@ struct nft_ct {
> >  		enum nft_registers	dreg:8;
> >  		enum nft_registers	sreg:8;
> >  	};
> > +	u8			set_label_bit;
> 
> I understand a specific nft_ct_label is too much at this stage, so:
> 
>         union {
>                 u8                      set_label_bit;
>         } imm;

Right, using union imm looks good.

I'll rework this tomorrow, thanks for the quick review.

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > @@ -777,6 +778,7 @@ enum nft_ct_attributes {
> > >  	NFTA_CT_KEY,
> > >  	NFTA_CT_DIRECTION,
> > >  	NFTA_CT_SREG,
> > > +	NFTA_CT_LABEL,
> > 
> > We can probably add:
> > 
> >         NFTA_CT_IMM
> 
> Right, we can derive the exact type based on the key.
> 
> > This would be a nested attribute, then inside there we can define the:
> > 
> >         NFTA_CT_LABEL
> 
> Hmm, why a nested attribute?
> What do we gain from that?
> 
> Do you want the nested attr so that we can define policies
> for the "immediate subtypes"...?
> 
> If its the latter, then ok.
> 
> I'd then suggest adding a new enum, for instance:
> 
> NFTA_CT_IMM_LABEL
> 
> That appear as nested attrs inside the NFTA_CT_IMM one.

i.e., this:

  * @NFTA_CT_SREG: source register (NLA_U32)
+ * @NFTA_CT_IMM: immediate value (NLA_NESTED)
  */
 enum nft_ct_attributes {
        NFTA_CT_UNSPEC,
@@ -783,6 +783,17 @@ enum nft_ct_attributes {
 };
 #define NFTA_CT_MAX            (__NFTA_CT_MAX - 1)
 
+/*
+ * enum nft_ct_imm_attributes - nf_tables ct expression immediate attributes
+ *
+ * @NFTA_CT_IMM_LABEL: label bit (NLA_U32)
+ */
+enum nft_ct_imm_attributes {
+       NFT_CT_IMM_LABEL,
+       __NFT_IMM_MAX
+};
+#define NFT_CT_IMM_MAX (__NFT_IMM_MAX - 1)

I still don't see the advantage of using a nested attribute for
this since we cannot have mutliple IMM keys at once.

Or was that something that you were interested in?

In that case we'd have to ignore NFTA_CT_KEY and just
rely on the presence of IMM values to tell us what we should do.

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Wed, Mar 16, 2016 at 10:39:33AM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > @@ -777,6 +778,7 @@ enum nft_ct_attributes {
> > > >  	NFTA_CT_KEY,
> > > >  	NFTA_CT_DIRECTION,
> > > >  	NFTA_CT_SREG,
> > > > +	NFTA_CT_LABEL,
> > > 
> > > We can probably add:
> > > 
> > >         NFTA_CT_IMM
> > 
> > Right, we can derive the exact type based on the key.
> > 
> > > This would be a nested attribute, then inside there we can define the:
> > > 
> > >         NFTA_CT_LABEL
> > 
> > Hmm, why a nested attribute?
> > What do we gain from that?
> > 
> > Do you want the nested attr so that we can define policies
> > for the "immediate subtypes"...?
> > 
> > If its the latter, then ok.
> > 
> > I'd then suggest adding a new enum, for instance:
> > 
> > NFTA_CT_IMM_LABEL
> > 
> > That appear as nested attrs inside the NFTA_CT_IMM one.
> 
> i.e., this:
> 
>   * @NFTA_CT_SREG: source register (NLA_U32)
> + * @NFTA_CT_IMM: immediate value (NLA_NESTED)
>   */
>  enum nft_ct_attributes {
>         NFTA_CT_UNSPEC,
> @@ -783,6 +783,17 @@ enum nft_ct_attributes {
>  };
>  #define NFTA_CT_MAX            (__NFTA_CT_MAX - 1)
>  
> +/*
> + * enum nft_ct_imm_attributes - nf_tables ct expression immediate attributes
> + *
> + * @NFTA_CT_IMM_LABEL: label bit (NLA_U32)
> + */
> +enum nft_ct_imm_attributes {
> +       NFT_CT_IMM_LABEL,
> +       __NFT_IMM_MAX
> +};
> +#define NFT_CT_IMM_MAX (__NFT_IMM_MAX - 1)
> 
> I still don't see the advantage of using a nested attribute for
> this since we cannot have mutliple IMM keys at once.
>
> Or was that something that you were interested in?

I'm trying to visualize here how the code will look in the end.

I was thinking on simplifying the codebase one we get more immediates,
we will at least get one more to handle ct helper assignment.

So from the code we can just check:

        if (nla[NFTA_CT_IMM]) {
                err = nla_parse_nested(tb, NFTA_CT_IMM, nla, nft_ct_imm_policy);
                if (err < 0)
                        return err;

                switch (key) {
                case NFT_CT_LABEL:
                        if (!tb[NFTA_CT_IMM_LABEL])
                                return -EINVAL;

                        /* ... set internal structure value */
                        break:
                case NFT_CT_HELPER:
                        if (!tb[NFTA_CT_IMM_HELPER])
                                return -EINVAL;

                        /* ... set internal structure value */
                        break;
                }
        }

With a plain linear attribute representation, I think this would look like:

        if (tb[NFTA_CT_IMM_LABEL]) {
                /* ... */
        } else if (tb[NFTA_CT_IMM_HELPER]) {
                /* ... */
        }

BTW, probably we should consider using some generic way to represent
the immediates through enum nft_data_attributes. So we have core code
that that every expression can reuse to handle immediates.  I think
the concern here is data length validation, eg. labels are 4 bytes and
helpers are strings limited to 16 bytes. From the libnftnl side, so
far we represent these as _DATA attributes:

enum {
        NFTNL_EXPR_CMP_SREG     = NFTNL_EXPR_BASE,
        NFTNL_EXPR_CMP_OP,
        NFTNL_EXPR_CMP_DATA,
};

Otherwise, we'll start seeing code in every expression handling
immediates in their own way (ie. in a not consolidated way).

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I was thinking on simplifying the codebase one we get more immediates,
> we will at least get one more to handle ct helper assignment.
> 
> So from the code we can just check:
> 
>         if (nla[NFTA_CT_IMM]) {
>                 err = nla_parse_nested(tb, NFTA_CT_IMM, nla, nft_ct_imm_policy);
>                 if (err < 0)
>                         return err;
> 
>                 switch (key) {
>                 case NFT_CT_LABEL:
>                         if (!tb[NFTA_CT_IMM_LABEL])
>                                 return -EINVAL;

Right, something like this.


> BTW, probably we should consider using some generic way to represent
> the immediates through enum nft_data_attributes. So we have core code
> that that every expression can reuse to handle immediates.  I think
> the concern here is data length validation, eg. labels are 4 bytes and
> helpers are strings limited to 16 bytes. From the libnftnl side, so
> far we represent these as _DATA attributes:
> 
> enum {
>         NFTNL_EXPR_CMP_SREG     = NFTNL_EXPR_BASE,
>         NFTNL_EXPR_CMP_OP,
>         NFTNL_EXPR_CMP_DATA,
> };
> 
> Otherwise, we'll start seeing code in every expression handling
> immediates in their own way (ie. in a not consolidated way).

Yes, thats a valid concern.

I guess it depends on wheter we want to support multi-action, or if
we will just put several actions after one another.

set label bla set helper ftp

If thats two nft_ct invocations, then generic representation
(NTA_IMM_DATA_U32, NFTA_IMM_DATA_STRING, ...) that can be used by any
expressions that need to carry their own immediate values is definitely
much better.

I'll go with using generic IMM_U32 for now, inside a CT_IMM nested
attr.

Thanks Pablo!

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Wed, Mar 16, 2016 at 02:31:42PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I was thinking on simplifying the codebase one we get more immediates,
> > we will at least get one more to handle ct helper assignment.
> > 
> > So from the code we can just check:
> > 
> >         if (nla[NFTA_CT_IMM]) {
> >                 err = nla_parse_nested(tb, NFTA_CT_IMM, nla, nft_ct_imm_policy);
> >                 if (err < 0)
> >                         return err;
> > 
> >                 switch (key) {
> >                 case NFT_CT_LABEL:
> >                         if (!tb[NFTA_CT_IMM_LABEL])
> >                                 return -EINVAL;
> 
> Right, something like this.
> 
> 
> > BTW, probably we should consider using some generic way to represent
> > the immediates through enum nft_data_attributes. So we have core code
> > that that every expression can reuse to handle immediates.  I think
> > the concern here is data length validation, eg. labels are 4 bytes and
> > helpers are strings limited to 16 bytes. From the libnftnl side, so
> > far we represent these as _DATA attributes:
> > 
> > enum {
> >         NFTNL_EXPR_CMP_SREG     = NFTNL_EXPR_BASE,
> >         NFTNL_EXPR_CMP_OP,
> >         NFTNL_EXPR_CMP_DATA,
> > };
> > 
> > Otherwise, we'll start seeing code in every expression handling
> > immediates in their own way (ie. in a not consolidated way).
> 
> Yes, thats a valid concern.
> 
> I guess it depends on wheter we want to support multi-action, or if
> we will just put several actions after one another.
> 
> set label bla set helper ftp

I would say not to this at this stage.

> If thats two nft_ct invocations, then generic representation
> (NTA_IMM_DATA_U32, NFTA_IMM_DATA_STRING, ...) that can be used by any
> expressions that need to carry their own immediate values is definitely
> much better.
> 
> I'll go with using generic IMM_U32 for now, inside a CT_IMM nested
> attr.

OK, give it a shot. Thanks Florian.

Re: [PATCH nf-next 1/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Wed, Mar 16, 2016 at 12:09:21AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Mar 15, 2016 at 05:10:09PM +0100, Florian Westphal wrote:
> > > Instead of taking the value to set from a source register, userspace
> > > passes the bit that we should set as a netlink attribute.
> > > 
> > > This follows a similar approach that xtables 'connlabel'
> > > match uses, so when user inputs
> > > 
> > >     ct label set bar
> > 
> > I think we can introduce:
> > 
> >         ct label bitset bar
> > 
> > so this is clear to the user this is just setting the bit at that
> > position.
> 
> I don't like this one bit ;)
> 
> Seriously, I think "label set bar" is fine.
> 
> We already treat "ct label foo" correctly without
> using "ct label testbit foo", so I like it better
> without the extra special-case "bitset" keyword.

OK, I don't have a strong opinion on the connlabel usecase, so let's
stick to 'label set bar'.

[PATCH libnftl 2/3] ct: add label set support

[Florian Westphal]

label set support is implemented by passing the bit value that we want
to set as a netlink attribute.

So kernel does
priv->set_label_bit = ntohl(nla_get_be32(tb[NFTA_CT_LABEL]));

and then uses
test_and_set_bit(priv->set_label_bit, ct_labels->bits);

to set it in atomic fashion.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/libnftnl/expr.h             |  1 +
 include/linux/netfilter/nf_tables.h |  2 ++
 src/expr/ct.c                       | 31 +++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/include/libnftnl/expr.h b/include/libnftnl/expr.h
index da6a251..ab5e2ec 100644
--- a/include/libnftnl/expr.h
+++ b/include/libnftnl/expr.h
@@ -140,6 +140,7 @@ enum {
 	NFTNL_EXPR_CT_KEY,
 	NFTNL_EXPR_CT_DIR,
 	NFTNL_EXPR_CT_SREG,
+	NFTNL_EXPR_CT_LABEL,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index b5fa7cb..2b41759 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -768,6 +768,7 @@ enum nft_ct_keys {
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
  * @NFTA_CT_SREG: source register (NLA_U32)
+ * @NFTA_CT_LABEL: label bit number to set (NLA_U32)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
@@ -775,6 +776,7 @@ enum nft_ct_attributes {
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
 	NFTA_CT_SREG,
+	NFTA_CT_LABEL,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/src/expr/ct.c b/src/expr/ct.c
index 4bee6b1..3250300 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -26,6 +26,7 @@ struct nftnl_expr_ct {
 	enum nft_registers	dreg;
 	enum nft_registers	sreg;
 	uint8_t			dir;
+	uint16_t		set_label_bit;
 };
 
 #define IP_CT_DIR_ORIGINAL	0
@@ -54,6 +55,9 @@ nftnl_expr_ct_set(struct nftnl_expr *e, uint16_t type,
 	case NFTNL_EXPR_CT_SREG:
 		ct->sreg = *((uint32_t *)data);
 		break;
+	case NFTNL_EXPR_CT_LABEL:
+		ct->set_label_bit = *((uint16_t *)data);
+		break;
 	default:
 		return -1;
 	}
@@ -79,6 +83,9 @@ nftnl_expr_ct_get(const struct nftnl_expr *e, uint16_t type,
 	case NFTNL_EXPR_CT_SREG:
 		*data_len = sizeof(ct->sreg);
 		return &ct->sreg;
+	case NFTNL_EXPR_CT_LABEL:
+		*data_len = sizeof(ct->set_label_bit);
+		return &ct->set_label_bit;
 	}
 	return NULL;
 }
@@ -95,6 +102,7 @@ static int nftnl_expr_ct_cb(const struct nlattr *attr, void *data)
 	case NFTA_CT_KEY:
 	case NFTA_CT_DREG:
 	case NFTA_CT_SREG:
+	case NFTA_CT_LABEL:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
 			abi_breakage();
 		break;
@@ -121,6 +129,8 @@ nftnl_expr_ct_build(struct nlmsghdr *nlh, struct nftnl_expr *e)
 		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
 	if (e->flags & (1 << NFTNL_EXPR_CT_SREG))
 		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
+	if (e->flags & (1 << NFTNL_EXPR_CT_LABEL))
+		mnl_attr_put_u32(nlh, NFTA_CT_LABEL, htonl(ct->set_label_bit));
 }
 
 static int
@@ -148,6 +158,10 @@ nftnl_expr_ct_parse(struct nftnl_expr *e, struct nlattr *attr)
 		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
 		e->flags |= (1 << NFTNL_EXPR_CT_DIR);
 	}
+	if (tb[NFTA_CT_LABEL]) {
+		ct->set_label_bit = ntohl(mnl_attr_get_u32(tb[NFTA_CT_LABEL]));
+		e->flags |= (1 << NFTNL_EXPR_CT_LABEL);
+	}
 
 	return 0;
 }
@@ -224,6 +238,7 @@ static int nftnl_expr_ct_json_parse(struct nftnl_expr *e, json_t *root,
 #ifdef JSON_PARSING
 	const char *key_str, *dir_str;
 	uint32_t reg;
+	uint16_t bit;
 	uint8_t dir;
 	int key;
 
@@ -252,6 +267,9 @@ static int nftnl_expr_ct_json_parse(struct nftnl_expr *e, json_t *root,
 		nftnl_expr_set_u8(e, NFTNL_EXPR_CT_DIR, dir);
 	}
 
+	if (nftnl_jansson_parse_val(root, "label", NFTNL_TYPE_U16, &bit, err) == 0)
+		nftnl_expr_set_u16(e, NFTNL_EXPR_CT_LABEL, bit);
+
 	return 0;
 err:
 	errno = EINVAL;
@@ -270,6 +288,7 @@ static int nftnl_expr_ct_xml_parse(struct nftnl_expr *e, mxml_node_t *tree,
 	const char *key_str, *dir_str;
 	int key;
 	uint8_t dir;
+	uint16_t bit;
 	uint32_t dreg, sreg;
 
 	if (nftnl_mxml_reg_parse(tree, "dreg", &dreg, MXML_DESCEND_FIRST,
@@ -300,6 +319,10 @@ static int nftnl_expr_ct_xml_parse(struct nftnl_expr *e, mxml_node_t *tree,
 		nftnl_expr_set_u8(e, NFTNL_EXPR_CT_DIR, dir);
 	}
 
+	if (nftnl_mxml_num_parse(tree, "label", MXML_DESCEND_FIRST, 10, &bit,
+				 NFTNL_TYPE_U16, NFTNL_XML_OPT, err) == 0)
+		nftnl_expr_set_u16(e, NFTNL_EXPR_CT_LABEL, bit);
+
 	return 0;
 err:
 	errno = EINVAL;
@@ -324,6 +347,8 @@ nftnl_expr_ct_export(char *buf, size_t size, struct nftnl_expr *e, int type)
 		nftnl_buf_str(&b, type, ctkey2str(ct->key), KEY);
 	if (e->flags & (1 << NFTNL_EXPR_CT_DIR))
 		nftnl_buf_str(&b, type, ctdir2str(ct->dir), DIR);
+	if (e->flags & (1 << NFTNL_EXPR_CT_LABEL))
+		nftnl_buf_u32(&b, type, ct->set_label_bit, NUM);
 
 	return nftnl_buf_done(&b);
 }
@@ -352,6 +377,12 @@ nftnl_expr_ct_snprintf_default(char *buf, size_t size, struct nftnl_expr *e)
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 
+	if (nftnl_expr_is_set(e, NFTNL_EXPR_CT_LABEL)) {
+		ret = snprintf(buf+offset, len, " set %s %u ",
+			       ctkey2str(NFT_CT_LABELS), ct->set_label_bit);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	return offset;
 }
 
-- 
2.4.10

[PATCH nft 3/3] ct: add conntrack label set support

[Florian Westphal]

Pablo suggested to support this by adding the label bit number
that we want to set as a netlink attribute and pass that to the kernel.

IOW, ct label set doesn't use an sreg -- instead, the bit that we
should set in the conntrack label area is taken directly from the user.

This works pretty much the same way as '-m connlabel --set foo'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 I find the placement of the expr <-> attribute conversion
 in netlink_(de)linearize to be pretty bad, but doing it
 during stmt_evaluate_ct() requires extra member in
 stmt->ct to store the 'extracted' bit value.

 Thoughts?

 src/evaluate.c            | 19 +++++++++++++++----
 src/netlink_delinearize.c | 24 +++++++++++++++++++++---
 src/netlink_linearize.c   | 20 ++++++++++++++------
 3 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 473f014..7a3be46 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1462,10 +1462,21 @@ static int stmt_evaluate_meta(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_ct(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	return stmt_evaluate_arg(ctx, stmt,
-				 stmt->ct.tmpl->dtype,
-				 stmt->ct.tmpl->len,
-				 &stmt->ct.expr);
+	int ret = stmt_evaluate_arg(ctx, stmt, stmt->ct.tmpl->dtype,
+				    stmt->ct.tmpl->len, &stmt->ct.expr);
+	if (ret < 0)
+		return ret;
+
+	switch (stmt->ct.key) {
+	case NFT_CT_LABELS:
+		if (stmt->ct.expr->ops->type != EXPR_VALUE)
+			return stmt_error(ctx, stmt, "label expected");
+		break;
+	default:
+		break;
+	}
+
+	return 0;
 }
 
 static int reject_payload_gen_dependency_tcp(struct eval_ctx *ctx,
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index d431588..86ff376 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -524,10 +524,28 @@ static void netlink_parse_ct_stmt(struct netlink_parse_ctx *ctx,
 	struct stmt *stmt;
 	struct expr *expr;
 
-	sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
-	expr = netlink_get_register(ctx, loc, sreg);
-
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
+	switch (key) {
+	case NFT_CT_LABELS: {
+		unsigned char data[128];
+		mpz_t value;
+
+		mpz_init(value);
+		mpz_setbit(value, nftnl_expr_get_u16(nle, NFTNL_EXPR_CT_LABEL));
+
+		mpz_export_data(data, value, BYTEORDER_HOST_ENDIAN, sizeof(data));
+		expr = constant_expr_alloc(loc, &integer_type,
+					   BYTEORDER_BIG_ENDIAN,
+					   BITS_PER_BYTE * sizeof(data), data);
+		break;
+	}
+	default:
+		sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
+		expr = netlink_get_register(ctx, loc, sreg);
+		break;
+	}
+
+
 	stmt = ct_stmt_alloc(loc, key, expr);
 	expr_set_type(expr, stmt->ct.tmpl->dtype, stmt->ct.tmpl->byteorder);
 
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index bb51de7..ab1103c 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -1082,15 +1082,23 @@ static void netlink_gen_queue_stmt(struct netlink_linearize_ctx *ctx,
 static void netlink_gen_ct_stmt(struct netlink_linearize_ctx *ctx,
 				  const struct stmt *stmt)
 {
-	struct nftnl_expr *nle;
+	struct nftnl_expr *nle = alloc_nft_expr("ct");
 	enum nft_registers sreg;
 
-	sreg = get_register(ctx, stmt->ct.expr);
-	netlink_gen_expr(ctx, stmt->ct.expr, sreg);
-	release_register(ctx, stmt->ct.expr);
+	switch (stmt->ct.key) {
+	case NFT_CT_LABELS:
+		nftnl_expr_set_u16(nle, NFTNL_EXPR_CT_LABEL,
+				   mpz_scan1(stmt->ct.expr->value, 0));
+		break;
+	default:
+		sreg = get_register(ctx, stmt->ct.expr);
+		netlink_gen_expr(ctx, stmt->ct.expr, sreg);
+		release_register(ctx, stmt->ct.expr);
+
+		netlink_put_register(nle, NFTNL_EXPR_CT_SREG, sreg);
+		break;
+	}
 
-	nle = alloc_nft_expr("ct");
-	netlink_put_register(nle, NFTNL_EXPR_CT_SREG, sreg);
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_CT_KEY, stmt->ct.key);
 	nftnl_rule_add_expr(ctx->nlr, nle);
 }
-- 
2.4.10

Re: [PATCH nft 3/3] ct: add conntrack label set support

[Pablo Neira Ayuso]

On Tue, Mar 15, 2016 at 05:10:11PM +0100, Florian Westphal wrote:
> Pablo suggested to support this by adding the label bit number
> that we want to set as a netlink attribute and pass that to the kernel.
> 
> IOW, ct label set doesn't use an sreg -- instead, the bit that we
> should set in the conntrack label area is taken directly from the user.
> 
> This works pretty much the same way as '-m connlabel --set foo'.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  I find the placement of the expr <-> attribute conversion
>  in netlink_(de)linearize to be pretty bad, but doing it
>  during stmt_evaluate_ct() requires extra member in
>  stmt->ct to store the 'extracted' bit value.
> 
>  Thoughts?
> 
>  src/evaluate.c            | 19 +++++++++++++++----
>  src/netlink_delinearize.c | 24 +++++++++++++++++++++---
>  src/netlink_linearize.c   | 20 ++++++++++++++------
>  3 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 473f014..7a3be46 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1462,10 +1462,21 @@ static int stmt_evaluate_meta(struct eval_ctx *ctx, struct stmt *stmt)
>  
>  static int stmt_evaluate_ct(struct eval_ctx *ctx, struct stmt *stmt)
>  {
> -	return stmt_evaluate_arg(ctx, stmt,
> -				 stmt->ct.tmpl->dtype,
> -				 stmt->ct.tmpl->len,
> -				 &stmt->ct.expr);
> +	int ret = stmt_evaluate_arg(ctx, stmt, stmt->ct.tmpl->dtype,
> +				    stmt->ct.tmpl->len, &stmt->ct.expr);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (stmt->ct.key) {
> +	case NFT_CT_LABELS:
> +		if (stmt->ct.expr->ops->type != EXPR_VALUE)
> +			return stmt_error(ctx, stmt, "label expected");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
>  }
>  
>  static int reject_payload_gen_dependency_tcp(struct eval_ctx *ctx,
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index d431588..86ff376 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -524,10 +524,28 @@ static void netlink_parse_ct_stmt(struct netlink_parse_ctx *ctx,
>  	struct stmt *stmt;
>  	struct expr *expr;
>  
> -	sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
> -	expr = netlink_get_register(ctx, loc, sreg);
> -
>  	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
> +	switch (key) {
> +	case NFT_CT_LABELS: {
> +		unsigned char data[128];
> +		mpz_t value;
> +
> +		mpz_init(value);
> +		mpz_setbit(value, nftnl_expr_get_u16(nle, NFTNL_EXPR_CT_LABEL));
> +
> +		mpz_export_data(data, value, BYTEORDER_HOST_ENDIAN, sizeof(data));
> +		expr = constant_expr_alloc(loc, &integer_type,
> +					   BYTEORDER_BIG_ENDIAN,
> +					   BITS_PER_BYTE * sizeof(data), data);
> +		break;
> +	}

If we have some generic way to parse immediates, this would look like:

        if (nfntl_attr_is_set(nle, NFTNL_EXPR_CT_SREG)) {
                ...
        } else if (nftnl_attr_is_set(nle, NFTNL_EXPR_CT_IMM)) {
                ...
        }

Would this look nicer this way to you?

> +	default:
> +		sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
> +		expr = netlink_get_register(ctx, loc, sreg);
> +		break;
> +	}
> +
> +
>  	stmt = ct_stmt_alloc(loc, key, expr);
>  	expr_set_type(expr, stmt->ct.tmpl->dtype, stmt->ct.tmpl->byteorder);
>  

Re: [PATCH nft 3/3] ct: add conntrack label set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> If we have some generic way to parse immediates, this would look like:
> 
>         if (nfntl_attr_is_set(nle, NFTNL_EXPR_CT_SREG)) {
>                 ...
>         } else if (nftnl_attr_is_set(nle, NFTNL_EXPR_CT_IMM)) {
>                 ...
>         }
> 
> Would this look nicer this way to you?

I'll give it a try.
We can't escape a bit more work anyway as the _IMM type will depend
on the key.