* Re: [PATCH] core: Add btd_adapter_recreate_bonding()
From: Alfonso Acosta @ 2014-10-10 23:56 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1412985213-5447-1-git-send-email-fons@spotify.com>
Please note that this patch depends on a previously submitted kernel
patch [1] to ensure that connection parameters are not lost when
rebonding.
[1] [PATCH] Bluetooth: Defer connection-parameter removal when
unpairing without disconnecting
On Sat, Oct 11, 2014 at 1:53 AM, Alfonso Acosta <fons@spotify.com> wrote:
> This patch adds btd_adapter_recreate_bonding() which rebonds a device,
> i.e. it performs an unbonding operation inmediately followed by a
> bonding operation.
>
> Although there is no internal use for btd_adapter_recreate_bonding()
> yet, it is useful for plugins dealing with devices which require
> renegotiaing their keys. For instance, when dealing with devices
> without persistent storage which lose their keys on reset.
>
> Certain caveats have been taken into account when rebonding with a LE
> device:
>
> * If the device to rebond is already connected, the rebonding is done
> without disconnecting to avoid the extra latency of reestablishing
> the connection.
>
> * If no connection exists, we connect before unbonding anyways to
> avoid losing the device's autoconnection and connection parameters,
> which are removed by the kernel when unpairing if no connection
> exists.
>
> * Not closing the connection requires defering attribute discovery
> until the rebonding is done. Otherwise, the security level could be
> elavated with the old LTK, which may be invalid since we are
> rebonding. When rebonding, attribute discovery is suspended and the
> ATT socket is saved to allow resuming it once bonding is finished.
> ---
> src/adapter.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/adapter.h | 7 ++++++-
> src/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/device.h | 4 ++++
> 4 files changed, 119 insertions(+), 7 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 92ee1a0..81e8f22 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5195,14 +5195,15 @@ int btd_adapter_read_clock(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
> }
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type)
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect)
> {
> struct mgmt_cp_unpair_device cp;
>
> memset(&cp, 0, sizeof(cp));
> bacpy(&cp.addr.bdaddr, bdaddr);
> cp.addr.type = bdaddr_type;
> - cp.disconnect = 1;
> + cp.disconnect = disconnect;
>
> if (mgmt_send(adapter->mgmt, MGMT_OP_UNPAIR_DEVICE,
> adapter->dev_id, sizeof(cp), &cp,
> @@ -5212,6 +5213,53 @@ int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> return -EIO;
> }
>
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap)
> +{
> + struct btd_device *device;
> + int err;
> +
> + device = btd_adapter_get_device(adapter, bdaddr, bdaddr_type);
> +
> + if (!device || !device_is_bonded(device, bdaddr_type))
> + return -EINVAL;
> +
> + device_set_rebonding(device, bdaddr_type, true);
> +
> + /* Make sure the device is connected before unbonding to avoid
> + * losing the device's autoconnection and connection
> + * parameters, which are removed by the kernel when unpairing
> + * if no connection exists. We would had anyways implicitly
> + * connected when bonding later on, so we might as well just
> + * do it explicitly now.
> + */
> + if (bdaddr_type != BDADDR_BREDR && !btd_device_is_connected(device)) {
> + err = device_connect_le(device);
> + if (err < 0)
> + goto failed;
> + }
> +
> + /* Unbond without disconnecting to avoid the connection
> + * re-establishment latency
> + */
> + err = btd_adapter_remove_bonding(adapter, bdaddr, bdaddr_type, 0);
> + if (err < 0)
> + goto failed;
> +
> + err = adapter_create_bonding(adapter, bdaddr, bdaddr_type, io_cap);
> + if (err < 0)
> + goto failed;
> +
> + return 0;
> +
> +failed:
> + error("failed: %s", strerror(-err));
> + device_set_rebonding(device, bdaddr_type, false);
> + return err;
> +}
> +
> static void pincode_reply_complete(uint8_t status, uint16_t length,
> const void *param, void *user_data)
> {
> @@ -5594,8 +5642,11 @@ static void bonding_complete(struct btd_adapter *adapter,
> else
> device = btd_adapter_find_device(adapter, bdaddr, addr_type);
>
> - if (device != NULL)
> + if (device != NULL) {
> device_bonding_complete(device, addr_type, status);
> + if (device_is_rebonding(device, addr_type))
> + device_rebonding_complete(device, addr_type);
> + }
>
> resume_discovery(adapter);
>
> diff --git a/src/adapter.h b/src/adapter.h
> index 6801fee..bd00d3e 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -158,7 +158,12 @@ int btd_adapter_disconnect_device(struct btd_adapter *adapter,
> uint8_t bdaddr_type);
>
> int btd_adapter_remove_bonding(struct btd_adapter *adapter,
> - const bdaddr_t *bdaddr, uint8_t bdaddr_type);
> + const bdaddr_t *bdaddr, uint8_t bdaddr_type,
> + uint8_t disconnect);
> +int btd_adapter_recreate_bonding(struct btd_adapter *adapter,
> + const bdaddr_t *bdaddr,
> + uint8_t bdaddr_type,
> + uint8_t io_cap);
>
> int btd_adapter_pincode_reply(struct btd_adapter *adapter,
> const bdaddr_t *bdaddr,
> diff --git a/src/device.c b/src/device.c
> index 875a5c5..4aab349 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -158,6 +158,7 @@ struct att_callbacks {
> struct bearer_state {
> bool paired;
> bool bonded;
> + bool rebonding;
> bool connected;
> bool svc_resolved;
> };
> @@ -221,6 +222,8 @@ struct btd_device {
> int8_t rssi;
>
> GIOChannel *att_io;
> + GIOChannel *att_rebond_io;
> +
> guint cleanup_id;
> guint store_id;
> };
> @@ -522,6 +525,9 @@ static void device_free(gpointer user_data)
>
> attio_cleanup(device);
>
> + if (device->att_rebond_io)
> + g_io_channel_unref(device->att_rebond_io);
> +
> if (device->tmp_records)
> sdp_list_free(device->tmp_records,
> (sdp_free_func_t) sdp_record_free);
> @@ -1739,6 +1745,23 @@ long device_bonding_last_duration(struct btd_device *device)
> return bonding->last_attempt_duration_ms;
> }
>
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + return state->rebonding;
> +}
> +
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding)
> +{
> + struct bearer_state *state = get_state(device, bdaddr_type);
> +
> + state->rebonding = rebonding;
> +
> + DBG("rebonding = %d", rebonding);
> +}
> +
> static void create_bond_req_exit(DBusConnection *conn, void *user_data)
> {
> struct btd_device *device = user_data;
> @@ -2029,7 +2052,7 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
>
> if (state->paired && !state->bonded)
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - bdaddr_type);
> + bdaddr_type, 1);
>
> if (device->bredr_state.connected || device->le_state.connected)
> return;
> @@ -2639,13 +2662,13 @@ static void device_remove_stored(struct btd_device *device)
> if (device->bredr_state.bonded) {
> device->bredr_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - BDADDR_BREDR);
> + BDADDR_BREDR, 1);
> }
>
> if (device->le_state.bonded) {
> device->le_state.bonded = false;
> btd_adapter_remove_bonding(device->adapter, &device->bdaddr,
> - device->bdaddr_type);
> + device->bdaddr_type, 1);
> }
>
> device->bredr_state.paired = false;
> @@ -3628,6 +3651,18 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
> GAttrib *attrib;
> BtIOSecLevel sec_level;
>
> + DBG("");
> +
> + if (dev->le_state.rebonding) {
> + DBG("postponing due to rebonding");
> + /* Keep the att socket, and defer attaching the attributes
> + * until rebonding is done.
> + */
> + if (!dev->att_rebond_io)
> + dev->att_rebond_io = g_io_channel_ref(io);
> + return false;
> + }
> +
> bt_io_get(io, &gerr, BT_IO_OPT_SEC_LEVEL, &sec_level,
> BT_IO_OPT_INVALID);
> if (gerr) {
> @@ -4269,6 +4304,23 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> }
> }
>
> +bool device_rebonding_complete(struct btd_device *device, uint8_t bdaddr_type)
> +{
> + bool ret = true;
> +
> + DBG("");
> +
> + device_set_rebonding(device, bdaddr_type, false);
> +
> + if (bdaddr_type != BDADDR_BREDR && device->att_rebond_io) {
> + ret = device_attach_attrib(device, device->att_rebond_io);
> + g_io_channel_unref(device->att_rebond_io);
> + device->att_rebond_io = NULL;
> + }
> +
> + return ret;
> +}
> +
> static gboolean svc_idle_cb(gpointer user_data)
> {
> struct svc_callback *cb = user_data;
> diff --git a/src/device.h b/src/device.h
> index 2e0473e..65b1018 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -94,6 +94,10 @@ bool device_is_retrying(struct btd_device *device);
> void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> uint8_t status);
> gboolean device_is_bonding(struct btd_device *device, const char *sender);
> +bool device_is_rebonding(struct btd_device *device, uint8_t bdaddr_type);
> +void device_set_rebonding(struct btd_device *device, uint8_t bdaddr_type,
> + bool rebonding);
> +bool device_complete_rebonding(struct btd_device *device, uint8_t bdaddr_type);
> void device_bonding_attempt_failed(struct btd_device *device, uint8_t status);
> void device_bonding_failed(struct btd_device *device, uint8_t status);
> struct btd_adapter_pin_cb_iter *device_bonding_iter(struct btd_device *device);
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Alfonso Acosta @ 2014-10-11 0:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <872F1D49-E810-4D33-9B05-ED3DDBA64336@holtmann.org>
Hi Marcel,
> I am not in favor of making Pair Device just do re-pairing. I think that =
is a dangerous behavior since want bondings in the kernel only in two ways.=
Either they are loaded via Load Long Term Keys or via Pair Device. Doing s=
omething magic is something that I consider dangerous and can lead into som=
e flaws. If we come with a clear error than we at least know that something=
went wrong.
>
> Repairing command is possible, but I am not 100% convinced that it is a g=
ood idea. The normal workflow is that you pair and unpair a device. If you =
get stuck in a weird state, you unpair first which ensures that everything =
is cleaned out and then pair again. For the connection parameters we can ju=
st be smarter.
Fair enough. Does this mean that the current patch is still of
interest? I hope so :)
> I bet there is still a lot of improvement for our connection parameter ha=
ndling. For example we could just keep all connection parameters around in =
cache and just expire them after 1 day or so. That would improve general ha=
ndling with devices with do not pair in the first. So I would focus on bein=
g smart when dropping connection parameters and not trying to bind it too m=
uch to mgmt API visible states.
I actually have a patch lying around which keeps track of
conn_parameters within bluetoothd. I think that it would be a good
start (I would like to complete the submissions I sent this week
getting to it though).
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 1:33 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..eb9988f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
conn->state = BT_CLOSED;
break;
}
+
+ if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
}
/* Enter sniff mode */
@@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..0af579d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
struct mgmt_rp_unpair_device rp;
struct hci_cp_disconnect dc;
struct pending_cmd *cmd;
- struct hci_conn *conn;
+ struct hci_conn *uninitialized_var(conn);
int err;
memset(&rp, 0, sizeof(rp));
@@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+
+ /* If the BLE connection is being used, defer clearing up
+ * the connection parameters until closing to give a
+ * chance of keeping them if a repairing happens.
+ */
+ if (conn && !cp->disconnect)
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+ else
+ hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->disconnect) {
+ /* Only lookup the connection in the BR/EDR since the
+ * LE connection was already looked up earlier.
+ */
if (cp->addr.type == BDADDR_BREDR)
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
} else {
conn = NULL;
}
@@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 1:37 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1412991237-20847-1-git-send-email-fons@spotify.com>
I shortened the commit message header and moved the
clear_bit(HCI_CONN_PARAM_REMOVAL_PEND) to cover all the pairing
completion cases.
On Sat, Oct 11, 2014 at 3:33 AM, Alfonso Acosta <fons@spotify.com> wrote:
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..eb9988f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
> conn->state = BT_CLOSED;
> break;
> }
> +
> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> }
>
> /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0af579d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> struct mgmt_rp_unpair_device rp;
> struct hci_cp_disconnect dc;
> struct pending_cmd *cmd;
> - struct hci_conn *conn;
> + struct hci_conn *uninitialized_var(conn);
> int err;
>
> memset(&rp, 0, sizeof(rp));
> @@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> +
> + /* If the BLE connection is being used, defer clearing up
> + * the connection parameters until closing to give a
> + * chance of keeping them if a repairing happens.
> + */
> + if (conn && !cp->disconnect)
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> + else
> + hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
>
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->disconnect) {
> + /* Only lookup the connection in the BR/EDR since the
> + * LE connection was already looked up earlier.
> + */
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> } else {
> conn = NULL;
> }
> @@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
> --
> 1.9.1
>
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-11 6:55 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend, werner
In-Reply-To: <20141010194116.GA31843@omega>
Hi Martin,
On Fri, Oct 10, 2014 at 09:41:16PM +0200, Alexander Aring wrote:
...
> > hdr.payload_len = htons(skb->len);
> > diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> > index c2e0d14..6643a7c 100644
> > --- a/net/bluetooth/6lowpan.c
> > +++ b/net/bluetooth/6lowpan.c
> > @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> > kfree_skb(local_skb);
> > kfree_skb(skb);
> > } else {
> > + /* Decompression may use pskb_expand_head so no shared skb's */
> > + skb = skb_share_check(skb, GFP_ATOMIC);
> > + if (!skb) {
> > + dev->stats.rx_dropped++;
> > + return NET_RX_DROP;
> > + }
> > +
> > switch (skb->data[0] & 0xe0) {
> > case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> > local_skb = skb_clone(skb, GFP_ATOMIC);
>
>
> Now we come to this function.
>
> There exist two global rules here:
>
> 1.
>
> If we change only the skb pointers head/tail with e.g. skb_push then we
> need a cloned skb only. We don't change the skb data buffer here.
>
> We need this in LOWPAN_DISPATCH_IPV6, here we run only one skb_pull for
> one byte, and the FRAGN and FRAG1 dispatches in 802.15.4 6LoWPAN.
>
> skb_share_check do this, we have surely a clone afterwards and the
> sk_buff attributes are not changed elsewhere, which means we are
> allowed to manipulate the skb attributes.
>
> 2.
>
> If we touch the data buffer, then we need a skb_copy. We need this at
> LOWPAN_DISPATCH_IPHC because here we replacing the data buffer.
>
> skb_unshare do this for us.
>
This is not correct. I mean the chain of thought is correct. But a
private copy of data buffer is already done by pskb_expand_head, which
is used before modify the data buffer. If we should not run it, then it
would be correct. This handling is done by decompression IPHC.
So we don't need skb_unshare. Now it's very interesting if we can use
skb_cow_head, when replacing transport/network header, because we only
need to modify the headroom. Otherwise we should use skb_cow.
- Alex
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-11 7:36 UTC (permalink / raw)
To: Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen,
Martin Townsend
In-Reply-To: <1412840794-17283-2-git-send-email-martin.townsend@xsilon.com>
Hi Martin,
This is only a summarize what we should now to try to do here for next
version.
I know... this patch ends in a global debate on principles how we deal
with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
more a hack.
On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> pskb_expand_head. It also checks to see if there is enough headroom
> first to ensure it's only done if necessary.
> As pskb_expand_head must only have one reference the calling code
> now ensures this.
>
> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> ---
> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> net/bluetooth/6lowpan.c | 7 +++++++
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 142eef5..853b4b8 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> struct net_device *dev, skb_delivery_cb deliver_skb)
> {
> - struct sk_buff *new;
> int stat;
>
> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
> - GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb_push(new, sizeof(struct ipv6hdr));
> - skb_reset_network_header(new);
> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> + skb_push(skb, sizeof(struct ipv6hdr));
> + skb_reset_network_header(skb);
> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>
> - new->protocol = htons(ETH_P_IPV6);
> - new->pkt_type = PACKET_HOST;
> - new->dev = dev;
> + skb->protocol = htons(ETH_P_IPV6);
> + skb->pkt_type = PACKET_HOST;
> + skb->dev = dev;
>
> raw_dump_table(__func__, "raw skb data dump before receiving",
> - new->data, new->len);
> + skb->data, skb->len);
>
> - stat = deliver_skb(new, dev);
> + stat = deliver_skb(skb, dev);
>
> - kfree_skb(new);
> + consume_skb(skb);
>
> return stat;
> }
> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* UDP data uncompression */
> if (iphc0 & LOWPAN_IPHC_NH_C) {
> struct udphdr uh;
> - struct sk_buff *new;
> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>
> if (uncompress_udp_header(skb, &uh))
> goto drop;
> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> /* replace the compressed UDP head by the uncompressed UDP
> * header
> */
> - new = skb_copy_expand(skb, sizeof(struct udphdr),
> - skb_tailroom(skb), GFP_ATOMIC);
> - kfree_skb(skb);
> -
> - if (!new)
> - return -ENOMEM;
> -
> - skb = new;
> + if (skb_headroom(skb) < needed) {
> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
use skb_cow_head here.
>
> skb_push(skb, sizeof(struct udphdr));
> skb_reset_transport_header(skb);
> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> (u8 *)&uh, sizeof(uh));
>
> hdr.nexthdr = UIP_PROTO_UDP;
> + } else {
> + if (skb_headroom(skb) < sizeof(hdr)) {
> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> + if (unlikely(err)) {
> + kfree_skb(skb);
> + return err;
> + }
> + }
same here.
> }
>
> hdr.payload_len = htons(skb->len);
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index c2e0d14..6643a7c 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> kfree_skb(local_skb);
> kfree_skb(skb);
> } else {
> + /* Decompression may use pskb_expand_head so no shared skb's */
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev->stats.rx_dropped++;
> + return NET_RX_DROP;
> + }
> +
I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
is the share_check really needed? In my opinion there are several things
wrong.
What bluetooth should do here is:
Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
- only run a skb_pull (doesn't change the data buffer, only skb attribute)
for the one byte dispatch.
- run other things like skb_reset_network_header, etc...
-> Currently this works because we run a complete skb_copy_expand here.
But this is not needed, we need a clone because we don't modify the
data buffer.
-> To Jukka: I don't see a skb_pull for the one byte dispatch value?
What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
for this here.
In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
this is already handled by "skb_share_check" then.
- Alex
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 8:32 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1412991237-20847-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..eb9988f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
> conn->state = BT_CLOSED;
> break;
> }
> +
> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
do we really need this one? The hci_conn_timeout should trigger when the connection establishment has a timeout and that is really non of the concerns here. And in case we hit an idle disconnect timeout, we are still ending up in hci_conn_del eventually, right? If not, I am having the slight feeling that you might have uncovered a bug that we should fix.
> }
>
> /* Enter sniff mode */
> @@ -544,6 +547,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..0af579d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2699,7 +2699,7 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> struct mgmt_rp_unpair_device rp;
> struct hci_cp_disconnect dc;
> struct pending_cmd *cmd;
> - struct hci_conn *conn;
> + struct hci_conn *uninitialized_var(conn);
> int err;
>
> memset(&rp, 0, sizeof(rp));
> @@ -2736,7 +2736,17 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> +
> + /* If the BLE connection is being used, defer clearing up
> + * the connection parameters until closing to give a
> + * chance of keeping them if a repairing happens.
> + */
> + if (conn && !cp->disconnect)
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> + else
> + hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
Coming to think about this a bit in detail, why are we making a difference on if we disconnect here or not. We could make the code a lot simpler. Just always set the PARAM_REMOVAL flag when unpairing and clear it when pairing. If we disconnect right away or not should not affect the usage of this flag.
As long as the connection is up, we can happily keep the flags around. Only when the connection goes away, we really need to ensure that we know if we want to remove them or not. Honestly, we should have done it that way in the first place and not spread hci_conn_params_del in different places. The central logic to delete or keep connection parameters should be run when hci_conn goes away.
>
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->disconnect) {
> + /* Only lookup the connection in the BR/EDR since the
> + * LE connection was already looked up earlier.
> + */
> if (cp->addr.type == BDADDR_BREDR)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> } else {
> conn = NULL;
> }
I know that Johan told you to do it this way, but seeing the patch now makes me a bit uneasy. The usage of uninitilized_var in this case can lead to errors in the future. The logic that the compiler is wrong here is not obvious. Which means a week from now all of us have forgotten why we did it.
So here is what I would do to make this easier.
if (cp->addr.type == BDADDR_BREDR) {
if (cp->disconnect) {
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
else
conn = NULL;
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
&cp->addr.bdaddr);
if (conn) {
set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
if (!cp->disconnect)
conn = NULL;
}
...
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
Add some minor comments on why we set conn = NULL and this should be a simpler block. Mainly because the difference between BR/EDR and LE are contained in a single spot and not spread out over two places.
This already takes into account my comment from above to just set the PARAM_REMOVAL flag no matter if we actually be asked to disconnect or not.
> @@ -3062,6 +3072,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth: Defer connection-parameter removal when unpairing without disconnecting
From: Marcel Holtmann @ 2014-10-11 8:44 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4q2ii0jGLXuBVu1ZOOqmQQ+m+zCne-Jizzjr6UkfcgWXA@mail.gmail.com>
Hi Alfonso,
>> I am not in favor of making Pair Device just do re-pairing. I think that is a dangerous behavior since want bondings in the kernel only in two ways. Either they are loaded via Load Long Term Keys or via Pair Device. Doing something magic is something that I consider dangerous and can lead into some flaws. If we come with a clear error than we at least know that something went wrong.
>>
>> Repairing command is possible, but I am not 100% convinced that it is a good idea. The normal workflow is that you pair and unpair a device. If you get stuck in a weird state, you unpair first which ensures that everything is cleaned out and then pair again. For the connection parameters we can just be smarter.
>
> Fair enough. Does this mean that the current patch is still of
> interest? I hope so :)
the kernel side for delaying the removal of the connection parameters is the right thing to do. We should have done in the first place actually.
>> I bet there is still a lot of improvement for our connection parameter handling. For example we could just keep all connection parameters around in cache and just expire them after 1 day or so. That would improve general handling with devices with do not pair in the first. So I would focus on being smart when dropping connection parameters and not trying to bind it too much to mgmt API visible states.
>
> I actually have a patch lying around which keeps track of
> conn_parameters within bluetoothd. I think that it would be a good
> start (I would like to complete the submissions I sent this week
> getting to it though).
I am not in favor of that. bluetoothd is supposed to be stupid when it comes to these values. I mean stupid in a sense that when the kernel tells us to store them (conn params, IRK, LTK etc.), the daemon will store them. And on reboot just reload the whole thing back into the kernel. On unpairing/removing a device, the only other thing the daemon is suppose to be doing is clear all we know about that device. Meaning removing all stored keys and params. That is all it is suppose to be doing. Plain and simple.
This basic concept has gotten us pretty far and allowed us to put the kernel in control of handling the nasty details when it comes to pairing and connection handling.
But as I said before, we can be smarter inside the kernel. So the advertising data can actually contain proposed connection interval values from the peripheral. A peripheral can you tell the central its preferred values from the start. We are currently not parsing the advertising data and pre-filling the connection request with these values from the advertising report. That is something we could do. That will clearly improve our handling of peripherals that do not create a bond with us.
And for peripherals that do not keep a bond with us, we can actually just keep the connection parameters for x amount of time and then expire them like we expire the inquiry cache. That is exactly how the inquiry cache in BR/EDR works to speed up the connection creation. We will try to re-read the clock offset before terminating a connection so that when you do a re-connection right away, we use the right clock offset.
There are tons of improvements that we can do. And most of them can be done without changing any behavior in bluetoothd or changing the mgmt API.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 11:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <F4236E26-CB50-4334-9AD9-0D930B2B9955@holtmann.org>
Hi Marcel,
>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *wor=
k)
>> conn->state =3D BT_CLOSED;
>> break;
>> }
>> +
>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type=
);
>
> do we really need this one? The hci_conn_timeout should trigger when the =
connection establishment has a timeout and that is really non of the concer=
ns here. And in case we hit an idle disconnect timeout, we are still ending=
up in hci_conn_del eventually, right? If not, I am having the slight feeli=
ng that you might have uncovered a bug that we should fix.
It's probably just lack of familiarity with the code, but it wasn't
all that clear to me that hci_conn_del is called after
hci_conn_timeout kicks in. I removed it.
>
>>
>> err =3D hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
>> }
>> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct=
hci_dev *hdev, void *data,
>> }
>>
>> if (cp->disconnect) {
>> + /* Only lookup the connection in the BR/EDR since the
>> + * LE connection was already looked up earlier.
>> + */
>> if (cp->addr.type =3D=3D BDADDR_BREDR)
>> conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> &cp->addr.bdaddr);
>> - else
>> - conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> - &cp->addr.bdaddr);
>> } else {
>> conn =3D NULL;
>> }
>
> I know that Johan told you to do it this way, but seeing the patch now ma=
kes me a bit uneasy. The usage of uninitilized_var in this case can lead to=
errors in the future. The logic that the compiler is wrong here is not obv=
ious. Which means a week from now all of us have forgotten why we did it.
>
> So here is what I would do to make this easier.
>
> if (cp->addr.type =3D=3D BDADDR_BREDR) {
> if (cp->disconnect) {
> conn =3D hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> else
> conn =3D NULL;
>
> err =3D hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK,
> &cp->addr.bdaddr);
> if (conn) {
> set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags=
);
>
> if (!cp->disconnect)
> conn =3D NULL;
> }
>
> ...
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> err =3D hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type)=
;
> }
>
> Add some minor comments on why we set conn =3D NULL and this should be a =
simpler block. Mainly because the difference between BR/EDR and LE are cont=
ained in a single spot and not spread out over two places.
>
> This already takes into account my comment from above to just set the PAR=
AM_REMOVAL flag no matter if we actually be asked to disconnect or not.
Yep, I agree, what you propose is way cleaner. Thanks.
V4 takes care of your remarks.
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 11:14 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..2911a5e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,29 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL; /* Avoid disconnecting later on*/
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ if (!cp->disconnect)
+ conn = NULL; /* Avoid disconnecting later on*/
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2755,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2764,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3068,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 11:53 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413026054-5913-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..11aac06 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..2911a5e 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2725,10 +2725,29 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->addr.type == BDADDR_BREDR) {
> + if (cp->disconnect)
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> + &cp->addr.bdaddr);
> + else
> + conn = NULL; /* Avoid disconnecting later on*/
> +
I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
Generally I would do comment on the whole code. So something like this:
/* When disconnect of the the connection is requested,
* then look up the connection. If the remote device is
* connected, it will be later used to terminate the
* link.
*
* Setting it to NULL explicitly will cause no
* termination of the link.
*/
if (cp->disconnect)
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
&cp->addr.bdaddr);
else
conn = NULL;
> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> u8 addr_type;
>
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> + if (conn) {
> + /* Defer clearing up the connection parameters
> + * until closing to give a chance of keeping
> + * them if a repairing happens.
> + */
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> +
> + if (!cp->disconnect)
> + conn = NULL; /* Avoid disconnecting later on*/
And here I would do this:
/* If disconnection is not requested, then clear the
* connection variable so that that the link is not
* terminated.
*/
if (!cp->disconnect)
conn = NULL;
> + }
> +
> if (cp->addr.type == BDADDR_LE_PUBLIC)
> addr_type = ADDR_LE_DEV_PUBLIC;
> else
> @@ -2736,8 +2755,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> -
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
>
> @@ -2747,17 +2764,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - if (cp->disconnect) {
> - if (cp->addr.type == BDADDR_BREDR)
> - conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> - &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> - } else {
> - conn = NULL;
> - }
> -
And here I would just add a minor comment to remind us why this is correct:
/* If the connection variable is set, then termination of the
* link is requested.
*/
> if (!conn) {
> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
> &rp, sizeof(rp));
> @@ -3062,6 +3068,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
> hci_conn_put(conn);
>
> mgmt_pending_remove(cmd);
> +
> + /* The device is paired so there is no need to remove
> + * its connection parameters anymore.
> + */
> + clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> }
>
> void mgmt_smp_complete(struct hci_conn *conn, bool complete)
>From a logic point of view the patch looks good to me. This is just style changes to make it easier to remember why things are done. Johan might should have a second look at it before applying.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 11:56 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4o7BE5hVzNgyXR0_jGPqNLnfjj1MgzuTU0iB-tvvgKa3A@mail.gmail.com>
Hi Alfonso,
>>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
>>> conn->state = BT_CLOSED;
>>> break;
>>> }
>>> +
>>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>
>> do we really need this one? The hci_conn_timeout should trigger when the connection establishment has a timeout and that is really non of the concerns here. And in case we hit an idle disconnect timeout, we are still ending up in hci_conn_del eventually, right? If not, I am having the slight feeling that you might have uncovered a bug that we should fix.
>
> It's probably just lack of familiarity with the code, but it wasn't
> all that clear to me that hci_conn_del is called after
> hci_conn_timeout kicks in. I removed it.
there is always a possibility that you found an actual bug that we trigger, but has not done any harm. At least not harm that anybody has noticed so far. So if this happens, then I like to fix the actual bug.
Regards
Marcel
^ permalink raw reply
* [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 14:27 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..063b0a7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ /* If disconnection is requested, then look up the
+ * connection. If the remote device is connected, it
+ * will be later used to terminate the link.
+ *
+ * Setting it to NULL explicitly will cause no
+ * termination of the link.
+ */
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL;
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ /* If disconnection is not requested, then
+ * clear the connection variable so that the
+ * link is not terminated.
+ */
+ if (!cp->disconnect)
+ conn = NULL;
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2775,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3079,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 14:30 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <0025B704-1B18-4B1E-B27D-DF84AA2F9201@holtmann.org>
Hi Marcel,
>> if (cp->addr.type == BDADDR_BREDR) {
>> + if (cp->disconnect)
>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> + &cp->addr.bdaddr);
>> + else
>> + conn = NULL; /* Avoid disconnecting later on*/
>> +
>
> I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
Excuse my ignorance, but Where is the network subsystem coding style
documented? I haven't been able to find it.
> Generally I would do comment on the whole code. So something like this:
> /* When disconnect of the the connection is requested,
> * then look up the connection. If the remote device is
> * connected, it will be later used to terminate the
> * link.
> *
> * Setting it to NULL explicitly will cause no
> * termination of the link.
> */
> if (cp->disconnect)
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> else
> conn = NULL;
>
>> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
>> } else {
>> u8 addr_type;
>>
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> + &cp->addr.bdaddr);
>> + if (conn) {
>> + /* Defer clearing up the connection parameters
>> + * until closing to give a chance of keeping
>> + * them if a repairing happens.
>> + */
>> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
>> +
>> + if (!cp->disconnect)
>> + conn = NULL; /* Avoid disconnecting later on*/
>
> And here I would do this:
>
> /* If disconnection is not requested, then clear the
> * connection variable so that that the link is not
> * terminated.
> */
> if (!cp->disconnect)
> conn = NULL;
Thanks, I submitted V5 to correct this.
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 16:10 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4oqcDKpPySM-qgscD8-=oTrm01Y_KnezTZt4irJ8C1FPg@mail.gmail.com>
Hi Alfonso,
>>> if (cp->addr.type == BDADDR_BREDR) {
>>> + if (cp->disconnect)
>>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>>> + &cp->addr.bdaddr);
>>> + else
>>> + conn = NULL; /* Avoid disconnecting later on*/
>>> +
>>
>> I do not think this is a comment style that is valid in the network subsystem coding style and even if it is valid, it would be pretty unusual.
>
> Excuse my ignorance, but Where is the network subsystem coding style
> documented? I haven't been able to find it.
that is actually a good question and I am not sure I have a really good answer for that.
It is essentially Documentation/CodingStyle plus a few extra special cases. Some of them I think are only documented in scripts/checkpatch.pl --strict. The --strict switch is also called --subjective so some of the rules really only apply to the net/ and drivers/net/. I think the term subjective gives it away pretty clearly ;)
We just end up following the rules of the network subsystem here. So there is a difference between the coding style that we use in BlueZ userspace and the one that we use in the kernel. Not a major difference, but it comes to comment styles and multi-line indentations you see the difference.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-11 16:13 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413037644-15858-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 07ddeed62..b8685a7 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -555,6 +555,7 @@ enum {
> HCI_CONN_STK_ENCRYPT,
> HCI_CONN_AUTH_INITIATOR,
> HCI_CONN_DROP,
> + HCI_CONN_PARAM_REMOVAL_PEND,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index b9517bd..11aac06 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
>
> hci_conn_del_sysfs(conn);
>
> + if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> +
> hci_dev_put(hdev);
>
> hci_conn_put(conn);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fd88b0..063b0a7 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> }
>
> if (cp->addr.type == BDADDR_BREDR) {
> + /* If disconnection is requested, then look up the
> + * connection. If the remote device is connected, it
> + * will be later used to terminate the link.
> + *
> + * Setting it to NULL explicitly will cause no
> + * termination of the link.
> + */
> + if (cp->disconnect)
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> + &cp->addr.bdaddr);
> + else
> + conn = NULL;
> +
> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> u8 addr_type;
>
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> + &cp->addr.bdaddr);
> + if (conn) {
> + /* Defer clearing up the connection parameters
> + * until closing to give a chance of keeping
> + * them if a repairing happens.
> + */
> + set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
> +
> + /* If disconnection is not requested, then
> + * clear the connection variable so that the
> + * link is not terminated.
> + */
> + if (!cp->disconnect)
> + conn = NULL;
> + }
> +
> if (cp->addr.type == BDADDR_LE_PUBLIC)
> addr_type = ADDR_LE_DEV_PUBLIC;
> else
> @@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> - hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
> -
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
>
> @@ -2747,17 +2775,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - if (cp->disconnect) {
> - if (cp->addr.type == BDADDR_BREDR)
> - conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> - &cp->addr.bdaddr);
> - else
> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> - &cp->addr.bdaddr);
> - } else {
> - conn = NULL;
> - }
> -
I would have put an extra comment here as well to remind us that conn is for indicating to terminate the connection or not. See my review of v4 of your patch.
> if (!conn) {
> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
> &rp, sizeof(rp));
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v5] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 21:44 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <2F926C4D-DBA0-4B6C-A368-FD8E95509A5A@holtmann.org>
Hi Marcel,
> I would have put an extra comment here as well to remind us that conn is for indicating to terminate the connection or not. See my review of v4 of your patch.
>
>> if (!conn) {
>> err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
>> &rp, sizeof(rp));
Ops, I missed that one. v6 includes the comment.
Thanks,
Fons
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-11 21:44 UTC (permalink / raw)
To: linux-bluetooth
Systematically removing the LE connection parameters and autoconnect
action is inconvenient for rebonding without disconnecting from
userland (i.e. unpairing followed by repairing without
disconnecting). The parameters will be lost after unparing and
userland needs to take care of book-keeping them and re-adding them.
This patch allows userland to forget about parameter management when
rebonding without disconnecting. It defers clearing the connection
parameters when unparing without disconnecting, giving a chance of
keeping the parameters if a repairing happens before the connection is
closed.
Signed-off-by: Alfonso Acosta <fons@spotify.com>
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 07ddeed62..b8685a7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -555,6 +555,7 @@ enum {
HCI_CONN_STK_ENCRYPT,
HCI_CONN_AUTH_INITIATOR,
HCI_CONN_DROP,
+ HCI_CONN_PARAM_REMOVAL_PEND,
};
static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9517bd..11aac06 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -544,6 +544,9 @@ int hci_conn_del(struct hci_conn *conn)
hci_conn_del_sysfs(conn);
+ if (test_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+
hci_dev_put(hdev);
hci_conn_put(conn);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fd88b0..9c4daf7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2725,10 +2725,40 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (cp->addr.type == BDADDR_BREDR) {
+ /* If disconnection is requested, then look up the
+ * connection. If the remote device is connected, it
+ * will be later used to terminate the link.
+ *
+ * Setting it to NULL explicitly will cause no
+ * termination of the link.
+ */
+ if (cp->disconnect)
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
+ &cp->addr.bdaddr);
+ else
+ conn = NULL;
+
err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
} else {
u8 addr_type;
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
+ &cp->addr.bdaddr);
+ if (conn) {
+ /* Defer clearing up the connection parameters
+ * until closing to give a chance of keeping
+ * them if a repairing happens.
+ */
+ set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
+
+ /* If disconnection is not requested, then
+ * clear the connection variable so that the
+ * link is not terminated.
+ */
+ if (!cp->disconnect)
+ conn = NULL;
+ }
+
if (cp->addr.type == BDADDR_LE_PUBLIC)
addr_type = ADDR_LE_DEV_PUBLIC;
else
@@ -2736,8 +2766,6 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
- hci_conn_params_del(hdev, &cp->addr.bdaddr, addr_type);
-
err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
}
@@ -2747,17 +2775,9 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- if (cp->disconnect) {
- if (cp->addr.type == BDADDR_BREDR)
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
- &cp->addr.bdaddr);
- else
- conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
- &cp->addr.bdaddr);
- } else {
- conn = NULL;
- }
-
+ /* If the connection variable is set, then termination of the
+ * link is requested.
+ */
if (!conn) {
err = cmd_complete(sk, hdev->id, MGMT_OP_UNPAIR_DEVICE, 0,
&rp, sizeof(rp));
@@ -3062,6 +3082,11 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
hci_conn_put(conn);
mgmt_pending_remove(cmd);
+
+ /* The device is paired so there is no need to remove
+ * its connection parameters anymore.
+ */
+ clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
}
void mgmt_smp_complete(struct hci_conn *conn, bool complete)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-12 0:16 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <5240CE7E-B366-410A-93C4-5FCC3358A68F@holtmann.org>
Hi Marcel,
>>>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *w=
ork)
>>>> conn->state =3D BT_CLOSED;
>>>> break;
>>>> }
>>>> +
>>>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags=
))
>>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_ty=
pe);
>>>
>>> do we really need this one? The hci_conn_timeout should trigger when th=
e connection establishment has a timeout and that is really non of the conc=
erns here. And in case we hit an idle disconnect timeout, we are still endi=
ng up in hci_conn_del eventually, right? If not, I am having the slight fee=
ling that you might have uncovered a bug that we should fix.
>>
>> It's probably just lack of familiarity with the code, but it wasn't
>> all that clear to me that hci_conn_del is called after
>> hci_conn_timeout kicks in. I removed it.
>
> there is always a possibility that you found an actual bug that we trigge=
r, but has not done any harm. At least not harm that anybody has noticed so=
far. So if this happens, then I like to fix the actual bug.
I have gone through the code again to revisit what made me think that
clearing the flag was necessary in hci_conn_timeout() and I may have
possibly found a couple of problems in the deallocation of connections
and handling of disconnection failures.
What made me think that the change in hci_conn_timeout() was necessary
were some call patterns like the following in hci_event.c:
hci_disconnect(conn, reason);
hci_conn_drop(conn);
I wrongly inferred from this that the connection clearing was
sometimes done in hci_conn_timeout() (hci_conn_drop() queues
hci_conn_timeout() when the reference count drops to zero) and not in
hci_conn_del(), to make sure that the connection was cleared
regardless of the status/completion events received from the
controller .
I see that, when receiving a Disconnect Complete event,
hci_disconn_complete_evt() deallocates the connection. But ...
1. What if the controller misbehaves and doesn't send a Disconnect
Complete event? AFAIU, the connection will never be deallocated in
this case.
Wouldn't it make sense to have a timer for this? e.g. make
hci_disconnect() queue hci_conn_timeout() and, in the later, delete
the connection if it's not referenced and it was already marked as
closed.
2. What if the controller sends an error in the Command Status event
and the Disconnect Complete never arrives?
I see that hci_cs_disconnect() notifies userland if it was triggered
through mgmt, but it doesn't provide a retry mechanism in case the
disconnection came from the kernel itself. In fact, what happens with
the connection's deallocation in this case? Is it ensured in any way?
I apologize in advance if my analysis doesn't make much sense or makes
wrong assumptions, as you know, I am still a newbie :)
--=20
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v3] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-12 10:45 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <CAHF=Y4r+ofDDBv5+YpFPcvctqYAAGiwNj0u94+XDsoskbSF4Mw@mail.gmail.com>
> I see that, when receiving a Disconnect Complete event,
> hci_disconn_complete_evt() deallocates the connection. But ...
>
> 1. What if the controller misbehaves and doesn't send a Disconnect
> Complete event? AFAIU, the connection will never be deallocated in
> this case.
>
> Wouldn't it make sense to have a timer for this? e.g. make
> hci_disconnect() queue hci_conn_timeout() and, in the later, delete
> the connection if it's not referenced and it was already marked as
> closed.
>
> 2. What if the controller sends an error in the Command Status event
> and the Disconnect Complete never arrives?
>
> I see that hci_cs_disconnect() notifies userland if it was triggered
> through mgmt, but it doesn't provide a retry mechanism in case the
> disconnection came from the kernel itself. In fact, what happens with
> the connection's deallocation in this case? Is it ensured in any way?
3. What if the controller sends an error in the Disconnect Complete? I
see the same issues as in (2).
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Marcel Holtmann @ 2014-10-12 22:45 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1413063887-25870-1-git-send-email-fons@spotify.com>
Hi Alfonso,
> Systematically removing the LE connection parameters and autoconnect
> action is inconvenient for rebonding without disconnecting from
> userland (i.e. unpairing followed by repairing without
> disconnecting). The parameters will be lost after unparing and
> userland needs to take care of book-keeping them and re-adding them.
>
> This patch allows userland to forget about parameter management when
> rebonding without disconnecting. It defers clearing the connection
> parameters when unparing without disconnecting, giving a chance of
> keeping the parameters if a repairing happens before the connection is
> closed.
>
> Signed-off-by: Alfonso Acosta <fons@spotify.com>
patch looks good, but I wonder why we do not have a diffstat here. Are you not using git format-patch and git send-email to send patches? If not, then you might want to start with that.
Anyhow, since we are in the middle of the merge window for 3.18, I am not yet applying the patch to bluetooth-next, but just for reference here, ack from my side.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v6] Bluetooth: Defer connection-parameter removal when unpairing
From: Alfonso Acosta @ 2014-10-13 7:35 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development
In-Reply-To: <EDC21CD6-FBF6-43DA-A8E4-40BF710EA50E@holtmann.org>
Hi Marcel
> patch looks good, but I wonder why we do not have a diffstat here. Are you not using git format-patch and git send-email to send patches? If not, then you might want to start with that.
I am indeed using format-patch. I went through my commands and I saw I
inserted a -p by error (probably a residue of "log -p").
Do you want me to send a new patch with the stats?
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
^ permalink raw reply
* Re: [PATCH v2 2/2] core: Add subscription API for Manufacturer Specific Data
From: Johan Hedberg @ 2014-10-13 8:01 UTC (permalink / raw)
To: Alfonso Acosta; +Cc: linux-bluetooth
In-Reply-To: <1412954626-30226-3-git-send-email-fons@spotify.com>
Hi Alfonso,
On Fri, Oct 10, 2014, Alfonso Acosta wrote:
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -30,6 +30,8 @@
> #include <glib.h>
> #include <stdbool.h>
>
> +#include "eir.h"
> +
> #define MAX_NAME_LENGTH 248
>
> /* Invalid SSP passkey value used to indicate negative replies */
> @@ -138,6 +140,14 @@ struct btd_adapter_pin_cb_iter *btd_adapter_pin_cb_iter_new(
> void btd_adapter_pin_cb_iter_free(struct btd_adapter_pin_cb_iter *iter);
> bool btd_adapter_pin_cb_iter_end(struct btd_adapter_pin_cb_iter *iter);
>
> +typedef void (*btd_msd_cb_t) (struct btd_adapter *adapter,
> + struct btd_device *dev,
> + const struct eir_msd *msd);
> +void btd_adapter_register_msd_cb(struct btd_adapter *adapter,
> + btd_msd_cb_t cb);
In our user space code we try to follow the following principles for
internal header files:
1) The c-file that includes them should also include the prerequisites
2) We don't use multi-include guards
The general idea is that we don't want hidden and implicit dependencies
but prefer having them explicitly spelled out. This practice also helps
detect circular dependencies. For public header files or those of
library-like modules we don't follow this practice (e.g. gdbus/gdbus.h).
As for your patch, I'd suggest to spell out each of the three variables
in your bt_msd_cb_t instead of using the "struct eir_msd" in adapter.h.
That way you don't have a dependency to eir.h from adapter.h.
Johan
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Martin Townsend @ 2014-10-13 8:44 UTC (permalink / raw)
To: Alexander Aring, Martin Townsend
Cc: linux-bluetooth, linux-wpan, marcel, jukka.rissanen
In-Reply-To: <20141011073620.GA28500@omega>
Hi Alex,
On 11/10/14 08:36, Alexander Aring wrote:
> Hi Martin,
>
> This is only a summarize what we should now to try to do here for next
> version.
>
> I know... this patch ends in a global debate on principles how we deal
> with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> more a hack.
>
> On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
>> Currently there are potentially 2 skb_copy_expand calls in IPHC
>> decompression. This patch replaces this with one call to
>> pskb_expand_head. It also checks to see if there is enough headroom
>> first to ensure it's only done if necessary.
>> As pskb_expand_head must only have one reference the calling code
>> now ensures this.
>>
>> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
>> ---
>> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
>> net/bluetooth/6lowpan.c | 7 +++++++
>> 2 files changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index 142eef5..853b4b8 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
>> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
>> struct net_device *dev, skb_delivery_cb deliver_skb)
>> {
>> - struct sk_buff *new;
>> int stat;
>>
>> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
>> - GFP_ATOMIC);
>> - kfree_skb(skb);
>> -
>> - if (!new)
>> - return -ENOMEM;
>> -
>> - skb_push(new, sizeof(struct ipv6hdr));
>> - skb_reset_network_header(new);
>> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
>> + skb_push(skb, sizeof(struct ipv6hdr));
>> + skb_reset_network_header(skb);
>> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
>>
>> - new->protocol = htons(ETH_P_IPV6);
>> - new->pkt_type = PACKET_HOST;
>> - new->dev = dev;
>> + skb->protocol = htons(ETH_P_IPV6);
>> + skb->pkt_type = PACKET_HOST;
>> + skb->dev = dev;
>>
>> raw_dump_table(__func__, "raw skb data dump before receiving",
>> - new->data, new->len);
>> + skb->data, skb->len);
>>
>> - stat = deliver_skb(new, dev);
>> + stat = deliver_skb(skb, dev);
>>
>> - kfree_skb(new);
>> + consume_skb(skb);
>>
>> return stat;
>> }
>> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> /* UDP data uncompression */
>> if (iphc0 & LOWPAN_IPHC_NH_C) {
>> struct udphdr uh;
>> - struct sk_buff *new;
>> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
>>
>> if (uncompress_udp_header(skb, &uh))
>> goto drop;
>> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> /* replace the compressed UDP head by the uncompressed UDP
>> * header
>> */
>> - new = skb_copy_expand(skb, sizeof(struct udphdr),
>> - skb_tailroom(skb), GFP_ATOMIC);
>> - kfree_skb(skb);
>> -
>> - if (!new)
>> - return -ENOMEM;
>> -
>> - skb = new;
>> + if (skb_headroom(skb) < needed) {
>> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
>> + if (unlikely(err)) {
>> + kfree_skb(skb);
>> + return err;
>> + }
>> + }
> use skb_cow_head here.
I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
skb_cow seems a good fit to me though.
>
>>
>> skb_push(skb, sizeof(struct udphdr));
>> skb_reset_transport_header(skb);
>> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
>> (u8 *)&uh, sizeof(uh));
>>
>> hdr.nexthdr = UIP_PROTO_UDP;
>> + } else {
>> + if (skb_headroom(skb) < sizeof(hdr)) {
>> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
>> + if (unlikely(err)) {
>> + kfree_skb(skb);
>> + return err;
>> + }
>> + }
> same here.
>
>> }
>>
>> hdr.payload_len = htons(skb->len);
>> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
>> index c2e0d14..6643a7c 100644
>> --- a/net/bluetooth/6lowpan.c
>> +++ b/net/bluetooth/6lowpan.c
>> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>> kfree_skb(local_skb);
>> kfree_skb(skb);
>> } else {
>> + /* Decompression may use pskb_expand_head so no shared skb's */
>> + skb = skb_share_check(skb, GFP_ATOMIC);
>> + if (!skb) {
>> + dev->stats.rx_dropped++;
>> + return NET_RX_DROP;
>> + }
>> +
> I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> is the share_check really needed? In my opinion there are several things
> wrong.
>
> What bluetooth should do here is:
>
> Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
>
> In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
>
> - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> for the one byte dispatch.
> - run other things like skb_reset_network_header, etc...
>
> -> Currently this works because we run a complete skb_copy_expand here.
> But this is not needed, we need a clone because we don't modify the
> data buffer.
>
> -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> for this here.
>
>
>
> In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> this is already handled by "skb_share_check" then.
Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
- Martin
^ permalink raw reply
* Re: [PATCH v5 bluetooth-next] 6lowpan: Use pskb_expand_head in IPHC decompression.
From: Alexander Aring @ 2014-10-13 8:55 UTC (permalink / raw)
To: Martin Townsend
Cc: Martin Townsend, linux-bluetooth, linux-wpan, marcel,
jukka.rissanen
In-Reply-To: <543B90FA.2060603@xsilon.com>
Hi Martin,
On Mon, Oct 13, 2014 at 09:44:42AM +0100, Martin Townsend wrote:
> Hi Alex,
>
> On 11/10/14 08:36, Alexander Aring wrote:
> > Hi Martin,
> >
> > This is only a summarize what we should now to try to do here for next
> > version.
> >
> > I know... this patch ends in a global debate on principles how we deal
> > with the replacement of 6LoWPAN -> IPv6 header. Current behaviour is
> > more a hack.
> >
> > On Thu, Oct 09, 2014 at 08:46:34AM +0100, Martin Townsend wrote:
> >> Currently there are potentially 2 skb_copy_expand calls in IPHC
> >> decompression. This patch replaces this with one call to
> >> pskb_expand_head. It also checks to see if there is enough headroom
> >> first to ensure it's only done if necessary.
> >> As pskb_expand_head must only have one reference the calling code
> >> now ensures this.
> >>
> >> Signed-off-by: Martin Townsend <martin.townsend@xsilon.com>
> >> ---
> >> net/6lowpan/iphc.c | 51 ++++++++++++++++++++++++-------------------------
> >> net/bluetooth/6lowpan.c | 7 +++++++
> >> 2 files changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> >> index 142eef5..853b4b8 100644
> >> --- a/net/6lowpan/iphc.c
> >> +++ b/net/6lowpan/iphc.c
> >> @@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
> >> static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
> >> struct net_device *dev, skb_delivery_cb deliver_skb)
> >> {
> >> - struct sk_buff *new;
> >> int stat;
> >>
> >> - new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
> >> - GFP_ATOMIC);
> >> - kfree_skb(skb);
> >> -
> >> - if (!new)
> >> - return -ENOMEM;
> >> -
> >> - skb_push(new, sizeof(struct ipv6hdr));
> >> - skb_reset_network_header(new);
> >> - skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
> >> + skb_push(skb, sizeof(struct ipv6hdr));
> >> + skb_reset_network_header(skb);
> >> + skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
> >>
> >> - new->protocol = htons(ETH_P_IPV6);
> >> - new->pkt_type = PACKET_HOST;
> >> - new->dev = dev;
> >> + skb->protocol = htons(ETH_P_IPV6);
> >> + skb->pkt_type = PACKET_HOST;
> >> + skb->dev = dev;
> >>
> >> raw_dump_table(__func__, "raw skb data dump before receiving",
> >> - new->data, new->len);
> >> + skb->data, skb->len);
> >>
> >> - stat = deliver_skb(new, dev);
> >> + stat = deliver_skb(skb, dev);
> >>
> >> - kfree_skb(new);
> >> + consume_skb(skb);
> >>
> >> return stat;
> >> }
> >> @@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> /* UDP data uncompression */
> >> if (iphc0 & LOWPAN_IPHC_NH_C) {
> >> struct udphdr uh;
> >> - struct sk_buff *new;
> >> + const int needed = sizeof(struct udphdr) + sizeof(hdr);
> >>
> >> if (uncompress_udp_header(skb, &uh))
> >> goto drop;
> >> @@ -468,14 +460,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> /* replace the compressed UDP head by the uncompressed UDP
> >> * header
> >> */
> >> - new = skb_copy_expand(skb, sizeof(struct udphdr),
> >> - skb_tailroom(skb), GFP_ATOMIC);
> >> - kfree_skb(skb);
> >> -
> >> - if (!new)
> >> - return -ENOMEM;
> >> -
> >> - skb = new;
> >> + if (skb_headroom(skb) < needed) {
> >> + err = pskb_expand_head(skb, needed, 0, GFP_ATOMIC);
> >> + if (unlikely(err)) {
> >> + kfree_skb(skb);
> >> + return err;
> >> + }
> >> + }
> > use skb_cow_head here.
> I don't know much about skb_cow_head but from the description it says that only the head is writable which I assume is the newly requested headroom? I imagine this is for the usual case where you are adding headers and you go through the protocol layers. For 6lowpan aren't we removing one header and replacing with a decompressed header which would suggest that we are going to modify the data (which would contain the 6lowpan header).
>
> skb_cow seems a good fit to me though.
okay, then use skb_cow here. I will remember that we could possible
maybe use "skb_cow_head" here. But yea I mean we running lot of pulls
and move the head pointer, then push now data on it, this would
overwrite the pulled data and other skb's may not pull here. If the data
is shared there, this would crash.
> >
> >>
> >> skb_push(skb, sizeof(struct udphdr));
> >> skb_reset_transport_header(skb);
> >> @@ -485,6 +476,14 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
> >> (u8 *)&uh, sizeof(uh));
> >>
> >> hdr.nexthdr = UIP_PROTO_UDP;
> >> + } else {
> >> + if (skb_headroom(skb) < sizeof(hdr)) {
> >> + err = pskb_expand_head(skb, sizeof(hdr), 0, GFP_ATOMIC);
> >> + if (unlikely(err)) {
> >> + kfree_skb(skb);
> >> + return err;
> >> + }
> >> + }
> > same here.
> >
> >> }
> >>
> >> hdr.payload_len = htons(skb->len);
> >> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> >> index c2e0d14..6643a7c 100644
> >> --- a/net/bluetooth/6lowpan.c
> >> +++ b/net/bluetooth/6lowpan.c
> >> @@ -343,6 +343,13 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> >> kfree_skb(local_skb);
> >> kfree_skb(skb);
> >> } else {
> >> + /* Decompression may use pskb_expand_head so no shared skb's */
> >> + skb = skb_share_check(skb, GFP_ATOMIC);
> >> + if (!skb) {
> >> + dev->stats.rx_dropped++;
> >> + return NET_RX_DROP;
> >> + }
> >> +
> > I see now that case LOWPAN_DISPATCH_IPHC: runs a "local_skb =
> > skb_clone(skb, GFP_ATOMIC);" and send the local_skb to process_data. So
> > is the share_check really needed? In my opinion there are several things
> > wrong.
> >
> > What bluetooth should do here is:
> >
> > Always run skb = skb_share_check(skb, GFP_ATOMIC); at first of this function.
> >
> > In if (skb->data[0] == LOWPAN_DISPATCH_IPV6):
> >
> > - only run a skb_pull (doesn't change the data buffer, only skb attribute)
> > for the one byte dispatch.
> > - run other things like skb_reset_network_header, etc...
> >
> > -> Currently this works because we run a complete skb_copy_expand here.
> > But this is not needed, we need a clone because we don't modify the
> > data buffer.
> >
> > -> To Jukka: I don't see a skb_pull for the one byte dispatch value?
> > What's happend here? Also IPv6 dispatch is part of rfc4944 and btle
> > 6LoWPAN is only realted to rfc6282 (IPHC). I think you don't need handling
> > for this here.
> >
> >
> >
> > In "case LOWPAN_DISPATCH_IPHC:" we should remove the skb_clone, because
> > this is already handled by "skb_share_check" then.
> Not necessarily, if the skb isn't shared, ie no-one has called skb_get on it then the clone will not happen. I tried removing the skb_clone as in theory psk_expand_head should handle cloned skb's but I think it caused the oops that Jukka was seeing. Because of this I only want to make changes absolutely necessary to get pskb_expand_head or skb_cow working as I can't test the bluetooth code and have to rely on Jukka.
>
> If it's ok with everyone I'll change pskb_expand_head to skb_cow or skb_cow_head and leave the bluetooth code as it is with this v5 patch.
For me it's confusing, you do that on skb, but we never used skb
afterwards (maybe on freeing) but we use local_skb, the local_skb should
always be cloned because we running skb_clone before.
skb_share_check is a function which checks is the skb shared, then clone
it. But we don't do anything with skb afterwards.
- Alex
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox