Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau <mathewm@codeaurora.org>]

Hi Marcel -

On Tue, 3 Aug 2010, Marcel Holtmann wrote:

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