Date: Wed, 27 Aug 2008 23:47:31 -0700 (PDT)
Message-Id: <20080827.234731.77434405.davem@davemloft.net>
To: tgraf@suug.ch
Cc: alexander.duyck@gmail.com, shemminger@vyatta.com,
       jeffrey.t.kirsher@intel.com, netdev@vger.kernel.org,
       linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
       alexander.h.duyck@intel.com
Subject: Re: [PATCH] IPROUTE: correct nla nested message generated by
 netem_parse_opt
From: David Miller <davem@davemloft.net>
In-Reply-To: <20080827144122.GT20815@postel.suug.ch>
References: <20080821211941.66090862@speedy>
	<5f2db9d90808211937o1d9b33t9ad933938872e08d@mail.gmail.com>
	<20080827144122.GT20815@postel.suug.ch>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4050
Lines: 124

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 27 Aug 2008 16:41:22 +0200

> 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)

Correct, but here is the sequence of events I discovered after
researching this fully:

1) Patrick adds nla_next_compat*() and NLA_NESTED_COMPAT, in this
   changeset:

	commit 1092cb219774a82b1f16781aec7b8d4ec727c981
	Author: Patrick McHardy <kaber@trash.net>
	Date:   Mon Jun 25 13:49:35 2007 -0700

	    [NETLINK]: attr: add nested compat attribute type

2) The multiqueue packet scheduler bits got added by Peter W. in
   this changeset:

	commit d62733c8e437fdb58325617c4b3331769ba82d70
	Author: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
	Date:   Thu Jun 28 21:04:31 2007 -0700

	    [SCHED]: Qdisc changes and sch_rr added for multiqueue

   The thing to note is that at this point this code is using
   rtattr_parse_nested_compat(), RTA_NEST_COMPAT*(), etc.

   This went into 2.6.23

3) iproute2 gets sch_rr support from Peter W. on August 14th, from
   this iproute2 commit:

	commit 292ce96bca64dee087fe00d38743f5e2d1895c5d
	Author: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com>
	Date:   Tue Aug 14 11:21:24 2007 -0700

	    iproute2: sch_rr support in tc

4) On January 22nd, 2008, Patrick converted all of the packet schedulers
   to nla_*(), nlattr, etc.

	commit 1e90474c377e92db7262a8968a45c1dd980ca9e5
	Author: Patrick McHardy <kaber@trash.net>
	Date:   Tue Jan 22 22:11:17 2008 -0800

	    [NET_SCHED]: Convert packet schedulers from rtnetlink to new netlink API

   At this point, even though PRIO and RR were converted as well, they
   were still working properly with iproute2 as-is.

   This went into 2.6.25

5) One day later Patrick converted netem to nla_parse_nested_compat:

	commit b03f4672007e533c8dbf0965f995182586216bf1
	Author: Patrick McHardy <kaber@trash.net>
	Date:   Wed Jan 23 20:32:21 2008 -0800

	    [NET_SCHED]: sch_netem: use nla_parse_nested_compat

   This broke netem.

   This also went into 2.6.25

6) And then on May 22nd, 2008 we get the changeset that has obtained
   a lot of discussion:

	commit b9a2f2e450b0f770bb4347ae8d48eb2dea701e24
	Author: Thomas Graf <tgraf@suug.ch>
	Date:   Thu May 22 10:48:59 2008 -0700

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

   This fixed netem but broke prio and rr qdiscs.

   This went into 2.6.26 and also 2.6.25.7

And as a result 2.6.26 was released with broken netlink parsing
for prio and rr qdiscs, as was 2.6.25.7 and later 2.6.25 releases.

Base 2.6.25 was fine, because it was after Patrick's conversion of
prio and rr to nla_parse_nested_compat(), but before Thomas's netem
fix in #6

So all the trouble started with Patrick's netem conversion in
commit #5 above.  It was not an equivalent transformation.  But
in fixing netem in #6 we broke prio and rr, which both expected
the previous behavior of nla_parse_nested_compat().

Looking at this situation, I have to say that Alexander has been
correct from the beginning, and I think we should revert both
#5 and #6 to fix this bug in all cases where they appear, and
that would be: mainline, 2.6.26-stable 2.6.25-stable

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/