[PATCH 0/3] rndis_host: handle bogus MAC addresses in ZTE RNDIS devices

[PATCH 0/3] rndis_host: handle bogus MAC addresses in ZTE RNDIS devices

[Lech Perczak]

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When porting support of ZTE MF286R to OpenWrt [1], it was discovered,
that its built-in LTE modem fails to adjust its target MAC address,
when a random MAC address is assigned to the interface, due to detection of
"locally-administered address" bit. This leads to dropping of ingress
trafficat the host. The modem uses RNDIS as its primary interface,
with some variants exposing both of them simultaneously.

Then it was discovered, that cdc_ether driver contains a fixup for that
exact issue, also appearing on CDC ECM interfaces.
I discussed how to proceed with that with Bjørn Mork at OpenWrt forum [3],
with the first approach would be to trust the locally-administered MAC
again, and add a quirk for the problematic ZTE devices, as suggested by
Kristian Evensen. before [4], but reusing the fixup from cdc_ether looks
like a safer and more generic solution.

Finally, according to Bjørn's suggestion. limit the scope of bogus MAC
addressdetection to ZTE devices, the same way as it is done in cdc_ether,
as this trait wasn't really observed outside of ZTE devices.
Do that for both flavours of RNDIS devices, with interface classes
02/02/ff and e0/01/03, as both types are reported by different modems.

[1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=7ac8da00609f42b8aba74b7efc6b0d055b7cef3e
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bfe9b9d2df669a57a95d641ed46eb018e204c6ce
[3] https://forum.openwrt.org/t/problem-with-modem-in-zte-mf286r/120988
[4] https://lore.kernel.org/all/CAKfDRXhDp3heiD75Lat7cr1JmY-kaJ-MS0tt7QXX=s8RFjbpUQ@mail.gmail.com/T/

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>

Lech Perczak (3):
  cdc_ether: export usbnet_cdc_zte_rx_fixup
  rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether
  rndis_host: limit scope of bogus MAC address detection to ZTE devices

 drivers/net/usb/cdc_ether.c  |  3 ++-
 drivers/net/usb/rndis_host.c | 39 ++++++++++++++++++++++++++++++++----
 include/linux/usb/usbnet.h   |  1 +
 3 files changed, 38 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/3] cdc_ether: export usbnet_cdc_zte_rx_fixup

[Lech Perczak]

Commit bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
introduces a workaround for certain ZTE modems reporting invalid MAC
addresses over CDC-ECM.
The same issue was present on their RNDIS interface,which was fixed in
commit a5a18bdf7453 ("rndis_host: Set valid random MAC on buggy devices").

However, internal modem of ZTE MF286R router, on its RNDIS interface, also
exhibits a second issue fixed already in CDC-ECM,of the device not respecting
configured random MAC address. In order to share the fixup for this with
rndis_host driver, export the workaround function, which will be re-used in
the following commit in rndis_host.

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---
 drivers/net/usb/cdc_ether.c | 3 ++-
 include/linux/usb/usbnet.h  | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 9b4dfa3001d6..2de09ad5bac0 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -479,7 +479,7 @@ static int usbnet_cdc_zte_bind(struct usbnet *dev, struct usb_interface *intf)
  * device MAC address has been updated). Always set MAC address to that of the
  * device.
  */
-static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 {
 	if (skb->len < ETH_HLEN || !(skb->data[0] & 0x02))
 		return 1;
@@ -489,6 +489,7 @@ static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(usbnet_cdc_zte_rx_fixup);
 
 /* Ensure correct link state
  *
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8336e86ce606..1b4d72d5e891 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -214,6 +214,7 @@ extern int usbnet_ether_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 extern int usbnet_cdc_bind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_unbind(struct usbnet *, struct usb_interface *);
 extern void usbnet_cdc_status(struct usbnet *, struct urb *);
+extern int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, struct sk_buff *skb);
 
 /* CDC and RNDIS support the same host-chosen packet filters for IN transfers */
 #define	DEFAULT_FILTER	(USB_CDC_PACKET_TYPE_BROADCAST \
-- 
2.30.2

[PATCH 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Lech Perczak]

Certain ZTE modems, namely: MF823. MF831, MF910, built-in modem from
MF286R, expose both CDC-ECM and RNDIS network interfaces.
They have a trait of ignoring the locally-administered MAC address
configured on the interface both in CDC-ECM and RNDIS part,
and this leads to dropping of incoming traffic by the host.
However, the workaround was only present in CDC-ECM, and MF286R
explicitly requires it in RNDIS mode.

Re-use the workaround in rndis_host as well, to fix operation of MF286R
module, some versions of which expose only the RNDIS interface.

Apply the workaround to both "flavors" of RNDIS interfaces, as older ZTE
modems, like MF823 found in the wild, report the USB_CLASS_COMM class
interfaces, while MF286R reports USB_CLASS_WIRELESS_CONTROLLER.

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Cc: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---
 drivers/net/usb/rndis_host.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index 247f58cb0f84..a7eb032115e8 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -578,6 +578,10 @@ rndis_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 }
 EXPORT_SYMBOL_GPL(rndis_tx_fixup);
 
