| Message ID | 20191119010723.39368-1-sbrivio@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnftnl] set: Add support for NFTA_SET_SUBKEY attributes | expand |
Hi, On Tue, Nov 19, 2019 at 02:07:23AM +0100, Stefano Brivio wrote: [...] > diff --git a/src/set.c b/src/set.c > index 78447c6..60a46d8 100644 > --- a/src/set.c > +++ b/src/set.c [...] > @@ -361,6 +366,23 @@ nftnl_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nftnl_set *s) > mnl_attr_nest_end(nlh, nest); > } > > +static void > +nftnl_set_nlmsg_build_subkey_payload(struct nlmsghdr *nlh, struct nftnl_set *s) > +{ > + struct nlattr *nest; > + uint32_t v; > + uint8_t *l; > + > + nest = mnl_attr_nest_start(nlh, NFTA_SET_SUBKEY); > + for (l = s->subkey_len; l - s->subkey_len < NFT_REG32_COUNT; l++) { While I like pointer arithmetics, too, I don't think it's much use here. Using good old index variable even allows to integrate the zero value check: | for (i = 0; i < NFT_REG32_COUNT && s->subkey_len[i]; i++) > + if (!*l) > + break; > + v = *l; > + mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v)); I guess you're copying the value here because how htonl() is declared, but may it change the input value non-temporarily? I mean, libnftnl is in control over the array so from my point of view it should be OK to directly pass it to htonl(). Cheers, Phil
On Wed, 20 Nov 2019 12:24:48 +0100 Phil Sutter <phil@nwl.cc> wrote: > Hi, > > On Tue, Nov 19, 2019 at 02:07:23AM +0100, Stefano Brivio wrote: > [...] > > diff --git a/src/set.c b/src/set.c > > index 78447c6..60a46d8 100644 > > --- a/src/set.c > > +++ b/src/set.c > [...] > > @@ -361,6 +366,23 @@ nftnl_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nftnl_set *s) > > mnl_attr_nest_end(nlh, nest); > > } > > > > +static void > > +nftnl_set_nlmsg_build_subkey_payload(struct nlmsghdr *nlh, struct nftnl_set *s) > > +{ > > + struct nlattr *nest; > > + uint32_t v; > > + uint8_t *l; > > + > > + nest = mnl_attr_nest_start(nlh, NFTA_SET_SUBKEY); > > + for (l = s->subkey_len; l - s->subkey_len < NFT_REG32_COUNT; l++) { > > While I like pointer arithmetics, too, I don't think it's much use here. > Using good old index variable even allows to integrate the zero value > check: > > | for (i = 0; i < NFT_REG32_COUNT && s->subkey_len[i]; i++) Oh, yes, better. I'll change this in v2. > > + if (!*l) > > + break; > > + v = *l; > > + mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v)); > > I guess you're copying the value here because how htonl() is declared, > but may it change the input value non-temporarily? I mean, libnftnl is > in control over the array so from my point of view it should be OK to > directly pass it to htonl(). It won't change the input value at all, but that's not the point: I'm reading from an array of 8-bit values and attributes are 32 bits. If I htonl() directly on the input array, it's going to use 24 bits around those 8 bits.
On Wed, 20 Nov 2019 13:01:52 +0100 Stefano Brivio <sbrivio@redhat.com> wrote: > On Wed, 20 Nov 2019 12:24:48 +0100 > Phil Sutter <phil@nwl.cc> wrote: > > [...] > > > > + if (!*l) > > > + break; > > > + v = *l; > > > + mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v)); > > > > I guess you're copying the value here because how htonl() is declared, > > but may it change the input value non-temporarily? I mean, libnftnl is > > in control over the array so from my point of view it should be OK to > > directly pass it to htonl(). > > It won't change the input value at all, but that's not the point: I'm > reading from an array of 8-bit values and attributes are 32 bits. If I > htonl() directly on the input array, it's going to use 24 bits around > those 8 bits. Err, wait, never mind :) I'm just passing the value, not the reference there -- no need to copy anything of course. I'll drop this copy in v2.
On Wed, Nov 20, 2019 at 01:12:56PM +0100, Stefano Brivio wrote: > On Wed, 20 Nov 2019 13:01:52 +0100 > Stefano Brivio <sbrivio@redhat.com> wrote: > > > On Wed, 20 Nov 2019 12:24:48 +0100 > > Phil Sutter <phil@nwl.cc> wrote: > > > > [...] > > > > > > + if (!*l) > > > > + break; > > > > + v = *l; > > > > + mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v)); > > > > > > I guess you're copying the value here because how htonl() is declared, > > > but may it change the input value non-temporarily? I mean, libnftnl is > > > in control over the array so from my point of view it should be OK to > > > directly pass it to htonl(). > > > > It won't change the input value at all, but that's not the point: I'm > > reading from an array of 8-bit values and attributes are 32 bits. If I > > htonl() directly on the input array, it's going to use 24 bits around > > those 8 bits. > > Err, wait, never mind :) I'm just passing the value, not the reference > there -- no need to copy anything of course. I'll drop this copy in v2. I wondered if htonl() may be implemented as a macro and therefore side-effects are indeed possible. In fact, the opposite is the case (declared with const attribute). Cheers, Phil
diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h index db3fa68..0a5e02a 100644 --- a/include/libnftnl/set.h +++ b/include/libnftnl/set.h @@ -29,6 +29,7 @@ enum nftnl_set_attr { NFTNL_SET_USERDATA, NFTNL_SET_OBJ_TYPE, NFTNL_SET_HANDLE, + NFTNL_SET_SUBKEY, __NFTNL_SET_MAX }; #define NFTNL_SET_MAX (__NFTNL_SET_MAX - 1) diff --git a/include/set.h b/include/set.h index 446acd2..41d5dba 100644 --- a/include/set.h +++ b/include/set.h @@ -31,6 +31,8 @@ struct nftnl_set { uint32_t flags; uint32_t gc_interval; uint64_t timeout; + + uint8_t subkey_len[NFT_REG32_COUNT]; }; struct nftnl_set_list; diff --git a/src/set.c b/src/set.c index 78447c6..60a46d8 100644 --- a/src/set.c +++ b/src/set.c @@ -91,6 +91,7 @@ void nftnl_set_unset(struct nftnl_set *s, uint16_t attr) case NFTNL_SET_DESC_SIZE: case NFTNL_SET_TIMEOUT: case NFTNL_SET_GC_INTERVAL: + case NFTNL_SET_SUBKEY: break; case NFTNL_SET_USERDATA: xfree(s->user.data); @@ -115,6 +116,7 @@ static uint32_t nftnl_set_validate[NFTNL_SET_MAX + 1] = { [NFTNL_SET_DESC_SIZE] = sizeof(uint32_t), [NFTNL_SET_TIMEOUT] = sizeof(uint64_t), [NFTNL_SET_GC_INTERVAL] = sizeof(uint32_t), + [NFTNL_SET_SUBKEY] = sizeof(uint8_t) * NFT_REG32_COUNT, }; EXPORT_SYMBOL(nftnl_set_set_data); @@ -190,6 +192,9 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data, memcpy(s->user.data, data, data_len); s->user.len = data_len; break; + case NFTNL_SET_SUBKEY: + memcpy(s->subkey_len, data, data_len); + break; } s->flags |= (1 << attr); return 0; @@ -361,6 +366,23 @@ nftnl_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nftnl_set *s) mnl_attr_nest_end(nlh, nest); } +static void +nftnl_set_nlmsg_build_subkey_payload(struct nlmsghdr *nlh, struct nftnl_set *s) +{ + struct nlattr *nest; + uint32_t v; + uint8_t *l; + + nest = mnl_attr_nest_start(nlh, NFTA_SET_SUBKEY); + for (l = s->subkey_len; l - s->subkey_len < NFT_REG32_COUNT; l++) { + if (!*l) + break; + v = *l; + mnl_attr_put_u32(nlh, NFTA_SET_SUBKEY_LEN, htonl(v)); + } + mnl_attr_nest_end(nlh, nest); +} + EXPORT_SYMBOL(nftnl_set_nlmsg_build_payload); void nftnl_set_nlmsg_build_payload(struct nlmsghdr *nlh, struct nftnl_set *s) { @@ -395,6 +417,8 @@ void nftnl_set_nlmsg_build_payload(struct nlmsghdr *nlh, struct nftnl_set *s) mnl_attr_put_u32(nlh, NFTA_SET_GC_INTERVAL, htonl(s->gc_interval)); if (s->flags & (1 << NFTNL_SET_USERDATA)) mnl_attr_put(nlh, NFTA_SET_USERDATA, s->user.len, s->user.data); + if (s->flags & (1 << NFTNL_SET_SUBKEY)) + nftnl_set_nlmsg_build_subkey_payload(nlh, s); } @@ -439,6 +463,10 @@ static int nftnl_set_parse_attr_cb(const struct nlattr *attr, void *data) if (mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0) abi_breakage(); break; + case NFTA_SET_SUBKEY: + if (mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0) + abi_breakage(); + break; } tb[type] = attr;
If the NFTNL_SET_SUBKEY flag is passed, send one NFTA_SET_SUBKEY attributes per set subkey_len attribute in the set description. Note that our internal representation, and nftables storage, for these attributes, is 8-bit wide, but the kernel uses 32 bits. As field length is expressed in bits, this is probably a good compromise to keep the UAPI future-proof and memory footprint to a minimum, for the moment being. This is the libnftnl counterpart for nftables patch: src: Add support for and export NFT_SET_SUBKEY attributes and depends on the UAPI changes from patch: [PATCH nf-next 1/8] netfilter: nf_tables: Support for subkeys, set with multiple ranged fields Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- I'm not updating the UAPI header copy in this series, please let me know if I'd better do that. include/libnftnl/set.h | 1 + include/set.h | 2 ++ src/set.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 31 insertions(+)