Subject: Re: [RESEND 1/1] Bluetooth: Use chan_list_lock to protect the whole put/destroy invokation

From: Lee Jones <[email-masked]>
Date: Wed, 20 Jul 2022 12:52:56 +0100

On Wed, 06 Jul 2022, Luiz Augusto von Dentz wrote:
> > > > > > Perhaps something like this:
> > > > >
> > > > > I'm struggling to apply this for test:
> > > > >
> > > > >   "error: corrupt patch at line 6"
> > > >
> > > > Check with the attached patch.
> > >
> > > With the patch applied:
> > >
> > > [  188.825418][   T75] refcount_t: addition on 0; use-after-free.
> > > [  188.825418][   T75] refcount_t: addition on 0; use-after-free.
> >
> > Looks like the changes just make the issue more visible since we are
> > trying to add a refcount when it is already 0 so this proves the
> > design is not quite right since it is removing the object from the
> > list only when destroying it while we probably need to do it before.
> >
> > How about we use kref_get_unless_zero as it appears it was introduced
> > exactly for such cases (patch attached.)
> 
> Looks like I missed a few places like l2cap_global_chan_by_psm so here
> is another version.

Okay, with the patch below the kernel doesn't produce a back-trace.

Only this, which I assume is expected?

[  535.398255][  T495] Bluetooth: hci0: unexpected cc 0x0c03 length: 249 > 1                                                          
[  535.398255][  T495] Bluetooth: hci0: unexpected cc 0x0c03 length: 249 > 1                                                          
[  535.417007][  T495] Bluetooth: hci0: unexpected cc 0x1003 length: 249 > 9                                                          
[  535.417007][  T495] Bluetooth: hci0: unexpected cc 0x1003 length: 249 > 9                                                          
[  535.434810][  T495] Bluetooth: hci0: unexpected cc 0x1001 length: 249 > 9                                                          
[  535.434810][  T495] Bluetooth: hci0: unexpected cc 0x1001 length: 249 > 9                                                          
[  535.452886][  T495] Bluetooth: hci0: unexpected cc 0x0c23 length: 249 > 4                                                          
[  535.452886][  T495] Bluetooth: hci0: unexpected cc 0x0c23 length: 249 > 4                                                          
[  535.470574][  T495] Bluetooth: hci0: unexpected cc 0x0c25 length: 249 > 3                                                          
[  535.470574][  T495] Bluetooth: hci0: unexpected cc 0x0c25 length: 249 > 3                                                          
[  535.488009][  T495] Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2                                                          
[  535.488009][  T495] Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2                                                          
[  537.551677][   T74] Bluetooth: hci0: command 0x0409 tx timeout                                                                     
[  537.551677][   T74] Bluetooth: hci0: command 0x0409 tx timeout                                                                     
[  539.641362][  T373] Bluetooth: hci0: command 0x041b tx timeout                                                                     
[  539.641362][  T373] Bluetooth: hci0: command 0x041b tx timeout                                                                     
[  541.711056][  T274] Bluetooth: hci0: command 0x040f tx timeout                                                                     
[  541.711056][  T274] Bluetooth: hci0: command 0x040f tx timeout                                                                     
[  543.790939][   T66] Bluetooth: hci0: command 0x0419 tx timeout                                                                     
[  543.790939][   T66] Bluetooth: hci0: command 0x0419 tx timeout

> From 235937ac7a39d16e5dabbfca0ac1d58e4cc814d9 Mon Sep 17 00:00:00 2001
> From: Luiz Augusto von Dentz <[email-masked]>
> Date: Tue, 28 Jun 2022 15:46:04 -0700
> Subject: [PATCH] Bluetooth: L2CAP: WIP
> 
> Signed-off-by: Luiz Augusto von Dentz <[email-masked]>
> ---
>  include/net/bluetooth/l2cap.h |  1 +
>  net/bluetooth/l2cap_core.c    | 58 +++++++++++++++++++++++++++--------
>  2 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 3c4f550e5a8b..2f766e3437ce 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -847,6 +847,7 @@ enum {
>  };
>  
>  void l2cap_chan_hold(struct l2cap_chan *c);
> +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c);
>  void l2cap_chan_put(struct l2cap_chan *c);
>  
>  static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 09ecaf556de5..3e5d81e971cc 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -111,7 +111,8 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn,
>  }
>  
>  /* Find channel with given SCID.
> - * Returns locked channel. */
> + * Returns a reference locked channel.
> + */
>  static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
>  						 u16 cid)
>  {
> @@ -119,15 +120,19 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
>  
>  	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_scid(conn, cid);
> -	if (c)
> -		l2cap_chan_lock(c);
> +	if (c) {
> +		/* Only lock if chan reference is not 0 */
> +		c = l2cap_chan_hold_unless_zero(c);
> +		if (c)
> +			l2cap_chan_lock(c);
> +	}
>  	mutex_unlock(&conn->chan_lock);
>  
>  	return c;
>  }
>  
>  /* Find channel with given DCID.
> - * Returns locked channel.
> + * Returns a reference locked channel.
>   */
>  static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
>  						 u16 cid)
> @@ -136,8 +141,12 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
>  
>  	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_dcid(conn, cid);
> -	if (c)
> -		l2cap_chan_lock(c);
> +	if (c) {
> +		/* Only lock if chan reference is not 0 */
> +		c = l2cap_chan_hold_unless_zero(c);
> +		if (c)
> +			l2cap_chan_lock(c);
> +	}
>  	mutex_unlock(&conn->chan_lock);
>  
>  	return c;
> @@ -162,8 +171,12 @@ static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
>  
>  	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_ident(conn, ident);
> -	if (c)
> -		l2cap_chan_lock(c);
> +	if (c) {
> +		/* Only lock if chan reference is not 0 */
> +		c = l2cap_chan_hold_unless_zero(c);
> +		if (c)
> +			l2cap_chan_lock(c);
> +	}
>  	mutex_unlock(&conn->chan_lock);
>  
>  	return c;
> @@ -497,6 +510,16 @@ void l2cap_chan_hold(struct l2cap_chan *c)
>  	kref_get(&c->kref);
>  }
>  
> +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c)
> +{
> +	BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> +
> +	if (!kref_get_unless_zero(&c->kref))
> +		return NULL;
> +
> +	return c;
> +}
> +
>  void l2cap_chan_put(struct l2cap_chan *c)
>  {
>  	BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> @@ -1969,7 +1992,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
>  			src_match = !bacmp(&c->src, src);
>  			dst_match = !bacmp(&c->dst, dst);
>  			if (src_match && dst_match) {
> -				l2cap_chan_hold(c);
> +				c = l2cap_chan_hold_unless_zero(c);
>  				read_unlock(&chan_list_lock);
>  				return c;
>  			}
> @@ -1984,7 +2007,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
>  	}
>  
>  	if (c1)
> -		l2cap_chan_hold(c1);
> +		c1 = l2cap_chan_hold_unless_zero(c1);
>  
>  	read_unlock(&chan_list_lock);
>  
> @@ -4464,6 +4487,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>  
>  unlock:
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  	return err;
>  }
>  
> @@ -4578,6 +4602,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
>  
>  done:
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  	return err;
>  }
>  
> @@ -5305,6 +5330,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
>  	l2cap_send_move_chan_rsp(chan, result);
>  
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return 0;
>  }
> @@ -5397,6 +5423,7 @@ static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
>  	}
>  
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  }
>  
>  static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> @@ -5426,6 +5453,7 @@ static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
>  	l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
>  
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  }
>  
>  static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> @@ -5489,6 +5517,7 @@ static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
>  	l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
>  
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return 0;
>  }
> @@ -5524,6 +5553,7 @@ static inline int l2cap_move_channel_confirm_rsp(struct l2cap_conn *conn,
>  	}
>  
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return 0;
>  }
> @@ -5896,12 +5926,11 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>  	if (credits > max_credits) {
>  		BT_ERR("LE credits overflow");
>  		l2cap_send_disconn_req(chan, ECONNRESET);
> -		l2cap_chan_unlock(chan);
>  
>  		/* Return 0 so that we don't trigger an unnecessary
>  		 * command reject packet.
>  		 */
> -		return 0;
> +		goto unlock;
>  	}
>  
>  	chan->tx_credits += credits;
> @@ -5912,7 +5941,9 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>  	if (chan->tx_credits)
>  		chan->ops->resume(chan);
>  
> +unlock:
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return 0;
>  }
> @@ -7598,6 +7629,7 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
>  
>  done:
>  	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  }
>  
>  static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
> @@ -8086,7 +8118,7 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
>  		if (src_type != c->src_type)
>  			continue;
>  
> -		l2cap_chan_hold(c);
> +		c = l2cap_chan_hold_unless_zero(c);
>  		read_unlock(&chan_list_lock);
>  		return c;
>  	}

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog