Re: [PATCH v2 net-next 01/12] net/sched: taprio: allow user input of per-tc max SDU

On Mon, Sep 26, 2022 at 01:38:29PM -0700, Jakub Kicinski wrote:

On Fri, 23 Sep 2022 19:32:59 +0300 Vladimir Oltean wrote:

quoted

+	if (!tb[TCA_TAPRIO_TC_ENTRY_INDEX]) {
+		NL_SET_ERR_MSG_MOD(extack, "TC entry index missing");

NL_SET_ERR_ATTR_MISS() ?

quoted

+		return -EINVAL;
+	}
+
+	tc = nla_get_u32(tb[TCA_TAPRIO_TC_ENTRY_INDEX]);
+	if (tc >= TC_QOPT_MAX_QUEUE) {
+		NL_SET_ERR_MSG_MOD(extack, "TC entry index out of range");

NLA_POLICY_MAX()

Are you not using those on purpose? :(

I don't exactly see it as being super user friendly to leave it to the
policy validator (or to use NL_SET_ERR_ATTR_MISS()) because all that
will be reported back to user space will be the offset to the original
attribute in the nlmsghdr, which is pretty hard to retrieve and
re-interpret (at least in the iproute2 tc source code, I can't seem to
find a way to stringify it or something like that). For the NLA_POLICY_MAX(),
all I'll get now is an uninformative "Error: integer out of range."
What integer?  What range?

I don't understand what is the gain of removing extack message strings
and just pointing to the netlink attribute via NLMSGERR_ATTR_OFFS? Could
I at least use the NL_SET_ERR_ATTR_MISS() helper *and* set a custom message?
That's for the missing nlattr. Regarding the range checking in the
policy, I'd like a custom message there as well, but the NLA_POLICY_MAX()
doesn't provide one. However, I see that struct nla_policy has a const
char *reject_message for NLA_REJECT types. Would it be an abuse to move
this outside of the union and allow U32 policies and such to also
provide it?