Toby MPCI - L201 cellular modem http hang after random MAC address assignment

Toby MPCI - L201 cellular modem http hang after random MAC address assignment

[Bobby Jones]

Hello net-dev,

I'm diagnosing a problem with the Toby MPCI-L201 cellular modem where
http operations hang. This is reproducible on the most recent kernel
by turning on the rndis_host driver and executing a wget or similar
http command. I found I was able to still ping but not transfer any
data. After bisecting I've found that commit
a5a18bdf7453d505783e40e47ebb84bfdd35f93b introduces this hang.

For reference the patch contents are:

>     rndis_host: Set valid random MAC on buggy devices
>
>     Some devices of the same type all export the same, random MAC address. This
>     behavior has been seen on the ZTE MF910, MF823 and MF831, and there are
>     probably more devices out there. Fix this by generating a valid random MAC
>     address if we read a random MAC from device.
>
>     Also, changed the memcpy() to ether_addr_copy(), as pointed out by
>     checkpatch.
>
>     Suggested-by: Bjørn Mork <bjorn@mork.no>
>     Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index 524a47a28120..4f4f71b2966b 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -428,7 +428,11 @@ generic_rndis_bind(struct usbnet *dev, struct usb_interface *intf, int flags)
>                 dev_err(&intf->dev, "rndis get ethaddr, %d\n", retval);
>                 goto halt_fail_and_release;
>         }
> -       memcpy(net->dev_addr, bp, ETH_ALEN);
> +
> +       if (bp[0] & 0x02)
> +               eth_hw_addr_random(net);
> +       else
> +               ether_addr_copy(net->dev_addr, bp);
>
>         /* set a nonzero filter to enable data transfers */
>         memset(u.set, 0, sizeof *u.set);

I know that there is some internal routing done by the modem firmware,
and I'm assuming that overwriting the MAC address breaks said routing.
Can anyone suggest what a proper fix would be?

Thanks,
Bobby

Re: Toby MPCI - L201 cellular modem http hang after random MAC address assignment

[Bobby Jones]

On Mon, Mar 23, 2020 at 10:21 AM Bobby Jones <rjones@gateworks.com> wrote:
>
> Hello net-dev,
>
> I'm diagnosing a problem with the Toby MPCI-L201 cellular modem where
> http operations hang. This is reproducible on the most recent kernel
> by turning on the rndis_host driver and executing a wget or similar
> http command. I found I was able to still ping but not transfer any
> data. After bisecting I've found that commit
> a5a18bdf7453d505783e40e47ebb84bfdd35f93b introduces this hang.
>
> For reference the patch contents are:
>
> >     rndis_host: Set valid random MAC on buggy devices
> >
> >     Some devices of the same type all export the same, random MAC address. This
> >     behavior has been seen on the ZTE MF910, MF823 and MF831, and there are
> >     probably more devices out there. Fix this by generating a valid random MAC
> >     address if we read a random MAC from device.
> >
> >     Also, changed the memcpy() to ether_addr_copy(), as pointed out by
> >     checkpatch.
> >
> >     Suggested-by: Bjørn Mork <bjorn@mork.no>
> >     Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> > index 524a47a28120..4f4f71b2966b 100644
> > --- a/drivers/net/usb/rndis_host.c
> > +++ b/drivers/net/usb/rndis_host.c
> > @@ -428,7 +428,11 @@ generic_rndis_bind(struct usbnet *dev, struct usb_interface *intf, int flags)
> >                 dev_err(&intf->dev, "rndis get ethaddr, %d\n", retval);
> >                 goto halt_fail_and_release;
> >         }
> > -       memcpy(net->dev_addr, bp, ETH_ALEN);
> > +
> > +       if (bp[0] & 0x02)
> > +               eth_hw_addr_random(net);
> > +       else
> > +               ether_addr_copy(net->dev_addr, bp);
> >
> >         /* set a nonzero filter to enable data transfers */
> >         memset(u.set, 0, sizeof *u.set);
>
> I know that there is some internal routing done by the modem firmware,
> and I'm assuming that overwriting the MAC address breaks said routing.
> Can anyone suggest what a proper fix would be?
>
> Thanks,
> Bobby

Adding some individuals and lists in hopes for feedback

Re: Toby MPCI - L201 cellular modem http hang after random MAC address assignment

[Bjørn Mork]

Bobby Jones <rjones@gateworks.com> writes:

>> --- a/drivers/net/usb/rndis_host.c
>> +++ b/drivers/net/usb/rndis_host.c
>> @@ -428,7 +428,11 @@ generic_rndis_bind(struct usbnet *dev, struct usb_interface *intf, int flags)
>>                 dev_err(&intf->dev, "rndis get ethaddr, %d\n", retval);
>>                 goto halt_fail_and_release;
>>         }
>> -       memcpy(net->dev_addr, bp, ETH_ALEN);
>> +
>> +       if (bp[0] & 0x02)
>> +               eth_hw_addr_random(net);
>> +       else
>> +               ether_addr_copy(net->dev_addr, bp);
>>
>>         /* set a nonzero filter to enable data transfers */
>>         memset(u.set, 0, sizeof *u.set);
>
> I know that there is some internal routing done by the modem firmware,
> and I'm assuming that overwriting the MAC address breaks said routing.
> Can anyone suggest what a proper fix would be?

That change should not break anything that wasn't already broken, given
that the driver allows userspace to set any valid ethernet address.

But I'm sure you're right that the modem doesn't like it. Since the
patch also fixed a real problem, then I guess we need to add a device
specific workaround for this modem.

I know exactly nothing about RNDIS and have no such device. But looking
at this now I wonder if the driver should have informed the device about
any adress changes?  I note that we have an
RNDIS_OID_802_3_CURRENT_ADDRESS, which is unused by the usb host driver.

Looking at the rndis gadget driver, I also note that it would refuse
setting RNDIS_OID_802_3_CURRENT_ADDRESS, Which probably means that any
Linux based device would fail if we tried that.  So it's probably not
a good idea as a workaround even if it helped your case. Which is very
unlikely after all. Don't even know if that's how it was supposed to
be.

Oh well.  Just add a device specific exception. Look at the
RNDIS_DRIVER_DATA_POLL_STATUS thing and do something similar for this
case, avoiding the unwanted address update


Bjørn