[PATCH 1/3] Bluetooth: Change default MTU for L2CAP ATT channel

[PATCH 1/3] Bluetooth: Change default MTU for L2CAP ATT channel

[Andre Guedes]

This patch changes the default MTU value for L2CAP ATT fixed channel
to L2CAP_DEFAULT_MTU (672 octets).

Differently from others L2CAP channels, in L2CAP ATT fixed channel
there is no MTU negotiation. The MTU value for that channel is up to
the L2CAP implementation. The only restriction in L2CAP spec is the
MTU value must not be less than 23 octets.

At ATT protocol level (on top of L2CAP), we have the ATT_MTU which
defines the maximum size of any ATT message sent between client and
server. GATT profile defines ATT_MTU default value to 23 octets. If
a GATT based profile wants to use ATT_MTU greater than 23 octets
(e.g. HID over GATT profile), it should negotiate a new value by
executing the GATT Exchange MTU sub-procedure.

Thus, in order to support any value of ATT_MTU negotiated at ATT
protocol level, our L2CAP implementation should have L2CAP ATT
fixed channel MTU equal or greater than ATT_MAX_MTU (512 octets).

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/l2cap_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 953ac53..9f513b1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -449,7 +449,7 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 	case L2CAP_CHAN_CONN_ORIENTED:
 		if (conn->hcon->type == LE_LINK) {
 			/* LE connection */
-			chan->omtu = L2CAP_LE_DEFAULT_MTU;
+			chan->omtu = L2CAP_DEFAULT_MTU;
 			chan->scid = L2CAP_CID_LE_DATA;
 			chan->dcid = L2CAP_CID_LE_DATA;
 		} else {
-- 
1.7.10.2

[PATCH 2/3] Bluetooth: Check MTU value in l2cap_sock_setsockopt_old

[Andre Guedes]

If user tries to set an invalid MTU value, l2cap_sock_setsockopt_old
should return -EINVAL.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/l2cap_sock.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index d856cc8..a7ac747 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -445,6 +445,22 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 	return err;
 }
 
+static bool is_valid_mtu(struct l2cap_chan *chan, u16 mtu)
+{
+	switch (chan->scid) {
+	case L2CAP_CID_LE_DATA:
+		if (mtu < L2CAP_LE_DEFAULT_MTU)
+			return 0;
+		break;
+
+	default:
+		if (mtu < L2CAP_DEFAULT_MIN_MTU)
+			return 0;
+	}
+
+	return 1;
+}
+
 static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
@@ -483,6 +499,11 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 			break;
 		}
 
+		if (!is_valid_mtu(chan, opts.imtu)) {
+			err = -EINVAL;
+			break;
+		}
+
 		chan->mode = opts.mode;
 		switch (chan->mode) {
 		case L2CAP_MODE_BASIC:
-- 
1.7.10.2

Re: [PATCH 2/3] Bluetooth: Check MTU value in l2cap_sock_setsockopt_old

[Gustavo Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2012-05-30 15:08:48 -0300]:

> If user tries to set an invalid MTU value, l2cap_sock_setsockopt_old
> should return -EINVAL.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/l2cap_sock.c |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index d856cc8..a7ac747 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -445,6 +445,22 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>  	return err;
>  }
>  
> +static bool is_valid_mtu(struct l2cap_chan *chan, u16 mtu)

I prefer if we call this l2cap_valid_mtu()

> +{
> +	switch (chan->scid) {
> +	case L2CAP_CID_LE_DATA:
> +		if (mtu < L2CAP_LE_DEFAULT_MTU)
> +			return 0;

If you want to return bool you might want to return false here.

> +		break;
> +
> +	default:
> +		if (mtu < L2CAP_DEFAULT_MIN_MTU)
> +			return 0;
> +	}
> +
> +	return 1;

and true here.

	Gustavo

Re: [PATCH 2/3] Bluetooth: Check MTU value in l2cap_sock_setsockopt_old

[Andre Guedes]

Hi Gustavo,

On Thu, May 31, 2012 at 10:42 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi Andre,
>
> * Andre Guedes <andre.guedes@openbossa.org> [2012-05-30 15:08:48 -0300]:
>
>> If user tries to set an invalid MTU value, l2cap_sock_setsockopt_old
>> should return -EINVAL.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>>  net/bluetooth/l2cap_sock.c |   21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> index d856cc8..a7ac747 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -445,6 +445,22 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>>       return err;
>>  }
>>
>> +static bool is_valid_mtu(struct l2cap_chan *chan, u16 mtu)
>
> I prefer if we call this l2cap_valid_mtu()

Ok, I'll rename it.

