* [PATCH 0/2] l2cap_chan init and delete fixes for ERTM @ 2012-05-17 23:20 Mat Martineau 2012-05-17 23:20 ` [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del Mat Martineau 2012-05-17 23:20 ` [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails Mat Martineau 0 siblings, 2 replies; 7+ messages in thread From: Mat Martineau @ 2012-05-17 23:20 UTC (permalink / raw) To: linux-bluetooth, gustavo, marcel; +Cc: pkrystad 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del 2012-05-17 23:20 [PATCH 0/2] l2cap_chan init and delete fixes for ERTM Mat Martineau @ 2012-05-17 23:20 ` Mat Martineau 2012-05-18 2:30 ` Marcel Holtmann 2012-05-17 23:20 ` [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails Mat Martineau 1 sibling, 1 reply; 7+ messages in thread From: Mat Martineau @ 2012-05-17 23:20 UTC (permalink / raw) To: linux-bluetooth, gustavo, marcel; +Cc: pkrystad 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del 2012-05-17 23:20 ` [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del Mat Martineau @ 2012-05-18 2:30 ` Marcel Holtmann 2012-05-18 3:29 ` Mat Martineau 0 siblings, 1 reply; 7+ messages in thread From: Marcel Holtmann @ 2012-05-18 2:30 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth, gustavo, pkrystad 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del 2012-05-18 2:30 ` Marcel Holtmann @ 2012-05-18 3:29 ` Mat Martineau 2012-05-18 3:30 ` Marcel Holtmann 0 siblings, 1 reply; 7+ messages in thread From: Mat Martineau @ 2012-05-18 3:29 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth, gustavo, pkrystad 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del 2012-05-18 3:29 ` Mat Martineau @ 2012-05-18 3:30 ` Marcel Holtmann 0 siblings, 0 replies; 7+ messages in thread From: Marcel Holtmann @ 2012-05-18 3:30 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth, gustavo, pkrystad 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails 2012-05-17 23:20 [PATCH 0/2] l2cap_chan init and delete fixes for ERTM Mat Martineau 2012-05-17 23:20 ` [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del Mat Martineau @ 2012-05-17 23:20 ` Mat Martineau 2012-05-18 2:31 ` Marcel Holtmann 1 sibling, 1 reply; 7+ messages in thread From: Mat Martineau @ 2012-05-17 23:20 UTC (permalink / raw) To: linux-bluetooth, gustavo, marcel; +Cc: pkrystad 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails 2012-05-17 23:20 ` [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails Mat Martineau @ 2012-05-18 2:31 ` Marcel Holtmann 0 siblings, 0 replies; 7+ messages in thread From: Marcel Holtmann @ 2012-05-18 2:31 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth, gustavo, pkrystad 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-18 3:30 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-17 23:20 [PATCH 0/2] l2cap_chan init and delete fixes for ERTM Mat Martineau 2012-05-17 23:20 ` [PATCH 1/2] Bluetooth: Fix early return from l2cap_chan_del Mat Martineau 2012-05-18 2:30 ` Marcel Holtmann 2012-05-18 3:29 ` Mat Martineau 2012-05-18 3:30 ` Marcel Holtmann 2012-05-17 23:20 ` [PATCH 2/2] Bluetooth: Free allocated ERTM SREJ list if init fails Mat Martineau 2012-05-18 2:31 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).