netdev - Re: [PATCH] IPROUTE: correct nla nested message generated by netem_parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080827204750.GV20815@postel.suug.ch>
Date:	Wed, 27 Aug 2008 22:47:50 +0200
From:	Thomas Graf <tgraf@...g.ch>
To:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
Cc:	Alexander Duyck <alexander.duyck@...il.com>,
	Stephen Hemminger <shemminger@...tta.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH] IPROUTE: correct nla nested message generated by netem_parse_opt

Please learn to use you enter key. Thank you.

* Duyck, Alexander H <alexander.h.duyck@...el.com> 2008-08-27 09:30
> >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()
> >
> >-       /* Handle nested options after initial queue options.
> >-        * Should have put all options in nested format but
> >too late now.
> >-        */
> >-       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?
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html