[PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Tom Herbert]

Currently if a device provides CHECKSUM_COMPLETE but the checksum
is calculated to be invalid we recompute the checksum and try
again in software. On the other hand, if device returns
CHECKSUM_UNNECESSARY we implicitly trust it and don't verify what it
did. This seems backwards!

Add a sysctl to trust the device and report an invalid checksum when
CHECKSUM_COMPLETE shows it is incorrect. sysctl defaults to enabled.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 include/linux/skbuff.h     | 7 ++++++-
 net/core/dev.c             | 3 +++
 net/core/sysctl_net_core.c | 7 +++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3b14acc..f3c6018 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2763,6 +2763,8 @@ static inline __sum16 __skb_checksum_validate_unnecessary(struct sk_buff *skb,
 	return 1;
 }
 
+extern int sysctl_trust_dev_checksum;
+
 /*
  * Validate (init) checksum based on checksum complete.
  *
@@ -2775,9 +2777,12 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
 						       __wsum psum)
 {
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
-		if (!csum_fold(csum_add(psum, skb->csum))) {
+		__sum16 csum = csum_fold(csum_add(psum, skb->csum));
+		if (!csum) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			return 0;
+		} else if (sysctl_trust_dev_checksum) {
+			return csum;
 		}
 	}
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 11d70e3..4f7980a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2918,6 +2918,9 @@ int netdev_tstamp_prequeue __read_mostly = 1;
 int netdev_budget __read_mostly = 300;
 int weight_p __read_mostly = 64;            /* old backlog weight */
 
+int sysctl_trust_dev_checksum = 1;
+EXPORT_SYMBOL(sysctl_trust_dev_checksum);
+
 /* Called with irq disabled */
 static inline void ____napi_schedule(struct softnet_data *sd,
 				     struct napi_struct *napi)
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index cf9cd13..246b9e3 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -263,6 +263,13 @@ static struct ctl_table net_core_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
+	{
+		.procname	= "trust_dev_checksum",
+		.data		= &sysctl_trust_dev_checksum,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
 #ifdef CONFIG_BPF_JIT
 	{
 		.procname	= "bpf_jit_enable",
-- 
1.9.1.423.g4596e3a

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

On Sat, 2014-04-26 at 14:26 -0700, Tom Herbert wrote:
> Currently if a device provides CHECKSUM_COMPLETE but the checksum
> is calculated to be invalid we recompute the checksum and try
> again in software. On the other hand, if device returns
> CHECKSUM_UNNECESSARY we implicitly trust it and don't verify what it
> did. This seems backwards!
> 
> Add a sysctl to trust the device and report an invalid checksum when
> CHECKSUM_COMPLETE shows it is incorrect. sysctl defaults to enabled.
[...]

I think the sysctl and variable names need an '_error' or '_err' on the
end, to distinguish this from the NETIF_F_RXCSUM feature flag.

The new sysctl also needs to be documented, again making that
distinction clear.

Finally, if we're going to start trusting device checksum errors,
perhaps there should be a way to indicate a checksum error from a device
that only reports a checksum good/bad flag.  I do understand that such
devices are more likely to have bugs in checksum validation, though.

Ben.

-- 
Ben Hutchings
The most exhausting thing in life is being insincere. - Anne Morrow Lindberg

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Stephen Hemminger]

On Sat, 26 Apr 2014 14:26:35 -0700 (PDT)
Tom Herbert <therbert@google.com> wrote:

> Currently if a device provides CHECKSUM_COMPLETE but the checksum
> is calculated to be invalid we recompute the checksum and try
> again in software. On the other hand, if device returns
> CHECKSUM_UNNECESSARY we implicitly trust it and don't verify what it
> did. This seems backwards!
> 
> Add a sysctl to trust the device and report an invalid checksum when
> CHECKSUM_COMPLETE shows it is incorrect. sysctl defaults to enabled.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---

NO. Make one choice and do it consistently.

Papering over driver bugs or design confusion with a sysctl is not a
reasonable choice.

If some device (or code path) has invalid checksum logic, it should
be reported once and go ahead and fix it in software. The problem with
CHECKSUM_UNNECESSARY is that there is no way to check that the device
is broken without computing the checksum (catch-22).

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Tom Herbert]

On Sat, Apr 26, 2014 at 7:31 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Sat, 26 Apr 2014 14:26:35 -0700 (PDT)
> Tom Herbert <therbert@google.com> wrote:
>
>> Currently if a device provides CHECKSUM_COMPLETE but the checksum
>> is calculated to be invalid we recompute the checksum and try
>> again in software. On the other hand, if device returns
>> CHECKSUM_UNNECESSARY we implicitly trust it and don't verify what it
>> did. This seems backwards!
>>
>> Add a sysctl to trust the device and report an invalid checksum when
>> CHECKSUM_COMPLETE shows it is incorrect. sysctl defaults to enabled.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>> ---
>
> NO. Make one choice and do it consistently.
>
> Papering over driver bugs or design confusion with a sysctl is not a
> reasonable choice.
>
> If some device (or code path) has invalid checksum logic, it should
> be reported once and go ahead and fix it in software. The problem with
> CHECKSUM_UNNECESSARY is that there is no way to check that the device
> is broken without computing the checksum (catch-22).
>
Unlike CHECKSUM_UNNECESSARY, CHECKSUM_COMPLETE provides information
for both valid and invalid checksums. Also, CHECKSUM_COMPLETE is well
defined and specific as to what the device and driver are returning
and I have to believe should be easier to get right. So I don't see
any reason why we shouldn't use the negative information returned by
CHECKSUM_COMPLETE; always recomputing an invalid checksum in SW is a
waste of CPU cycles and could become a basis for DOS attack at high
speeds.

In the unlikely event that the HW (or driver) is incorrectly computing
the checksum this would manifest itself as checksum errors. This is
quite debuggable, and the sysctl would be useful to narrow down
whether the issue is in checksum calculation logic or actual bad
checksums. Compare this to incorrect reporting of CHECKSUM_UNNECESSARY
which could result in packets with errors being accepted without any
detection-- it might take years to identify such a problem!

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Sun, 27 Apr 2014 10:03:48 -0700

> On Sat, Apr 26, 2014 at 7:31 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Sat, 26 Apr 2014 14:26:35 -0700 (PDT)
>> Tom Herbert <therbert@google.com> wrote:
>>
>>> Currently if a device provides CHECKSUM_COMPLETE but the checksum
>>> is calculated to be invalid we recompute the checksum and try
>>> again in software. On the other hand, if device returns
>>> CHECKSUM_UNNECESSARY we implicitly trust it and don't verify what it
>>> did. This seems backwards!
>>>
>>> Add a sysctl to trust the device and report an invalid checksum when
>>> CHECKSUM_COMPLETE shows it is incorrect. sysctl defaults to enabled.
>>>
>>> Signed-off-by: Tom Herbert <therbert@google.com>
>>> ---
>>
>> NO. Make one choice and do it consistently.
>>
>> Papering over driver bugs or design confusion with a sysctl is not a
>> reasonable choice.
>>
>> If some device (or code path) has invalid checksum logic, it should
>> be reported once and go ahead and fix it in software. The problem with
>> CHECKSUM_UNNECESSARY is that there is no way to check that the device
>> is broken without computing the checksum (catch-22).
>>
> Unlike CHECKSUM_UNNECESSARY, CHECKSUM_COMPLETE provides information
> for both valid and invalid checksums. Also, CHECKSUM_COMPLETE is well
> defined and specific as to what the device and driver are returning
> and I have to believe should be easier to get right. So I don't see
> any reason why we shouldn't use the negative information returned by
> CHECKSUM_COMPLETE; always recomputing an invalid checksum in SW is a
> waste of CPU cycles and could become a basis for DOS attack at high
> speeds.
> 
> In the unlikely event that the HW (or driver) is incorrectly computing
> the checksum this would manifest itself as checksum errors. This is
> quite debuggable, and the sysctl would be useful to narrow down
> whether the issue is in checksum calculation logic or actual bad
> checksums. Compare this to incorrect reporting of CHECKSUM_UNNECESSARY
> which could result in packets with errors being accepted without any
> detection-- it might take years to identify such a problem!

If we do anything, we should do it consistently and not just for one
specific checksum delivery type.

So if we add a sysctl, it should revalidate the checksum in software
for all checksum offload variants, and such a sysctl should be off by
default.

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Tom Herbert]

> If we do anything, we should do it consistently and not just for one
> specific checksum delivery type.
>
> So if we add a sysctl, it should revalidate the checksum in software
> for all checksum offload variants, and such a sysctl should be off by
> default.

Okay, but I would want to add a new checksum type to distinguish
checksum_complete that was done in software as opposed to one received
from a device. I was planning on doing this in a later patch set. I
suppose you can disregard this patch for now.

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[David Miller]

From: Tom Herbert <therbert@google.com>
Date: Sun, 27 Apr 2014 21:07:24 -0700

>> If we do anything, we should do it consistently and not just for one
>> specific checksum delivery type.
>>
>> So if we add a sysctl, it should revalidate the checksum in software
>> for all checksum offload variants, and such a sysctl should be off by
>> default.
> 
> Okay, but I would want to add a new checksum type to distinguish
> checksum_complete that was done in software as opposed to one received
> from a device.

I don't think it's wise to keep CHECKSUM_COMPLETE once you've software
verified it.

If anything, you should at that point treat it as we would have treated
it were it marked CHECKSUM_NONE.

Therefore I see no reason for a new checksum type.

Please repost your series if you want me to consider it with this patch
removed, rather than just saying "apply the series without patch X".

Thanks.

Re: [PATCH 9/9 v2] net: Add sysctl to trust checksum_complete

[Tom Herbert]

On Sun, Apr 27, 2014 at 9:15 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <therbert@google.com>
> Date: Sun, 27 Apr 2014 21:07:24 -0700
>
>>> If we do anything, we should do it consistently and not just for one
>>> specific checksum delivery type.
>>>
>>> So if we add a sysctl, it should revalidate the checksum in software
>>> for all checksum offload variants, and such a sysctl should be off by
>>> default.
>>
>> Okay, but I would want to add a new checksum type to distinguish
>> checksum_complete that was done in software as opposed to one received
>> from a device.
>
> I don't think it's wise to keep CHECKSUM_COMPLETE once you've software
> verified it.
>
> If anything, you should at that point treat it as we would have treated
> it were it marked CHECKSUM_NONE.
>
We want to keep it if there are multiple checksums in the packet which
is possible with encapsulation. It really shouldn't add any cost to
maintain it with the packet.