[RFC 1/8] gattrib: Fix GAttrib buffer allocation

[RFC 1/8] gattrib: Fix GAttrib buffer allocation

[Andre Guedes]

GAttrib buffer should be allocated according to ATT_MTU value. Over
BR/EDR, ATT_MTU should be set to the L2CAP imtu negotiated during
L2CAP configuration phase. Over LE, ATT_MTU should be 23 octets.
---
 attrib/gattrib.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 29c3585..137e0ab 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -433,15 +433,34 @@ done:
 GAttrib *g_attrib_new(GIOChannel *io)
 {
 	struct _GAttrib *attrib;
-	uint16_t omtu;
+	uint16_t imtu;
+	uint16_t att_mtu;
+	uint16_t cid;
+	GError *gerr = NULL;
 
 	g_io_channel_set_encoding(io, NULL, NULL);
 	g_io_channel_set_buffered(io, FALSE);
 
+	bt_io_get(io, BT_IO_L2CAP, &gerr,
+			BT_IO_OPT_IMTU, &imtu,
+			BT_IO_OPT_CID, &cid,
+			BT_IO_OPT_INVALID);
+
+	if (gerr) {
+		error("%s", gerr->message);
+		g_error_free(gerr);
+		return NULL;
+	}
+
 	attrib = g_try_new0(struct _GAttrib, 1);
 	if (attrib == NULL)
 		return NULL;
 
+	att_mtu = (cid == ATT_CID) ? ATT_DEFAULT_LE_MTU : imtu;
+
+	attrib->buf = g_malloc0(att_mtu);
+	attrib->buflen = att_mtu;
+
 	attrib->io = g_io_channel_ref(io);
 	attrib->requests = g_queue_new();
 	attrib->responses = g_queue_new();
@@ -450,17 +469,6 @@ GAttrib *g_attrib_new(GIOChannel *io)
 			G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
 			received_data, attrib);
 
-	if (bt_io_get(attrib->io, BT_IO_L2CAP, NULL,
-			BT_IO_OPT_OMTU, &omtu,
-			BT_IO_OPT_INVALID)) {
-		if (omtu == 0 || omtu > ATT_MAX_MTU)
-			omtu = ATT_MAX_MTU;
-	} else
-		omtu = ATT_DEFAULT_LE_MTU;
-
-	attrib->buf = g_malloc0(omtu);
-	attrib->buflen = omtu;
-
 	return g_attrib_ref(attrib);
 }
 
-- 
1.7.10.3

[RFC 2/8] gattrib: Fix g_attrib_set_mtu

[Andre Guedes]

23 octets is the default (and minimum) ATT_MTU value. If someone tries
to set ATT_MTU less than 23 octets g_attrib_set_mtu should fail (return
FALSE). Additionally, there is no constraint regarding the maximum value
of ATT_MTU, so we should not check for it.

