netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu
@ 2021-06-19 16:18 Thadeu Lima de Souza Cascardo
  2021-06-19 16:25 ` Oliver Hartkopp
  2021-06-19 20:12 ` Marc Kleine-Budde
  0 siblings, 2 replies; 3+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2021-06-19 16:18 UTC (permalink / raw)
  To: linux-can
  Cc: Oliver Hartkopp, Marc Kleine-Budde, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel,
	Thadeu Lima de Souza Cascardo, syzbot+0f7e7e5e2f4f40fa89c0,
	Norbert Slusarek

can_rx_register callbacks may be called concurrently to the call to
can_rx_unregister. The callbacks and callback data, though, are protected by
RCU and the struct sock reference count.

So the callback data is really attached to the life of sk, meaning that it
should be released on sk_destruct. However, bcm_remove_op calls tasklet_kill,
and RCU callbacks may be called under RCU softirq, so that cannot be used on
kernels before the introduction of HRTIMER_MODE_SOFT.

However, bcm_rx_handler is called under RCU protection, so after calling
can_rx_unregister, we may call synchronize_rcu in order to wait for any RCU
read-side critical sections to finish. That is, bcm_rx_handler won't be called
anymore for those ops. So, we only free them, after we do that synchronize_rcu.

Reported-by: syzbot+0f7e7e5e2f4f40fa89c0@syzkaller.appspotmail.com
Reported-by: Norbert Slusarek <nslusarek@gmx.net>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 net/can/bcm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index f3e4d9528fa3..c67916020e63 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -785,6 +785,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 						  bcm_rx_handler, op);
 
 			list_del(&op->list);
+			synchronize_rcu();
 			bcm_remove_op(op);
 			return 1; /* done */
 		}
@@ -1533,6 +1534,11 @@ static int bcm_release(struct socket *sock)
 					  REGMASK(op->can_id),
 					  bcm_rx_handler, op);
 
+	}
+
+	synchronize_rcu();
+
+	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
 		bcm_remove_op(op);
 	}
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu
  2021-06-19 16:18 [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu Thadeu Lima de Souza Cascardo
@ 2021-06-19 16:25 ` Oliver Hartkopp
  2021-06-19 20:12 ` Marc Kleine-Budde
  1 sibling, 0 replies; 3+ messages in thread
From: Oliver Hartkopp @ 2021-06-19 16:25 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo, linux-can
  Cc: Marc Kleine-Budde, David S. Miller, Jakub Kicinski, netdev,
	linux-kernel, syzbot+0f7e7e5e2f4f40fa89c0, Norbert Slusarek



On 19.06.21 18:18, Thadeu Lima de Souza Cascardo wrote:
> can_rx_register callbacks may be called concurrently to the call to
> can_rx_unregister. The callbacks and callback data, though, are protected by
> RCU and the struct sock reference count.
> 
> So the callback data is really attached to the life of sk, meaning that it
> should be released on sk_destruct. However, bcm_remove_op calls tasklet_kill,
> and RCU callbacks may be called under RCU softirq, so that cannot be used on
> kernels before the introduction of HRTIMER_MODE_SOFT.
> 
> However, bcm_rx_handler is called under RCU protection, so after calling
> can_rx_unregister, we may call synchronize_rcu in order to wait for any RCU
> read-side critical sections to finish. That is, bcm_rx_handler won't be called
> anymore for those ops. So, we only free them, after we do that synchronize_rcu.
> 
> Reported-by: syzbot+0f7e7e5e2f4f40fa89c0@syzkaller.appspotmail.com
> Reported-by: Norbert Slusarek <nslusarek@gmx.net>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks Norbert for reporting and Thadeu for working out the fix!

Best regards,
Oliver

> ---
>   net/can/bcm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index f3e4d9528fa3..c67916020e63 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -785,6 +785,7 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
>   						  bcm_rx_handler, op);
>   
>   			list_del(&op->list);
> +			synchronize_rcu();
>   			bcm_remove_op(op);
>   			return 1; /* done */
>   		}
> @@ -1533,6 +1534,11 @@ static int bcm_release(struct socket *sock)
>   					  REGMASK(op->can_id),
>   					  bcm_rx_handler, op);
>   
> +	}
> +
> +	synchronize_rcu();
> +
> +	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
>   		bcm_remove_op(op);
>   	}
>   
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu
  2021-06-19 16:18 [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu Thadeu Lima de Souza Cascardo
  2021-06-19 16:25 ` Oliver Hartkopp
@ 2021-06-19 20:12 ` Marc Kleine-Budde
  1 sibling, 0 replies; 3+ messages in thread
From: Marc Kleine-Budde @ 2021-06-19 20:12 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo
  Cc: linux-can, Oliver Hartkopp, David S. Miller, Jakub Kicinski,
	netdev, linux-kernel, syzbot+0f7e7e5e2f4f40fa89c0,
	Norbert Slusarek

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On 19.06.2021 13:18:13, Thadeu Lima de Souza Cascardo wrote:
> can_rx_register callbacks may be called concurrently to the call to
> can_rx_unregister. The callbacks and callback data, though, are protected by
> RCU and the struct sock reference count.
> 
> So the callback data is really attached to the life of sk, meaning that it
> should be released on sk_destruct. However, bcm_remove_op calls tasklet_kill,
> and RCU callbacks may be called under RCU softirq, so that cannot be used on
> kernels before the introduction of HRTIMER_MODE_SOFT.
> 
> However, bcm_rx_handler is called under RCU protection, so after calling
> can_rx_unregister, we may call synchronize_rcu in order to wait for any RCU
> read-side critical sections to finish. That is, bcm_rx_handler won't be called
> anymore for those ops. So, we only free them, after we do that synchronize_rcu.
> 
> Reported-by: syzbot+0f7e7e5e2f4f40fa89c0@syzkaller.appspotmail.com
> Reported-by: Norbert Slusarek <nslusarek@gmx.net>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Added to linux-can/testing.

Thanks,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-19 20:13 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-19 16:18 [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu Thadeu Lima de Souza Cascardo
2021-06-19 16:25 ` Oliver Hartkopp
2021-06-19 20:12 ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).