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.
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
> + 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
> + 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 (???)
[...]
> +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
?
Otherwise LGTM!
Cheers,
Paolo
