[PATCH 0/2] l2cap_chan init and delete fixes for ERTM

[PATCH 0/2] l2cap_chan init and delete fixes for ERTM

[Mat Martineau]

I came across a repeatable crash when testing the new ERTM state
machine, and traced it down to some unexpected control flow in
l2cap_chan_del.  This also led me to discover a possible, but
unlikely, leak in some recently added l2cap_ertm_init code.

Mat Martineau (2):
  Bluetooth: Fix early return from l2cap_chan_del
  Bluetooth: Free allocated ERTM SREJ list if init fails

 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
1.7.10

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del

[Mat Martineau]

This fixes a regression from commit
2ead70b8390d199ca04cd35311b51f5f3676079e that is present in all
kernels starting at v3.0.

When L2CAP information was moved to struct l2cap_chan, a check was
added to l2cap_chan_del to avoid certain cleanup operations when ERTM
or streaming mode had not yet been initialized.  The logic in the
check did not take in to account that chan->conf_state is set to 0 in
l2cap_chan_ready, so l2cap_chan_del failed to cancel timers and leaked
memory any time the ERTM queues or lists were not empty.

This change makes sure that l2cap_chan_del only returns early if
ERTM initialization was not performed.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 1c7d1cd..452fcc4 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -597,6 +597,7 @@ enum {
 	CONF_EWS_RECV,
 	CONF_LOC_CONF_PEND,
 	CONF_REM_CONF_PEND,
+	CONF_NOT_COMPLETE,
 };
 
 #define L2CAP_CONF_MAX_CONF_REQ 2
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 24f144b..36edd42 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -391,6 +391,7 @@ struct l2cap_chan *l2cap_chan_create(void)
 	chan->state = BT_OPEN;
 
 	atomic_set(&chan->refcnt, 1);
+	set_bit(CONF_NOT_COMPLETE, &chan->conf_state);
 
 	BT_DBG("chan %p", chan);
 
@@ -509,8 +510,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 	release_sock(sk);
 
-	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
-			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
+	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
 		return;
 
 	skb_queue_purge(&chan->tx_q);
-- 
1.7.10

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails

[Mat Martineau]

If the ERTM SREJ list is properly allocated but the retransmit list
allocation fails, the SREJ list must be freed before returning from
l2cap_ertm_init.  l2cap_chan_del will not clean up the SREJ list
if l2cap_ertm_init returns a failure code.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 36edd42..aeb4e6e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2381,7 +2381,11 @@ static inline int l2cap_ertm_init(struct l2cap_chan *chan)
 	if (err < 0)
 		return err;
 
-	return l2cap_seq_list_init(&chan->retrans_list, chan->remote_tx_win);
+	err = l2cap_seq_list_init(&chan->retrans_list, chan->remote_tx_win);
+	if (err < 0)
+		l2cap_seq_list_free(&chan->srej_list);
+
+	return err;
 }
 
 static inline __u8 l2cap_select_mode(__u8 mode, __u16 remote_feat_mask)
-- 
1.7.10

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del

[Marcel Holtmann]

Hi Mat,

> This fixes a regression from commit
> 2ead70b8390d199ca04cd35311b51f5f3676079e that is present in all
> kernels starting at v3.0.
> 
> When L2CAP information was moved to struct l2cap_chan, a check was
> added to l2cap_chan_del to avoid certain cleanup operations when ERTM
> or streaming mode had not yet been initialized.  The logic in the
> check did not take in to account that chan->conf_state is set to 0 in
> l2cap_chan_ready, so l2cap_chan_del failed to cancel timers and leaked
> memory any time the ERTM queues or lists were not empty.
> 
> This change makes sure that l2cap_chan_del only returns early if
> ERTM initialization was not performed.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  net/bluetooth/l2cap_core.c    |    4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 1c7d1cd..452fcc4 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -597,6 +597,7 @@ enum {
>  	CONF_EWS_RECV,
>  	CONF_LOC_CONF_PEND,
>  	CONF_REM_CONF_PEND,
> +	CONF_NOT_COMPLETE,
>  };
>  
>  #define L2CAP_CONF_MAX_CONF_REQ 2
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 24f144b..36edd42 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -391,6 +391,7 @@ struct l2cap_chan *l2cap_chan_create(void)
>  	chan->state = BT_OPEN;
>  
>  	atomic_set(&chan->refcnt, 1);
> +	set_bit(CONF_NOT_COMPLETE, &chan->conf_state);
>  
>  	BT_DBG("chan %p", chan);
>  
> @@ -509,8 +510,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>  
>  	release_sock(sk);
>  
> -	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
> -			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
> +	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
>  		return;

explain to me how this works? We never clear that bit.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails

[Marcel Holtmann]

Hi Mat,

> If the ERTM SREJ list is properly allocated but the retransmit list
> allocation fails, the SREJ list must be freed before returning from
> l2cap_ertm_init.  l2cap_chan_del will not clean up the SREJ list
> if l2cap_ertm_init returns a failure code.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del

[Mat Martineau]

Hi Marcel -

On Thu, 17 May 2012, Marcel Holtmann wrote:

> Hi Mat,
>
>> This fixes a regression from commit
>> 2ead70b8390d199ca04cd35311b51f5f3676079e that is present in all
>> kernels starting at v3.0.
>>
>> When L2CAP information was moved to struct l2cap_chan, a check was
>> added to l2cap_chan_del to avoid certain cleanup operations when ERTM
>> or streaming mode had not yet been initialized.  The logic in the
>> check did not take in to account that chan->conf_state is set to 0 in
>> l2cap_chan_ready, so l2cap_chan_del failed to cancel timers and leaked
>> memory any time the ERTM queues or lists were not empty.
>>
>> This change makes sure that l2cap_chan_del only returns early if
>> ERTM initialization was not performed.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    1 +
>>  net/bluetooth/l2cap_core.c    |    4 ++--
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 1c7d1cd..452fcc4 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -597,6 +597,7 @@ enum {
>>  	CONF_EWS_RECV,
>>  	CONF_LOC_CONF_PEND,
>>  	CONF_REM_CONF_PEND,
>> +	CONF_NOT_COMPLETE,
>>  };
>>
>>  #define L2CAP_CONF_MAX_CONF_REQ 2
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 24f144b..36edd42 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -391,6 +391,7 @@ struct l2cap_chan *l2cap_chan_create(void)
>>  	chan->state = BT_OPEN;
>>
>>  	atomic_set(&chan->refcnt, 1);
>> +	set_bit(CONF_NOT_COMPLETE, &chan->conf_state);
>>
>>  	BT_DBG("chan %p", chan);
>>
>> @@ -509,8 +510,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>
>>  	release_sock(sk);
>>
>> -	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
>> -			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
>> +	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
>>  		return;
>
> explain to me how this works? We never clear that bit.

Line 926 in l2cap_chan_ready (called after configuration is done) is 
existing code that I didn't change:

         chan->conf_state = 0;

So all bits are cleared at once, including CONF_NOT_COMPLETE.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del

[Marcel Holtmann]

Hi Mat,

> >> This fixes a regression from commit
> >> 2ead70b8390d199ca04cd35311b51f5f3676079e that is present in all
> >> kernels starting at v3.0.
> >>
> >> When L2CAP information was moved to struct l2cap_chan, a check was
> >> added to l2cap_chan_del to avoid certain cleanup operations when ERTM
> >> or streaming mode had not yet been initialized.  The logic in the
> >> check did not take in to account that chan->conf_state is set to 0 in
> >> l2cap_chan_ready, so l2cap_chan_del failed to cancel timers and leaked
> >> memory any time the ERTM queues or lists were not empty.
> >>
> >> This change makes sure that l2cap_chan_del only returns early if
> >> ERTM initialization was not performed.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >>  include/net/bluetooth/l2cap.h |    1 +
> >>  net/bluetooth/l2cap_core.c    |    4 ++--
> >>  2 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >> index 1c7d1cd..452fcc4 100644
> >> --- a/include/net/bluetooth/l2cap.h
> >> +++ b/include/net/bluetooth/l2cap.h
> >> @@ -597,6 +597,7 @@ enum {
> >>  	CONF_EWS_RECV,
> >>  	CONF_LOC_CONF_PEND,
> >>  	CONF_REM_CONF_PEND,
> >> +	CONF_NOT_COMPLETE,
> >>  };
> >>
> >>  #define L2CAP_CONF_MAX_CONF_REQ 2
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index 24f144b..36edd42 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -391,6 +391,7 @@ struct l2cap_chan *l2cap_chan_create(void)
> >>  	chan->state = BT_OPEN;
> >>
> >>  	atomic_set(&chan->refcnt, 1);
> >> +	set_bit(CONF_NOT_COMPLETE, &chan->conf_state);
> >>
> >>  	BT_DBG("chan %p", chan);
> >>
> >> @@ -509,8 +510,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> >>
> >>  	release_sock(sk);
> >>
> >> -	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
> >> -			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
> >> +	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
> >>  		return;
> >
> > explain to me how this works? We never clear that bit.
> 
> Line 926 in l2cap_chan_ready (called after configuration is done) is 
> existing code that I didn't change:
> 
>          chan->conf_state = 0;
> 
> So all bits are cleared at once, including CONF_NOT_COMPLETE.

that stuff needs a comment somewhere in the code. Best is to mention it
when calling set_bit().

Regards

Marcel