Return-Path: MIME-Version: 1.0 In-Reply-To: <20120213144911.GC19288@aemeltch-MOBL1> References: <1328882113-19810-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1328882113-19810-3-git-send-email-Andrei.Emeltchenko.news@gmail.com> <20120213085841.GB21179@aemeltch-MOBL1> <20120213144911.GC19288@aemeltch-MOBL1> Date: Mon, 13 Feb 2012 22:06:10 -0300 Message-ID: Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list From: Ulisses Furquim To: Emeltchenko Andrei , Ulisses Furquim , linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andrei, On Mon, Feb 13, 2012 at 11:49 AM, Emeltchenko Andrei 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 > > > wrote: > > > > From: Andrei Emeltchenko > > > > > > > > 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 > > > > > > 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