From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-02.galae.net (smtpout-02.galae.net [185.246.84.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6074437D137 for ; Wed, 29 Apr 2026 09:27:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.84.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777454832; cv=none; b=J7/BrVvI+cSeOU2UDVfJt4Hnyg9yLTjg4KQBg1GZf/yMpEJZKfQvkOsG0GYNAlsxei3tz19p796E2Sb+TdVmFp9VVOFn0iHZcPNoKPJN7vM0SXOXTcai5+VKGUBJe2k6zpqY5k/Verm/feU6EATaEAJk6i05w8peH1718+yTNQc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777454832; c=relaxed/simple; bh=e9kpVznoXWPmdDZYzo5h7YVmLm1mINreekjC/IMMlhU=; h=Mime-Version:Content-Type:Date:Message-Id:From:Subject:Cc:To: References:In-Reply-To; b=V1O+fK5cQ7c8DsDVtG5YdngSnPF6H2GqgtYdfc8dV+lfXvNMQTgiqPozjoin0mGmasZ7BASPx9JCA763DkC/rtk8I4MQ8iMk1hXQXizMaTF8FuJhWIXtWmXQRdX0mlGj8dsG8am1kAtba7UcargQpcwMxU9xL8Z7Q7uLMRviHbo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=1nArF057; arc=none smtp.client-ip=185.246.84.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="1nArF057" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-02.galae.net (Postfix) with ESMTPS id 066D51A3478; Wed, 29 Apr 2026 09:27:03 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id CCBA9601DF; Wed, 29 Apr 2026 09:27:02 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id B918D10729379; Wed, 29 Apr 2026 11:26:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1777454821; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:in-reply-to:references; bh=xZ8VMjmAuXE7bj1xiXzUWvyTTPxElhI0qs5AYciLeQ8=; b=1nArF057MsMdZUgpYhN6s2KAv0T+92sKVtzrc9vC4QAfR0RXrUhkbBhxtmA9h1DCjxoujY QVnIjPjFlbwJud/AmAcnGvuzmaW9fNl+GOe/afSDYnBUDnQOsdmCsbUSVr07HAWddXsSj6 IaJksiqOzEQDbMp/0m5HTQvuorwLatoJIiupolrhAKmWuSoHdq+e/nz2zG+3ZoZdJ3NoaR 4IcS4+0df/byy5pe321xxmEhjDt40kcvH7xHhQeSRjvf3VI3HkbxyISGfetNNkseaCEuuH piAruBTpPYZ/tTLULngBtcnjj1Br7ipcaommegW5qt2UElS4/W4X+FyrPFgq1A== Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 29 Apr 2026 11:26:47 +0200 Message-Id: From: =?utf-8?q?Th=C3=A9o_Lebrun?= Subject: Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close Cc: "Nicolas Ferre" , "Claudiu Beznea" , "Andrew Lunn" , "David S. Miller" , "Eric Dumazet" , "Jakub Kicinski" , "Paolo Abeni" , "Haavard Skinnemoen" , "Jeff Garzik" , "Paolo Valerio" , "Conor Dooley" , , , "Vladimir Kondratiev" , "Gregory CLEMENT" , =?utf-8?q?Beno=C3=AEt_Monin?= , "Tawfik Bayouk" , "Thomas Petazzoni" , "Maxime Chevallier" , To: "Nicolai Buchwitz" X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260428-macb-drop-tx-v2-0-647f5199d8df@bootlin.com> <20260428-macb-drop-tx-v2-2-647f5199d8df@bootlin.com> <75229fab491465e06a98ee580a51f0b4@tipi-net.de> In-Reply-To: <75229fab491465e06a98ee580a51f0b4@tipi-net.de> X-Last-TLS-Session-Version: TLSv1.3 Hello Nicolai, On Tue Apr 28, 2026 at 11:30 PM CEST, Nicolai Buchwitz wrote: > On 28.4.2026 18:32, Th=C3=A9o Lebrun wrote: >> The MACB driver has since forever leaked the outgoing SKBs that >> have not yet been marked as completed. They live in queue->tx_skb >> which gets freed without remorse nor checking. >>=20 >> macb_free_consistent() gets called in a few codepaths, but only >> close will trigger the added expressions. In macb_open() and >> macb_alloc_consistent() failure cases, tx_skb just got allocated >> and is empty. >>=20 >> Use the new macb_tx_unmap() prototype to report our error as >> SKB_DROP_REASON_NOT_SPECIFIED rather than SKB_CONSUMED which makes it >> sound like no error occurred. Equivalent to dev_kfree_skb_any(). >>=20 >> Fixes: 89e5785fc8a6 ("[PATCH] Atmel MACB ethernet driver") >> Cc: stable@vger.kernel.org >> Signed-off-by: Th=C3=A9o Lebrun >> --- >> drivers/net/ethernet/cadence/macb_main.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >>=20 >> diff --git a/drivers/net/ethernet/cadence/macb_main.c=20 >> b/drivers/net/ethernet/cadence/macb_main.c >> index 9caae1ef52b1..5a2500bd59a6 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -2678,8 +2678,26 @@ static void macb_free_consistent(struct macb=20 >> *bp) >> dma_free_coherent(dev, size, bp->queues[0].rx_ring,=20 >> bp->queues[0].rx_ring_dma); >>=20 >> for (q =3D 0, queue =3D bp->queues; q < bp->num_queues; ++q, ++queue) = { >> - kfree(queue->tx_skb); >> - queue->tx_skb =3D NULL; >> + if (queue->tx_skb) { >> + unsigned int dropped =3D 0, tail; >> + >> + for (tail =3D queue->tx_tail; tail !=3D queue->tx_head; >> + tail++) { >> + if (macb_tx_skb(queue, tail)->skb) >> + dropped++; >> + macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0, >> + SKB_DROP_REASON_NOT_SPECIFIED); >> + } > > Reviewed-by: Nicolai Buchwitz Thanks for the review! We are quite a few caring about MACB which is nice. > Side note, not blocking: macb_close() doesn't cancel tx_error_task, > so the workqueue handler can race with this loop on tx_skb[]. The > exposure is pre-existing, but maybe worth a follow-up adding > cancel_work_sync() between napi_disable() and macb_free_consistent(). Yes, noticed that while working on the context swapping series [0]. The goal here is to improve MACB piecewise, so I won't take that on in the current series. [0]: https://lore.kernel.org/all/90f843aa3940bdbabadddce27314c1f1@tipi-net.= de/t/#mda18f759c27a4d833084b23605463994632d97e3 (and the two replies) Thanks, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com