[NET 0/2] CAN fixes for 6.5-rc7

[NET 0/2] CAN fixes for 6.5-rc7

[Oliver Hartkopp]

Hello Jakub,

as Marc is probably on vacation I send these two fixes directly to the netdev
mailing list to hopefully get them into the current 6.5 cycle.

The isotp fix removes an unnecessary check which leads to delays and/or a wrong
error notification.

The fix for the CAN_RAW socket solves the last issue that has been introduced
with commit ee8b94c8510c ("can: raw: fix receiver memory leak") in this
upstream cycle (detected by Eric Dumazet).

Best regards,
Oliver

Oliver Hartkopp (2):
  can: isotp: fix support for transmission of SF without flow control
  can: raw: add missing refcount for memory leak fix

 net/can/isotp.c | 22 +++++++---------------
 net/can/raw.c   | 35 ++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 24 deletions(-)

-- 
2.39.2

[NET 1/2] can: isotp: fix support for transmission of SF without flow control

[Oliver Hartkopp]

The original implementation had a very simple handling for single frame
transmissions as it just sent the single frame without a timeout handling.

With the new echo frame handling the echo frame was also introduced for
single frames but the former exception ('simple without timers') has been
maintained by accident. This leads to a 1 second timeout when closing the
socket and to an -ECOMM error when CAN_ISOTP_WAIT_TX_DONE is selected.

As the echo handling is always active (also for single frames) remove the
wrong extra condition for single frames.

Fixes: 9f39d36530e5 ("can: isotp: add support for transmission without flow control")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 99770ed28531..f02b5d3e4733 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -186,16 +186,10 @@ static bool isotp_register_rxid(struct isotp_sock *so)
 {
 	/* no broadcast modes => register rx_id for FC frame reception */
 	return (isotp_bc_flags(so) == 0);
 }
 
-static bool isotp_register_txecho(struct isotp_sock *so)
-{
-	/* all modes but SF_BROADCAST register for tx echo skbs */
-	return (isotp_bc_flags(so) != CAN_ISOTP_SF_BROADCAST);
-}
-
 static enum hrtimer_restart isotp_rx_timer_handler(struct hrtimer *hrtimer)
 {
 	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
 					     rxtimer);
 	struct sock *sk = &so->sk;
@@ -1207,11 +1201,11 @@ static int isotp_release(struct socket *sock)
 	spin_unlock(&isotp_notifier_lock);
 
 	lock_sock(sk);
 
 	/* remove current filters & unregister */
-	if (so->bound && isotp_register_txecho(so)) {
+	if (so->bound) {
 		if (so->ifindex) {
 			struct net_device *dev;
 
 			dev = dev_get_by_index(net, so->ifindex);
 			if (dev) {
@@ -1330,18 +1324,16 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 
 	if (isotp_register_rxid(so))
 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
 				isotp_rcv, sk, "isotp", sk);
 
-	if (isotp_register_txecho(so)) {
-		/* no consecutive frame echo skb in flight */
-		so->cfecho = 0;
+	/* no consecutive frame echo skb in flight */
+	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);
-	}
+	/* register for echo skb's */
+	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
+			isotp_rcv_echo, sk, "isotpe", sk);
 
 	dev_put(dev);
 
 	/* switch to new settings */
 	so->ifindex = ifindex;
@@ -1558,11 +1550,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
 		/* remove current filters & unregister */
-		if (so->bound && isotp_register_txecho(so)) {
+		if (so->bound) {
 			if (isotp_register_rxid(so))
 				can_rx_unregister(dev_net(dev), dev, so->rxid,
 						  SINGLE_MASK(so->rxid),
 						  isotp_rcv, sk);
 
-- 
2.39.2

[NET 2/2] can: raw: add missing refcount for memory leak fix

[Oliver Hartkopp]

Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced
a new reference to the CAN netdevice that has assigned CAN filters.
But this new ro->dev reference did not maintain its own refcount which
lead to another KASAN use-after-free splat found by Eric Dumazet.

This patch ensures a proper refcount for the CAN nedevice.

Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
Reported-by: Eric Dumazet <edumazet@google.com>
Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/raw.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/net/can/raw.c b/net/can/raw.c
index e10f59375659..d50c3f3d892f 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -83,10 +83,11 @@ struct uniqframe {
 struct raw_sock {
 	struct sock sk;
 	int bound;
 	int ifindex;
 	struct net_device *dev;
+	netdevice_tracker dev_tracker;
 	struct list_head notifier;
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
 	int xl_frames;
@@ -283,12 +284,14 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
 		/* remove current filters & unregister */
-		if (ro->bound)
+		if (ro->bound) {
 			raw_disable_allfilters(dev_net(dev), dev, sk);
+			netdev_put(dev, &ro->dev_tracker);
+		}
 
 		if (ro->count > 1)
 			kfree(ro->filter);
 
 		ro->ifindex = 0;
@@ -389,14 +392,16 @@ static int raw_release(struct socket *sock)
 	rtnl_lock();
 	lock_sock(sk);
 
 	/* remove current filters & unregister */
 	if (ro->bound) {
-		if (ro->dev)
+		if (ro->dev) {
 			raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
-		else
+			netdev_put(ro->dev, &ro->dev_tracker);
+		} else {
 			raw_disable_allfilters(sock_net(sk), NULL, sk);
+		}
 	}
 
 	if (ro->count > 1)
 		kfree(ro->filter);
 
@@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
 		}
 		if (dev->type != ARPHRD_CAN) {
-			dev_put(dev);
 			err = -ENODEV;
-			goto out;
+			goto out_put_dev;
 		}
+
 		if (!(dev->flags & IFF_UP))
 			notify_enetdown = 1;
 
 		ifindex = dev->ifindex;
 
 		/* filters set by default/setsockopt */
 		err = raw_enable_allfilters(sock_net(sk), dev, sk);
-		dev_put(dev);
+		if (err)
+			goto out_put_dev;
+
 	} else {
 		ifindex = 0;
 
 		/* filters set by default/setsockopt */
 		err = raw_enable_allfilters(sock_net(sk), NULL, sk);
 	}
 
 	if (!err) {
 		if (ro->bound) {
 			/* unregister old filters */
-			if (ro->dev)
+			if (ro->dev) {
 				raw_disable_allfilters(dev_net(ro->dev),
 						       ro->dev, sk);
-			else
+				/* drop reference to old ro->dev */
+				netdev_put(ro->dev, &ro->dev_tracker);
+			} else {
 				raw_disable_allfilters(sock_net(sk), NULL, sk);
+			}
 		}
 		ro->ifindex = ifindex;
 		ro->bound = 1;
+		/* bind() ok -> hold a reference for new ro->dev */
 		ro->dev = dev;
+		if (ro->dev)
+			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
 	}
 
- out:
+out_put_dev:
+	/* remove potential reference from dev_get_by_index() */
+	if (dev)
+		dev_put(dev);
+out:
 	release_sock(sk);
 	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
-- 
2.39.2

Re: [NET 2/2] can: raw: add missing refcount for memory leak fix

[Simon Horman]

On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote:
> Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced
> a new reference to the CAN netdevice that has assigned CAN filters.
> But this new ro->dev reference did not maintain its own refcount which
> lead to another KASAN use-after-free splat found by Eric Dumazet.
> 
> This patch ensures a proper refcount for the CAN nedevice.
> 
> Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
> Reported-by: Eric Dumazet <edumazet@google.com>
> Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

...

> @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>  		if (!dev) {
>  			err = -ENODEV;
>  			goto out;
>  		}
>  		if (dev->type != ARPHRD_CAN) {
> -			dev_put(dev);
>  			err = -ENODEV;
> -			goto out;
> +			goto out_put_dev;
>  		}
> +
>  		if (!(dev->flags & IFF_UP))
>  			notify_enetdown = 1;
>  
>  		ifindex = dev->ifindex;
>  
>  		/* filters set by default/setsockopt */
>  		err = raw_enable_allfilters(sock_net(sk), dev, sk);
> -		dev_put(dev);
> +		if (err)
> +			goto out_put_dev;
> +
>  	} else {
>  		ifindex = 0;
>  
>  		/* filters set by default/setsockopt */
>  		err = raw_enable_allfilters(sock_net(sk), NULL, sk);
>  	}
>  
>  	if (!err) {
>  		if (ro->bound) {
>  			/* unregister old filters */
> -			if (ro->dev)
> +			if (ro->dev) {
>  				raw_disable_allfilters(dev_net(ro->dev),
>  						       ro->dev, sk);
> -			else
> +				/* drop reference to old ro->dev */
> +				netdev_put(ro->dev, &ro->dev_tracker);
> +			} else {
>  				raw_disable_allfilters(sock_net(sk), NULL, sk);
> +			}
>  		}
>  		ro->ifindex = ifindex;
>  		ro->bound = 1;
> +		/* bind() ok -> hold a reference for new ro->dev */
>  		ro->dev = dev;
> +		if (ro->dev)
> +			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
>  	}
>  
> - out:
> +out_put_dev:
> +	/* remove potential reference from dev_get_by_index() */
> +	if (dev)
> +		dev_put(dev);

Hi Oliver,

this is possibly not worth a respin, but there is no need to check if dev
is NULL before calling dev_put(), dev_put() will effectively be a no-op
with a NULL argument.

> +out:
>  	release_sock(sk);
>  	rtnl_unlock();
>  
>  	if (notify_enetdown) {
>  		sk->sk_err = ENETDOWN;
> -- 
> 2.39.2
> 
> 

Re: [NET 2/2] can: raw: add missing refcount for memory leak fix

[Oliver Hartkopp]

On 22.08.23 09:59, Simon Horman wrote:
> On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote:
>> Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced
>> a new reference to the CAN netdevice that has assigned CAN filters.
>> But this new ro->dev reference did not maintain its own refcount which
>> lead to another KASAN use-after-free splat found by Eric Dumazet.
>>
>> This patch ensures a proper refcount for the CAN nedevice.
>>
>> Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
>> Reported-by: Eric Dumazet <edumazet@google.com>
>> Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ...
> 
>> @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
>>   		if (!dev) {
>>   			err = -ENODEV;
>>   			goto out;
>>   		}
>>   		if (dev->type != ARPHRD_CAN) {
>> -			dev_put(dev);
>>   			err = -ENODEV;
>> -			goto out;
>> +			goto out_put_dev;
>>   		}
>> +
>>   		if (!(dev->flags & IFF_UP))
>>   			notify_enetdown = 1;
>>   
>>   		ifindex = dev->ifindex;
>>   
>>   		/* filters set by default/setsockopt */
>>   		err = raw_enable_allfilters(sock_net(sk), dev, sk);
>> -		dev_put(dev);
>> +		if (err)
>> +			goto out_put_dev;
>> +
>>   	} else {
>>   		ifindex = 0;
>>   
>>   		/* filters set by default/setsockopt */
>>   		err = raw_enable_allfilters(sock_net(sk), NULL, sk);
>>   	}
>>   
>>   	if (!err) {
>>   		if (ro->bound) {
>>   			/* unregister old filters */
>> -			if (ro->dev)
>> +			if (ro->dev) {
>>   				raw_disable_allfilters(dev_net(ro->dev),
>>   						       ro->dev, sk);
>> -			else
>> +				/* drop reference to old ro->dev */
>> +				netdev_put(ro->dev, &ro->dev_tracker);
>> +			} else {
>>   				raw_disable_allfilters(sock_net(sk), NULL, sk);
>> +			}
>>   		}
>>   		ro->ifindex = ifindex;
>>   		ro->bound = 1;
>> +		/* bind() ok -> hold a reference for new ro->dev */
>>   		ro->dev = dev;
>> +		if (ro->dev)
>> +			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
>>   	}
>>   
>> - out:
>> +out_put_dev:
>> +	/* remove potential reference from dev_get_by_index() */
>> +	if (dev)
>> +		dev_put(dev);
> 
> Hi Oliver,
> 
> this is possibly not worth a respin, but there is no need to check if dev
> is NULL before calling dev_put(), dev_put() will effectively be a no-op
> with a NULL argument.
> 

Hi Simon,

thanks for your feedback.

In fact I had in mind that someone recently removed some of these "if 
(dev)" statements before "dev_put(dev)" in the netdev subtree.

The reason why I still wanted to point out this check is because of dev 
== NULL is also a valid value for CAN_RAW sockets that are not bound to 
a specific netdev but to 'ALL' CAN netdevs.

So it was more like a documentation purpose than a programming need.

As you don't see a need for a respin too, I can send a patch to can-next 
to remove it, if that fits for you.

Best regards,
Oliver



>> +out:
>>   	release_sock(sk);
>>   	rtnl_unlock();
>>   
>>   	if (notify_enetdown) {
>>   		sk->sk_err = ENETDOWN;
>> -- 
>> 2.39.2
>>
>>
> 

Re: [NET 2/2] can: raw: add missing refcount for memory leak fix

[Simon Horman]

On Tue, Aug 22, 2023 at 06:45:25PM +0200, Oliver Hartkopp wrote:
> 
> 
> On 22.08.23 09:59, Simon Horman wrote:
> > On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote:
> > > Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced
> > > a new reference to the CAN netdevice that has assigned CAN filters.
> > > But this new ro->dev reference did not maintain its own refcount which
> > > lead to another KASAN use-after-free splat found by Eric Dumazet.
> > > 
> > > This patch ensures a proper refcount for the CAN nedevice.
> > > 
> > > Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
> > > Reported-by: Eric Dumazet <edumazet@google.com>
> > > Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
> > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > 
> > ...
> > 
> > > @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> > >   		if (!dev) {
> > >   			err = -ENODEV;
> > >   			goto out;
> > >   		}
> > >   		if (dev->type != ARPHRD_CAN) {
> > > -			dev_put(dev);
> > >   			err = -ENODEV;
> > > -			goto out;
> > > +			goto out_put_dev;
> > >   		}
> > > +
> > >   		if (!(dev->flags & IFF_UP))
> > >   			notify_enetdown = 1;
> > >   		ifindex = dev->ifindex;
> > >   		/* filters set by default/setsockopt */
> > >   		err = raw_enable_allfilters(sock_net(sk), dev, sk);
> > > -		dev_put(dev);
> > > +		if (err)
> > > +			goto out_put_dev;
> > > +
> > >   	} else {
> > >   		ifindex = 0;
> > >   		/* filters set by default/setsockopt */
> > >   		err = raw_enable_allfilters(sock_net(sk), NULL, sk);
> > >   	}
> > >   	if (!err) {
> > >   		if (ro->bound) {
> > >   			/* unregister old filters */
> > > -			if (ro->dev)
> > > +			if (ro->dev) {
> > >   				raw_disable_allfilters(dev_net(ro->dev),
> > >   						       ro->dev, sk);
> > > -			else
> > > +				/* drop reference to old ro->dev */
> > > +				netdev_put(ro->dev, &ro->dev_tracker);
> > > +			} else {
> > >   				raw_disable_allfilters(sock_net(sk), NULL, sk);
> > > +			}
> > >   		}
> > >   		ro->ifindex = ifindex;
> > >   		ro->bound = 1;
> > > +		/* bind() ok -> hold a reference for new ro->dev */
> > >   		ro->dev = dev;
> > > +		if (ro->dev)
> > > +			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
> > >   	}
> > > - out:
> > > +out_put_dev:
> > > +	/* remove potential reference from dev_get_by_index() */
> > > +	if (dev)
> > > +		dev_put(dev);
> > 
> > Hi Oliver,
> > 
> > this is possibly not worth a respin, but there is no need to check if dev
> > is NULL before calling dev_put(), dev_put() will effectively be a no-op
> > with a NULL argument.
> > 
> 
> Hi Simon,
> 
> thanks for your feedback.
> 
> In fact I had in mind that someone recently removed some of these "if (dev)"
> statements before "dev_put(dev)" in the netdev subtree.
> 
> The reason why I still wanted to point out this check is because of dev ==
> NULL is also a valid value for CAN_RAW sockets that are not bound to a
> specific netdev but to 'ALL' CAN netdevs.
> 
> So it was more like a documentation purpose than a programming need.
> 
> As you don't see a need for a respin too, I can send a patch to can-next to
> remove it, if that fits for you.

Thanks Oliver, that would be fine by me.

Re: [NET 0/2] CAN fixes for 6.5-rc7

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 21 Aug 2023 16:45:45 +0200 you wrote:
> Hello Jakub,
> 
> as Marc is probably on vacation I send these two fixes directly to the netdev
> mailing list to hopefully get them into the current 6.5 cycle.
> 
> The isotp fix removes an unnecessary check which leads to delays and/or a wrong
> error notification.
> 
> [...]

Here is the summary with links:
  - [NET,1/2] can: isotp: fix support for transmission of SF without flow control
    https://git.kernel.org/netdev/net/c/0bfe71159230
  - [NET,2/2] can: raw: add missing refcount for memory leak fix
    https://git.kernel.org/netdev/net/c/c275a176e4b6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html