On Tue, Mar 26, 2024 at 10:48 AM Paolo Abeni <pabeni@xxxxxxxxxx> 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
>
