can: raw: take rtnl_lock() before leaving the notifier list
Subsystems (2)
Oliver Hartkopp
maintainerPATCH 1/1 can: raw: take rtnl_lock() before leaving the notifier list
raw_release() removes the socket from raw_notifier_list before it takes rtnl_lock(). As the netdev notifier chain runs under RTNL, a socket that leaves the list in this window is skipped by raw_notify(). This is not harmful for raw as the filters are removed with dev_net(ro->dev) under RTNL, but the receivers may briefly remain on a device that has already been moved to another namespace.
Align raw_release() with the isotp and bcm fixes for the same window and take rtnl_lock() before removing the socket from the notifier list. As raw_busy_notifier can not point to the released socket under RTNL, replace the busy-wait loop with a WARN_ON_ONCE().
--- net/can/raw.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
net/can/raw.c +7 -6
@@ -420,20 +420,21 @@ static int raw_release(struct socket *sock)return 0 ; ro =raw_sk (sk); net =sock_net (sk); +rtnl_lock (); + +/* The netdev notifier chain runs under RTNL, so raw_notifier() can't +* be walking raw_notifier_list and point to this socket. The spinlock +* is still needed as raw_init() adds sockets without holding RTNL. +*/ spin_lock (&raw_notifier_lock); -while (raw_busy_notifier == ro) { -spin_unlock (&raw_notifier_lock); -schedule_timeout_uninterruptible (1 ); -spin_lock (&raw_notifier_lock); - } +WARN_ON_ONCE (raw_busy_notifier == ro);list_del (&ro->notifier);spin_unlock (&raw_notifier_lock); -rtnl_lock ();lock_sock (sk);/* remove current filters & unregister */ if (ro->bound) {if (ro->dev) {
Signature · 1 line
2.53.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [Medium] The commit message for raw_release() (net/can/raw.c) says the change is meant to "Align raw_release() with the isotp and bcm fixes…
--
can: raw: take rtnl_lock() before leaving the notifier list
This commit moves rtnl_lock() in raw_release() so it runs before the socket is removed from raw_notifier_list. A concurrent netdev notifier then can't skip a socket that is being released. Under RTNL, raw_busy_notifier can't point to the released socket, so the busy-wait loop is replaced with a WARN_ON_ONCE().
Quoted 4 lines
Align
raw_release()with the isotp and bcm fixes for the same window and takertnl_lock()before removing the socket from the notifier list. Asraw_busy_notifiercan not point to the released socket under RTNL, replace the busy-wait loop with aWARN_ON_ONCE().
[Severity: Medium]
Which isotp and bcm fixes does this refer to? The message has no commit ids or Link: tags. In this tree, both of those release paths still look the way raw_release() looked before this patch.
isotp_release() in net/can/isotp.c still calls list_del() before it takes RTNL:
net/can/isotp.c:isotp_release() {
...
spin_lock(&isotp_notifier_lock);
while (isotp_busy_notifier == so) {
spin_unlock(&isotp_notifier_lock);
schedule_timeout_uninterruptible(1);
spin_lock(&isotp_notifier_lock);
}
list_del(&so->notifier);
spin_unlock(&isotp_notifier_lock);
rtnl_lock();
lock_sock(sk);
...
}bcm_release() in net/can/bcm.c has the same busy-wait loop. It calls list_del(&bo->notifier) without holding RTNL at all.
For isotp, this window also seems to matter more than it does for raw. Suppose dev_change_net_namespace() runs in the window (for example, ip link set <candev> netns X). isotp_notifier() skips the socket, because it is already off isotp_notifier_list. isotp_release() then calls can_rx_unregister() with net = sock_net(sk), and that returns early once the device has moved:
net/can/af_can.c:can_rx_unregister() {
...
if (dev && !net_eq(net, dev_net(dev)))
return;
...
}Wouldn't that leave receivers with data == sk on the device's receive lists after sock_put() frees the socket? raw avoids this only because raw_release() passes dev_net(ro->dev) instead of sock_net(sk).
Could the commit message name the isotp and bcm commits it relies on? Alternatively, it could be reworded so it doesn't suggest isotp and bcm are already fixed for this window. As written, the isotp and bcm changes that are still needed could be missed in review or stable backporting.
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013115557.114582-1-socketcan%40hartkopp.net
Oliver Hartkopp
maintainerThis is an old feedback.
Corrected patch based on this sashiko review is already on its way:
https://lore.kernel.org/linux-can/[email protected]/
Quoted 80 lines · On 15.10.26 03:02, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [Medium] The commit message for
raw_release()(net/can/raw.c) says the change is meant to "Alignraw_release()with the isotp and bcm fixes…--
can: raw: take
rtnl_lock()before leaving the notifier listThis commit moves
rtnl_lock()inraw_release()so it runs before the socket is removed fromraw_notifier_list. A concurrent netdev notifier then can't skip a socket that is being released. Under RTNL,raw_busy_notifiercan't point to the released socket, so the busy-wait loop is replaced with aWARN_ON_ONCE().Quoted 4 lines
Align
raw_release()with the isotp and bcm fixes for the same window and takertnl_lock()before removing the socket from the notifier list. Asraw_busy_notifiercan not point to the released socket under RTNL, replace the busy-wait loop with aWARN_ON_ONCE().[Severity: Medium]
Which isotp and bcm fixes does this refer to? The message has no commit ids or Link: tags. In this tree, both of those release paths still look the wayraw_release()looked before this patch.
isotp_release()innet/can/isotp.cstill callslist_del()before it takes RTNL:net/can/isotp.c:isotp_release() { ... spin_lock(&isotp_notifier_lock); while (isotp_busy_notifier == so) { spin_unlock(&isotp_notifier_lock); schedule_timeout_uninterruptible(1); spin_lock(&isotp_notifier_lock); } list_del(&so->notifier); spin_unlock(&isotp_notifier_lock); rtnl_lock(); lock_sock(sk); ... }
bcm_release()innet/can/bcm.chas the same busy-wait loop. It callslist_del(&bo->notifier)without holding RTNL at all.For isotp, this window also seems to matter more than it does for raw. Suppose
dev_change_net_namespace()runs in the window (for example, ip link set <candev> netns X).isotp_notifier()skips the socket, because it is already offisotp_notifier_list.isotp_release()then callscan_rx_unregister()with net =sock_net(sk), and that returns early once the device has moved:net/can/af_can.c:can_rx_unregister() { ... if (dev && !net_eq(net, dev_net(dev))) return; ... }Wouldn't that leave receivers with data == sk on the device's receive lists after
sock_put()frees the socket? raw avoids this only becauseraw_release()passesdev_net(ro->dev)instead ofsock_net(sk).Could the commit message name the isotp and bcm commits it relies on? Alternatively, it could be reworded so it doesn't suggest isotp and bcm are already fixed for this window. As written, the isotp and bcm changes that are still needed could be missed in review or stable backporting.