diff mbox

[libnftables,v2] Add support for ct set

Message ID 1389170211-7024-1-git-send-email-kristian.evensen@gmail.com
State Superseded
Headers show

Commit Message

Kristian Evensen Jan. 8, 2014, 8:36 a.m. UTC 
From: Kristian Evensen <kristian.evensen@gmail.com>

This patch adds userspace support for setting properties of tracked connections.
Currently, the connection mark is supported. This can be used to implemented the
same functionality as iptables -j CONNMARK --save-mark.

v1->v2:
- Fixed a style error.
- Improved error handling. Fail if neither sreg nor dreg is set (was already
  present when parsing XML).

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
---
 include/libnftables/expr.h          |   1 +
 include/linux/netfilter/nf_tables.h |   2 +
 src/expr/ct.c                       | 122 ++++++++++++++++++++++++++++--------
 3 files changed, 100 insertions(+), 25 deletions(-)

Comments

Pablo Neira Ayuso Jan. 10, 2014, 12:50 a.m. UTC | #1 
On Wed, Jan 08, 2014 at 09:36:51AM +0100, Kristian Evensen wrote:
> From: Kristian Evensen <kristian.evensen@gmail.com>
> 
> This patch adds userspace support for setting properties of tracked connections.
> Currently, the connection mark is supported. This can be used to implemented the
> same functionality as iptables -j CONNMARK --save-mark.

This applies cleanly to libnftables, but I need to request you some
changes.

> v1->v2:
> - Fixed a style error.
> - Improved error handling. Fail if neither sreg nor dreg is set (was already
>   present when parsing XML).
> 
> Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
> ---
>  include/libnftables/expr.h          |   1 +
>  include/linux/netfilter/nf_tables.h |   2 +
>  src/expr/ct.c                       | 122 ++++++++++++++++++++++++++++--------
>  3 files changed, 100 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
> index 25455e4..653bbb0 100644
> --- a/include/libnftables/expr.h
> +++ b/include/libnftables/expr.h
> @@ -124,6 +124,7 @@ enum {
>  	NFT_EXPR_CT_DREG	= NFT_RULE_EXPR_ATTR_BASE,
>  	NFT_EXPR_CT_KEY,
>  	NFT_EXPR_CT_DIR,
> +	NFT_EXPR_CT_SREG,
>  };
>  
>  enum {
> diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
> index e08f80e..1b6362c 100644
> --- a/include/linux/netfilter/nf_tables.h
> +++ b/include/linux/netfilter/nf_tables.h
> @@ -526,12 +526,14 @@ enum nft_ct_keys {
>   * @NFTA_CT_DREG: destination register (NLA_U32)
>   * @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)
>   */
>  enum nft_ct_attributes {
>  	NFTA_CT_UNSPEC,
>  	NFTA_CT_DREG,
>  	NFTA_CT_KEY,
>  	NFTA_CT_DIRECTION,
> +	NFTA_CT_SREG,
>  	__NFTA_CT_MAX
>  };
>  #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
> diff --git a/src/expr/ct.c b/src/expr/ct.c
> index 46e3cef..00de64b 100644
> --- a/src/expr/ct.c
> +++ b/src/expr/ct.c
> @@ -24,7 +24,10 @@
>  
>  struct nft_expr_ct {
>  	enum nft_ct_keys        key;
> -	uint32_t		dreg;	/* enum nft_registers */
> +	union {
> +		uint32_t		dreg;	/* enum nft_registers */
> +		uint32_t		sreg;	/* enum nft_registers */
> +	};

I know you have based this patch on the meta/set support, which does
exactly this thing, but I think we have to have two different fields
sreg and dreg (so remove the union), the reason is explain below.

>  	uint8_t			dir;
>  };
>  
> @@ -51,6 +54,9 @@ nft_rule_expr_ct_set(struct nft_rule_expr *e, uint16_t type,
>  	case NFT_EXPR_CT_DREG:
>  		ct->dreg = *((uint32_t *)data);
>  		break;
> +	case NFT_EXPR_CT_SREG:
> +		ct->sreg = *((uint32_t *)data);
> +		break;
>  	default:
>  		return -1;
>  	}
> @@ -73,6 +79,9 @@ nft_rule_expr_ct_get(const struct nft_rule_expr *e, uint16_t type,
>  	case NFT_EXPR_CT_DREG:
>  		*data_len = sizeof(ct->dreg);
>  		return &ct->dreg;
> +	case NFT_EXPR_CT_SREG:
> +		*data_len = sizeof(ct->sreg);
> +		return &ct->sreg;
>  	}
>  	return NULL;
>  }
> @@ -88,6 +97,7 @@ static int nft_rule_expr_ct_cb(const struct nlattr *attr, void *data)
>  	switch(type) {
>  	case NFTA_CT_KEY:
>  	case NFTA_CT_DREG:
> +	case NFTA_CT_SREG:
>  		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) {
>  			perror("mnl_attr_validate");
>  			return MNL_CB_ERROR;
> @@ -116,6 +126,8 @@ nft_rule_expr_ct_build(struct nlmsghdr *nlh, struct nft_rule_expr *e)
>  		mnl_attr_put_u32(nlh, NFTA_CT_DREG, htonl(ct->dreg));
>  	if (e->flags & (1 << NFT_EXPR_CT_DIR))
>  		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
> +	if (e->flags & (1 << NFT_EXPR_CT_SREG))
> +		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
>  }
>  
>  static int
> @@ -131,14 +143,19 @@ nft_rule_expr_ct_parse(struct nft_rule_expr *e, struct nlattr *attr)
>  		ct->key = ntohl(mnl_attr_get_u32(tb[NFTA_CT_KEY]));
>  		e->flags |= (1 << NFT_EXPR_CT_KEY);
>  	}
> -	if (tb[NFTA_CT_DREG]) {
> -		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
> -		e->flags |= (1 << NFT_EXPR_CT_DREG);
> -	}
>  	if (tb[NFTA_CT_DIRECTION]) {
>  		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
>  		e->flags |= (1 << NFT_EXPR_CT_DIR);
>  	}
> +	if (tb[NFTA_CT_DREG]) {
> +		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
> +		e->flags |= (1 << NFT_EXPR_CT_DREG);
> +	} else if (tb[NFTA_CT_SREG]) {
> +		ct->sreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_SREG]));
> +		e->flags |= (1 << NFT_EXPR_CT_SREG);

