[PATCH 0/3] can: bcm: avoid RTNL stalls and enable memcg accounting

[PATCH 0/3] can: bcm: avoid RTNL stalls and enable memcg accounting

[Oliver Hartkopp]

This series addresses issues that allow unprivileged users to affect
the whole system through CAN sockets.

Patch 1 copies the CAN frames in bcm_sendmsg() outside of lock_sock(),
so that a stalled copy from user space can not block bcm_notify(),
which takes lock_sock() under RTNL.

Patch 2 keeps the procfs entry of a CAN_BCM socket until bcm_release(),
as remove_proc_entry() in bcm_notify() waits for stalled procfs readers
while holding RTNL.

Patch 3 enables memcg accounting for the allocations that unprivileged
users can trigger in the CAN subsystem. As there is no specific commit
to point to, it has no Fixes: tag and is not marked for stable, like
most of the memcg accounting patches in the networking subsystem. If
you prefer this patch for net-next, please let me know.

CAN_J1939 has similar issues with lock_sock() under RTNL, which will
be addressed separately once the currently discussed j1939 patches
have been merged.

---
Oliver Hartkopp (3):
      can: bcm: copy the CAN frames outside of lock_sock()
      can: bcm: do not remove the procfs entry in bcm_notify()
      can: enable memcg accounting for user triggered allocations

 net/can/af_can.c       | 12 ++++----
 net/can/bcm.c          | 80 ++++++++++++++++++++++++++++++++++++++++----------
 net/can/isotp.c        |  2 +-
 net/can/j1939/main.c   |  2 +-
 net/can/j1939/socket.c | 11 +++++--
 net/can/raw.c          | 13 +++++---
 6 files changed, 90 insertions(+), 30 deletions(-)
---
base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
change-id: 20261107-can-bcmlock-memcg-2d8ae7361838

Best regards,
--  
Oliver Hartkopp <socketcan@hartkopp.net>

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

[Oliver Hartkopp]

bcm_sendmsg() copies the CAN frames of TX_SETUP, RX_SETUP and TX_SEND
from user space while holding lock_sock(). A copy that faults or
stalls, e.g. on a FUSE backed mapping, holds lock_sock() for its whole
duration.

bcm_notify() takes lock_sock() of every CAN_BCM socket in the namespace
of a vanishing CAN device while holding RTNL, independent of the
binding of the socket. So a stalled copy on any CAN_BCM socket can
block RTNL on the unregistration or namespace change of a CAN device in
this namespace, e.g. of a vcan device in an unprivileged user and
network namespace.

Copy the CAN frames that the opcode handler reads to kernel memory
before lock_sock() and hand them to the handlers with a kvec based
msghdr. bcm_tx_setup(), bcm_rx_setup() and bcm_tx_send() keep their
checks and read the frames with memcpy_from_msg() as before. A single
CAN frame is copied to a buffer on the stack, multiple frames to a
kvmalloc() buffer of up to MAX_NFRAMES + 1 CAN FD frames.

As the frames are copied before lock_sock() now, a failing copy is
reported before the error conditions that are checked under
lock_sock(). This is in line with POSIX, which does not define an
order in which multiple error conditions are detected and reported.

The generic SOL_SOCKET options in sk_setsockopt() are not affected by
this patch.

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 7bd2aa39777a..9c39f7363ca5 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1641,10 +1641,15 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 {
 	struct sock *sk = sock->sk;
 	struct bcm_sock *bo = bcm_sk(sk);
 	int ifindex;
 	struct bcm_msg_head msg_head;
+	struct msghdr fmsg = {};	/* CAN frames copied to kernel memory */
+	struct kvec kv;
+	struct canfd_frame sframe;	/* buffer for a single CAN CC / FD frame */
+	void *frames = &sframe;
+	size_t flen = 0;
 	int cfsiz;
 	int ret; /* read bytes or error codes as return value */
 
 	/* Lockless fast-path check for bound socket */
 	if (!READ_ONCE(bo->bound))
@@ -1661,10 +1666,51 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 	cfsiz = CFSIZ(msg_head.flags);
 	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.
+	 */
+	switch (msg_head.opcode) {
+	case TX_SETUP:
+		if (msg_head.nframes >= 1 && msg_head.nframes <= MAX_NFRAMES)
+			flen = msg_head.nframes * cfsiz;
+		break;
+
+	case RX_SETUP:
+		if (!(msg_head.flags & RX_FILTER_ID) &&
+		    msg_head.nframes <= MAX_NFRAMES + 1)
+			flen = msg_head.nframes * cfsiz;
+		break;
+
+	case TX_SEND:
+		if (msg_head.nframes == 1 && size == cfsiz + MHSIZ)
+			flen = cfsiz;
+		break;
+	}
+
+	/* a message with missing frames still fails in the handler */
+	flen = min(flen, msg_data_left(msg));
+
+	if (flen > sizeof(sframe)) {
+		frames = kvmalloc(flen, GFP_KERNEL);
+		if (!frames)
+			return -ENOMEM;
+	}
+
+	if (!copy_from_iter_full(frames, flen, &msg->msg_iter)) {
+		ret = -EFAULT;
+		goto out_free;
+	}
+
+	kv.iov_base = frames;
+	kv.iov_len = flen;
+	iov_iter_kvec(&fmsg.msg_iter, ITER_SOURCE, &kv, 1, flen);
+
 	lock_sock(sk);
 
 	/* Re-validate under the socket lock: a concurrent bcm_notify()
 	 * may have unbound this socket (device removal) after the
 	 * lockless fast-path check above. bo->ifindex is only ever
@@ -1719,15 +1765,15 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	}
 
 	switch (msg_head.opcode) {
 
 	case TX_SETUP:
-		ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
+		ret = bcm_tx_setup(&msg_head, &fmsg, ifindex, sk);
 		break;
 
 	case RX_SETUP:
-		ret = bcm_rx_setup(&msg_head, msg, ifindex, sk);
+		ret = bcm_rx_setup(&msg_head, &fmsg, ifindex, sk);
 		break;
 
 	case TX_DELETE:
 		if (bcm_delete_tx_op(&bo->tx_ops, &msg_head, ifindex))
 			ret = MHSIZ;
@@ -1757,20 +1803,23 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	case TX_SEND:
 		/* we need exactly one CAN frame behind the msg head */
 		if ((msg_head.nframes != 1) || (size != cfsiz + MHSIZ))
 			ret = -EINVAL;
 		else
-			ret = bcm_tx_send(msg, ifindex, sk, cfsiz);
+			ret = bcm_tx_send(&fmsg, ifindex, sk, cfsiz);
 		break;
 
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
 out_release:
 	release_sock(sk);
+out_free:
+	if (frames != &sframe)
+		kvfree(frames);
 
 	return ret;
 }
 
 /*

-- 
2.53.0

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

[sashiko-bot]

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

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

[Oliver Hartkopp]

On 07.11.26 14:15, sashiko-bot@kernel.org wrote:
> 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]?

This bot is really not that intelligent when it isn't even able to look 
into the following patch [2/3] of this series, which fixes exactly this.
m(

> 
> 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
> 

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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).

- [High] The patch does not close the RTNL-blocking vector that its
  commit message describes.
- [Low] The new comment on the stack buffer in bcm_sendmsg() (`struct
  canfd_frame sframe; /* buffer for a single CAN CC / FD frame */`) and…

--

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

This patch moves the user space copy of the CAN frames for TX_SETUP,
RX_SETUP and TX_SEND in bcm_sendmsg() to before lock_sock(). The frames
go to a stack buffer or a kvmalloc() buffer and are handed to the opcode
handlers through a kvec based msghdr. The goal is that a stalled copy
can no longer hold lock_sock() while bcm_notify() waits for it 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
> @@ -1643,6 +1643,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	struct bcm_sock *bo = bcm_sk(sk);
>  	int ifindex;
>  	struct bcm_msg_head msg_head;
> +	struct msghdr fmsg = {};	/* CAN frames copied to kernel memory */
> +	struct kvec kv;
> +	struct canfd_frame sframe;	/* buffer for a single CAN CC / FD frame */

[Severity: Low]
This isn't a bug, but this comment and the commit message don't match
what the code does. The commit message says:

    A single CAN frame is copied to a buffer on the stack, multiple
    frames to a kvmalloc() buffer

The code picks the buffer by byte size, not by frame count:

    if (flen > sizeof(sframe)) {
        frames = kvmalloc(flen, GFP_KERNEL);

sizeof(struct canfd_frame) is 72 (CANFD_MTU). For TX_SETUP or RX_SETUP
without CAN_FD_FRAME, cfsiz is CAN_MTU (16). So 2 to 4 classic CAN
frames (flen of 32, 48 or 64) land in sframe, and kvmalloc() only starts
at the fifth frame.

Could the comment and the commit message say the stack buffer holds up
to sizeof(struct canfd_frame) bytes? A later change that trusts the
"single frame" wording, for example shrinking sframe to struct
can_frame for the CC case, would let the byte-size check overflow the
stack object.

> +	void *frames = &sframe;
> +	size_t flen = 0;
>  	int cfsiz;
>  	int ret; /* read bytes or error codes as return value */
>  
> @@ -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.
> +	 */

[ ... ]

> +	kv.iov_base = frames;
> +	kv.iov_len = flen;
> +	iov_iter_kvec(&fmsg.msg_iter, ITER_SOURCE, &kv, 1, flen);
> +
>  	lock_sock(sk);

[Severity: High]
Can RTNL still be blocked the same way through other paths on a CAN_BCM
socket?

The commit message names the root cause as bcm_notify() taking
lock_sock() of every CAN_BCM socket in the netns while RTNL is held:

net/can/bcm.c:bcm_notify() {
    ...
	case NETDEV_UNREGISTER:
		lock_sock(sk);
    ...
}

That part is unchanged. bcm_ops has no .setsockopt or .getsockopt, so
SOL_SOCKET options go to the generic sk_setsockopt(), which copies user
memory while holding the socket lock:

net/core/sock.c:sk_setsockopt() {
    ...
	sockopt_lock_sock(sk);

	switch (optname) {
    ...
		ret = copy_bpf_fprog_from_user(&fprog, optval, optlen);
		if (!ret)
			ret = sk_attach_filter(&fprog, sk);
    ...
}

sk_attach_filter()->__get_filter() then does:

	if (copy_from_user(prog->insns, fprog->filter, fsize)) {

fprog->filter is a pointer chosen by the user, and outside of TCP no
capability is needed. SO_ATTACH_REUSEPORT_CBPF follows the same path.
SO_GET_FILTER->sk_get_filter() also writes to user memory with
copy_to_sockptr() while sockopt_lock_sock() is held.

So an unprivileged user in a user and net namespace could do:

    open CAN_BCM socket
    setsockopt(SOL_SOCKET, SO_ATTACH_FILTER) with fprog.filter on a
      FUSE backed mapping that never completes the fault
      -> __get_filter() stalls with the socket lock held
    delete the vcan device (or tear down the netns)
      -> bcm_notifier()->bcm_notify()->lock_sock() sleeps under RTNL

Isn't that the same scenario the commit message describes as fixed? The
statement "The generic SOL_SOCKET options in sk_setsockopt() are not
affected by this patch" doesn't say that this path can still block
RTNL, and it doesn't mention the getsockopt path.

bcm_notify() still takes lock_sock(sk) under NETDEV_UNREGISTER after the
later patches in this series ("can: bcm: do not remove the procfs entry
in bcm_notify()" and "can: enable memcg accounting for user triggered
allocations").

Would it be better to change bcm_notify() so it doesn't sleep on
lock_sock() of unrelated sockets under RTNL, or to defer the per-socket
cleanup out of the RTNL section? Either of those would replace moving
individual copies in bcm_sendmsg() out of the lock.

>  
>  	/* Re-validate under the socket lock: a concurrent bcm_notify()

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-can-bcmlock-memcg-v1-0-482d9f5be1fd%40hartkopp.net

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

[Oliver Hartkopp]

On 09.11.26 03:18, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
> 
> 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).
> 
> - [High] The patch does not close the RTNL-blocking vector that its
>    commit message describes.
> - [Low] The new comment on the stack buffer in bcm_sendmsg() (`struct
>    canfd_frame sframe; /* buffer for a single CAN CC / FD frame */`) and…
> 
> --
> 
> can: bcm: copy the CAN frames outside of lock_sock()
> 
> This patch moves the user space copy of the CAN frames for TX_SETUP,
> RX_SETUP and TX_SEND in bcm_sendmsg() to before lock_sock(). The frames
> go to a stack buffer or a kvmalloc() buffer and are handed to the opcode
> handlers through a kvec based msghdr. The goal is that a stalled copy
> can no longer hold lock_sock() while bcm_notify() waits for it 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
>> @@ -1643,6 +1643,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>>   	struct bcm_sock *bo = bcm_sk(sk);
>>   	int ifindex;
>>   	struct bcm_msg_head msg_head;
>> +	struct msghdr fmsg = {};	/* CAN frames copied to kernel memory */
>> +	struct kvec kv;
>> +	struct canfd_frame sframe;	/* buffer for a single CAN CC / FD frame */
> 
> [Severity: Low]
> This isn't a bug, but this comment and the commit message don't match
> what the code does. The commit message says:
> 
>      A single CAN frame is copied to a buffer on the stack, multiple
>      frames to a kvmalloc() buffer
> 
> The code picks the buffer by byte size, not by frame count:
> 
>      if (flen > sizeof(sframe)) {
>          frames = kvmalloc(flen, GFP_KERNEL);
> 
> sizeof(struct canfd_frame) is 72 (CANFD_MTU). For TX_SETUP or RX_SETUP
> without CAN_FD_FRAME, cfsiz is CAN_MTU (16). So 2 to 4 classic CAN
> frames (flen of 32, 48 or 64) land in sframe, and kvmalloc() only starts
> at the fifth frame.
> 
> Could the comment and the commit message say the stack buffer holds up
> to sizeof(struct canfd_frame) bytes? A later change that trusts the
> "single frame" wording, for example shrinking sframe to struct
> can_frame for the CC case, would let the byte-size check overflow the
> stack object.

Correct. Reworded the commit message in v2:

Up to
sizeof(struct canfd_frame) bytes, i.e. a single CAN FD frame or up to
four CAN CC frames, are copied to a buffer on the stack, more frames to
a kvmalloc() buffer of up to MAX_NFRAMES + 1 CAN FD frames.

https://lore.kernel.org/linux-can/20261108-can-bcmlock-memcg-v2-1-19fafdfc9d12@hartkopp.net/

pw-bot: cr

> 
>> +	void *frames = &sframe;
>> +	size_t flen = 0;
>>   	int cfsiz;
>>   	int ret; /* read bytes or error codes as return value */
>>   
>> @@ -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.
>> +	 */
> 
> [ ... ]
> 
>> +	kv.iov_base = frames;
>> +	kv.iov_len = flen;
>> +	iov_iter_kvec(&fmsg.msg_iter, ITER_SOURCE, &kv, 1, flen);
>> +
>>   	lock_sock(sk);
> 
> [Severity: High]
> Can RTNL still be blocked the same way through other paths on a CAN_BCM
> socket?
> 
> The commit message names the root cause as bcm_notify() taking
> lock_sock() of every CAN_BCM socket in the netns while RTNL is held:
> 
> net/can/bcm.c:bcm_notify() {
>      ...
> 	case NETDEV_UNREGISTER:
> 		lock_sock(sk);
>      ...
> }
> 
> That part is unchanged. bcm_ops has no .setsockopt or .getsockopt, so
> SOL_SOCKET options go to the generic sk_setsockopt(), which copies user
> memory while holding the socket lock:
> 

Right, but not focus of this patch. Added this to v2 commit message:

The generic SOL_SOCKET options in sk_setsockopt() and sk_getsockopt(),
e.g. SO_ATTACH_FILTER, still access user space under the socket lock.
This is not specific to CAN_BCM and not addressed by this patch.

In fact this is not CAN specific and sk_attach_filter() is not used 
together with CAN as we have our specific filtering infrastructure.

> net/core/sock.c:sk_setsockopt() {
>      ...
> 	sockopt_lock_sock(sk);
> 
> 	switch (optname) {
>      ...
> 		ret = copy_bpf_fprog_from_user(&fprog, optval, optlen);
> 		if (!ret)
> 			ret = sk_attach_filter(&fprog, sk);
>      ...
> }
> 
> sk_attach_filter()->__get_filter() then does:
> 
> 	if (copy_from_user(prog->insns, fprog->filter, fsize)) {
> 
> fprog->filter is a pointer chosen by the user, and outside of TCP no
> capability is needed. SO_ATTACH_REUSEPORT_CBPF follows the same path.
> SO_GET_FILTER->sk_get_filter() also writes to user memory with
> copy_to_sockptr() while sockopt_lock_sock() is held.
> 
> So an unprivileged user in a user and net namespace could do:
> 
>      open CAN_BCM socket
>      setsockopt(SOL_SOCKET, SO_ATTACH_FILTER) with fprog.filter on a
>        FUSE backed mapping that never completes the fault
>        -> __get_filter() stalls with the socket lock held
>      delete the vcan device (or tear down the netns)
>        -> bcm_notifier()->bcm_notify()->lock_sock() sleeps under RTNL
> 
> Isn't that the same scenario the commit message describes as fixed? The
> statement "The generic SOL_SOCKET options in sk_setsockopt() are not
> affected by this patch" doesn't say that this path can still block
> RTNL, and it doesn't mention the getsockopt path.
> 
> bcm_notify() still takes lock_sock(sk) under NETDEV_UNREGISTER after the
> later patches in this series ("can: bcm: do not remove the procfs entry
> in bcm_notify()" and "can: enable memcg accounting for user triggered
> allocations").
> 
> Would it be better to change bcm_notify() so it doesn't sleep on
> lock_sock() of unrelated sockets under RTNL, or to defer the per-socket
> cleanup out of the RTNL section? Either of those would replace moving
> individual copies in bcm_sendmsg() out of the lock.
> 
>>   
>>   	/* Re-validate under the socket lock: a concurrent bcm_notify()
> 
> [ ... ]
> 

[PATCH 2/3] can: bcm: do not remove the procfs entry in bcm_notify()

[Oliver Hartkopp]

bcm_notify() removes the procfs entry of a CAN_BCM socket that is bound
to a vanishing CAN device with remove_proc_entry() while holding RTNL.
remove_proc_entry() waits for all running readers of this entry. A
reader of /proc/net/can-bcm/<inode> that stalls in its copy to user
space, e.g. on a FUSE backed mapping, therefore blocks RTNL on the
unregistration or namespace change of the bound CAN device.

The entry has been removed in bcm_notify() so that a later
bcm_connect() of the unbound socket can register it again without a
"proc_dir_entry already registered" warning.

The CAN_BCM socket itself stays usable after a NETDEV_UNREGISTER:
bcm_notify() only unbinds it and reports ENODEV, so the application can
connect() the socket again, e.g. to a re-created CAN device. The name
of the procfs entry is the inode number of the socket, which does not
change until the socket is closed.

Keep the procfs entry until bcm_release() instead, which removes it
after the socket has left the notifier list and without holding RTNL.
bcm_connect() only creates the entry, if it does not exist yet.
bcm_proc_show() reports an unbound socket as "unbound" instead of
showing the cleared ifindex as "any".

Fixes: 76fe372ccb81 ("can: bcm: Remove proc entry when dev is unregistered.")
Cc: stable@vger.kernel.org
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 9c39f7363ca5..0631293a8b54 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -221,11 +221,15 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 
 	seq_printf(m, ">>> sock inode %s", bo->procname);
 	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
-	seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));
+	if (READ_ONCE(bo->bound))
+		seq_printf(m, " / bound %s",
+			   bcm_proc_getifname(net, ifname, bo->ifindex));
+	else
+		seq_puts(m, " / unbound");
 	seq_printf(m, " <<<\n");
 
 	rcu_read_lock();
 
 	list_for_each_entry_rcu(op, &bo->rx_ops, list) {
@@ -1864,16 +1868,10 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 			if (op->ifindex == dev->ifindex)
 				hrtimer_cancel(&op->timer);
 
 		/* remove device reference, if this is our bound device */
 		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
 			/* Paired with the lockless fast-path check in
 			 * bcm_sendmsg(); bo->ifindex itself is only ever
 			 * accessed under lock_sock() so it needs no
 			 * annotation.
 			 */
@@ -2067,11 +2065,12 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 		/* no interface reference for ifindex = 0 ('any' CAN device) */
 		bo->ifindex = 0;
 	}
 
 #if IS_ENABLED(CONFIG_PROC_FS)
-	if (net->can.bcmproc_dir) {
+	/* create the procfs entry once, it is removed in bcm_release() */
+	if (net->can.bcmproc_dir && !bo->bcm_proc_read) {
 		/* use unique socket inode number as filename */
 		snprintf(bo->procname, sizeof(bo->procname),
 			 "%llu", sock_i_ino(sk));
 		bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644,
 						     net->can.bcmproc_dir,

-- 
2.53.0

Re: [PATCH 2/3] can: bcm: do not remove the procfs entry in bcm_notify()

[netdev-bot+sashiko]

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 procfs reader now races with bind and unbind.

--

can: bcm: do not remove the procfs entry in bcm_notify()

bcm_notify() no longer calls remove_proc_entry() while holding RTNL.
The entry now lives from the first bcm_connect() until bcm_release().
bcm_connect() creates the entry only if it does not exist yet, and
bcm_proc_show() reports an unbound socket as "unbound".

> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 9c39f7363ca59..0631293a8b54a 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -223,7 +223,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>  
>  	seq_printf(m, ">>> sock inode %s", bo->procname);
>  	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
> -	seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));
> +	if (READ_ONCE(bo->bound))
> +		seq_printf(m, " / bound %s",
> +			   bcm_proc_getifname(net, ifname, bo->ifindex));

[Severity: Medium]

Can bcm_proc_show() now race with bcm_notify() and bcm_connect() on
bo->bound and bo->ifindex?

Before this patch, bcm_notify() called remove_proc_entry() before it
cleared bo->bound and bo->ifindex. remove_proc_entry() waits for running
readers. bcm_connect() also created a fresh entry only after it had
assigned bo->ifindex. So bcm_proc_show() never ran while bo->ifindex was
being written.

The entry now stays from the first connect() until bcm_release(). That
means these writers can run while a reader is inside bcm_proc_show():

bcm_notify() NETDEV_UNREGISTER:
    WRITE_ONCE(bo->bound, 0);
    bo->ifindex = 0;

bcm_connect():
    bo->ifindex = dev->ifindex;
    ...
    WRITE_ONCE(bo->bound, 1);

Take the unbind case: the reader loads bound == 1, then bcm_notify()
clears both fields, then the reader loads ifindex == 0. In that case
bcm_proc_getifname() returns "any" and the output is "bound any". Isn't
that the output the commit message says this patch removes?

For the rebind case, WRITE_ONCE() and READ_ONCE() don't give
release/acquire ordering. Could a reader see bound == 1 together with
the stale ifindex == 0, and print "any" for a socket bound to a specific
device?

The plain store to bo->ifindex and the plain load in bcm_proc_show()
also look like a data race that KCSAN would report.

The comment this patch keeps in bcm_notify() no longer seems accurate:

    /* Paired with the lockless fast-path check in
     * bcm_sendmsg(); bo->ifindex itself is only ever
     * accessed under lock_sock() so it needs no
     * annotation.
     */

The comment in bcm_sendmsg() relies on the same assumption, that
bo->ifindex is only mutated under lock_sock().

Taking lock_sock() in bcm_proc_show() would not fix this.
bcm_release() calls remove_proc_entry() while holding lock_sock(sk),
and that call waits for active readers:

bcm_release()
    lock_sock(sk);
    ...
    remove_proc_entry(bo->procname, net->can.bcmproc_dir);

Would it work to use WRITE_ONCE()/READ_ONCE() on bo->ifindex and
smp_store_release()/smp_load_acquire() on bo->bound? bcm_proc_show()
could then read one snapshot of ifindex into a local variable.

> +	else
> +		seq_puts(m, " / unbound");
>  	seq_printf(m, " <<<\n");
>  
>  	rcu_read_lock();

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-can-bcmlock-memcg-v1-0-482d9f5be1fd%40hartkopp.net

Re: [PATCH 2/3] can: bcm: do not remove the procfs entry in bcm_notify()

[Oliver Hartkopp]

On 09.11.26 03:18, netdev-bot+sashiko@kernel.org 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 procfs reader now races with bind and unbind.

Addressed in v2:

https://lore.kernel.org/linux-can/20261108-can-bcmlock-memcg-v2-2-19fafdfc9d12@hartkopp.net/

As bcm_proc_show() can now run while the socket is bound or unbound, it
reads bo->ifindex locklessly with READ_ONCE() and the writers use
WRITE_ONCE(). bo->ifindex is still only modified under lock_sock(). A
concurrent bind or unbind may show an intermediate state in the procfs
output, which is acceptable for this debug information.

pw-bot: cr
> 
> --
> 
> can: bcm: do not remove the procfs entry in bcm_notify()
> 
> bcm_notify() no longer calls remove_proc_entry() while holding RTNL.
> The entry now lives from the first bcm_connect() until bcm_release().
> bcm_connect() creates the entry only if it does not exist yet, and
> bcm_proc_show() reports an unbound socket as "unbound".
> 
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 9c39f7363ca59..0631293a8b54a 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -223,7 +223,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>>   
>>   	seq_printf(m, ">>> sock inode %s", bo->procname);
>>   	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
>> -	seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));
>> +	if (READ_ONCE(bo->bound))
>> +		seq_printf(m, " / bound %s",
>> +			   bcm_proc_getifname(net, ifname, bo->ifindex));
> 
> [Severity: Medium]
> 
> Can bcm_proc_show() now race with bcm_notify() and bcm_connect() on
> bo->bound and bo->ifindex?
> 
> Before this patch, bcm_notify() called remove_proc_entry() before it
> cleared bo->bound and bo->ifindex. remove_proc_entry() waits for running
> readers. bcm_connect() also created a fresh entry only after it had
> assigned bo->ifindex. So bcm_proc_show() never ran while bo->ifindex was
> being written.
> 
> The entry now stays from the first connect() until bcm_release(). That
> means these writers can run while a reader is inside bcm_proc_show():
> 
> bcm_notify() NETDEV_UNREGISTER:
>      WRITE_ONCE(bo->bound, 0);
>      bo->ifindex = 0;
> 
> bcm_connect():
>      bo->ifindex = dev->ifindex;
>      ...
>      WRITE_ONCE(bo->bound, 1);
> 
> Take the unbind case: the reader loads bound == 1, then bcm_notify()
> clears both fields, then the reader loads ifindex == 0. In that case
> bcm_proc_getifname() returns "any" and the output is "bound any". Isn't
> that the output the commit message says this patch removes?
> 
> For the rebind case, WRITE_ONCE() and READ_ONCE() don't give
> release/acquire ordering. Could a reader see bound == 1 together with
> the stale ifindex == 0, and print "any" for a socket bound to a specific
> device?
> 
> The plain store to bo->ifindex and the plain load in bcm_proc_show()
> also look like a data race that KCSAN would report.
> 
> The comment this patch keeps in bcm_notify() no longer seems accurate:
> 
>      /* Paired with the lockless fast-path check in
>       * bcm_sendmsg(); bo->ifindex itself is only ever
>       * accessed under lock_sock() so it needs no
>       * annotation.
>       */
> 
> The comment in bcm_sendmsg() relies on the same assumption, that
> bo->ifindex is only mutated under lock_sock().
> 
> Taking lock_sock() in bcm_proc_show() would not fix this.
> bcm_release() calls remove_proc_entry() while holding lock_sock(sk),
> and that call waits for active readers:
> 
> bcm_release()
>      lock_sock(sk);
>      ...
>      remove_proc_entry(bo->procname, net->can.bcmproc_dir);
> 
> Would it work to use WRITE_ONCE()/READ_ONCE() on bo->ifindex and
> smp_store_release()/smp_load_acquire() on bo->bound? bcm_proc_show()
> could then read one snapshot of ifindex into a local variable.
> 
>> +	else
>> +		seq_puts(m, " / unbound");
>>   	seq_printf(m, " <<<\n");
>>   
>>   	rcu_read_lock();
> 
> [ ... ]
> 

[PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oliver Hartkopp]

Unprivileged users can create CAN sockets and trigger allocations that
stay in the kernel for the lifetime of the socket or of the related
objects. E.g. each CAN_BCM RX_SETUP for a new CAN ID creates a bcm_op
with optional frame arrays of up to 257 CAN FD frames and a CAN
receiver, without requiring a CAN device (ifindex 0) and without a
limit for the number of ops per socket.

None of these allocations are accounted to the memory cgroup of the
process that triggers them. So a process can bypass the memory limit
of its cgroup, e.g. of a container or a systemd service, and exhaust
the memory of the whole system. The OOM killer can not attribute this
kernel memory to the responsible process either, which has nearly no
RSS, and terminates unrelated processes instead.

Enable memcg accounting for these allocations, as it has been done for
other objects that can be created by unprivileged users. This does not
introduce any new CAN specific limit and does not make assumptions
about the number of sockets, filters or BCM operations an application
needs. The CAN related kernel memory is just charged to the cgroup of
the allocating process, so that the memory limits which are already
configured for this cgroup apply to it as well. Without a configured
limit the behaviour is unchanged.

A per socket limit for the number or the memory of the BCM operations,
as suggested in the reports, does not bound the memory consumption, as
a process can open many sockets and fork. Hard-coded limits can also
break existing applications, e.g. one that monitors all 2048 SFF CAN
IDs with individual RX operations, without a possibility to adjust
them.

The following allocations are accounted:

- af_can: the CAN socket structures (e.g. struct isotp_sock contains
  two DEFAULT_MAX_PDU_SIZE buffers), the CAN receivers of all CAN
  protocols and the per-netns receive lists and statistics
- bcm: the bcm_op structures and their frames/last_frames arrays
- raw: the per-cpu uniq data and the CAN_RAW_FILTER filters
- isotp: the TX buffer for PDUs > DEFAULT_MAX_PDU_SIZE
- j1939: the SO_J1939_FILTER filters and the per device j1939_priv,
  which is created by the first bind() to a CAN device

The CAN socket structures are allocated with GFP_KERNEL_ACCOUNT in
sk_alloc() instead of creating dedicated slab caches with
proto_register(), as the CAN protocols copy socket options directly
from their socket structures to user space, which would require
usercopy whitelists for these caches.

memdup_sockptr() does not support memcg accounting, so the filters are
allocated and copied explicitly.

The isotp RX buffer and the j1939 RX sessions are allocated in softirq
context and can not be attributed to a process. The j1939 TX sessions
only exist for the duration of a transfer and the can-gw jobs require
CAP_NET_ADMIN in the initial user namespace, so they are not changed.
Temporary buffers that are freed before the syscall returns are not
accounted either.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260707-bcm_fixes-v5-0-c3c74f8d8ff7@hartkopp.net
Reported-by: Zihan Xi <zihanx@nebusec.ai>
Closes: https://lore.kernel.org/linux-can/cover.1793089479.git.zihanx@nebusec.ai/
Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/af_can.c       | 12 +++++++-----
 net/can/bcm.c          | 10 +++++-----
 net/can/isotp.c        |  2 +-
 net/can/j1939/main.c   |  2 +-
 net/can/j1939/socket.c | 11 ++++++++---
 net/can/raw.c          | 13 +++++++++----
 6 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 0f3051a61aab..6f958c2bb49d 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -153,11 +153,11 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
 		goto errout;
 	}
 
 	sock->ops = cp->ops;
 
-	sk = sk_alloc(net, PF_CAN, GFP_KERNEL, cp->prot, kern);
+	sk = sk_alloc(net, PF_CAN, GFP_KERNEL_ACCOUNT, cp->prot, kern);
 	if (!sk) {
 		err = -ENOMEM;
 		goto errout;
 	}
 
@@ -828,18 +828,20 @@ void can_proto_unregister(const struct can_proto *cp)
 EXPORT_SYMBOL(can_proto_unregister);
 
 static int can_pernet_init(struct net *net)
 {
 	spin_lock_init(&net->can.rcvlists_lock);
-	net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list);
+	net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list,
+					      GFP_KERNEL_ACCOUNT);
 	if (!net->can.rx_alldev_list)
 		goto out;
 	spin_lock_init(&net->can.rx_alldev_list->lock);
-	net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats);
+	net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats, GFP_KERNEL_ACCOUNT);
 	if (!net->can.pkg_stats)
 		goto out_free_rx_alldev_list;
-	net->can.rcv_lists_stats = kzalloc_obj(*net->can.rcv_lists_stats);
+	net->can.rcv_lists_stats = kzalloc_obj(*net->can.rcv_lists_stats,
+					       GFP_KERNEL_ACCOUNT);
 	if (!net->can.rcv_lists_stats)
 		goto out_free_pkg_stats;
 
 	if (IS_ENABLED(CONFIG_PROC_FS)) {
 		/* the statistics are updated every second (timer triggered) */
@@ -917,11 +919,11 @@ static __init int can_init(void)
 		     offsetof(struct canfd_frame, data));
 
 	pr_info("can: controller area network core\n");
 
 	rcv_cache = kmem_cache_create("can_receiver", sizeof(struct receiver),
-				      0, 0, NULL);
+				      0, SLAB_ACCOUNT, NULL);
 	if (!rcv_cache)
 		return -ENOMEM;
 
 	err = register_pernet_subsys(&can_pernet_ops);
 	if (err)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 0631293a8b54..83c1b6ee6d27 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1157,11 +1157,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		kfree(new_frames);
 
 	} else {
 		/* insert new BCM operation for the given can_id */
 
-		op = kzalloc(OPSIZ, GFP_KERNEL);
+		op = kzalloc(OPSIZ, GFP_KERNEL_ACCOUNT);
 		if (!op)
 			return -ENOMEM;
 
 		spin_lock_init(&op->bcm_tx_lock);
 		op->can_id = msg_head->can_id;
@@ -1174,11 +1174,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 		/* create array for CAN frames and copy the data */
 		if (msg_head->nframes > 1) {
 			op->frames = kmalloc_array(msg_head->nframes,
 						   op->cfsiz,
-						   GFP_KERNEL);
+						   GFP_KERNEL_ACCOUNT);
 			if (!op->frames) {
 				kfree(op);
 				return -ENOMEM;
 			}
 		} else
@@ -1405,11 +1405,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		 */
 		do_rx_register = (ifindex && !op->rx_reg_dev) ? 1 : 0;
 
 	} else {
 		/* insert new BCM operation for the given can_id */
-		op = kzalloc(OPSIZ, GFP_KERNEL);
+		op = kzalloc(OPSIZ, GFP_KERNEL_ACCOUNT);
 		if (!op)
 			return -ENOMEM;
 
 		spin_lock_init(&op->bcm_tx_lock);
 		spin_lock_init(&op->bcm_rx_update_lock);
@@ -1420,20 +1420,20 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 		if (msg_head->nframes > 1) {
 			/* create array for CAN frames and copy the data */
 			op->frames = kmalloc_array(msg_head->nframes,
 						   op->cfsiz,
-						   GFP_KERNEL);
+						   GFP_KERNEL_ACCOUNT);
 			if (!op->frames) {
 				kfree(op);
 				return -ENOMEM;
 			}
 
 			/* create and init array for received CAN frames */
 			op->last_frames = kcalloc(msg_head->nframes,
 						  op->cfsiz,
-						  GFP_KERNEL);
+						  GFP_KERNEL_ACCOUNT);
 			if (!op->last_frames) {
 				kfree(op->frames);
 				kfree(op);
 				return -ENOMEM;
 			}
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 7a90ba3f1cf1..34cccb480141 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1182,11 +1182,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 		goto err_out_drop;
 	}
 
 	/* PDU size > default => try max_pdu_size */
 	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
-		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL_ACCOUNT);
 
 		if (newbuf) {
 			so->tx.buf = newbuf;
 			so->tx.buflen = max_pdu_size;
 		}
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index d9384cf0e356..77adb817b36c 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -126,11 +126,11 @@ static DEFINE_MUTEX(j1939_netdev_lock);
 
 static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
 {
 	struct j1939_priv *priv;
 
-	priv = kzalloc_obj(*priv);
+	priv = kzalloc_obj(*priv, GFP_KERNEL_ACCOUNT);
 	if (!priv)
 		return NULL;
 
 	rwlock_init(&priv->lock);
 	INIT_LIST_HEAD(&priv->ecus);
diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 50a598ef5fd4..c83246587604 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -714,13 +714,18 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
 			if (optlen > J1939_FILTER_MAX *
 			    sizeof(struct j1939_filter))
 				return -EINVAL;
 
 			count = optlen / sizeof(*filters);
-			filters = memdup_sockptr(optval, optlen);
-			if (IS_ERR(filters))
-				return PTR_ERR(filters);
+			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
+			if (!filters)
+				return -ENOMEM;
+
+			if (copy_from_sockptr(filters, optval, optlen)) {
+				kfree(filters);
+				return -EFAULT;
+			}
 
 			for (f = filters, c = count; c; f++, c--) {
 				f->name &= f->name_mask;
 				f->pgn &= f->pgn_mask;
 				f->addr &= f->addr_mask;
diff --git a/net/can/raw.c b/net/can/raw.c
index f14270b56f52..dd5b9600bad9 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -414,11 +414,11 @@ static int raw_init(struct sock *sk)
 	ro->fd_frames        = 0;
 	ro->xl_frames        = 0;
 	ro->join_filters     = 0;
 
 	/* alloc_percpu provides zero'ed memory */
-	ro->uniq = alloc_percpu(struct uniqframe);
+	ro->uniq = alloc_percpu_gfp(struct uniqframe, GFP_KERNEL_ACCOUNT);
 	if (unlikely(!ro->uniq))
 		return -ENOMEM;
 
 	sk->sk_destruct = raw_sock_destruct;
 
@@ -758,13 +758,18 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		count = optlen / sizeof(struct can_filter);
 
 		if (count > 1) {
 			/* filter does not fit into dfilter => alloc space */
-			filter = memdup_sockptr(optval, optlen);
-			if (IS_ERR(filter))
-				return PTR_ERR(filter);
+			filter = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
+			if (!filter)
+				return -ENOMEM;
+
+			if (copy_from_sockptr(filter, optval, optlen)) {
+				kfree(filter);
+				return -EFAULT;
+			}
 		} else if (count == 1) {
 			if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
 				return -EFAULT;
 			filter = &sfilter;
 		}

-- 
2.53.0

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[sashiko-bot]

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:
- [Medium] can: j1939: non-zero setsockopt return value in
  j1939_sk_setsockopt_flag() [1]
--

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

can: enable memcg accounting for user triggered allocations

This commit enables memcg accounting via GFP_KERNEL_ACCOUNT and
SLAB_ACCOUNT for user-triggered allocations in the CAN core, BCM, ISOTP,
J1939, and RAW socket protocols.

Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/

> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index 50a598ef5fd4a..c832465876049 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -716,9 +716,14 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
>  				return -EINVAL;
>  
>  			count = optlen / sizeof(*filters);
> -			filters = memdup_sockptr(optval, optlen);
> -			if (IS_ERR(filters))
> -				return PTR_ERR(filters);
> +			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
> +			if (!filters)
> +				return -ENOMEM;
> +
> +			if (copy_from_sockptr(filters, optval, optlen)) {
> +				kfree(filters);
> +				return -EFAULT;
> +			}
>  
>  			for (f = filters, c = count; c; f++, c--) {
>  				f->name &= f->name_mask;

[Severity: Medium]
This problem wasn't introduced by this patch, but should
j1939_sk_setsockopt_flag() return 0 instead of tmp [1]?

net/can/j1939/socket.c:j1939_sk_setsockopt_flag() {
    ...
	lock_sock(&jsk->sk);
	if (tmp)
		jsk->state |= flag;
	else
		jsk->state &= ~flag;
	release_sock(&jsk->sk);
	return tmp;
}

When userspace enables SO_J1939_PROMISC or SO_J1939_ERRQUEUE with a
non-zero value in j1939_sk_setsockopt(), that value is returned to
userspace from setsockopt(), breaking callers that check for a 0 return on
success.

[1] https://sashiko.dev/#/bug/linux-35e22274-4165-4b2b-985e-c5f5773b9ef9

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

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oliver Hartkopp]

On 07.11.26 14:14, sashiko-bot@kernel.org wrote:
> 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:
> - [Medium] can: j1939: non-zero setsockopt return value in
>    j1939_sk_setsockopt_flag() [1]
> --
> 
> commit b3ee6688711331e68a7e275f9b72d5b3fa8b7ee5
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> can: enable memcg accounting for user triggered allocations
> 
> This commit enables memcg accounting via GFP_KERNEL_ACCOUNT and
> SLAB_ACCOUNT for user-triggered allocations in the CAN core, BCM, ISOTP,
> J1939, and RAW socket protocols.
> 
> Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/
> 
>> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
>> index 50a598ef5fd4a..c832465876049 100644
>> --- a/net/can/j1939/socket.c
>> +++ b/net/can/j1939/socket.c
>> @@ -716,9 +716,14 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
>>   				return -EINVAL;
>>   
>>   			count = optlen / sizeof(*filters);
>> -			filters = memdup_sockptr(optval, optlen);
>> -			if (IS_ERR(filters))
>> -				return PTR_ERR(filters);
>> +			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
>> +			if (!filters)
>> +				return -ENOMEM;
>> +
>> +			if (copy_from_sockptr(filters, optval, optlen)) {
>> +				kfree(filters);
>> +				return -EFAULT;
>> +			}
>>   
>>   			for (f = filters, c = count; c; f++, c--) {
>>   				f->name &= f->name_mask;
> 
> [Severity: Medium]
> This problem wasn't introduced by this patch, but should
> j1939_sk_setsockopt_flag() return 0 instead of tmp [1]?

This might be a can-next topic and has nothing to do with this patch.

@Oleksij: Would you like to take this suggestion?

Best regards,
Oliver
> 
> net/can/j1939/socket.c:j1939_sk_setsockopt_flag() {
>      ...
> 	lock_sock(&jsk->sk);
> 	if (tmp)
> 		jsk->state |= flag;
> 	else
> 		jsk->state &= ~flag;
> 	release_sock(&jsk->sk);
> 	return tmp;
> }
> 
> When userspace enables SO_J1939_PROMISC or SO_J1939_ERRQUEUE with a
> non-zero value in j1939_sk_setsockopt(), that value is returned to
> userspace from setsockopt(), breaking callers that check for a 0 return on
> success.
> 
> [1] https://sashiko.dev/#/bug/linux-35e22274-4165-4b2b-985e-c5f5773b9ef9
> 

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oleksij Rempel]

On Sat, Nov 07, 2026 at 02:35:20PM +0100, Oliver Hartkopp wrote:
> 
> 
> On 07.11.26 14:14, sashiko-bot@kernel.org wrote:
> > 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:
> > - [Medium] can: j1939: non-zero setsockopt return value in
> >    j1939_sk_setsockopt_flag() [1]
> > --
> > 
> > commit b3ee6688711331e68a7e275f9b72d5b3fa8b7ee5
> > Author: Oliver Hartkopp <socketcan@hartkopp.net>
> > 
> > can: enable memcg accounting for user triggered allocations
> > 
> > This commit enables memcg accounting via GFP_KERNEL_ACCOUNT and
> > SLAB_ACCOUNT for user-triggered allocations in the CAN core, BCM, ISOTP,
> > J1939, and RAW socket protocols.
> > 
> > Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/
> > 
> > > diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> > > index 50a598ef5fd4a..c832465876049 100644
> > > --- a/net/can/j1939/socket.c
> > > +++ b/net/can/j1939/socket.c
> > > @@ -716,9 +716,14 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
> > >   				return -EINVAL;
> > >   			count = optlen / sizeof(*filters);
> > > -			filters = memdup_sockptr(optval, optlen);
> > > -			if (IS_ERR(filters))
> > > -				return PTR_ERR(filters);
> > > +			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
> > > +			if (!filters)
> > > +				return -ENOMEM;
> > > +
> > > +			if (copy_from_sockptr(filters, optval, optlen)) {
> > > +				kfree(filters);
> > > +				return -EFAULT;
> > > +			}
> > >   			for (f = filters, c = count; c; f++, c--) {
> > >   				f->name &= f->name_mask;
> > 
> > [Severity: Medium]
> > This problem wasn't introduced by this patch, but should
> > j1939_sk_setsockopt_flag() return 0 instead of tmp [1]?
> 
> This might be a can-next topic and has nothing to do with this patch.
> 
> @Oleksij: Would you like to take this suggestion?

Ack, on my todo.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oleksij Rempel]

On Sat, Nov 07, 2026 at 01:14:06PM +0000, sashiko-bot@kernel.org wrote:
> 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:
> - [Medium] can: j1939: non-zero setsockopt return value in
>   j1939_sk_setsockopt_flag() [1]

Thx!

Adding to my endless TODO, except some one will address it.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oleksij Rempel]

On Sat, Nov 07, 2026 at 02:06:46PM +0100, Oliver Hartkopp wrote:
> Unprivileged users can create CAN sockets and trigger allocations that
> stay in the kernel for the lifetime of the socket or of the related
> objects. E.g. each CAN_BCM RX_SETUP for a new CAN ID creates a bcm_op
> with optional frame arrays of up to 257 CAN FD frames and a CAN
> receiver, without requiring a CAN device (ifindex 0) and without a
> limit for the number of ops per socket.
> 
> None of these allocations are accounted to the memory cgroup of the
> process that triggers them. So a process can bypass the memory limit
> of its cgroup, e.g. of a container or a systemd service, and exhaust
> the memory of the whole system. The OOM killer can not attribute this
> kernel memory to the responsible process either, which has nearly no
> RSS, and terminates unrelated processes instead.
> 
> Enable memcg accounting for these allocations, as it has been done for
> other objects that can be created by unprivileged users. This does not
> introduce any new CAN specific limit and does not make assumptions
> about the number of sockets, filters or BCM operations an application
> needs. The CAN related kernel memory is just charged to the cgroup of
> the allocating process, so that the memory limits which are already
> configured for this cgroup apply to it as well. Without a configured
> limit the behaviour is unchanged.
> 
> A per socket limit for the number or the memory of the BCM operations,
> as suggested in the reports, does not bound the memory consumption, as
> a process can open many sockets and fork. Hard-coded limits can also
> break existing applications, e.g. one that monitors all 2048 SFF CAN
> IDs with individual RX operations, without a possibility to adjust
> them.
> 
> The following allocations are accounted:
> 
> - af_can: the CAN socket structures (e.g. struct isotp_sock contains
>   two DEFAULT_MAX_PDU_SIZE buffers), the CAN receivers of all CAN
>   protocols and the per-netns receive lists and statistics
> - bcm: the bcm_op structures and their frames/last_frames arrays
> - raw: the per-cpu uniq data and the CAN_RAW_FILTER filters
> - isotp: the TX buffer for PDUs > DEFAULT_MAX_PDU_SIZE
> - j1939: the SO_J1939_FILTER filters and the per device j1939_priv,
>   which is created by the first bind() to a CAN device
> 
> The CAN socket structures are allocated with GFP_KERNEL_ACCOUNT in
> sk_alloc() instead of creating dedicated slab caches with
> proto_register(), as the CAN protocols copy socket options directly
> from their socket structures to user space, which would require
> usercopy whitelists for these caches.
> 
> memdup_sockptr() does not support memcg accounting, so the filters are
> allocated and copied explicitly.
> 
> The isotp RX buffer and the j1939 RX sessions are allocated in softirq
> context and can not be attributed to a process. The j1939 TX sessions
> only exist for the duration of a transfer and the can-gw jobs require
> CAP_NET_ADMIN in the initial user namespace, so they are not changed.
> Temporary buffers that are freed before the syscall returns are not
> accounted either.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260707-bcm_fixes-v5-0-c3c74f8d8ff7@hartkopp.net
> Reported-by: Zihan Xi <zihanx@nebusec.ai>
> Closes: https://lore.kernel.org/linux-can/cover.1793089479.git.zihanx@nebusec.ai/
> Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  net/can/af_can.c       | 12 +++++++-----
>  net/can/bcm.c          | 10 +++++-----
>  net/can/isotp.c        |  2 +-
>  net/can/j1939/main.c   |  2 +-
>  net/can/j1939/socket.c | 11 ++++++++---

For the J1939 part:
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch says it accounts the j1939 objects that unprivileged
  users trigger and that live as long as the socket or a related object.
- [Low] The patch replaces memdup_sockptr() in raw_setsockopt()
  (CAN_RAW_FILTER) and j1939_sk_setsockopt() (SO_J1939_FILTER) with an…

--

can: enable memcg accounting for user triggered allocations

This commit adds memcg accounting to CAN allocations that unprivileged
users can trigger and that live for the lifetime of a socket or related
object. It covers socket structures, receivers, bcm_op frame arrays, raw
and j1939 filters, the isotp TX buffer and j1939_priv. memdup_sockptr()
is replaced with an explicit kmalloc() plus copy_from_sockptr() for the
filter copies.

Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index d9384cf0e356f..77adb817b36c8 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -128,7 +128,7 @@ static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
>  {
>  	struct j1939_priv *priv;
>  
> -	priv = kzalloc_obj(*priv);
> +	priv = kzalloc_obj(*priv, GFP_KERNEL_ACCOUNT);
>  	if (!priv)
>  		return NULL;
>  

[Severity: Low]
The commit message names j1939_priv as the j1939 object that bind()
creates. Is the local ECU, created on the same bind() path, meant to stay
unaccounted?

j1939_sk_bind()
  j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa)
    write_lock_bh(&priv->lock);
    j1939_ecu_create_locked(priv, name)
      ecu = kzalloc_obj(*ecu, gfp_any());

gfp_any() returns GFP_ATOMIC here because BH is disabled. The code is
still in task context, though, running on behalf of the process that
called bind(). So the softirq reasoning in the commit message ("can not
be attributed to a process") does not seem to apply to this call site.

The ECU holds a kref, a netdev_hold() tracker and an hrtimer. It stays
alive until the socket unbinds, rebinds or is released through
j1939_local_ecu_put().

Should this allocation get __GFP_ACCOUNT as well? If not, could the
commit message list it with the other deliberate exclusions?

> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
> index 50a598ef5fd4a..c832465876049 100644
> --- a/net/can/j1939/socket.c
> +++ b/net/can/j1939/socket.c
> @@ -716,9 +716,14 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
>  				return -EINVAL;
>  
>  			count = optlen / sizeof(*filters);
> -			filters = memdup_sockptr(optval, optlen);
> -			if (IS_ERR(filters))
> -				return PTR_ERR(filters);
> +			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);

[Severity: Low]
memdup_sockptr() allocates with GFP_USER | __GFP_NOWARN:

include/linux/sockptr.h:memdup_sockptr_noprof() {
	void *p = kmalloc_track_caller_noprof(len, GFP_USER | __GFP_NOWARN);
	...
}

As well as adding __GFP_ACCOUNT, GFP_KERNEL_ACCOUNT drops __GFP_HARDWALL
and __GFP_NOWARN. Was that intended?

Userspace controls optlen here, up to J1939_FILTER_MAX *
sizeof(struct j1939_filter), which is 16 KiB. That size becomes an
order-2 page allocation, so a failure can now print a warn_alloc()
splat.

The same change applies to the CAN_RAW_FILTER allocation in
raw_setsockopt(), where the size can reach 4 KiB.

Would GFP_USER | __GFP_ACCOUNT | __GFP_NOWARN keep the old behavior for
both call sites?

> +			if (!filters)
> +				return -ENOMEM;
> +
> +			if (copy_from_sockptr(filters, optval, optlen)) {
> +				kfree(filters);
> +				return -EFAULT;
> +			}
>  
>  			for (f = filters, c = count; c; f++, c--) {
>  				f->name &= f->name_mask;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-can-bcmlock-memcg-v1-0-482d9f5be1fd%40hartkopp.net

Re: [PATCH 3/3] can: enable memcg accounting for user triggered allocations

[Oliver Hartkopp]

On 09.11.26 03:18, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The patch says it accounts the j1939 objects that unprivileged
>    users trigger and that live as long as the socket or a related object.
> - [Low] The patch replaces memdup_sockptr() in raw_setsockopt()
>    (CAN_RAW_FILTER) and j1939_sk_setsockopt() (SO_J1939_FILTER) with an…
> 
> --
> 
> can: enable memcg accounting for user triggered allocations
> 
> This commit adds memcg accounting to CAN allocations that unprivileged
> users can trigger and that live for the lifetime of a socket or related
> object. It covers socket structures, receivers, bcm_op frame arrays, raw
> and j1939 filters, the isotp TX buffer and j1939_priv. memdup_sockptr()
> is replaced with an explicit kmalloc() plus copy_from_sockptr() for the
> filter copies.
> 
> Link: https://lore.kernel.org/linux-can/350df96b-5f69-490e-b39e-ed6e9bca4754@hartkopp.net/
> 
> [ ... ]
> 
>> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
>> index d9384cf0e356f..77adb817b36c8 100644
>> --- a/net/can/j1939/main.c
>> +++ b/net/can/j1939/main.c
>> @@ -128,7 +128,7 @@ static struct j1939_priv *j1939_priv_create(struct net_device *ndev)
>>   {
>>   	struct j1939_priv *priv;
>>   
>> -	priv = kzalloc_obj(*priv);
>> +	priv = kzalloc_obj(*priv, GFP_KERNEL_ACCOUNT);
>>   	if (!priv)
>>   		return NULL;
>>   
> 
> [Severity: Low]
> The commit message names j1939_priv as the j1939 object that bind()
> creates. Is the local ECU, created on the same bind() path, meant to stay
> unaccounted?

The local j1939 ECUs created by bind() are not accounted, as they are
allocated with BH disabled and their number is limited by the number
of bound j1939 sockets, which are accounted.

Added this comment to v2 commit message:

https://lore.kernel.org/linux-can/20261108-can-bcmlock-memcg-v2-3-19fafdfc9d12@hartkopp.net/

No code change here.
> 
> j1939_sk_bind()
>    j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa)
>      write_lock_bh(&priv->lock);
>      j1939_ecu_create_locked(priv, name)
>        ecu = kzalloc_obj(*ecu, gfp_any());
> 
> gfp_any() returns GFP_ATOMIC here because BH is disabled. The code is
> still in task context, though, running on behalf of the process that
> called bind(). So the softirq reasoning in the commit message ("can not
> be attributed to a process") does not seem to apply to this call site.
> 
> The ECU holds a kref, a netdev_hold() tracker and an hrtimer. It stays
> alive until the socket unbinds, rebinds or is released through
> j1939_local_ecu_put().
> 
> Should this allocation get __GFP_ACCOUNT as well? If not, could the
> commit message list it with the other deliberate exclusions?
> 
>> diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
>> index 50a598ef5fd4a..c832465876049 100644
>> --- a/net/can/j1939/socket.c
>> +++ b/net/can/j1939/socket.c
>> @@ -716,9 +716,14 @@ static int j1939_sk_setsockopt(struct socket *sock, int level, int optname,
>>   				return -EINVAL;
>>   
>>   			count = optlen / sizeof(*filters);
>> -			filters = memdup_sockptr(optval, optlen);
>> -			if (IS_ERR(filters))
>> -				return PTR_ERR(filters);
>> +			filters = kmalloc(optlen, GFP_KERNEL_ACCOUNT);
> 
> [Severity: Low]
> memdup_sockptr() allocates with GFP_USER | __GFP_NOWARN:
> 
> include/linux/sockptr.h:memdup_sockptr_noprof() {
> 	void *p = kmalloc_track_caller_noprof(len, GFP_USER | __GFP_NOWARN);
> 	...
> }
> 
> As well as adding __GFP_ACCOUNT, GFP_KERNEL_ACCOUNT drops __GFP_HARDWALL
> and __GFP_NOWARN. Was that intended?
> 
> Userspace controls optlen here, up to J1939_FILTER_MAX *
> sizeof(struct j1939_filter), which is 16 KiB. That size becomes an
> order-2 page allocation, so a failure can now print a warn_alloc()
> splat.
> 
> The same change applies to the CAN_RAW_FILTER allocation in
> raw_setsockopt(), where the size can reach 4 KiB.
> 
> Would GFP_USER | __GFP_ACCOUNT | __GFP_NOWARN keep the old behavior for
> both call sites?

Good catch!

Fixed in v2 in j1939 and raw.
+			filter = kmalloc(optlen, GFP_USER | __GFP_NOWARN |
+					 __GFP_ACCOUNT);

https://lore.kernel.org/linux-can/20261108-can-bcmlock-memcg-v2-3-19fafdfc9d12@hartkopp.net/

pw-bot: cr

> 
>> +			if (!filters)
>> +				return -ENOMEM;
>> +
>> +			if (copy_from_sockptr(filters, optval, optlen)) {
>> +				kfree(filters);
>> +				return -EFAULT;
>> +			}
>>   
>>   			for (f = filters, c = count; c; f++, c--) {
>>   				f->name &= f->name_mask;
> 
> [ ... ]
>