Re: [PATCH iproute2-next 3/3] treewide: Use addattr_nest()/addattr_nest_end() to handle nested attributes

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 13 Apr 2018 15:57:37 -0700
Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:

> Hi,
> 
> Serhey Popovych <serhe.popovych@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.