This is a collection of ERTM-related patches, including some L2CAP
configuration fixes and allowing partial-frame reads from L2CAP
SOCK_STREAM sockets. PSM validation is also included, although that
is not restricted to ERTM.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
* Mat Martineau <[email protected]> [2010-08-03 08:58:57 -0700]:
>
> On Mon, 2 Aug 2010, Marcel Holtmann wrote:
>
> >Hi Mat,
> >
> >>>* Marcel Holtmann <[email protected]> [2010-08-02 12:38:32 -0700]:
> >>>
> >>>>Hi Mat,
> >>>>
> >>>>>Signed-off-by: Mat Martineau <[email protected]>
> >>>>>---
> >>>>> net/bluetooth/l2cap.c | 12 ++++++++----
> >>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>
> >>>>>diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>>index 9ba1e8e..aed72f2 100644
> >>>>>--- a/net/bluetooth/l2cap.c
> >>>>>+++ b/net/bluetooth/l2cap.c
> >>>>>@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>> goto unlock;
> >>>>>
> >>>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>>- if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>- l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>>+ if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>>+ l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>>+ (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>+ l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>
> >>>>this becomes unreadable and my brain starts to throw a core dump. So it
> >>>>clearly needs to be put into a helper inline function.
> >>>
> >>>Actually we don't need that, since the code that deals with Basic Mode
> >>>never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>>value in the Basic Mode.
> >>
> >>There isn't currently any Basic Mode code that triggers this latent
> >>bug, but I have a patch coming up that does require this fix.
> >>
> >>As it stands, getsockopt() on a connected basic mode socket shows FCS
> >>enabled, so this bug is visible from userspace.
> >
> >can we just fail the setsockopt() when trying to set basic mode and FCS
> >off.
>
> It definitely makes sense to have more validation of L2CAP_OPTIONS
> passed to setsockopt().
>
> >And also in case fallback to basic mode happens, then FCS should be set
> >to be enabled. Since for FCS and basic mode we always have to use FCS.
> >So that seems just fine to me.
>
> The spec says "The FCS option shall only be used when the mode is
> being, or is already configured to Enhanced Retransmission mode or
> Streaming mode." FCS is never used in basic mode (fallback or not).
>
> (Maybe I've misunderstood your point)
>
> >Maybe you need to explain a bit more in detail what you are trying to
> >achieve in conjunction with userspace API.
>
> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when
> the FCS option is actually in use. Otherwise, any logic checking
> for FCS also has to check the L2CAP mode. Might as well check the
> mode once and set fcs accordingly -- which is what my patch does.
>
> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked
> on code paths used with ERTM and streaming mode. However, future
> code (including a patch I'll be posting soon) will depend on the fcs
> value being accurate in all modes.
So queue this togheter with the one you are going to send soon. It will
be easy the real need for the checks you are doing here.
>
> I only mentioned getsockopt() to show that this issue is not
> completely invisible, and is worth patching.
You can also set l2cap_pi(sk)->fcs to NOFCS when the mode configured
is Basic Mode.
--
Gustavo F. Padovan
http://padovan.org
Hi Mat,
> >>>>>>> Signed-off-by: Mat Martineau <[email protected]>
> >>>>>>> ---
> >>>>>>> net/bluetooth/l2cap.c | 12 ++++++++----
> >>>>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>>>> index 9ba1e8e..aed72f2 100644
> >>>>>>> --- a/net/bluetooth/l2cap.c
> >>>>>>> +++ b/net/bluetooth/l2cap.c
> >>>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>>>> goto unlock;
> >>>>>>>
> >>>>>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>>>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>>>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>>>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>>>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>>>
> >>>>>> this becomes unreadable and my brain starts to throw a core dump. So it
> >>>>>> clearly needs to be put into a helper inline function.
> >>>>>
> >>>>> Actually we don't need that, since the code that deals with Basic Mode
> >>>>> never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>>>> value in the Basic Mode.
> >>>>
> >>>> There isn't currently any Basic Mode code that triggers this latent
> >>>> bug, but I have a patch coming up that does require this fix.
> >>>>
> >>>> As it stands, getsockopt() on a connected basic mode socket shows FCS
> >>>> enabled, so this bug is visible from userspace.
> >>>
> >>> can we just fail the setsockopt() when trying to set basic mode and FCS
> >>> off.
> >>
> >> It definitely makes sense to have more validation of L2CAP_OPTIONS
> >> passed to setsockopt().
> >>
> >>> And also in case fallback to basic mode happens, then FCS should be set
> >>> to be enabled. Since for FCS and basic mode we always have to use FCS.
> >>> So that seems just fine to me.
> >>
> >> The spec says "The FCS option shall only be used when the mode is
> >> being, or is already configured to Enhanced Retransmission mode or
> >> Streaming mode." FCS is never used in basic mode (fallback or not).
> >>
> >> (Maybe I've misunderstood your point)
> >
> > So basic mode uses CRC16 as FCS. And with basic that is not changeable.
> > If you try basic mode + no FCS then we should clearly fail the call to
> > setsockopt() for that.
>
> That's the opposite of what I'm trying to say! :)
>
> Basic mode B-frames have *no* FCS (L2CAP spec section 3.1). The FCS
> field is only found in I-frames and S-frames used in ERTM and
> streaming modes. If you try basic mode + FCS, that should fail.
I confused myself right now. You are fully correct. The default for
basic mode is no fcs.
And as soon as an application wants to use ERTM we just make it to
specific the FCS option. That being CRC16 or no FCS. And of course the
getsockopt() call will return whatever we ended up configuring.
And yes, basic mode + crc16 should fail.
> >>> Maybe you need to explain a bit more in detail what you are trying to
> >>> achieve in conjunction with userspace API.
> >>
> >> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
> >> FCS option is actually in use. Otherwise, any logic checking for FCS
> >> also has to check the L2CAP mode. Might as well check the mode once
> >> and set fcs accordingly -- which is what my patch does.
> >>
> >> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
> >> code paths used with ERTM and streaming mode. However, future code
> >> (including a patch I'll be posting soon) will depend on the fcs value
> >> being accurate in all modes.
> >
> > As I said, I have no problem with returning basic mode + crc16. Since
> > that is actually what you are doing in that case.
> >
> > The only thing that we have to keep in mind to be backward compatible is
> > to allow setting of basic mode and fcs = 0x00 and then change that into
> > crc16.
>
> The fcs field in L2CAP_OPTIONS is new for ERTM/Streaming modes, so
> there should be no backward compatibility issues with basic mode.
There is none. I got confused. Too many emails at the same time.
Regards
Marcel
Hi Marcel -
On Tue, 3 Aug 2010, Marcel Holtmann wrote:
>>>>>>> Signed-off-by: Mat Martineau <[email protected]>
>>>>>>> ---
>>>>>>> net/bluetooth/l2cap.c | 12 ++++++++----
>>>>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>>>>>> index 9ba1e8e..aed72f2 100644
>>>>>>> --- a/net/bluetooth/l2cap.c
>>>>>>> +++ b/net/bluetooth/l2cap.c
>>>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>>>>>> goto unlock;
>>>>>>>
>>>>>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>>>>>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>>>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>>>>>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>>>>>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>>>>>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>>>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>>>>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>>>>>
>>>>>> this becomes unreadable and my brain starts to throw a core dump. So it
>>>>>> clearly needs to be put into a helper inline function.
>>>>>
>>>>> Actually we don't need that, since the code that deals with Basic Mode
>>>>> never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
>>>>> value in the Basic Mode.
>>>>
>>>> There isn't currently any Basic Mode code that triggers this latent
>>>> bug, but I have a patch coming up that does require this fix.
>>>>
>>>> As it stands, getsockopt() on a connected basic mode socket shows FCS
>>>> enabled, so this bug is visible from userspace.
>>>
>>> can we just fail the setsockopt() when trying to set basic mode and FCS
>>> off.
>>
>> It definitely makes sense to have more validation of L2CAP_OPTIONS
>> passed to setsockopt().
>>
>>> And also in case fallback to basic mode happens, then FCS should be set
>>> to be enabled. Since for FCS and basic mode we always have to use FCS.
>>> So that seems just fine to me.
>>
>> The spec says "The FCS option shall only be used when the mode is
>> being, or is already configured to Enhanced Retransmission mode or
>> Streaming mode." FCS is never used in basic mode (fallback or not).
>>
>> (Maybe I've misunderstood your point)
>
> So basic mode uses CRC16 as FCS. And with basic that is not changeable.
> If you try basic mode + no FCS then we should clearly fail the call to
> setsockopt() for that.
That's the opposite of what I'm trying to say! :)
Basic mode B-frames have *no* FCS (L2CAP spec section 3.1). The FCS
field is only found in I-frames and S-frames used in ERTM and
streaming modes. If you try basic mode + FCS, that should fail.
>>> Maybe you need to explain a bit more in detail what you are trying to
>>> achieve in conjunction with userspace API.
>>
>> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
>> FCS option is actually in use. Otherwise, any logic checking for FCS
>> also has to check the L2CAP mode. Might as well check the mode once
>> and set fcs accordingly -- which is what my patch does.
>>
>> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
>> code paths used with ERTM and streaming mode. However, future code
>> (including a patch I'll be posting soon) will depend on the fcs value
>> being accurate in all modes.
>
> As I said, I have no problem with returning basic mode + crc16. Since
> that is actually what you are doing in that case.
>
> The only thing that we have to keep in mind to be backward compatible is
> to allow setting of basic mode and fcs = 0x00 and then change that into
> crc16.
The fcs field in L2CAP_OPTIONS is new for ERTM/Streaming modes, so
there should be no backward compatibility issues with basic mode.
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Marcel,
On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> Hi Mat,
>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> include/net/bluetooth/l2cap.h | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> this should have a commit message with technical details to explain why
> it is a good idea.
>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 16e412f..93aba17 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -35,7 +35,7 @@
>> #define L2CAP_DEFAULT_MAX_TX 3
>> #define L2CAP_DEFAULT_RETRANS_TO 2000 /* 2 seconds */
>> #define L2CAP_DEFAULT_MONITOR_TO 12000 /* 12 seconds */
>> -#define L2CAP_DEFAULT_MAX_PDU_SIZE 672
>> +#define L2CAP_DEFAULT_MAX_PDU_SIZE 1011 /* Sized for 3-DH5 packet */
>> #define L2CAP_DEFAULT_ACK_TO 200
>> #define L2CAP_LOCAL_BUSY_TRIES 12
>
> Also the default PDU size for basic mode should stay with 672. And only
> for ERTM move it to 1010. Is this ensured?
>
> And then again here. Is that explained in the commit message?
It is ensured that the max PDU size is only used for ERTM and
streaming mode. The default MTU for basic mode and ERTM/streaming
remains unchanged.
I will update the commit message to fully describe this.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi mat,
> >>>>> Signed-off-by: Mat Martineau <[email protected]>
> >>>>> ---
> >>>>> net/bluetooth/l2cap.c | 12 ++++++++----
> >>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>> index 9ba1e8e..aed72f2 100644
> >>>>> --- a/net/bluetooth/l2cap.c
> >>>>> +++ b/net/bluetooth/l2cap.c
> >>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>> goto unlock;
> >>>>>
> >>>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>
> >>>> this becomes unreadable and my brain starts to throw a core dump. So it
> >>>> clearly needs to be put into a helper inline function.
> >>>
> >>> Actually we don't need that, since the code that deals with Basic Mode
> >>> never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>> value in the Basic Mode.
> >>
> >> There isn't currently any Basic Mode code that triggers this latent
> >> bug, but I have a patch coming up that does require this fix.
> >>
> >> As it stands, getsockopt() on a connected basic mode socket shows FCS
> >> enabled, so this bug is visible from userspace.
> >
> > can we just fail the setsockopt() when trying to set basic mode and FCS
> > off.
>
> It definitely makes sense to have more validation of L2CAP_OPTIONS
> passed to setsockopt().
>
> > And also in case fallback to basic mode happens, then FCS should be set
> > to be enabled. Since for FCS and basic mode we always have to use FCS.
> > So that seems just fine to me.
>
> The spec says "The FCS option shall only be used when the mode is
> being, or is already configured to Enhanced Retransmission mode or
> Streaming mode." FCS is never used in basic mode (fallback or not).
>
> (Maybe I've misunderstood your point)
So basic mode uses CRC16 as FCS. And with basic that is not changeable.
If you try basic mode + no FCS then we should clearly fail the call to
setsockopt() for that.
> > Maybe you need to explain a bit more in detail what you are trying to
> > achieve in conjunction with userspace API.
>
> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
> FCS option is actually in use. Otherwise, any logic checking for FCS
> also has to check the L2CAP mode. Might as well check the mode once
> and set fcs accordingly -- which is what my patch does.
>
> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
> code paths used with ERTM and streaming mode. However, future code
> (including a patch I'll be posting soon) will depend on the fcs value
> being accurate in all modes.
As I said, I have no problem with returning basic mode + crc16. Since
that is actually what you are doing in that case.
The only thing that we have to keep in mind to be backward compatible is
to allow setting of basic mode and fcs = 0x00 and then change that into
crc16.
Regards
Marcel
On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> Hi Mat,
>
>>> * Marcel Holtmann <[email protected]> [2010-08-02 12:38:32 -0700]:
>>>
>>>> Hi Mat,
>>>>
>>>>> Signed-off-by: Mat Martineau <[email protected]>
>>>>> ---
>>>>> net/bluetooth/l2cap.c | 12 ++++++++----
>>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>>>> index 9ba1e8e..aed72f2 100644
>>>>> --- a/net/bluetooth/l2cap.c
>>>>> +++ b/net/bluetooth/l2cap.c
>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>>>> goto unlock;
>>>>>
>>>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>>>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>>>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>>>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>>>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>>>
>>>> this becomes unreadable and my brain starts to throw a core dump. So it
>>>> clearly needs to be put into a helper inline function.
>>>
>>> Actually we don't need that, since the code that deals with Basic Mode
>>> never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
>>> value in the Basic Mode.
>>
>> There isn't currently any Basic Mode code that triggers this latent
>> bug, but I have a patch coming up that does require this fix.
>>
>> As it stands, getsockopt() on a connected basic mode socket shows FCS
>> enabled, so this bug is visible from userspace.
>
> can we just fail the setsockopt() when trying to set basic mode and FCS
> off.
It definitely makes sense to have more validation of L2CAP_OPTIONS
passed to setsockopt().
> And also in case fallback to basic mode happens, then FCS should be set
> to be enabled. Since for FCS and basic mode we always have to use FCS.
> So that seems just fine to me.
The spec says "The FCS option shall only be used when the mode is
being, or is already configured to Enhanced Retransmission mode or
Streaming mode." FCS is never used in basic mode (fallback or not).
(Maybe I've misunderstood your point)
> Maybe you need to explain a bit more in detail what you are trying to
> achieve in conjunction with userspace API.
My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
FCS option is actually in use. Otherwise, any logic checking for FCS
also has to check the L2CAP mode. Might as well check the mode once
and set fcs accordingly -- which is what my patch does.
Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
code paths used with ERTM and streaming mode. However, future code
(including a patch I'll be posting soon) will depend on the fcs value
being accurate in all modes.
I only mentioned getsockopt() to show that this issue is not
completely invisible, and is worth patching.
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Marcel & Gustavo -
On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> Hi Mat,
<snip>
> since 2.6.36 will be the first kernel we enable ERTM by default, I
> prefer that we have known bugs/regressions actually fixed.
Gustavo has done a lot of great work to get ERTM ready to be turned on
by default, but there is one potential issue I see with ERTM that I'd
like to discuss: locking.
Kernel timers are used for the ack, retransmit, and monitor timeouts.
The handler functions for these timers use bh_lock_sock() and
bh_unlock_sock() to protect socket state. While this does protect
against simultaneous timeouts, or access in tasklet context for
incoming HCI data, it doesn't protect against access in user context.
Here's a scenario:
* Application calls send()
* lock_sock() is called in l2cap_sock_sendmsg()
* L2CAP code starts to modify l2cap_pinfo(sk) state in user context
* Timer fires, preempting l2cap_sock_sendmsg() or on another CPU
* Timer does not spin on bh_lock_sock()
* Timer handler changes l2cap_pinfo(sk) state
* Timer calls bh_unlock_sock() and finishes
* Code called by l2cap_sock_sendmsg() continues running, but
l2cap_pinfo(sk) was changed by the timer.
* release_sock() is called and l2cap_sock_sendmsg() returns.
It doesn't look like this would lead to a kernel panic, but can
cause data coherency problems in l2cap_pinfo(sk) (especially
l2cap_pinfo(sk)->conn_state). The most likely symptom would be
spurious disconnects when the other end of the ERTM connection sees
some odd behavior.
One solution is to use a workqueue for these timeouts, and then use
lock_sock() and release_sock() to protect l2cap_pinfo(sk) in the
workqueue handlers. There's a big catch, though: the socket might be
locked for a while if bt_skb_send_alloc() blocks and ERTM is not
sending data from the TX queue (it might be retransmitting frames
instead). As long as the lock is held, all the incoming data is put
in the socket backlog and any ack/retrans/monitor timers that fire on
the workqueue will block everything on that queue. The ERTM
connection can stall.
I think the ERTM code would need to either release the socket lock
when calling bt_skb_send_alloc(), or another spinlock is needed to
protect l2cap_pi(sk). Per-socket workqueues might be a good idea too,
so one locked socket can't block timeouts on other sockets.
This is a non-issue in basic mode, because no socket state is changed
when data is sent or received.
Do you agree that there's an ERTM issue here to be addressed? Any
ideas regarding the workqueue solution?
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
> > * Marcel Holtmann <[email protected]> [2010-08-02 12:38:32 -0700]:
> >
> >> Hi Mat,
> >>
> >>> Signed-off-by: Mat Martineau <[email protected]>
> >>> ---
> >>> net/bluetooth/l2cap.c | 12 ++++++++----
> >>> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>> index 9ba1e8e..aed72f2 100644
> >>> --- a/net/bluetooth/l2cap.c
> >>> +++ b/net/bluetooth/l2cap.c
> >>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>> goto unlock;
> >>>
> >>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>
> >> this becomes unreadable and my brain starts to throw a core dump. So it
> >> clearly needs to be put into a helper inline function.
> >
> > Actually we don't need that, since the code that deals with Basic Mode
> > never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> > value in the Basic Mode.
>
> There isn't currently any Basic Mode code that triggers this latent
> bug, but I have a patch coming up that does require this fix.
>
> As it stands, getsockopt() on a connected basic mode socket shows FCS
> enabled, so this bug is visible from userspace.
can we just fail the setsockopt() when trying to set basic mode and FCS
off.
And also in case fallback to basic mode happens, then FCS should be set
to be enabled. Since for FCS and basic mode we always have to use FCS.
So that seems just fine to me.
Maybe you need to explain a bit more in detail what you are trying to
achieve in conjunction with userspace API.
Regards
Marcel
Hi Gustavo -
On Mon, 2 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Marcel Holtmann <[email protected]> [2010-08-02 12:38:32 -0700]:
>
>> Hi Mat,
>>
>>> Signed-off-by: Mat Martineau <[email protected]>
>>> ---
>>> net/bluetooth/l2cap.c | 12 ++++++++----
>>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>> index 9ba1e8e..aed72f2 100644
>>> --- a/net/bluetooth/l2cap.c
>>> +++ b/net/bluetooth/l2cap.c
>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>> goto unlock;
>>>
>>> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>
>> this becomes unreadable and my brain starts to throw a core dump. So it
>> clearly needs to be put into a helper inline function.
>
> Actually we don't need that, since the code that deals with Basic Mode
> never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> value in the Basic Mode.
There isn't currently any Basic Mode code that triggers this latent
bug, but I have a patch coming up that does require this fix.
As it stands, getsockopt() on a connected basic mode socket shows FCS
enabled, so this bug is visible from userspace.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Marcel,
On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> Hi Mat,
>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> include/net/bluetooth/bluetooth.h | 2 +
>> net/bluetooth/af_bluetooth.c | 107 +++++++++++++++++++++++++++++++++++++
>> net/bluetooth/rfcomm/sock.c | 104 ++---------------------------------
>> 3 files changed, 115 insertions(+), 98 deletions(-)
>
> looks all like a good idea, but I really have to insist that the commit
> message explain everything in detail. Give a reason why the code is
> similar or the same and why this makes sense.
>
> Also splitting these into two or more patches makes sense. One adding
> the stream receive and the other modifying L2CAP and RFCOMM to use it.
Agreed.
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 27a902d..08b6c2a 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -126,6 +126,8 @@ int bt_sock_unregister(int proto);
>> void bt_sock_link(struct bt_sock_list *l, struct sock *s);
>> void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
>> int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
>> +int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>> + struct msghdr *msg, size_t len, int flags);
>> uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
>> int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
>> int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 421c45b..73047f5 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>> }
>> EXPORT_SYMBOL(bt_sock_recvmsg);
>>
>> +static long bt_sock_data_wait(struct sock *sk, long timeo)
>> +{
>> + DECLARE_WAITQUEUE(wait, current);
>> +
>> + add_wait_queue(sk_sleep(sk), &wait);
>> + for (;;) {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> +
>> + if (!skb_queue_empty(&sk->sk_receive_queue) ||
>> + sk->sk_err ||
>> + (sk->sk_shutdown & RCV_SHUTDOWN) ||
>> + signal_pending(current) ||
>> + !timeo)
>> + break;
>
> This makes my brain hurt. Please lets do this readable and with proper
> coding style and/or a helper function.
That's unchanged code from rfcomm_sock_data_wait() in rfcomm/sock.c -
but I'll go ahead and break it up using multiple 'if' statements.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
> >> This is a collection of ERTM-related patches, including some L2CAP
> >> configuration fixes and allowing partial-frame reads from L2CAP
> >> SOCK_STREAM sockets. PSM validation is also included, although that
> >> is not restricted to ERTM.
> >
> > so the merge window for 2.6.36 is basically closed and these patches
> > came in too late. If they fix a bug, I like you to explain that in
> > detail in the commit message so I can pick them out and include them for
> > submission to John.
>
> I'll fix up my commit messages and make the other changes you
> recommended. I don't have a specific need for these to be in 2.6.36,
> but with more detail on the bug fixes you can decide if they're urgent
> enough.
since 2.6.36 will be the first kernel we enable ERTM by default, I
prefer that we have known bugs/regressions actually fixed.
Regards
Marcel
On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> Hi Mat,
>
>> This is a collection of ERTM-related patches, including some L2CAP
>> configuration fixes and allowing partial-frame reads from L2CAP
>> SOCK_STREAM sockets. PSM validation is also included, although that
>> is not restricted to ERTM.
>
> so the merge window for 2.6.36 is basically closed and these patches
> came in too late. If they fix a bug, I like you to explain that in
> detail in the commit message so I can pick them out and include them for
> submission to John.
Marcel -
I'll fix up my commit messages and make the other changes you
recommended. I don't have a specific need for these to be in 2.6.36,
but with more detail on the bug fixes you can decide if they're urgent
enough.
Thanks for the feedback.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
* Marcel Holtmann <[email protected]> [2010-08-02 12:39:51 -0700]:
> Hi Mat,
>
> > Signed-off-by: Mat Martineau <[email protected]>
> > ---
> > net/bluetooth/l2cap.c | 12 ++++++++++++
> > 1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index aed72f2..44bc6ee 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> > goto done;
> > }
> >
> > + /* If specified, PSM must be odd and lsb of upper byte must be 0 */
> > + if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> > + err = -EINVAL;
> > + goto done;
> > + }
> > +
>
> this was a nice feature of L2CAP in BlueZ actually ;)
And please add a commit message for this patch.
--
Gustavo F. Padovan
http://padovan.org
Hi Mat,
* Marcel Holtmann <[email protected]> [2010-08-02 12:38:32 -0700]:
> Hi Mat,
>
> > Signed-off-by: Mat Martineau <[email protected]>
> > ---
> > net/bluetooth/l2cap.c | 12 ++++++++----
> > 1 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index 9ba1e8e..aed72f2 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > goto unlock;
> >
> > if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> > - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> > + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> > + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> > + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> > l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>
> this becomes unreadable and my brain starts to throw a core dump. So it
> clearly needs to be put into a helper inline function.
Actually we don't need that, since the code that deals with Basic Mode
never check and use the l2cap_pi(sk)->fcs. So we don't care about FCS
value in the Basic Mode.
--
Gustavo F. Padovan
http://padovan.org
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 582975b..8e9fa51 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1923,6 +1923,7 @@ done:
> static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
> {
> struct sock *sk = sock->sk;
> + int len_or_err;
>
> lock_sock(sk);
>
> @@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
>
> release_sock(sk);
>
> - return bt_sock_recvmsg(iocb, sock, msg, len, flags);
> + if (sock->type == SOCK_STREAM)
> + len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
> + flags);
> + else
> + len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
> +
> + return len_or_err;
> }
I don't like this variable name. Just call it "len" or just use "ret"
here. Check what other parts of net/bluetooth/ are using.
Also since you are not checking is value anyway, why not return right
away from the if.
if (sock->type == SOCK_STREAM)
return bt_sock_stream_recvmsg(...);
return bt_sock_recvmsg(...);
That way the compiler should also not complain. And in addition your
patch looks dead simple ;)
Regards
Marcel
Hi Mat,
> This is a collection of ERTM-related patches, including some L2CAP
> configuration fixes and allowing partial-frame reads from L2CAP
> SOCK_STREAM sockets. PSM validation is also included, although that
> is not restricted to ERTM.
so the merge window for 2.6.36 is basically closed and these patches
came in too late. If they fix a bug, I like you to explain that in
detail in the commit message so I can pick them out and include them for
submission to John.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
this should have a commit message with technical details to explain why
it is a good idea.
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 16e412f..93aba17 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -35,7 +35,7 @@
> #define L2CAP_DEFAULT_MAX_TX 3
> #define L2CAP_DEFAULT_RETRANS_TO 2000 /* 2 seconds */
> #define L2CAP_DEFAULT_MONITOR_TO 12000 /* 12 seconds */
> -#define L2CAP_DEFAULT_MAX_PDU_SIZE 672
> +#define L2CAP_DEFAULT_MAX_PDU_SIZE 1011 /* Sized for 3-DH5 packet */
> #define L2CAP_DEFAULT_ACK_TO 200
> #define L2CAP_LOCAL_BUSY_TRIES 12
Also the default PDU size for basic mode should stay with 672. And only
for ERTM move it to 1010. Is this ensured?
And then again here. Is that explained in the commit message?
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/bluetooth.h | 2 +
> net/bluetooth/af_bluetooth.c | 107 +++++++++++++++++++++++++++++++++++++
> net/bluetooth/rfcomm/sock.c | 104 ++---------------------------------
> 3 files changed, 115 insertions(+), 98 deletions(-)
looks all like a good idea, but I really have to insist that the commit
message explain everything in detail. Give a reason why the code is
similar or the same and why this makes sense.
Also splitting these into two or more patches makes sense. One adding
the stream receive and the other modifying L2CAP and RFCOMM to use it.
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 27a902d..08b6c2a 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -126,6 +126,8 @@ int bt_sock_unregister(int proto);
> void bt_sock_link(struct bt_sock_list *l, struct sock *s);
> void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
> int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
> +int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> + struct msghdr *msg, size_t len, int flags);
> uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
> int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 421c45b..73047f5 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> }
> EXPORT_SYMBOL(bt_sock_recvmsg);
>
> +static long bt_sock_data_wait(struct sock *sk, long timeo)
> +{
> + DECLARE_WAITQUEUE(wait, current);
> +
> + add_wait_queue(sk_sleep(sk), &wait);
> + for (;;) {
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + if (!skb_queue_empty(&sk->sk_receive_queue) ||
> + sk->sk_err ||
> + (sk->sk_shutdown & RCV_SHUTDOWN) ||
> + signal_pending(current) ||
> + !timeo)
> + break;
This makes my brain hurt. Please lets do this readable and with proper
coding style and/or a helper function.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
please write a proper commit message. Your subject is not meant to be
used for explaining what is wrong and what is right and how you did it.
It is called a SUBJECT.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap.c | 9 ++++-----
> 1 files changed, 4 insertions(+), 5 deletions(-)
same for subject line vs commit message. And please explain why and if
this fixes a bug.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
I do expect some more details commit messages. The subject line is
suppose to around 50 chars and give an idea what the patch is about. The
actual commit message should have the full blown explanation why etc.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aed72f2..44bc6ee 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> goto done;
> }
>
> + /* If specified, PSM must be odd and lsb of upper byte must be 0 */
> + if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> + err = -EINVAL;
> + goto done;
> + }
> +
this was a nice feature of L2CAP in BlueZ actually ;)
I am fine with forbidding this. However, please send a patch that fixes
l2test's default PSM first. Otherwise you gonna break that one.
Regards
Marcel
Hi Mat,
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 9ba1e8e..aed72f2 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> goto unlock;
>
> if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> - if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> - l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> + if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> + l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> + (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> + l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
this becomes unreadable and my brain starts to throw a core dump. So it
clearly needs to be put into a helper inline function.
Regards
Marcel
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/l2cap.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 16e412f..93aba17 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -35,7 +35,7 @@
#define L2CAP_DEFAULT_MAX_TX 3
#define L2CAP_DEFAULT_RETRANS_TO 2000 /* 2 seconds */
#define L2CAP_DEFAULT_MONITOR_TO 12000 /* 12 seconds */
-#define L2CAP_DEFAULT_MAX_PDU_SIZE 672
+#define L2CAP_DEFAULT_MAX_PDU_SIZE 1011 /* Sized for 3-DH5 packet */
#define L2CAP_DEFAULT_ACK_TO 200
#define L2CAP_LOCAL_BUSY_TRIES 12
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 582975b..8e9fa51 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1923,6 +1923,7 @@ done:
static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
{
struct sock *sk = sock->sk;
+ int len_or_err;
lock_sock(sk);
@@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
release_sock(sk);
- return bt_sock_recvmsg(iocb, sock, msg, len, flags);
+ if (sock->type == SOCK_STREAM)
+ len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
+ flags);
+ else
+ len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
+
+ return len_or_err;
}
static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/bluetooth.h | 2 +
net/bluetooth/af_bluetooth.c | 107 +++++++++++++++++++++++++++++++++++++
net/bluetooth/rfcomm/sock.c | 104 ++---------------------------------
3 files changed, 115 insertions(+), 98 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a902d..08b6c2a 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -126,6 +126,8 @@ int bt_sock_unregister(int proto);
void bt_sock_link(struct bt_sock_list *l, struct sock *s);
void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
+int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+ struct msghdr *msg, size_t len, int flags);
uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..73047f5 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
}
EXPORT_SYMBOL(bt_sock_recvmsg);
+static long bt_sock_data_wait(struct sock *sk, long timeo)
+{
+ DECLARE_WAITQUEUE(wait, current);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ for (;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ if (!skb_queue_empty(&sk->sk_receive_queue) ||
+ sk->sk_err ||
+ (sk->sk_shutdown & RCV_SHUTDOWN) ||
+ signal_pending(current) ||
+ !timeo)
+ break;
+
+ set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+ release_sock(sk);
+ timeo = schedule_timeout(timeo);
+ lock_sock(sk);
+ clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+ }
+
+ __set_current_state(TASK_RUNNING);
+ remove_wait_queue(sk_sleep(sk), &wait);
+ return timeo;
+}
+
+int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+ struct msghdr *msg, size_t size, int flags)
+{
+ struct sock *sk = sock->sk;
+ int err = 0;
+ size_t target, copied = 0;
+ long timeo;
+
+ if (flags & MSG_OOB)
+ return -EOPNOTSUPP;
+
+ msg->msg_namelen = 0;
+
+ BT_DBG("sk %p size %zu", sk, size);
+
+ lock_sock(sk);
+
+ target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
+ timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
+ do {
+ struct sk_buff *skb;
+ int chunk;
+
+ skb = skb_dequeue(&sk->sk_receive_queue);
+ if (!skb) {
+ if (copied >= target)
+ break;
+
+ if ((err = sock_error(sk)) != 0)
+ break;
+ if (sk->sk_shutdown & RCV_SHUTDOWN)
+ break;
+
+ err = -EAGAIN;
+ if (!timeo)
+ break;
+
+ timeo = bt_sock_data_wait(sk, timeo);
+
+ if (signal_pending(current)) {
+ err = sock_intr_errno(timeo);
+ goto out;
+ }
+ continue;
+ }
+
+ chunk = min_t(unsigned int, skb->len, size);
+ if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+ skb_queue_head(&sk->sk_receive_queue, skb);
+ if (!copied)
+ copied = -EFAULT;
+ break;
+ }
+ copied += chunk;
+ size -= chunk;
+
+ sock_recv_ts_and_drops(msg, sk, skb);
+
+ if (!(flags & MSG_PEEK)) {
+ skb_pull(skb, chunk);
+ if (skb->len) {
+ skb_queue_head(&sk->sk_receive_queue, skb);
+ break;
+ }
+ kfree_skb(skb);
+
+ } else {
+ /* put message back and return */
+ skb_queue_head(&sk->sk_receive_queue, skb);
+ break;
+ }
+ } while (size);
+
+out:
+ release_sock(sk);
+ return copied ? : err;
+}
+EXPORT_SYMBOL(bt_sock_stream_recvmsg);
+
static inline unsigned int bt_accept_poll(struct sock *parent)
{
struct list_head *p, *n;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 44a6232..5c92929 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -617,121 +617,29 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
return sent;
}
-static long rfcomm_sock_data_wait(struct sock *sk, long timeo)
-{
- DECLARE_WAITQUEUE(wait, current);
-
- add_wait_queue(sk_sleep(sk), &wait);
- for (;;) {
- set_current_state(TASK_INTERRUPTIBLE);
-
- if (!skb_queue_empty(&sk->sk_receive_queue) ||
- sk->sk_err ||
- (sk->sk_shutdown & RCV_SHUTDOWN) ||
- signal_pending(current) ||
- !timeo)
- break;
-
- set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
- release_sock(sk);
- timeo = schedule_timeout(timeo);
- lock_sock(sk);
- clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
- }
-
- __set_current_state(TASK_RUNNING);
- remove_wait_queue(sk_sleep(sk), &wait);
- return timeo;
-}
-
static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
struct msghdr *msg, size_t size, int flags)
{
struct sock *sk = sock->sk;
struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
- int err = 0;
- size_t target, copied = 0;
- long timeo;
+ int len_or_err;
if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
rfcomm_dlc_accept(d);
return 0;
}
- if (flags & MSG_OOB)
- return -EOPNOTSUPP;
-
- msg->msg_namelen = 0;
-
- BT_DBG("sk %p size %zu", sk, size);
+ len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, size, flags);
lock_sock(sk);
+ if (!(flags & MSG_PEEK) && len_or_err > 0)
+ atomic_sub(len_or_err, &sk->sk_rmem_alloc);
- target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
- timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-
- do {
- struct sk_buff *skb;
- int chunk;
-
- skb = skb_dequeue(&sk->sk_receive_queue);
- if (!skb) {
- if (copied >= target)
- break;
-
- if ((err = sock_error(sk)) != 0)
- break;
- if (sk->sk_shutdown & RCV_SHUTDOWN)
- break;
-
- err = -EAGAIN;
- if (!timeo)
- break;
-
- timeo = rfcomm_sock_data_wait(sk, timeo);
-
- if (signal_pending(current)) {
- err = sock_intr_errno(timeo);
- goto out;
- }
- continue;
- }
-
- chunk = min_t(unsigned int, skb->len, size);
- if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
- skb_queue_head(&sk->sk_receive_queue, skb);
- if (!copied)
- copied = -EFAULT;
- break;
- }
- copied += chunk;
- size -= chunk;
-
- sock_recv_ts_and_drops(msg, sk, skb);
-
- if (!(flags & MSG_PEEK)) {
- atomic_sub(chunk, &sk->sk_rmem_alloc);
-
- skb_pull(skb, chunk);
- if (skb->len) {
- skb_queue_head(&sk->sk_receive_queue, skb);
- break;
- }
- kfree_skb(skb);
-
- } else {
- /* put message back and return */
- skb_queue_head(&sk->sk_receive_queue, skb);
- break;
- }
- } while (size);
-
-out:
if (atomic_read(&sk->sk_rmem_alloc) <= (sk->sk_rcvbuf >> 2))
rfcomm_dlc_unthrottle(rfcomm_pi(sk)->dlc);
-
release_sock(sk);
- return copied ? : err;
+
+ return len_or_err;
}
static int rfcomm_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index aed72f2..44bc6ee 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
goto done;
}
+ /* If specified, PSM must be odd and lsb of upper byte must be 0 */
+ if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+ err = -EINVAL;
+ goto done;
+ }
+
if (la.l2_psm && __le16_to_cpu(la.l2_psm) < 0x1001 &&
!capable(CAP_NET_BIND_SERVICE)) {
err = -EACCES;
@@ -1190,6 +1196,12 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
goto done;
}
+ /* PSM must be odd and lsb of upper byte must be 0 */
+ if ((__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+ err = -EINVAL;
+ goto done;
+ }
+
/* Set destination address and psm */
bacpy(&bt_sk(sk)->dst, &la.l2_bdaddr);
l2cap_pi(sk)->psm = la.l2_psm;
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9780ab0..582975b 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
if (*result == L2CAP_CONF_SUCCESS) {
switch (rfc.mode) {
case L2CAP_MODE_ERTM:
- pi->remote_tx_win = rfc.txwin_size;
pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
pi->mps = le16_to_cpu(rfc.max_pdu_size);
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 44bc6ee..9780ab0 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2708,10 +2708,10 @@ done:
case L2CAP_MODE_ERTM:
pi->remote_tx_win = rfc.txwin_size;
pi->remote_max_tx = rfc.max_transmit;
- if (rfc.max_pdu_size > pi->conn->mtu - 10)
- rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+ if (pi->remote_mps > pi->conn->mtu - 10)
+ pi->remote_mps = pi->conn->mtu - 10;
rfc.retrans_timeout =
le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);
@@ -2726,10 +2726,9 @@ done:
break;
case L2CAP_MODE_STREAMING:
- if (rfc.max_pdu_size > pi->conn->mtu - 10)
- rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+ if (pi->remote_mps > pi->conn->mtu - 10)
+ pi->remote_mps = pi->conn->mtu - 10;
pi->conf_state |= L2CAP_CONF_MODE_DONE;
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/l2cap.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 636724b..16e412f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -33,7 +33,7 @@
#define L2CAP_DEFAULT_FLUSH_TO 0xffff
#define L2CAP_DEFAULT_TX_WINDOW 63
#define L2CAP_DEFAULT_MAX_TX 3
-#define L2CAP_DEFAULT_RETRANS_TO 1000 /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO 2000 /* 2 seconds */
#define L2CAP_DEFAULT_MONITOR_TO 12000 /* 12 seconds */
#define L2CAP_DEFAULT_MAX_PDU_SIZE 672
#define L2CAP_DEFAULT_ACK_TO 200
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9ba1e8e..aed72f2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
goto unlock;
if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
- if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
- l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+ if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+ l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+ (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+ l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
sk->sk_state = BT_CONNECTED;
@@ -3217,8 +3219,10 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
if (l2cap_pi(sk)->conf_state & L2CAP_CONF_OUTPUT_DONE) {
- if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
- l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+ if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+ l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+ (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+ l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
sk->sk_state = BT_CONNECTED;
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum