Hi Andrei,
On Mon, Feb 13, 2012 at 11:49 AM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@xxxxxxxxx> wrote:
>
> Hi Ulisses,
>
> On Mon, Feb 13, 2012 at 10:58:43AM +0200, Emeltchenko Andrei wrote:
> > Hi Ulisses,
> >
> > On Fri, Feb 10, 2012 at 04:24:57PM -0200, Ulisses Furquim wrote:
> > > Hi Andrei,
> > >
> > > On Fri, Feb 10, 2012 at 11:54 AM, Emeltchenko Andrei
> > > <Andrei.Emeltchenko.news@xxxxxxxxx> wrote:
> > > > From: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> > > >
> > > > Usage of RCU list looks not reasonalbe for a number of reasons:
> > > > our code sleep and we have to use socket spinlocks, some parts
> > > > of code are updaters thus we need to use mutexes anyway.
> > > >
> > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> > >
> > > No need to lock and unlock conn->chan_lock in l2cap_disconnect_rsp()?
> > > This change from RCU to mutexes really should be just one commit IMO.
> >
> > I try to add chunks which are not in different patches but then this patch
> > would several hundreds lines long. If this OK I just merge them.
>
> I've picked chunks which might come after this patch that are dealing with
> locking conn->chan_lock. Please check it below. Do you think it needs to
> be merged with "RCU remove" patch?
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c0a35c5..356ce6e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -240,11 +240,13 @@ static void l2cap_chan_timeout(struct work_struct
> *work)
> {
> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> chan_timer.work);
> + struct l2cap_conn *conn = chan->conn;
> struct sock *sk = chan->sk;
> int reason;
>
> BT_DBG("chan %p state %d", chan, chan->state);
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(sk);
>
> if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> @@ -258,6 +260,7 @@ static void l2cap_chan_timeout(struct work_struct
> *work)
> l2cap_chan_close(chan, reason);
>
> release_sock(sk);
> + mutex_unlock(&conn->chan_lock);
>
> chan->ops->close(chan->data);
> l2cap_chan_put(chan);
> @@ -2619,6 +2622,7 @@ static inline int l2cap_connect_req(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
>
> parent = pchan->sk;
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(parent);
>
> /* Check if the ACL is secure enough (if not SDP) */
> @@ -2692,6 +2696,7 @@ static inline int l2cap_connect_req(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
>
> response:
> release_sock(parent);
> + mutex_unlock(&conn->chan_lock);
>
> sendresp:
> rsp.scid = cpu_to_le16(scid);
> @@ -2733,6 +2738,7 @@ static inline int l2cap_connect_rsp(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
> struct l2cap_chan *chan;
> struct sock *sk;
> u8 req[128];
> + int err;
>
> scid = __le16_to_cpu(rsp->scid);
> dcid = __le16_to_cpu(rsp->dcid);
> @@ -2742,16 +2748,24 @@ static inline int l2cap_connect_rsp(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
> dcid, scid, result,
> status);
>
> + mutex_lock(&conn->chan_lock);
> +
> if (scid) {
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> } else {
> - chan = l2cap_get_chan_by_ident(conn, cmd->ident);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> }
>
> + err = 0;
> +
> sk = chan->sk;
>
> switch (result) {
> @@ -2779,7 +2793,10 @@ static inline int l2cap_connect_rsp(struct
> l2cap_conn *conn, struct l2cap_cmd_hd
> }
>
> release_sock(sk);
> - return 0;
> +unlock:
> + mutex_unlock(&conn->chan_lock);
> +
> + return err;
> }
>
> static inline void set_default_fcs(struct l2cap_chan *chan)
> @@ -3025,9 +3042,13 @@ static inline int l2cap_disconnect_req(struct
> l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
>
> - chan = l2cap_get_chan_by_scid(conn, dcid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, dcid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
> + }
>
> sk = chan->sk;
>
> @@ -3039,6 +3060,7 @@ static inline int l2cap_disconnect_req(struct
> l2cap_conn *conn, struct l2cap_cmd
>
> l2cap_chan_del(chan, ECONNRESET);
> release_sock(sk);
> + mutex_unlock(&conn->chan_lock);
>
> chan->ops->close(chan->data);
> return 0;
> @@ -3056,14 +3078,19 @@ static inline int l2cap_disconnect_rsp(struct
> l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
>
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
> + }
>
> sk = chan->sk;
>
> l2cap_chan_del(chan, 0);
> release_sock(sk);
> + mutex_unlock(&conn->chan_lock);
>
> chan->ops->close(chan->data);
> return 0;
> --
> 1.7.8.3
Yes, I do think they belong together. And please, check l2cap_sock.c
where l2cap_chan_close() seems to be called without locking
conn->chan_lock in l2cap_sock_shutdown(). And please remove the bogus
comment below from l2cap_conn_start, ok?
/* l2cap_chan_close() calls list_del(chan)
* so release the lock */
Thanks,
Regards,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
