From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 E9F133DA7DD for ; Tue, 7 Jul 2026 10:14:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783419262; cv=none; b=lIpaqNDSby5MJcAJvnA7Ebti0gpJFnB8AQr8jxkKcnEScgKPQOw41U1dTgRbNG9PdkW0onA+y/36nYpP2WQKUeZ7qKgoNU23AyJ9480VMiLTp6fQ/2qPbIeNmaguu7O8pjuEgxqk0c9GXcdJd0w9nHEF2ImYeRne1Z0c0+jsRU8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783419262; c=relaxed/simple; bh=TBY/EQjj4zzuCqeEAWAPvJPomMlXSs6AIHUWV7Yt0x0=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=aiG2TYfMOik2g7MKXitb3/gZB4/sQRF0nthXPYRx/9W2qHnYfqEZY/0ZX8aITu/NXePYIpdaZh8U2eaE38PoqpolsA1bvsVjHD5JYKyod3ftFiRqN0iUdStfxraIRRy/2O5SyvL1ATJo2JOla9eW1EEEHlMPYssMB5c72kEb+pE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=am8tLYIR; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="am8tLYIR" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 47F261F000E9; Tue, 7 Jul 2026 10:14:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783419260; bh=hoXwavVAbHtmVMmdLfm2WlaHYwkGfCMMskzTZB5GBI8=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=am8tLYIRJeFV0Yujq00RupjuufiE2SGFhO049vgjBg4mZRtlZ2MKDG5wpszUmDwnA Q8UnZpP6xHx7GkGAWLG1X2kyZupl3z3cEgjrN+PlZnDGZOO4bKBPpo4vYrkJn3siAy ky6s13y4K1n5EK4U9Nd9PvgPkVji1Gyba2aPsMyCX3UI2X6RHQZizuKEDWESxbW2jK UR42NNRThsgNxyjjH4N+7ZAk1cWOUCmeeGVTXTPbW/cHnAo12C0zALRqEBRz1nK52h fXneiLaFSkR6YoRsTIpHstJtk0MZpIJUQxvXd8f87WF0xt0PS2yrxr8Pkad9dKXc9s mg4hY654R0ksA== From: sashiko-bot@kernel.org Subject: Re: [PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release() Reply-To: sashiko-reviews@lists.linux.dev To: "Oliver Hartkopp" Cc: "Marc Kleine-Budde" , "Vincent Mailhol" , "Oliver Hartkopp" , "Oleksij Rempel" , linux-can@vger.kernel.org In-Reply-To: <20260707094716.63578-1-socketcan@hartkopp.net> References: <20260707094716.63578-1-socketcan@hartkopp.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Tue, 07 Jul 2026 10:14:19 +0000 Message-Id: <20260707101420.47F261F000E9@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-can@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 1 potential issue(= s) to consider: Pre-existing issues: - [Critical] Skipped CAN filter unregistration during concurrent device rem= oval leads to Use-After-Free and memory leak. -- --- Patch [1]: [PATCH v2] can: isotp: use unconditional synchronize_rcu() i= n isotp_release() --- commit 0af34892c79e9f6de424253e9d5e275db6cb6280 Author: Oliver Hartkopp can: isotp: use unconditional synchronize_rcu() in isotp_release() This commit addresses a race condition in the timer callback of isotp sockets. It enforces an unconditional synchronize_rcu() in isotp_release() before cancelling timers to avoid issues during concurrent NETDEV_UNREGISTER notifications. Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.= kernel.org/ > diff --git a/net/can/isotp.c b/net/can/isotp.c > --- a/net/can/isotp.c > +++ b/net/can/isotp.c [ ... ] > @@ -1227,14 +1227,20 @@ static int isotp_release(struct socket *sock) > /* remove current filters & unregister */ > if (so->bound) { > if (so->ifindex) { > struct net_device *dev; > =20 > dev =3D dev_get_by_index(net, so->ifindex); > if (dev) { > if (isotp_register_rxid(so)) > can_rx_unregister(net, dev, so->rxid, > SINGLE_MASK(so->rxid), > isotp_rcv, sk); [Severity: Critical] This is a pre-existing issue, but could this dev_get_by_index() call return NULL if a device unregistration is running concurrently? If unregister_netdevice_many() unlists the device before the NETDEV_UNREGISTER notifier executes, and isotp_release() is called at the same time, it removes the socket from the isotp notifier list. When it calls dev_get_by_index() here, it will return NULL because the device is unlisted. The if (dev) condition fails and we skip calling can_rx_unregister(). The NETDEV_UNREGISTER notifier will miss this socket because it was already removed from the notifier list. Since the socket is freed at the end of this function, does this leave an active CAN filter pointing to a freed socket? Any delayed or looped-back CAN frames processed during device teardown could invoke isotp_rcv() on this freed socket, causing a use-after-free and a memory leak. > =20 > can_rx_unregister(net, dev, so->txid, > SINGLE_MASK(so->txid), > isotp_rcv_echo, sk); > dev_put(dev); > - synchronize_rcu(); > } > } > } > =20 > + /* Always wait for a grace period before touching the timers below. > + * A concurrent NETDEV_UNREGISTER may have already unregistered our --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260707094716.6357= 8-1-socketcan@hartkopp.net?part=3D1