[PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Michal Kubecek]

When --checksum_fill action is applied to a GSO packet, checksum_tg() calls
skb_checksum_help() which is only meant to be applied to non-GSO packets so
that it issues a warning.

This can be easily triggered by using e.g.

  iptables -t mangle -A OUTPUT -j CHECKSUM --checksum-fill

and sending TCP stream via a device with GSO enabled.

While this can be considered a misconfiguration, I believe the bad offload
warning is supposed to catch bugs in drivers and networking stack, not
misconfigured firewalls. So let's ignore such packets and only issue a one
time warning with pr_warn_once() rather than a WARN with stack trace and
tainted kernel.

Reported-by: Markos Chandras <markos.chandras@suse.com>
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 net/netfilter/xt_CHECKSUM.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_CHECKSUM.c b/net/netfilter/xt_CHECKSUM.c
index 0f642ef8cd26..9a007153ba7f 100644
--- a/net/netfilter/xt_CHECKSUM.c
+++ b/net/netfilter/xt_CHECKSUM.c
@@ -25,8 +25,12 @@ MODULE_ALIAS("ip6t_CHECKSUM");
 static unsigned int
 checksum_tg(struct sk_buff *skb, const struct xt_action_param *par)
 {
-	if (skb->ip_summed == CHECKSUM_PARTIAL)
-		skb_checksum_help(skb);
+	if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		if (unlikely(skb_is_gso(skb)))
+			pr_warn_once("cannot mangle checksum of a GSO packet\n");
+		else
+			skb_checksum_help(skb);
+	}
 
 	return XT_CONTINUE;
 }
-- 
2.14.1

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Florian Westphal]

Michal Kubecek <mkubecek@suse.cz> wrote:
> When --checksum_fill action is applied to a GSO packet, checksum_tg() calls
> skb_checksum_help() which is only meant to be applied to non-GSO packets so
> that it issues a warning.
> 
> This can be easily triggered by using e.g.
> 
>   iptables -t mangle -A OUTPUT -j CHECKSUM --checksum-fill
> 
> and sending TCP stream via a device with GSO enabled.
> 
> While this can be considered a misconfiguration, I believe the bad offload
> warning is supposed to catch bugs in drivers and networking stack, not
> misconfigured firewalls. So let's ignore such packets and only issue a one
> time warning with pr_warn_once() rather than a WARN with stack trace and
> tainted kernel.

Why issue a warning at all?
What kind of action should be taken upon seeing such warning?

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Michal Kubecek]

On Thu, Aug 24, 2017 at 12:51:18PM +0200, Florian Westphal wrote:
> Michal Kubecek <mkubecek@suse.cz> wrote:
> > When --checksum_fill action is applied to a GSO packet, checksum_tg() calls
> > skb_checksum_help() which is only meant to be applied to non-GSO packets so
> > that it issues a warning.
> > 
> > This can be easily triggered by using e.g.
> > 
> >   iptables -t mangle -A OUTPUT -j CHECKSUM --checksum-fill
> > 
> > and sending TCP stream via a device with GSO enabled.
> > 
> > While this can be considered a misconfiguration, I believe the bad offload
> > warning is supposed to catch bugs in drivers and networking stack, not
> > misconfigured firewalls. So let's ignore such packets and only issue a one
> > time warning with pr_warn_once() rather than a WARN with stack trace and
> > tainted kernel.
> 
> Why issue a warning at all?
> What kind of action should be taken upon seeing such warning?

Check and fix the configuration. The reason why I left at least some
kind of warning is that the module does something that is unexpected as
the checksum is not calculated (this module is often used in
virtualization environments where "hardware checksum offload" in fact
means the checksum is not computed at all).

But maybe it would suffice to add a note in iptables-extensions(8) man
page explicitely saying that it doesn't work with GSO packets (and is of
questionable usefulness for TCP in general).

                                                         Michal Kubecek

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Davide Caratti]

On Thu, 2017-08-24 at 13:07 +0200, Michal Kubecek wrote:
> On Thu, Aug 24, 2017 at 12:51:18PM +0200, Florian Westphal wrote:
> > Michal Kubecek <mkubecek@suse.cz> wrote:
> > > When --checksum_fill action is applied to a GSO packet, checksum_tg() calls
> > > skb_checksum_help() which is only meant to be applied to non-GSO packets so
> > > that it issues a warning.
> > > 
> > > This can be easily triggered by using e.g.
> > > 
> > >   iptables -t mangle -A OUTPUT -j CHECKSUM --checksum-fill
> > > 
> > > and sending TCP stream via a device with GSO enabled.
> > > 
> > > While this can be considered a misconfiguration, I believe the bad offload
> > > warning is supposed to catch bugs in drivers and networking stack, not
> > > misconfigured firewalls. So let's ignore such packets and only issue a one
> > > time warning with pr_warn_once() rather than a WARN with stack trace and
> > > tainted kernel.
> > 
> > Why issue a warning at all?
> > What kind of action should be taken upon seeing such warning?
> 
> Check and fix the configuration. The reason why I left at least some
> kind of warning is that the module does something that is unexpected as
> the checksum is not calculated (this module is often used in
> virtualization environments where "hardware checksum offload" in fact
> means the checksum is not computed at all).
> 

hello Michal,

GSO should be capable of computing the checksum on individual segments
later, so I also think the warning can be removed.

Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
hit xt_CHECKSUM target?

thank you in advance,
regards
-- 
davide

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Florian Westphal]

Davide Caratti <dcaratti@redhat.com> wrote:
> Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
> skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
> hit xt_CHECKSUM target?

Alternatively we could restrict the target to udp only.

AFAIU the only reason this thing exists is to fix up udp checksum
for old dhcp clients that use AF_PACKET without evaluating the extra
metadata that indicates when a 'bad' checksum is in fact ok because it
is supposed to be filled in by hardware later.

This can happen in virtual environemnt when such skb is directly passed
to vm.

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Michal Kubecek]

On Thu, Aug 24, 2017 at 03:08:42PM +0200, Davide Caratti wrote:
> On Thu, 2017-08-24 at 13:07 +0200, Michal Kubecek wrote:
> > On Thu, Aug 24, 2017 at 12:51:18PM +0200, Florian Westphal wrote:
> > > Michal Kubecek <mkubecek@suse.cz> wrote:
> > > > When --checksum_fill action is applied to a GSO packet, checksum_tg() calls
> > > > skb_checksum_help() which is only meant to be applied to non-GSO packets so
> > > > that it issues a warning.
> > > > 
> > > > This can be easily triggered by using e.g.
> > > > 
> > > >   iptables -t mangle -A OUTPUT -j CHECKSUM --checksum-fill
> > > > 
> > > > and sending TCP stream via a device with GSO enabled.
> > > > 
> > > > While this can be considered a misconfiguration, I believe the bad offload
> > > > warning is supposed to catch bugs in drivers and networking stack, not
> > > > misconfigured firewalls. So let's ignore such packets and only issue a one
> > > > time warning with pr_warn_once() rather than a WARN with stack trace and
> > > > tainted kernel.
> > > 
> > > Why issue a warning at all?
> > > What kind of action should be taken upon seeing such warning?
> > 
> > Check and fix the configuration. The reason why I left at least some
> > kind of warning is that the module does something that is unexpected as
> > the checksum is not calculated (this module is often used in
> > virtualization environments where "hardware checksum offload" in fact
> > means the checksum is not computed at all).
> > 
> 
> hello Michal,
> 
> GSO should be capable of computing the checksum on individual segments
> later, so I also think the warning can be removed.
> 
> Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
> skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
> hit xt_CHECKSUM target?

That sounds like an independent issue so it should be probably handled
by a separate patch.

Michal Kubecek

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Michal Kubecek]

