On Mon, Dec 07, 2009 at 04:24:02PM +0100, Franco Fichtner wrote:
> Hi Neil,
>
> Neil Horman wrote:
>> Update e1000 driver to not allow dma beyond the end of the allocated skb
>> Signed-off-by: Neil Horman <[email protected]>
>>
>>
>> e1000_main.c | 34 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 33 insertions(+), 1 deletion(-)
>>
>>
>> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
>> index 7e855f9..7600deb 100644
>> --- a/drivers/net/e1000/e1000_main.c
>> +++ b/drivers/net/e1000/e1000_main.c
>> @@ -1667,6 +1667,19 @@ int e1000_setup_all_rx_resources(struct e1000_adapter
>> *adapter)
>> return err;
>> }
>> +static inline u32 normalize_rx_len(u32 len)
>> +{
>> + u32 match, last_match;
>> +
>>
> Skip newline and get rid of last_match. Also, there is a whitespace error...
I'll just wash this all out, based on your comments, ben's and Michals, its not
needed..
>> <snip>
> If you modify rx_buffer_len anyway, then get rid of normed_rx_len and
> do a quick
>
> adapter->rx_buffer_len = normalize_rx_len(adapter->rx_buffer_len);
>
> instead. With the modification above, it never fails, so no need to check
> for !normed_rx_len.
>
Agreed, by using roundup_pow_of_two as was previously suggested the extra
variable is no longer needed.
> But I don't really know the context of this change. Is it okay to shorten
> rx_buffer_len here? Why was it not set appropriately as the driver
> expects?
>
We're not shortening, we're rounding up. And yes its both ok, and necessecary.
The problem (from the intial email I sent), was that this driver tells the
hardware that potentially it can dma up to X bytes to a given rx buffer, but it
is possible and likely that we only provide Y bytes to dma to, where X > Y.
This leads to corruption of the skb_shared_info structure.
> Oh, BTW, the default case in the switch statement is stupid and should
> be removed.
>
I agree, its silly, but with the above changes, it becomes needed (or beneficial
again), to catch cases in which roundup_pow_of_two returns a value beyond what
the hardware can handle. I think in this case we should just panic, as
rx_buffer_len should never be larger than 16384.
Thanks all for the comments, heres version two of this patch, taking all
comments into account:
Update e1000 driver to not allow dma beyond the end of the allocated skb
Signed-off-by: Neil Horman <[email protected]>
e1000_main.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 7e855f9..cb16615 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1697,6 +1697,17 @@ static void e1000_setup_rctl(struct e1000_adapter
*adapter)
/* Setup buffer sizes */
rctl &= ~E1000_RCTL_SZ_4096;
rctl |= E1000_RCTL_BSEX;
+
+ /*
+ * We need to normalize the rx_buffer_len here
+ * since the hardware only knows about 7 discrete
+ * frame lengths here. To accomodate that we need
+ * to set the rx length in the hardware to the next highest
+ * size over the rx_buffer_len, then increase rx_buffer_len
+ * to match it, so that we can get a full mtu sized frame
+ */
+ adapter->rx_buffer_len = roundup_pow_of_two(adapter->rx_buffer_len);
+
switch (adapter->rx_buffer_len) {
case E1000_RXBUFFER_256:
rctl |= E1000_RCTL_SZ_256;
@@ -1711,7 +1722,6 @@ static void e1000_setup_rctl(struct e1000_adapter
*adapter)
rctl &= ~E1000_RCTL_BSEX;
break;
case E1000_RXBUFFER_2048:
- default:
rctl |= E1000_RCTL_SZ_2048;
rctl &= ~E1000_RCTL_BSEX;
break;
@@ -1724,6 +1734,9 @@ static void e1000_setup_rctl(struct e1000_adapter
*adapter)
case E1000_RXBUFFER_16384:
rctl |= E1000_RCTL_SZ_16384;
break;
+ default:
+ panic("Bad rx_buffer_len size\n");
+ break;
}
ew32(RCTL, rctl);
------------------------------------------------------------------------------
Join us December 9, 2009 for the Red Hat Virtual Experience,
a free event focused on virtualization and cloud computing.
Attend in-depth sessions from your desk. Your couch. Anywhere.
http://p.sf.net/sfu/redhat-sfdev2dev
_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel
