On Fri, 13 Apr 2018 15:57:37 -0700 Vinicius Costa Gomes <vinicius.go...@intel.com> wrote:
> Hi, > > Serhey Popovych <serhe.popov...@gmail.com> writes: > > [...] > > > diff --git a/tc/q_mqprio.c b/tc/q_mqprio.c > > index 89b4600..207d644 100644 > > --- a/tc/q_mqprio.c > > +++ b/tc/q_mqprio.c > > @@ -173,8 +173,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int > > argc, > > argc--; argv++; > > } > > > > - tail = NLMSG_TAIL(n); > > - addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt)); > > + tail = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt)); > > > > if (flags & TC_MQPRIO_F_MODE) > > addattr_l(n, 1024, TCA_MQPRIO_MODE, > > @@ -209,7 +208,7 @@ static int mqprio_parse_opt(struct qdisc_util *qu, int > > argc, > > addattr_nest_end(n, start); > > } > > > > - tail->rta_len = (void *)NLMSG_TAIL(n) - (void *)tail; > > + addattr_nest_compat_end(n, tail); > > > > return 0; > > } > > Sorry if I am too late, but this breaks mqprio, i.e. something like > this: > > $ tc qdisc replace dev enp2s0 handle 100: parent root mqprio \ > num_tc 3 map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ > queues 1@0 1@1 2@2 hw 0 > > that used to work, now doesn't. > > This patch looks right, so I thought that it could be possible that mqprio > (in the kernel side) was making some wrong assumptions about the format > of the messages. > > And after some investigation, what seems to be happening is something > like this (not too familiar with netlink protocol internals, I may be > missing something). > > In the "wire", after this patch, the mqprio part of message may be > represented as: > > /* The message format is [ len | type | payload ] */ > > [ S | 2 | <S bytes> ] > [ 0 | 2 | ] > > Some notes: > - S is the aligned value of sizeof(opt); > - The value of TCA_OPTIONS is 2; > > Before this patch, I think it was something like: > > [ S | 2 | <S bytes> ] > > The problem is that mqprio defines an internal type with the same value > as TCA_OPTIONS (2), and that finalizing (empty) is interpreted as the > "internal" field instead of indicating the end of TCA_OPTIONS, which > causes a size mismatch with 'mqprio_policy', causing the command to > create a mqprio qdisc to fail. > > In short, I think that replacing the "open coded" version with > addattr_nest_compat() is not a functionally equivalent change. > > > Cheers, > -- > Vinicius There are also a couple of legacy cases where kernel expects or sends nested netlink messages without the NLA_NESTED flag. I ran into this several years ago, forgot where.
