On Wed, 12 Jun 2019 21:18:59 +0200, Michal Kubecek wrote: > On Wed, Jun 12, 2019 at 08:56:10PM +0200, Johannes Berg wrote: > > (switching to my personal email) > > > > > > I can't add these actions with current net-next and iproute-next: > > > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000 > > > > Error: NLA_F_NESTED is missing. > > > > We have an error talking to the kernel > > > > > > > > This also happens with the current post of act_ct and should also > > > > happen with the act_mpls post (thus why Cc'ing John as well). > > > > > > > > I'm not sure how we should fix this. In theory the kernel can't get > > > > stricter with userspace here, as that breaks user applications as > > > > above, so older actions can't use the more stricter parser. Should we > > > > have some actions behaving one way, and newer ones in a different way? > > > > That seems bad. > > > > I think you could just fix all of the actions in userspace, since the > > older kernel would allow both with and without the flag, and then from a > > userspace POV it all behaves the same, just the kernel accepts some > > things without the flag for compatibility with older iproute2? > > > > > > Or maybe all actions should just use nla_parse_nested_deprecated()? > > > > I'm thinking this last. Yet, then the _deprecated suffix may not make > > > > much sense here. WDYT? > > > > > > Surely for new actions we can require strict validation, there is > > > no existing user space to speak of.. > > > > That was the original idea. > > > > > Perhaps act_ctinfo and act_ct > > > got slightly confused with the race you described, but in principle > > > there is nothing stopping new actions from implementing the user space > > > correctly, right? > > > > There's one potential thing where you have a new command in netlink > > (which thus will use strict validation), but you use existing code in > > userspace to build the netlink message or parts thereof? > > > > But then again you can just fix that while you test it, and the current > > and older kernel will accept the stricter version for the existing use > > of the existing code too, right? > > Userspace can safely set NLA_F_NESTED on every nested attribute as there > are only few places in kernel where nla->type is accessed directly > rather than through nla_type() and those are rather specific (mostly > when attribute type is actually used as an array index). So the best > course of action would be letting userspace always set NLA_F_NESTED. > So kernel can only by strict on newly added attributes but userspace can > (and should) set NLA_F_NESTED always. > > The opposite direction (kernel -> userspace) is more tricky as we can > never be sure there isn't some userspace client accessing the type directly > without masking out the flags. Thus kernel can only set NLA_F_NESTED on > new attributes where there cannot be any userspace program used to it > not being set.
Agreed, so it's just the slight inconsistency in the dumps, which I'd think is a fair price to pay here. Old user space won't recognize the new attributes, anyway, so doesn't matter what flags they have..
