* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
@ 2015-11-17 0:26 Mark D Rustad
2015-11-17 5:21 ` Tom Herbert
0 siblings, 1 reply; 10+ messages in thread
From: Mark D Rustad @ 2015-11-17 0:26 UTC (permalink / raw)
To: intel-wired-lan
Check for and handle IPv6 extended headers so that tx checksum
offload can be done. Thanks to Tom Herbert for noticing this
problem. Thanks to Alexander Duyck for recognizing problems with
the first version of this patch.
Reported-by: Tom Herbert <tom@herbertland.com>
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
---
Changed in V2:
- Use ipv6_skip_exthdr instead of ipv6_find_hdr
- Move code into L3 processing and avoid any possible loop
- Handle encapsulated IPv6 extended headers correctly
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 1ffbe85eab7b..23f316a9ad5a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
struct tcphdr *tcphdr;
u8 *raw;
} transport_hdr;
+ __be16 frag_off;
+ int rc;
if (skb->encapsulation) {
network_hdr.raw = skb_inner_network_header(skb);
@@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
case 6:
vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
l4_hdr = network_hdr.ipv6->nexthdr;
+ if (likely((transport_hdr.raw - network_hdr.raw) ==
+ sizeof(struct ipv6hdr)))
+ break;
+ rc = ipv6_skip_exthdr(skb, network_hdr.raw +
+ sizeof(struct ipv6hdr) -
+ skb->data,
+ &l4_hdr, &frag_off);
+ if (rc < 0) {
+ if (unlikely(net_ratelimit())) {
+ dev_warn(tx_ring->dev,
+ "ipv6_skip_exthdr returned %d\n",
+ rc);
+ }
+ break;
+ }
+ if (frag_off)
+ l4_hdr = 0;
break;
default:
if (unlikely(net_ratelimit())) {
^ permalink raw reply related [flat|nested] 10+ messages in thread* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 0:26 [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path Mark D Rustad
@ 2015-11-17 5:21 ` Tom Herbert
2015-11-17 16:41 ` Alexander Duyck
0 siblings, 1 reply; 10+ messages in thread
From: Tom Herbert @ 2015-11-17 5:21 UTC (permalink / raw)
To: intel-wired-lan
On Mon, Nov 16, 2015 at 4:26 PM, Mark D Rustad <mark.d.rustad@intel.com> wrote:
> Check for and handle IPv6 extended headers so that tx checksum
> offload can be done. Thanks to Tom Herbert for noticing this
> problem. Thanks to Alexander Duyck for recognizing problems with
> the first version of this patch.
>
> Reported-by: Tom Herbert <tom@herbertland.com>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> ---
> Changed in V2:
> - Use ipv6_skip_exthdr instead of ipv6_find_hdr
> - Move code into L3 processing and avoid any possible loop
> - Handle encapsulated IPv6 extended headers correctly
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 1ffbe85eab7b..23f316a9ad5a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
> struct tcphdr *tcphdr;
> u8 *raw;
> } transport_hdr;
> + __be16 frag_off;
> + int rc;
>
> if (skb->encapsulation) {
> network_hdr.raw = skb_inner_network_header(skb);
> @@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
> case 6:
> vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
> l4_hdr = network_hdr.ipv6->nexthdr;
> + if (likely((transport_hdr.raw - network_hdr.raw) ==
> + sizeof(struct ipv6hdr)))
> + break;
> + rc = ipv6_skip_exthdr(skb, network_hdr.raw +
> + sizeof(struct ipv6hdr) -
> + skb->data,
> + &l4_hdr, &frag_off);
> + if (rc < 0) {
> + if (unlikely(net_ratelimit())) {
> + dev_warn(tx_ring->dev,
> + "ipv6_skip_exthdr returned %d\n",
> + rc);
> + }
> + break;
> + }
> + if (frag_off)
> + l4_hdr = 0;
> break;
> default:
> if (unlikely(net_ratelimit())) {
>
Failures should not result in dev_warn, just call skb_checksum_help
instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
checksum!
The patch parses the extension headers and verifies the protocol is
supported (TCP, UDP, SCTP). But how does the device know the offset of
the transport header to perform the checksum? Is the device capable of
parsing the extension headers on transmit?
Thanks,
Tom
^ permalink raw reply [flat|nested] 10+ messages in thread* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 5:21 ` Tom Herbert
@ 2015-11-17 16:41 ` Alexander Duyck
2015-11-17 16:48 ` Tom Herbert
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Duyck @ 2015-11-17 16:41 UTC (permalink / raw)
To: intel-wired-lan
On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, Nov 16, 2015 at 4:26 PM, Mark D Rustad <mark.d.rustad@intel.com> wrote:
>> Check for and handle IPv6 extended headers so that tx checksum
>> offload can be done. Thanks to Tom Herbert for noticing this
>> problem. Thanks to Alexander Duyck for recognizing problems with
>> the first version of this patch.
>>
>> Reported-by: Tom Herbert <tom@herbertland.com>
>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> ---
>> Changed in V2:
>> - Use ipv6_skip_exthdr instead of ipv6_find_hdr
>> - Move code into L3 processing and avoid any possible loop
>> - Handle encapsulated IPv6 extended headers correctly
>> ---
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 1ffbe85eab7b..23f316a9ad5a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>> struct tcphdr *tcphdr;
>> u8 *raw;
>> } transport_hdr;
>> + __be16 frag_off;
>> + int rc;
>>
>> if (skb->encapsulation) {
>> network_hdr.raw = skb_inner_network_header(skb);
>> @@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>> case 6:
>> vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
>> l4_hdr = network_hdr.ipv6->nexthdr;
>> + if (likely((transport_hdr.raw - network_hdr.raw) ==
>> + sizeof(struct ipv6hdr)))
>> + break;
>> + rc = ipv6_skip_exthdr(skb, network_hdr.raw +
>> + sizeof(struct ipv6hdr) -
>> + skb->data,
>> + &l4_hdr, &frag_off);
>> + if (rc < 0) {
>> + if (unlikely(net_ratelimit())) {
>> + dev_warn(tx_ring->dev,
>> + "ipv6_skip_exthdr returned %d\n",
>> + rc);
>> + }
>> + break;
>> + }
>> + if (frag_off)
>> + l4_hdr = 0;
>> break;
>> default:
>> if (unlikely(net_ratelimit())) {
>>
>
> Failures should not result in dev_warn, just call skb_checksum_help
> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
> checksum!
The problem is there are scenarios here where a checksum shouldn't be
computed. If for example the frame is fragmented we should be
displaying some sort of error message and not computing the checksum
because no matter what we compute it isn't going to be valid.
However I can see your point. At least then this driver would be
forward compatible in the event of an offload request for something
that isn't supported by the hardware.
> The patch parses the extension headers and verifies the protocol is
> supported (TCP, UDP, SCTP). But how does the device know the offset of
> the transport header to perform the checksum? Is the device capable of
> parsing the extension headers on transmit?
The device doesn't need to parse the extension headers. All the
device needs is a pointer to the IPv6 header and the L4 header in
order to compute and place the L4 checksum. That is the data that is
being placed in the descriptor by the tx_csum function.
- Alex
^ permalink raw reply [flat|nested] 10+ messages in thread* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 16:41 ` Alexander Duyck
@ 2015-11-17 16:48 ` Tom Herbert
2015-11-17 17:01 ` Alexander Duyck
0 siblings, 1 reply; 10+ messages in thread
From: Tom Herbert @ 2015-11-17 16:48 UTC (permalink / raw)
To: intel-wired-lan
On Tue, Nov 17, 2015 at 8:41 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Mon, Nov 16, 2015 at 4:26 PM, Mark D Rustad <mark.d.rustad@intel.com> wrote:
>>> Check for and handle IPv6 extended headers so that tx checksum
>>> offload can be done. Thanks to Tom Herbert for noticing this
>>> problem. Thanks to Alexander Duyck for recognizing problems with
>>> the first version of this patch.
>>>
>>> Reported-by: Tom Herbert <tom@herbertland.com>
>>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>>> ---
>>> Changed in V2:
>>> - Use ipv6_skip_exthdr instead of ipv6_find_hdr
>>> - Move code into L3 processing and avoid any possible loop
>>> - Handle encapsulated IPv6 extended headers correctly
>>> ---
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19 +++++++++++++++++++
>>> 1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 1ffbe85eab7b..23f316a9ad5a 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>> struct tcphdr *tcphdr;
>>> u8 *raw;
>>> } transport_hdr;
>>> + __be16 frag_off;
>>> + int rc;
>>>
>>> if (skb->encapsulation) {
>>> network_hdr.raw = skb_inner_network_header(skb);
>>> @@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>> case 6:
>>> vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
>>> l4_hdr = network_hdr.ipv6->nexthdr;
>>> + if (likely((transport_hdr.raw - network_hdr.raw) ==
>>> + sizeof(struct ipv6hdr)))
>>> + break;
>>> + rc = ipv6_skip_exthdr(skb, network_hdr.raw +
>>> + sizeof(struct ipv6hdr) -
>>> + skb->data,
>>> + &l4_hdr, &frag_off);
>>> + if (rc < 0) {
>>> + if (unlikely(net_ratelimit())) {
>>> + dev_warn(tx_ring->dev,
>>> + "ipv6_skip_exthdr returned %d\n",
>>> + rc);
>>> + }
>>> + break;
>>> + }
>>> + if (frag_off)
>>> + l4_hdr = 0;
>>> break;
>>> default:
>>> if (unlikely(net_ratelimit())) {
>>>
>>
>> Failures should not result in dev_warn, just call skb_checksum_help
>> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
>> checksum!
>
> The problem is there are scenarios here where a checksum shouldn't be
> computed. If for example the frame is fragmented we should be
> displaying some sort of error message and not computing the checksum
> because no matter what we compute it isn't going to be valid.
>
If the stack is sending fragments with CHECKSUM_PARTIAL set then that
is a bug higher in the stack-- the driver does not need to worry about
that.
^ permalink raw reply [flat|nested] 10+ messages in thread* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 16:48 ` Tom Herbert
@ 2015-11-17 17:01 ` Alexander Duyck
2015-11-17 18:09 ` Rustad, Mark D
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Duyck @ 2015-11-17 17:01 UTC (permalink / raw)
To: intel-wired-lan
On Tue, Nov 17, 2015 at 8:48 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Nov 17, 2015 at 8:41 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Mon, Nov 16, 2015 at 4:26 PM, Mark D Rustad <mark.d.rustad@intel.com> wrote:
>>>> Check for and handle IPv6 extended headers so that tx checksum
>>>> offload can be done. Thanks to Tom Herbert for noticing this
>>>> problem. Thanks to Alexander Duyck for recognizing problems with
>>>> the first version of this patch.
>>>>
>>>> Reported-by: Tom Herbert <tom@herbertland.com>
>>>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>>>> ---
>>>> Changed in V2:
>>>> - Use ipv6_skip_exthdr instead of ipv6_find_hdr
>>>> - Move code into L3 processing and avoid any possible loop
>>>> - Handle encapsulated IPv6 extended headers correctly
>>>> ---
>>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 19 +++++++++++++++++++
>>>> 1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 1ffbe85eab7b..23f316a9ad5a 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -7229,6 +7229,8 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>>> struct tcphdr *tcphdr;
>>>> u8 *raw;
>>>> } transport_hdr;
>>>> + __be16 frag_off;
>>>> + int rc;
>>>>
>>>> if (skb->encapsulation) {
>>>> network_hdr.raw = skb_inner_network_header(skb);
>>>> @@ -7252,6 +7254,23 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring,
>>>> case 6:
>>>> vlan_macip_lens |= transport_hdr.raw - network_hdr.raw;
>>>> l4_hdr = network_hdr.ipv6->nexthdr;
>>>> + if (likely((transport_hdr.raw - network_hdr.raw) ==
>>>> + sizeof(struct ipv6hdr)))
>>>> + break;
>>>> + rc = ipv6_skip_exthdr(skb, network_hdr.raw +
>>>> + sizeof(struct ipv6hdr) -
>>>> + skb->data,
>>>> + &l4_hdr, &frag_off);
>>>> + if (rc < 0) {
>>>> + if (unlikely(net_ratelimit())) {
>>>> + dev_warn(tx_ring->dev,
>>>> + "ipv6_skip_exthdr returned %d\n",
>>>> + rc);
>>>> + }
>>>> + break;
>>>> + }
>>>> + if (frag_off)
>>>> + l4_hdr = 0;
>>>> break;
>>>> default:
>>>> if (unlikely(net_ratelimit())) {
>>>>
>>>
>>> Failures should not result in dev_warn, just call skb_checksum_help
>>> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
>>> checksum!
>>
>> The problem is there are scenarios here where a checksum shouldn't be
>> computed. If for example the frame is fragmented we should be
>> displaying some sort of error message and not computing the checksum
>> because no matter what we compute it isn't going to be valid.
>>
> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
> is a bug higher in the stack-- the driver does not need to worry about
> that.
Right, but it is still something that needs to get fixed. By
triggering the dev_warn here we at least create visibility that
something has gone horribly wrong.
- Alex
^ permalink raw reply [flat|nested] 10+ messages in thread* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 17:01 ` Alexander Duyck
@ 2015-11-17 18:09 ` Rustad, Mark D
2015-11-17 18:22 ` Tom Herbert
0 siblings, 1 reply; 10+ messages in thread
From: Rustad, Mark D @ 2015-11-17 18:09 UTC (permalink / raw)
To: intel-wired-lan
Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Tue, Nov 17, 2015 at 8:48 AM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Nov 17, 2015 at 8:41 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>
>>>> Failures should not result in dev_warn, just call skb_checksum_help
>>>> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
>>>> checksum!
But calling skb_checksum_help I think is a false sense of security. In the case of fragmentation for example, it can't possibly help. I'd rather find out that it is failing and figure out what to do about it.
>>> The problem is there are scenarios here where a checksum shouldn't be
>>> computed. If for example the frame is fragmented we should be
>>> displaying some sort of error message and not computing the checksum
>>> because no matter what we compute it isn't going to be valid.
>> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
>> is a bug higher in the stack-- the driver does not need to worry about
>> that.
>
> Right, but it is still something that needs to get fixed. By
> triggering the dev_warn here we at least create visibility that
> something has gone horribly wrong.
Right. It needs to be fixed one way or another. In some cases it would be the stack that needs a fix, in some cases a call to skb_checksum_help might be needed or maybe something else.
--
Mark Rustad, Networking Division, Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151117/a6ec251f/attachment-0001.asc>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 18:09 ` Rustad, Mark D
@ 2015-11-17 18:22 ` Tom Herbert
2015-11-17 19:02 ` Rustad, Mark D
0 siblings, 1 reply; 10+ messages in thread
From: Tom Herbert @ 2015-11-17 18:22 UTC (permalink / raw)
To: intel-wired-lan
On Tue, Nov 17, 2015 at 10:09 AM, Rustad, Mark D
<mark.d.rustad@intel.com> wrote:
> Alexander Duyck <alexander.duyck@gmail.com> wrote:
>
>> On Tue, Nov 17, 2015 at 8:48 AM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Tue, Nov 17, 2015 at 8:41 AM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Mon, Nov 16, 2015 at 9:21 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>>
>>>>> Failures should not result in dev_warn, just call skb_checksum_help
>>>>> instead. Ignoring CHECKSUM_PARTIAL is guaranteed to be sending a bad
>>>>> checksum!
>
> But calling skb_checksum_help I think is a false sense of security. In the case of fragmentation for example, it can't possibly help. I'd rather find out that it is failing and figure out what to do about it.
The _only_ thing that is authoritative in checksum offload are the
values of csum_start and csum_offset. If a driver ensures that
checksum is properly calculated from start and set at the offset, then
driver is implementing correct behavior and has no additional
requirements as far as the stack is concerned. Neither does the stack
does not care if this is done with offload to the device or by calling
skb_checksum_help.
>
>>>> The problem is there are scenarios here where a checksum shouldn't be
>>>> computed. If for example the frame is fragmented we should be
>>>> displaying some sort of error message and not computing the checksum
>>>> because no matter what we compute it isn't going to be valid.
>>> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
>>> is a bug higher in the stack-- the driver does not need to worry about
>>> that.
>>
>> Right, but it is still something that needs to get fixed. By
>> triggering the dev_warn here we at least create visibility that
>> something has gone horribly wrong.
>
> Right. It needs to be fixed one way or another. In some cases it would be the stack that needs a fix, in some cases a call to skb_checksum_help might be needed or maybe something else.
Are you actually seeing fragments with CHECKSUM_PARTIAL set?
Tom
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 18:22 ` Tom Herbert
@ 2015-11-17 19:02 ` Rustad, Mark D
2015-11-17 19:11 ` Tom Herbert
0 siblings, 1 reply; 10+ messages in thread
From: Rustad, Mark D @ 2015-11-17 19:02 UTC (permalink / raw)
To: intel-wired-lan
Tom Herbert <tom@herbertland.com> wrote:
> The _only_ thing that is authoritative in checksum offload are the
> values of csum_start and csum_offset. If a driver ensures that
> checksum is properly calculated from start and set at the offset, then
> driver is implementing correct behavior and has no additional
> requirements as far as the stack is concerned. Neither does the stack
> does not care if this is done with offload to the device or by calling
> skb_checksum_help.
>
>>>>> The problem is there are scenarios here where a checksum shouldn't be
>>>>> computed. If for example the frame is fragmented we should be
>>>>> displaying some sort of error message and not computing the checksum
>>>>> because no matter what we compute it isn't going to be valid.
>>>> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
>>>> is a bug higher in the stack-- the driver does not need to worry about
>>>> that.
>>>
>>> Right, but it is still something that needs to get fixed. By
>>> triggering the dev_warn here we at least create visibility that
>>> something has gone horribly wrong.
>>
>> Right. It needs to be fixed one way or another. In some cases it would be the stack that needs a fix, in some cases a call to skb_checksum_help might be needed or maybe something else.
>
> Are you actually seeing fragments with CHECKSUM_PARTIAL set?
I haven't seen any yet and I don't expect to. But the dev_warn that was in this area prior to the current patch was triggered when I first sent IPv6 packets with extended headers. Yes, the checksum was not computed, but there also was a warning in the log. Likewise when I didn't have the encapsulation working for this case, when I tried it I got a warning in the log. So it does indicate when things are broken and so is useful. Of course these were fixed by the driver patch because they were the result of deficiencies in the driver, which you first spotted by code inspection.
--
Mark Rustad, Networking Division, Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151117/9596ceba/attachment.asc>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 19:02 ` Rustad, Mark D
@ 2015-11-17 19:11 ` Tom Herbert
2015-11-17 19:29 ` Rustad, Mark D
0 siblings, 1 reply; 10+ messages in thread
From: Tom Herbert @ 2015-11-17 19:11 UTC (permalink / raw)
To: intel-wired-lan
On Tue, Nov 17, 2015 at 11:02 AM, Rustad, Mark D
<mark.d.rustad@intel.com> wrote:
> Tom Herbert <tom@herbertland.com> wrote:
>
>> The _only_ thing that is authoritative in checksum offload are the
>> values of csum_start and csum_offset. If a driver ensures that
>> checksum is properly calculated from start and set at the offset, then
>> driver is implementing correct behavior and has no additional
>> requirements as far as the stack is concerned. Neither does the stack
>> does not care if this is done with offload to the device or by calling
>> skb_checksum_help.
>>
>>>>>> The problem is there are scenarios here where a checksum shouldn't be
>>>>>> computed. If for example the frame is fragmented we should be
>>>>>> displaying some sort of error message and not computing the checksum
>>>>>> because no matter what we compute it isn't going to be valid.
>>>>> If the stack is sending fragments with CHECKSUM_PARTIAL set then that
>>>>> is a bug higher in the stack-- the driver does not need to worry about
>>>>> that.
>>>>
>>>> Right, but it is still something that needs to get fixed. By
>>>> triggering the dev_warn here we at least create visibility that
>>>> something has gone horribly wrong.
>>>
>>> Right. It needs to be fixed one way or another. In some cases it would be the stack that needs a fix, in some cases a call to skb_checksum_help might be needed or maybe something else.
>>
>> Are you actually seeing fragments with CHECKSUM_PARTIAL set?
>
> I haven't seen any yet and I don't expect to. But the dev_warn that was in this area prior to the current patch was triggered when I first sent IPv6 packets with extended headers. Yes, the checksum was not computed, but there also was a warning in the log. Likewise when I didn't have the encapsulation working for this case, when I tried it I got a warning in the log. So it does indicate when things are broken and so is useful. Of course these were fixed by the driver patch because they were the result of deficiencies in the driver, which you first spotted by code inspection.
>
This does not entirely fix the problem though. It is a valid use case
that someone can send an UDP packet with an extension header that is
unknown to ipv6_skip_exthdr. The driver can do a dev_warn if it wants
to in this case I suppose, but sending out the packet without setting
the checksum is incorrect. The rule is simple: if the drive cannot
offload a checksum then call skb_checksum_help-- this as a default can
never make matters worse.
Tom
> --
> Mark Rustad, Networking Division, Intel Corporation
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path
2015-11-17 19:11 ` Tom Herbert
@ 2015-11-17 19:29 ` Rustad, Mark D
0 siblings, 0 replies; 10+ messages in thread
From: Rustad, Mark D @ 2015-11-17 19:29 UTC (permalink / raw)
To: intel-wired-lan
Tom Herbert <tom@herbertland.com> wrote:
> This does not entirely fix the problem though. It is a valid use case
> that someone can send an UDP packet with an extension header that is
> unknown to ipv6_skip_exthdr. The driver can do a dev_warn if it wants
> to in this case I suppose, but sending out the packet without setting
> the checksum is incorrect. The rule is simple: if the drive cannot
> offload a checksum then call skb_checksum_help-- this as a default can
> never make matters worse.
I suppose there could be both a message and the call to skb_checksum_help. The only drawback to that is a rate-limited message spamming the log that might go unnoticed for some time because things "work". I don't know what the security wonks would think about unrecognized extended headers being generated by an application. Hmm. Is it really possible to use an extended header that the kernel doesn't know about? If the kernel knows about it, then ipv6_skip_exthdr should work.
If the consensus is to add the call, I don't have a strong objection, I just have some reservations about it.
--
Mark Rustad, Networking Division, Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151117/795243e6/attachment.asc>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-11-17 19:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-17 0:26 [Intel-wired-lan] [PATCH V2] ixgbe: Handle extended IPv6 headers in tx path Mark D Rustad
2015-11-17 5:21 ` Tom Herbert
2015-11-17 16:41 ` Alexander Duyck
2015-11-17 16:48 ` Tom Herbert
2015-11-17 17:01 ` Alexander Duyck
2015-11-17 18:09 ` Rustad, Mark D
2015-11-17 18:22 ` Tom Herbert
2015-11-17 19:02 ` Rustad, Mark D
2015-11-17 19:11 ` Tom Herbert
2015-11-17 19:29 ` Rustad, Mark D
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.