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
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 <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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