+static int zte_rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	return rndis_rx_fixup(dev, skb) && usbnet_cdc_zte_rx_fixup(dev, skb);
+}
 
 static const struct driver_info	rndis_info = {
 	.description =	"RNDIS device",
@@ -600,6 +604,16 @@ static const struct driver_info	rndis_poll_status_info = {
 	.tx_fixup =	rndis_tx_fixup,
 };
 
+static const struct driver_info	zte_rndis_info = {
+	.description =	"ZTE RNDIS device",
+	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
+	.bind =		rndis_bind,
+	.unbind =	rndis_unbind,
+	.status =	rndis_status,
+	.rx_fixup =	zte_rndis_rx_fixup,
+	.tx_fixup =	rndis_tx_fixup,
+};
+
 /*-------------------------------------------------------------------------*/
 
 static const struct usb_device_id	products [] = {
@@ -613,6 +627,16 @@ static const struct usb_device_id	products [] = {
 	USB_VENDOR_AND_INTERFACE_INFO(0x238b,
 				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
 	.driver_info = (unsigned long)&rndis_info,
+}, {
+	/* ZTE WWAN modules */
+	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
+				      USB_CLASS_WIRELESS_CONTROLLER, 1, 3),
+	.driver_info = (unsigned long)&zte_rndis_info,
+}, {
+	/* ZTE WWAN modules, ACM flavour */
+	USB_VENDOR_AND_INTERFACE_INFO(0x19d2,
+				      USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
+	.driver_info = (unsigned long)&zte_rndis_info,
 }, {
 	/* RNDIS is MSFT's un-official variant of CDC ACM */
 	USB_INTERFACE_INFO(USB_CLASS_COMM, 2 /* ACM */, 0x0ff),
-- 
2.30.2

Re: [PATCH 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Bjørn Mork]

Lech Perczak <lech.perczak@gmail.com> writes:

> +static int zte_rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> +{
> +	return rndis_rx_fixup(dev, skb) && usbnet_cdc_zte_rx_fixup(dev, skb);
> +}
>  

Does this work as expected? Only the last ethernet packet in the rndis
frame will end up being handled by usbnet_cdc_zte_rx_fixup().  The
others are cloned and submitted directly to usbnet_skb_return().

I don't know how to best solve that, but maybe add another
RNDIS_DRIVER_DATA_x flag and test that in rndis_rx_fixup?  I.e something
like

	bool fixup_dst = dev->driver_info->data & RNDIS_DRIVER_DATA_FIXUP_DST:
        ..

		/* try to return all the packets in the batch */
		skb2 = skb_clone(skb, GFP_ATOMIC);
		if (unlikely(!skb2))
			break;
		skb_pull(skb, msg_len - sizeof *hdr);
		skb_trim(skb2, data_len);
                if (fixup_dst)
                	usbnet_cdc_zte_rx_fixup(dev, skb2);
		usbnet_skb_return(dev, skb2);
	}
        if (fixup_dst)
                usbnet_cdc_zte_rx_fixup(dev, skb);

	/* caller will usbnet_skb_return the remaining packet */
	return 1;
}



Bjørn

Re: [PATCH 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Lech Perczak]

Hi Bjørn,

Many thanks you for your review! Answers inline.

W dniu 2022-04-07 o 08:25, Bjørn Mork pisze:
> Lech Perczak <lech.perczak@gmail.com> writes:
>
>> +static int zte_rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>> +{
>> +	return rndis_rx_fixup(dev, skb) && usbnet_cdc_zte_rx_fixup(dev, skb);
>> +}
>>   
> Does this work as expected? Only the last ethernet packet in the rndis
> frame will end up being handled by usbnet_cdc_zte_rx_fixup().  The
> others are cloned and submitted directly to usbnet_skb_return().
I've got some positive reports from at least two owners of the device - 
I don't have one myself. In the meantime asked them to run tests with 
high traffic, because this should most probably manifest itself in that 
scenario easily - my wild guess is that the modem doesn't use batching, 
but you are most certainly right in the general case. And for testing on 
older modems, we can probably only count on Kristian.
>
> I don't know how to best solve that, but maybe add another
> RNDIS_DRIVER_DATA_x flag and test that in rndis_rx_fixup?  I.e something
> like
>
> 	bool fixup_dst = dev->driver_info->data & RNDIS_DRIVER_DATA_FIXUP_DST:
>          ..
>
> 		/* try to return all the packets in the batch */
> 		skb2 = skb_clone(skb, GFP_ATOMIC);
> 		if (unlikely(!skb2))
> 			break;
> 		skb_pull(skb, msg_len - sizeof *hdr);
> 		skb_trim(skb2, data_len);
>                  if (fixup_dst)
>                  	usbnet_cdc_zte_rx_fixup(dev, skb2);
> 		usbnet_skb_return(dev, skb2);
> 	}
>          if (fixup_dst)
>                  usbnet_cdc_zte_rx_fixup(dev, skb);
>
> 	/* caller will usbnet_skb_return the remaining packet */
> 	return 1;
> }

