[PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Oliver Hartkopp]

Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct
net_device") introduced an explicit way to assign the midlayer private
pointer (dev->ml_priv) to named users like ML_PRIV_CAN. A valid pointer
returned by can_get_ml_priv() therefore identifies a real CAN device.

The referenced commit only converted the j1939 and proc code to this
check, while the other parts of the CAN subsystem still rely on
dev->type == ARPHRD_CAN.

The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware
type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed
netdevice passes the ARPHRD_CAN checks in the CAN subsystem, which can
lead to a NULL pointer dereference of the CAN specific ml_priv. This
only requires owning a TUN/TAP device or CAP_NET_ADMIN in an
unprivileged user namespace.

can_rx_register() already rejects such a device since commit 8aa59e355949
("can: af_can: prevent NULL pointer dereference in can_rx_register()"),
but can_rx_unregister() only checks dev->type and dereferences the NULL
ml_priv in can_dev_rcv_lists_find().

This path is reachable via isotp: isotp_bind() only checked for
ARPHRD_CAN and ignored the -ENODEV from can_rx_register(), so the socket
was marked as bound to the TUN device. A later isotp_release() or
NETDEV_UNREGISTER notification then crashes in can_rx_unregister().

Replace all remaining ARPHRD_CAN checks in net/can with
can_get_ml_priv(). For can_rx_register() and the can*_rcv() functions
this is a simplification without functional change.

Fixes: 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")
Cc: stable@kernel.org
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 15 ++++++---------
 net/can/bcm.c    |  7 ++++---
 net/can/gw.c     |  7 ++++---
 net/can/isotp.c  |  5 +++--
 net/can/raw.c    |  4 ++--
 5 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 34fe3b28d576..dc27ace43719 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -224,11 +224,11 @@ int can_send(struct sk_buff *skb, int loop)
 	if (unlikely(skb->len > READ_ONCE(skb->dev->mtu))) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
-	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+	if (unlikely(!can_get_ml_priv(skb->dev))) {
 		err = -EPERM;
 		goto inval_skb;
 	}
 
 	if (unlikely(!(skb->dev->flags & IFF_UP))) {
@@ -450,11 +450,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 
 	/* insert new receiver  (dev,canid,mask) -> (func,data) */
 
-	if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
+	if (dev && !can_get_ml_priv(dev))
 		return -ENODEV;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return -ENODEV;
 
@@ -517,11 +517,11 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *rcv = NULL;
 	struct hlist_head *rcv_list;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
-	if (dev && dev->type != ARPHRD_CAN)
+	if (dev && !can_get_ml_priv(dev))
 		return;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return;
 
@@ -682,12 +682,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_can_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_can_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CAN_RX_INVALID_FRAME);
 		return NET_RX_DROP;
@@ -708,12 +707,11 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canfd_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canfd_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANFD_RX_INVALID_FRAME);
 		return NET_RX_DROP;
@@ -734,12 +732,11 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canxl_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canxl_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANXL_RX_INVALID_FRAME);
 		return NET_RX_DROP;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..60406439a13f 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -52,10 +52,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
@@ -1717,11 +1718,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			if (!dev) {
 				ret = -ENODEV;
 				goto out_release;
 			}
 
-			if (dev->type != ARPHRD_CAN) {
+			if (!can_get_ml_priv(dev)) {
 				dev_put(dev);
 				ret = -ENODEV;
 				goto out_release;
 			}
 
@@ -1865,11 +1866,11 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(bcm_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;
@@ -2022,11 +2023,11 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 		dev = dev_get_by_index(net, addr->can_ifindex);
 		if (!dev) {
 			ret = -ENODEV;
 			goto fail;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			dev_put(dev);
 			ret = -ENODEV;
 			goto fail;
 		}
 
diff --git a/net/can/gw.c b/net/can/gw.c
index 5793cb2420ed..54bb5bd3242a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -50,10 +50,11 @@
 #include <linux/net.h>
 #include <linux/netdevice.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/gw.h>
 #include <net/can.h>
 #include <net/rtnetlink.h>
@@ -610,11 +611,11 @@ static int cgw_notifier(struct notifier_block *nb,
 			unsigned long msg, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net *net = dev_net(dev);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 
 	if (msg == NETDEV_UNREGISTER) {
 		struct cgw_job *gwj = NULL;
 		struct hlist_node *nx;
@@ -1161,19 +1162,19 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	gwj->src.dev = __dev_get_by_index(net, gwj->ccgw.src_idx);
 
 	if (!gwj->src.dev)
 		goto out;
 
-	if (gwj->src.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->src.dev))
 		goto out;
 
 	gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx);
 
 	if (!gwj->dst.dev)
 		goto out;
 
-	if (gwj->dst.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->dst.dev))
 		goto out;
 
 	/* is sending the skb back to the incoming interface intended? */
 	if (gwj->src.dev == gwj->dst.dev &&
 	    !(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK)) {
diff --git a/net/can/isotp.c b/net/can/isotp.c
index f5dc9d04bd68..6c28802c0605 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -63,10 +63,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
 #include <linux/slab.h>
 #include <net/can.h>
@@ -1608,11 +1609,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	dev = dev_get_by_index(net, addr->can_ifindex);
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
 		err = -ENODEV;
 		goto out_put_dev;
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
 		err = -EINVAL;
@@ -1895,11 +1896,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 			  void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(isotp_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;
diff --git a/net/can/raw.c b/net/can/raw.c
index 0acd4f6c6dd6..e1d741a25be3 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -346,11 +346,11 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 static int raw_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(raw_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;
@@ -489,11 +489,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
 
 		if (!(dev->flags & IFF_UP))
-- 
2.53.0

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: isotp: silent bind() failure if can_rx_register() returns -ENOMEM
--

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

can: replace ARPHRD_CAN type checks with can_get_ml_priv()

This commit replaces explicit ARPHRD_CAN hardware type checks with
can_get_ml_priv() to prevent NULL pointer dereferences when a TUN/TAP
device is repurposed to mimic a CAN device.

Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index f5dc9d04bd686..6c28802c0605f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1610,7 +1611,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		err = -ENODEV;
>  		goto out;
>  	}
> -	if (dev->type != ARPHRD_CAN) {
> +	if (!can_get_ml_priv(dev)) {
>  		err = -ENODEV;
>  		goto out_put_dev;
>  	}

[Severity: Medium]
A few lines lower in isotp_bind(), the return values of can_rx_register()
are ignored:

	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, "isotp", sk);

	/* no consecutive frame echo skb in flight */
	WRITE_ONCE(so->cfecho, 0);

	/* register for echo skb's */
	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
			isotp_rcv_echo, sk, "isotpe", sk);

If can_rx_register() fails (for example, returning -ENOMEM due to a failed
kmem_cache_alloc()), bind() will succeed but the socket is not fully
registered to receive CAN frames. This leads to a silent failure where
user-space believes the socket is successfully bound, but no traffic will
ever be received.

Could this regression be prevented by checking the return values and
propagating the error appropriately, or cleaning up any partial
registrations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016093525.327367-1-socketcan@hartkopp.net?part=1

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Oliver Hartkopp]

On 16.10.26 11:50, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] can: isotp: silent bind() failure if can_rx_register() returns -ENOMEM
> --
> 
> commit 95cbcd2505f7f236f0737cc7471505243facbfe5
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> can: replace ARPHRD_CAN type checks with can_get_ml_priv()
> 
> This commit replaces explicit ARPHRD_CAN hardware type checks with
> can_get_ml_priv() to prevent NULL pointer dereferences when a TUN/TAP
> device is repurposed to mimic a CAN device.
> 
> Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index f5dc9d04bd686..6c28802c0605f 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -1610,7 +1611,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>>   		err = -ENODEV;
>>   		goto out;
>>   	}
>> -	if (dev->type != ARPHRD_CAN) {
>> +	if (!can_get_ml_priv(dev)) {
>>   		err = -ENODEV;
>>   		goto out_put_dev;
>>   	}
> 
> [Severity: Medium]
> A few lines lower in isotp_bind(), the return values of can_rx_register()
> are ignored:
> 
> 	if (isotp_register_rxid(so))
> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> 				isotp_rcv, sk, "isotp", sk);
> 
> 	/* no consecutive frame echo skb in flight */
> 	WRITE_ONCE(so->cfecho, 0);
> 
> 	/* register for echo skb's */
> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> 			isotp_rcv_echo, sk, "isotpe", sk);
> 
> If can_rx_register() fails (for example, returning -ENOMEM due to a failed
> kmem_cache_alloc()), bind() will succeed but the socket is not fully
> registered to receive CAN frames. This leads to a silent failure where
> user-space believes the socket is successfully bound, but no traffic will
> ever be received.
> 
> Could this regression be prevented by checking the return values and
> propagating the error appropriately, or cleaning up any partial
> registrations?
> 
When this patch is applied isotp_bind() can only fail with -ENOMEM and 
nothing breaks. The socket just becomes unusable. And if we have -ENOMEM 
we have a big problem anyway.

If I would provide such a patch the proc pointer removal patch would 
break next.

I have ~20 patches for isotp and its CAN XL support. I will add the 
isotp_bind() error handling there when these current (can-next) fixes 
are in mainline.

Best regards,
Oliver

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Marc Kleine-Budde]

> Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct
> net_device") introduced an explicit way to assign the midlayer private
> pointer (dev->ml_priv) to named users like ML_PRIV_CAN. A valid pointer
> returned by can_get_ml_priv() therefore identifies a real CAN device.
>
> The referenced commit only converted the j1939 and proc code to this
> check, while the other parts of the CAN subsystem still rely on
> dev->type == ARPHRD_CAN.
>
> The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware
> type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed
> netdevice passes the ARPHRD_CAN checks in the CAN subsystem, which can
> lead to a NULL pointer dereference of the CAN specific ml_priv. This
> only requires owning a TUN/TAP device or CAP_NET_ADMIN in an
> unprivileged user namespace.
>
> can_rx_register() already rejects such a device since commit 8aa59e355949
> ("can: af_can: prevent NULL pointer dereference in can_rx_register()"),
> but can_rx_unregister() only checks dev->type and dereferences the NULL
> ml_priv in can_dev_rcv_lists_find().
>
> This path is reachable via isotp: isotp_bind() only checked for
> ARPHRD_CAN and ignored the -ENODEV from can_rx_register(), so the socket
> was marked as bound to the TUN device. A later isotp_release() or
> NETDEV_UNREGISTER notification then crashes in can_rx_unregister().

Let's hope the bots will be happy :)

Marc

-- 
Marc Kleine-Budde <mkl@pengutronix.de>

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Oliver Hartkopp]

On 16.10.26 13:55, Marc Kleine-Budde wrote:
>> Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct
>> net_device") introduced an explicit way to assign the midlayer private
>> pointer (dev->ml_priv) to named users like ML_PRIV_CAN. A valid pointer
>> returned by can_get_ml_priv() therefore identifies a real CAN device.
>>
>> The referenced commit only converted the j1939 and proc code to this
>> check, while the other parts of the CAN subsystem still rely on
>> dev->type == ARPHRD_CAN.
>>
>> The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware
>> type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed
>> netdevice passes the ARPHRD_CAN checks in the CAN subsystem, which can
>> lead to a NULL pointer dereference of the CAN specific ml_priv. This
>> only requires owning a TUN/TAP device or CAP_NET_ADMIN in an
>> unprivileged user namespace.
>>
>> can_rx_register() already rejects such a device since commit 8aa59e355949
>> ("can: af_can: prevent NULL pointer dereference in can_rx_register()"),
>> but can_rx_unregister() only checks dev->type and dereferences the NULL
>> ml_priv in can_dev_rcv_lists_find().
>>
>> This path is reachable via isotp: isotp_bind() only checked for
>> ARPHRD_CAN and ignored the -ENODEV from can_rx_register(), so the socket
>> was marked as bound to the TUN device. A later isotp_release() or
>> NETDEV_UNREGISTER notification then crashes in can_rx_unregister().
> 
> Let's hope the bots will be happy :)
The crash is fixed. The other stuff is no stable material.

;-)

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Marc Kleine-Budde]

