netdev - Re: [p4tc-discussions] 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: Mon, 5 Jun 2023 10:26:35 -0400
From: Jamal Hadi Salim <hadi@...atatu.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org, deb.chatterjee@...el.com, 
	tom@...anda.io, p4tc-discussions@...devconf.info, Mahesh.Shirshyad@....com, 
	Vipin.Jain@....com, tomasz.osinski@...el.com, xiyou.wangcong@...il.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	khalidm@...dia.com, toke@...hat.com
Subject: Re: [p4tc-discussions] Re: [PATCH RFC v2 net-next 09/28] p4tc: add P4
 data types

On Mon, Jun 5, 2023 at 6:08 AM Simon Horman via p4tc-discussions
<p4tc-discussions@...devconf.info> wrote:
>
> 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

Ok, we will add this tool to our CICD.

> > +
> > +     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.

The cast is wrong, thanks for the catch Simon - we'll fix in the next
version. We do run sparse - we'll double check how we missed this one.

cheers,
jamal

> > +
> > +     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;
> > +}
>
> ...
> _______________________________________________
> p4tc-discussions mailing list -- p4tc-discussions@...devconf.info
> To unsubscribe send an email to p4tc-discussions-leave@...devconf.info