From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 9540D3B2A4 for ; Thu, 9 Jan 2025 10:06:40 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736435200; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gzcRoRMpLXdkB7rzsc91g4b2b0MisGJnEZzQL6cNwJI=; b=i8BpYmIfPPzbz+qrtxrfFmjUGEAdIT2tqBhOeYgFEJvukr5HAFsliSA1WL1+vcD6pqTXGs NEbd0H9z+jlhWOEVEAG5re/ib33wTmb5NB/4OVNIc0DaysmUpHdfor7DjBjyHRKUSh4LsQ HE/sAUCD7XYykFAp04DY1LozXR6TTRs= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-479-jFgUhjH9OZ6cAEOtizIjdQ-1; Thu, 09 Jan 2025 10:06:39 -0500 X-MC-Unique: jFgUhjH9OZ6cAEOtizIjdQ-1 X-Mimecast-MFC-AGG-ID: jFgUhjH9OZ6cAEOtizIjdQ Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-43628594d34so5722745e9.2 for ; Thu, 09 Jan 2025 07:06:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736435198; x=1737039998; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gzcRoRMpLXdkB7rzsc91g4b2b0MisGJnEZzQL6cNwJI=; b=pId2fcs9Oz8TXD3j9IyiuqsqZvRy9R2+0vo/1koKtdbNrRPlelrgjbchwLcLJLoDE7 RlKp8DR2EXs9E+KlzhrSZtN/kNZVHcjsY5L2Hxkys7+zH5cuBf7QYwEVUi2RchTdk9hD G1A9Bc1KK4Xesp5AsOOOUVteJpNwWIr2VB3j8d0fNIVYlVOd/COvRpQHsnmMmdISjhSK +b4zwVJ64rwZXWldCMeNN/pvyz0lcDSE0U2y6Kua7M3TCLgP/9rXm1+kD0wPEjN1v9L8 uTo2UONTsSSYJW8/3jP6K58kGUSXlviiJd6vjhEffV9/m5Gs5RL7a/gQzxsqWiDUKSTl 2m7g== X-Forwarded-Encrypted: i=1; AJvYcCVsSFfKu4VtiOU1UyfOkv0rfsZu1PqmbEAJnpnWs9/6tv/Tx+QwHG+q3269IN2/77DlAS3E@lists.bufferbloat.net X-Gm-Message-State: AOJu0YylZF4J6VJ/wIr5Gs9uAePigzmYiwemSw6dbhND0Fp0d+xrBL88 eNP+9ajoaQzsz3EMLNJ4A63zO6alIr/9FBAiy+gC5Hr9Frhk8RCtCoS1mpXKcEBo1VAYN+M8SyP mw2LCfan4d9Hd1ExgAc8kLEUu7+xarN62fUp46M+MSYlA/p/R/iAppSwaKmo= X-Gm-Gg: ASbGnctcpFPIKt93AL38CTknie7G81mWaqbaPr1B+xORL6zbm+I8gNYil+NFm+aefVu eyxE4bf8zu4Ahm57Q+KdP/ujEVRTxE0PfQ7MIeLutfEWo+FbTp+hEeRuVg9h4qtue3lwZsBXjGg 6+tAXv83RYFdcI4VdRC6vxDjW2tQysgOEX1tbbuYf4tG5I7vdt5w19GEfjx5OHw269uiNmuCE3g JPZThwEcQGws8yZ1xcukqM1VxLgLFLZ0cYDFp0DaG7nUMKHYlNxFqwiXGxCZ0e3/Qo6uKbiu9KM mZ2QlZPn X-Received: by 2002:a05:600c:4f4e:b0:434:a902:97cd with SMTP id 5b1f17b1804b1-436e26935cbmr44133855e9.12.1736435197648; Thu, 09 Jan 2025 07:06:37 -0800 (PST) X-Google-Smtp-Source: AGHT+IFz4fqEltbHoSuDNDTp1SE8zUB62F/bzYqQ5DX0WgJpNw0B1w2i4HNjw+Zz7/wY/VcFCtkUvA== X-Received: by 2002:a05:600c:4f4e:b0:434:a902:97cd with SMTP id 5b1f17b1804b1-436e26935cbmr44133285e9.12.1736435197100; Thu, 09 Jan 2025 07:06:37 -0800 (PST) Received: from [192.168.88.253] (146-241-2-244.dyn.eolo.it. [146.241.2.244]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-436e2e89de4sm57384505e9.34.2025.01.09.07.06.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jan 2025 07:06:36 -0800 (PST) Message-ID: <11915c70-ec5e-4d94-b890-f07f41094e2c@redhat.com> MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= , Jamal Hadi Salim , Cong Wang , Jiri Pirko Cc: syzbot+f63600d288bfb7057424@syzkaller.appspotmail.com, "David S. Miller" , Eric Dumazet , Jakub Kicinski , Simon Horman , cake@lists.bufferbloat.net, netdev@vger.kernel.org References: <20250107120105.70685-1-toke@redhat.com> <87plkwi27e.fsf@toke.dk> From: Paolo Abeni In-Reply-To: <87plkwi27e.fsf@toke.dk> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: VQrhDUDa5NStP7fY5A5envbL_YAyv4g45X_PVddBqN0_1736435198 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Mailman-Approved-At: Fri, 06 Jun 2025 06:40:58 -0400 Subject: Re: [Cake] [PATCH net v2] sched: sch_cake: add bounds checks to host bulk flow fairness counts 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: , Date: Thu, 09 Jan 2025 15:06:40 -0000 X-Original-Date: Thu, 9 Jan 2025 16:06:35 +0100 X-List-Received-Date: Thu, 09 Jan 2025 15:06:40 -0000 On 1/9/25 1:47 PM, Toke Høiland-Jørgensen wrote: > Paolo Abeni writes: >> On 1/7/25 1:01 PM, Toke Høiland-Jørgensen wrote: >>> Even though we fixed a logic error in the commit cited below, syzbot >>> still managed to trigger an underflow of the per-host bulk flow >>> counters, leading to an out of bounds memory access. >>> >>> To avoid any such logic errors causing out of bounds memory accesses, >>> this commit factors out all accesses to the per-host bulk flow counters >>> to a series of helpers that perform bounds-checking before any >>> increments and decrements. This also has the benefit of improving >>> readability by moving the conditional checks for the flow mode into >>> these helpers, instead of having them spread out throughout the >>> code (which was the cause of the original logic error). >>> >>> v2: >>> - Remove now-unused srchost and dsthost local variables in cake_dequeue() >> >> Small nit: the changelog should come after the '---' separator. No need >> to repost just for this. > > Oh, I was under the impression that we wanted them preserved in the git > log (and hence above the ---). Is that not the case (anymore?)? It was some time ago. Is this way since a while: https://elixir.bootlin.com/linux/v6.13-rc3/source/Documentation/process/maintainer-netdev.rst#L229 [...] >> dithering is now applied on both enqueue and dequeue, while prior to >> this patch it only happened on dequeue. Is that intentional? can't lead >> to (small) flow_deficit increase? > > Yeah, that was deliberate. The flow quantum is only set on enqueue when > the flow is first initialised as a sparse flow, not for every packet. > The only user-visible effect I can see this having is that the maximum > packet size that can be sent while a flow stays sparse will now vary > with +/- one byte in some cases. I am pretty sure this won't have any > consequence in practice, and I don't think it's worth complicating the > code (with a 'dither' argument to cake_flow_get_quantum(), say) to > preserve the old behaviour. Understood, and fine by me. > I guess I should have mentioned in the commit message that this was > deliberate. Since it seems you'll be editing that anyway (cf the above), > how about adding a paragraph like: > > As part of this change, the flow quantum calculation is consolidated > into a helper function, which means that the dithering applied to the > host load scaling is now applied both in the DRR rotation and when a > sparse flow's quantum is first initiated. The only user-visible effect > of this is that the maximum packet size that can be sent while a flow > stays sparse will now vary with +/- one byte in some cases. This should > not make a noticeable difference in practice, and thus it's not worth > complicating the code to preserve the old behaviour. It's in Jakub's hands now, possibly he could prefer a repost to reduce the maintainer's overhead. Thanks, Paolo