| Message ID | 1c8f7f6ceca5a37c5115c75ed2ebcc337e78a3d1.1579432712.git.sbrivio@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Attributes for concatenated ranges | expand |
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.
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.
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.
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);
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(-)