Toke Høiland-Jørgensen <[email protected]> writes:
> Dave Taht <[email protected]> writes:
>
>> On Mon, Jul 2, 2018 at 12:23 PM Toke Høiland-Jørgensen <[email protected]> wrote:
>>>
>>> Kevin Darbyshire-Bryant <[email protected]> writes:
>>>
>>> >> On 2 Jul 2018, at 19:39, Dave Taht <[email protected]> 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;
}
_______________________________________________
Cake mailing list
[email protected]
https://lists.bufferbloat.net/listinfo/cake
