On Tue, 2020-10-06 at 09:16 -0700, Jakub Kicinski wrote: > On Tue, 06 Oct 2020 17:10:44 +0200 Johannes Berg wrote: > > Sorry, hat to run out earlier and forgot to comment here. > > > > On Tue, 2020-10-06 at 14:32 +0200, Johannes Berg wrote: > > > + /* the max policy content is currently ~44 bytes for range min/max */ > > > + if (err && nlk_has_extack && extack && extack->policy) > > > + tlvlen += 64; > > > > So I'm not really happy with this. I counted 44 bytes content (so 48 > > bytes for the nested attribute) for the biggest that we have now, but if > > we ever implement e.g. dumping out the reject string for NLA_REJECT > > (though not sure anyone even uses that?) then it'd be more variable. > > I wonder if we should in fact dump the reject string, in this case it > feels like an omission not to have it... although as you say, grep for > reject_message reveals it's completely unused today.
Yeah. I'm not even sure why I allowed for it. I mean, I added NLA_REJECT so you could explicitly reject old stuff when you don't have strict policy enforcement yet (where NLA_UNSPEC is basically the same), but then never used the string ... *shrug* Perhaps if somebody wants it they can add it? > > I couldn't really come up with any better idea, but I believe we do need > > to size the skb fairly well to return the original one ... > > > > The only solution I _could_ think of was to allocate another skb, put > > the attribute into it, check the length, and then later append it to the > > message ... but that seemed kinda ugly. > > > > Any preferences? > > It'd feel pretty idiomatic for (rt)netlink to have > > netlink_policy_dump_attr_size() > > which would calculate the size. That'd cost us probably ~100 LoC? >From an API POV I'd agree, but it's ~100 LOC that's effectively duplicated, and we'd have to maintain both. And if we add something (like you added the mask) we'd have to add it again there in the size calculation, and somehow maintain that the two are always in sync. Feels like a lot of pain for no practical gain? > If that's too much we could at least add a define for this constant, > and WARN_ON_ONCE() in __netlink_policy_dump_write_attr() if the dump > ends up larger? I didn't feel very comfortable putting a WARN_ON there that effectively ends up being user-triggerable at will when we made a mistake somewhere, but will basically never happen on our (developers') machines ... But hmm, yeah, if we put it into __netlink_policy_dump_write_attr() rather than netlink_ack() which I had considered, at least there's a chance we'd exercise the code path in "good" cases an hit the WARN_ON, and, more importantly, developers will hopefully at least once test their code and hit it. So yeah, I think I'll do that. The other option would be to implement *both* (in a way), and check that the size returned by netlink_policy_dump_attr_size() matched the actual size (or at least was bigger), but I still don't really know if I want to have the duplication? johannes
