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

["Bjørn Mork" <bjorn-yOkvZcmFvRU@public.gmane.org>]

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