[libnftnl,v3,1/2] set: Add support for NFTA_SET_DESC_CONCAT attributes
diff mbox series

Message ID 1c8f7f6ceca5a37c5115c75ed2ebcc337e78a3d1.1579432712.git.sbrivio@redhat.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • Attributes for concatenated ranges
Related show

Commit Message

Stefano Brivio Jan. 19, 2020, 1:35 p.m. UTC
If NFTNL_SET_DESC_CONCAT data is passed, pass that to the kernel
as NFTA_SET_DESC_CONCAT attributes: it describes the length of
single concatenated fields, in bytes.

Similarly, parse NFTA_SET_DESC_CONCAT attributes if received
from the kernel.

This is the libnftnl counterpart for nftables patch:
  src: Add support for NFTNL_SET_DESC_CONCAT

v3:
 - use NFTNL_SET_DESC_CONCAT and NFTA_SET_DESC_CONCAT instead of a
   stand-alone NFTA_SET_SUBKEY attribute (Pablo Neira Ayuso)
 - pass field length in bytes instead of bits, fields would get
   unnecessarily big otherwise
v2:
 - fixed grammar in commit message
 - removed copy of array bytes in nftnl_set_nlmsg_build_subkey_payload(),
   we're simply passing values to htonl() (Phil Sutter)

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 include/libnftnl/set.h |   1 +
 include/set.h          |   2 +
 src/set.c              | 111 ++++++++++++++++++++++++++++++++++-------
 3 files changed, 95 insertions(+), 19 deletions(-)

Comments

Pablo Neira Ayuso Jan. 28, 2020, 7:30 p.m. UTC | #1
On Sun, Jan 19, 2020 at 02:35:25PM +0100, Stefano Brivio wrote:
> If NFTNL_SET_DESC_CONCAT data is passed, pass that to the kernel
> as NFTA_SET_DESC_CONCAT attributes: it describes the length of
> single concatenated fields, in bytes.
> 
> Similarly, parse NFTA_SET_DESC_CONCAT attributes if received
> from the kernel.
> 
> This is the libnftnl counterpart for nftables patch:
>   src: Add support for NFTNL_SET_DESC_CONCAT
> 
> v3:
>  - use NFTNL_SET_DESC_CONCAT and NFTA_SET_DESC_CONCAT instead of a
>    stand-alone NFTA_SET_SUBKEY attribute (Pablo Neira Ayuso)
>  - pass field length in bytes instead of bits, fields would get
>    unnecessarily big otherwise
> v2:
>  - fixed grammar in commit message
>  - removed copy of array bytes in nftnl_set_nlmsg_build_subkey_payload(),
>    we're simply passing values to htonl() (Phil Sutter)
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  include/libnftnl/set.h |   1 +
>  include/set.h          |   2 +
>  src/set.c              | 111 ++++++++++++++++++++++++++++++++++-------
>  3 files changed, 95 insertions(+), 19 deletions(-)
> 
> diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> index db3fa686d60a..dcae354b76c4 100644
> --- a/include/libnftnl/set.h
> +++ b/include/libnftnl/set.h
> @@ -24,6 +24,7 @@ enum nftnl_set_attr {
>  	NFTNL_SET_ID,
>  	NFTNL_SET_POLICY,
>  	NFTNL_SET_DESC_SIZE,
> +	NFTNL_SET_DESC_CONCAT,

This one needs to be defined at the end to not break binary interface.

Compilation breaks for some reason:

In file included from ../include/internal.h:10,
                 from gen.c:9:
../include/set.h:28:22: error: ‘NFT_REG32_COUNT’ undeclared here (not
in a function); did you mean ‘NFT_REG32_15’?
   28 |   uint8_t  field_len[NFT_REG32_COUNT];
      |                      ^~~~~~~~~~~~~~~
      |                      NFT_REG32_15

Thanks.
Stefano Brivio Jan. 28, 2020, 8:17 p.m. UTC | #2
On Tue, 28 Jan 2020 20:30:16 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Sun, Jan 19, 2020 at 02:35:25PM +0100, Stefano Brivio wrote:
> > If NFTNL_SET_DESC_CONCAT data is passed, pass that to the kernel
> > as NFTA_SET_DESC_CONCAT attributes: it describes the length of
> > single concatenated fields, in bytes.
> > 
> > Similarly, parse NFTA_SET_DESC_CONCAT attributes if received
> > from the kernel.
> > 
> > This is the libnftnl counterpart for nftables patch:
> >   src: Add support for NFTNL_SET_DESC_CONCAT
> > 
> > v3:
> >  - use NFTNL_SET_DESC_CONCAT and NFTA_SET_DESC_CONCAT instead of a
> >    stand-alone NFTA_SET_SUBKEY attribute (Pablo Neira Ayuso)
> >  - pass field length in bytes instead of bits, fields would get
> >    unnecessarily big otherwise
> > v2:
> >  - fixed grammar in commit message
> >  - removed copy of array bytes in nftnl_set_nlmsg_build_subkey_payload(),
> >    we're simply passing values to htonl() (Phil Sutter)
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  include/libnftnl/set.h |   1 +
> >  include/set.h          |   2 +
> >  src/set.c              | 111 ++++++++++++++++++++++++++++++++++-------
> >  3 files changed, 95 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> > index db3fa686d60a..dcae354b76c4 100644
> > --- a/include/libnftnl/set.h
> > +++ b/include/libnftnl/set.h
> > @@ -24,6 +24,7 @@ enum nftnl_set_attr {
> >  	NFTNL_SET_ID,
> >  	NFTNL_SET_POLICY,
> >  	NFTNL_SET_DESC_SIZE,
> > +	NFTNL_SET_DESC_CONCAT,  
> 
> This one needs to be defined at the end to not break binary interface.

Hah, right, I just focused on not breaking kernel UAPI and didn't check
this. I'll move it.

> Compilation breaks for some reason:
> 
> In file included from ../include/internal.h:10,
>                  from gen.c:9:
> ../include/set.h:28:22: error: ‘NFT_REG32_COUNT’ undeclared here (not
> in a function); did you mean ‘NFT_REG32_15’?
>    28 |   uint8_t  field_len[NFT_REG32_COUNT];
>       |                      ^~~~~~~~~~~~~~~
>       |                      NFT_REG32_15

That's something that comes from kernel headers changes, now
commit f3a2181e16f1 ("netfilter: nf_tables: Support for sets with
multiple ranged fields"), this hunk:

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index c13106496bd2..065218a20bb7 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -48,6 +48,7 @@ enum nft_registers {
 
 #define NFT_REG_SIZE   16
 #define NFT_REG32_SIZE 4
+#define NFT_REG32_COUNT        (NFT_REG32_15 - NFT_REG32_00 + 1)
 
 /**
  * enum nft_verdicts - nf_tables internal verdicts

I didn't include those in userspace patches, following e.g. current
iproute2 practice. Let me know if I should actually submit that as
separate change -- I thought it would be more practical for you to sync
headers as needed.
Pablo Neira Ayuso Jan. 28, 2020, 9:27 p.m. UTC | #3
On Tue, Jan 28, 2020 at 09:17:52PM +0100, Stefano Brivio wrote:
> On Tue, 28 Jan 2020 20:30:16 +0100
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Sun, Jan 19, 2020 at 02:35:25PM +0100, Stefano Brivio wrote:
> > > If NFTNL_SET_DESC_CONCAT data is passed, pass that to the kernel
> > > as NFTA_SET_DESC_CONCAT attributes: it describes the length of
> > > single concatenated fields, in bytes.
> > > 
> > > Similarly, parse NFTA_SET_DESC_CONCAT attributes if received
> > > from the kernel.
> > > 
> > > This is the libnftnl counterpart for nftables patch:
> > >   src: Add support for NFTNL_SET_DESC_CONCAT
> > > 
> > > v3:
> > >  - use NFTNL_SET_DESC_CONCAT and NFTA_SET_DESC_CONCAT instead of a
> > >    stand-alone NFTA_SET_SUBKEY attribute (Pablo Neira Ayuso)
> > >  - pass field length in bytes instead of bits, fields would get
> > >    unnecessarily big otherwise
> > > v2:
> > >  - fixed grammar in commit message
> > >  - removed copy of array bytes in nftnl_set_nlmsg_build_subkey_payload(),
> > >    we're simply passing values to htonl() (Phil Sutter)
> > > 
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  include/libnftnl/set.h |   1 +
> > >  include/set.h          |   2 +
> > >  src/set.c              | 111 ++++++++++++++++++++++++++++++++++-------
> > >  3 files changed, 95 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> > > index db3fa686d60a..dcae354b76c4 100644
> > > --- a/include/libnftnl/set.h
> > > +++ b/include/libnftnl/set.h
> > > @@ -24,6 +24,7 @@ enum nftnl_set_attr {
> > >  	NFTNL_SET_ID,
> > >  	NFTNL_SET_POLICY,
> > >  	NFTNL_SET_DESC_SIZE,
> > > +	NFTNL_SET_DESC_CONCAT,  
> > 
> > This one needs to be defined at the end to not break binary interface.
> 
> Hah, right, I just focused on not breaking kernel UAPI and didn't check
> this. I'll move it.

Good, thanks.

> > Compilation breaks for some reason:
> > 
> > In file included from ../include/internal.h:10,
> >                  from gen.c:9:
> > ../include/set.h:28:22: error: ‘NFT_REG32_COUNT’ undeclared here (not
> > in a function); did you mean ‘NFT_REG32_15’?
> >    28 |   uint8_t  field_len[NFT_REG32_COUNT];
> >       |                      ^~~~~~~~~~~~~~~
> >       |                      NFT_REG32_15
> 
> That's something that comes from kernel headers changes, now
> commit f3a2181e16f1 ("netfilter: nf_tables: Support for sets with
> multiple ranged fields"), this hunk:
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index c13106496bd2..065218a20bb7 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -48,6 +48,7 @@ enum nft_registers {
>  
>  #define NFT_REG_SIZE   16
>  #define NFT_REG32_SIZE 4
> +#define NFT_REG32_COUNT        (NFT_REG32_15 - NFT_REG32_00 + 1)
>  
>  /**
>   * enum nft_verdicts - nf_tables internal verdicts
> 
> I didn't include those in userspace patches, following e.g. current
> iproute2 practice. Let me know if I should actually submit that as
> separate change -- I thought it would be more practical for you to sync
> headers as needed.

I'd suggest you send a separated patch to get the nf_tables.h cached
copy under libnftnl/include/linux/

I occasionally make patches like this one:

commit 239fabea9a436aaa7b787f389d80dfb57f7b893c
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Tue Aug 13 21:41:45 2019 +0200

    include: resync nf_tables.h cache copy

    Get this header in sync with 5.3-rc1.

You bring all pending updates, so you help keep it sync :-)

Thanks.

Patch
diff mbox series

diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
index db3fa686d60a..dcae354b76c4 100644
--- a/include/libnftnl/set.h
+++ b/include/libnftnl/set.h
@@ -24,6 +24,7 @@  enum nftnl_set_attr {
 	NFTNL_SET_ID,
 	NFTNL_SET_POLICY,
 	NFTNL_SET_DESC_SIZE,
+	NFTNL_SET_DESC_CONCAT,
 	NFTNL_SET_TIMEOUT,
 	NFTNL_SET_GC_INTERVAL,
 	NFTNL_SET_USERDATA,
diff --git a/include/set.h b/include/set.h
index 446acd24ca7c..895ffdb48bdb 100644
--- a/include/set.h
+++ b/include/set.h
@@ -25,6 +25,8 @@  struct nftnl_set {
 	enum nft_set_policies	policy;
 	struct {
 		uint32_t	size;
+		uint8_t		field_len[NFT_REG32_COUNT];
+		uint8_t		field_count;
 	} desc;
 	struct list_head	element_list;
 
diff --git a/src/set.c b/src/set.c
index 78447c676f51..651dcfa56022 100644
--- a/src/set.c
+++ b/src/set.c
@@ -89,6 +89,7 @@  void nftnl_set_unset(struct nftnl_set *s, uint16_t attr)
 	case NFTNL_SET_ID:
 	case NFTNL_SET_POLICY:
 	case NFTNL_SET_DESC_SIZE:
+	case NFTNL_SET_DESC_CONCAT:
 	case NFTNL_SET_TIMEOUT:
 	case NFTNL_SET_GC_INTERVAL:
 		break;
@@ -174,6 +175,10 @@  int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 	case NFTNL_SET_DESC_SIZE:
 		memcpy(&s->desc.size, data, sizeof(s->desc.size));
 		break;
+	case NFTNL_SET_DESC_CONCAT:
+		memcpy(&s->desc.field_len, data, data_len);
+		while (s->desc.field_len[++s->desc.field_count]);
+		break;
 	case NFTNL_SET_TIMEOUT:
 		memcpy(&s->timeout, data, sizeof(s->timeout));
 		break;
@@ -266,6 +271,9 @@  const void *nftnl_set_get_data(const struct nftnl_set *s, uint16_t attr,
 	case NFTNL_SET_DESC_SIZE:
 		*data_len = sizeof(uint32_t);
 		return &s->desc.size;
+	case NFTNL_SET_DESC_CONCAT:
+		*data_len = s->desc.field_count;
+		return s->desc.field_len;
 	case NFTNL_SET_TIMEOUT:
 		*data_len = sizeof(uint64_t);
 		return &s->timeout;
@@ -351,13 +359,42 @@  err:
 	return NULL;
 }
 
+static void nftnl_set_nlmsg_build_desc_size_payload(struct nlmsghdr *nlh,
+						    struct nftnl_set *s)
+{
+	mnl_attr_put_u32(nlh, NFTA_SET_DESC_SIZE, htonl(s->desc.size));
+}
+
+static void nftnl_set_nlmsg_build_desc_concat_payload(struct nlmsghdr *nlh,
+						      struct nftnl_set *s)
+{
+	struct nlattr *nest;
+	int i;
+
+	nest = mnl_attr_nest_start(nlh, NFTA_SET_DESC_CONCAT);
+	for (i = 0; i < NFT_REG32_COUNT && i < s->desc.field_count; i++) {
+		struct nlattr *nest_elem;
+
+		nest_elem = mnl_attr_nest_start(nlh, NFTA_LIST_ELEM);
+		mnl_attr_put_u32(nlh, NFTA_SET_FIELD_LEN,
+				 htonl(s->desc.field_len[i]));
+		mnl_attr_nest_end(nlh, nest_elem);
+	}
+	mnl_attr_nest_end(nlh, nest);
+}
+
 static void
 nftnl_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nftnl_set *s)
 {
 	struct nlattr *nest;
 
 	nest = mnl_attr_nest_start(nlh, NFTA_SET_DESC);
-	mnl_attr_put_u32(nlh, NFTA_SET_DESC_SIZE, htonl(s->desc.size));
+
+	if (s->flags & (1 << NFTNL_SET_DESC_SIZE))
+		nftnl_set_nlmsg_build_desc_size_payload(nlh, s);
+	if (s->flags & (1 << NFTNL_SET_DESC_CONCAT))
+		nftnl_set_nlmsg_build_desc_concat_payload(nlh, s);
+
 	mnl_attr_nest_end(nlh, nest);
 }
 
@@ -387,7 +424,7 @@  void nftnl_set_nlmsg_build_payload(struct nlmsghdr *nlh, struct nftnl_set *s)
 		mnl_attr_put_u32(nlh, NFTA_SET_ID, htonl(s->id));
 	if (s->flags & (1 << NFTNL_SET_POLICY))
 		mnl_attr_put_u32(nlh, NFTA_SET_POLICY, htonl(s->policy));
-	if (s->flags & (1 << NFTNL_SET_DESC_SIZE))
+	if (s->flags & (1 << NFTNL_SET_DESC_SIZE | 1 << NFTNL_SET_DESC_CONCAT))
 		nftnl_set_nlmsg_build_desc_payload(nlh, s);
 	if (s->flags & (1 << NFTNL_SET_TIMEOUT))
 		mnl_attr_put_u64(nlh, NFTA_SET_TIMEOUT, htobe64(s->timeout));
@@ -445,39 +482,75 @@  static int nftnl_set_parse_attr_cb(const struct nlattr *attr, void *data)
 	return MNL_CB_OK;
 }
 
-static int nftnl_set_desc_parse_attr_cb(const struct nlattr *attr, void *data)
+static int
+nftnl_set_desc_concat_field_parse_attr_cb(const struct nlattr *attr, void *data)
+{
+	int type = mnl_attr_get_type(attr);
+	struct nftnl_set *s = data;
+
+	if (type != NFTA_SET_FIELD_LEN)
+		return MNL_CB_OK;
+
+	if (mnl_attr_validate(attr, MNL_TYPE_U32))
+		return MNL_CB_ERROR;
+
+	s->desc.field_len[s->desc.field_count] = ntohl(mnl_attr_get_u32(attr));
+	s->desc.field_count++;
+
+	return MNL_CB_OK;
+}
+
+static int
+nftnl_set_desc_concat_parse_attr_cb(const struct nlattr *attr, void *data)
 {
-	const struct nlattr **tb = data;
 	int type = mnl_attr_get_type(attr);
+	struct nftnl_set *s = data;
+
+	if (type != NFTA_LIST_ELEM)
+		return MNL_CB_OK;
+
+	return mnl_attr_parse_nested(attr,
+				     nftnl_set_desc_concat_field_parse_attr_cb,
+				     s);
+}
+
+static int nftnl_set_desc_parse_attr_cb(const struct nlattr *attr, void *data)
+{
+	int type = mnl_attr_get_type(attr), err;
+	struct nftnl_set *s = data;
 
 	if (mnl_attr_type_valid(attr, NFTA_SET_DESC_MAX) < 0)
 		return MNL_CB_OK;
 
 	switch (type) {
 	case NFTA_SET_DESC_SIZE:
-		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
+		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) {
 			abi_breakage();
+			break;
+		}
+
+		s->desc.size = ntohl(mnl_attr_get_u32(attr));
+		s->flags |= (1 << NFTNL_SET_DESC_SIZE);
+		break;
+	case NFTA_SET_DESC_CONCAT:
+		err = mnl_attr_parse_nested(attr,
+					    nftnl_set_desc_concat_parse_attr_cb,
+					    s);
+		if (err != MNL_CB_OK)
+			abi_breakage();
+
+		s->flags |= (1 << NFTNL_SET_DESC_CONCAT);
+		break;
+	default:
 		break;
 	}
 
-	tb[type] = attr;
 	return MNL_CB_OK;
 }
 
-static int nftnl_set_desc_parse(struct nftnl_set *s,
-			      const struct nlattr *attr)
+static int nftnl_set_desc_parse(struct nftnl_set *s, const struct nlattr *attr)
 {
-	struct nlattr *tb[NFTA_SET_DESC_MAX + 1] = {};
-
-	if (mnl_attr_parse_nested(attr, nftnl_set_desc_parse_attr_cb, tb) < 0)
-		return -1;
-
-	if (tb[NFTA_SET_DESC_SIZE]) {
-		s->desc.size = ntohl(mnl_attr_get_u32(tb[NFTA_SET_DESC_SIZE]));
-		s->flags |= (1 << NFTNL_SET_DESC_SIZE);
-	}
-
-	return 0;
+	return mnl_attr_parse_nested(attr, nftnl_set_desc_parse_attr_cb, s);
 }
 
 EXPORT_SYMBOL(nftnl_set_nlmsg_parse);