On Mon, 05 Oct 2020 21:05:23 +0200 Johannes Berg wrote:
> On Mon, 2020-10-05 at 08:57 -0700, Jakub Kicinski wrote:
> > We don't have good validation policy for existing unsigned int attrs
> > which serve as flags (for new ones we could use NLA_BITFIELD32).
> > With increased use of policy dumping having the validation be
> > expressed as part of the policy is important. Add validation
> > policy in form of a mask of supported/valid bits.
>
> Nice, I'll surely use this as well somewhere :)
>
> > #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
> > +#define NLA_ENSURE_UINT_TYPE(tp) \
> > + (__NLA_ENSURE(tp == NLA_U8 || tp == NLA_U16 || \
> > + tp == NLA_U32 || tp == NLA_U64) + tp)
> > #define NLA_ENSURE_UINT_OR_BINARY_TYPE(tp) \
>
> nit: maybe express this (_OR_BINARY_TYPE) in terms of UINT_TYPE() ||
> tp==NLA_BINARY? Doesn't matter much though.
Will do!
> > +static int nla_validate_mask(const struct nla_policy *pt,
> > + const struct nlattr *nla,
> > + struct netlink_ext_ack *extack)
> > +{
> > + u64 value;
> > +
> > + switch (pt->type) {
> > + case NLA_U8:
> > + value = nla_get_u8(nla);
> > + break;
> > + case NLA_U16:
> > + value = nla_get_u16(nla);
> > + break;
> > + case NLA_U32:
> > + value = nla_get_u32(nla);
> > + break;
> > + case NLA_U64:
> > + value = nla_get_u64(nla);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (value & ~(u64)pt->mask) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla, "reserved bit set");
> > + return -EINVAL;
>
> You had an export of the valid bits there in ethtool, using the cookie.
> Just pointing out you lost it now. I'm not sure I like using the cookie,
> that seems a bit strange, but we could easily define a different attr?
>
> OTOH, one can always query the policy export too (which hopefully got
> wired up) so it wouldn't really matter much.
My thinking is that there are no known uses of the cookie, it'd only
have practical use to test for new flags - and we're adding first new
flag in 5.10.
> Either way is fine with me on both of these points.
>
> Reviewed-by: Johannes Berg <johan...@sipsolutions.net>
Thanks!