Also, we should not change the L2CAP ATT fixed channel MTU. bt_io_set
call will always fail since we are not supposed to change L2CAP MTU
after connection is established.
---
 attrib/gattrib.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index 137e0ab..6729f36 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -627,14 +627,6 @@ uint8_t *g_attrib_get_buffer(GAttrib *attrib, int *len)
 gboolean g_attrib_set_mtu(GAttrib *attrib, int mtu)
 {
 	if (mtu < ATT_DEFAULT_LE_MTU)
-		mtu = ATT_DEFAULT_LE_MTU;
-
-	if (mtu > ATT_MAX_MTU)
-		mtu = ATT_MAX_MTU;
-
-	if (!bt_io_set(attrib->io, BT_IO_L2CAP, NULL,
-			BT_IO_OPT_OMTU, mtu,
-			BT_IO_OPT_INVALID))
 		return FALSE;
 
 	attrib->buf = g_realloc(attrib->buf, mtu);
-- 
1.7.10.3

Re: [RFC 2/8] gattrib: Fix g_attrib_set_mtu

[Anderson Lizardo]

Hi Andre,

On Wed, Jun 6, 2012 at 8:40 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> 23 octets is the default (and minimum) ATT_MTU value. If someone tries
> to set ATT_MTU less than 23 octets g_attrib_set_mtu should fail (return
> FALSE). Additionally, there is no constraint regarding the maximum value
> of ATT_MTU, so we should not check for it.

Even though the spec does not mention a maximum ATT_MTU, we now need
to review and fix the ATT operations that mention limits on their PDU
(now that we removed the "artificial" 256 octect limit). Some I found:

- 3.2.9 Long Attribute Values (page 1839):

The maximum length of an attribute value shall be 512 octets.

- 3.4.4.2 Read By Type Response (page 1853):

The maximum length of an attribute handle-value pair is 255 octets, bounded
by the Length parameter that is one octet.

- 3.4.4.10 Read by Group Type Response (page 1861):

The maximum length of an Attribute Data is 255 octets, bounded by the Length
parameter that is one octet.

Also, what will happen to the ATT_MTU_MAX definition? Is it still
being used in other places?

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [RFC 2/8] gattrib: Fix g_attrib_set_mtu

[Andre Guedes]

Hi Lizardo,

On Wed, Jun 6, 2012 at 10:26 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Andre,
>
> On Wed, Jun 6, 2012 at 8:40 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
>> 23 octets is the default (and minimum) ATT_MTU value. If someone tries
>> to set ATT_MTU less than 23 octets g_attrib_set_mtu should fail (return
>> FALSE). Additionally, there is no constraint regarding the maximum value
>> of ATT_MTU, so we should not check for it.
>
> Even though the spec does not mention a maximum ATT_MTU, we now need
> to review and fix the ATT operations that mention limits on their PDU
> (now that we removed the "artificial" 256 octect limit). Some I found:

Yes, we're working on that.

> Also, what will happen to the ATT_MTU_MAX definition? Is it still
> being used in other places?

Yes, we use ATT_MTU_MAX to allocate opdu buffers, but we should
replace ATT_MTU_MAX by the current ATT_MTU.

BR,

Andre

[RFC 3/8] attrib-server: Fix gatt_channel MTU value

[Andre Guedes]

In attrib_channel_attach, channel->mtu should be initialized according
to ATT_MTU value.

Over BR/EDR, ATT_MTU should be set to the L2CAP imtu negotiated during
L2CAP configuration phase. Over LE, ATT_MTU should be 23 octets.
---
 src/attrib-server.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index d2a2520..4dc3ff9 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -1033,6 +1033,7 @@ guint attrib_channel_attach(GAttrib *attrib)
 	GError *gerr = NULL;
 	char addr[18];
 	uint16_t cid;
+	guint mtu = 0;
 
 	io = g_attrib_get_channel(attrib);
 
@@ -1042,7 +1043,7 @@ guint attrib_channel_attach(GAttrib *attrib)
 			BT_IO_OPT_SOURCE_BDADDR, &channel->src,
 			BT_IO_OPT_DEST_BDADDR, &channel->dst,
 			BT_IO_OPT_CID, &cid,
-			BT_IO_OPT_OMTU, &channel->mtu,
+			BT_IO_OPT_IMTU, &mtu,
 			BT_IO_OPT_INVALID);
 	if (gerr) {
 		error("bt_io_get: %s", gerr->message);
@@ -1069,14 +1070,13 @@ guint attrib_channel_attach(GAttrib *attrib)
 	if (device == NULL || device_is_bonded(device) == FALSE)
 		delete_device_ccc(&channel->src, &channel->dst);
 
-	if (channel->mtu > ATT_MAX_MTU)
-		channel->mtu = ATT_MAX_MTU;
-
-	if (cid != ATT_CID)
+	if (cid != ATT_CID) {
 		channel->le = FALSE;
-	else
+		channel->mtu = mtu;
+	} else {
 		channel->le = TRUE;
-
+		channel->mtu = ATT_DEFAULT_LE_MTU;
+	}
 
 	channel->attrib = g_attrib_ref(attrib);
 	channel->id = g_attrib_register(channel->attrib, GATTRIB_ALL_REQS,
-- 
1.7.10.3

[RFC 4/8] attrib-server: Fix mtu_exchange

[Andre Guedes]

If the client requests an ATT_MTU less than the minimum ATT_MTU,
the server should send an Error Response message with Request Not
Supported code.

According to GATT spec, the server shall respond to Exchange MTU
Requests messages with an Exchange MTU Response with the Server
Rx MTU parameter set to the maximum MTU that this server can
receive. Thus, we should get L2CAP imtu value in order to properly
send the Exchange MTU Response message.

Additionally, we should not change the L2CAP ATT fixed channel MTU.
bt_io_set call will always fail since we are not supposed to change
L2CAP MTU after connection is established.
---
 src/attrib-server.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 4dc3ff9..3954f99 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -867,18 +867,27 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
 static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 		uint8_t *pdu, int len)
 {
-	guint old_mtu = channel->mtu;
+	GError *gerr = NULL;
+	GIOChannel *io;
+	uint16_t imtu;
 
 	if (mtu < ATT_DEFAULT_LE_MTU)
-		channel->mtu = ATT_DEFAULT_LE_MTU;
-	else
-		channel->mtu = MIN(mtu, channel->mtu);
+		return enc_error_resp(ATT_OP_MTU_REQ, 0,
+					ATT_ECODE_REQ_NOT_SUPP, pdu, len);
 
-	bt_io_set(channel->server->le_io, BT_IO_L2CAP, NULL,
-			BT_IO_OPT_OMTU, channel->mtu,
+	io = g_attrib_get_channel(channel->attrib);
+
+	bt_io_get(io, BT_IO_L2CAP, &gerr,
+			BT_IO_OPT_IMTU, &imtu,
 			BT_IO_OPT_INVALID);
 
-	return enc_mtu_resp(old_mtu, pdu, len);
+	if (gerr)
+		return enc_error_resp(ATT_OP_MTU_REQ, 0,
+					ATT_ECODE_UNLIKELY, pdu, len);
+
+	channel->mtu = MIN(mtu, imtu);
+
+	return enc_mtu_resp(imtu, pdu, len);
 }
 
 static void channel_remove(struct gatt_channel *channel)
-- 
1.7.10.3

[RFC 5/8] attrib-server: Update GAttrib buffer after Exchange MTU

[Andre Guedes]

We should update the GAttrib buffer length after exchanging ATT_MTU.
---
 src/attrib-server.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 3954f99..5adbf92 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -886,6 +886,7 @@ static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
 					ATT_ECODE_UNLIKELY, pdu, len);
 
 	channel->mtu = MIN(mtu, imtu);
+	g_attrib_set_mtu(channel->attrib, channel->mtu);
 
 	return enc_mtu_resp(imtu, pdu, len);
 }
