From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.toke.dk (mail.toke.dk [52.28.52.200]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id BC02B3BA8E for ; Mon, 2 Jul 2018 16:05:02 -0400 (EDT) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=20161023; t=1530561900; bh=I1yXdy2nTOoQPMr/bnTooqvcni8wL/+fFcMGNPCdShg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Hp2BoQdJEqqUexrjwJduWODPaKMFagpcMif4EjkTTNS5u/Dx7+SzVJmHkgu5OC0JK tpaSrGebiVEuO2Z0gowsgW1wk5q+X+x6idOX7wptyrVnNy31EIDDIMfwLnfdknUjHH tTJrMgbBUE3W0ynpflvjbktuqJ4+KX30L0/uTHFsrYoUGGPzbs9HKs/dWWgG6oIoqp boB/nZNXUTBpf8InP1qSExpP34vSK6spS9Yk5dKPm9RT67H0Wt/hAP6s3rB22FIxJQ mefA2Z+82pIhDjyLUBE5qd+llLVFDWs2/Fn+DeB3BzyGQN/KZFqyfkE+8mkQOxEziK CMd15RZckaDKw== To: Dave Taht Cc: Kevin Darbyshire-Bryant , Cake List In-Reply-To: <87tvphh1hu.fsf@toke.dk> References: <1A8BA286-6B31-4581-86C9-6855AC28C245@heistp.net> <673EAD3F-AB09-4B90-88BB-5DCE0BD65534@heistp.net> <6FE8D434-01BE-41A1-BD6B-EFFD67AC8784@heistp.net> <94C9790F-E9BC-4D59-9845-17C305E4B910@darbyshire-bryant.me.uk> <17AF79A0-0213-44E3-95B9-62795A644A47@heistp.net> <87lgatj13k.fsf@toke.dk> <87fu11ipir.fsf@toke.dk> <9F0353E5-D191-492A-9413-AA6BD03DD4C6@darbyshire-bryant.me.uk> <877emdigqe.fsf@toke.dk> <87tvphh1hu.fsf@toke.dk> Date: Mon, 02 Jul 2018 22:05:11 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87o9fph094.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] Cake on openwrt - falling behind X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jul 2018 20:05:03 -0000 Toke H=C3=B8iland-J=C3=B8rgensen writes: > Dave Taht writes: > >> On Mon, Jul 2, 2018 at 12:23 PM Toke H=C3=B8iland-J=C3=B8rgensen wrote: >>> >>> Kevin Darbyshire-Bryant writes: >>> >>> >> On 2 Jul 2018, at 19:39, Dave Taht wrote: >>> >> >>> >>> >>> >> >>> >> This seems like it will introduce problems with stuff that isn't or = is >>> >> legitimately broken in the first place, pointing to potentially rand= om >>> >> 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 ty= pe, int tc_stats_type, d->lock =3D lock; spin_lock_bh(lock); } - if (d->tail) - return gnet_stats_copy(d, type, NULL, 0, padattr); + if (d->tail) { + int ret =3D 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 =3D=3D 0 && d->tail->nla_type =3D=3D padattr) + d->tail =3D (struct nlattr *)((char *)d->tail + + NLA_ALIGN(d->tail->nla_len)); + return ret; + } =20 return 0; }