BNEP multicast filter is filtering multicast addresses in default case although differently specified

PATCH In review Danny Schweizer 2 participants 6 messages Nov 18, 2015 – Dec 5, 2015

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Danny Schweizer

author

2015-11-18 12:16 UTC #0 lore raw

PATCH 1/1 BNEP multicast filter is filtering multicast addresses in default case although differently specified

This patch fixes the Bug 106691 by changing the default multicast filter to not filter out any multicast addresses.

---
 net/bluetooth/bnep/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/bluetooth/bnep/core.c +0 -0

index 1641367..ba2467a 100644
@@ -609,7 +609,7 @@ int bnep_add_connection(struct bnep_connadd_req

*req, struct socket *sock)

#ifdef CONFIG_BT_BNEP_MC_FILTER
/* Set default mc filter */
- set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
+ s->mc_filter = ~0LL;
#endif

#ifdef CONFIG_BT_BNEP_PROTO_FILTER

Signature · 1 line

2.1.4

Marcel Holtmann

maintainer

2015-11-18 12:45 UTC #1 lore raw

Hi Danny,

Quoted 12 lines

This patch fixes the Bug 106691 by changing the default multicast filter to not filter out any multicast addresses.
---
net/bluetooth/bnep/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/bnep/core.c +0 -0
index 1641367..ba2467a 100644