-- 
1.7.10.3

[RFC 6/8] Remove omtu parameter from LE bt_io_connect calls

[Andre Guedes]

There is no need to set the omtu of L2CAP ATT fixed channel. We
use the default value.
---
 attrib/utils.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/attrib/utils.c b/attrib/utils.c
index c951f44..92bba09 100644
--- a/attrib/utils.c
+++ b/attrib/utils.c
@@ -94,7 +94,6 @@ GIOChannel *gatt_connect(const gchar *src, const gchar *dst,
 				BT_IO_OPT_DEST_BDADDR, &dba,
 				BT_IO_OPT_DEST_TYPE, dest_type,
 				BT_IO_OPT_CID, ATT_CID,
-				BT_IO_OPT_OMTU, mtu,
 				BT_IO_OPT_SEC_LEVEL, sec,
 				BT_IO_OPT_INVALID);
 	else
-- 
1.7.10.3

[RFC 7/8] Fix gatt_connect for BR/EDR

[Andre Guedes]

Use BT_IO_OPT_IMTU instead of BT_IO_OPT_OMTU in bt_io_connect.
We cannot control omtu value since it is negotiated during L2CAP
configuration phase.
---
 attrib/utils.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attrib/utils.c b/attrib/utils.c
index 92bba09..f809b1f 100644
--- a/attrib/utils.c
+++ b/attrib/utils.c
@@ -101,7 +101,7 @@ GIOChannel *gatt_connect(const gchar *src, const gchar *dst,
 				BT_IO_OPT_SOURCE_BDADDR, &sba,
 				BT_IO_OPT_DEST_BDADDR, &dba,
 				BT_IO_OPT_PSM, psm,
-				BT_IO_OPT_OMTU, mtu,
+				BT_IO_OPT_IMTU, mtu,
 				BT_IO_OPT_SEC_LEVEL, sec,
 				BT_IO_OPT_INVALID);
 
