netdev - Re: [PATCH net-next v13 06/15] 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: <CAM0EoMkaa_u+Dd3OTyQDj=7W6KBSyFGf1emHp9VgCOdRRANpUw@mail.gmail.com>
Date: Tue, 26 Mar 2024 19:13:03 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, deb.chatterjee@...el.com, anjali.singhai@...el.com, 
	namrata.limaye@...el.com, tom@...anda.io, 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, vladbu@...dia.com, horms@...nel.org, 
	khalidm@...dia.com, toke@...hat.com, daniel@...earbox.net, 
	victor@...atatu.com, pctammela@...atatu.com, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v13 06/15] p4tc: add P4 data types

On Tue, Mar 26, 2024 at 10:48 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Mon, 2024-03-25 at 10:28 -0400, Jamal Hadi Salim wrote:
> > +static int p4t_s32_validate(struct p4tc_type *container, void *value,
> > +                         u16 bitstart, u16 bitend,
> > +                         struct netlink_ext_ack *extack)
> > +{
> > +     s32 minsz = S32_MIN, maxsz = S32_MAX;
> > +     s32 *val = value;
> > +
> > +     if (val && (*val > maxsz || *val < minsz)) {
> > +             NL_SET_ERR_MSG_MOD(extack, "S32 value out of range");
>
> I'm sorry for the additional questions/points below which could/should
> have been flagged earlier.
>
> Out of sheer ignorance IDK if a single P4 command could use multiple
> types, possibly use NL_SET_ERR_MSG_FMT_MOD() and specify the bogus
> value/range.
>

yes, that would be a more useful message. Will change.

> The same point/question has a few other instances below.
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
>
> [...]
>
> > +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;
> > +     __be32 *val_u32 = value;
> > +     __u32 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_to_cpu(*val_u32);
> > +
> > +     maxval = GENMASK(bitend, 0);
> > +     if (val && (val > container_maxsz || val > maxval)) {
>
> The first condition 'val > container_maxsz' is a bit confusing
> (unneeded), as 'val' type is u32 and 'container_maxsz' is U32_MAX
>

Good point..

> > +             NL_SET_ERR_MSG_MOD(extack, "BE32 value out of range");
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
>
> [...]
>
> > +static int p4t_u16_validate(struct p4tc_type *container, void *value,
> > +                         u16 bitstart, u16 bitend,
> > +                         struct netlink_ext_ack *extack)
> > +{
> > +     u16 container_maxsz = U16_MAX;
> > +     u16 *val = value;
> > +     u16 maxval;
> > +     int ret;
> > +
> > +     ret = p4t_validate_bitpos(bitstart, bitend, 15, 15, extack);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     maxval = GENMASK(bitend, 0);
> > +     if (val && (*val > container_maxsz || *val > maxval)) {
>
> Mutatis mutandis, same thing here
>

noted ;->

> > +             NL_SET_ERR_MSG_MOD(extack, "U16 value out of range");
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct p4tc_type_mask_shift *
> > +p4t_u16_bitops(u16 bitsiz, u16 bitstart, u16 bitend,
> > +            struct netlink_ext_ack *extack)
> > +{
> > +     struct p4tc_type_mask_shift *mask_shift;
> > +     u16 mask = GENMASK(bitend, bitstart);
> > +     u16 *cmask;
> > +
> > +     mask_shift = kzalloc(sizeof(*mask_shift), GFP_KERNEL);
>
> (Not specifically related to _this_ allocation) I'm wondering if the
> allocations in this file should GFP_KERNEL_ACCOUNT? If I read correctly
> the user-space can (and is expected to) create an quite large number of
> instances of this structs (???)
>

I think changing to GFP_KERNEL_ACCOUNT would be useful regardless
especially because we are namespace aware.. Yes, there could be
millions of table entries in some cases.

> [...]
> > +void __p4tc_type_host_write(const struct p4tc_type_ops *ops,
> > +                         struct p4tc_type *container,
> > +                         struct p4tc_type_mask_shift *mask_shift, void *sval,
> > +                         void *dval)
> > +{
> > +     #define HWRITE(cops) \
> > +     do { \
> > +             if (ops == &(cops)) \
> > +                     return (cops).host_write(container, mask_shift, sval, \
> > +                                              dval); \
> > +     } while (0)
> > +
> > +     HWRITE(u8_ops);
> > +     HWRITE(u16_ops);
> > +     HWRITE(u32_ops);
> > +     HWRITE(u64_ops);
> > +     HWRITE(u128_ops);
> > +     HWRITE(s16_ops);
> > +     HWRITE(s32_ops);
> > +     HWRITE(be16_ops);
> > +     HWRITE(be32_ops);
> > +     HWRITE(mac_ops);
> > +     HWRITE(ipv4_ops);
> > +     HWRITE(bool_ops);
> > +     HWRITE(dev_ops);
> > +     HWRITE(key_ops);
>
> possibly
>
> #undef HWRITE
>

Nod.

> ?
>
> Otherwise LGTM!


Thanks for taking the time to review!

cheers,
jamal

>
> Cheers,
>
> Paolo
>