[PATCH 0/2 ] cdc_ncm: Handle multicast Ethernet traffic

[PATCH 0/2 ] cdc_ncm: Handle multicast Ethernet traffic

[Miguel Rodríguez Pérez]

Sending again, as the previous try had the wrong subjects. Sorry about that.

Dell D6000 dock (and I guess other docks too) exposes a CDC_NCM device
for Ethernet traffic. However, multicast Ethernet traffic is not
processed making IPv6 not functional. Other services, like mDNS used for
LAN service discovery are also hindered.

The actual reason is that CDC_NCM driver was not processing requests to
filter (admit) multicast traffic. I provide two patches to the linux
kernel that admit all Ethernet multicast traffic whenever a multicast
group is being joined.

The solution is not optimal, as it makes the system receive more traffic
than that strictly needed, but otherwise this only happens when the
computer is connected to a dock and thus is running on AC power. I
believe it is not worth the hassle to join only the requested groups.
This is the same that is done in the CDN_ETHER driver.

Best regards,

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

Re: [PATCH 1/2] cdc_ncm: Hook into usbnet_change_mtu respecting usbnet,driver_info

[Miguel Rodríguez Pérez]

Change the way cdc_ncm_change_mtu hooks into the netdev_ops
structure so that changes into usbnet driver_info operations
can be respected. Without this, is was not possible to hook
into usbnet_set_rx_mode.

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
---
 drivers/net/usb/cdc_ncm.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 9e1b74590682..d6b51e2b9495 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -750,16 +750,7 @@ int cdc_ncm_change_mtu(struct net_device *net, int
new_mtu)
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);

-static const struct net_device_ops cdc_ncm_netdev_ops = {
-	.ndo_open	     = usbnet_open,
-	.ndo_stop	     = usbnet_stop,
-	.ndo_start_xmit	     = usbnet_start_xmit,
-	.ndo_tx_timeout	     = usbnet_tx_timeout,
-	.ndo_get_stats64     = usbnet_get_stats64,
-	.ndo_change_mtu	     = cdc_ncm_change_mtu,
-	.ndo_set_mac_address = eth_mac_addr,
-	.ndo_validate_addr   = eth_validate_addr,
-};
+static struct net_device_ops cdc_ncm_netdev_ops;

 int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf,
u8 data_altsetting, int drvflags)
 {
@@ -939,6 +930,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct
usb_interface *intf, u8 data_
 	dev->net->sysfs_groups[0] = &cdc_ncm_sysfs_attr_group;

 	/* must handle MTU changes */
+	cdc_ncm_netdev_ops = *dev->net->netdev_ops;
+	cdc_ncm_netdev_ops.ndo_change_mtu = cdc_ncm_change_mtu;
 	dev->net->netdev_ops = &cdc_ncm_netdev_ops;
 	dev->net->max_mtu = cdc_ncm_max_dgram_size(dev) - cdc_ncm_eth_hlen(dev);

-- 
2.17.1

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

Re: [PATCH 1/2] cdc_ncm: Hook into usbnet_change_mtu respecting usbnet,driver_info

[Bjørn Mork]

Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:

> Change the way cdc_ncm_change_mtu hooks into the netdev_ops
> structure so that changes into usbnet driver_info operations
> can be respected. Without this, is was not possible to hook
> into usbnet_set_rx_mode.

Please export usbnet_set_rx_mode instead, so that cdc_ncm_netdev_ops
can be kept const.


Bjørn

Re: [PATCH 1/2] cdc_ncm: Hook into usbnet_change_mtu respecting usbnet,driver_info

[Miguel Rodríguez Pérez]

On 30/06/18 14:01, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
> 
>> Change the way cdc_ncm_change_mtu hooks into the netdev_ops
>> structure so that changes into usbnet driver_info operations
>> can be respected. Without this, is was not possible to hook
>> into usbnet_set_rx_mode.
> 
> Please export usbnet_set_rx_mode instead, so that cdc_ncm_netdev_ops
> can be kept const.
> 
I don't see how this would help. From my point of view, the problem is
that cdc_ncm driver should not be setting net_device_ops in the first
place, as that is usbnet.c job. Sadly there seems to be no way currently
for cdc_ncm to be notified of mtu changes. This was the least intrusive
way I could think of to respect usbnet's net_device_ops structure while
keeping cdc_ncm notified of mtu changes. Without this change,
set_rx_mode was not getting called for cdc_ncm even after modifying
struct driver_info.
> 
> Bjørn
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

Re: [PATCH 2/2] cdc_ncm: Admit multicast traffic

[Miguel Rodríguez Pérez]

Some CDC_NCM devices are used as docks for laptops. In this case, it
makes sense to accept multicast Ethernet traffic, as these devices
can reside in a proper LAN. Without this, mDNS or IPv6 simply do not
work.

Signed-off-by: Miguel Rodríguez Pérez <miguel@det.uvigo.gal>
---
 drivers/net/usb/cdc_ncm.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index d6b51e2b9495..50af1d9d0102 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -132,6 +132,33 @@ static void cdc_ncm_get_strings(struct net_device
__always_unused *netdev, u32 s

 static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32
new_tx);

+static void cdc_ncm_update_filter(struct usbnet *dev)
+{
+       struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
+	struct net_device *net = dev->net;
+
+	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
+			| USB_CDC_PACKET_TYPE_BROADCAST;
+
+	/* filtering on the device is an optional feature and not worth
+	 * the hassle so we just roughly care about snooping and if any
+	 * multicast is requested, we take every multicast
+	 */
+	if (net->flags & IFF_PROMISC)
+		cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
+	if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
+		cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
+
+	usbnet_write_cmd(dev,
+			USB_CDC_SET_ETHERNET_PACKET_FILTER,
+			USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
+			cdc_filter,
+			iface_no,
+			NULL,
+			0);
+}
+
 static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.get_link          = usbnet_get_link,
 	.nway_reset        = usbnet_nway_reset,
@@ -1652,6 +1679,7 @@ static const struct driver_info cdc_ncm_info = {
 	.status = cdc_ncm_status,
 	.rx_fixup = cdc_ncm_rx_fixup,
 	.tx_fixup = cdc_ncm_tx_fixup,
+	.set_rx_mode = cdc_ncm_update_filter,
 };

 /* Same as cdc_ncm_info, but with FLAG_WWAN */
-- 
2.17.1


On 30/06/18 13:21, Miguel Rodríguez Pérez wrote:
> Sending again, as the previous try had the wrong subjects. Sorry about that.
> 
> Dell D6000 dock (and I guess other docks too) exposes a CDC_NCM device
> for Ethernet traffic. However, multicast Ethernet traffic is not
> processed making IPv6 not functional. Other services, like mDNS used for
> LAN service discovery are also hindered.
> 
> The actual reason is that CDC_NCM driver was not processing requests to
> filter (admit) multicast traffic. I provide two patches to the linux
> kernel that admit all Ethernet multicast traffic whenever a multicast
> group is being joined.
> 
> The solution is not optimal, as it makes the system receive more traffic
> than that strictly needed, but otherwise this only happens when the
> computer is connected to a dock and thus is running on AC power. I
> believe it is not worth the hassle to join only the requested groups.
> This is the same that is done in the CDN_ETHER driver.
> 
> Best regards,
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

Re: [PATCH 2/2] cdc_ncm: Admit multicast traffic

[Bjørn Mork]

Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:

> +static void cdc_ncm_update_filter(struct usbnet *dev)
> +{
> +       struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> +	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
> +	struct net_device *net = dev->net;

Why not change usbnet_cdc_update_filter to get the interface number from
dev->intf instead? Then you can export it and reuse it here instead of
creating a copy.  And it will also work for any other usbnet minidriver.

I.e. change this:

static void usbnet_cdc_update_filter(struct usbnet *dev)
{
	struct cdc_state	*info = (void *) &dev->data;
	struct usb_interface	*intf = info->control;

into:

static void usbnet_cdc_update_filter(struct usbnet *dev)
{
	struct usb_interface	*intf = dev->intf;


or simply use dev->intf to deref directly into an interface number, like
you do in your version.  The local 'intf' variable doesn't do much
good here.



> +
> +	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
> +			| USB_CDC_PACKET_TYPE_BROADCAST;
> +
> +	/* filtering on the device is an optional feature and not worth
> +	 * the hassle so we just roughly care about snooping and if any
> +	 * multicast is requested, we take every multicast
> +	 */
> +	if (net->flags & IFF_PROMISC)
> +		cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
> +	if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
> +		cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
> +
> +	usbnet_write_cmd(dev,

This is a nice change. It should be probably done in
usbnet_cdc_update_filter in any case.  Unless there is some reason it
doesn't alreay use usbnet_write_cmd?

> +			USB_CDC_SET_ETHERNET_PACKET_FILTER,
> +			USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
> +			cdc_filter,
> +			iface_no,
> +			NULL,
> +			0);
> +}
> +
>  static const struct ethtool_ops cdc_ncm_ethtool_ops = {
>  	.get_link          = usbnet_get_link,
>  	.nway_reset        = usbnet_nway_reset,
> @@ -1652,6 +1679,7 @@ static const struct driver_info cdc_ncm_info = {
>  	.status = cdc_ncm_status,
>  	.rx_fixup = cdc_ncm_rx_fixup,
>  	.tx_fixup = cdc_ncm_tx_fixup,
> +	.set_rx_mode = cdc_ncm_update_filter,
>  };
>
>  /* Same as cdc_ncm_info, but with FLAG_WWAN */


As the comment indicates:  There are more 'struct driver_info' variants
here.  Please add .set_rx_mode to all of them, unless you have a reason
not to?


Bjørn

Re: [PATCH 2/2] cdc_ncm: Admit multicast traffic

[Miguel Rodríguez Pérez]

On 30/06/18 14:17, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
> 
>> +static void cdc_ncm_update_filter(struct usbnet *dev)
>> +{
>> +       struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
>> +	u8 iface_no = ctx->control->cur_altsetting->desc.bInterfaceNumber;
>> +	struct net_device *net = dev->net;
> 
> Why not change usbnet_cdc_update_filter to get the interface number from
> dev->intf instead? Then you can export it and reuse it here instead of
> creating a copy.  And it will also work for any other usbnet minidriver.

Well, I just wanted to limit my changes to the minimum, as I lack
experience with kernel development. Where should I declare
usbnet_cdc_update then? Would linux/usb/cdc.h the right place? Is it
enough to do EXPORT_GPL_SYMBOL(usbnet_cdc_update_filter) in cdc_ether.c
for it to be loaded automatically from cdc_ncm.c?
> 
> I.e. change this:
> 
> static void usbnet_cdc_update_filter(struct usbnet *dev)
> {
> 	struct cdc_state	*info = (void *) &dev->data;
> 	struct usb_interface	*intf = info->control;
> 
> into:
> 
> static void usbnet_cdc_update_filter(struct usbnet *dev)
> {
> 	struct usb_interface	*intf = dev->intf;
> 
> 
> or simply use dev->intf to deref directly into an interface number, like
> you do in your version.  The local 'intf' variable doesn't do much
> good here.
> 
Ok, done.
> 
> 
>> +
>> +	u16 cdc_filter = USB_CDC_PACKET_TYPE_DIRECTED
>> +			| USB_CDC_PACKET_TYPE_BROADCAST;
>> +
>> +	/* filtering on the device is an optional feature and not worth
>> +	 * the hassle so we just roughly care about snooping and if any
>> +	 * multicast is requested, we take every multicast
>> +	 */
>> +	if (net->flags & IFF_PROMISC)
>> +		cdc_filter |= USB_CDC_PACKET_TYPE_PROMISCUOUS;
>> +	if (!netdev_mc_empty(net) || (net->flags & IFF_ALLMULTI))
>> +		cdc_filter |= USB_CDC_PACKET_TYPE_ALL_MULTICAST;
>> +
>> +	usbnet_write_cmd(dev,
> 
> This is a nice change. It should be probably done in
> usbnet_cdc_update_filter in any case.  Unless there is some reason it
> doesn't alreay use usbnet_write_cmd?
I don't know of any, so I'll keep it changed.
> 
>> +			USB_CDC_SET_ETHERNET_PACKET_FILTER,
>> +			USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
>> +			cdc_filter,
>> +			iface_no,
>> +			NULL,
>> +			0);
>> +}
>> +
>>  static const struct ethtool_ops cdc_ncm_ethtool_ops = {
>>  	.get_link          = usbnet_get_link,
>>  	.nway_reset        = usbnet_nway_reset,
>> @@ -1652,6 +1679,7 @@ static const struct driver_info cdc_ncm_info = {
>>  	.status = cdc_ncm_status,
>>  	.rx_fixup = cdc_ncm_rx_fixup,
>>  	.tx_fixup = cdc_ncm_tx_fixup,
>> +	.set_rx_mode = cdc_ncm_update_filter,
>>  };
>>
>>  /* Same as cdc_ncm_info, but with FLAG_WWAN */
> 
> 
> As the comment indicates:  There are more 'struct driver_info' variants
> here.  Please add .set_rx_mode to all of them, unless you have a reason
> not to?
I simply lack access to the hardware to test and I didn't want to break
anybody else's hardware. In any case, the change seems quite safe, so I
modified them all too.

I will for your comments before sending the updated version of the patches.
> 
> 
> Bjørn
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo

Re: [PATCH 0/2 ] cdc_ncm: Handle multicast Ethernet traffic

[Bjørn Mork]

Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:

> Sending again, as the previous try had the wrong subjects. Sorry about that.

No problem really, but please use version numbers so it's easy to see
which set is most current.

And you didn't address Olivers comment.  Why not?


Bjørn

Re: [PATCH 0/2 ] cdc_ncm: Handle multicast Ethernet traffic

[Miguel Rodríguez Pérez]

On 30/06/18 14:22, Bjørn Mork wrote:
> Miguel Rodríguez Pérez <miguel@det.uvigo.gal> writes:
> 
>> Sending again, as the previous try had the wrong subjects. Sorry about that.
> 
> No problem really, but please use version numbers so it's easy to see
> which set is most current.
Fine, I'll do so in next revision.
> 
> And you didn't address Olivers comment.  Why not?
Well, I did, but I forgot to format it as plain text and it didn't reach
the least. I have just resent it.
> 
> 
> Bjørn
> 

-- 
Miguel Rodríguez Pérez
Laboratorio de Redes
EE Telecomunicación – Universidade de Vigo