-- 
1.7.10.3

[RFC 8/8] Remove workaround in gatt_connect

[Andre Guedes]

This workaround is not necessary anymore since setsockopt is now
checking for minimum MTU.
---
 attrib/utils.c |   13 -------------
 1 file changed, 13 deletions(-)

diff --git a/attrib/utils.c b/attrib/utils.c
index f809b1f..d856fe2 100644
--- a/attrib/utils.c
+++ b/attrib/utils.c
@@ -36,10 +36,6 @@
 #include "btio.h"
 #include "gatttool.h"
 
-/* Minimum MTU for ATT connections */
-#define ATT_MIN_MTU_LE		23
-#define ATT_MIN_MTU_L2CAP	48
-
 GIOChannel *gatt_connect(const gchar *src, const gchar *dst,
 				const gchar *dst_type, const gchar *sec_level,
 				int psm, int mtu, BtIOConnect connect_cb)
@@ -49,15 +45,6 @@ GIOChannel *gatt_connect(const gchar *src, const gchar *dst,
 	uint8_t dest_type;
 	GError *err = NULL;
 	BtIOSecLevel sec;
-	int minimum_mtu;
-
-	/* This check is required because currently setsockopt() returns no
-	 * errors for MTU values smaller than the allowed minimum. */
-	minimum_mtu = psm ? ATT_MIN_MTU_L2CAP : ATT_MIN_MTU_LE;
-	if (mtu != 0 && mtu < minimum_mtu) {
-		g_printerr("MTU cannot be smaller than %d\n", minimum_mtu);
-		return NULL;
-	}
 
 	/* Remote device */
 	if (dst == NULL) {
-- 
1.7.10.3

Re: [RFC 1/8] gattrib: Fix GAttrib buffer allocation

[Johan Hedberg]

Hi Andre,

On Wed, Jun 06, 2012, Andre Guedes wrote:
> GAttrib buffer should be allocated according to ATT_MTU value. Over
> BR/EDR, ATT_MTU should be set to the L2CAP imtu negotiated during
> L2CAP configuration phase. Over LE, ATT_MTU should be 23 octets.
> ---
>  attrib/gattrib.c |   32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)

I've applied all patches as they look ok and the behavior can't get
much more broken than it was previously. I'll also continue testing them
at the UPF and do/propose additional fixes if necessary.

Johan

Re: [RFC 1/8] gattrib: Fix GAttrib buffer allocation

[Anderson Lizardo]

Hi Johan,

On Thu, Jun 7, 2012 at 3:03 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> I've applied all patches as they look ok and the behavior can't get
> much more broken than it was previously. I'll also continue testing them
> at the UPF and do/propose additional fixes if necessary.

FWIW I reviewed the patches as well and didn't find any issues (only
minor comments/recommendations on a single patch). We should at some
point run the MTU tests on PTS to make sure they are passing (although
the IOP tests on UPF are usually better).

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil