Hi Ulisses,
Thanks for the review, please see my comment below:
On Fri, Feb 03, 2012 at 07:14:24PM -0200, Ulisses Furquim wrote:
> On Fri, Feb 3, 2012 at 12:36 PM, 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.
>
> Just a comment. RCU updater side should be locked in most cases so
> that's not really a big reason for removing RCU usage here.
Yes, the main reason was sleeping inside RCU.
...
> > static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
> > {
> > - struct l2cap_chan *c, *r = NULL;
> > -
> > - rcu_read_lock();
> > + struct l2cap_chan *c;
> >
> > - list_for_each_entry_rcu(c, &conn->chan_l, list) {
> > - if (c->scid == cid) {
> > - r = c;
> > - break;
> > - }
> > + list_for_each_entry(c, &conn->chan_l, list) {
> > + if (c->scid == cid)
> > + return c;
> > }
> > -
> > - rcu_read_unlock();
> > - return r;
> > + return NULL;
> > }
> >
> > /* Find channel with given SCID.
> > @@ -115,36 +103,34 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
> > {
> > struct l2cap_chan *c;
> >
> > + mutex_lock(&conn->chan_lock);
> > c = __l2cap_get_chan_by_scid(conn, cid);
> > if (c)
> > lock_sock(c->sk);
> > + mutex_unlock(&conn->chan_lock);
> > return c;
> > }
>
> Ok. We have now chan_lock around the unlocked versions and we still
> return a locked sock. However, we do have to be careful not to open
> windows where we can deadlock. See my other comments below about that.
The function above is not used anywhere and should be removed. I think I
need to put chan_lock to __* versions of get channel.
> > static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
> > {
> > - struct l2cap_chan *c, *r = NULL;
> > -
> > - rcu_read_lock();
> > + struct l2cap_chan *c;
> >
> > - list_for_each_entry_rcu(c, &conn->chan_l, list) {
> > - if (c->ident == ident) {
> > - r = c;
> > - break;
> > - }
> > + list_for_each_entry(c, &conn->chan_l, list) {
> > + if (c->ident == ident)
> > + return c;
> > }
> > -
> > - rcu_read_unlock();
> > - return r;
> > + return NULL;
> > }
> >
> > static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
> > {
> > struct l2cap_chan *c;
> >
> > + mutex_lock(&conn->chan_lock);
> > c = __l2cap_get_chan_by_ident(conn, ident);
> > if (c)
> > lock_sock(c->sk);
> > + mutex_unlock(&conn->chan_lock);
> > return c;
> > }
> >
> > @@ -259,6 +245,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
> >
> > BT_DBG("chan %p state %d", chan, chan->state);
> >
> > + mutex_lock(&chan->conn->chan_lock);
> > lock_sock(sk);
> >
> > if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> > @@ -272,6 +259,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
> > l2cap_chan_close(chan, reason);
> >
> > release_sock(sk);
> > + mutex_unlock(&chan->conn->chan_lock);
> >
> > chan->ops->close(chan->data);
> > l2cap_chan_put(chan);
> > @@ -357,7 +345,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> >
> > l2cap_chan_hold(chan);
> >
> > - list_add_rcu(&chan->list, &conn->chan_l);
> > + mutex_lock(&conn->chan_lock);
> > + list_add(&chan->list, &conn->chan_l);
> > + mutex_unlock(&conn->chan_lock);
> > }
>
> Hmm, I'm not sure this is correct. Have you seen l2cap_connect_req()
> calls __l2cap_get_chan_by_dcid() without the chan_lock? It's like that
> because it was relying on RCU but now we do need to lock chan_lock
> there as well. I'd recommend we turn l2cap_chan_add() into
> __l2cap_chan_add() and lock chan_lock in the callers. Do you agree?
Yes, this sounds reasonable. We can lock with chan_lock l2cap_connect_req
so that socket lock remains inside chan_lock and lockdep is happy.
> > /* Delete channel.
> > @@ -374,8 +364,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> >
> > if (conn) {
> > /* Delete from channel list */
> > - list_del_rcu(&chan->list);
> > - synchronize_rcu();
> > + list_del(&chan->list);
> >
> > l2cap_chan_put(chan);
> >
> > @@ -426,10 +415,14 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
> > /* Close not yet accepted channels */
> > while ((sk = bt_accept_dequeue(parent, NULL))) {
> > struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> > +
> > + mutex_lock(&chan->conn->chan_lock);
> > __clear_chan_timer(chan);
> > lock_sock(sk);
> > l2cap_chan_close(chan, ECONNRESET);
> > release_sock(sk);
> > + mutex_unlock(&chan->conn->chan_lock);
> > +
> > chan->ops->close(chan->data);
> > }
> > }
>
> I see you're adding locks around l2cap_chan_close() instead of just
> locking list_del() which might be a good approach. Is that what you
> want? Before we had to drop the lock to call l2cap_chan_close()
Yes, the idea is to minimize locks/unlocks count.
> because it would be locked again to change the list (which I thought
> wasn't good). And could you please remove the leftover comment in
> l2cap_conn_start() about releasing the lock before calling
> l2cap_chan_close()?
Yes.
> Have you seen l2cap_sock_shutdown() in l2cap_sock.c also calls
> l2cap_chan_close()? Why don't we need to lock chan_lock there?
I think chan_lock is missing there.
> If that's what you wanted then please describe this kind of change in
> the commit message (at least). This way we can refer to it if anything
> happens or if we need to understand the change. I do think you write
> short commit messages so please be more verbose.
OK, I 'll try to write longer commit messages and split this commit to
several chunks so that it would be easier to understand them.
>
> > /* ---- L2CAP signalling commands ---- */
> > @@ -2755,6 +2753,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> > return -EFAULT;
> > }
> >
> > + mutex_lock(&conn->chan_lock);
> > sk = chan->sk;
> >
> > switch (result) {
> > @@ -2782,6 +2781,8 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> > }
> >
> > release_sock(sk);
> > + mutex_unlock(&conn->chan_lock);
> > +
> > return 0;
> > }
> > @@ -3032,6 +3033,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> > if (!chan)
> > return 0;
> >
> > + mutex_lock(&conn->chan_lock);
> > sk = chan->sk;
> >
> > rsp.dcid = cpu_to_le16(chan->scid);
> > @@ -3042,6 +3044,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;
>
> Here in l2cap_connect_rsp() and l2cap_disconnect_req() it seems we
> have a window where we can deadlock chan_lock and sock lock. Have you
> seen that? It can be that it may never happen but it'd be good to
> avoid that IMO. We receive a locked sock from either
> l2cap_get_chan_by_scid() or l2cap_get_chan_by_ident() and then we try
> to lock chan_lock. That's a different order you've been locking them.
> Maybe we can lock chan_lock then use the unlocked versions and
> explicitly lock sock after receiving chan?
Actually I am using unlocked versions of get_chan functions. But I will
move chan_lock before those functions since them have to be locked.
> > @@ -3063,6 +3066,7 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
> > if (!chan)
> > return 0;
> >
> > + mutex_lock(&conn->chan_lock);
> > sk = chan->sk;
> >
> > l2cap_chan_del(chan, 0);
>
> The last comment also applies here. Besides, aren't we missing an unlock?
Yes, apparently. See comment above.
> How much have you been testing these changes? I'd like to avoid
> introducing more bugs with changes are supposed to fix things so
> please do test this as much as you can.
I was testing it with my test scripts and I am going to test with PTS. But
still some bugs may be introduced since this is quite sensitive area.
Best regards
Andrei Emeltchenko
--
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
