[PATCH] can: remove WARN() statement from list operation sanity check

[PATCH] can: remove WARN() statement from list operation sanity check

[Oliver Hartkopp]

To detect potential bugs in CAN protocol implementations (double removal
of receiver entries) a WARN() statement has been used if no matching list
item was found for removal.

The fault injection issued by syzkaller was able to create a situation
where the closing of a socket runs simultaneously to the notifier call
chain for removing the CAN network device in use.

This case is very unlikely in real life but it doesn't break anything.
Therefore we just replace the WARN() statement with pr_warn() to
preserve the notification for the CAN protocol development.

Reported-by: syzbot+381d06e0c8eaacb8706f@syzkaller.appspotmail.com
Reported-by: syzbot+d0ddd88c9a7432f041e6@syzkaller.appspotmail.com
Reported-by: syzbot+76d62d3b8162883c7d11@syzkaller.appspotmail.com
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/af_can.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 5d124c155904..7c5ccdec89e1 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -539,14 +539,17 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 			break;
 	}
 
 	/* Check for bugs in CAN protocol implementations using af_can.c:
 	 * 'rcv' will be NULL if no matching list item was found for removal.
+	 * As this case may potentially happen when closing a socket while
+	 * the notifier for removing the CAN netdev is running we just print
+	 * a warning here. Reported by syskaller (see commit message)
 	 */
 	if (!rcv) {
-		WARN(1, "BUG: receive list entry not found for dev %s, id %03X, mask %03X\n",
-		     DNAME(dev), can_id, mask);
+		pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
+			DNAME(dev), can_id, mask);
 		goto out;
 	}
 
 	hlist_del_rcu(&rcv->list);
 	dev_rcv_lists->entries--;
-- 
2.29.2

Re: [PATCH] can: remove WARN() statement from list operation sanity check

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1230 bytes --]

On 11/26/20 8:21 PM, Oliver Hartkopp wrote:
> To detect potential bugs in CAN protocol implementations (double removal
> of receiver entries) a WARN() statement has been used if no matching list
> item was found for removal.
> 
> The fault injection issued by syzkaller was able to create a situation
> where the closing of a socket runs simultaneously to the notifier call
> chain for removing the CAN network device in use.
> 
> This case is very unlikely in real life but it doesn't break anything.
> Therefore we just replace the WARN() statement with pr_warn() to
> preserve the notification for the CAN protocol development.
> 
> Reported-by: syzbot+381d06e0c8eaacb8706f@syzkaller.appspotmail.com
> Reported-by: syzbot+d0ddd88c9a7432f041e6@syzkaller.appspotmail.com
> Reported-by: syzbot+76d62d3b8162883c7d11@syzkaller.appspotmail.com
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

applied to can/testing.

tnx,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: remove WARN() statement from list operation sanity check

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2522 bytes --]

On 11/26/20 8:21 PM, Oliver Hartkopp wrote:
> To detect potential bugs in CAN protocol implementations (double removal
> of receiver entries) a WARN() statement has been used if no matching list
> item was found for removal.
> 
> The fault injection issued by syzkaller was able to create a situation
> where the closing of a socket runs simultaneously to the notifier call
> chain for removing the CAN network device in use.
> 
> This case is very unlikely in real life but it doesn't break anything.
> Therefore we just replace the WARN() statement with pr_warn() to
> preserve the notification for the CAN protocol development.
> 
> Reported-by: syzbot+381d06e0c8eaacb8706f@syzkaller.appspotmail.com
> Reported-by: syzbot+d0ddd88c9a7432f041e6@syzkaller.appspotmail.com
> Reported-by: syzbot+76d62d3b8162883c7d11@syzkaller.appspotmail.com
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  net/can/af_can.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 5d124c155904..7c5ccdec89e1 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -539,14 +539,17 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
>  			break;
>  	}
>  
>  	/* Check for bugs in CAN protocol implementations using af_can.c:
>  	 * 'rcv' will be NULL if no matching list item was found for removal.
> +	 * As this case may potentially happen when closing a socket while
> +	 * the notifier for removing the CAN netdev is running we just print
> +	 * a warning here. Reported by syskaller (see commit message)
I've removed the "Reported by syskaller (see commit message)" while applying the
patch, to keep this comment short and to the point. Use tig/git blame (or any
other future tool) to figure out the commit message for details :D

>  	 */
>  	if (!rcv) {
> -		WARN(1, "BUG: receive list entry not found for dev %s, id %03X, mask %03X\n",
> -		     DNAME(dev), can_id, mask);
> +		pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
> +			DNAME(dev), can_id, mask);
>  		goto out;
>  	}
>  
>  	hlist_del_rcu(&rcv->list);
>  	dev_rcv_lists->entries--;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: remove WARN() statement from list operation sanity check

[Oliver Hartkopp]

On 27.11.20 10:48, Marc Kleine-Budde wrote:

>>   	/* Check for bugs in CAN protocol implementations using af_can.c:
>>   	 * 'rcv' will be NULL if no matching list item was found for removal.
>> +	 * As this case may potentially happen when closing a socket while
>> +	 * the notifier for removing the CAN netdev is running we just print
>> +	 * a warning here. Reported by syskaller (see commit message)
> I've removed the "Reported by syskaller (see commit message)" while applying the
> patch, to keep this comment short and to the point. Use tig/git blame (or any
> other future tool) to figure out the commit message for details :D
> 

Is fine for me ;-)

Thanks Marc!