I would like to avoid this sort of obscure behaviour. I know that,
from the kernel representation point of view, it doesn't make any
sense to have set both sreg and dreg, but we should not make such
assumption from userspace. If the user sets both sreg and dreg, it's
wrong and the kernel should reply -EINVAL so the user knows that it
has to fix its userspace code. This requires a bit more memory in
userspace, but we don't have the memory restrictions that we have in
kernelspace, so a bit more consumption won't harm.

Please, rework this. It would be good to rework the meta/set part
available in libnftables next-3.14. If you cannot make it, let me know
and I'll schedule time to fix that. 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
Kristian Evensen Jan. 10, 2014, 8:26 a.m. UTC | #2 
Hi Pablo,

On Fri, Jan 10, 2014 at 1:50 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Please, rework this. It would be good to rework the meta/set part
> available in libnftables next-3.14. If you cannot make it, let me know
> and I'll schedule time to fix that. Thanks.

Thank you for your feedback, I will try to rework the patch today and
if not, then over the weekend.

Btw, during development I noticed that the dreg in ct (in libnftables)
is store as a uint32, while meta uses a uint8 for dreg/sreg. I use a
uint32 for sreg to be consistent with what is already there, but after
looking more into the code this seems not be needed as kernel
sreg/dreg is only 8 bits wide. Should I change the storage type for
sreg/dreg at the same time, or does it qualify as obscure behavior
too?

-Kristian
--
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 Jan. 10, 2014, 10:37 a.m. UTC | #3 
On Fri, Jan 10, 2014 at 09:26:12AM +0100, Kristian Evensen wrote:
> Hi Pablo,
> 
> On Fri, Jan 10, 2014 at 1:50 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Please, rework this. It would be good to rework the meta/set part
> > available in libnftables next-3.14. If you cannot make it, let me know
> > and I'll schedule time to fix that. Thanks.
> 
> Thank you for your feedback, I will try to rework the patch today and
> if not, then over the weekend.
> 
> Btw, during development I noticed that the dreg in ct (in libnftables)
> is store as a uint32, while meta uses a uint8 for dreg/sreg. I use a
> uint32 for sreg to be consistent with what is already there, but after
> looking more into the code this seems not be needed as kernel
> sreg/dreg is only 8 bits wide. Should I change the storage type for
> sreg/dreg at the same time, or does it qualify as obscure behavior
> too?

That's inconsistent and needs to be fixed. My suggestion is to fix it
by using u32 for registers. 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
Arturo Borrero Jan. 10, 2014, 11:06 a.m. UTC | #4 
On 10 January 2014 11:37, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> That's inconsistent and needs to be fixed. My suggestion is to fix it
> by using u32 for registers. Thanks.

