From: Lee Jones <lee.jones@linaro.org>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>,
LKML <linux-kernel@vger.kernel.org>,
stable@kernel.org, Marcel Holtmann <marcel@holtmann.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
"linux-bluetooth@vger.kernel.org"
<linux-bluetooth@vger.kernel.org>,
netdev <netdev@vger.kernel.org>
Subject: Re: [RESEND 1/1] Bluetooth: Use chan_list_lock to protect the whole put/destroy invokation
Date: Wed, 20 Jul 2022 12:52:56 +0100 [thread overview]
Message-ID: <YtfsmHt4R/WxJOTV@google.com> (raw)
In-Reply-To: <CABBYNZLTzW3afEPVfg=uS=xsPP-JpW6UBp6W=Urhhab+ai+dcA@mail.gmail.com>
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@intel.com>
> Date: Tue, 28 Jun 2022 15:46:04 -0700
> Subject: [PATCH] Bluetooth: L2CAP: WIP
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> 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
next prev parent reply other threads:[~2022-07-20 11:53 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 8:27 [RESEND 1/1] Bluetooth: Use chan_list_lock to protect the whole put/destroy invokation Lee Jones
2022-06-22 9:15 ` [RESEND,1/1] " bluez.test.bot
2022-06-27 14:17 ` [RESEND 1/1] " Lee Jones
2022-06-27 14:41 ` Eric Dumazet
2022-06-27 23:39 ` Luiz Augusto von Dentz
2022-06-28 18:36 ` Luiz Augusto von Dentz
2022-06-29 15:28 ` Lee Jones
2022-07-05 17:21 ` Luiz Augusto von Dentz
2022-07-06 10:53 ` Lee Jones
2022-07-06 20:36 ` Luiz Augusto von Dentz
2022-07-06 20:58 ` Luiz Augusto von Dentz
2022-07-14 17:46 ` Luiz Augusto von Dentz
2022-07-15 7:28 ` Lee Jones
2022-07-20 11:52 ` Lee Jones [this message]
2022-07-20 17:10 ` Luiz Augusto von Dentz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YtfsmHt4R/WxJOTV@google.com \
--to=lee.jones@linaro.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=johan.hedberg@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stable@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.