Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Dave Jones]

[-- Attachment #1: Type: text/plain, Size: 34 bytes --]


-- 
http://www.codemonkey.org.uk

[-- Attachment #2: Type: message/rfc822, Size: 6335 bytes --]

From: bugzilla@redhat.com
To: davej@codemonkey.org.uk
Subject: [Bug 447812] New: Netlink messages from "tc"  to sch_netem module are not interpreted correctly
Date: Wed, 21 May 2008 17:38:25 -0400
Message-ID: <bug-447812-263030@bugzilla.redhat.com>

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.




https://bugzilla.redhat.com/show_bug.cgi?id=447812

           Summary: Netlink messages from "tc"  to sch_netem module are not
                    interpreted correctly
           Product: Fedora
           Version: rawhide
          Platform: x86_64
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: low
         Component: kernel
        AssignedTo: kernel-maint@redhat.com
        ReportedBy: karl@iwl.com
         QAContact: extras-qa@fedoraproject.org
                CC: nhorman@redhat.com


>From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.14) Gecko/20080416 Fedora/2.0.0.14-1.fc8 Firefox/2.0.0.14

Description of problem:
When using the tc command to send netem settings to the netem module, the messages cause a kernel error to be emitted into dmesg and part of the data in the netlink message may be lost.

This makes the netem mechanism quietly unreliable when used on a 2.6.25 kernel.

I have tried this on all of the 2.6.25.X kernels and the results are the same.

This problem did not occur with the2.6.24 kernels.

The problem is the same on x86_64 and i386 architectures.

Version-Release number of selected component (if applicable):
2.6.25.3-18.fc9.x86_64

How reproducible:
Always


Steps to Reproduce:
Here is a shell script (needs to be run as root):

#!/bin/bash

DEV=eth0
TC=/sbin/tc

# Clean out any prior settings.
# This may generate some messages of the form:
#   RTNETLINK answers: No such file or directory
${TC} qdisc del dev ${DEV} root > /dev/null 2>&1
${TC} qdisc del dev ${DEV} ingress > /dev/null 2>&1

${TC} qdisc add dev ${DEV} root handle 1: prio bands 5 priomap 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
${TC} qdisc add dev ${DEV} parent 1:1 handle 10: netem

# If the kernel is acting up this will cause a kernel
# message of the following form to be emitted and visible
# via dmesg | tail
#  netlink: 12 bytes leftover after parsing attributes.
${TC} qdisc change dev ${DEV} parent 1:1 handle 10: netem delay 50ms 5ms 10% corrupt 8%
echo
echo
echo "Does the following contain a netlink message about leftover bytes?"
echo "If so, the the kernel code in .../net/netlink/attr.c"
echo "is unhappy with the netlink messages from the tc command."
dmesg | tail -3

# Take a look at the netem status and see whether a corruption
# value has been established or not.
echo
echo
echo "Does the following show a corruption setting or not?"
echo "If not then the kernel module .../net/sched/sch_netem.c"
echo "did not pick up all the pieces from the netlink message"
echo "complained of by .../net/netlink/attr.c"
echo "A GOOD response should look like this:"
echo "    qdisc prio 1: root bands 5 priomap  4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4"
echo "    qdisc netem 10: parent 1:1 limit 1000 delay 50.0ms  5.0ms 10% corrupt 8%"

${TC} qdisc show dev ${DEV}

${TC} qdisc show dev ${DEV} | grep netem | grep -q corrupt > /dev/null
RC=$?

if [ "${RC}" != 0 ] ; then
   echo
   echo "I did not see any corruption setting, did you?"
   echo "Seems like there is a bug in the tc-to-netem module netlink."
fi

# Clean up after ourselves.
# This may generate some messages of the form:
#   RTNETLINK answers: No such file or directory
${TC} qdisc del dev ${DEV} root > /dev/null 2>&1
${TC} qdisc del dev ${DEV} ingress > /dev/null 2>&1


Actual Results:
On all 2.6.25 kernels, whether i386 or x86_64, this causes a kernel message to be emitted about unused bytes.  In addition, the netem module does not pick up all of the data that was sent to it, such as the corruption settings.

All of this stuff worked in the 2.6.24 kernels.

Expected Results:
No kernel message should have been emitted.

The data sent by the user via the "tc" command should have been received by the netem module.

Additional info:
I tried to figure out whether the problem is in "tc" or in the kernel.

So I ran some old "tc" binaries on new kernels.  The result was the same as if I had run current "tc" binaries.  This suggests that the problem is in the kernel rather than in the "tc" command.

I also did some simple printk debugging on the way that .../net/netlink/attr.c was parsing the netlink messages.  The number of bytes that it reported unused always was the sizeof the first chunk of netem data that was lost in the netlink message.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Stephen Hemminger]

Something in the new netlink parsing made netem break. One other person
saw it and was working on fixing, but no definitive answer yet.

Begin forwarded message:

Date: Thu, 08 May 2008 17:59:24 -0700
From: Karl Auerbach 
To: Stephen Hemminger <shemminger@linux-foundation.org>
Subject: Re: Some netem issues with 2.6.25 kernel



I've dug even deeper into the issue of netem on the 2.6.25.x kernels.

(And I'm also attaching a cleaner version of my patch to q_netem.c for 
iproute2.2.6.25: I pulled out the gratuitous changes and left only the 
necessary ones to fix the bug in which uninitialized data was being sent 
across the netlink API, plus one teensy one to remove some whitespace.) 
  (I believe that there are still some remaining issues in the tc 
support of netem in which some correlation values, for instance, could 
be zeroed out, but I did not go after those.)

 From what I can see the larger problem with the netlink messages is 
happening on the kernel side of the boundary.

I used several old binary images of the 'tc' command, several of which I 
built but also ones "borrowed" from Fedora 8/32-bit and tried 'em. 
Every one showed caused the 2.6.25.x kernel to emit the warnings while 
they worked fine on a 2.6.24.x kernel.

I wondered whether this might be caused by my kernel on the AMD Geode 
LX, so I hopped over to a more typical platform - I  went and built a 
64-bit version of 2.6.25.1 and slapped it onto a Fedora 8/64-bit box and 
it, using the Fedora 8 version of 'tc', also showed an error.

My typical test case is a script that clears things out and then imposes 
an impairment on the last line.  (But I get the same problem with other 
command sequences as well, but this one is nice and short.)

/sbin/tc qdisc del dev eth1 root
/sbin/tc qdisc del dev eth1 ingress
/sbin/tc qdisc add dev eth1 root handle 1: prio bands 5 priomap 4 4 4 4 
4 4 4 4 4 4 4 4 4 4 4 4
/sbin/tc qdisc add dev eth1 parent 1:1 handle 10: netem
/sbin/tc qdisc add dev eth1 parent 10:1 handle 100: tbf rate 2147483647 
burst 1600 latency 2000000 mpu 64
/sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 
5ms 10% corrupt 8%

It also generates the warning if the last line of the above is simply:

/sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 
5ms 10%

But not if that last line is (i.e. with the correlation part dropped.)

/sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 5ms

So, all in all, it seems to me that a bug has crept into the kernel 
interpretation of the netem netlink messages.

		--karl--

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Thomas Graf]

* Stephen Hemminger <shemminger@vyatta.com> 2008-05-21 15:10
> /sbin/tc qdisc del dev eth1 root
> /sbin/tc qdisc del dev eth1 ingress
> /sbin/tc qdisc add dev eth1 root handle 1: prio bands 5 priomap 4 4 4 4 
> 4 4 4 4 4 4 4 4 4 4 4 4
> /sbin/tc qdisc add dev eth1 parent 1:1 handle 10: netem
> /sbin/tc qdisc add dev eth1 parent 10:1 handle 100: tbf rate 2147483647 
> burst 1600 latency 2000000 mpu 64
> /sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 
> 5ms 10% corrupt 8%
> 
> It also generates the warning if the last line of the above is simply:
> 
> /sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 
> 5ms 10%
> 
> But not if that last line is (i.e. with the correlation part dropped.)
> 
> /sbin/tc qdisc change dev eth1 parent 1:1 handle 10: netem delay 50ms 5ms
> 
> So, all in all, it seems to me that a bug has crept into the kernel 
> interpretation of the netem netlink messages.

Could you try this patch?

Index: net-2.6/include/net/netlink.h
===================================================================
--- net-2.6.orig/include/net/netlink.h	2008-05-22 02:12:48.000000000 +0200
+++ net-2.6/include/net/netlink.h	2008-05-22 02:15:25.000000000 +0200
@@ -772,12 +772,13 @@
 					    const struct nla_policy *policy,
 					    int len)
 {
-	if (nla_len(nla) < len)
+	int nested_len = nla_len(nla) - NLA_ALIGN(len);
+
+	if (nested_len < 0)
 		return -1;
-	if (nla_len(nla) >= NLA_ALIGN(len) + sizeof(struct nlattr))
-		return nla_parse_nested(tb, maxtype,
-					nla_data(nla) + NLA_ALIGN(len),
-					policy);
+	if (nested_len >= nla_attr_size(0))
+		return nla_parse(tb, maxtype, nla_data(nla) + NLA_ALIGN(len),
+				 nested_len, policy);
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 	return 0;
 }

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Rick Jones]

> Could you try this patch?

I'm not sure what it does to the tc case, unless I botched applying the 
patch, my rtnetlink case doesn't seem to change:

