[RFC] net: usbnet: prevent buggy devices from killing us

[RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

A device sending 0 length frames as fast as it can has been
observed killing the host system due to the resulting memory
pressure. We handle the done queue as fast as we can, so
if this queue is filling up then that is an indication that we
are under too heavy pressure.  Refusing further allocations
until the done queue is handled prevents the buggy device
from taking the system down.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
Hello Oliver,

The MBIM firmware for the Sierra Wireless MC7710 is a nice source
of "interesting" device issues.  One of the uglier ones is that
it under certain conditions will start flooding us with frames
having length 0 as fast as it can.  And that is pretty fast...

My older laptop dies immediately under this.  It just cannot keep
up with the infinite allocations usbnet will do when the done
queue first starts growing beyond reason.

I really do not have a clue how to handle this problem, but this
patch seems to do the job for me without affecting normal devices.
The queue limit is just a number which Works For Me, leaving the
system running with the buggy device and not kicking in under
normal load.

What do you think? Is there some other way this should be solved?



Bjørn

 drivers/net/usb/usbnet.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f34b2eb..85c7ffd 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -380,6 +380,14 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 	unsigned long		lockflags;
 	size_t			size = dev->rx_urb_size;
 
+	/* Do not let a device flood us to death! */
+	if (dev->done.qlen > 1024) {
+		netif_dbg(dev, rx_err, dev->net, "done queue filling up (%u) - throttling\n", dev->done.qlen);
+		usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
+		usb_free_urb (urb);
+		return -ENOMEM;
+	}
+
 	skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
 	if (!skb) {
 		netif_dbg(dev, rx_err, dev->net, "no rx skb\n");
-- 
1.7.2.5

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Thursday 24 January 2013 11:25:52 Bjørn Mork wrote:
> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
> of "interesting" device issues.  One of the uglier ones is that
> it under certain conditions will start flooding us with frames
> having length 0 as fast as it can.  And that is pretty fast...

If you can tell that those frames are bogus, why not count them
as opposed to generic qlen? Say, if you see 20 among the last 30
are of this type, throttle.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:

> A device sending 0 length frames as fast as it can has been
> observed killing the host system due to the resulting memory
> pressure. We handle the done queue as fast as we can, so
> if this queue is filling up then that is an indication that we
> are under too heavy pressure.  Refusing further allocations
> until the done queue is handled prevents the buggy device
> from taking the system down.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
> Hello Oliver,
>
> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
> of "interesting" device issues.  One of the uglier ones is that
> it under certain conditions will start flooding us with frames
> having length 0 as fast as it can.  And that is pretty fast...
>
> My older laptop dies immediately under this.  It just cannot keep
> up with the infinite allocations usbnet will do when the done
> queue first starts growing beyond reason.
>
> I really do not have a clue how to handle this problem, but this
> patch seems to do the job for me without affecting normal devices.
> The queue limit is just a number which Works For Me, leaving the
> system running with the buggy device and not kicking in under
> normal load.
>
> What do you think? Is there some other way this should be solved?


To illustrate the problem, this the start and stop debug output for such
a buggy device session *with* the RFC patch applied:

Jan 24 11:16:23 nemi kernel: [ 3187.624164] qmi_wwan 8-4:1.8 wwan0: open: enable queueing (rx 60, tx 60) mtu 1500 simple framing
Jan 24 11:16:38 nemi kernel: [ 3202.536921] qmi_wwan 8-4:1.8 wwan0: stop stats: rx/tx 1/11, errs 738980/0

I believe the stats tell the full story...

I do not have any logs without the throttling patch, as that takes down
everything on my laptop including the ahci driver and keyboard.  Not
even the magic sysrq is working then.

If anyone is interested in the full debug log (211KB compressed) from
the above session, then I've put it on
http://www.mork.no/~bjorn/usbnet-zero-packet-fix.log.gz


It is mostly full of "rx length 0" lines, but with an occasional
sequence of

Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1025) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1026) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1027) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1028) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1029) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1030) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1031) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1032) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1033) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: rx length 0
Jan 24 11:16:23 nemi kernel: [ 3187.682669] qmi_wwan 8-4:1.8 wwan0: done queue filling up (1034) - throttling
Jan 24 11:16:23 nemi kernel: [ 3187.697826] qmi_wwan 8-4:1.8 wwan0: rxqlen 0 --> 10


showing that the throttling is kicking in and doing its job.



Bjørn

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:

> On Thursday 24 January 2013 11:25:52 Bjørn Mork wrote:
>> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
>> of "interesting" device issues.  One of the uglier ones is that
>> it under certain conditions will start flooding us with frames
>> having length 0 as fast as it can.  And that is pretty fast...
>
> If you can tell that those frames are bogus, why not count them
> as opposed to generic qlen? Say, if you see 20 among the last 30
> are of this type, throttle.

Sounds like a good idea, but where do I add/get that statistics?


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Thursday 24 January 2013 11:52:22 Bjørn Mork wrote:
> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
> 
> > On Thursday 24 January 2013 11:25:52 Bjørn Mork wrote:
> >> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
> >> of "interesting" device issues.  One of the uglier ones is that
> >> it under certain conditions will start flooding us with frames
> >> having length 0 as fast as it can.  And that is pretty fast...
> >
> > If you can tell that those frames are bogus, why not count them
> > as opposed to generic qlen? Say, if you see 20 among the last 30
> > are of this type, throttle.
> 
> Sounds like a good idea, but where do I add/get that statistics?

rx_complete

It need not be very accurate.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:

> On Thursday 24 January 2013 11:52:22 Bjørn Mork wrote:
>> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
>> 
>> > On Thursday 24 January 2013 11:25:52 Bjørn Mork wrote:
>> >> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
>> >> of "interesting" device issues.  One of the uglier ones is that
>> >> it under certain conditions will start flooding us with frames
>> >> having length 0 as fast as it can.  And that is pretty fast...
>> >
>> > If you can tell that those frames are bogus, why not count them
>> > as opposed to generic qlen? Say, if you see 20 among the last 30
>> > are of this type, throttle.
>> 
>> Sounds like a good idea, but where do I add/get that statistics?
>
> rx_complete
>
> It need not be very accurate.

Sorry for being daft, but how do I code the "20 among the last 30" part
there?


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Thursday 24 January 2013 12:22:54 Bjørn Mork wrote:
> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
> 
> > On Thursday 24 January 2013 11:52:22 Bjørn Mork wrote:
> >> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
> >> 
> >> > On Thursday 24 January 2013 11:25:52 Bjørn Mork wrote:
> >> >> The MBIM firmware for the Sierra Wireless MC7710 is a nice source
> >> >> of "interesting" device issues.  One of the uglier ones is that
> >> >> it under certain conditions will start flooding us with frames
> >> >> having length 0 as fast as it can.  And that is pretty fast...
> >> >
> >> > If you can tell that those frames are bogus, why not count them
> >> > as opposed to generic qlen? Say, if you see 20 among the last 30
> >> > are of this type, throttle.
> >> 
> >> Sounds like a good idea, but where do I add/get that statistics?
> >
> > rx_complete
> >
> > It need not be very accurate.
> 
> Sorry for being daft, but how do I code the "20 among the last 30" part
> there?

Just by agreeing that you can live with false negatives but not false positives

if (++counter > 30) {
	counter = bogus = 0;
} else {
	if (is_bogus(packet)
		bogus++;
	if (bogus > counter/2)
		throttle();
}

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Sergei Shtylyov]

Hello.

On 24-01-2013 14:25, Bjørn Mork wrote:

> A device sending 0 length frames as fast as it can has been
> observed killing the host system due to the resulting memory
> pressure. We handle the done queue as fast as we can, so
> if this queue is filling up then that is an indication that we
> are under too heavy pressure.  Refusing further allocations
> until the done queue is handled prevents the buggy device
> from taking the system down.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
[...]

>   drivers/net/usb/usbnet.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)

> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index f34b2eb..85c7ffd 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -380,6 +380,14 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
>   	unsigned long		lockflags;
>   	size_t			size = dev->rx_urb_size;
>
> +	/* Do not let a device flood us to death! */
> +	if (dev->done.qlen > 1024) {
> +		netif_dbg(dev, rx_err, dev->net, "done queue filling up (%u) - throttling\n", dev->done.qlen);
> +		usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
> +		usb_free_urb (urb);

    Run your patch thru scripts/checkpatch.pl please -- spaces before parens 
are not allowed.

WBR, Sergei

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oneukum@suse.de> writes:
> On Thursday 24 January 2013 12:22:54 Bjørn Mork wrote:
>
>> Sorry for being daft, but how do I code the "20 among the last 30" part
>> there?
>
> Just by agreeing that you can live with false negatives but not false positives
>
> if (++counter > 30) {
> 	counter = bogus = 0;
> } else {
> 	if (is_bogus(packet)
> 		bogus++;
> 	if (bogus > counter/2)
> 		throttle();
> }

So, add two new counters to struct usbnet for this?  That seems a little
overkill to me, but I don't see how else to implement anything like that.

It is still not completely clear to me how the throttling/unthrottling
should be done.  It tested with static counters (to avoid having to
rebuild everything for this test) and a new EVENT_RX_THROTTLE flag.
Still on top of my previous patch just for safety while testing, as I am
fed up of having to reboot all the time :-)

Doing the flag test in rx_submit seems simpler than trying to track all
the places this is called.  Still checking the dev->done.qlen to be able
to unthrottle.

Was this along the lines you thought?




diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 85c7ffd..e3a1d63 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -380,6 +382,13 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 	unsigned long		lockflags;
 	size_t			size = dev->rx_urb_size;
 
+
+	if (test_bit(EVENT_RX_THROTTLE, &dev->flags) && dev->done.qlen > 0) {
+		netif_dbg(dev, rx_err, dev->net, "%s: EVENT_RX_THROTTLE: (done.qlen=%u)\n", __func__, dev->done.qlen);
+		usb_free_urb(urb);
+		return -ENOLINK;
+	}
+
 	/* Do not let a device flood us to death! */
 	if (dev->done.qlen > 1024) {
 		netif_dbg(dev, rx_err, dev->net, "done queue filling up (%u) - throttling\n", dev->done.qlen);
@@ -482,6 +491,7 @@ static void rx_complete (struct urb *urb)
 	struct usbnet		*dev = entry->dev;
 	int			urb_status = urb->status;
 	enum skb_state		state;
+	static int counter, bogus;
 
 	skb_put (skb, urb->actual_length);
 	state = rx_done;
@@ -547,6 +557,17 @@ block:
 		break;
 	}
 
+	/* keep track of bogus packet ratio */
+	if (++counter > 30) {
+		counter = bogus = 0;
+		clear_bit(EVENT_RX_THROTTLE, &dev->flags);
+	} else {
+		if (state == rx_cleanup)
+			bogus++;
+		if (bogus > counter/2)
+			set_bit(EVENT_RX_THROTTLE, &dev->flags);
+	}
+
 	state = defer_bh(dev, skb, &dev->rxq, state);
 
 	if (urb) {

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Joe Perches]

On Thu, 2013-01-24 at 16:39 +0400, Sergei Shtylyov wrote:
> On 24-01-2013 14:25, Bjørn Mork wrote:
> > A device sending 0 length frames as fast as it can has been
> > observed killing the host system due to the resulting memory
> > pressure.
[]
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
[]
> > +	/* Do not let a device flood us to death! */
> > +	if (dev->done.qlen > 1024) {
> > +		netif_dbg(dev, rx_err, dev->net, "done queue filling up (%u) - throttling\n", dev->done.qlen);
> > +		usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
> > +		usb_free_urb (urb);
> 
>     Run your patch thru scripts/checkpatch.pl please

And maybe ratelimit the netif_dbg

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Thursday 24 January 2013 13:47:40 Bjørn Mork wrote:
> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
> > On Thursday 24 January 2013 12:22:54 Bjørn Mork wrote:
> >
> >> Sorry for being daft, but how do I code the "20 among the last 30" part
> >> there?
> >
> > Just by agreeing that you can live with false negatives but not false positives
> >
> > if (++counter > 30) {
> > 	counter = bogus = 0;
> > } else {
> > 	if (is_bogus(packet)
> > 		bogus++;
> > 	if (bogus > counter/2)

Should probably be something like bogus > counter/2 + 10

> > 		throttle();
> > }
> 
> So, add two new counters to struct usbnet for this?  That seems a little
> overkill to me, but I don't see how else to implement anything like that.

Memory is cheap.
 
> It is still not completely clear to me how the throttling/unthrottling
> should be done.  It tested with static counters (to avoid having to
> rebuild everything for this test) and a new EVENT_RX_THROTTLE flag.
> Still on top of my previous patch just for safety while testing, as I am
> fed up of having to reboot all the time :-)
> 
> Doing the flag test in rx_submit seems simpler than trying to track all
> the places this is called.  Still checking the dev->done.qlen to be able
> to unthrottle.

Ideally we would do some error handling. Does the device keep spewing
zero packets for all eternity?

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:

> On Thursday 24 January 2013 13:47:40 Bjørn Mork wrote:
>> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> writes:
>> > On Thursday 24 January 2013 12:22:54 Bjørn Mork wrote:
>> >
>> >> Sorry for being daft, but how do I code the "20 among the last 30" part
>> >> there?
>> >
>> > Just by agreeing that you can live with false negatives but not false positives
>> >
>> > if (++counter > 30) {
>> > 	counter = bogus = 0;
>> > } else {
>> > 	if (is_bogus(packet)
>> > 		bogus++;
>> > 	if (bogus > counter/2)
>
> Should probably be something like bogus > counter/2 + 10

right

>> > 		throttle();
>> > }
>> 
>> So, add two new counters to struct usbnet for this?  That seems a little
>> overkill to me, but I don't see how else to implement anything like that.
>
> Memory is cheap.

OK

>> It is still not completely clear to me how the throttling/unthrottling
>> should be done.  It tested with static counters (to avoid having to
>> rebuild everything for this test) and a new EVENT_RX_THROTTLE flag.
>> Still on top of my previous patch just for safety while testing, as I am
>> fed up of having to reboot all the time :-)
>> 
>> Doing the flag test in rx_submit seems simpler than trying to track all
>> the places this is called.  Still checking the dev->done.qlen to be able
>> to unthrottle.
>
> Ideally we would do some error handling. Does the device keep spewing
> zero packets for all eternity?

Yes. The only way to get rid of the bug once it has triggered seems to
be powering the device off/on.  So unthrottling is not important for
this device.  But I guess it will be for other devices with more
temporary problems.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

A device sending 0 length frames as fast as it can has been
observed killing the host system due to the resulting memory
pressure.

Temporarily disable RX skb allocation and URB submission when
the current error ratio is high, preventing us from trying to
allocate an infinite number of skbs.  Reenable as soon as we
are finished processing the done queue, allowing the device
to continue working after short error bursts.

Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
---
So is this starting to look OK?

usbnet already uses "throttle", "halt" and "stop" for other
functions, so I decided to name the new flag "kill".  No other
reason.

Didn't see any point in calculating the error limit.  A fixed
number works just as well.

Restarting in usbnet_bh was a simple way to achieve what I
wanted: enabling RX again when we know we can handle it.


Bjørn

 drivers/net/usb/usbnet.c   |   26 ++++++++++++++++++++++++++
 include/linux/usb/usbnet.h |    2 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f34b2eb..64657d6 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -380,6 +380,12 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 	unsigned long		lockflags;
 	size_t			size = dev->rx_urb_size;
 
+	/* prevent rx skb allocation when error ratio is high */
+	if (test_bit(EVENT_RX_KILL, &dev->flags)) {
+		usb_free_urb(urb);
+		return -ENOLINK;
+	}
+
 	skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
 	if (!skb) {
 		netif_dbg(dev, rx_err, dev->net, "no rx skb\n");
@@ -539,6 +545,22 @@ block:
 		break;
 	}
 
+	/* stop rx if packet error rate is high */
+	if (++dev->pkt_cnt > 30) {
+		dev->pkt_cnt = 0;
+		dev->pkt_err = 0;
+	} else {
+		if (state == rx_cleanup)
+			dev->pkt_err++;
+		if (dev->pkt_err > 20) {
+			set_bit(EVENT_RX_KILL, &dev->flags);
+			if (net_ratelimit())
+				netif_dbg(dev, rx_err, dev->net,
+					  "rx kill: high error rate\n");
+			dev->pkt_err = 0;
+		}
+	}
+
 	state = defer_bh(dev, skb, &dev->rxq, state);
 
 	if (urb) {
@@ -1254,6 +1276,10 @@ static void usbnet_bh (unsigned long param)
 		}
 	}
 
+	/* restart RX again after disabling due to high error rate */
+	if (test_and_clear_bit(EVENT_RX_KILL, &dev->flags) && net_ratelimit())
+		netif_dbg(dev, rx_err, dev->net, "rx kill: restarting\n");
+
 	// waiting for all pending urbs to complete?
 	if (dev->wait) {
 		if ((dev->txq.qlen + dev->rxq.qlen + dev->done.qlen) == 0) {
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 5de7a22..0de078d 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -33,6 +33,7 @@ struct usbnet {
 	wait_queue_head_t	*wait;
 	struct mutex		phy_mutex;
 	unsigned char		suspend_count;
+	unsigned char		pkt_cnt, pkt_err;
 
 	/* i/o info: pipes etc */
 	unsigned		in, out;
@@ -70,6 +71,7 @@ struct usbnet {
 #		define EVENT_DEV_OPEN	7
 #		define EVENT_DEVICE_REPORT_IDLE	8
 #		define EVENT_NO_RUNTIME_PM	9
+#		define EVENT_RX_KILL	10
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Thursday 24 January 2013 20:16:56 Bjørn Mork wrote:
> A device sending 0 length frames as fast as it can has been
> observed killing the host system due to the resulting memory
> pressure.
> 
> Temporarily disable RX skb allocation and URB submission when
> the current error ratio is high, preventing us from trying to
> allocate an infinite number of skbs.  Reenable as soon as we
> are finished processing the done queue, allowing the device
> to continue working after short error bursts.
> 
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
> So is this starting to look OK?

It seems to me that we at least need to try some error recovery.
How about resetting the device when it is no longer used?

	Regards
		Oliver

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Joe Perches]

On Thu, 2013-01-24 at 20:16 +0100, Bjørn Mork wrote:
> A device sending 0 length frames as fast as it can has been
> observed killing the host system due to the resulting memory
> pressure.
[]
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
[]
> @@ -539,6 +545,22 @@ block:
>  		break;
>  	}
>  
> +	/* stop rx if packet error rate is high */
> +	if (++dev->pkt_cnt > 30) {
> +		dev->pkt_cnt = 0;
> +		dev->pkt_err = 0;
> +	} else {
> +		if (state == rx_cleanup)
> +			dev->pkt_err++;
> +		if (dev->pkt_err > 20) {
> +			set_bit(EVENT_RX_KILL, &dev->flags);
> +			if (net_ratelimit())
> +				netif_dbg(dev, rx_err, dev->net,
> +					  "rx kill: high error rate\n");
> +			dev->pkt_err = 0;
> +		}
> +	}

Maybe use ratelimit() here?

> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
[]
> @@ -33,6 +33,7 @@ struct usbnet {
>  	wait_queue_head_t	*wait;
>  	struct mutex		phy_mutex;
>  	unsigned char		suspend_count;
> +	unsigned char		pkt_cnt, pkt_err;

and instead:

	struct ratelimit_state errors;



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oliver@neukum.org> writes:
> On Thursday 24 January 2013 20:16:56 Bjørn Mork wrote:
>> A device sending 0 length frames as fast as it can has been
>> observed killing the host system due to the resulting memory
>> pressure.
>> 
>> Temporarily disable RX skb allocation and URB submission when
>> the current error ratio is high, preventing us from trying to
>> allocate an infinite number of skbs.  Reenable as soon as we
>> are finished processing the done queue, allowing the device
>> to continue working after short error bursts.
>> 
>> Signed-off-by: Bjørn Mork <bjorn@mork.no>
>> ---
>> So is this starting to look OK?
>
> It seems to me that we at least need to try some error recovery.

Won't the disabling code in usbnet_bh do? RX will only stay disabled
until the done queue is handled.

> How about resetting the device when it is no longer used?

Yes, that we should do. I guess usbnet_open is the place to reset the
flag and counters? I'll send another version taking care of this and
Joes comment.


Bjørn

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> writes:

> On Thu, 2013-01-24 at 20:16 +0100, Bjørn Mork wrote:
>> A device sending 0 length frames as fast as it can has been
>> observed killing the host system due to the resulting memory
>> pressure.
> []
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> []
>> @@ -539,6 +545,22 @@ block:
>>  		break;
>>  	}
>>  
>> +	/* stop rx if packet error rate is high */
>> +	if (++dev->pkt_cnt > 30) {
>> +		dev->pkt_cnt = 0;
>> +		dev->pkt_err = 0;
>> +	} else {
>> +		if (state == rx_cleanup)
>> +			dev->pkt_err++;
>> +		if (dev->pkt_err > 20) {
>> +			set_bit(EVENT_RX_KILL, &dev->flags);
>> +			if (net_ratelimit())
>> +				netif_dbg(dev, rx_err, dev->net,
>> +					  "rx kill: high error rate\n");
>> +			dev->pkt_err = 0;
>> +		}
>> +	}
>
> Maybe use ratelimit() here?
>
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> []
>> @@ -33,6 +33,7 @@ struct usbnet {
>>  	wait_queue_head_t	*wait;
>>  	struct mutex		phy_mutex;
>>  	unsigned char		suspend_count;
>> +	unsigned char		pkt_cnt, pkt_err;
>
> and instead:
>
> 	struct ratelimit_state errors;

Thanks.  I took a look at this, but it seems to be more complex than I
really wanted for keeping the debug noise down here.  The rest of usbnet
does not care much about rate limiting debug messages at all.  I'll get
a message for every 0 length packet for example.

Maybe usbnet should get a private debug ratelimiter all over?

Is the problem that these instances will hide more important net
messages?  Would it help to make the ratelimit call depend on whether
debugging is enabled like ath5k and brcm80211 seems to do?



Bjørn

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Oliver Neukum]

On Friday 25 January 2013 08:13:15 Bjørn Mork wrote:
> Oliver Neukum <oliver@neukum.org> writes:
> > On Thursday 24 January 2013 20:16:56 Bjørn Mork wrote:
> >> A device sending 0 length frames as fast as it can has been
> >> observed killing the host system due to the resulting memory
> >> pressure.
> >> 
> >> Temporarily disable RX skb allocation and URB submission when
> >> the current error ratio is high, preventing us from trying to
> >> allocate an infinite number of skbs.  Reenable as soon as we
> >> are finished processing the done queue, allowing the device
> >> to continue working after short error bursts.
> >> 
> >> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> >> ---
> >> So is this starting to look OK?
> >
> > It seems to me that we at least need to try some error recovery.
> 
> Won't the disabling code in usbnet_bh do? RX will only stay disabled
> until the done queue is handled.

So will the burst of bogus packets stop by itself?

> 
> > How about resetting the device when it is no longer used?
> 
> Yes, that we should do. I guess usbnet_open is the place to reset the
> flag and counters? I'll send another version taking care of this and
> Joes comment.

I was thinking about resetting the device, not just counters.
But yes, open() needs to reset the counters, too.

	Regards
		Oliver

Re: [PATCH net] net: usbnet: prevent buggy devices from killing us

[Bjørn Mork]

Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> writes:
> On Friday 25 January 2013 08:13:15 Bjørn Mork wrote:
>> Oliver Neukum <oliver-GvhC2dPhHPQdnm+yROfE0A@public.gmane.org> writes:
>> > On Thursday 24 January 2013 20:16:56 Bjørn Mork wrote:
>> >> A device sending 0 length frames as fast as it can has been
>> >> observed killing the host system due to the resulting memory
>> >> pressure.
>> >> 
>> >> Temporarily disable RX skb allocation and URB submission when
>> >> the current error ratio is high, preventing us from trying to
>> >> allocate an infinite number of skbs.  Reenable as soon as we
>> >> are finished processing the done queue, allowing the device
>> >> to continue working after short error bursts.
>> >> 
>> >> Signed-off-by: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
>> >> ---
>> >> So is this starting to look OK?
>> >
>> > It seems to me that we at least need to try some error recovery.
>> 
>> Won't the disabling code in usbnet_bh do? RX will only stay disabled
>> until the done queue is handled.
>
> So will the burst of bogus packets stop by itself?

No, in the case I am looking at it won't.  So we end up switching this
off/on endlessly.

But I believe that is fine. There is no way we can *know* that the
errors won't stop unless we start receiving packets again.  Other
devices may have similar temporary bugs, making them start working again
after a while. If we permanently disable RX then we will just make any
such device fail for no good reason.

My only wish for this patch is that it makes usbnet survive the buggy
device without bringing the host down.  Not magically fix the device (of
course impossible), or even hide the bug in any way.  A non-functional
device will still appear as a non-functional device. Manual user
intervention is required to make it work.  This might involve a firmware
upgrade for all we know...

>> > How about resetting the device when it is no longer used?
>> 
>> Yes, that we should do. I guess usbnet_open is the place to reset the
>> flag and counters? I'll send another version taking care of this and
>> Joes comment.
>
> I was thinking about resetting the device, not just counters.

What's the point? We only risk making the issue worse if some device has
a similar temporary bug, fixing itself a while after reset.  I think we
should leave any such actions to the user.

> But yes, open() needs to reset the counters, too.

OK, will add that.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html