Re: [Cake] Cake on openwrt - falling behind

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Dave Taht <dave.taht@gmail.com> writes:
>
>> On Mon, Jul 2, 2018 at 12:23 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>>
>>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>>>
>>> >> On 2 Jul 2018, at 19:39, Dave Taht <dave.taht@gmail.com> wrote:
>>> >>
>>> >>>
>>> >>
>>> >> This seems like it will introduce problems with stuff that isn't or is
>>> >> legitimately broken in the first place, pointing to potentially random
>>> >> data in the wrong place.
>>> >>
>>> >> would a workaround be adding more padding to the cake stats output so
>>> >> it's always even?
>>> >>
>>> >> why does it work as written on arm?
>>> >
>>> > If I understand correctly: This will only be a problem on
>>> > architectures that require alignment of 64 bit values to 8 byte
>>> > boundaries which is achieved by padding the structure by a dummy (4
>>> > byte) value if required. So to hit this bug we need kernel symbol
>>> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS undefined *and* we need a
>>> > netlink stats structure that needs a 4 byte dummy pad value to align
>>> > to 8 bytes. Of the architectures tested, MIPS is the only one that
>>> > DOES NOT set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus may be
>>> > exposed to the bug.
>>> >
>>> > arm sets CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and thus no padding is
>>> > ever required/added, thus pointers always point to the correct data
>>> > location.
>>>
>>> Yup, exactly. This has always been broken on MIPS, I assume, but because
>>> most other qdiscs send their stats output as a serialised struct, tc
>>> just automatically falls back to the legacy data format, and no one has
>>> noticed. But because we switched to sending each stat as an individual
>>> netlink attribute (and thus no fallback legacy stats struct), we expose
>>> the bug...
>>
>> Well, if you wrap that patch in
>>
>> #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
>> bla
>> #endif
>>
>> I guess I'd sleep better but I do generally get nervous when
>> arbitrarily subtracting something
>> from a pointer
>
> Ah, right; well, that was just a proof of concept to make sure it fixed
> the issue. I'll look harder at the code to see if I can find a better
> solution before submitting it upstream (at a first glance the API
> doesn't make it easy, but I'll have another look).

Here's a safer version; anyone care to do a quick test before I submit
it? :)

-Toke



@@ -77,8 +77,20 @@ gnet_stats_start_copy_compat(struct sk_buff *skb, int type, int tc_stats_type,
 		d->lock = lock;
 		spin_lock_bh(lock);
 	}
-	if (d->tail)
-		return gnet_stats_copy(d, type, NULL, 0, padattr);
+	if (d->tail) {
+		int ret = gnet_stats_copy(d, type, NULL, 0, padattr);
+
+		/* The initial attribute added in gnet_stats_copy() may be
+		 * preceded by a padding attribute, in which case d->tail will
+		 * end up pointing at the padding instead of the real attribute.
+		 * Fix this so gnet_stats_finish_copy() adjusts the length of
+		 * the right attribute.
+		 */
+		if (ret == 0 && d->tail->nla_type == padattr)
+			d->tail = (struct nlattr *)((char *)d->tail +
+						    NLA_ALIGN(d->tail->nla_len));
+		return ret;
+	}
 
 	return 0;
 }