From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.toke.dk (mail.toke.dk [IPv6:2001:470:dc45:1000::1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 094843BA8E for ; Thu, 26 Apr 2018 03:14:59 -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=1524726897; bh=4lPL1ynGFG5CdhqMDKnCnkEvwAcDGD1pxSciZPd53nI=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=qsByF9gX8hZEIfScOHq0fRtApYyiXG4s9WquoOuZIztM4rjT+wCgFwzwGHx47FqoL lSusaT+CKxDrQExTaNack3FsBvF5hzkeR9WVG3RbMqkyNltclE/mU0AP3AUw/XbsLk lIp2l46UxBhjoK/UOO32TUCwolWagPsbtOCWsjLsDJ2JW1LAUHmFO2VdI0G+Oy7ZTh W3rV244tBCzjJmPPtUnk4xZmLjin/WTwKXsr8Ag8uS/Q0gPXKzrL3OwzW1FIPr1UnQ vmDFpL0f7XRkeUcVmeJrn52sCU1DSCKXT7cXYfCTjIRpxiGwDrIgpEAyPSAK4y6LGE TEE97WeyrGhaQ== To: Ryan Mounce Cc: Cake List In-Reply-To: References: <87vacf3th7.fsf@toke.dk> <87po2m4gka.fsf@toke.dk> Date: Thu, 26 Apr 2018 09:14:55 +0200 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87muxq4exc.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] CAKE upstreaming - testers wanted, ACK filtering rescuers needed 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: Thu, 26 Apr 2018 07:14:59 -0000 Ryan Mounce writes: >> On 26 Apr 2018, at 16:09, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> >> Ryan Mounce writes: >> >>> I'll investigate making the ACK filtering code safe, it is my mess afte= r all :) >>> >>> Eric obviously understands this stuff a lot better than me, it looks >>> like there are two issues? >>> - Lack of minimum length check for TCP header, should be fairly >>> straight-forward to fix >>> - The possibility of unsafely filtering part of a split GSO >>> super-packet? >> >> The issue with the ACK filter in relation to GSO was just that a GSO >> segment can't contain pure ACKs, so there's no reason to check after >> splitting. I've already removed that part, so it's just the length >> issue. I think it's just a matter of finding the length and calling >> pskb_may_pull() (and aborting if that returns false). >> >> -Toke > > Yep I think I=E2=80=99ve got all of that together now and I=E2=80=99ve su= bmitted a PR > against the cobalt branch. Awesome thanks! Only a small nit, which I commented on the PR. > An optimisation to what I=E2=80=99ve submitted would be moving the separa= te > TCP header length check to only the IPv4 case, and adding > sizeof(tcphdr) to the initial IPv6 header length check. Not that my > ACK filtering code is so efficient to begin with... Yeah, might as well? Another obvious optimisation would be to add a flag to the cb struct for pure ACKs (which would be set at the beginning, when ack_check is called on packet enqueue). That way the loop could skip over anything that is not marked as a pure ACK and a lot of checks could be skipped for the packets being iterated over in the queue... -Toke