IPROUTE: correct nla nested message generated by netem_parse_opt

From: Alexander Duyck <[email protected]>

netem_parse_opt was generating a malformed nested compat message. This patch corrects it so that the nested arguments are contained within a nested nla header.

Signed-off-by Alexander Duyck <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

---

 tc/q_netem.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

tc/q_netem.c +2 -5

index d06932e..a3365c1 100644
@@ -128,7 +128,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 			   struct nlmsghdr *n)
 {
 	size_t dist_size = 0;
-	struct rtattr *tail;
+	struct rtattr *nest;
 	struct tc_netem_qopt opt;
 	struct tc_netem_corr cor;
 	struct tc_netem_reorder reorder;

@@ -257,8 +257,6 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 		argc--; argv++;
 	}
 
-	tail = NLMSG_TAIL(n);
-
 	if (reorder.probability) {
 		if (opt.latency == 0) {
 			fprintf(stderr, "reordering not possible without specifying some delay\n");

@@ -277,8 +275,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 		return -1;
 	}
 
-	if (addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt)) < 0)
-		return -1;
+	nest = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
 
 	if (present[TCA_NETEM_CORR] &&
 	    addattr_l(n, 1024, TCA_NETEM_CORR, &cor, sizeof(cor)) < 0)

Quoted 43 lines · On Thu, 21 Aug 2008 17:50:11 -0700 Jeff Kirsher <[email protected]>…

From: Alexander Duyck <[email protected]>

netem_parse_opt was generating a malformed nested compat message. This patch corrects it so that the nested arguments are contained within a nested nla header.

Signed-off-by Alexander Duyck <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

---

 tc/q_netem.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

tc/q_netem.c +2 -5

index d06932e..a3365c1 100644
@@ -128,7 +128,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                         struct nlmsghdr *n)
 {
      size_t dist_size = 0;
-     struct rtattr *tail;
+     struct rtattr *nest;
      struct tc_netem_qopt opt;
      struct tc_netem_corr cor;
      struct tc_netem_reorder reorder;

@@ -257,8 +257,6 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
              argc--; argv++;
      }
 
-     tail = NLMSG_TAIL(n);
-
      if (reorder.probability) {
              if (opt.latency == 0) {
                      fprintf(stderr, "reordering not possible without specifying some delay\n");

@@ -277,8 +275,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
              return -1;
      }
 
-     if (addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt)) < 0)
-             return -1;
+     nest = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
 

tc/q_netem.c static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv, · line 302

Quoted 12 lines

tc/q_netem.c +1 -1

      if (present[TCA_NETEM_CORR] &&
          addattr_l(n, 1024, TCA_NETEM_CORR, &cor, sizeof(cor)) < 0)

@@ -299,7 +296,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                      return -1;
              free(dist_data);
      }
-     tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
+     addattr_nest_compat_end(n, nest);
      return 0;
 }
 

The kernel ABI can not change? The nested attribute order should not change.

Quoted 46 lines · Stephen Hemminger

Quoted 43 lines · On Thu, 21 Aug 2008 17:50:11 -0700 Jeff Kirsher <[email protected]>…

From: Alexander Duyck <[email protected]>

netem_parse_opt was generating a malformed nested compat message. This patch corrects it so that the nested arguments are contained within a nested nla header.

Signed-off-by Alexander Duyck <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

---

 tc/q_netem.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

tc/q_netem.c +2 -5

index d06932e..a3365c1 100644
@@ -128,7 +128,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                         struct nlmsghdr *n)
 {
      size_t dist_size = 0;
-     struct rtattr *tail;
+     struct rtattr *nest;
      struct tc_netem_qopt opt;
      struct tc_netem_corr cor;
      struct tc_netem_reorder reorder;

@@ -257,8 +257,6 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
              argc--; argv++;
      }
 
-     tail = NLMSG_TAIL(n);
-
      if (reorder.probability) {
              if (opt.latency == 0) {
                      fprintf(stderr, "reordering not possible without specifying some delay\n");

@@ -277,8 +275,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
              return -1;
      }
 
-     if (addattr_l(n, 1024, TCA_OPTIONS, &opt, sizeof(opt)) < 0)
-             return -1;
+     nest = addattr_nest_compat(n, 1024, TCA_OPTIONS, &opt, sizeof(opt));
 

Quoted 14 lines

tc/q_netem.c static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv, · line 302

Quoted 12 lines

tc/q_netem.c +1 -1

      if (present[TCA_NETEM_CORR] &&
          addattr_l(n, 1024, TCA_NETEM_CORR, &cor, sizeof(cor)) < 0)

@@ -299,7 +296,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
                      return -1;
              free(dist_data);
      }
-     tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
+     addattr_nest_compat_end(n, nest);
      return 0;
 }
 

The kernel ABI can not change? The nested attribute order should not change.

The problem is the current kernel ABI was changed in commit b9a2f2e450b0f770bb4347ae8d48eb2dea701e24 "netlink: Fix nla_parse_nested_compat() to call nla_parse() directly" to support a format that I have only seen generated in netem_parse_opt. The kernel and prio_parse_opt qdisc both generate the other format which includes the extra attribute header to contain the nested attributes. This patch fixes tc so that netem will use the correct nested attribute order for kernels prior to this commit and once this commit has been reverted.

The problem is the current kernel ABI was changed in commit b9a2f2e450b0f770bb4347ae8d48eb2dea701e24 "netlink: Fix nla_parse_nested_compat() to call nla_parse() directly" to support a format that I have only seen generated in netem_parse_opt. The kernel and prio_parse_opt qdisc both generate the other format which includes the extra attribute header to contain the nested attributes. This patch fixes tc so that netem will use the correct nested attribute order for kernels prior to this commit and once this commit has been reverted.

How was it ever supposed to work? The code looked like the following prior to commit b03f4672007e533c8dbf0965f995182586216bf1 which made it used nla_parse_nested_compat()

(quoted diff) +0 -2

-       /* Handle nested options after initial queue options.
-        * Should have put all options in nested format but

too late now.

(quoted diff) +0 -6

-        */
-       if (nla_len(opt) > sizeof(*qopt)) {
-               struct nlattr *tb[TCA_NETEM_MAX + 1];
-               if (nla_parse(tb, TCA_NETEM_MAX,
-                             nla_data(opt) + sizeof(*qopt),
-                             nla_len(opt) - sizeof(*qopt), NULL))

nla_parse_nested_compat() now does exactly what the above code does. So in what way has the kernel ABI changed?

There is two ways of sending a fixed struct + attributes inside an attribute:

a) (old and outdated method)
attr foo
fixed struct
[nested attr 1]
[nested attr 2]

This format can be parsed with nla_parse_nested_compat(attr foo)

b) (new method)
attr foo
fixed struct
attr container
[nested attr 1]
[nested attr 2]

This format is parsed with nla_parse_nested(attr container)

Quoted 14 lines

How was it ever supposed to work? The code looked like the following prior to commit b03f4672007e533c8dbf0965f995182586216bf1 which made it used nla_parse_nested_compat()

(quoted diff) +0 -2

