ACK: [X][B][D][E][F][SRU][PATCH 1/1] can, slip: Protect tty->disc_data in write_wakeup and close with RCU

[Colin Ian King]

On 06/02/2020 10:37, Po-Hsu Lin wrote:
> From: Richard Palethorpe <rpalethorpe at suse.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1862114
> 
> write_wakeup can happen in parallel with close/hangup where tty->disc_data
> is set to NULL and the netdevice is freed thus also freeing
> disc_data. write_wakeup accesses disc_data so we must prevent close from
> freeing the netdev while write_wakeup has a non-NULL view of
> tty->disc_data.
> 
> We also need to make sure that accesses to disc_data are atomic. Which can
> all be done with RCU.
> 
> This problem was found by Syzkaller on SLCAN, but the same issue is
> reproducible with the SLIP line discipline using an LTP test based on the
> Syzkaller reproducer.
> 
> A fix which didn't use RCU was posted by Hillf Danton.
> 
> Fixes: 661f7fda21b1 ("slip: Fix deadlock in write_wakeup")
> Fixes: a8e83b17536a ("slcan: Port write_wakeup deadlock fix from slip")
> Reported-by: syzbot+017e491ae13c0068598a at syzkaller.appspotmail.com
> Signed-off-by: Richard Palethorpe <rpalethorpe at suse.com>
> Cc: Wolfgang Grandegger <wg at grandegger.com>
> Cc: Marc Kleine-Budde <mkl at pengutronix.de>
> Cc: "David S. Miller" <davem at davemloft.net>
> Cc: Tyler Hall <tylerwhall at gmail.com>
> Cc: linux-can at vger.kernel.org
> Cc: netdev at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: syzkaller at googlegroups.com
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit 0ace17d56824165c7f4c68785d6b58971db954dd)
> Signed-off-by: Po-Hsu Lin <po-hsu.lin at canonical.com>
> ---
>  drivers/net/can/slcan.c | 12 ++++++++++--
>  drivers/net/slip/slip.c | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index cf0769a..b2e5bca 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -343,9 +343,16 @@ static void slcan_transmit(struct work_struct *work)
>   */
>  static void slcan_write_wakeup(struct tty_struct *tty)
>  {
> -	struct slcan *sl = tty->disc_data;
> +	struct slcan *sl;
> +
> +	rcu_read_lock();
> +	sl = rcu_dereference(tty->disc_data);
> +	if (!sl)
> +		goto out;
>  
>  	schedule_work(&sl->tx_work);
> +out:
> +	rcu_read_unlock();
>  }
>  
>  /* Send a can_frame to a TTY queue. */
> @@ -640,10 +647,11 @@ static void slcan_close(struct tty_struct *tty)
>  		return;
>  
>  	spin_lock_bh(&sl->lock);
> -	tty->disc_data = NULL;
> +	rcu_assign_pointer(tty->disc_data, NULL);
>  	sl->tty = NULL;
>  	spin_unlock_bh(&sl->lock);
>  
> +	synchronize_rcu();
>  	flush_work(&sl->tx_work);
>  
>  	/* Flush network side */
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 2a91c19..61d7e0d 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -452,9 +452,16 @@ static void slip_transmit(struct work_struct *work)
>   */
>  static void slip_write_wakeup(struct tty_struct *tty)
>  {
> -	struct slip *sl = tty->disc_data;
> +	struct slip *sl;
> +
> +	rcu_read_lock();
> +	sl = rcu_dereference(tty->disc_data);
> +	if (!sl)
> +		goto out;
>  
>  	schedule_work(&sl->tx_work);
> +out:
> +	rcu_read_unlock();
>  }
>  
>  static void sl_tx_timeout(struct net_device *dev)
> @@ -882,10 +889,11 @@ static void slip_close(struct tty_struct *tty)
>  		return;
>  
>  	spin_lock_bh(&sl->lock);
> -	tty->disc_data = NULL;
> +	rcu_assign_pointer(tty->disc_data, NULL);
>  	sl->tty = NULL;
>  	spin_unlock_bh(&sl->lock);
>  
> +	synchronize_rcu();
>  	flush_work(&sl->tx_work);
>  
>  	/* VSV = very important to remove timers */
> 

Looks good to me.

Acked-by: Colin Ian King <colin.king at canonical.com>