I'll consider that. My concern with that approach is degradation of 
performance by testing for that flag, both for ZTE and non-ZTE devices, 
for each and every packet. But this might be the only solution, as I 
cannot catch the n-1 sk_buffs from the batch mid-flight, only the last 
one. The only other way that currently comes to my mind, is to duplicate 
rndis_rx_fixup, with added calls to usbnet_cdc_zte_rx_fixup in the right 
places. But the amount of duplicated code by doing so would be huge, so 
I'd like to avoid that as well.

I will definitely send a V2 after I decide on a solution and do some 
testing, including high downlink traffic.

>
>
>
> Bjørn

-- 
Pozdrawiam/Kind regards,
Lech Perczak

Re: [PATCH 2/3] rndis_host: enable the bogus MAC fixup for ZTE devices from cdc_ether

[Lech Perczak]

W dniu 2022-04-07 o 22:51, Lech Perczak pisze:
> Hi Bjørn,
>
> Many thanks you for your review! Answers inline.
>
> W dniu 2022-04-07 o 08:25, Bjørn Mork pisze:
>> Lech Perczak <lech.perczak@gmail.com> writes:
>>
>>> +static int zte_rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>>> +{
>>> +    return rndis_rx_fixup(dev, skb) && usbnet_cdc_zte_rx_fixup(dev, 
>>> skb);
>>> +}
>> Does this work as expected? Only the last ethernet packet in the rndis
>> frame will end up being handled by usbnet_cdc_zte_rx_fixup(). The
>> others are cloned and submitted directly to usbnet_skb_return().
> I've got some positive reports from at least two owners of the device 
> - I don't have one myself. In the meantime asked them to run tests 
> with high traffic, because this should most probably manifest itself 
> in that scenario easily - my wild guess is that the modem doesn't use 
> batching, but you are most certainly right in the general case. And 
> for testing on older modems, we can probably only count on Kristian.
>>
>> I don't know how to best solve that, but maybe add another
>> RNDIS_DRIVER_DATA_x flag and test that in rndis_rx_fixup?  I.e something
>> like
>>
>>     bool fixup_dst = dev->driver_info->data & 
>> RNDIS_DRIVER_DATA_FIXUP_DST:
>>          ..
>>
>>         /* try to return all the packets in the batch */
>>         skb2 = skb_clone(skb, GFP_ATOMIC);
>>         if (unlikely(!skb2))
>>             break;
>>         skb_pull(skb, msg_len - sizeof *hdr);
>>         skb_trim(skb2, data_len);
>>                  if (fixup_dst)
>>                      usbnet_cdc_zte_rx_fixup(dev, skb2);
>>         usbnet_skb_return(dev, skb2);
>>     }
>>          if (fixup_dst)
>>                  usbnet_cdc_zte_rx_fixup(dev, skb);
>>
>>     /* caller will usbnet_skb_return the remaining packet */
>>     return 1;
>> }
>
> I'll consider that. My concern with that approach is degradation of 
> performance by testing for that flag, both for ZTE and non-ZTE 
> devices, for each and every packet. But this might be the only 
> solution, as I cannot catch the n-1 sk_buffs from the batch 
> mid-flight, only the last one. The only other way that currently comes 
> to my mind, is to duplicate rndis_rx_fixup, with added calls to 
> usbnet_cdc_zte_rx_fixup in the right places. But the amount of 
> duplicated code by doing so would be huge, so I'd like to avoid that 
> as well.
>
> I will definitely send a V2 after I decide on a solution and do some 
> testing, including high downlink traffic.
>
>>
>>
>>
>> Bjørn
>
Hi Bjørn,

