netdev - Re: [PATCH RFC v2 net-next 09/28] p4tc: add P4 data types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZH20IH+yHMk5kAcb@corigine.com>
Date: Mon, 5 Jun 2023 12:08:32 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org, deb.chatterjee@...el.com,
	anjali.singhai@...el.com, namrata.limaye@...el.com, tom@...anda.io,
	p4tc-discussions@...devconf.info, mleitner@...hat.com,
	Mahesh.Shirshyad@....com, Vipin.Jain@....com,
	tomasz.osinski@...el.com, jiri@...nulli.us,
	xiyou.wangcong@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, vladbu@...dia.com,
	khalidm@...dia.com, toke@...hat.com
Subject: Re: [PATCH RFC v2 net-next 09/28] p4tc: add P4 data types

On Wed, May 17, 2023 at 07:02:13AM -0400, Jamal Hadi Salim wrote:
> Introduce abstraction that represents P4 data types.
> This also introduces the Kconfig and Makefile which later patches use.
> Types could be little, host or big endian definitions. The abstraction also
> supports defining:
> 
> a) bitstrings using annotations in control that look like "bitX" where X
>    is the number of bits defined in a type
> 
> b) bitslices such that one can define in control bit8[0-3] and
>    bit16[0-9]. A 4-bit slice from bits 0-3 and a 10-bit slice from bits
>    0-9 respectively.
> 
> Each type has a bitsize, a name (for debugging purposes), an ID and
> methods/ops. The P4 types will be used by metadata, headers, dynamic
> actions and other part of P4TC.
> 
> Each type has four ops:
> 
> - validate_p4t: Which validates if a given value of a specific type
>   meets valid boundary conditions.
> 
> - create_bitops: Which, given a bitsize, bitstart and bitend allocates and
>   returns a mask and a shift value. For example, if we have type bit8[3-3]
>   meaning bitstart = 3 and bitend = 3, we'll create a mask which would only
>   give us the fourth bit of a bit8 value, that is, 0x08. Since we are
>   interested in the fourth bit, the bit shift value will be 3.
> 
> - host_read : Which reads the value of a given type and transforms it to
>   host order
> 
> - host_write : Which writes a provided host order value and transforms it
>   to the type's native order
> 
> Co-developed-by: Victor Nogueira <victor@...atatu.com>
> Signed-off-by: Victor Nogueira <victor@...atatu.com>
> Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>

Hi Victor, Pedro and Jamal,

some minor feedback from my side.

> diff --git a/net/sched/p4tc/p4tc_types.c b/net/sched/p4tc/p4tc_types.c

...

> +static struct p4tc_type *p4type_find_byname(const char *name)
> +{
> +	struct p4tc_type *type;
> +	unsigned long tmp, typeid;

As per my comment on another patch in this series,
please use reverse xmas tree - longest line to shortest -
for local variable declarations in networking code.

The following tool can help:
https://github.com/ecree-solarflare/xmastree

> +
> +	idr_for_each_entry_ul(&p4tc_types_idr, type, tmp, typeid) {
> +		if (!strncmp(type->name, name, P4T_MAX_STR_SZ))
> +			return type;
> +	}
> +
> +	return NULL;
> +}

...

> +static int p4t_be32_validate(struct p4tc_type *container, void *value,
> +			     u16 bitstart, u16 bitend,
> +			     struct netlink_ext_ack *extack)
> +{
> +	size_t container_maxsz = U32_MAX;
> +	__u32 *val_u32 = value;
> +	__be32 val = 0;
> +	size_t maxval;
> +	int ret;
> +
> +	ret = p4t_validate_bitpos(bitstart, bitend, 31, 31, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value)
> +		val = (__be32)(be32_to_cpu(*val_u32));

>From a type annotation point of view, a value can be either CPU byte order
or big endian. It can't be both. What was the byte order of val_u32?  What is
the desired byte order of val?

Sparse, invoked using make C=1, flags this and
sever other issues with endineness handling.

> +
> +	maxval = GENMASK(bitend, 0);
> +	if (val && (val > container_maxsz || val > maxval)) {
> +		NL_SET_ERR_MSG_MOD(extack, "BE32 value out of range");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...