Re: [PATCH 3/4] libnftnl: set: Implement new buffer of TLV objects.

[Patrick McHardy <kaber@trash.net>]

On 03.01, Carlos Falgueras García wrote:
> +/* TLV validation */
> +enum nftnl_attr_data_type {
> +	NFTNL_ATTR_TYPE_UNSPEC,
> +	NFTNL_ATTR_TYPE_U8,
> +	NFTNL_ATTR_TYPE_U16,
> +	NFTNL_ATTR_TYPE_U32,
> +	NFTNL_ATTR_TYPE_U64,
> +	NFTNL_ATTR_TYPE_STRING,
> +	NFTNL_ATTR_TYPE_FLAG,
> +	NFTNL_ATTR_TYPE_MSECS,
> +	NFTNL_ATTR_TYPE_NESTED,
> +	NFTNL_ATTR_TYPE_NESTED_COMPAT,
> +	NFTNL_ATTR_TYPE_NUL_STRING,
> +	NFTNL_ATTR_TYPE_BINARY,
> +	__NFTNL_ATTR_TYPE_MAX,

We don't need quite a few of these. Compat is obviously unnecessary. Same thing
goes for MSECS and NUL_STRING since the kernel doesn't do interpretation of
the user data. FLAG I would avoid since it uses a full u8, so better have
userspace simply use that and do interpretation as a flag.

> +/*
> + * TLV structures:
> + * nftnl_attr
> + *  <-- sizeof(nftnl_attr) -->             <-- nftnl_attr->len -->
> + * +--------------------------+- - - - - -+- - - - -  -- - - - - -+- - - - - +
> + * |          Header          |  Pading   |       Payload         |  Pading  |
> + * |         (4 bytes)        | (4 bytes) |                       |          |
> + * +--------------------------+- - - - - -+- - - - - - - - - - - -+- - - - - +
> + *  <------------------------ nftnl_attr_get_size() ------------------------>

We don't need a header I'd think. Just the raw attributes.

> + * nftnl_attr->type (16 bits)
> + * +---+---+-------------------------------+
> + * | N | O | Attribute Type                |
> + * +---+---+-------------------------------+
> + * N := Carries nested attributes
> + * O := Payload stored in network byte order
> + *
> + * Note: The N and O flag are mutually exclusive.
> + */
> +
> +struct nftnl_attr {
> +	uint16_t type;
> +	uint16_t len;

u16 is too large for both. Our maximum length is 256 anyways, without the
header u8 is enough. For type I'd also say u8 should be enough, especially
when considering the very limited space.

> +#define NFTNLA_F_NESTED		(1 << 15)
> +#define NFTNLA_F_NET_BYTEORDER	(1 << 14)
> +#define NFTNLA_TYPE_MASK	~(NFTNLA_F_NESTED | NFTNLA_F_NET_BYTEORDER)

Also unnecessary since the kernel doesn't do interpretation.

> +#define NFTNL_ALIGNTO		8
> +#define NFTNL_ALIGN(len)	(((len)+NFTNL_ALIGNTO-1) & ~(NFTNL_ALIGNTO-1))

I'd at least consider whether we really want this or whether userspace should
do an unconditional copy to avoid unaligned accesses. We have very limited
space and should try to pack tightly.
--
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