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]

Date: Fri, 2 Jun 2023 13:30:38 -0700
From: Marcelo Ricardo Leitner <mleitner@...hat.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, 
	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, 
	simon.horman@...igine.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:
> +bool p4tc_type_unsigned(int typeid)

Nit, maybe name it p4tc_is_type_unsigned() instead.

> +{
> +	switch (typeid) {
> +	case P4T_U8:
> +	case P4T_U16:
> +	case P4T_U32:
> +	case P4T_U64:
> +	case P4T_U128:
> +	case P4T_BOOL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +int p4t_copy(struct p4tc_type_mask_shift *dst_mask_shift,
> +	     struct p4tc_type *dst_t, void *dstv,
> +	     struct p4tc_type_mask_shift *src_mask_shift,
> +	     struct p4tc_type *src_t, void *srcv)
> +{
> +	u64 readval[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> +	const struct p4tc_type_ops *srco, *dsto;
> +
> +	dsto = dst_t->ops;
> +	srco = src_t->ops;
> +
> +	__p4tc_type_host_read(srco, src_t, src_mask_shift, srcv,
> +			      &readval);
> +	__p4tc_type_host_write(dsto, dst_t, dst_mask_shift, &readval,
> +			       dstv);
> +
> +	return 0;

The return value on these (write) functions seems to be inconsistent.
All the write functions are returning 0. Then, __p4tc_type_host_write
itself propagates the return value, but then here it doesn't.

> +}
> +
> +int p4t_cmp(struct p4tc_type_mask_shift *dst_mask_shift,
> +	    struct p4tc_type *dst_t, void *dstv,
> +	    struct p4tc_type_mask_shift *src_mask_shift,
> +	    struct p4tc_type *src_t, void *srcv)
> +{
> +	u64 a[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> +	u64 b[BITS_TO_U64(P4TC_MAX_KEYSZ)] = {0};
> +	const struct p4tc_type_ops *srco, *dsto;
> +
> +	dsto = dst_t->ops;
> +	srco = src_t->ops;
> +
> +	__p4tc_type_host_read(dsto, dst_t, dst_mask_shift, dstv, a);
> +	__p4tc_type_host_read(srco, src_t, src_mask_shift, srcv, b);
> +
> +	return memcmp(a, b, sizeof(a));
> +}
> +
> +void p4t_release(struct p4tc_type_mask_shift *mask_shift)
> +{
> +	kfree(mask_shift->mask);
> +	kfree(mask_shift);
> +}
> +
> +static int p4t_validate_bitpos(u16 bitstart, u16 bitend, u16 maxbitstart,
> +			       u16 maxbitend, struct netlink_ext_ack *extack)
> +{
> +	if (bitstart > maxbitstart) {
> +		NL_SET_ERR_MSG_MOD(extack, "bitstart too high");
> +		return -EINVAL;
> +	}
> +	if (bitend > maxbitend) {
> +		NL_SET_ERR_MSG_MOD(extack, "bitend too high");
> +		return -EINVAL;
> +	}

Do we want a condition for
 +	if (bitstart > bitend) {
 +		NL_SET_ERR_MSG_MOD(extack, "bitstart after bitend");
 +		return -EINVAL;
 +	}
?

> +
> +	return 0;
> +}
> +
> +//XXX: Latter immedv will be 64 bits
> +static int p4t_u32_validate(struct p4tc_type *container, void *value,
> +			    u16 bitstart, u16 bitend,
> +			    struct netlink_ext_ack *extack)
> +{
> +	u32 container_maxsz = U32_MAX;
> +	u32 *val = value;
> +	size_t maxval;
> +	int ret;
> +
> +	ret = p4t_validate_bitpos(bitstart, bitend, 31, 31, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	maxval = GENMASK(bitend, 0);
> +	if (val && (*val > container_maxsz || *val > maxval)) {
> +		NL_SET_ERR_MSG_MOD(extack, "U32 value out of range");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct p4tc_type_mask_shift *
> +p4t_u32_bitops(u16 bitsiz, u16 bitstart, u16 bitend,
> +	       struct netlink_ext_ack *extack)
> +{
> +	u32 mask = GENMASK(bitend, bitstart);
> +	struct p4tc_type_mask_shift *mask_shift;
> +	u32 *cmask;
> +
> +	mask_shift = kzalloc(sizeof(*mask_shift), GFP_KERNEL);
> +	if (!mask_shift)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cmask = kzalloc(sizeof(u32), GFP_KERNEL);
> +	if (!cmask) {
> +		kfree(mask_shift);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	*cmask = mask;
> +
> +	mask_shift->mask = cmask;
> +	mask_shift->shift = bitstart;

AFAICT, mask_shift->mask is never shared. So maybe consider
embedding mask onto mask_shift itself, to avoid the double allocation.
I mean, something like
+	mask_shift = kzalloc(sizeof(*mask_shift)+sizeof(u32), GFP_KERNEL);
+	cmask = mask_shift+1;

This may also help with cache miss later on.

> +
> +	return mask_shift;
> +}