Messages in this thread | | | Subject | Re: [PATCH] can: bcm: delay release of struct bcm_op after synchronize_rcu | From | Oliver Hartkopp <> | Date | Sat, 19 Jun 2021 18:25:47 +0200 |
| |
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); > } > >
| |