-       /* Handle nested options after initial queue options.
-        * Should have put all options in nested format but

too late now.

(quoted diff) +0 -6

-        */
-       if (nla_len(opt) > sizeof(*qopt)) {
-               struct nlattr *tb[TCA_NETEM_MAX + 1];
-               if (nla_parse(tb, TCA_NETEM_MAX,
-                             nla_data(opt) + sizeof(*qopt),
-                             nla_len(opt) - sizeof(*qopt), NULL))

Quoted 2 lines

nla_parse_nested_compat() now does exactly what the above code does. So in what way has the kernel ABI changed?

This is exactly what I am talking about. Someone assumed it was just manually coding the nested compat format and it wasn't. It was using its own custom message format. The change you made changed the kernel ABI to support that custom format instead fixing the message sent to fit the kernel ABI format.

Please learn to use you enter key. Thank you.

* Duyck, Alexander H <[email protected]> 2008-08-27 09:30

Quoted 14 lines

Quoted 14 lines

How was it ever supposed to work? The code looked like the following prior to commit b03f4672007e533c8dbf0965f995182586216bf1 which made it used nla_parse_nested_compat()

(quoted diff) +0 -2

-       /* Handle nested options after initial queue options.
-        * Should have put all options in nested format but

too late now.

(quoted diff) +0 -6

-        */
-       if (nla_len(opt) > sizeof(*qopt)) {
-               struct nlattr *tb[TCA_NETEM_MAX + 1];
-               if (nla_parse(tb, TCA_NETEM_MAX,
-                             nla_data(opt) + sizeof(*qopt),
-                             nla_len(opt) - sizeof(*qopt), NULL))

Quoted 4 lines

Quoted 2 lines

nla_parse_nested_compat() now does exactly what the above code does. So in what way has the kernel ABI changed?

This is exactly what I am talking about. Someone assumed it was just manually coding the nested compat format and it wasn't. It was using its own custom message format. The change you made changed the kernel ABI to support that custom format instead fixing the message sent to fit the kernel ABI format.

For the last time, the message format *CANNOT* be changed, the ABI has to be stable. I didn't change the ABI, I _restored the ABI to the original state after it was broken by the initial nla_parse_nested_compat() implementation which contained a bug.

It's actually trivial to figure this out in 5 minutes using git-whatchanged, not sure why you didn't do it.

1092cb219774a82b1f16781aec7b8d4ec727c981
[NETLINK]: attr: add nested compat attribute type

Introduced nla_parse_nested_compat() for use in netem, unfortunately it contained a bug which made it behave incorrectly and broke netem.

b9a2f2e450b0f770bb4347ae8d48eb2dea701e24
netlink: Fix nla_parse_nested_compat() to call nla_parse() directly

This fixed nla_parse_nested_compat() in the way it was supposed to be from the beginning. It restored the original ABI.

Unfortunately, commit 1e90474c377e92db7262a8968a45c1dd980ca9e5 made prio use nla_parse_nested_compat() which it shouldn't have as it does not use the same format. It worked due to the bug in nla_parse_nested_compat() and then broke when nla_parse_nested_compat() was fixed. Since prio was removed, this is no longer a problem.

Ever since, netem is the only user of nla_parse_nested_compat() so I have no idea what you mean when you say I broke it for everybody else.

To put it very very simple, users of the message format as found in netem is supposed to use nla_parse_nested_compat(), everybody else is supposed to be using nla_parse_nested().

I won't bother to comment on the rest, since it shoudl be answered with this or simply didn't make any sense.

There is two ways of sending a fixed struct + attributes inside an attribute:

a) (old and outdated method)
attr foo
fixed struct
[nested attr 1]
[nested attr 2]

This format can be parsed with nla_parse_nested_compat(attr foo)

b) (new method)
attr foo
fixed struct
attr container
[nested attr 1]
[nested attr 2]

This format is parsed with nla_parse_nested(attr container)

It doesn't matter what "correct" compat nested attribute parsing is or is not. The fact is that RR and PRIO were using a certain format, consistently, prior to commit #6. This is what was codified in userspace in the iproute2 sources and worked correctly until #6.

Fixing prio and rr in stable is a one liner.

nla_parse_nested_compat() was used to parse two different message formats in the netem and prio qdisc, when it was "fixed" to work with netem, it broke the multi queue support in the prio qdisc. Since the prio qdisc code in question is already removed in the development tree, this patch only fixes the regression in the stable tree.

Based on original patch from Alexander H Duyck <[email protected]>

Signed-off-by Thomas Graf <[email protected]>