On 2026-10-16 12:02 +0200, Oliver Hartkopp wrote:
> > [Severity: Medium]
> > A few lines lower in isotp_bind(), the return values of can_rx_register()
> > are ignored:
> >
> > 	if (isotp_register_rxid(so))
> > 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> > 				isotp_rcv, sk, "isotp", sk);
> >
> > 	/* no consecutive frame echo skb in flight */
> > 	WRITE_ONCE(so->cfecho, 0);
> >
> > 	/* register for echo skb's */
> > 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> > 			isotp_rcv_echo, sk, "isotpe", sk);
> >
> > If can_rx_register() fails (for example, returning -ENOMEM due to a failed
> > kmem_cache_alloc()), bind() will succeed but the socket is not fully
> > registered to receive CAN frames. This leads to a silent failure where
> > user-space believes the socket is successfully bound, but no traffic will
> > ever be received.
> >
> > Could this regression be prevented by checking the return values and
> > propagating the error appropriately, or cleaning up any partial
> > registrations?
> >
> When this patch is applied isotp_bind() can only fail with -ENOMEM and
> nothing breaks. The socket just becomes unusable. And if we have -ENOMEM
> we have a big problem anyway.
>

Please run "b4 prep --check" before sending patches:


I've taken the patch and fixed the checkpatch warning, but I suspect the
netdev sashiko will complain about the missing error handling, too.

> If I would provide such a patch the proc pointer removal patch would
> break next.

I think this is not an argument for sashiko :/

> I have ~20 patches for isotp and its CAN XL support. I will add the
> isotp_bind() error handling there when these current (can-next) fixes
> are in mainline.

regards,
Marc

-- 
Marc Kleine-Budde <mkl@pengutronix.de>

Re: [PATCH can-next v2] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Oliver Hartkopp]

On 16.10.26 14:15, Marc Kleine-Budde wrote:
> On 2026-10-16 12:02 +0200, Oliver Hartkopp wrote:
>>> [Severity: Medium]
>>> A few lines lower in isotp_bind(), the return values of can_rx_register()
>>> are ignored:
>>>
>>> 	if (isotp_register_rxid(so))
>>> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
>>> 				isotp_rcv, sk, "isotp", sk);
>>>
>>> 	/* no consecutive frame echo skb in flight */
>>> 	WRITE_ONCE(so->cfecho, 0);
>>>
>>> 	/* register for echo skb's */
>>> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
>>> 			isotp_rcv_echo, sk, "isotpe", sk);
>>>
>>> If can_rx_register() fails (for example, returning -ENOMEM due to a failed
>>> kmem_cache_alloc()), bind() will succeed but the socket is not fully
>>> registered to receive CAN frames. This leads to a silent failure where
>>> user-space believes the socket is successfully bound, but no traffic will
>>> ever be received.
>>>
>>> Could this regression be prevented by checking the return values and
>>> propagating the error appropriately, or cleaning up any partial
>>> registrations?
>>>
>> When this patch is applied isotp_bind() can only fail with -ENOMEM and
>> nothing breaks. The socket just becomes unusable. And if we have -ENOMEM
>> we have a big problem anyway.
>>
> 
> Please run "b4 prep --check" before sending patches:

I'll use b4 when I start sending the ISO-TP CAN XL patches. It still 
feels unhandy for the reactive review traffic to me.

But I missed checkpatch ... sorry.

> I've taken the patch and fixed the checkpatch warning, but I suspect the
> netdev sashiko will complain about the missing error handling, too.
> 
>> If I would provide such a patch the proc pointer removal patch would
>> break next.
> 
> I think this is not an argument for sashiko :/

The check of can_rx_register() in isotp_bind() is neither important nor 
urgent. I'll address it later.

It can not be the right way that important and urgent fixes are delayed 
for weeks when sashiko-bot finds some patch unrelated pre-existing issues.

I generally like assistant systems (also in the car) until they hinder 
the intended operation. This is like refusing to start because we have 
freezing temperatures.

Best regards,
Oliver

> 
>> I have ~20 patches for isotp and its CAN XL support. I will add the
>> isotp_bind() error handling there when these current (can-next) fixes
>> are in mainline.
> 
> regards,
> Marc
>