[RFC] Bluetooth: hci_sock: Set flag to all sockets

Commit Message

Tedd Ho-Jeong An Oct. 1, 2021, 3:59 a.m. UTC

From: Tedd Ho-Jeong An <[e-mail]>

The mgmt_limited_event() send the event to the socket that matches the
flag type, but also it skips to the given socket object in the
parameter.

For Local Out of Band Data Updated Event and Experimental Feature
Changed Event, it sets flags only for the socket which the change was
triggered, the event cannot be sent to the client via any sockets
because the flag is not set for other sockets and it doens't send to the
socket which the change was triggered.

This patch adds the function that sets the flag for all available
management sockets, so the mgmt_limited_event() still can send the event
to the management sockets other than the one through which the change
was triggered.

Signed-off-by: Tedd Ho-Jeong An <[e-mail]>
---
 include/net/bluetooth/bluetooth.h |  1 +
 net/bluetooth/hci_sock.c          |  9 +++++++++
 net/bluetooth/mgmt.c              | 16 ++++++++--------
 3 files changed, 18 insertions(+), 8 deletions(-)

Comments

Marcel Holtmann Oct. 1, 2021, 5:43 a.m. UTC | #1

Hi Tedd,

> The mgmt_limited_event() send the event to the socket that matches the
> flag type, but also it skips to the given socket object in the
> parameter.
> 
> For Local Out of Band Data Updated Event and Experimental Feature
> Changed Event, it sets flags only for the socket which the change was
> triggered, the event cannot be sent to the client via any sockets
> because the flag is not set for other sockets and it doens't send to the
> socket which the change was triggered.
> 
> This patch adds the function that sets the flag for all available
> management sockets, so the mgmt_limited_event() still can send the event
> to the management sockets other than the one through which the change
> was triggered.

actually that is on purpose. Only the socket that used a specific mgmt commands gets to see the new events. So if you have a second listening socket that just cares about the events, it has to at least issue the “read” command to tell mgmt that it does understand it. There is no point in sending out signals to all mgmt sockets if you haven’t read an initial status first. The updates would make no sense to you.

Regards

Marcel

An, Tedd Oct. 1, 2021, 10:07 p.m. UTC | #2

Hi Marcel

On Fri, 2021-10-01 at 07:43 +0200, Marcel Holtmann wrote:
> Hi Tedd,
> 
> > The mgmt_limited_event() send the event to the socket that matches the
> > flag type, but also it skips to the given socket object in the
> > parameter.
> > 
> > For Local Out of Band Data Updated Event and Experimental Feature
> > Changed Event, it sets flags only for the socket which the change was
> > triggered, the event cannot be sent to the client via any sockets
> > because the flag is not set for other sockets and it doens't send to the
> > socket which the change was triggered.
> > 
> > This patch adds the function that sets the flag for all available
> > management sockets, so the mgmt_limited_event() still can send the event
> > to the management sockets other than the one through which the change
> > was triggered.
> 
> actually that is on purpose. Only the socket that used a specific mgmt commands gets to see the
> new events. So if you have a second listening socket that just cares about the events, it has to
> at least issue the “read” command to tell mgmt that it does understand it. There is no point in
> sending out signals to all mgmt sockets if you haven’t read an initial status first. The updates
> would make no sense to you.
> 
Thanks for the details. And the code make sense now. Need to update mgmt-tester with these then.

> Regards
> 
> Marcel
> 

Regards,

Tedd

Patch

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 3271870fd85e..e7ff29842137 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -513,6 +513,7 @@  static inline struct sk_buff *bt_skb_sendmmsg(struct sock *sk,
 int bt_to_errno(u16 code);
 
 void hci_sock_set_flag(struct sock *sk, int nr);
+void hci_sock_set_flag_all(int nr);
 void hci_sock_clear_flag(struct sock *sk, int nr);
 int hci_sock_test_flag(struct sock *sk, int nr);
 unsigned short hci_sock_get_channel(struct sock *sk);
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 99de17922bda..eba86c141ced 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -162,6 +162,15 @@  static struct bt_sock_list hci_sk_list = {
 	.lock = __RW_LOCK_UNLOCKED(hci_sk_list.lock)
 };
 
+void hci_sock_set_flag_all(int nr)
+{
+	struct sock *sk;
+
+	sk_for_each(sk, &hci_sk_list.head) {
+		hci_sock_set_flag(sk, nr);
+	}
+}
+
 static bool is_filtered_packet(struct sock *sk, struct sk_buff *skb)
 {
 	struct hci_filter *flt;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3e5283607b97..333e2aa5b176 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3890,7 +3890,7 @@  static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
 	/* After reading the experimental features information, enable
 	 * the events to update client on any future change.
 	 */
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 
 	return mgmt_cmd_complete(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
 				 MGMT_OP_READ_EXP_FEATURES_INFO,
@@ -3975,7 +3975,7 @@  static int set_zero_key_func(struct sock *sk, struct hci_dev *hdev,
 			exp_ll_privacy_feature_changed(false, hdev, sk);
 	}
 
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 
 	return mgmt_cmd_complete(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
 				 MGMT_OP_SET_EXP_FEATURE, 0,
@@ -4016,7 +4016,7 @@  static int set_debug_func(struct sock *sk, struct hci_dev *hdev,
 	memcpy(rp.uuid, debug_uuid, 16);
 	rp.flags = cpu_to_le32(val ? BIT(0) : 0);
 
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 
 	err = mgmt_cmd_complete(sk, MGMT_INDEX_NONE,
 				MGMT_OP_SET_EXP_FEATURE, 0,
@@ -4082,7 +4082,7 @@  static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
 	memcpy(rp.uuid, rpa_resolution_uuid, 16);
 	rp.flags = cpu_to_le32(flags);
 
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 
 	err = mgmt_cmd_complete(sk, hdev->id,
 				MGMT_OP_SET_EXP_FEATURE, 0,
@@ -4150,7 +4150,7 @@  static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev,
 
 	memcpy(rp.uuid, quality_report_uuid, 16);
 	rp.flags = cpu_to_le32(val ? BIT(0) : 0);
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 	err = mgmt_cmd_complete(sk, hdev->id,
 				MGMT_OP_SET_EXP_FEATURE, 0,
 				&rp, sizeof(rp));
@@ -4223,7 +4223,7 @@  static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev,
 
 	memcpy(rp.uuid, offload_codecs_uuid, 16);
 	rp.flags = cpu_to_le32(val ? BIT(0) : 0);
-	hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_EXP_FEATURE_EVENTS);
 	err = mgmt_cmd_complete(sk, hdev->id,
 				MGMT_OP_SET_EXP_FEATURE, 0,
 				&rp, sizeof(rp));
@@ -7460,7 +7460,7 @@  static void read_local_oob_ext_data_complete(struct hci_dev *hdev, u8 status,
 	if (err < 0 || status)
 		goto done;
 
-	hci_sock_set_flag(cmd->sk, HCI_MGMT_OOB_DATA_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_OOB_DATA_EVENTS);
 
 	err = mgmt_limited_event(MGMT_EV_LOCAL_OOB_DATA_UPDATED, hdev,
 				 mgmt_rp, sizeof(*mgmt_rp) + eir_len,
@@ -7636,7 +7636,7 @@  static int read_local_oob_ext_data(struct sock *sk, struct hci_dev *hdev,
 
 	hci_dev_unlock(hdev);
 
-	hci_sock_set_flag(sk, HCI_MGMT_OOB_DATA_EVENTS);
+	hci_sock_set_flag_all(HCI_MGMT_OOB_DATA_EVENTS);
 
 	status = MGMT_STATUS_SUCCESS;