[ovs-dev] datapath: ensure UFO traffic is actually fragmented

Message ID 1527617179-28985-1-git-send-email-neal@digitalocean.com
State Changes Requested
Delegated to: pravin shelar
Headers show
Series
  • [ovs-dev] datapath: ensure UFO traffic is actually fragmented
Related show

Commit Message

Sriharsha Basavapatna via dev May 29, 2018, 6:06 p.m.
While investigating a kernel panic, our team noticed that UDP traffic
recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP.
After decap, we also noticed that traffic that had this flag set had its
fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction.

When the connection tracker encounters this, it assumes it's already
dealing with fragmented traffic, which might not be the case.  This
patch simply ensures we're dealing with an actual fragment before sending
the skb off to be reassembled.

Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
Signed-off-by: Neal Shrader <neal@digitalocean.com>
---
 datapath/conntrack.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Ben Pfaff June 15, 2018, 8:05 p.m. | #1
On Tue, May 29, 2018 at 06:06:19PM +0000, Neal Shrader via dev wrote:
> While investigating a kernel panic, our team noticed that UDP traffic
> recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP.
> After decap, we also noticed that traffic that had this flag set had its
> fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction.
> 
> When the connection tracker encounters this, it assumes it's already
> dealing with fragmented traffic, which might not be the case.  This
> patch simply ensures we're dealing with an actual fragment before sending
> the skb off to be reassembled.
> 
> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
> Signed-off-by: Neal Shrader <neal@digitalocean.com>

Thanks a lot for the patch.

Greg, have you taken a look at this?
Gregory Rose June 15, 2018, 8:21 p.m. | #2
On 6/15/2018 1:05 PM, Ben Pfaff wrote:
> On Tue, May 29, 2018 at 06:06:19PM +0000, Neal Shrader via dev wrote:
>> While investigating a kernel panic, our team noticed that UDP traffic
>> recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP.
>> After decap, we also noticed that traffic that had this flag set had its
>> fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction.
>>
>> When the connection tracker encounters this, it assumes it's already
>> dealing with fragmented traffic, which might not be the case.  This
>> patch simply ensures we're dealing with an actual fragment before sending
>> the skb off to be reassembled.
>>
>> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
>> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
>> Signed-off-by: Neal Shrader <neal@digitalocean.com>
> Thanks a lot for the patch.
>
> Greg, have you taken a look at this?

I had it flagged for review but have not yet had a chance to get to it.  
I'll do so now.

Thanks,

- Greg
Pravin Shelar June 15, 2018, 11:10 p.m. | #3
On Fri, Jun 15, 2018 at 1:21 PM, Gregory Rose <gvrose8192@gmail.com> wrote:
> On 6/15/2018 1:05 PM, Ben Pfaff wrote:
>>
>> On Tue, May 29, 2018 at 06:06:19PM +0000, Neal Shrader via dev wrote:
>>>
>>> While investigating a kernel panic, our team noticed that UDP traffic
>>> recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP.
>>> After decap, we also noticed that traffic that had this flag set had its
>>> fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction.
>>>
>>> When the connection tracker encounters this, it assumes it's already
>>> dealing with fragmented traffic, which might not be the case.  This
>>> patch simply ensures we're dealing with an actual fragment before sending
>>> the skb off to be reassembled.
>>>
>>> Reported-by: Johannes Erdfelt <johannes@erdfelt.com>
>>> Reported-at:
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html
>>> Signed-off-by: Neal Shrader <neal@digitalocean.com>
>>
>> Thanks a lot for the patch.
>>
>> Greg, have you taken a look at this?
>
>
> I had it flagged for review but have not yet had a chance to get to it.
> I'll do so now.
>

I do not think this is right approach to fix the issue.
I have posted my comment on discuss mailing thread:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html

Patch

diff --git a/datapath/conntrack.c b/datapath/conntrack.c
index e53b8e3..04dbeb4 100644
--- a/datapath/conntrack.c
+++ b/datapath/conntrack.c
@@ -1187,9 +1187,18 @@  int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 		return err;
 
 	if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
-		err = handle_fragments(net, key, info->zone.id, skb);
-		if (err)
-			return err;
+		bool real_fragment = true;
+
+#ifdef HAVE_SKB_GSO_UDP
+		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+			real_fragment = !!(ip_hdr(skb)->frag_off & htons(IP_OFFSET | IP_MF));
+#endif
+
+		if (real_fragment) {
+			err = handle_fragments(net, key, info->zone.id, skb);
+			if (err)
+				return err;
+		}
 	}
 
 	if (info->commit)