From: David Miller <davem@davemloft.net> Date: 2014-10-10 00:12:52
So, we have a bit of a problem, this is on sparc64:
[423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203] [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624] [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521] [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323] [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128] [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083] [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184] [00000000008243c8] net_rx_action+0xa8/0x1c0
The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.
So any larger than 16-bit access to the IP and later headers will be
unaligned.
We have various ways we can deal with this based upon the capabilities
of the chips involved. Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?
From: Alexander Duyck <hidden> Date: 2014-10-10 03:10:04
On 10/09/2014 05:12 PM, David Miller wrote:
So, we have a bit of a problem, this is on sparc64:
[423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203] [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624] [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521] [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323] [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128] [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083] [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184] [00000000008243c8] net_rx_action+0xa8/0x1c0
The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.
So any larger than 16-bit access to the IP and later headers will be
unaligned.
We have various ways we can deal with this based upon the capabilities
of the chips involved. Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?
The problem is the igb part expects to be able to use 2K buffers which
means it will always try to use the full half of a page. I had
forgotten that the function this replaced had worked with unaligned
accesses as all of the fields I was pulling were only 16b in width. I
think I assumed that this function was already setup to handle that.
Actually the fix should be pretty simple. Just do what we already
appear to be doing for the iph_to_flow_copy_addrs. We can use memcpy to
copy the 4 bytes for the port data instead of doing the direct assignment.
I'll try to submit a patch, just need to see if I have a tree setup as
it has been a couple weeks.
Thanks,
Alex
From: David Miller <davem@davemloft.net> Date: 2014-10-10 04:43:18
From: Alexander Duyck <redacted>
Date: Thu, 09 Oct 2014 20:10:01 -0700
On 10/09/2014 05:12 PM, David Miller wrote:
quoted
So, we have a bit of a problem, this is on sparc64:
[423475.740836] Kernel unaligned access at TPC[81d330]
__skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203] [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624] [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521] [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323] [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128] [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083] [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184] [00000000008243c8] net_rx_action+0xa8/0x1c0
The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.
So any larger than 16-bit access to the IP and later headers will be
unaligned.
We have various ways we can deal with this based upon the capabilities
of the chips involved. Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?
The problem is the igb part expects to be able to use 2K buffers which
means it will always try to use the full half of a page.
Let me try to ask this again.
Can you configure the MAC to put two garbage bytes at the head of
the packet data as it feeds it into the DMA fifos on the IGB chip?
This is an essential (again: _essential_) feature for chips that
manage RX buffers as power-of-2 chunks of pages, as it is the only
cheap way to get the IP headers 32-bit aligned in those power-of-2 DMA
buffer blocks.
That would solve the whole problem.
From: David Laight <hidden> Date: 2014-10-10 11:01:33
From: David Miller
From: Alexander Duyck <redacted>
Date: Thu, 09 Oct 2014 20:10:01 -0700
quoted
On 10/09/2014 05:12 PM, David Miller wrote:
quoted
So, we have a bit of a problem, this is on sparc64:
[423475.740836] Kernel unaligned access at TPC[81d330]
__skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203] [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624] [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521] [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323] [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128] [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083] [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184] [00000000008243c8] net_rx_action+0xa8/0x1c0
The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.
So any larger than 16-bit access to the IP and later headers will be
unaligned.
We have various ways we can deal with this based upon the capabilities
of the chips involved. Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?
The problem is the igb part expects to be able to use 2K buffers which
means it will always try to use the full half of a page.
Let me try to ask this again.
Can you configure the MAC to put two garbage bytes at the head of
the packet data as it feeds it into the DMA fifos on the IGB chip?
This is an essential (again: _essential_) feature for chips that
manage RX buffers as power-of-2 chunks of pages, as it is the only
cheap way to get the IP headers 32-bit aligned in those power-of-2 DMA
buffer blocks.
Not only the IP header, at some point all of the data is likely to
accessed (preferably) as words - if only as a final copy of the
userdata somewhere.
So if you can't efficiently DMA the destination MAC address to a 4n+2
boundary you might as well do a realigning copy of the entire frame
into a separate skb.
Which probably means you should use a different ethernet adapter.
This isn't exactly a new problem.
One of the changes between the Sbus 'DMA' and 'DMA+' parts was that the
latter would do Sbus bursts for transfers (from the lance) that started
misaligned.
David
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <redacted>
---
drivers/net/bonding/bond_main.c | 2 +-
include/net/flow_keys.h | 10 ++++++----
net/core/flow_dissector.c | 21 +++++++++++++--------
3 files changed, 20 insertions(+), 13 deletions(-)
From: David Miller <davem@davemloft.net> Date: 2014-10-10 04:47:09
From: alexander.duyck@gmail.com
Date: Thu, 09 Oct 2014 21:03:28 -0700
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <redacted>
Guess what the compiler will output for the memcpy()....
*(u32 *)dest = *(u32 *)src;
Using memcpy() is never a valid way to avoid misaligned loads and
stores.
If the types have a given alignment, the compiler can legitimately
expand the memcpy() inline with suitably sized loads and stores.
Please see my other reply in the original thread, we have to use
the hardware when we can to manage this situation, by configuring
it to output two garbage bytes before the packet contents when
DMA'ing into power-of-2 aligned blocks of memory.
Thanks.
From: Alexander Duyck <hidden> Date: 2014-10-10 14:43:02
On 10/09/2014 09:47 PM, David Miller wrote:
From: alexander.duyck@gmail.com
Date: Thu, 09 Oct 2014 21:03:28 -0700
quoted
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <redacted>
Guess what the compiler will output for the memcpy()....
*(u32 *)dest = *(u32 *)src;
Using memcpy() is never a valid way to avoid misaligned loads and
stores.
If needed we could convert ports to a __be16 I suppose.
If the types have a given alignment, the compiler can legitimately
expand the memcpy() inline with suitably sized loads and stores.
That is what I get for trying to come up with a fix at the end of the
day. Although it does leave me scratching my head why the IPv4 and IPv6
addresses were not causing unaligned accesses or were they triggering
them as well?
Please see my other reply in the original thread, we have to use
the hardware when we can to manage this situation, by configuring
it to output two garbage bytes before the packet contents when
DMA'ing into power-of-2 aligned blocks of memory.
Thanks.
The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
inserting padding from its side. The whole point of the xxx_get_headlen
functions was to determine the size needed in order to generate the
correct memcpy so that we could generate an aligned header. It sounds
like we can't do that with this function now because there are multiple
instances where the data will be accessed unaligned if it isn't aligned
in the first place.
The way I see it now there are two possible solutions. The first one
being to update the flow hash functions to work with unaligned accesses,
and the second being to split the get_headlen code flow off and for now
use my original code which already had all the alignment issues
resolved. I'm open to suggestions either way, but for now I will try to
just address the items you have pointed out here with the current patch
and submit a v2.
Thanks,
Alex
From: David Laight <hidden> Date: 2014-10-10 14:59:18
From: Alexander Duyck
On 10/09/2014 09:47 PM, David Miller wrote:
quoted
From: alexander.duyck@gmail.com
Date: Thu, 09 Oct 2014 21:03:28 -0700
quoted
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <redacted>
Guess what the compiler will output for the memcpy()....
*(u32 *)dest = *(u32 *)src;
Using memcpy() is never a valid way to avoid misaligned loads and
stores.
If needed we could convert ports to a __be16 I suppose.
You would have to cast it somewhere where the compiler cannot
tell what the original type was.
This usually means you have to call a non-static function, which
might have to be in a different compilation unit.
...
That is what I get for trying to come up with a fix at the end of the
day. Although it does leave me scratching my head why the IPv4 and IPv6
addresses were not causing unaligned accesses or were they triggering
them as well?
I think there is code to copy the IP and TCP headers to aligned memory
before they are parsed.
...
The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
inserting padding from its side...
Shoot the hardware engineers.
You aren't going to get the performance you expect from a 10Ge card
unless the rx buffers are 'correctly' aligned.
David
From: Alexander Duyck <hidden> Date: 2014-10-10 15:14:25
On 10/10/2014 07:57 AM, David Laight wrote:
From: Alexander Duyck
quoted
On 10/09/2014 09:47 PM, David Miller wrote:
quoted
From: alexander.duyck@gmail.com
Date: Thu, 09 Oct 2014 21:03:28 -0700
quoted
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Alexander Duyck <redacted>
Guess what the compiler will output for the memcpy()....
*(u32 *)dest = *(u32 *)src;
Using memcpy() is never a valid way to avoid misaligned loads and
stores.
If needed we could convert ports to a __be16 I suppose.
You would have to cast it somewhere where the compiler cannot
tell what the original type was.
This usually means you have to call a non-static function, which
might have to be in a different compilation unit.
...
The pointer itself gets assigned from skb->data + offset, since
skb->data is an unsigned char I would think that it should be safe to
cast it as a __be16 and have that stick. It is only if we cast it as a
__be32 that it should be an issue as we are casting a value that starts
off only byte aligned.
quoted
That is what I get for trying to come up with a fix at the end of the
day. Although it does leave me scratching my head why the IPv4 and IPv6
addresses were not causing unaligned accesses or were they triggering
them as well?
I think there is code to copy the IP and TCP headers to aligned memory
before they are parsed.
...
The code I am talking about is run before we actually get into the
header parsing. So for example if you take a look at
iph_to_flow_copy_addrs that should be getting called before we even get
to the code that is supposedly triggering this and it is doing 32b
aligned accesses for the source and destination IP addresses.
quoted
The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
inserting padding from its side...
Shoot the hardware engineers.
You aren't going to get the performance you expect from a 10Ge card
unless the rx buffers are 'correctly' aligned.
David
I think you might be coming to this a little late. The igb and ixgbe
drivers had been working this way for a long time. We did a memcpy to
move the headers from the page and into the skb->data at an aligned
offset. In order to determine the length to memcpy we had a function
that could walk through the DMA aligned data to get the header length.
The function for that was replaced with the __skb_flow_dissect as it was
considered a duplication of code with the flow_dissection functions.
However that is obviously not the case now that we are hitting these
alignment issues.
The question I have in all this is do I push forward and make
__skb_flow_dissect work with unaligned accesses, or do I back off and
put something equivilent to igb/ixgbe_get_headlen functions in the
kernel in order to deal with the unaligned accesses as they had no
issues with them since they were only concerned with getting the header
length and kept all accesses 16b aligned.
Thanks,
Alex
From: Eric Dumazet <hidden> Date: 2014-10-10 15:29:35
On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:
I think you might be coming to this a little late. The igb and ixgbe
drivers had been working this way for a long time. We did a memcpy to
move the headers from the page and into the skb->data at an aligned
offset. In order to determine the length to memcpy we had a function
that could walk through the DMA aligned data to get the header length.
The function for that was replaced with the __skb_flow_dissect as it was
considered a duplication of code with the flow_dissection functions.
However that is obviously not the case now that we are hitting these
alignment issues.
The question I have in all this is do I push forward and make
__skb_flow_dissect work with unaligned accesses, or do I back off and
put something equivilent to igb/ixgbe_get_headlen functions in the
kernel in order to deal with the unaligned accesses as they had no
issues with them since they were only concerned with getting the header
length and kept all accesses 16b aligned.
I see nothing wrong dealing with unaligned accesses, as these helpers
are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.
From: Alexander Duyck <hidden> Date: 2014-10-10 16:50:23
On 10/10/2014 08:29 AM, Eric Dumazet wrote:
On Fri, 2014-10-10 at 08:14 -0700, Alexander Duyck wrote:
quoted
I think you might be coming to this a little late. The igb and ixgbe
drivers had been working this way for a long time. We did a memcpy to
move the headers from the page and into the skb->data at an aligned
offset. In order to determine the length to memcpy we had a function
that could walk through the DMA aligned data to get the header length.
The function for that was replaced with the __skb_flow_dissect as it was
considered a duplication of code with the flow_dissection functions.
However that is obviously not the case now that we are hitting these
alignment issues.
The question I have in all this is do I push forward and make
__skb_flow_dissect work with unaligned accesses, or do I back off and
put something equivilent to igb/ixgbe_get_headlen functions in the
kernel in order to deal with the unaligned accesses as they had no
issues with them since they were only concerned with getting the header
length and kept all accesses 16b aligned.
I see nothing wrong dealing with unaligned accesses, as these helpers
are nop on x86 or CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y arches.
Still it means possibly hurting performance on those platforms that
don't have it defined.
If I just use get_unaligned that is pretty easy in terms of cleanup for
the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Thanks,
Alex
From: David Miller <davem@davemloft.net> Date: 2014-10-10 17:58:53
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 09:50:17 -0700
If I just use get_unaligned that is pretty easy in terms of cleanup
for the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Actually, it's not that simple.
When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.
So we might need to sprinkle a "attribute((packed))" here and there
to make it work.
From: Alexander Duyck <hidden> Date: 2014-10-10 18:02:05
On 10/10/2014 10:58 AM, David Miller wrote:
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 09:50:17 -0700
quoted
If I just use get_unaligned that is pretty easy in terms of cleanup
for the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Actually, it's not that simple.
When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.
So we might need to sprinkle a "attribute((packed))" here and there
to make it work.
I'll do some digging. I know I had this working in igb/ixgbe before so
I probably just need to add a few small tweaks to resolve the remaining
issues for v4 of the patch.
Thanks,
Alex
From: David Miller <davem@davemloft.net> Date: 2014-10-10 18:14:02
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 11:02:04 -0700
On 10/10/2014 10:58 AM, David Miller wrote:
quoted
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 09:50:17 -0700
quoted
If I just use get_unaligned that is pretty easy in terms of cleanup
for the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Actually, it's not that simple.
When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.
So we might need to sprinkle a "attribute((packed))" here and there
to make it work.
I'll do some digging. I know I had this working in igb/ixgbe before so
I probably just need to add a few small tweaks to resolve the remaining
issues for v4 of the patch.
Ok, I think v3 + resolving the th->doff thing will get rid of everything.
From: David Miller <davem@davemloft.net> Date: 2014-10-10 18:16:01
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 11:02:04 -0700
On 10/10/2014 10:58 AM, David Miller wrote:
quoted
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 09:50:17 -0700
quoted
If I just use get_unaligned that is pretty easy in terms of cleanup
for the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Actually, it's not that simple.
When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.
So we might need to sprinkle a "attribute((packed))" here and there
to make it work.
I'll do some digging. I know I had this working in igb/ixgbe before so
I probably just need to add a few small tweaks to resolve the remaining
issues for v4 of the patch.
Your original code works because you do things like "byte[12] & 0xf0" to
extract these fields.
From: David Miller <davem@davemloft.net> Date: 2014-10-10 18:22:58
From: David Miller <davem@davemloft.net>
Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)
Your original code works because you do things like "byte[12] & 0xf0" to
extract these fields.
Changing that th->doff sequence to instead be:
const u8 *bp;
u8 buf[13];
bp = __skb_header_pointer(skb, poff, sizeof(buf),
data, hlen, &buf);
if (!bp)
return poff;
poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
break;
on top of your v3 patch works for me.
Please double-check my calculations.
From: Alexander Duyck <hidden> Date: 2014-10-10 18:53:41
On 10/10/2014 11:22 AM, David Miller wrote:
From: David Miller <davem@davemloft.net>
Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)
quoted
Your original code works because you do things like "byte[12] & 0xf0" to
extract these fields.
Changing that th->doff sequence to instead be:
const u8 *bp;
u8 buf[13];
bp = __skb_header_pointer(skb, poff, sizeof(buf),
data, hlen, &buf);
if (!bp)
return poff;
poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
break;
on top of your v3 patch works for me.
Please double-check my calculations.
Any reason why you are grabbing all 13 bytes instead of just the 1 we
care about? Seems like we could just use a u8 buf instead of the array
since we are only grabbing doff.
Thanks,
Alex
From: David Miller <davem@davemloft.net> Date: 2014-10-10 19:32:40
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 11:53:35 -0700
On 10/10/2014 11:22 AM, David Miller wrote:
quoted
From: David Miller <davem@davemloft.net>
Date: Fri, 10 Oct 2014 14:15:59 -0400 (EDT)
quoted
Your original code works because you do things like "byte[12] & 0xf0"
to
extract these fields.
Changing that th->doff sequence to instead be:
const u8 *bp;
u8 buf[13];
bp = __skb_header_pointer(skb, poff, sizeof(buf),
data, hlen, &buf);
if (!bp)
return poff;
poff += max_t(u32, sizeof(struct tcphdr), (bp[12] & 0xf0) >> 2);
break;
on top of your v3 patch works for me.
Please double-check my calculations.
Any reason why you are grabbing all 13 bytes instead of just the 1 we
care about? Seems like we could just use a u8 buf instead of the
array since we are only grabbing doff.
No reason, just a thinko, my brain implemented this as if skb_header_pointer
worked like skb_pull :-/
From: David Laight <hidden> Date: 2014-10-13 08:34:03
From: David Miller
From: Alexander Duyck <redacted>
Date: Fri, 10 Oct 2014 09:50:17 -0700
quoted
If I just use get_unaligned that is pretty easy in terms of cleanup
for the ports and IPv4 addresses, the IPv6 will still be a significant
hurdle to overcome though.
Actually, it's not that simple.
When the compiler sees things like "th->doff" it will load the 32-bit
word that 4-bit field contains and extract the value using shifts and
masking.
So we might need to sprinkle a "attribute((packed))" here and there
to make it work.
Marking a structure 'packed' forces the compiler to generate byte accesses.
It is enough to mark the 32bit members with __attribute__((aligned(2)))
(or use a typedef for u32 that includes that attribute).
Then the compiler will use 16bit accesses for that field.
David
From: David Laight <hidden> Date: 2014-10-10 16:32:32
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: 10 October 2014 16:34
To: David Laight
Cc: 'alexander.h.duyck@redhat.com'; David Miller; alexander.duyck@gmail.com; netdev@vger.kernel.org
Subject: Re: [PATCH] flow-dissector: Fix alignment issue in __skb_flow_get_ports
On Fri, 2014-10-10 at 14:57 +0000, David Laight wrote:
quoted
I think there is code to copy the IP and TCP headers to aligned memory
before they are parsed.
There is no such thing. You are here on netdev list, please read the
code before doing such claims.
I did say 'I think'...
I must be thinking of some similar code somewhere else.
Possibly just the code that ensures the header isn't fragmented.
quoted
quoted
The problem is the igb / ixgbe / fm10k hardware doesn't have a means of
inserting padding from its side...
Shoot the hardware engineers.
You aren't going to get the performance you expect from a 10Ge card
unless the rx buffers are 'correctly' aligned.
That is simply not true on current x86 cpus. They simply dont care at
all.
I was referring to using them on sparc64, not x86.
I know that current intel x86 cpu have support for misaligned 'rep movsd',
but I thought there was still a small cost (maybe one clock) for
single word transfers.
So maybe they care 'just a little bit'.
You cannot blame Intel for other arches.
True, but this does mean that you don't really want to use these adapters
on a system that can't to unaligned accesses.
David
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Signed-off-by: Alexander Duyck <redacted>
---
v2: Changed ports to a __be16 * to prevent optimization for __be32 *
drivers/net/bonding/bond_main.c | 2 +-
include/net/flow_keys.h | 10 ++++++----
net/core/flow_dissector.c | 22 ++++++++++++++--------
3 files changed, 21 insertions(+), 13 deletions(-)
From: Eric Dumazet <hidden> Date: 2014-10-10 15:36:18
On Fri, 2014-10-10 at 07:59 -0700, alexander.duyck@gmail.com wrote:
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Signed-off-by: Alexander Duyck <redacted>
---
I believe you also need to take care of calls to ipv6_addr_hash()
The IPv4 part also needs something in iph_to_flow_copy_addrs(),
otherwise compiler might assume &iph->saddr is word aligned.
From: David Miller <davem@davemloft.net> Date: 2014-10-10 17:55:14
From: Eric Dumazet <redacted>
Date: Fri, 10 Oct 2014 08:36:16 -0700
On Fri, 2014-10-10 at 07:59 -0700, alexander.duyck@gmail.com wrote:
quoted
From: Alexander Duyck <redacted>
This patch addresses a kernel unaligned access bug seen on a sparc64 system
with an igb adapter. Specifically the __skb_flow_get_ports was returning a
be32 pointer which was then having the value directly returned.
In order to keep the handling of the ports consistent with how we were
handling the IPv4 and IPv6 addresses I have instead replaced the assignment
with a memcpy to the flow key ports value. This way it should stay a
memcpy on systems that cannot handle unaligned access, and will likely be
converted to a 32b assignment on the systems that can support it.
Signed-off-by: Alexander Duyck <redacted>
---
I believe you also need to take care of calls to ipv6_addr_hash()
The IPv4 part also needs something in iph_to_flow_copy_addrs(),
otherwise compiler might assume &iph->saddr is word aligned.
Right, I still get the unaligned accesses even with this patch:
[487667.804777] Kernel unaligned access at TPC[81de40] __skb_get_poff+0xa0/0x100
[487667.818767] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #3
[487667.830930] Call Trace:
[487667.835954] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[487667.848276] [000000000042a824] sun4v_do_mna+0x84/0xa0
[487667.858698] [0000000000406cd0] sun4v_mna+0x5c/0x68
[487667.868592] [000000000081de40] __skb_get_poff+0xa0/0x100
[487667.879531] [0000000000843d2c] eth_get_headlen+0x6c/0xa0
[487667.890486] [000000001003ed54] igb_poll+0x8d4/0xf60 [igb]
[487667.901584] [0000000000824428] net_rx_action+0xa8/0x1c0
[487667.912348] [000000000046a1fc] __do_softirq+0xdc/0x2e0
[487667.922932] [000000000042b96c] do_softirq_own_stack+0x2c/0x40
[487667.934751] [000000000046a6b8] irq_exit+0x98/0xc0
[487667.944473] [000000000042b900] handler_irq+0xc0/0x100
[487667.954888] [00000000004208b4] tl0_irq5+0x14/0x20
[487667.964610] [000000000042c0d4] arch_cpu_idle+0x74/0xa0
[487667.975201] [0000000000499cdc] cpu_startup_entry+0x17c/0x2c0
[487667.986843] [0000000000ab69b8] start_kernel+0x408/0x418
[487667.997603] [00000000008c3628] tlb_fixup_done+0x98/0xb0
From: Tom Herbert <hidden> Date: 2014-10-10 18:41:47
On Thu, Oct 9, 2014 at 5:12 PM, David Miller [off-list ref] wrote:
So, we have a bit of a problem, this is on sparc64:
[423475.740836] Kernel unaligned access at TPC[81d330] __skb_flow_get_ports+0x70/0xe0
[423475.755756] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.17.0+ #2
[423475.767854] Call Trace:
[423475.772877] [0000000000433288] kernel_unaligned_trap+0x368/0x5c0
[423475.785203] [000000000042a824] sun4v_do_mna+0x84/0xa0
[423475.795624] [0000000000406cd0] sun4v_mna+0x5c/0x68
[423475.805521] [000000000081d330] __skb_flow_get_ports+0x70/0xe0
[423475.817323] [000000000081d6ac] __skb_flow_dissect+0x1ac/0x460
[423475.829128] [0000000000843c98] eth_get_headlen+0x38/0xa0
[423475.840083] [0000000010064d54] igb_poll+0x8d4/0xf60 [igb]
[423475.851184] [00000000008243c8] net_rx_action+0xa8/0x1c0
The chip DMA's to the beginning of a frag page and (unless timestamps
are enabled) that's where the ethernet header begins.
So any larger than 16-bit access to the IP and later headers will be
unaligned.
Will this also be a problem for GRE packet carrying Ethernet packet?
We have various ways we can deal with this based upon the capabilities
of the chips involved. Can we configure the IGB to put 2 "don't care"
bytes at the beginning of the packet?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html