I implemented the fix according to your suggestion and did some testing.
Although I don't have a full MF286R myself, I used my Raspberry Pi 
Zero's USB gadget
to simulate the modem's RNDIS interface, and compared three scenarios:
- generic VID/PID with "locally administered" bit off,
- generic VID/PID with "locally administered" bit on,
- ZTE VID/PID (of MF286R's modem) with "locally administered" bit on.

Of course, only the last one activated the MAC fixup path.

For testing I used one of my modem-less MF286A cross-flashed to MF286R 
using current
OpenWrt master - which are exactly the same hardware, modulo the 
internal modem.
In all three scenarios, when running iperf3 server on the Pi Zero,
I got constant 150Mbps of traffic in both directions, with iperf3 client 
running on the
router itself. When router was uploading data to my "modem", CPU usage 
was around 66%.
When downloading, the total usage would hit 100%, with about 15% 
attributed to syscalls,
and about 85% attributed to softirq.
When using iperf3 client on a PC connected to the router, and enabling 
flow offload,
the softirq load would drop to around 75%, and CPU would idle for the 
rest of time,
in both directions, but the downlink speed would drop to around 125Mbps, 
with upload the
same as if running iperf3 on router itself.

I compared all of this against a build without this patchset, with 
scenario one - getting
exactly  the same performance.
So, suumming up - it seems my concerns about performance were 
exaggerated, so I decided to just
introduce the check inside zte_rndis_fixup(), just as suggested in this 
thread. V2 coming shortly.

One strange quirk I noticed while testing, is that when "locally 
administered" bit in MAC address
was set, the interface would get "usb" prefix on the host side, and 
"eth" otherwise.

-- 
Pozdrawiam/Kind regards,
Lech Perczak

[PATCH 3/3] rndis_host: limit scope of bogus MAC address detection to ZTE devices

[Lech Perczak]

Reporting of bogus MAC addresses and ignoring configuration of new
destination address wasn't observed outside of a range of ZTE devices,
among which this seems to be the common bug. Align rndis_host driver
with implementation found in cdc_ether, which also limits this workaround
to ZTE devices.

Cc: Kristian Evensen <kristian.evensen@gmail.com>
Suggested-by: Bjørn Mork <bjorn@mork.no>
Cc: Oliver Neukum <oliver@neukum.org>
Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
---
 drivers/net/usb/rndis_host.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index a7eb032115e8..55ff0461d39b 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -418,10 +418,7 @@ generic_rndis_bind(struct usbnet *dev, struct usb_interface *intf, int flags)
 		goto halt_fail_and_release;
 	}
 
-	if (bp[0] & 0x02)
-		eth_hw_addr_random(net);
-	else
-		eth_hw_addr_set(net, bp);
+	eth_hw_addr_set(net, bp);
 
 	/* set a nonzero filter to enable data transfers */
 	memset(u.set, 0, sizeof *u.set);
@@ -463,6 +460,16 @@ static int rndis_bind(struct usbnet *dev, struct usb_interface *intf)
 	return generic_rndis_bind(dev, intf, FLAG_RNDIS_PHYM_NOT_WIRELESS);
 }
 
+static int zte_rndis_bind(struct usbnet *dev, struct usb_interface *intf)
+{
+	int status = rndis_bind(dev, intf);
+
+	if (!status && (dev->net->dev_addr[0] & 0x02))
+		eth_hw_addr_random(dev->net);
+
+	return status;
+}
+
 void rndis_unbind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct rndis_halt	*halt;
@@ -607,7 +614,7 @@ static const struct driver_info	rndis_poll_status_info = {
 static const struct driver_info	zte_rndis_info = {
 	.description =	"ZTE RNDIS device",
 	.flags =	FLAG_ETHER | FLAG_POINTTOPOINT | FLAG_FRAMING_RN | FLAG_NO_SETINT,
-	.bind =		rndis_bind,
+	.bind =		zte_rndis_bind,
 	.unbind =	rndis_unbind,
 	.status =	rndis_status,
 	.rx_fixup =	zte_rndis_rx_fixup,
-- 
2.30.2

Re: [PATCH 3/3] rndis_host: limit scope of bogus MAC address detection to ZTE devices

[Bjørn Mork]

Lech Perczak <lech.perczak@gmail.com> writes:

> Reporting of bogus MAC addresses and ignoring configuration of new
> destination address wasn't observed outside of a range of ZTE devices,
> among which this seems to be the common bug. Align rndis_host driver
> with implementation found in cdc_ether, which also limits this workaround
> to ZTE devices.

Reviewed-by: Bjørn Mork <bjorn@mork.no>

Yes, this is a much better solution.

We have no business rejecting the address chosen by the device, even if
it is "locally administered".  The device has every right to use a local
address on a link with no other devices, which is the case for every
cellular modem for example.

And even if we believe the device is wrong there isn't much we can do
about that.

Rejecting the device address, with no way to inform the device about a
new address, implies that host and device disagrees about it.  This does
not fix anything.  It just makes the host to silentlig drop all packets,
leaving the user with a non-working device.

I take full responibility for coming up with the idea of over-
simplifying the original workaround proposed by Kristian. It wasn't very
well thought over.

Thanks to Lech for fixing this!



Bjørn