Return-Path: Date: Thu, 26 Apr 2012 11:22:16 +0300 From: Andrei Emeltchenko To: Mat Martineau Cc: linux-bluetooth@vger.kernel.org, gustavo@padovan.org, marcel@holtmann.org, pkrystad@codeaurora.org Subject: Re: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending Message-ID: <20120426082214.GC2659@aemeltch-MOBL1> References: <1335396979-11692-1-git-send-email-mathewm@codeaurora.org> <1335396979-11692-7-git-send-email-mathewm@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1335396979-11692-7-git-send-email-mathewm@codeaurora.org> List-ID: Hi Mat, On Wed, Apr 25, 2012 at 04:36:17PM -0700, Mat Martineau wrote: > The ERTM and streaming mode transmit queue must only be accessed while > the L2CAP channel lock is held. Adding this lock ensures that > multiple threads cannot simultaneously manipulate the queue when > sending and receiving concurrently. I think the lock shall be added in l2cap_sock_sendmsg like: lock() err = l2cap_chan_send(chan, msg, len, sk->sk_priority); unlock() I actually use this l2cap_chan_send in A2MP code already because it gets locked in l2cap_data_channel. Best regards Andrei Emeltchenko > > Signed-off-by: Mat Martineau > --- > net/bluetooth/l2cap_core.c | 40 ++++++++++++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 10 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 6aefeaa..8ed6a74 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -2035,26 +2035,35 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len, > u32 control; > int err; > > + l2cap_chan_hold(chan); > + > /* Connectionless channel */ > if (chan->chan_type == L2CAP_CHAN_CONN_LESS) { > skb = l2cap_create_connless_pdu(chan, msg, len, priority); > - if (IS_ERR(skb)) > - return PTR_ERR(skb); > + if (IS_ERR(skb)) { > + err = PTR_ERR(skb); > + goto done; > + } > > l2cap_do_send(chan, skb); > - return len; > + err = len; > + goto done; > } > > switch (chan->mode) { > case L2CAP_MODE_BASIC: > /* Check outgoing MTU */ > - if (len > chan->omtu) > - return -EMSGSIZE; > + if (len > chan->omtu) { > + err = -EMSGSIZE; > + break; > + } > > /* Create a basic PDU */ > skb = l2cap_create_basic_pdu(chan, msg, len, priority); > - if (IS_ERR(skb)) > - return PTR_ERR(skb); > + if (IS_ERR(skb)) { > + err = PTR_ERR(skb); > + break; > + } > > l2cap_do_send(chan, skb); > err = len; > @@ -2067,30 +2076,38 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len, > control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED); > skb = l2cap_create_iframe_pdu(chan, msg, len, control, > 0); > - if (IS_ERR(skb)) > - return PTR_ERR(skb); > + if (IS_ERR(skb)) { > + err = PTR_ERR(skb); > + break; > + } > > + l2cap_chan_lock(chan); > __skb_queue_tail(&chan->tx_q, skb); > > if (chan->tx_send_head == NULL) > chan->tx_send_head = skb; > > + l2cap_chan_unlock(chan); > } else { > /* Segment SDU into multiples PDUs */ > err = l2cap_sar_segment_sdu(chan, msg, len); > if (err < 0) > - return err; > + break; > } > > if (chan->mode == L2CAP_MODE_STREAMING) { > + l2cap_chan_lock(chan); > l2cap_streaming_send(chan); > + l2cap_chan_unlock(chan); > err = len; > break; > } > > + l2cap_chan_lock(chan); > if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) && > test_bit(CONN_WAIT_F, &chan->conn_state)) { > err = len; > + l2cap_chan_unlock(chan); > break; > } > > @@ -2098,6 +2115,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len, > if (err >= 0) > err = len; > > + l2cap_chan_unlock(chan); > break; > > default: > @@ -2105,6 +2123,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len, > err = -EBADFD; > } > > +done: > + l2cap_chan_put(chan); > return err; > } > > -- > 1.7.10 > > -- > Mat Martineau > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum