| Message ID | 20190821032035.16263-2-matthew.ruffell@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Xenial,1/1] netfilter: xt_checksum: ignore gso skbs | expand |
On 21.08.19 05:20, Matthew Ruffell wrote: > From: Florian Westphal <fw@strlen.de> > > BugLink: https://bugs.launchpad.net/bugs/1840619 > > Satish Patel reports a skb_warn_bad_offload() splat caused > by -j CHECKSUM rules: > > -A POSTROUTING -p tcp -m tcp --sport 80 -j CHECKSUM > > The CHECKSUM target has never worked with GSO skbs, and the above rule > makes no sense as kernel will handle checksum updates on transmit. > > Unfortunately, there are 3rd party tools that install such rules, so we > cannot reject this from the config plane without potential breakage. > > Amend Kconfig text to clarify that the CHECKSUM target is only useful > in virtualized environments, where old dhcp clients that use AF_PACKET > used to discard UDP packets with a 'bad' header checksum and add a > one-time warning in case such rule isn't restricted to UDP. > > v2: check IP6T_F_PROTO flag before cmp (Michal Kubecek) > > Reported-by: Satish Patel <satish.txt@gmail.com> > Reported-by: Markos Chandras <markos.chandras@suse.com> > Reported-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Florian Westphal <fw@strlen.de> > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > (backported from commit 10568f6c5761db24249c610c94d6e44d5505a0ba) > [mruffell: minor context adjustment] > Signed-off-by: Matthew Ruffell <matthew.ruffell@canonical.com> The bug report kept the development task incomplete, however this patch is from 4.19 (and released for Bionic with Ubuntu-4.15.0-56.61), so I set the devel task to fix released. Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > net/netfilter/Kconfig | 12 ++++++------ > net/netfilter/xt_CHECKSUM.c | 23 ++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig > index 4692782b5280..e13e2eecbaf4 100644 > --- a/net/netfilter/Kconfig > +++ b/net/netfilter/Kconfig > @@ -635,13 +635,13 @@ config NETFILTER_XT_TARGET_CHECKSUM > depends on NETFILTER_ADVANCED > ---help--- > This option adds a `CHECKSUM' target, which can be used in the iptables mangle > - table. > + table to work around buggy DHCP clients in virtualized environments. > > - You can use this target to compute and fill in the checksum in > - a packet that lacks a checksum. This is particularly useful, > - if you need to work around old applications such as dhcp clients, > - that do not work well with checksum offloads, but don't want to disable > - checksum offload in your device. > + Some old DHCP clients drop packets because they are not aware > + that the checksum would normally be offloaded to hardware and > + thus should be considered valid. > + This target can be used to fill in the checksum using iptables > + when such packets are sent via a virtual network device. > > To compile it as a module, choose M here. If unsure, say N. > > diff --git a/net/netfilter/xt_CHECKSUM.c b/net/netfilter/xt_CHECKSUM.c > index 0f642ef8cd26..db286fc0bc32 100644 > --- a/net/netfilter/xt_CHECKSUM.c > +++ b/net/netfilter/xt_CHECKSUM.c > @@ -16,6 +16,9 @@ > #include <linux/netfilter/x_tables.h> > #include <linux/netfilter/xt_CHECKSUM.h> > > +#include <linux/netfilter_ipv4/ip_tables.h> > +#include <linux/netfilter_ipv6/ip6_tables.h> > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Michael S. Tsirkin <mst@redhat.com>"); > MODULE_DESCRIPTION("Xtables: checksum modification"); > @@ -25,7 +28,7 @@ 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) > + if (skb->ip_summed == CHECKSUM_PARTIAL && !skb_is_gso(skb)) > skb_checksum_help(skb); > > return XT_CONTINUE; > @@ -34,6 +37,8 @@ checksum_tg(struct sk_buff *skb, const struct xt_action_param *par) > static int checksum_tg_check(const struct xt_tgchk_param *par) > { > const struct xt_CHECKSUM_info *einfo = par->targinfo; > + const struct ip6t_ip6 *i6 = par->entryinfo; > + const struct ipt_ip *i4 = par->entryinfo; > > if (einfo->operation & ~XT_CHECKSUM_OP_FILL) { > pr_info("unsupported CHECKSUM operation %x\n", einfo->operation); > @@ -43,6 +48,22 @@ static int checksum_tg_check(const struct xt_tgchk_param *par) > pr_info("no CHECKSUM operation enabled\n"); > return -EINVAL; > } > + > + switch (par->family) { > + case NFPROTO_IPV4: > + if (i4->proto == IPPROTO_UDP && > + (i4->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + case NFPROTO_IPV6: > + if ((i6->flags & IP6T_F_PROTO) && > + i6->proto == IPPROTO_UDP && > + (i6->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + } > + > + pr_warn_once("CHECKSUM should be avoided. If really needed, restrict with \"-p udp\" and only use in OUTPUT\n"); > return 0; > } > >
On 8/20/19 8:20 PM, Matthew Ruffell wrote: > From: Florian Westphal <fw@strlen.de> > > BugLink: https://bugs.launchpad.net/bugs/1840619 > > Satish Patel reports a skb_warn_bad_offload() splat caused > by -j CHECKSUM rules: > > -A POSTROUTING -p tcp -m tcp --sport 80 -j CHECKSUM > > The CHECKSUM target has never worked with GSO skbs, and the above rule > makes no sense as kernel will handle checksum updates on transmit. > > Unfortunately, there are 3rd party tools that install such rules, so we > cannot reject this from the config plane without potential breakage. > > Amend Kconfig text to clarify that the CHECKSUM target is only useful > in virtualized environments, where old dhcp clients that use AF_PACKET > used to discard UDP packets with a 'bad' header checksum and add a > one-time warning in case such rule isn't restricted to UDP. > > v2: check IP6T_F_PROTO flag before cmp (Michal Kubecek) > > Reported-by: Satish Patel <satish.txt@gmail.com> > Reported-by: Markos Chandras <markos.chandras@suse.com> > Reported-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Florian Westphal <fw@strlen.de> > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > (backported from commit 10568f6c5761db24249c610c94d6e44d5505a0ba) > [mruffell: minor context adjustment] > Signed-off-by: Matthew Ruffell <matthew.ruffell@canonical.com> Acked-by: Connor Kuehl <connor.kuehl@canonical.com> > --- > net/netfilter/Kconfig | 12 ++++++------ > net/netfilter/xt_CHECKSUM.c | 23 ++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig > index 4692782b5280..e13e2eecbaf4 100644 > --- a/net/netfilter/Kconfig > +++ b/net/netfilter/Kconfig > @@ -635,13 +635,13 @@ config NETFILTER_XT_TARGET_CHECKSUM > depends on NETFILTER_ADVANCED > ---help--- > This option adds a `CHECKSUM' target, which can be used in the iptables mangle > - table. > + table to work around buggy DHCP clients in virtualized environments. > > - You can use this target to compute and fill in the checksum in > - a packet that lacks a checksum. This is particularly useful, > - if you need to work around old applications such as dhcp clients, > - that do not work well with checksum offloads, but don't want to disable > - checksum offload in your device. > + Some old DHCP clients drop packets because they are not aware > + that the checksum would normally be offloaded to hardware and > + thus should be considered valid. > + This target can be used to fill in the checksum using iptables > + when such packets are sent via a virtual network device. > > To compile it as a module, choose M here. If unsure, say N. > > diff --git a/net/netfilter/xt_CHECKSUM.c b/net/netfilter/xt_CHECKSUM.c > index 0f642ef8cd26..db286fc0bc32 100644 > --- a/net/netfilter/xt_CHECKSUM.c > +++ b/net/netfilter/xt_CHECKSUM.c > @@ -16,6 +16,9 @@ > #include <linux/netfilter/x_tables.h> > #include <linux/netfilter/xt_CHECKSUM.h> > > +#include <linux/netfilter_ipv4/ip_tables.h> > +#include <linux/netfilter_ipv6/ip6_tables.h> > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Michael S. Tsirkin <mst@redhat.com>"); > MODULE_DESCRIPTION("Xtables: checksum modification"); > @@ -25,7 +28,7 @@ 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) > + if (skb->ip_summed == CHECKSUM_PARTIAL && !skb_is_gso(skb)) > skb_checksum_help(skb); > > return XT_CONTINUE; > @@ -34,6 +37,8 @@ checksum_tg(struct sk_buff *skb, const struct xt_action_param *par) > static int checksum_tg_check(const struct xt_tgchk_param *par) > { > const struct xt_CHECKSUM_info *einfo = par->targinfo; > + const struct ip6t_ip6 *i6 = par->entryinfo; > + const struct ipt_ip *i4 = par->entryinfo; > > if (einfo->operation & ~XT_CHECKSUM_OP_FILL) { > pr_info("unsupported CHECKSUM operation %x\n", einfo->operation); > @@ -43,6 +48,22 @@ static int checksum_tg_check(const struct xt_tgchk_param *par) > pr_info("no CHECKSUM operation enabled\n"); > return -EINVAL; > } > + > + switch (par->family) { > + case NFPROTO_IPV4: > + if (i4->proto == IPPROTO_UDP && > + (i4->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + case NFPROTO_IPV6: > + if ((i6->flags & IP6T_F_PROTO) && > + i6->proto == IPPROTO_UDP && > + (i6->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + } > + > + pr_warn_once("CHECKSUM should be avoided. If really needed, restrict with \"-p udp\" and only use in OUTPUT\n"); > return 0; > } > >
On 8/21/19 5:20 AM, Matthew Ruffell wrote: > From: Florian Westphal <fw@strlen.de> > > BugLink: https://bugs.launchpad.net/bugs/1840619 > > Satish Patel reports a skb_warn_bad_offload() splat caused > by -j CHECKSUM rules: > > -A POSTROUTING -p tcp -m tcp --sport 80 -j CHECKSUM > > The CHECKSUM target has never worked with GSO skbs, and the above rule > makes no sense as kernel will handle checksum updates on transmit. > > Unfortunately, there are 3rd party tools that install such rules, so we > cannot reject this from the config plane without potential breakage. > > Amend Kconfig text to clarify that the CHECKSUM target is only useful > in virtualized environments, where old dhcp clients that use AF_PACKET > used to discard UDP packets with a 'bad' header checksum and add a > one-time warning in case such rule isn't restricted to UDP. > > v2: check IP6T_F_PROTO flag before cmp (Michal Kubecek) > > Reported-by: Satish Patel <satish.txt@gmail.com> > Reported-by: Markos Chandras <markos.chandras@suse.com> > Reported-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Florian Westphal <fw@strlen.de> > Reviewed-by: Michal Kubecek <mkubecek@suse.cz> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > (backported from commit 10568f6c5761db24249c610c94d6e44d5505a0ba) > [mruffell: minor context adjustment] > Signed-off-by: Matthew Ruffell <matthew.ruffell@canonical.com> > --- > net/netfilter/Kconfig | 12 ++++++------ > net/netfilter/xt_CHECKSUM.c | 23 ++++++++++++++++++++++- > 2 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig > index 4692782b5280..e13e2eecbaf4 100644 > --- a/net/netfilter/Kconfig > +++ b/net/netfilter/Kconfig > @@ -635,13 +635,13 @@ config NETFILTER_XT_TARGET_CHECKSUM > depends on NETFILTER_ADVANCED > ---help--- > This option adds a `CHECKSUM' target, which can be used in the iptables mangle > - table. > + table to work around buggy DHCP clients in virtualized environments. > > - You can use this target to compute and fill in the checksum in > - a packet that lacks a checksum. This is particularly useful, > - if you need to work around old applications such as dhcp clients, > - that do not work well with checksum offloads, but don't want to disable > - checksum offload in your device. > + Some old DHCP clients drop packets because they are not aware > + that the checksum would normally be offloaded to hardware and > + thus should be considered valid. > + This target can be used to fill in the checksum using iptables > + when such packets are sent via a virtual network device. > > To compile it as a module, choose M here. If unsure, say N. > > diff --git a/net/netfilter/xt_CHECKSUM.c b/net/netfilter/xt_CHECKSUM.c > index 0f642ef8cd26..db286fc0bc32 100644 > --- a/net/netfilter/xt_CHECKSUM.c > +++ b/net/netfilter/xt_CHECKSUM.c > @@ -16,6 +16,9 @@ > #include <linux/netfilter/x_tables.h> > #include <linux/netfilter/xt_CHECKSUM.h> > > +#include <linux/netfilter_ipv4/ip_tables.h> > +#include <linux/netfilter_ipv6/ip6_tables.h> > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Michael S. Tsirkin <mst@redhat.com>"); > MODULE_DESCRIPTION("Xtables: checksum modification"); > @@ -25,7 +28,7 @@ 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) > + if (skb->ip_summed == CHECKSUM_PARTIAL && !skb_is_gso(skb)) > skb_checksum_help(skb); > > return XT_CONTINUE; > @@ -34,6 +37,8 @@ checksum_tg(struct sk_buff *skb, const struct xt_action_param *par) > static int checksum_tg_check(const struct xt_tgchk_param *par) > { > const struct xt_CHECKSUM_info *einfo = par->targinfo; > + const struct ip6t_ip6 *i6 = par->entryinfo; > + const struct ipt_ip *i4 = par->entryinfo; > > if (einfo->operation & ~XT_CHECKSUM_OP_FILL) { > pr_info("unsupported CHECKSUM operation %x\n", einfo->operation); > @@ -43,6 +48,22 @@ static int checksum_tg_check(const struct xt_tgchk_param *par) > pr_info("no CHECKSUM operation enabled\n"); > return -EINVAL; > } > + > + switch (par->family) { > + case NFPROTO_IPV4: > + if (i4->proto == IPPROTO_UDP && > + (i4->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + case NFPROTO_IPV6: > + if ((i6->flags & IP6T_F_PROTO) && > + i6->proto == IPPROTO_UDP && > + (i6->invflags & XT_INV_PROTO) == 0) > + return 0; > + break; > + } > + > + pr_warn_once("CHECKSUM should be avoided. If really needed, restrict with \"-p udp\" and only use in OUTPUT\n"); > return 0; > } > > Applied to xenial/master-next branch. Thanks, Kleber
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig index 4692782b5280..e13e2eecbaf4 100644 --- a/net/netfilter/Kconfig +++ b/net/netfilter/Kconfig @@ -635,13 +635,13 @@ config NETFILTER_XT_TARGET_CHECKSUM depends on NETFILTER_ADVANCED ---help--- This option adds a `CHECKSUM' target, which can be used in the iptables mangle - table. + table to work around buggy DHCP clients in virtualized environments. - You can use this target to compute and fill in the checksum in - a packet that lacks a checksum. This is particularly useful, - if you need to work around old applications such as dhcp clients, - that do not work well with checksum offloads, but don't want to disable - checksum offload in your device. + Some old DHCP clients drop packets because they are not aware + that the checksum would normally be offloaded to hardware and + thus should be considered valid. + This target can be used to fill in the checksum using iptables + when such packets are sent via a virtual network device. To compile it as a module, choose M here. If unsure, say N. diff --git a/net/netfilter/xt_CHECKSUM.c b/net/netfilter/xt_CHECKSUM.c index 0f642ef8cd26..db286fc0bc32 100644 --- a/net/netfilter/xt_CHECKSUM.c +++ b/net/netfilter/xt_CHECKSUM.c @@ -16,6 +16,9 @@ #include <linux/netfilter/x_tables.h> #include <linux/netfilter/xt_CHECKSUM.h> +#include <linux/netfilter_ipv4/ip_tables.h> +#include <linux/netfilter_ipv6/ip6_tables.h> + MODULE_LICENSE("GPL"); MODULE_AUTHOR("Michael S. Tsirkin <mst@redhat.com>"); MODULE_DESCRIPTION("Xtables: checksum modification"); @@ -25,7 +28,7 @@ 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) + if (skb->ip_summed == CHECKSUM_PARTIAL && !skb_is_gso(skb)) skb_checksum_help(skb); return XT_CONTINUE; @@ -34,6 +37,8 @@ checksum_tg(struct sk_buff *skb, const struct xt_action_param *par) static int checksum_tg_check(const struct xt_tgchk_param *par) { const struct xt_CHECKSUM_info *einfo = par->targinfo; + const struct ip6t_ip6 *i6 = par->entryinfo; + const struct ipt_ip *i4 = par->entryinfo; if (einfo->operation & ~XT_CHECKSUM_OP_FILL) { pr_info("unsupported CHECKSUM operation %x\n", einfo->operation); @@ -43,6 +48,22 @@ static int checksum_tg_check(const struct xt_tgchk_param *par) pr_info("no CHECKSUM operation enabled\n"); return -EINVAL; } + + switch (par->family) { + case NFPROTO_IPV4: + if (i4->proto == IPPROTO_UDP && + (i4->invflags & XT_INV_PROTO) == 0) + return 0; + break; + case NFPROTO_IPV6: + if ((i6->flags & IP6T_F_PROTO) && + i6->proto == IPPROTO_UDP && + (i6->invflags & XT_INV_PROTO) == 0) + return 0; + break; + } + + pr_warn_once("CHECKSUM should be avoided. If really needed, restrict with \"-p udp\" and only use in OUTPUT\n"); return 0; }