Re: [Cake] Cake on openwrt - falling behind

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

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

-Toke