cmp [0] also uses uint8_t for a register.

Also, others [1] use 'enum nft_registers'.

Should I patch all and switch to enum nft_registers?

regards

[0] http://git.netfilter.org/libnftables/tree/src/expr/cmp.c#n29
[1] http://git.netfilter.org/libnftables/tree/src/expr/immediate.c#n27
Pablo Neira Ayuso Jan. 10, 2014, 11:30 a.m. UTC | #5 
On Fri, Jan 10, 2014 at 12:06:29PM +0100, Arturo Borrero Gonzalez wrote:
> On 10 January 2014 11:37, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > That's inconsistent and needs to be fixed. My suggestion is to fix it
> > by using u32 for registers. Thanks.
> 
> cmp [0] also uses uint8_t for a register.
> 
> Also, others [1] use 'enum nft_registers'.
> 
> Should I patch all and switch to enum nft_registers?

Yes please, go ahead review that. 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
Kristian Evensen Jan. 10, 2014, 11:37 a.m. UTC | #6 
Hi,

On Fri, Jan 10, 2014 at 12:06 PM, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> Should I patch all and switch to enum nft_registers?

I can do ct.c as a part of reworking the ct set-patch.

-Kristian
--
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
Arturo Borrero Jan. 10, 2014, 11:42 a.m. UTC | #7 
On 10 January 2014 12:37, Kristian Evensen <kristian.evensen@gmail.com> wrote:
> Hi,
>
> On Fri, Jan 10, 2014 at 12:06 PM, Arturo Borrero Gonzalez
> <arturo.borrero.glez@gmail.com> wrote:
>> Should I patch all and switch to enum nft_registers?
>
> I can do ct.c as a part of reworking the ct set-patch.
>

Good, I'll handle the others.

thanks.
diff mbox

Patch

diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
index 25455e4..653bbb0 100644
--- a/include/libnftables/expr.h
+++ b/include/libnftables/expr.h
@@ -124,6 +124,7 @@  enum {
 	NFT_EXPR_CT_DREG	= NFT_RULE_EXPR_ATTR_BASE,
 	NFT_EXPR_CT_KEY,
 	NFT_EXPR_CT_DIR,
+	NFT_EXPR_CT_SREG,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index e08f80e..1b6362c 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -526,12 +526,14 @@  enum nft_ct_keys {
  * @NFTA_CT_DREG: destination register (NLA_U32)
  * @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)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
 	NFTA_CT_DREG,
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
+	NFTA_CT_SREG,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/src/expr/ct.c b/src/expr/ct.c
index 46e3cef..00de64b 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -24,7 +24,10 @@ 
 
 struct nft_expr_ct {
 	enum nft_ct_keys        key;
-	uint32_t		dreg;	/* enum nft_registers */
+	union {
+		uint32_t		dreg;	/* enum nft_registers */
+		uint32_t		sreg;	/* enum nft_registers */
+	};
 	uint8_t			dir;
 };
 
@@ -51,6 +54,9 @@  nft_rule_expr_ct_set(struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		ct->dreg = *((uint32_t *)data);
 		break;
+	case NFT_EXPR_CT_SREG:
+		ct->sreg = *((uint32_t *)data);
+		break;
 	default:
 		return -1;
 	}
@@ -73,6 +79,9 @@  nft_rule_expr_ct_get(const struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		*data_len = sizeof(ct->dreg);
 		return &ct->dreg;
+	case NFT_EXPR_CT_SREG:
+		*data_len = sizeof(ct->sreg);
+		return &ct->sreg;
 	}
 	return NULL;
 }
@@ -88,6 +97,7 @@  static int nft_rule_expr_ct_cb(const struct nlattr *attr, void *data)
 	switch(type) {
 	case NFTA_CT_KEY:
 	case NFTA_CT_DREG:
+	case NFTA_CT_SREG:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) {
 			perror("mnl_attr_validate");
 			return MNL_CB_ERROR;
@@ -116,6 +126,8 @@  nft_rule_expr_ct_build(struct nlmsghdr *nlh, struct nft_rule_expr *e)
 		mnl_attr_put_u32(nlh, NFTA_CT_DREG, htonl(ct->dreg));
 	if (e->flags & (1 << NFT_EXPR_CT_DIR))
 		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
 }
 
 static int
@@ -131,14 +143,19 @@  nft_rule_expr_ct_parse(struct nft_rule_expr *e, struct nlattr *attr)
 		ct->key = ntohl(mnl_attr_get_u32(tb[NFTA_CT_KEY]));
 		e->flags |= (1 << NFT_EXPR_CT_KEY);
 	}