izzy:~/netperf2_trunk# ./testfoo 16.89.133.212
destination address is 16.89.133.212
egress interface 0x6000000000004010 eth6
izzy:~/netperf2_trunk# ./testfoo 16.89.133.212 10.208.0.17
netlink: 8 bytes leftover after parsing attributes.
destination address is 16.89.133.212
egress interface 0x6000000000004010 eth6
izzy:~/netperf2_trunk# uname -a
Linux izzy 2.6.26-rc1-netlink #1 SMP Wed May 21 17:36:53 PDT 2008 ia64 
GNU/Linux

rick jones

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Thomas Graf]

* Rick Jones <rick.jones2@hp.com> 2008-05-21 17:57
> 
> >Could you try this patch?
> 
> I'm not sure what it does to the tc case, unless I botched applying the 
> patch, my rtnetlink case doesn't seem to change:
> 
> izzy:~/netperf2_trunk# ./testfoo 16.89.133.212
> destination address is 16.89.133.212
> egress interface 0x6000000000004010 eth6
> izzy:~/netperf2_trunk# ./testfoo 16.89.133.212 10.208.0.17
> netlink: 8 bytes leftover after parsing attributes.
> destination address is 16.89.133.212
> egress interface 0x6000000000004010 eth6
> izzy:~/netperf2_trunk# uname -a
> Linux izzy 2.6.26-rc1-netlink #1 SMP Wed May 21 17:36:53 PDT 2008 ia64 
> GNU/Linux

I looked at netrt_rtnetlink.c, the problem is unrelated. Look at
the code when adding the source attribtue:

  struct rtattr *rtap;

  if (source) {
    rtap +=  rtap->rta_len;

You want to increment rtap by rta_len bytes, not rta_len*sizeof(*rtap)
bytes.

Also, the code uses RTA_LENGTH() to calculate attributes sizes which
is wrong, it must use RTA_SPACE() as RTA_LENGTH() does not align the
actual data part and thus the next attribute is unaligned if the data
of the previous attribute is not aligned to 4 bytes.

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Rick Jones]

> I looked at netrt_rtnetlink.c, the problem is unrelated. Look at
> the code when adding the source attribtue:
> 
>   struct rtattr *rtap;
> 
>   if (source) {
>     rtap +=  rtap->rta_len;
> 
> You want to increment rtap by rta_len bytes, not rta_len*sizeof(*rtap)
> bytes.
> 
> Also, the code uses RTA_LENGTH() to calculate attributes sizes which
> is wrong, it must use RTA_SPACE() as RTA_LENGTH() does not align the
> actual data part and thus the next attribute is unaligned if the data
> of the previous attribute is not aligned to 4 bytes.

I'll make the fixes in the netperf code - thanks, hope the bugs there 
weren't too big a distraction.

rick jones

[NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly

[Thomas Graf]

The purpose of nla_parse_nested_compat() is to parse attributes
which contain a struct followed by a stream of nested attributes.
So far, it called nla_parse_nested() to parse the stream of
nested attributes which was wrong, as nla_parse_nested() expects
a container attribute as data which holds the attribute stream.
It needs to call nla_parse() directly while pointing at the
next possible alignment point after the struct in the beginning
of the attribute.

With this patch, I can no longer reproduce the reported leftover
warnings.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6/include/net/netlink.h
===================================================================
--- net-2.6.orig/include/net/netlink.h	2008-05-22 13:00:10.000000000 +0200
+++ net-2.6/include/net/netlink.h	2008-05-22 13:02:08.000000000 +0200
@@ -772,12 +772,13 @@
 					    const struct nla_policy *policy,
 					    int len)
 {
-	if (nla_len(nla) < len)
+	int nested_len = nla_len(nla) - NLA_ALIGN(len);
+
+	if (nested_len < 0)
 		return -1;
-	if (nla_len(nla) >= NLA_ALIGN(len) + sizeof(struct nlattr))
-		return nla_parse_nested(tb, maxtype,
-					nla_data(nla) + NLA_ALIGN(len),
-					policy);
+	if (nested_len >= nla_attr_size(0))
+		return nla_parse(tb, maxtype, nla_data(nla) + NLA_ALIGN(len),
+				 nested_len, policy);
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 	return 0;
 }

Re: [NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly

[Patrick McHardy]

Thomas Graf wrote:
> The purpose of nla_parse_nested_compat() is to parse attributes
> which contain a struct followed by a stream of nested attributes.
> So far, it called nla_parse_nested() to parse the stream of
> nested attributes which was wrong, as nla_parse_nested() expects
> a container attribute as data which holds the attribute stream.
> It needs to call nla_parse() directly while pointing at the
> next possible alignment point after the struct in the beginning
> of the attribute.
>
> With this patch, I can no longer reproduce the reported leftover
> warnings.
>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Looks good to me, thanks for fixing this Thomas.

Re: [NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 22 May 2008 14:45:23 +0200

> Thomas Graf wrote:
> > The purpose of nla_parse_nested_compat() is to parse attributes
> > which contain a struct followed by a stream of nested attributes.
> > So far, it called nla_parse_nested() to parse the stream of
> > nested attributes which was wrong, as nla_parse_nested() expects
> > a container attribute as data which holds the attribute stream.
> > It needs to call nla_parse() directly while pointing at the
> > next possible alignment point after the struct in the beginning
> > of the attribute.
> >
> > With this patch, I can no longer reproduce the reported leftover
> > warnings.
> >
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> Looks good to me, thanks for fixing this Thomas.

Patch applied, thanks everyone.

Re: [NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly

[Stephen Hemminger]

On Thu, 22 May 2008 10:49:13 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 22 May 2008 14:45:23 +0200
> 
> > Thomas Graf wrote:
> > > The purpose of nla_parse_nested_compat() is to parse attributes
> > > which contain a struct followed by a stream of nested attributes.
> > > So far, it called nla_parse_nested() to parse the stream of
> > > nested attributes which was wrong, as nla_parse_nested() expects
> > > a container attribute as data which holds the attribute stream.
> > > It needs to call nla_parse() directly while pointing at the
> > > next possible alignment point after the struct in the beginning
> > > of the attribute.
> > >
> > > With this patch, I can no longer reproduce the reported leftover
> > > warnings.
> > >
> > > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> > 
> > Looks good to me, thanks for fixing this Thomas.
> 
> Patch applied, thanks everyone.

Pls, push this to stable as well.

Re: [NETLINK]: Fix nla_parse_nested_compat() to call nla_parse() directly

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 22 May 2008 11:02:58 -0700

> Pls, push this to stable as well.

Will do.

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Rick Jones]

Dave Jones wrote:
> # If the kernel is acting up this will cause a kernel
> # message of the following form to be emitted and visible
> # via dmesg | tail
> #  netlink: 12 bytes leftover after parsing attributes.

Not _much_ added value, but I can add that I see a similar message - 
with 8 replacing 12 - when running netperf "omni" tests:

netlink: 8 bytes leftover after parsing attributes.
netlink: 8 bytes leftover after parsing attributes.
netlink: 8 bytes leftover after parsing attributes.
c12:~/netperf2_trunk# uname -a
Linux c12 2.6.24-1-mckinley #1 SMP Sat Apr 19 00:19:28 UTC 2008 ia64 
GNU/Linux

And netperf was compiled on that system itself so ostensibly had the 
correct headers etc...

I also see those messages on a 2.6.26-rc1 kernel:

izzy:~/netperf2_trunk# ./configure --enable-omni
...
izzy:~/netperf2_trunk# src/netperf -t omni -H c12.cup.hp.com -- -o foo
OMNI TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to c12.cup.hp.com 
(16.89.133.212) port 0 AF_INET
netlink: 8 bytes leftover after parsing attributes.
Throughput
94.02
izzy:~/netperf2_trunk# uname -a
Linux izzy 2.6.26-rc1-raj #1 SMP Thu May 8 15:24:23 PDT 2008 ia64 GNU/Linux
izzy:~/netperf2_trunk# cat foo
throughput

rick jones

Re: Fwd: [Bug 447812] New: Netlink messages from "tc" to sch_netem module are not interpreted correctly

[Rick Jones]

However, the message is not emitted if one compiles 
src/netrt_rtnetlink.c for standalone debug:

cc -otestfoo -DNETPERF_STANDALONE_DEBUG netrt_rtnetlink.c


izzy:~/netperf2_trunk# ./testfoo 16.89.133.212
address is 16.89.133.212
egress interface 0x6000000000004010 eth6


The one difference I see in that case is that the source IP is not 
passed-in, and in the full-up netperf omni case it is.  To confirm that 
I modified the -DNETPERF_STANDALONE_DEBUG part to take an optional parm 
for the source IP, and sure enough, I can get the netlink message that 
way with the standalone version:

izzy:~/netperf2_trunk# ./testfoo 16.89.133.212
destination address is 16.89.133.212
egress interface 0x6000000000004010 eth6
izzy:~/netperf2_trunk# ./testfoo 16.89.133.212 10.208.0.17
netlink: 8 bytes leftover after parsing attributes.
destination address is 16.89.133.212
egress interface 0x6000000000004010 eth6
izzy:~/netperf2_trunk# uname -a
Linux izzy 2.6.26-rc1-raj #1 SMP Thu May 8 15:24:23 PDT 2008 ia64 GNU/Linux

http://www.netperf.org/svn/netperf2/trunk/src/netrt_rtnetlink.c

fwiw,

rick jones