All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
@ 2019-11-19  1:07 Stefano Brivio
  2019-11-20 11:24 ` Phil Sutter
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Brivio @ 2019-11-19  1:07 UTC (permalink / raw)
  To: Pablo Neira Ayuso, netfilter-devel
  Cc: Florian Westphal, Kadlecsik József, Eric Garver, Phil Sutter

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(+)

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;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
  2019-11-19  1:07 [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes Stefano Brivio
@ 2019-11-20 11:24 ` Phil Sutter
  2019-11-20 12:01   ` Stefano Brivio
  0 siblings, 1 reply; 5+ messages in thread
From: Phil Sutter @ 2019-11-20 11:24 UTC (permalink / raw)
  To: Stefano Brivio
  Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal,
	Kadlecsik József, Eric Garver

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
  2019-11-20 11:24 ` Phil Sutter
@ 2019-11-20 12:01   ` Stefano Brivio
  2019-11-20 12:12     ` Stefano Brivio
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Brivio @ 2019-11-20 12:01 UTC (permalink / raw)
  To: Phil Sutter
  Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal,
	Kadlecsik József, Eric Garver

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.

-- 
Stefano


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
  2019-11-20 12:01   ` Stefano Brivio
@ 2019-11-20 12:12     ` Stefano Brivio
  2019-11-20 14:13       ` Phil Sutter
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Brivio @ 2019-11-20 12:12 UTC (permalink / raw)
  To: Phil Sutter
  Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal,
	Kadlecsik József, Eric Garver

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.

-- 
Stefano


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes
  2019-11-20 12:12     ` Stefano Brivio
@ 2019-11-20 14:13       ` Phil Sutter
  0 siblings, 0 replies; 5+ messages in thread
From: Phil Sutter @ 2019-11-20 14:13 UTC (permalink / raw)
  To: Stefano Brivio
  Cc: Pablo Neira Ayuso, netfilter-devel, Florian Westphal,
	Kadlecsik József, Eric Garver

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-11-20 14:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19  1:07 [PATCH libnftnl] set: Add support for NFTA_SET_SUBKEY attributes Stefano Brivio
2019-11-20 11:24 ` Phil Sutter
2019-11-20 12:01   ` Stefano Brivio
2019-11-20 12:12     ` Stefano Brivio
2019-11-20 14:13       ` Phil Sutter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.