Re: [PATCH] netlink: add policy attribute range validation

[Michal Kubecek <mkubecek@suse.cz>]

On Thu, Sep 27, 2018 at 10:12:09AM +0200, Johannes Berg wrote:
> On Thu, 2018-09-27 at 09:16 +0200, Michal Kubecek wrote:
> 
> > The overloading still feels a bit complicated. Perhaps we could rather
> > use validation_data in the natural way, i.e. as a pointer to validation
> > data. That would be a struct (maybe array) of two values of the
> > corresponding type. It would mean a bit more data and a bit more writing
> > but it would be IMHO more straightforward.
> 
> I considered that, but I didn't really like it either. The memory
> wasting isn't *that* bad (even if we go to s64 that'd only be ~20x16
> bytes for nl80211, eating up 320 out of the 550 saved, but still); I'm
> more worried about making this really hard to actually *do*.
> 
> Consider
> 
> policy[] = {
> 	...
> 	[NL80211_ATTR_WIPHY_RETRY_SHORT] =
> 		NLA_POLICY_RANGE(NLA_U8, 1, 255),
> 	...
> };
> 
> vs.
> 
> static const struct netlink_policy_range retry_range = {
> 	.min = 1,
> 	.max = 255,
> };

We could still use helper macros so this part could become

DEFINE_NLA_U8_RANGE(retry_range, 1, 255);

or

DEFINE_NLA_RANGE(retry_range, u8, 1, 255);

> 
> policy[] = {
> 	...
> 	[NL80211_ATTR_WIPHY_RETRY_SHORT] = {
> 		.type = NLA_U8,
> 		.validation_data = &retry_range,
> 	},
> 	...
> };

And this could be also shortened using a macro.

It would still be longer but not that much.

> That's significantly more to type, to the point where I'd seriously
> consider doing this only for attributes that are used and checked in
> many places - it doesn't feel like a big win over manual range-checking.
> 
> But I want it to be a win over manual range-checking so it gets used
> more because it's more efficient, less prone to getting messed up if
> multiple places use the same attribute and validates attributes even if
> they're ignored by an operation.
> 
> 
> I'd also say that we're certainly no strangers to union/overloading, so
> I don't feel like this is a big argument. One doesn't even really have
> to be *aware* of it for the most part: if it were a struct instead of a
> union, it'd actually have the same effect since the .type field
> indicates which part gets used. That it's overloaded in a union is
> basically just a space saving measure, I don't think it makes the
> reasoning much more complex?

I didn't mean it as a serious objection, rather a note that the gain may
not be worth the additional complexity. But if you want to follow in the
direction you indicated later (in particular, allowing different
interpretations of validation_data for the same type), overloading does
indeed make more sense.

Michal Kubecek