net/bluetooth/bnep/core.c int bnep_add_connection(struct bnep_connadd_req · line 614

Quoted 7 lines

net/bluetooth/bnep/core.c +1 -1

@@ -609,7 +609,7 @@ int bnep_add_connection(struct bnep_connadd_req
 *req, struct socket *sock)
 
 #ifdef CONFIG_BT_BNEP_MC_FILTER
 	/* Set default mc filter */
-	set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
+	s->mc_filter = ~0LL;
 #endif

I am not convinced that these two are actually the same expressions. You need to explain this a lot more in detail on why this change is correct.

Regards

Marcel

Danny Schweizer

author

2015-11-18 13:12 UTC #2 lore raw

Hi Marcel,

Quoted 24 lines

Hi Danny,
Quoted 12 lines
This patch fixes the Bug 106691 by changing the default multicast filter to not filter out any multicast addresses.
---
net/bluetooth/bnep/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/bnep/core.c +0 -0
index 1641367..ba2467a 100644
net/bluetooth/bnep/core.c int bnep_add_connection(struct bnep_connadd_req · line 614
Quoted 7 lines
net/bluetooth/bnep/core.c +1 -1
@@ -609,7 +609,7 @@ int bnep_add_connection(struct bnep_connadd_req
 *req, struct socket *sock)
 
 #ifdef CONFIG_BT_BNEP_MC_FILTER
 	/* Set default mc filter */
-	set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
+	s->mc_filter = ~0LL;
 #endif
I am not convinced that these two are actually the same expressions. You need to explain this a lot more in detail on why this change is correct.

Quoted 3 lines

Regards
Marcel

The new expression should set the default mc_filter such that it does not filter out any multicast addresses (it sets the mc_filter to all ones). Up to now, the default mc_filter filters out all multicast addresses except for the broadcast address. The reason why this is needed is explained in the description of bug 106691. I will copy it here:

A Linux PC is connected with another device over Bluetooth PAN using a BNEP interface.

Whenever a packet is tried to be sent over the BNEP interface, the function "bnep_net_xmit()" in "net/bluetooth/bnep/netdev.c" is called. This function calls "bnep_net_mc_filter()", which checks (if the destination address is multicast) if the address is set in a certain multicast filter (&s->mc_filter). If it is not, then it is not sent out.

This filter is only changed in two other functions, found in net/bluetooth/bnep/core.c": in "bnep_ctrl_set_mc_filter()", which is only called if a message of type "BNEP_FILTER_MULTI_ADDR_SET" is received. Otherwise, it is set in "bnep_add_connection()", where it is set to a default value which only adds the broadcast address to the filter:

set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);

To sum up, if the BNEP interface does not receive any message of type "BNEP_FILTER_MULTI_ADDR_SET", it will not send out any messages with multicast destination addresses except for broadcast.

However, in the BNEP specification (page 27 in
http://grouper.ieee.org/groups/802/15/Bluetooth/BNEP.pdf), it is said that per default, all multicast addresses should not be filtered, i.e. the BNEP interface should be able to send packets with any multicast destination address.

It seems that the default case is wrong: the multicast filter should not block almost all multicast addresses, but should not filter out any.

This leads to the problem that e.g. Neighbor Solicitation messages sent with Bluetooth PAN over the BNEP interface to a multicast destination address other than broadcast are blocked and not sent out.

Regards,
Danny

Marcel Holtmann

maintainer

2015-11-19 13:28 UTC #3 lore raw

Hi Danny,

Quoted 58 lines

Quoted 14 lines
Quoted 14 lines
This patch fixes the Bug 106691 by changing the default multicast filter to not filter out any multicast addresses.
---
net/bluetooth/bnep/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/bnep/core.c +0 -0
index 1641367..ba2467a 100644
@@ -609,7 +609,7 @@ int bnep_add_connection(struct bnep_connadd_req
 *req, struct socket *sock)
 
Quoted 8 lines
net/bluetooth/bnep/core.c int bnep_add_connection(struct bnep_connadd_req · line 614
Quoted 5 lines
net/bluetooth/bnep/core.c +1 -1
 #ifdef CONFIG_BT_BNEP_MC_FILTER
 	/* Set default mc filter */
-	set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
+	s->mc_filter = ~0LL;
 #endif
I am not convinced that these two are actually the same expressions. You need to explain this a lot more in detail on why this change is correct.
The new expression should set the default mc_filter such that it does not filter out any multicast addresses (it sets the mc_filter to all ones). Up to now, the default mc_filter filters out all multicast addresses except for the broadcast address. The reason why this is needed is explained in the description of bug 106691. I will copy it here:
A Linux PC is connected with another device over Bluetooth PAN using a BNEP interface.
Whenever a packet is tried to be sent over the BNEP interface, the function "bnep_net_xmit()" in "net/bluetooth/bnep/netdev.c" is called. This function calls "bnep_net_mc_filter()", which checks (if the destination address is multicast) if the address is set in a certain multicast filter (&s->mc_filter). If it is not, then it is not sent out.
This filter is only changed in two other functions, found in net/bluetooth/bnep/core.c": in "bnep_ctrl_set_mc_filter()", which is only called if a message of type "BNEP_FILTER_MULTI_ADDR_SET" is received. Otherwise, it is set in "bnep_add_connection()", where it is set to a default value which only adds the broadcast address to the filter:
set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
To sum up, if the BNEP interface does not receive any message of type "BNEP_FILTER_MULTI_ADDR_SET", it will not send out any messages with multicast destination addresses except for broadcast.
However, in the BNEP specification (page 27 in
http://grouper.ieee.org/groups/802/15/Bluetooth/BNEP.pdf), it is said that per default, all multicast addresses should not be filtered, i.e. the BNEP interface should be able to send packets with any multicast destination address.

Quoted 6 lines

It seems that the default case is wrong: the multicast filter should not block almost all multicast addresses, but should not filter out any.
This leads to the problem that e.g. Neighbor Solicitation messages sent with Bluetooth PAN over the BNEP interface to a multicast destination address other than broadcast are blocked and not sent out.

and something along these lines should be in the commit message as explanation for your change. And the reference to the specification should be added as an extra comment in the code.

Regards

Marcel

Danny Schweizer

author

2015-12-02 08:07 UTC #4 lore raw

A Linux PC is connected with another device over Bluetooth
PAN using a BNEP interface.

set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);

To sum up, if the BNEP interface does not receive any message of type "BNEP_FILTER_MULTI_ADDR_SET", it will not send out any messages with multicast destination addresses except for broadcast.

It seems that the default case is wrong: the multicast filter should not block almost all multicast addresses, but should not filter out any.

Therefore, in the default case, we set the mc_filter to ~0LL to not filter out any multicast addresses.

---
 net/bluetooth/bnep/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

net/bluetooth/bnep/core.c +0 -0

index 1641367..8e02289 100644
@@ -608,8 +608,12 @@ int bnep_add_connection(struct bnep_connadd_req

*req, struct socket *sock)
s->msg.msg_flags = MSG_NOSIGNAL;

#ifdef CONFIG_BT_BNEP_MC_FILTER
- /* Set default mc filter */
- set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
+ /*
+ * Set default mc filter to not filter out any mc addresses
+ * as defined in the BNEP specification (revision 0.95a)
+ * http://grouper.ieee.org/groups/802/15/Bluetooth/BNEP.pdf
+ */
+ s->mc_filter = ~0LL;
#endif

#ifdef CONFIG_BT_BNEP_PROTO_FILTER

Signature · 1 line

2.1.4

Marcel Holtmann

maintainer

2015-12-05 18:20 UTC #5 lore raw

Hi Danny,

Quoted 34 lines

A Linux PC is connected with another device over Bluetooth PAN using a BNEP interface.
Whenever a packet is tried to be sent over the BNEP interface, the function "bnep_net_xmit()" in "net/bluetooth/bnep/netdev.c" is called. This function calls "bnep_net_mc_filter()", which checks (if the destination address is multicast) if the address is set in a certain multicast filter (&s->mc_filter). If it is not, then it is not sent out.
This filter is only changed in two other functions, found in net/bluetooth/bnep/core.c": in "bnep_ctrl_set_mc_filter()", which is only called if a message of type "BNEP_FILTER_MULTI_ADDR_SET" is received. Otherwise, it is set in "bnep_add_connection()", where it is set to a default value which only adds the broadcast address to the filter:
set_bit(bnep_mc_hash(dev->broadcast), (ulong *) &s->mc_filter);
To sum up, if the BNEP interface does not receive any message of type "BNEP_FILTER_MULTI_ADDR_SET", it will not send out any messages with multicast destination addresses except for broadcast.
However, in the BNEP specification (page 27 in
http://grouper.ieee.org/groups/802/15/Bluetooth/BNEP.pdf), it is said that per default, all multicast addresses should not be filtered, i.e. the BNEP interface should be able to send packets with any multicast destination address.
It seems that the default case is wrong: the multicast filter should not block almost all multicast addresses, but should not filter out any.
This leads to the problem that e.g. Neighbor Solicitation messages sent with Bluetooth PAN over the BNEP interface to a multicast destination address other than broadcast are blocked and not sent out.

Quoted 5 lines

Therefore, in the default case, we set the mc_filter to ~0LL to not filter out any multicast addresses.
---
net/bluetooth/bnep/core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

I can not apply this patch since it comes in weirdly. Please send a clean copy and shorten the subject line so it fits appropriately. And maybe use git send-email.

Regards

Marcel