Re: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending

[Mat Martineau <mathewm@codeaurora.org>]

Andrei -

On Thu, 26 Apr 2012, Andrei Emeltchenko wrote:

> 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.
>

You're right.

There's still a problem where the lock needs to be released when 
bt_skb_send_alloc is called, because that function can block.  What do 
you think about releasing and reacquiring the channel lock in 
l2cap_sock_alloc_skb_cb() instead?

>
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  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