Re: [PATCH 1/3] can: bcm: copy the CAN frames outside of lock_sock()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] can: bcm: indefinite rtnl_lock stall in bcm_notify() [1]
--

commit 21cad73125ec2cd172e1d3266d5282c5a3ca76be
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: copy the CAN frames outside of lock_sock()

This commit copies CAN frames from user space into a kernel buffer before
acquiring lock_sock() in bcm_sendmsg(), preventing a stalled user-space copy
from blocking bcm_notify() under RTNL.

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 7bd2aa39777af..9c39f7363ca59 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c

[ ... ]

> @@ -1663,6 +1668,47 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	if ((size - MHSIZ) % cfsiz)
>  		return -EINVAL;
>  
> +	/* Copy the CAN frames from user space before lock_sock(), so a
> +	 * stalled copy can not block bcm_notify() waiting for lock_sock()
> +	 * under RTNL. Only the frames the opcode handler reads are copied.
> +	 * The handlers keep their checks and read the frames from fmsg.
> +	 */

[Severity: High]
This problem wasn't introduced by this patch, but can bcm_notify() still
block RTNL indefinitely in remove_proc_entry() [1]?

When a bound CAN device is unregistered, bcm_notify() is called under
rtnl_lock and tears down the socket's /proc/net/can-bcm/<inode> entry:

net/can/bcm.c:bcm_notify() {
	...
	case NETDEV_UNREGISTER:
		lock_sock(sk);
		...
		if (bo->bound && bo->ifindex == dev->ifindex) {
#if IS_ENABLED(CONFIG_PROC_FS)
			if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
				remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
				bo->bcm_proc_read = NULL;
			}
#endif
	...
}

Because that procfs entry is world-readable and remove_proc_entry() waits
for active readers (pde_users) to finish, a local user who stalls a read()
on /proc/net/can-bcm/<inode> (for example via userfaultfd or FUSE) causes
remove_proc_entry() to block forever while holding rtnl_lock and lock_sock().

[1] https://sashiko.dev/#/bug/linux-23a32da5-4d8e-457b-9e84-376ce500a113

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-can-bcmlock-memcg-v1-0-482d9f5be1fd@hartkopp.net?part=1