On Thu, Aug 24, 2017 at 03:17:22PM +0200, Florian Westphal wrote:
> Davide Caratti <dcaratti@redhat.com> wrote:
> > Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
> > skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
> > hit xt_CHECKSUM target?
> 
> Alternatively we could restrict the target to udp only.
> 
> AFAIU the only reason this thing exists is to fix up udp checksum
> for old dhcp clients that use AF_PACKET without evaluating the extra
> metadata that indicates when a 'bad' checksum is in fact ok because it
> is supposed to be filled in by hardware later.
> 
> This can happen in virtual environemnt when such skb is directly passed
> to vm.

Based on what the documentation and the commit message of the commit
introducing xt_CHECKSUM module say, it seems so. But I must admit I'm
not sure where is the target is used and how (and why). In particular,
our issue was most likely result of

  https://github.com/openstack/openstack-ansible-tests/blob/master/test-prepare-host.yml#L196-L197

where they explicitely confine it to TCP packets. Unfortunately these
lines come from "Initial testing commit" so it's hard to say what the
intention behind that was.

                                                       Michal Kubecek

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Florian Westphal]

Michal Kubecek <mkubecek@suse.cz> wrote:
> On Thu, Aug 24, 2017 at 03:17:22PM +0200, Florian Westphal wrote:
> > Davide Caratti <dcaratti@redhat.com> wrote:
> > > Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
> > > skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
> > > hit xt_CHECKSUM target?
> > 
> > Alternatively we could restrict the target to udp only.
> > 
> > AFAIU the only reason this thing exists is to fix up udp checksum
> > for old dhcp clients that use AF_PACKET without evaluating the extra
> > metadata that indicates when a 'bad' checksum is in fact ok because it
> > is supposed to be filled in by hardware later.
> > 
> > This can happen in virtual environemnt when such skb is directly passed
> > to vm.
> 
> Based on what the documentation and the commit message of the commit
> introducing xt_CHECKSUM module say, it seems so. But I must admit I'm
> not sure where is the target is used and how (and why). In particular,
> our issue was most likely result of
> 
>   https://github.com/openstack/openstack-ansible-tests/blob/master/test-prepare-host.yml#L196-L197

Sigh.  Ok, that pretty much leaves your patch as the only viable option,
however, I still think the warning isn't useful.

Can you send a v2 with gso check but without warning?

Thanks!

Re: [PATCH nf-next] netfilter: xt_CHECKSUM: avoid bad offload warnings on GSO packets

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Michal Kubecek <mkubecek@suse.cz> wrote:
> > On Thu, Aug 24, 2017 at 03:17:22PM +0200, Florian Westphal wrote:
> > > Davide Caratti <dcaratti@redhat.com> wrote:
> > > > Small nit: may I suggest you to call skb_csum_hwoffload_help() instead of
> > > > skb_checksum_help(), so that we avoid corrupting SCTP packets in case they
> > > > hit xt_CHECKSUM target?
> > > 
> > > Alternatively we could restrict the target to udp only.
> > > 
> > > AFAIU the only reason this thing exists is to fix up udp checksum
> > > for old dhcp clients that use AF_PACKET without evaluating the extra
> > > metadata that indicates when a 'bad' checksum is in fact ok because it
> > > is supposed to be filled in by hardware later.
> > > 
> > > This can happen in virtual environemnt when such skb is directly passed
> > > to vm.
> > 
> > Based on what the documentation and the commit message of the commit
> > introducing xt_CHECKSUM module say, it seems so. But I must admit I'm
> > not sure where is the target is used and how (and why). In particular,
> > our issue was most likely result of
> > 
> >   https://github.com/openstack/openstack-ansible-tests/blob/master/test-prepare-host.yml#L196-L197
> 
> Sigh.  Ok, that pretty much leaves your patch as the only viable option,
> however, I still think the warning isn't useful.

Addendum: for net-next it makes sense to restrict this to udp and tcp
to avoid spreading this to sctp and other protocols.

We will however need to be lazy and can't just restrict it
in checkentry (as it might break existing config).

We could print a warning and have the target function ignores protocols
other than tcp and udp.