* [PATCH] openvswitch: Trim off padding before L3 conntrack processing
@ 2017-12-12 16:17 Ed Swierk
2017-12-14 0:58 ` Pravin Shelar
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-12 16:17 UTC (permalink / raw)
To: ovs-dev, netdev, Pravin Shelar
Cc: Lance Richardson, Ed Swierk, Benjamin Warren, Keith Holleman
A short IPv4 packet may have up to 6 bytes of padding following the IP
payload when received on an Ethernet device.
In the normal IPv4 receive path, ip_rcv() trims the packet to
ip_hdr->tot_len before invoking NF_INET_PRE_ROUTING hooks (including
conntrack). Then any subsequent L3+ processing steps, like
nf_checksum(), use skb->len as the length of the packet, rather than
referring back to ip_hdr->tot_len. In the IPv6 receive path, ip6_rcv()
does the same using ipv6_hdr->payload_len.
In the OVS conntrack receive path, this trimming does not occur, so
the checksum verification in tcp_header() fails, printing "nf_ct_tcp:
bad TCP checksum". Extra zero bytes don't affect the checksum, but the
length in the IP pseudoheader does. That length is based on skb->len,
and without trimming, it doesn't match the length the sender used when
computing the checksum.
With this change, OVS conntrack trims IPv4 and IPv6 packets prior to
L3 processing.
Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
net/openvswitch/conntrack.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index d558e882ca0c..3a7c9215c431 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1105,12 +1105,29 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
const struct ovs_conntrack_info *info)
{
int nh_ofs;
+ unsigned int nh_len;
int err;
/* The conntrack module expects to be working at L3. */
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ /* Trim to L3 length since nf_checksum() doesn't expect padding. */
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ nh_len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ nh_len = ntohs(ipv6_hdr(skb)->payload_len)
+ + sizeof(struct ipv6hdr);
+ break;
+ default:
+ nh_len = skb->len;
+ }
+ err = pskb_trim_rcsum(skb, nh_len);
+ if (err)
+ return err;
+
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
if (err)
--
2.11.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] openvswitch: Trim off padding before L3 conntrack processing
2017-12-12 16:17 [PATCH] openvswitch: Trim off padding before L3 conntrack processing Ed Swierk
@ 2017-12-14 0:58 ` Pravin Shelar
2017-12-14 20:05 ` Ed Swierk
2017-12-21 15:17 ` [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Ed Swierk
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Pravin Shelar @ 2017-12-14 0:58 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Pravin Shelar,
Lance Richardson, Benjamin Warren, Keith Holleman
On Tue, Dec 12, 2017 at 8:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> A short IPv4 packet may have up to 6 bytes of padding following the IP
> payload when received on an Ethernet device.
>
> In the normal IPv4 receive path, ip_rcv() trims the packet to
> ip_hdr->tot_len before invoking NF_INET_PRE_ROUTING hooks (including
> conntrack). Then any subsequent L3+ processing steps, like
> nf_checksum(), use skb->len as the length of the packet, rather than
> referring back to ip_hdr->tot_len. In the IPv6 receive path, ip6_rcv()
> does the same using ipv6_hdr->payload_len.
>
> In the OVS conntrack receive path, this trimming does not occur, so
> the checksum verification in tcp_header() fails, printing "nf_ct_tcp:
> bad TCP checksum". Extra zero bytes don't affect the checksum, but the
> length in the IP pseudoheader does. That length is based on skb->len,
> and without trimming, it doesn't match the length the sender used when
> computing the checksum.
>
> With this change, OVS conntrack trims IPv4 and IPv6 packets prior to
> L3 processing.
>
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
> net/openvswitch/conntrack.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index d558e882ca0c..3a7c9215c431 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1105,12 +1105,29 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> const struct ovs_conntrack_info *info)
> {
> int nh_ofs;
> + unsigned int nh_len;
> int err;
>
> /* The conntrack module expects to be working at L3. */
> nh_ofs = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_ofs);
>
> + /* Trim to L3 length since nf_checksum() doesn't expect padding. */
Can you explore if nf_checksum can be changed to avoid the padding?
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + nh_len = ntohs(ip_hdr(skb)->tot_len);
> + break;
> + case htons(ETH_P_IPV6):
> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
> + + sizeof(struct ipv6hdr);
> + break;
> + default:
> + nh_len = skb->len;
> + }
> + err = pskb_trim_rcsum(skb, nh_len);
> + if (err)
> + return err;
> +
In case of error skb needs to be freed.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] openvswitch: Trim off padding before L3 conntrack processing
2017-12-14 0:58 ` Pravin Shelar
@ 2017-12-14 20:05 ` Ed Swierk
2017-12-17 19:22 ` Pravin Shelar
2017-12-17 19:22 ` Pravin Shelar
0 siblings, 2 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-14 20:05 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Lance Richardson,
Benjamin Warren, Keith Holleman
On Wed, Dec 13, 2017 at 4:58 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Tue, Dec 12, 2017 at 8:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> A short IPv4 packet may have up to 6 bytes of padding following the IP
>> payload when received on an Ethernet device.
>>
>> In the normal IPv4 receive path, ip_rcv() trims the packet to
>> ip_hdr->tot_len before invoking NF_INET_PRE_ROUTING hooks (including
>> conntrack). Then any subsequent L3+ processing steps, like
>> nf_checksum(), use skb->len as the length of the packet, rather than
>> referring back to ip_hdr->tot_len. In the IPv6 receive path, ip6_rcv()
>> does the same using ipv6_hdr->payload_len.
>>
>> In the OVS conntrack receive path, this trimming does not occur, so
>> the checksum verification in tcp_header() fails, printing "nf_ct_tcp:
>> bad TCP checksum". Extra zero bytes don't affect the checksum, but the
>> length in the IP pseudoheader does. That length is based on skb->len,
>> and without trimming, it doesn't match the length the sender used when
>> computing the checksum.
>>
>> With this change, OVS conntrack trims IPv4 and IPv6 packets prior to
>> L3 processing.
>>
>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> ---
>> net/openvswitch/conntrack.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index d558e882ca0c..3a7c9215c431 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -1105,12 +1105,29 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>> const struct ovs_conntrack_info *info)
>> {
>> int nh_ofs;
>> + unsigned int nh_len;
>> int err;
>>
>> /* The conntrack module expects to be working at L3. */
>> nh_ofs = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_ofs);
>>
>> + /* Trim to L3 length since nf_checksum() doesn't expect padding. */
> Can you explore if nf_checksum can be changed to avoid the padding?
The nf_ip_checksum() and nf_ip6_checksum() helper functions can easily
be changed to avoid the padding.
My worry is that conntrack is just one of many netfilter hooks that
perform L3+ processing, and may assume that once skb->data points to
the L3 header, skb->len reflects the length of the L3 header and
payload. For example, in nf_conntrack_ftp.c, help() uses skb->len to
determine the length of the FTP payload and the TCP sequence number of
the next packet; this would be thrown off by lower-layer padding.
br_netfilter, a cousin of OVS, has always preserved this
assumption--like ip_rcv() and ip6_rcv(), br_validate_ipv4() and
br_validate_ipv6() trim the skb to the L3 length before they invoke
NF_INET_PRE_ROUTING hooks. Modifying OVS to fit the mold seems more
straightforward than changing this assumption.
>> + switch (skb->protocol) {
>> + case htons(ETH_P_IP):
>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>> + break;
>> + case htons(ETH_P_IPV6):
>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>> + + sizeof(struct ipv6hdr);
>> + break;
>> + default:
>> + nh_len = skb->len;
>> + }
>> + err = pskb_trim_rcsum(skb, nh_len);
>> + if (err)
>> + return err;
>> +
> In case of error skb needs to be freed.
Thanks, I will fix this.
--Ed
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] openvswitch: Trim off padding before L3 conntrack processing
2017-12-14 20:05 ` Ed Swierk
@ 2017-12-17 19:22 ` Pravin Shelar
2017-12-17 19:22 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2017-12-17 19:22 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Lance Richardson,
Benjamin Warren, Keith Holleman
On Thu, Dec 14, 2017 at 12:05 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Wed, Dec 13, 2017 at 4:58 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Tue, Dec 12, 2017 at 8:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> A short IPv4 packet may have up to 6 bytes of padding following the IP
>>> payload when received on an Ethernet device.
>>>
>>> In the normal IPv4 receive path, ip_rcv() trims the packet to
>>> ip_hdr->tot_len before invoking NF_INET_PRE_ROUTING hooks (including
>>> conntrack). Then any subsequent L3+ processing steps, like
>>> nf_checksum(), use skb->len as the length of the packet, rather than
>>> referring back to ip_hdr->tot_len. In the IPv6 receive path, ip6_rcv()
>>> does the same using ipv6_hdr->payload_len.
>>>
>>> In the OVS conntrack receive path, this trimming does not occur, so
>>> the checksum verification in tcp_header() fails, printing "nf_ct_tcp:
>>> bad TCP checksum". Extra zero bytes don't affect the checksum, but the
>>> length in the IP pseudoheader does. That length is based on skb->len,
>>> and without trimming, it doesn't match the length the sender used when
>>> computing the checksum.
>>>
>>> With this change, OVS conntrack trims IPv4 and IPv6 packets prior to
>>> L3 processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> net/openvswitch/conntrack.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index d558e882ca0c..3a7c9215c431 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -1105,12 +1105,29 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>>> const struct ovs_conntrack_info *info)
>>> {
>>> int nh_ofs;
>>> + unsigned int nh_len;
>>> int err;
>>>
>>> /* The conntrack module expects to be working at L3. */
>>> nh_ofs = skb_network_offset(skb);
>>> skb_pull_rcsum(skb, nh_ofs);
>>>
>>> + /* Trim to L3 length since nf_checksum() doesn't expect padding. */
>> Can you explore if nf_checksum can be changed to avoid the padding?
>
> The nf_ip_checksum() and nf_ip6_checksum() helper functions can easily
> be changed to avoid the padding.
>
> My worry is that conntrack is just one of many netfilter hooks that
> perform L3+ processing, and may assume that once skb->data points to
> the L3 header, skb->len reflects the length of the L3 header and
> payload. For example, in nf_conntrack_ftp.c, help() uses skb->len to
> determine the length of the FTP payload and the TCP sequence number of
> the next packet; this would be thrown off by lower-layer padding.
>
> br_netfilter, a cousin of OVS, has always preserved this
> assumption--like ip_rcv() and ip6_rcv(), br_validate_ipv4() and
> br_validate_ipv6() trim the skb to the L3 length before they invoke
> NF_INET_PRE_ROUTING hooks. Modifying OVS to fit the mold seems more
> straightforward than changing this assumption.
>
we could avoid extra processing in fast path, thats why I wanted to
explore this, But if it is too complex, I am fine with this patch.
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>>> + return err;
>>> +
>> In case of error skb needs to be freed.
>
> Thanks, I will fix this.
>
> --Ed
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] openvswitch: Trim off padding before L3 conntrack processing
2017-12-14 20:05 ` Ed Swierk
2017-12-17 19:22 ` Pravin Shelar
@ 2017-12-17 19:22 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2017-12-17 19:22 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Lance Richardson,
Benjamin Warren, Keith Holleman
On Thu, Dec 14, 2017 at 12:05 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Wed, Dec 13, 2017 at 4:58 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Tue, Dec 12, 2017 at 8:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> A short IPv4 packet may have up to 6 bytes of padding following the IP
>>> payload when received on an Ethernet device.
>>>
>>> In the normal IPv4 receive path, ip_rcv() trims the packet to
>>> ip_hdr->tot_len before invoking NF_INET_PRE_ROUTING hooks (including
>>> conntrack). Then any subsequent L3+ processing steps, like
>>> nf_checksum(), use skb->len as the length of the packet, rather than
>>> referring back to ip_hdr->tot_len. In the IPv6 receive path, ip6_rcv()
>>> does the same using ipv6_hdr->payload_len.
>>>
>>> In the OVS conntrack receive path, this trimming does not occur, so
>>> the checksum verification in tcp_header() fails, printing "nf_ct_tcp:
>>> bad TCP checksum". Extra zero bytes don't affect the checksum, but the
>>> length in the IP pseudoheader does. That length is based on skb->len,
>>> and without trimming, it doesn't match the length the sender used when
>>> computing the checksum.
>>>
>>> With this change, OVS conntrack trims IPv4 and IPv6 packets prior to
>>> L3 processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> net/openvswitch/conntrack.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index d558e882ca0c..3a7c9215c431 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -1105,12 +1105,29 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>>> const struct ovs_conntrack_info *info)
>>> {
>>> int nh_ofs;
>>> + unsigned int nh_len;
>>> int err;
>>>
>>> /* The conntrack module expects to be working at L3. */
>>> nh_ofs = skb_network_offset(skb);
>>> skb_pull_rcsum(skb, nh_ofs);
>>>
>>> + /* Trim to L3 length since nf_checksum() doesn't expect padding. */
>> Can you explore if nf_checksum can be changed to avoid the padding?
>
> The nf_ip_checksum() and nf_ip6_checksum() helper functions can easily
> be changed to avoid the padding.
>
> My worry is that conntrack is just one of many netfilter hooks that
> perform L3+ processing, and may assume that once skb->data points to
> the L3 header, skb->len reflects the length of the L3 header and
> payload. For example, in nf_conntrack_ftp.c, help() uses skb->len to
> determine the length of the FTP payload and the TCP sequence number of
> the next packet; this would be thrown off by lower-layer padding.
>
> br_netfilter, a cousin of OVS, has always preserved this
> assumption--like ip_rcv() and ip6_rcv(), br_validate_ipv4() and
> br_validate_ipv6() trim the skb to the L3 length before they invoke
> NF_INET_PRE_ROUTING hooks. Modifying OVS to fit the mold seems more
> straightforward than changing this assumption.
>
we could avoid extra processing in fast path, thats why I wanted to
explore this, But if it is too complex, I am fine with this patch.
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>>> + return err;
>>> +
>> In case of error skb needs to be freed.
>
> Thanks, I will fix this.
>
> --Ed
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-12 16:17 [PATCH] openvswitch: Trim off padding before L3 conntrack processing Ed Swierk
2017-12-14 0:58 ` Pravin Shelar
@ 2017-12-21 15:17 ` Ed Swierk
2017-12-22 23:31 ` Pravin Shelar
2017-12-22 23:31 ` Pravin Shelar
2017-12-21 15:21 ` [PATCH v2 RESEND] " Ed Swierk
2017-12-21 15:21 ` Ed Swierk
3 siblings, 2 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-21 15:17 UTC (permalink / raw)
To: ovs-dev, netdev, Pravin Shelar; +Cc: Ed Swierk, Benjamin Warren, Keith Holleman
IPv4 and IPv6 packets may arrive with lower-layer padding that is not
included in the L3 length. For example, a short IPv4 packet may have
up to 6 bytes of padding following the IP payload when received on an
Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
packet to ip_hdr->tot_len before invoking netfilter hooks (including
conntrack and nat).
In the IPv6 receive path, ip6_rcv() does the same using
ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
length before invoking NF_INET_PRE_ROUTING hooks.
In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
the L3 header but does not trim it to the L3 length before calling
nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
encounters a packet with lower-layer padding, nf_checksum() fails and
logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
affect the checksum, the length in the IP pseudoheader does. That
length is based on skb->len, and without trimming, it doesn't match
the length the sender used when computing the checksum.
The assumption throughout nf_conntrack and nf_nat is that skb->len
reflects the length of the L3 header and payload, so there is no need
to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
This change brings OVS into line with other netfilter users, trimming
IPv4 and IPv6 packets prior to L3+ netfilter processing.
Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
v2:
- Trim packet in nat receive path as well as conntrack
- Free skb on error
---
net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..1bdc78f 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
return ct_executed;
}
+/* Trim the skb to the L3 length. Assumes the skb is already pulled to
+ * the L3 header. The skb is freed on error.
+ */
+static int skb_trim_l3(struct sk_buff *skb)
+{
+ unsigned int nh_len;
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ nh_len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ nh_len = ntohs(ipv6_hdr(skb)->payload_len)
+ + sizeof(struct ipv6hdr);
+ break;
+ default:
+ nh_len = skb->len;
+ }
+
+ err = pskb_trim_rcsum(skb, nh_len);
+ if (err)
+ kfree_skb(skb);
+
+ return err;
+}
+
#ifdef CONFIG_NF_NAT_NEEDED
/* Modelled after nf_nat_ipv[46]_fn().
* range is only used for new, uninitialized NAT state.
@@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
{
int hooknum, nh_off, err = NF_ACCEPT;
+ /* The nat module expects to be working at L3. */
nh_off = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_off);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
/* See HOOK2MANIP(). */
if (maniptype == NF_NAT_MANIP_SRC)
@@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
/* The conntrack module expects to be working at L3. */
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 RESEND] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-12 16:17 [PATCH] openvswitch: Trim off padding before L3 conntrack processing Ed Swierk
` (2 preceding siblings ...)
2017-12-21 15:21 ` [PATCH v2 RESEND] " Ed Swierk
@ 2017-12-21 15:21 ` Ed Swierk
3 siblings, 0 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-21 15:21 UTC (permalink / raw)
To: ovs-dev, netdev, Pravin Shelar; +Cc: Ed Swierk, Benjamin Warren, Keith Holleman
IPv4 and IPv6 packets may arrive with lower-layer padding that is not
included in the L3 length. For example, a short IPv4 packet may have
up to 6 bytes of padding following the IP payload when received on an
Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
packet to ip_hdr->tot_len before invoking netfilter hooks (including
conntrack and nat).
In the IPv6 receive path, ip6_rcv() does the same using
ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
length before invoking NF_INET_PRE_ROUTING hooks.
In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
the L3 header but does not trim it to the L3 length before calling
nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
encounters a packet with lower-layer padding, nf_checksum() fails and
logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
affect the checksum, the length in the IP pseudoheader does. That
length is based on skb->len, and without trimming, it doesn't match
the length the sender used when computing the checksum.
The assumption throughout nf_conntrack and nf_nat is that skb->len
reflects the length of the L3 header and payload, so there is no need
to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
This change brings OVS into line with other netfilter users, trimming
IPv4 and IPv6 packets prior to L3+ netfilter processing.
Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
(resent with Pravin's correct address)
v2:
- Trim packet in nat receive path as well as conntrack
- Free skb on error
---
net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..1bdc78f 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
return ct_executed;
}
+/* Trim the skb to the L3 length. Assumes the skb is already pulled to
+ * the L3 header. The skb is freed on error.
+ */
+static int skb_trim_l3(struct sk_buff *skb)
+{
+ unsigned int nh_len;
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ nh_len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ nh_len = ntohs(ipv6_hdr(skb)->payload_len)
+ + sizeof(struct ipv6hdr);
+ break;
+ default:
+ nh_len = skb->len;
+ }
+
+ err = pskb_trim_rcsum(skb, nh_len);
+ if (err)
+ kfree_skb(skb);
+
+ return err;
+}
+
#ifdef CONFIG_NF_NAT_NEEDED
/* Modelled after nf_nat_ipv[46]_fn().
* range is only used for new, uninitialized NAT state.
@@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
{
int hooknum, nh_off, err = NF_ACCEPT;
+ /* The nat module expects to be working at L3. */
nh_off = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_off);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
/* See HOOK2MANIP(). */
if (maniptype == NF_NAT_MANIP_SRC)
@@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
/* The conntrack module expects to be working at L3. */
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 RESEND] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-12 16:17 [PATCH] openvswitch: Trim off padding before L3 conntrack processing Ed Swierk
2017-12-14 0:58 ` Pravin Shelar
2017-12-21 15:17 ` [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Ed Swierk
@ 2017-12-21 15:21 ` Ed Swierk
2017-12-21 15:21 ` Ed Swierk
3 siblings, 0 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-21 15:21 UTC (permalink / raw)
To: ovs-dev, netdev, Pravin Shelar; +Cc: Ed Swierk, Benjamin Warren, Keith Holleman
IPv4 and IPv6 packets may arrive with lower-layer padding that is not
included in the L3 length. For example, a short IPv4 packet may have
up to 6 bytes of padding following the IP payload when received on an
Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
packet to ip_hdr->tot_len before invoking netfilter hooks (including
conntrack and nat).
In the IPv6 receive path, ip6_rcv() does the same using
ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
length before invoking NF_INET_PRE_ROUTING hooks.
In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
the L3 header but does not trim it to the L3 length before calling
nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
encounters a packet with lower-layer padding, nf_checksum() fails and
logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
affect the checksum, the length in the IP pseudoheader does. That
length is based on skb->len, and without trimming, it doesn't match
the length the sender used when computing the checksum.
The assumption throughout nf_conntrack and nf_nat is that skb->len
reflects the length of the L3 header and payload, so there is no need
to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
This change brings OVS into line with other netfilter users, trimming
IPv4 and IPv6 packets prior to L3+ netfilter processing.
Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
---
(resent with Pravin's correct address)
v2:
- Trim packet in nat receive path as well as conntrack
- Free skb on error
---
net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..1bdc78f 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
return ct_executed;
}
+/* Trim the skb to the L3 length. Assumes the skb is already pulled to
+ * the L3 header. The skb is freed on error.
+ */
+static int skb_trim_l3(struct sk_buff *skb)
+{
+ unsigned int nh_len;
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ nh_len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ nh_len = ntohs(ipv6_hdr(skb)->payload_len)
+ + sizeof(struct ipv6hdr);
+ break;
+ default:
+ nh_len = skb->len;
+ }
+
+ err = pskb_trim_rcsum(skb, nh_len);
+ if (err)
+ kfree_skb(skb);
+
+ return err;
+}
+
#ifdef CONFIG_NF_NAT_NEEDED
/* Modelled after nf_nat_ipv[46]_fn().
* range is only used for new, uninitialized NAT state.
@@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
{
int hooknum, nh_off, err = NF_ACCEPT;
+ /* The nat module expects to be working at L3. */
nh_off = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_off);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
/* See HOOK2MANIP(). */
if (maniptype == NF_NAT_MANIP_SRC)
@@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
/* The conntrack module expects to be working at L3. */
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ err = skb_trim_l3(skb);
+ if (err)
+ return err;
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-21 15:17 ` [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Ed Swierk
@ 2017-12-22 23:31 ` Pravin Shelar
2017-12-22 23:31 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2017-12-22 23:31 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
> included in the L3 length. For example, a short IPv4 packet may have
> up to 6 bytes of padding following the IP payload when received on an
> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
> packet to ip_hdr->tot_len before invoking netfilter hooks (including
> conntrack and nat).
>
> In the IPv6 receive path, ip6_rcv() does the same using
> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
> length before invoking NF_INET_PRE_ROUTING hooks.
>
> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
> the L3 header but does not trim it to the L3 length before calling
> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
> encounters a packet with lower-layer padding, nf_checksum() fails and
> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
> affect the checksum, the length in the IP pseudoheader does. That
> length is based on skb->len, and without trimming, it doesn't match
> the length the sender used when computing the checksum.
>
> The assumption throughout nf_conntrack and nf_nat is that skb->len
> reflects the length of the L3 header and payload, so there is no need
> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>
> This change brings OVS into line with other netfilter users, trimming
> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
> v2:
> - Trim packet in nat receive path as well as conntrack
> - Free skb on error
> ---
> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index b27c5c6..1bdc78f 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
> return ct_executed;
> }
>
> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
> + * the L3 header. The skb is freed on error.
> + */
> +static int skb_trim_l3(struct sk_buff *skb)
> +{
> + unsigned int nh_len;
> + int err;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + nh_len = ntohs(ip_hdr(skb)->tot_len);
> + break;
> + case htons(ETH_P_IPV6):
> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
> + + sizeof(struct ipv6hdr);
> + break;
> + default:
> + nh_len = skb->len;
> + }
> +
> + err = pskb_trim_rcsum(skb, nh_len);
> + if (err)
This should is unlikely.
> + kfree_skb(skb);
> +
> + return err;
> +}
> +
This looks like a generic function, it probably does not belong to OVS
code base.
> #ifdef CONFIG_NF_NAT_NEEDED
> /* Modelled after nf_nat_ipv[46]_fn().
> * range is only used for new, uninitialized NAT state.
> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> {
> int hooknum, nh_off, err = NF_ACCEPT;
>
> + /* The nat module expects to be working at L3. */
> nh_off = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_off);
> + err = skb_trim_l3(skb);
> + if (err)
> + return err;
>
ct-nat is executed within ct action, so I do not see why you you call
skb-trim again from ovs_ct_nat_execute().
ovs_ct_execute() trim should take care of the skb.
> /* See HOOK2MANIP(). */
> if (maniptype == NF_NAT_MANIP_SRC)
> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> /* The conntrack module expects to be working at L3. */
> nh_ofs = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_ofs);
> + err = skb_trim_l3(skb);
> + if (err)
> + return err;
>
> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
> err = handle_fragments(net, key, info->zone.id, skb);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-21 15:17 ` [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Ed Swierk
2017-12-22 23:31 ` Pravin Shelar
@ 2017-12-22 23:31 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
` (2 more replies)
1 sibling, 3 replies; 26+ messages in thread
From: Pravin Shelar @ 2017-12-22 23:31 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
> included in the L3 length. For example, a short IPv4 packet may have
> up to 6 bytes of padding following the IP payload when received on an
> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
> packet to ip_hdr->tot_len before invoking netfilter hooks (including
> conntrack and nat).
>
> In the IPv6 receive path, ip6_rcv() does the same using
> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
> length before invoking NF_INET_PRE_ROUTING hooks.
>
> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
> the L3 header but does not trim it to the L3 length before calling
> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
> encounters a packet with lower-layer padding, nf_checksum() fails and
> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
> affect the checksum, the length in the IP pseudoheader does. That
> length is based on skb->len, and without trimming, it doesn't match
> the length the sender used when computing the checksum.
>
> The assumption throughout nf_conntrack and nf_nat is that skb->len
> reflects the length of the L3 header and payload, so there is no need
> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>
> This change brings OVS into line with other netfilter users, trimming
> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>
> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> ---
> v2:
> - Trim packet in nat receive path as well as conntrack
> - Free skb on error
> ---
> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index b27c5c6..1bdc78f 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
> return ct_executed;
> }
>
> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
> + * the L3 header. The skb is freed on error.
> + */
> +static int skb_trim_l3(struct sk_buff *skb)
> +{
> + unsigned int nh_len;
> + int err;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + nh_len = ntohs(ip_hdr(skb)->tot_len);
> + break;
> + case htons(ETH_P_IPV6):
> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
> + + sizeof(struct ipv6hdr);
> + break;
> + default:
> + nh_len = skb->len;
> + }
> +
> + err = pskb_trim_rcsum(skb, nh_len);
> + if (err)
This should is unlikely.
> + kfree_skb(skb);
> +
> + return err;
> +}
> +
This looks like a generic function, it probably does not belong to OVS
code base.
> #ifdef CONFIG_NF_NAT_NEEDED
> /* Modelled after nf_nat_ipv[46]_fn().
> * range is only used for new, uninitialized NAT state.
> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> {
> int hooknum, nh_off, err = NF_ACCEPT;
>
> + /* The nat module expects to be working at L3. */
> nh_off = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_off);
> + err = skb_trim_l3(skb);
> + if (err)
> + return err;
>
ct-nat is executed within ct action, so I do not see why you you call
skb-trim again from ovs_ct_nat_execute().
ovs_ct_execute() trim should take care of the skb.
> /* See HOOK2MANIP(). */
> if (maniptype == NF_NAT_MANIP_SRC)
> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> /* The conntrack module expects to be working at L3. */
> nh_ofs = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_ofs);
> + err = skb_trim_l3(skb);
> + if (err)
> + return err;
>
> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
> err = handle_fragments(net, key, info->zone.id, skb);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-22 23:31 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
@ 2017-12-23 0:39 ` Ed Swierk
2018-01-04 3:49 ` Ed Swierk
2 siblings, 0 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-23 0:39 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>> included in the L3 length. For example, a short IPv4 packet may have
>> up to 6 bytes of padding following the IP payload when received on an
>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>> conntrack and nat).
>>
>> In the IPv6 receive path, ip6_rcv() does the same using
>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>> length before invoking NF_INET_PRE_ROUTING hooks.
>>
>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>> the L3 header but does not trim it to the L3 length before calling
>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>> encounters a packet with lower-layer padding, nf_checksum() fails and
>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>> affect the checksum, the length in the IP pseudoheader does. That
>> length is based on skb->len, and without trimming, it doesn't match
>> the length the sender used when computing the checksum.
>>
>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>> reflects the length of the L3 header and payload, so there is no need
>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>
>> This change brings OVS into line with other netfilter users, trimming
>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>
>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> ---
>> v2:
>> - Trim packet in nat receive path as well as conntrack
>> - Free skb on error
>> ---
>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index b27c5c6..1bdc78f 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>> return ct_executed;
>> }
>>
>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>> + * the L3 header. The skb is freed on error.
>> + */
>> +static int skb_trim_l3(struct sk_buff *skb)
>> +{
>> + unsigned int nh_len;
>> + int err;
>> +
>> + switch (skb->protocol) {
>> + case htons(ETH_P_IP):
>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>> + break;
>> + case htons(ETH_P_IPV6):
>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>> + + sizeof(struct ipv6hdr);
>> + break;
>> + default:
>> + nh_len = skb->len;
>> + }
>> +
>> + err = pskb_trim_rcsum(skb, nh_len);
>> + if (err)
> This should is unlikely.
I'll add unlikely().
>> + kfree_skb(skb);
>> +
>> + return err;
>> +}
>> +
> This looks like a generic function, it probably does not belong to OVS
> code base.
Indeed. I'll move it to skbuff.c, unless you have a better idea.
>> #ifdef CONFIG_NF_NAT_NEEDED
>> /* Modelled after nf_nat_ipv[46]_fn().
>> * range is only used for new, uninitialized NAT state.
>> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>> {
>> int hooknum, nh_off, err = NF_ACCEPT;
>>
>> + /* The nat module expects to be working at L3. */
>> nh_off = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_off);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
> ct-nat is executed within ct action, so I do not see why you you call
> skb-trim again from ovs_ct_nat_execute().
> ovs_ct_execute() trim should take care of the skb.
I see. Doesn't that mean that skb_pull_rcsum() is also unnecessary in
ovs_ct_nat_execute(), as ovs_ct_execute() has already pulled the skb
to the L3 header?
>> /* See HOOK2MANIP(). */
>> if (maniptype == NF_NAT_MANIP_SRC)
>> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>> /* The conntrack module expects to be working at L3. */
>> nh_ofs = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_ofs);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
>> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
>> err = handle_fragments(net, key, info->zone.id, skb);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-22 23:31 ` Pravin Shelar
@ 2017-12-23 0:39 ` Ed Swierk
2018-01-03 6:21 ` Pravin Shelar
2018-01-03 6:21 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
2018-01-04 3:49 ` Ed Swierk
2 siblings, 2 replies; 26+ messages in thread
From: Ed Swierk @ 2017-12-23 0:39 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>> included in the L3 length. For example, a short IPv4 packet may have
>> up to 6 bytes of padding following the IP payload when received on an
>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>> conntrack and nat).
>>
>> In the IPv6 receive path, ip6_rcv() does the same using
>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>> length before invoking NF_INET_PRE_ROUTING hooks.
>>
>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>> the L3 header but does not trim it to the L3 length before calling
>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>> encounters a packet with lower-layer padding, nf_checksum() fails and
>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>> affect the checksum, the length in the IP pseudoheader does. That
>> length is based on skb->len, and without trimming, it doesn't match
>> the length the sender used when computing the checksum.
>>
>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>> reflects the length of the L3 header and payload, so there is no need
>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>
>> This change brings OVS into line with other netfilter users, trimming
>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>
>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> ---
>> v2:
>> - Trim packet in nat receive path as well as conntrack
>> - Free skb on error
>> ---
>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index b27c5c6..1bdc78f 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>> return ct_executed;
>> }
>>
>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>> + * the L3 header. The skb is freed on error.
>> + */
>> +static int skb_trim_l3(struct sk_buff *skb)
>> +{
>> + unsigned int nh_len;
>> + int err;
>> +
>> + switch (skb->protocol) {
>> + case htons(ETH_P_IP):
>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>> + break;
>> + case htons(ETH_P_IPV6):
>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>> + + sizeof(struct ipv6hdr);
>> + break;
>> + default:
>> + nh_len = skb->len;
>> + }
>> +
>> + err = pskb_trim_rcsum(skb, nh_len);
>> + if (err)
> This should is unlikely.
I'll add unlikely().
>> + kfree_skb(skb);
>> +
>> + return err;
>> +}
>> +
> This looks like a generic function, it probably does not belong to OVS
> code base.
Indeed. I'll move it to skbuff.c, unless you have a better idea.
>> #ifdef CONFIG_NF_NAT_NEEDED
>> /* Modelled after nf_nat_ipv[46]_fn().
>> * range is only used for new, uninitialized NAT state.
>> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>> {
>> int hooknum, nh_off, err = NF_ACCEPT;
>>
>> + /* The nat module expects to be working at L3. */
>> nh_off = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_off);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
> ct-nat is executed within ct action, so I do not see why you you call
> skb-trim again from ovs_ct_nat_execute().
> ovs_ct_execute() trim should take care of the skb.
I see. Doesn't that mean that skb_pull_rcsum() is also unnecessary in
ovs_ct_nat_execute(), as ovs_ct_execute() has already pulled the skb
to the L3 header?
>> /* See HOOK2MANIP(). */
>> if (maniptype == NF_NAT_MANIP_SRC)
>> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>> /* The conntrack module expects to be working at L3. */
>> nh_ofs = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_ofs);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
>> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
>> err = handle_fragments(net, key, info->zone.id, skb);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-23 0:39 ` Ed Swierk
2018-01-03 6:21 ` Pravin Shelar
@ 2018-01-03 6:21 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-03 6:21 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Dec 22, 2017 at 4:39 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>> included in the L3 length. For example, a short IPv4 packet may have
>>> up to 6 bytes of padding following the IP payload when received on an
>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>> conntrack and nat).
>>>
>>> In the IPv6 receive path, ip6_rcv() does the same using
>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>
>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>> the L3 header but does not trim it to the L3 length before calling
>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>> affect the checksum, the length in the IP pseudoheader does. That
>>> length is based on skb->len, and without trimming, it doesn't match
>>> the length the sender used when computing the checksum.
>>>
>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>> reflects the length of the L3 header and payload, so there is no need
>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>
>>> This change brings OVS into line with other netfilter users, trimming
>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> v2:
>>> - Trim packet in nat receive path as well as conntrack
>>> - Free skb on error
>>> ---
>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index b27c5c6..1bdc78f 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>> return ct_executed;
>>> }
>>>
>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>> + * the L3 header. The skb is freed on error.
>>> + */
>>> +static int skb_trim_l3(struct sk_buff *skb)
>>> +{
>>> + unsigned int nh_len;
>>> + int err;
>>> +
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> +
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>> This should is unlikely.
>
> I'll add unlikely().
>
>>> + kfree_skb(skb);
>>> +
>>> + return err;
>>> +}
>>> +
>> This looks like a generic function, it probably does not belong to OVS
>> code base.
>
> Indeed. I'll move it to skbuff.c, unless you have a better idea.
>
>>> #ifdef CONFIG_NF_NAT_NEEDED
>>> /* Modelled after nf_nat_ipv[46]_fn().
>>> * range is only used for new, uninitialized NAT state.
>>> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>> {
>>> int hooknum, nh_off, err = NF_ACCEPT;
>>>
>>> + /* The nat module expects to be working at L3. */
>>> nh_off = skb_network_offset(skb);
>>> skb_pull_rcsum(skb, nh_off);
>>> + err = skb_trim_l3(skb);
>>> + if (err)
>>> + return err;
>>>
>> ct-nat is executed within ct action, so I do not see why you you call
>> skb-trim again from ovs_ct_nat_execute().
>> ovs_ct_execute() trim should take care of the skb.
>
> I see. Doesn't that mean that skb_pull_rcsum() is also unnecessary in
> ovs_ct_nat_execute(), as ovs_ct_execute() has already pulled the skb
> to the L3 header?
>
Yes, It looks redundant. but lets address it in separate patch.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-23 0:39 ` Ed Swierk
@ 2018-01-03 6:21 ` Pravin Shelar
2018-01-03 6:21 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-03 6:21 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Dec 22, 2017 at 4:39 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>> included in the L3 length. For example, a short IPv4 packet may have
>>> up to 6 bytes of padding following the IP payload when received on an
>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>> conntrack and nat).
>>>
>>> In the IPv6 receive path, ip6_rcv() does the same using
>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>
>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>> the L3 header but does not trim it to the L3 length before calling
>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>> affect the checksum, the length in the IP pseudoheader does. That
>>> length is based on skb->len, and without trimming, it doesn't match
>>> the length the sender used when computing the checksum.
>>>
>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>> reflects the length of the L3 header and payload, so there is no need
>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>
>>> This change brings OVS into line with other netfilter users, trimming
>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> v2:
>>> - Trim packet in nat receive path as well as conntrack
>>> - Free skb on error
>>> ---
>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index b27c5c6..1bdc78f 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>> return ct_executed;
>>> }
>>>
>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>> + * the L3 header. The skb is freed on error.
>>> + */
>>> +static int skb_trim_l3(struct sk_buff *skb)
>>> +{
>>> + unsigned int nh_len;
>>> + int err;
>>> +
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> +
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>> This should is unlikely.
>
> I'll add unlikely().
>
>>> + kfree_skb(skb);
>>> +
>>> + return err;
>>> +}
>>> +
>> This looks like a generic function, it probably does not belong to OVS
>> code base.
>
> Indeed. I'll move it to skbuff.c, unless you have a better idea.
>
>>> #ifdef CONFIG_NF_NAT_NEEDED
>>> /* Modelled after nf_nat_ipv[46]_fn().
>>> * range is only used for new, uninitialized NAT state.
>>> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>>> {
>>> int hooknum, nh_off, err = NF_ACCEPT;
>>>
>>> + /* The nat module expects to be working at L3. */
>>> nh_off = skb_network_offset(skb);
>>> skb_pull_rcsum(skb, nh_off);
>>> + err = skb_trim_l3(skb);
>>> + if (err)
>>> + return err;
>>>
>> ct-nat is executed within ct action, so I do not see why you you call
>> skb-trim again from ovs_ct_nat_execute().
>> ovs_ct_execute() trim should take care of the skb.
>
> I see. Doesn't that mean that skb_pull_rcsum() is also unnecessary in
> ovs_ct_nat_execute(), as ovs_ct_execute() has already pulled the skb
> to the L3 header?
>
Yes, It looks redundant. but lets address it in separate patch.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2017-12-22 23:31 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
2017-12-23 0:39 ` Ed Swierk
@ 2018-01-04 3:49 ` Ed Swierk
2018-01-05 3:36 ` Pravin Shelar
2018-01-05 3:36 ` Pravin Shelar
2 siblings, 2 replies; 26+ messages in thread
From: Ed Swierk @ 2018-01-04 3:49 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>> included in the L3 length. For example, a short IPv4 packet may have
>> up to 6 bytes of padding following the IP payload when received on an
>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>> conntrack and nat).
>>
>> In the IPv6 receive path, ip6_rcv() does the same using
>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>> length before invoking NF_INET_PRE_ROUTING hooks.
>>
>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>> the L3 header but does not trim it to the L3 length before calling
>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>> encounters a packet with lower-layer padding, nf_checksum() fails and
>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>> affect the checksum, the length in the IP pseudoheader does. That
>> length is based on skb->len, and without trimming, it doesn't match
>> the length the sender used when computing the checksum.
>>
>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>> reflects the length of the L3 header and payload, so there is no need
>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>
>> This change brings OVS into line with other netfilter users, trimming
>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>
>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>> ---
>> v2:
>> - Trim packet in nat receive path as well as conntrack
>> - Free skb on error
>> ---
>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index b27c5c6..1bdc78f 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>> return ct_executed;
>> }
>>
>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>> + * the L3 header. The skb is freed on error.
>> + */
>> +static int skb_trim_l3(struct sk_buff *skb)
>> +{
>> + unsigned int nh_len;
>> + int err;
>> +
>> + switch (skb->protocol) {
>> + case htons(ETH_P_IP):
>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>> + break;
>> + case htons(ETH_P_IPV6):
>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>> + + sizeof(struct ipv6hdr);
>> + break;
>> + default:
>> + nh_len = skb->len;
>> + }
>> +
>> + err = pskb_trim_rcsum(skb, nh_len);
>> + if (err)
> This should is unlikely.
>> + kfree_skb(skb);
>> +
>> + return err;
>> +}
>> +
> This looks like a generic function, it probably does not belong to OVS
> code base.
It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
the IP header is actually there; and for IPv4 it should validate the
IP header checksum, including options. Once we add all these steps,
skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
and ip6_rcv(). It would be nice to avoid duplicating this logic yet
again.
What if we turn br_validate_ipv4() and br_validate_ipv6() into generic
functions and call them from both br_netfilter and ovs_ct--should
there be any fundamental difference between these two receive paths,
at least for L3+ conntrack processing?
For example, currently br_netfilter updates the
IPSTATS_MIB_INTRUNCATEDPKTS and IPSTATS_MIB_INDISCARDS counters. It
would be easy to make this conditional in a generic function, if we
still don't want ovs_ct to update those counters.
>> #ifdef CONFIG_NF_NAT_NEEDED
>> /* Modelled after nf_nat_ipv[46]_fn().
>> * range is only used for new, uninitialized NAT state.
>> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
>> {
>> int hooknum, nh_off, err = NF_ACCEPT;
>>
>> + /* The nat module expects to be working at L3. */
>> nh_off = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_off);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
> ct-nat is executed within ct action, so I do not see why you you call
> skb-trim again from ovs_ct_nat_execute().
> ovs_ct_execute() trim should take care of the skb.
>
>> /* See HOOK2MANIP(). */
>> if (maniptype == NF_NAT_MANIP_SRC)
>> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>> /* The conntrack module expects to be working at L3. */
>> nh_ofs = skb_network_offset(skb);
>> skb_pull_rcsum(skb, nh_ofs);
>> + err = skb_trim_l3(skb);
>> + if (err)
>> + return err;
>>
>> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
>> err = handle_fragments(net, key, info->zone.id, skb);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-04 3:49 ` Ed Swierk
@ 2018-01-05 3:36 ` Pravin Shelar
2018-01-05 3:36 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-05 3:36 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>> included in the L3 length. For example, a short IPv4 packet may have
>>> up to 6 bytes of padding following the IP payload when received on an
>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>> conntrack and nat).
>>>
>>> In the IPv6 receive path, ip6_rcv() does the same using
>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>
>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>> the L3 header but does not trim it to the L3 length before calling
>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>> affect the checksum, the length in the IP pseudoheader does. That
>>> length is based on skb->len, and without trimming, it doesn't match
>>> the length the sender used when computing the checksum.
>>>
>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>> reflects the length of the L3 header and payload, so there is no need
>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>
>>> This change brings OVS into line with other netfilter users, trimming
>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> v2:
>>> - Trim packet in nat receive path as well as conntrack
>>> - Free skb on error
>>> ---
>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index b27c5c6..1bdc78f 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>> return ct_executed;
>>> }
>>>
>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>> + * the L3 header. The skb is freed on error.
>>> + */
>>> +static int skb_trim_l3(struct sk_buff *skb)
>>> +{
>>> + unsigned int nh_len;
>>> + int err;
>>> +
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> +
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>> This should is unlikely.
>>> + kfree_skb(skb);
>>> +
>>> + return err;
>>> +}
>>> +
>> This looks like a generic function, it probably does not belong to OVS
>> code base.
>
> It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
> before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
> the IP header is actually there; and for IPv4 it should validate the
> IP header checksum, including options. Once we add all these steps,
> skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
> br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
> and ip6_rcv(). It would be nice to avoid duplicating this logic yet
> again.
>
OVS already pull all required headers in skb linear data, so no need
to redo all of it. only check required is the ip-checksum validation.
I think we could avoid it in most of cases by checking skb length to
ipheader length before verifying the ip header-checksum.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-04 3:49 ` Ed Swierk
2018-01-05 3:36 ` Pravin Shelar
@ 2018-01-05 3:36 ` Pravin Shelar
2018-01-05 18:14 ` Ed Swierk
2018-01-05 18:14 ` Ed Swierk
1 sibling, 2 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-05 3:36 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>> included in the L3 length. For example, a short IPv4 packet may have
>>> up to 6 bytes of padding following the IP payload when received on an
>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>> conntrack and nat).
>>>
>>> In the IPv6 receive path, ip6_rcv() does the same using
>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>
>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>> the L3 header but does not trim it to the L3 length before calling
>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>> affect the checksum, the length in the IP pseudoheader does. That
>>> length is based on skb->len, and without trimming, it doesn't match
>>> the length the sender used when computing the checksum.
>>>
>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>> reflects the length of the L3 header and payload, so there is no need
>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>
>>> This change brings OVS into line with other netfilter users, trimming
>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>> ---
>>> v2:
>>> - Trim packet in nat receive path as well as conntrack
>>> - Free skb on error
>>> ---
>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 34 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index b27c5c6..1bdc78f 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>> return ct_executed;
>>> }
>>>
>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>> + * the L3 header. The skb is freed on error.
>>> + */
>>> +static int skb_trim_l3(struct sk_buff *skb)
>>> +{
>>> + unsigned int nh_len;
>>> + int err;
>>> +
>>> + switch (skb->protocol) {
>>> + case htons(ETH_P_IP):
>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> + break;
>>> + case htons(ETH_P_IPV6):
>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> + + sizeof(struct ipv6hdr);
>>> + break;
>>> + default:
>>> + nh_len = skb->len;
>>> + }
>>> +
>>> + err = pskb_trim_rcsum(skb, nh_len);
>>> + if (err)
>> This should is unlikely.
>>> + kfree_skb(skb);
>>> +
>>> + return err;
>>> +}
>>> +
>> This looks like a generic function, it probably does not belong to OVS
>> code base.
>
> It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
> before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
> the IP header is actually there; and for IPv4 it should validate the
> IP header checksum, including options. Once we add all these steps,
> skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
> br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
> and ip6_rcv(). It would be nice to avoid duplicating this logic yet
> again.
>
OVS already pull all required headers in skb linear data, so no need
to redo all of it. only check required is the ip-checksum validation.
I think we could avoid it in most of cases by checking skb length to
ipheader length before verifying the ip header-checksum.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-05 3:36 ` Pravin Shelar
2018-01-05 18:14 ` Ed Swierk
@ 2018-01-05 18:14 ` Ed Swierk
1 sibling, 0 replies; 26+ messages in thread
From: Ed Swierk @ 2018-01-05 18:14 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>>> included in the L3 length. For example, a short IPv4 packet may have
>>>> up to 6 bytes of padding following the IP payload when received on an
>>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>>> conntrack and nat).
>>>>
>>>> In the IPv6 receive path, ip6_rcv() does the same using
>>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>>
>>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>>> the L3 header but does not trim it to the L3 length before calling
>>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>>> affect the checksum, the length in the IP pseudoheader does. That
>>>> length is based on skb->len, and without trimming, it doesn't match
>>>> the length the sender used when computing the checksum.
>>>>
>>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>>> reflects the length of the L3 header and payload, so there is no need
>>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>>
>>>> This change brings OVS into line with other netfilter users, trimming
>>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>>
>>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>>> ---
>>>> v2:
>>>> - Trim packet in nat receive path as well as conntrack
>>>> - Free skb on error
>>>> ---
>>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>>> index b27c5c6..1bdc78f 100644
>>>> --- a/net/openvswitch/conntrack.c
>>>> +++ b/net/openvswitch/conntrack.c
>>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>>> return ct_executed;
>>>> }
>>>>
>>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>>> + * the L3 header. The skb is freed on error.
>>>> + */
>>>> +static int skb_trim_l3(struct sk_buff *skb)
>>>> +{
>>>> + unsigned int nh_len;
>>>> + int err;
>>>> +
>>>> + switch (skb->protocol) {
>>>> + case htons(ETH_P_IP):
>>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>>> + break;
>>>> + case htons(ETH_P_IPV6):
>>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>>> + + sizeof(struct ipv6hdr);
>>>> + break;
>>>> + default:
>>>> + nh_len = skb->len;
>>>> + }
>>>> +
>>>> + err = pskb_trim_rcsum(skb, nh_len);
>>>> + if (err)
>>> This should is unlikely.
>>>> + kfree_skb(skb);
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>> This looks like a generic function, it probably does not belong to OVS
>>> code base.
>>
>> It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
>> before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
>> the IP header is actually there; and for IPv4 it should validate the
>> IP header checksum, including options. Once we add all these steps,
>> skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
>> br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
>> and ip6_rcv(). It would be nice to avoid duplicating this logic yet
>> again.
>>
> OVS already pull all required headers in skb linear data, so no need
> to redo all of it. only check required is the ip-checksum validation.
> I think we could avoid it in most of cases by checking skb length to
> ipheader length before verifying the ip header-checksum.
Shouldn't the IP header checksum be verified even earlier, like in
key_extract(), before actually using any of the fields in the IP
header?
And since key_extract() is already inspecting the IP/IPv6 header, it
would be a convenient spot to check whether the skb->len matches. If
there's a difference, it could record the number of bytes to trim in
an ovs_skb_cb field. Then ovs_ct_execute() would look at this field
and trim the skb only if necessary.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-05 3:36 ` Pravin Shelar
@ 2018-01-05 18:14 ` Ed Swierk
[not found] ` <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
2018-01-05 18:14 ` Ed Swierk
1 sibling, 1 reply; 26+ messages in thread
From: Ed Swierk @ 2018-01-05 18:14 UTC (permalink / raw)
To: Pravin Shelar
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
>>>> included in the L3 length. For example, a short IPv4 packet may have
>>>> up to 6 bytes of padding following the IP payload when received on an
>>>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the
>>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>>> conntrack and nat).
>>>>
>>>> In the IPv6 receive path, ip6_rcv() does the same using
>>>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path,
>>>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3
>>>> length before invoking NF_INET_PRE_ROUTING hooks.
>>>>
>>>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to
>>>> the L3 header but does not trim it to the L3 length before calling
>>>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp
>>>> encounters a packet with lower-layer padding, nf_checksum() fails and
>>>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't
>>>> affect the checksum, the length in the IP pseudoheader does. That
>>>> length is based on skb->len, and without trimming, it doesn't match
>>>> the length the sender used when computing the checksum.
>>>>
>>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>>> reflects the length of the L3 header and payload, so there is no need
>>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>>
>>>> This change brings OVS into line with other netfilter users, trimming
>>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>>
>>>> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
>>>> ---
>>>> v2:
>>>> - Trim packet in nat receive path as well as conntrack
>>>> - Free skb on error
>>>> ---
>>>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>>> index b27c5c6..1bdc78f 100644
>>>> --- a/net/openvswitch/conntrack.c
>>>> +++ b/net/openvswitch/conntrack.c
>>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>>> return ct_executed;
>>>> }
>>>>
>>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>>> + * the L3 header. The skb is freed on error.
>>>> + */
>>>> +static int skb_trim_l3(struct sk_buff *skb)
>>>> +{
>>>> + unsigned int nh_len;
>>>> + int err;
>>>> +
>>>> + switch (skb->protocol) {
>>>> + case htons(ETH_P_IP):
>>>> + nh_len = ntohs(ip_hdr(skb)->tot_len);
>>>> + break;
>>>> + case htons(ETH_P_IPV6):
>>>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>>> + + sizeof(struct ipv6hdr);
>>>> + break;
>>>> + default:
>>>> + nh_len = skb->len;
>>>> + }
>>>> +
>>>> + err = pskb_trim_rcsum(skb, nh_len);
>>>> + if (err)
>>> This should is unlikely.
>>>> + kfree_skb(skb);
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>> This looks like a generic function, it probably does not belong to OVS
>>> code base.
>>
>> It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
>> before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
>> the IP header is actually there; and for IPv4 it should validate the
>> IP header checksum, including options. Once we add all these steps,
>> skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
>> br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
>> and ip6_rcv(). It would be nice to avoid duplicating this logic yet
>> again.
>>
> OVS already pull all required headers in skb linear data, so no need
> to redo all of it. only check required is the ip-checksum validation.
> I think we could avoid it in most of cases by checking skb length to
> ipheader length before verifying the ip header-checksum.
Shouldn't the IP header checksum be verified even earlier, like in
key_extract(), before actually using any of the fields in the IP
header?
And since key_extract() is already inspecting the IP/IPv6 header, it
would be a convenient spot to check whether the skb->len matches. If
there's a difference, it could record the number of bytes to trim in
an ovs_skb_cb field. Then ovs_ct_execute() would look at this field
and trim the skb only if necessary.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
[not found] ` <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
@ 2018-01-06 6:17 ` Pravin Shelar
2018-01-06 6:17 ` Pravin Shelar
1 sibling, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-06 6:17 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
> wrote:
>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> OVS already pull all required headers in skb linear data, so no need
>>> to redo all of it. only check required is the ip-checksum validation.
>>> I think we could avoid it in most of cases by checking skb length to
>>> ipheader length before verifying the ip header-checksum.
>>
>> Shouldn't the IP header checksum be verified even earlier, like in
>> key_extract(), before actually using any of the fields in the IP
>> header?
>
> Something like this for verifying the IP header checksum (not tested):
>
AFAIU openflow does not need this verification, so it is not required
in flow extract.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
[not found] ` <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
2018-01-06 6:17 ` Pravin Shelar
@ 2018-01-06 6:17 ` Pravin Shelar
[not found] ` <CAO_EM_=2qt3zSW1xprkLvcQVKGRTFMUQxCc4-cVLsUcRLj63Hg@mail.gmail.com>
1 sibling, 1 reply; 26+ messages in thread
From: Pravin Shelar @ 2018-01-06 6:17 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
> wrote:
>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> OVS already pull all required headers in skb linear data, so no need
>>> to redo all of it. only check required is the ip-checksum validation.
>>> I think we could avoid it in most of cases by checking skb length to
>>> ipheader length before verifying the ip header-checksum.
>>
>> Shouldn't the IP header checksum be verified even earlier, like in
>> key_extract(), before actually using any of the fields in the IP
>> header?
>
> Something like this for verifying the IP header checksum (not tested):
>
AFAIU openflow does not need this verification, so it is not required
in flow extract.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
[not found] ` <CAO_EM_=2qt3zSW1xprkLvcQVKGRTFMUQxCc4-cVLsUcRLj63Hg@mail.gmail.com>
@ 2018-01-06 18:57 ` Pravin Shelar
2018-01-09 0:05 ` Pravin Shelar
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-06 18:57 UTC (permalink / raw)
To: Ed Swierk; +Cc: ovs-dev, netdev, Benjamin Warren, Keith Holleman
On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>
>
> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@ovn.org> wrote:
>
> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com>
> wrote:
>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
>> wrote:
>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>>> OVS already pull all required headers in skb linear data, so no need
>>>> to redo all of it. only check required is the ip-checksum validation.
>>>> I think we could avoid it in most of cases by checking skb length to
>>>> ipheader length before verifying the ip header-checksum.
>>>
>>> Shouldn't the IP header checksum be verified even earlier, like in
>>> key_extract(), before actually using any of the fields in the IP
>>> header?
>>
>> Something like this for verifying the IP header checksum (not tested):
>>
> AFAIU openflow does not need this verification, so it is not required
> in flow extract.
>
>
> Okay. How about my proposed trimming implementation, caching the pad length
> in the ovs cb?
>
Caching the length is not that simple, OVS actions can change the
length. Keeping it consistent with packet would be more work, so lets
calculate it in ovs-ct function.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-06 18:57 ` Pravin Shelar
@ 2018-01-09 0:05 ` Pravin Shelar
2018-01-09 3:02 ` Ed Swierk
2018-01-09 3:02 ` Ed Swierk
2 siblings, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-09 0:05 UTC (permalink / raw)
To: Ed Swierk; +Cc: ovs-dev, netdev, Benjamin Warren, Keith Holleman
On Sat, Jan 6, 2018 at 10:57 AM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>
>>
>> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@ovn.org> wrote:
>>
>> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com>
>> wrote:
>>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
>>> wrote:
>>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>>>> OVS already pull all required headers in skb linear data, so no need
>>>>> to redo all of it. only check required is the ip-checksum validation.
>>>>> I think we could avoid it in most of cases by checking skb length to
>>>>> ipheader length before verifying the ip header-checksum.
>>>>
>>>> Shouldn't the IP header checksum be verified even earlier, like in
>>>> key_extract(), before actually using any of the fields in the IP
>>>> header?
>>>
>>> Something like this for verifying the IP header checksum (not tested):
>>>
>> AFAIU openflow does not need this verification, so it is not required
>> in flow extract.
>>
>>
>> Okay. How about my proposed trimming implementation, caching the pad length
>> in the ovs cb?
>>
> Caching the length is not that simple, OVS actions can change the
> length. Keeping it consistent with packet would be more work, so lets
> calculate it in ovs-ct function.
You could make it specific for skb-len-trimming, something like
boolean flag. so that it is easy to reason with.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-06 18:57 ` Pravin Shelar
2018-01-09 0:05 ` Pravin Shelar
@ 2018-01-09 3:02 ` Ed Swierk
2018-01-09 3:02 ` Ed Swierk
2 siblings, 0 replies; 26+ messages in thread
From: Ed Swierk @ 2018-01-09 3:02 UTC (permalink / raw)
To: Pravin Shelar; +Cc: ovs-dev, netdev, Benjamin Warren, Keith Holleman
On 1/6/18 10:57, Pravin Shelar wrote:
> On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>
>>
>> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@ovn.org> wrote:
>>
>> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com>
>> wrote:
>>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
>>> wrote:
>>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>>>> OVS already pull all required headers in skb linear data, so no need
>>>>> to redo all of it. only check required is the ip-checksum validation.
>>>>> I think we could avoid it in most of cases by checking skb length to
>>>>> ipheader length before verifying the ip header-checksum.
>>>>
>>>> Shouldn't the IP header checksum be verified even earlier, like in
>>>> key_extract(), before actually using any of the fields in the IP
>>>> header?
>>>
>>> Something like this for verifying the IP header checksum (not tested):
>>>
>> AFAIU openflow does not need this verification, so it is not required
>> in flow extract.
>>
>>
>> Okay. How about my proposed trimming implementation, caching the pad length
>> in the ovs cb?
>>
> Caching the length is not that simple, OVS actions can change the
> length. Keeping it consistent with packet would be more work, so lets
> calculate it in ovs-ct function.
>
Something like this?
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a38c80e..282325d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4084,6 +4084,8 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
unsigned int transport_len,
__sum16(*skb_chkf)(struct sk_buff *skb));
+int skb_network_trim(struct sk_buff *skb);
+
/**
* skb_head_is_locked - Determine if the skb->head is locked down
* @skb: skb to check
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 08f5740..c68e927 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4740,6 +4740,41 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
}
EXPORT_SYMBOL(skb_checksum_trimmed);
+/**
+ * skb_network_trim - trim skb to length specified by the network header
+ * @skb: the skb to trim
+ *
+ * Trims the skb to the length specified by the network header,
+ * removing any trailing padding. Leaves the skb alone if the protocol
+ * is not IP or IPv6. Frees the skb on error.
+ *
+ * Caller needs to pull the skb to the network header.
+ */
+int skb_network_trim(struct sk_buff *skb)
+{
+ unsigned int len;
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ len = sizeof(struct ipv6hdr)
+ + ntohs(ipv6_hdr(skb)->payload_len);
+ break;
+ default:
+ len = skb->len;
+ }
+
+ err = pskb_trim_rcsum(skb, len);
+ if (unlikely(err))
+ kfree_skb(skb);
+
+ return err;
+}
+EXPORT_SYMBOL(skb_network_trim);
+
void __skb_warn_lro_forwarding(const struct sk_buff *skb)
{
net_warn_ratelimited("%s: received packets cannot be forwarded while LRO is enabled\n",
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..73418d3 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1112,6 +1112,10 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ err = skb_network_trim(skb);
+ if (err)
+ return err;
+
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
if (err)
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-06 18:57 ` Pravin Shelar
2018-01-09 0:05 ` Pravin Shelar
2018-01-09 3:02 ` Ed Swierk
@ 2018-01-09 3:02 ` Ed Swierk
2018-01-09 22:06 ` Pravin Shelar
2 siblings, 1 reply; 26+ messages in thread
From: Ed Swierk @ 2018-01-09 3:02 UTC (permalink / raw)
To: Pravin Shelar; +Cc: ovs-dev, netdev, Benjamin Warren, Keith Holleman
On 1/6/18 10:57, Pravin Shelar wrote:
> On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>
>>
>> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@ovn.org> wrote:
>>
>> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com>
>> wrote:
>>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
>>> wrote:
>>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>>>> OVS already pull all required headers in skb linear data, so no need
>>>>> to redo all of it. only check required is the ip-checksum validation.
>>>>> I think we could avoid it in most of cases by checking skb length to
>>>>> ipheader length before verifying the ip header-checksum.
>>>>
>>>> Shouldn't the IP header checksum be verified even earlier, like in
>>>> key_extract(), before actually using any of the fields in the IP
>>>> header?
>>>
>>> Something like this for verifying the IP header checksum (not tested):
>>>
>> AFAIU openflow does not need this verification, so it is not required
>> in flow extract.
>>
>>
>> Okay. How about my proposed trimming implementation, caching the pad length
>> in the ovs cb?
>>
> Caching the length is not that simple, OVS actions can change the
> length. Keeping it consistent with packet would be more work, so lets
> calculate it in ovs-ct function.
>
Something like this?
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a38c80e..282325d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4084,6 +4084,8 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
unsigned int transport_len,
__sum16(*skb_chkf)(struct sk_buff *skb));
+int skb_network_trim(struct sk_buff *skb);
+
/**
* skb_head_is_locked - Determine if the skb->head is locked down
* @skb: skb to check
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 08f5740..c68e927 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4740,6 +4740,41 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
}
EXPORT_SYMBOL(skb_checksum_trimmed);
+/**
+ * skb_network_trim - trim skb to length specified by the network header
+ * @skb: the skb to trim
+ *
+ * Trims the skb to the length specified by the network header,
+ * removing any trailing padding. Leaves the skb alone if the protocol
+ * is not IP or IPv6. Frees the skb on error.
+ *
+ * Caller needs to pull the skb to the network header.
+ */
+int skb_network_trim(struct sk_buff *skb)
+{
+ unsigned int len;
+ int err;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ len = ntohs(ip_hdr(skb)->tot_len);
+ break;
+ case htons(ETH_P_IPV6):
+ len = sizeof(struct ipv6hdr)
+ + ntohs(ipv6_hdr(skb)->payload_len);
+ break;
+ default:
+ len = skb->len;
+ }
+
+ err = pskb_trim_rcsum(skb, len);
+ if (unlikely(err))
+ kfree_skb(skb);
+
+ return err;
+}
+EXPORT_SYMBOL(skb_network_trim);
+
void __skb_warn_lro_forwarding(const struct sk_buff *skb)
{
net_warn_ratelimited("%s: received packets cannot be forwarded while LRO is enabled\n",
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..73418d3 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1112,6 +1112,10 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
+ err = skb_network_trim(skb);
+ if (err)
+ return err;
+
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
if (err)
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
2018-01-09 3:02 ` Ed Swierk
@ 2018-01-09 22:06 ` Pravin Shelar
0 siblings, 0 replies; 26+ messages in thread
From: Pravin Shelar @ 2018-01-09 22:06 UTC (permalink / raw)
To: Ed Swierk; +Cc: ovs-dev, netdev, Benjamin Warren, Keith Holleman
On Mon, Jan 8, 2018 at 7:02 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On 1/6/18 10:57, Pravin Shelar wrote:
>> On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>>>
>>>
>>> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@ovn.org> wrote:
>>>
>>> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com>
>>> wrote:
>>>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
>>>> wrote:
>>>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>>>>> OVS already pull all required headers in skb linear data, so no need
>>>>>> to redo all of it. only check required is the ip-checksum validation.
>>>>>> I think we could avoid it in most of cases by checking skb length to
>>>>>> ipheader length before verifying the ip header-checksum.
>>>>>
>>>>> Shouldn't the IP header checksum be verified even earlier, like in
>>>>> key_extract(), before actually using any of the fields in the IP
>>>>> header?
>>>>
>>>> Something like this for verifying the IP header checksum (not tested):
>>>>
>>> AFAIU openflow does not need this verification, so it is not required
>>> in flow extract.
>>>
>>>
>>> Okay. How about my proposed trimming implementation, caching the pad length
>>> in the ovs cb?
>>>
>> Caching the length is not that simple, OVS actions can change the
>> length. Keeping it consistent with packet would be more work, so lets
>> calculate it in ovs-ct function.
>>
>
> Something like this?
>
Sure, Can you submit formal patch?
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a38c80e..282325d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4084,6 +4084,8 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
> unsigned int transport_len,
> __sum16(*skb_chkf)(struct sk_buff *skb));
>
> +int skb_network_trim(struct sk_buff *skb);
> +
> /**
> * skb_head_is_locked - Determine if the skb->head is locked down
> * @skb: skb to check
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 08f5740..c68e927 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4740,6 +4740,41 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
> }
> EXPORT_SYMBOL(skb_checksum_trimmed);
>
> +/**
> + * skb_network_trim - trim skb to length specified by the network header
> + * @skb: the skb to trim
> + *
> + * Trims the skb to the length specified by the network header,
> + * removing any trailing padding. Leaves the skb alone if the protocol
> + * is not IP or IPv6. Frees the skb on error.
> + *
> + * Caller needs to pull the skb to the network header.
> + */
> +int skb_network_trim(struct sk_buff *skb)
> +{
> + unsigned int len;
> + int err;
> +
> + switch (skb->protocol) {
> + case htons(ETH_P_IP):
> + len = ntohs(ip_hdr(skb)->tot_len);
> + break;
> + case htons(ETH_P_IPV6):
> + len = sizeof(struct ipv6hdr)
> + + ntohs(ipv6_hdr(skb)->payload_len);
> + break;
> + default:
> + len = skb->len;
> + }
> +
> + err = pskb_trim_rcsum(skb, len);
> + if (unlikely(err))
> + kfree_skb(skb);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(skb_network_trim);
> +
> void __skb_warn_lro_forwarding(const struct sk_buff *skb)
> {
> net_warn_ratelimited("%s: received packets cannot be forwarded while LRO is enabled\n",
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index b27c5c6..73418d3 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1112,6 +1112,10 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> nh_ofs = skb_network_offset(skb);
> skb_pull_rcsum(skb, nh_ofs);
>
> + err = skb_network_trim(skb);
> + if (err)
> + return err;
> +
> if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
> err = handle_fragments(net, key, info->zone.id, skb);
> if (err)
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-01-09 22:06 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-12 16:17 [PATCH] openvswitch: Trim off padding before L3 conntrack processing Ed Swierk
2017-12-14 0:58 ` Pravin Shelar
2017-12-14 20:05 ` Ed Swierk
2017-12-17 19:22 ` Pravin Shelar
2017-12-17 19:22 ` Pravin Shelar
2017-12-21 15:17 ` [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Ed Swierk
2017-12-22 23:31 ` Pravin Shelar
2017-12-22 23:31 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
2018-01-03 6:21 ` Pravin Shelar
2018-01-03 6:21 ` Pravin Shelar
2017-12-23 0:39 ` Ed Swierk
2018-01-04 3:49 ` Ed Swierk
2018-01-05 3:36 ` Pravin Shelar
2018-01-05 3:36 ` Pravin Shelar
2018-01-05 18:14 ` Ed Swierk
[not found] ` <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
2018-01-06 6:17 ` Pravin Shelar
2018-01-06 6:17 ` Pravin Shelar
[not found] ` <CAO_EM_=2qt3zSW1xprkLvcQVKGRTFMUQxCc4-cVLsUcRLj63Hg@mail.gmail.com>
2018-01-06 18:57 ` Pravin Shelar
2018-01-09 0:05 ` Pravin Shelar
2018-01-09 3:02 ` Ed Swierk
2018-01-09 3:02 ` Ed Swierk
2018-01-09 22:06 ` Pravin Shelar
2018-01-05 18:14 ` Ed Swierk
2017-12-21 15:21 ` [PATCH v2 RESEND] " Ed Swierk
2017-12-21 15:21 ` Ed Swierk
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.