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 <luiz.von.dentz@xxxxxxxxx>
> Date: Tue, 28 Jun 2022 15:46:04 -0700
> Subject: [PATCH] Bluetooth: L2CAP: WIP
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
> 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