-	if (tb[NFTA_CT_DREG]) {
-		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
-		e->flags |= (1 << NFT_EXPR_CT_DREG);
-	}
 	if (tb[NFTA_CT_DIRECTION]) {
 		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
 		e->flags |= (1 << NFT_EXPR_CT_DIR);
 	}
+	if (tb[NFTA_CT_DREG]) {
+		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
+		e->flags |= (1 << NFT_EXPR_CT_DREG);
+	} else if (tb[NFTA_CT_SREG]) {
+		ct->sreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_SREG]));
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
+	} else {
+		return -1;
+	}
 
 	return 0;
 }
@@ -186,10 +203,19 @@  static int nft_rule_expr_ct_json_parse(struct nft_rule_expr *e, json_t *root)
 	uint8_t dir;
 	int key;
 
-	if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
-		return -1;
+	if (nft_jansson_node_exist(root, "dreg")) {
+		if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
 
-	nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+	} else if (nft_jansson_node_exist(root, "sreg")) {
+		if (nft_jansson_parse_reg(root, "sreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
+
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_SREG, reg);
+	} else {
+		return -1;
+	}
 
 	if (nft_jansson_node_exist(root, "key")) {
 		key_str = nft_jansson_parse_str(root, "key");
@@ -235,11 +261,17 @@  static int nft_rule_expr_ct_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree
 	uint8_t dir;
 
 	reg = nft_mxml_reg_parse(tree, "dreg", MXML_DESCEND_FIRST);
-	if (reg < 0)
-		return -1;
+	if (reg >= 0) {
+		ct->dreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_DREG);
+	} else {
+		reg = nft_mxml_reg_parse(tree, "sreg", MXML_DESCEND_FIRST);
+		if (reg < 0)
+			return -1;
 
-	ct->dreg = reg;
-	e->flags |= (1 << NFT_EXPR_CT_DREG);
+		ct->sreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
+	}
 
 	key_str = nft_mxml_str_parse(tree, "key", MXML_DESCEND_FIRST,
 				     NFT_XML_MAND);
@@ -274,13 +306,60 @@  err:
 }
 
 static int
-nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
+nft_rule_expr_ct_snprintf_default(char *buf, size_t size,
+				  struct nft_rule_expr *e)
+{
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		return snprintf(buf, size, "set %s with reg %u ",
+				ctkey2str(ct->key), ct->sreg);
+
+	return snprintf(buf, size, "load %s => reg %u dir %u ",
+			ctkey2str(ct->key), ct->dreg, ct->dir);
+}
+
+static int
+nft_rule_expr_ct_snprintf_xml(char *buf, size_t size, struct nft_rule_expr *e)
 {
 	int ret, len = size, offset = 0;
 	struct nft_expr_ct *ct = nft_expr_data(e);
 
-	ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "<dreg>%u</dreg>", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	} else if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "<sreg>%u</sreg>", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
+		ret = snprintf(buf+offset, len, "<key>%s</key>",
+			       ctkey2str(ct->key));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_DIR)) {
+		ret = snprintf(buf+offset, len, "<dir>%u</dir>", ct->dir);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	return offset;
+}
+
+static int
+nft_rule_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
+{
+	int ret, len = size, offset = 0;
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	} else if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "\"sreg:\":%u", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
 
 	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
 		ret = snprintf(buf+offset, len, ",\"key\":\"%s\"",
@@ -294,26 +373,19 @@  nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
 	}
 
 	return offset;
-
 }
 
 static int
 nft_rule_expr_ct_snprintf(char *buf, size_t len, uint32_t type,
 			    uint32_t flags, struct nft_rule_expr *e)
 {
-	struct nft_expr_ct *ct = nft_expr_data(e);
-
 	switch(type) {
 	case NFT_OUTPUT_DEFAULT:
-		return snprintf(buf, len, "load %s => reg %u dir %u ",
-				ctkey2str(ct->key), ct->dreg, ct->dir);
+		return nft_rule_expr_ct_snprintf_default(buf, len, e);
 	case NFT_OUTPUT_XML:
-		return snprintf(buf, len, "<dreg>%u</dreg>"
-					  "<key>%s</key>"
-					  "<dir>%u</dir>",
-				ct->dreg, ctkey2str(ct->key), ct->dir);
+		return nft_rule_expr_ct_snprintf_xml(buf, len, e);
 	case NFT_OUTPUT_JSON:
-		return nft_expr_ct_snprintf_json(buf, len, e);
+		return nft_rule_expr_ct_snprintf_json(buf, len, e);
 	default:
 		break;
 	}