>> +{
>> +     switch (chan->scid) {
>> +     case L2CAP_CID_LE_DATA:
>> +             if (mtu < L2CAP_LE_DEFAULT_MTU)
>> +                     return 0;
>
> If you want to return bool you might want to return false here.
>
>> +             break;
>> +
>> +     default:
>> +             if (mtu < L2CAP_DEFAULT_MIN_MTU)
>> +                     return 0;
>> +     }
>> +
>> +     return 1;
>
> and true here.

Sure. I'll fix this.

I'll send a new version of this patch series.

BR,

Andre

[PATCH 3/3] Bluetooth: Rename L2CAP_LE_DEFAULT_MTU

[Andre Guedes]

This patch renames L2CAP_LE_DEFAULT_MTU macro to L2CAP_LE_MIN_MTU
since it represents the minimum MTU value, not the default MTU
value for LE.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/l2cap.h |    2 +-
 net/bluetooth/l2cap_sock.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index a00b43e..ce99c56 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -40,11 +40,11 @@
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	1009    /* Sized for 3-DH5 packet */
 #define L2CAP_DEFAULT_ACK_TO		200
-#define L2CAP_LE_DEFAULT_MTU		23
 #define L2CAP_DEFAULT_MAX_SDU_SIZE	0xFFFF
 #define L2CAP_DEFAULT_SDU_ITIME		0xFFFFFFFF
 #define L2CAP_DEFAULT_ACC_LAT		0xFFFFFFFF
 #define L2CAP_BREDR_MAX_PAYLOAD		1019    /* 3-DH5 packet */
+#define L2CAP_LE_MIN_MTU		23
 
 #define L2CAP_DISC_TIMEOUT		msecs_to_jiffies(100)
 #define L2CAP_DISC_REJ_TIMEOUT		msecs_to_jiffies(5000)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a7ac747..3ee7995 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -449,7 +449,7 @@ static bool is_valid_mtu(struct l2cap_chan *chan, u16 mtu)
 {
 	switch (chan->scid) {
 	case L2CAP_CID_LE_DATA:
-		if (mtu < L2CAP_LE_DEFAULT_MTU)
+		if (mtu < L2CAP_LE_MIN_MTU)
 			return 0;
 		break;
 
-- 
1.7.10.2

Re: [PATCH 1/3] Bluetooth: Change default MTU for L2CAP ATT channel

[Gustavo Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2012-05-30 15:08:47 -0300]:

> This patch changes the default MTU value for L2CAP ATT fixed channel
> to L2CAP_DEFAULT_MTU (672 octets).
> 
> Differently from others L2CAP channels, in L2CAP ATT fixed channel
> there is no MTU negotiation. The MTU value for that channel is up to
> the L2CAP implementation. The only restriction in L2CAP spec is the
> MTU value must not be less than 23 octets.
> 
> At ATT protocol level (on top of L2CAP), we have the ATT_MTU which
> defines the maximum size of any ATT message sent between client and
> server. GATT profile defines ATT_MTU default value to 23 octets. If
> a GATT based profile wants to use ATT_MTU greater than 23 octets
> (e.g. HID over GATT profile), it should negotiate a new value by
> executing the GATT Exchange MTU sub-procedure.
> 
> Thus, in order to support any value of ATT_MTU negotiated at ATT
> protocol level, our L2CAP implementation should have L2CAP ATT
> fixed channel MTU equal or greater than ATT_MAX_MTU (512 octets).

Shouldn't we define a L2CAP_LE_MAX_MTU then and set its value to 512?
If we will never have anything greater than 512 octets why allow it?

	Gustavo

Re: [PATCH 1/3] Bluetooth: Change default MTU for L2CAP ATT channel

[Andre Guedes]

Hi Gustavo,

On Thu, May 31, 2012 at 10:33 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi Andre,
>
> * Andre Guedes <andre.guedes@openbossa.org> [2012-05-30 15:08:47 -0300]:
>
>> This patch changes the default MTU value for L2CAP ATT fixed channel
>> to L2CAP_DEFAULT_MTU (672 octets).
>>
>> Differently from others L2CAP channels, in L2CAP ATT fixed channel
>> there is no MTU negotiation. The MTU value for that channel is up to
>> the L2CAP implementation. The only restriction in L2CAP spec is the
>> MTU value must not be less than 23 octets.
>>
>> At ATT protocol level (on top of L2CAP), we have the ATT_MTU which
>> defines the maximum size of any ATT message sent between client and
>> server. GATT profile defines ATT_MTU default value to 23 octets. If
>> a GATT based profile wants to use ATT_MTU greater than 23 octets
>> (e.g. HID over GATT profile), it should negotiate a new value by
>> executing the GATT Exchange MTU sub-procedure.
>>
>> Thus, in order to support any value of ATT_MTU negotiated at ATT
>> protocol level, our L2CAP implementation should have L2CAP ATT
>> fixed channel MTU equal or greater than ATT_MAX_MTU (512 octets).
>
> Shouldn't we define a L2CAP_LE_MAX_MTU then and set its value to 512?
> If we will never have anything greater than 512 octets why allow it?

Yes, we could do it. But 512 octets ATT_MAX_MTU is a ATT definition,
not L2CAP. That's why I would prefer to use L2CAP_DEFAULT_MTU here.
Besides, this way, L2CAP ATT fixed channel MTU is initialized just
like other L2CAP channels.

BR,

Andre