From: John Lumby <johnlumby@hotmail•com>
To: netdev@vger•kernel.org
Cc: Ben Hutchings <bhutchings@solarflare•com>,
Francois Romieu <romieu@fr•zoreil.com>,
nic_swsd@realtek•com
Subject: Re: r8169 : always copying the rx buffer to new skb
Date: Mon, 18 Apr 2011 17:26:22 -0400 [thread overview]
Message-ID: <4DACAC7E.4070400@hotmail.com> (raw)
In-Reply-To: <1303147676.2857.20.camel@bwh-desktop>
On 04/18/11 13:27, Ben Hutchings wrote:
> At least some variants of the hardware have a bug [...] avoid allocation failures later on (and
> to save memory) the buffers must be copied rather than passed up the
> stack and reallocated.
Yes, I can see that the always-copy method eliminates all possibility
of an allocation failure, but an *occasional* allocation failure does
no harm - the driver just retains ownership of that descriptor and
tries again on the next rx_interrupt. With a rx ring of N buffers,
it would take something like N-(small_number) consecutive allocation
failures to cause a failure to be exposed up to the application.
That's the way the code used to work and the way I've re-patched it to
work and I've verified that on my 8168c by simulating an allocation
failure on 15 out of every 16 rx-Interrupts (unhooking the current skb
and then simply not allocating a new skb and not giving the
corresponding descriptor to the asic) and everything works just fine,
with just a slight drop in throughput (down to 987 Mbits/sec, still
well ahead of the always-copy).
So do we really need to be that concerned about occasional allocation
failure?
And if someone is that concerned, then, with my proposal, they can
leave the rx_copybreak at its default of 16383, when every packet is
copied anyway. (My patch takes a slightly different approach if the
allocation of the new skb fails - current 2.6.39 drops the packet,
I would propose to unhook and retain the descriptor because I can
replenish later - but that is also debatable). Also that's why I
favour making the rx ring size configurable.
On 04/18/11 14:21, Francois Romieu wrote:
> Short answer: it's mostly related to CVE-2009-4537 (see git log).
I understand the need to make the rx_buf_size 16383 to defeat the DOS
attacker, no suggestion to alter that. I'm just not sure I see why
that has to imply the always_copy.
> I may resurrect some alternate fix - i.e. detect corrupted Tx descriptors
> and reset before things gets wrong - but it is not easy to prove it right
> since it may be necessary to tailor it for each member of the 816x / 810x
> family. Some input from Realtek may help though.
>
Yes, more input the better, and especially I recognize that I have
tested only my RTL8168c and maybe other models behave differently.
On 04/18/11 13:27, Ben Hutchings wrote:
>> the number of rx buffers allocated at open should be configurable by
>> module param.
> [...]
>
> No, it should implement the ethtool set_ringparam operation.
>
Ah ok thanks. I'll take a look at that.
John Lumby
next prev parent reply other threads:[~2011-04-18 21:26 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-18 17:08 r8169 : always copying the rx buffer to new skb John Lumby
2011-04-18 17:27 ` Ben Hutchings
2011-04-18 21:26 ` John Lumby [this message]
2011-04-20 19:13 ` Francois Romieu
2011-04-21 3:41 ` John Lumby
2011-04-21 3:52 ` John Lumby
2011-04-27 2:18 ` John Lumby
2011-04-27 3:57 ` Eric Dumazet
2011-04-27 20:35 ` Francois Romieu
2011-04-29 1:55 ` John Lumby
2011-04-29 4:54 ` Eric Dumazet
2011-05-02 19:04 ` Chris Friesen
2011-05-03 11:59 ` hayeswang
2011-04-18 18:21 ` Francois Romieu
-- strict thread matches above, loose matches on Subject: below --
2011-06-27 22:54 John Lumby
2011-06-28 7:55 ` Francois Romieu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DACAC7E.4070400@hotmail.com \
--to=johnlumby@hotmail$(echo .)com \
--cc=bhutchings@solarflare$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nic_swsd@realtek$(echo .)com \
--cc=romieu@fr$(echo .)zoreil.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox