diff mbox series

[ovs-dev,2/2] dpif-netdev: Drop invalid parsed packets

Message ID 20240505064209.2866336-2-roid@nvidia.com
State Rejected
Headers show
Series [ovs-dev,1/2] flow: Return value from miniflow_extract() | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Roi Dayan May 5, 2024, 6:42 a.m. UTC 
From: Eli Britstein <elibr@nvidia.com>

In case of a malformed packet, its parsing fails. Instead of continuing
and possible form a wrong flow, drop the packet.

Relevant tests changed to send valid packets.

Signed-off-by: Eli Britstein <elibr@nvidia.com>
Acked-by: Roi Dayan <roid@nvidia.com>
---
 lib/dpif-netdev.c        | 7 ++++++-
 tests/mpls-xlate.at      | 4 ++--
 tests/ofproto-dpif.at    | 6 +++---
 tests/ofproto.at         | 6 +++---
 tests/tunnel-push-pop.at | 2 +-
 5 files changed, 15 insertions(+), 10 deletions(-)

Comments

Ilya Maximets May 6, 2024, 11:05 a.m. UTC | #1 
On 5/5/24 08:42, Roi Dayan via dev wrote:
> From: Eli Britstein <elibr@nvidia.com>
> 
> In case of a malformed packet, its parsing fails. Instead of continuing
> and possible form a wrong flow, drop the packet.

Hi, Eli and Roi.  Thanks for the patch!

But I don't think we can do that.  There are few reasons why the
packets should not be dropped in the datapath:

1. OVS is a switch, the only reasons why it should drop packets are:
    - User configuration
    - Inability to make a forwarding decision
   Both are not the case here.  For example, if the packet has some
   issue in the IPv4 header, we should still forward it in case we
   just acting as an L2 learning switch.  L2 learning switch doesn't
   need any information from IPv4 header to function.

2. Datapath should not make forwarding decisions including a decision
   to drop a packet.  Datapath implementation should just execute
   actions that OpenFlow layers tell it to execute.  OpenFlow layers
   should decide what to do.

Also, inability to parse certain parts of the packet is not a parsing
failure.  The resulted flow structure should be valid regardless of
the packet content.  Fields that were not extracted remain zero and
OpenFlow layers should correctly handle that and execute appropriate
actions, i.e. properly match on all-zero values if they were used to
make a forwarding decision.

If the wrong flow can be installed in this situation - it's a bug
somewhere in the flow translation logic that should be fixed.

Hope this makes sense.

Best regards, Ilya Maximets.
Roi Dayan May 6, 2024, 1:20 p.m. UTC | #2 
On 06/05/2024 14:05, Ilya Maximets wrote:
> On 5/5/24 08:42, Roi Dayan via dev wrote:
>> From: Eli Britstein <elibr@nvidia.com>
>>
>> In case of a malformed packet, its parsing fails. Instead of continuing
>> and possible form a wrong flow, drop the packet.
> 
> Hi, Eli and Roi.  Thanks for the patch!
> 
> But I don't think we can do that.  There are few reasons why the
> packets should not be dropped in the datapath:
> 
> 1. OVS is a switch, the only reasons why it should drop packets are:
>     - User configuration
>     - Inability to make a forwarding decision
>    Both are not the case here.  For example, if the packet has some
>    issue in the IPv4 header, we should still forward it in case we
>    just acting as an L2 learning switch.  L2 learning switch doesn't
>    need any information from IPv4 header to function.
> 
> 2. Datapath should not make forwarding decisions including a decision
>    to drop a packet.  Datapath implementation should just execute
>    actions that OpenFlow layers tell it to execute.  OpenFlow layers
>    should decide what to do.
> 
> Also, inability to parse certain parts of the packet is not a parsing
> failure.  The resulted flow structure should be valid regardless of
> the packet content.  Fields that were not extracted remain zero and
> OpenFlow layers should correctly handle that and execute appropriate
> actions, i.e. properly match on all-zero values if they were used to
> make a forwarding decision.
> 
> If the wrong flow can be installed in this situation - it's a bug
> somewhere in the flow translation logic that should be fixed.
> 
> Hope this makes sense.
> 
> Best regards, Ilya Maximets.


thanks Ilya for the explanation.
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c7f9e149025e..cf27d2631a94 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -8588,7 +8588,12 @@  dfc_processing(struct dp_netdev_pmd_thread *pmd,
             }
         }
 
-        miniflow_extract(packet, &key->mf);
+        if (OVS_UNLIKELY(!miniflow_extract(packet, &key->mf))) {
+            dp_packet_delete(packet);
+            COVERAGE_INC(datapath_drop_rx_invalid_packet);
+            continue;
+        }
+
         key->len = 0; /* Not computed yet. */
         key->hash =
                 (md_is_valid == false)
diff --git a/tests/mpls-xlate.at b/tests/mpls-xlate.at
index 7474becc8914..c0f33715dc82 100644
--- a/tests/mpls-xlate.at
+++ b/tests/mpls-xlate.at
@@ -72,13 +72,13 @@  AT_CHECK([tail -1 stdout], [0],
 ])
 
 for d in 0 1 2 3; do
-    pkt="in_port(1),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x8847),mpls(label=22,tc=0,ttl=64,bos=1)"
+    pkt="f8bc124658e0f8bc124434b6884700016140450000180001000040007ae6000000000000000000000000"
     AT_CHECK([ovs-appctl netdev-dummy/receive p0 $pkt])
 done
 
 AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/packets.*actions:1/actions:1/' | strip_used | strip_ufid | sort], [0], [dnl
 flow-dump from the main thread:
-recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8847),mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:3, bytes:54, used:0.0s, actions:pop_mpls(eth_type=0x800),recirc(0x3)
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8847),mpls(label=22/0xfffff,tc=0/0,ttl=64/0x0,bos=1/1), packets:3, bytes:126, used:0.0s, actions:pop_mpls(eth_type=0x800),recirc(0x3)
 recirc_id(0x3),in_port(1),packet_type(ns=0,id=0),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=0.0.0.0,dst=0.0.0.0,proto=0,frag=no), actions:100
 ])
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 0b23fd6c5ea6..65c94a59abae 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -7477,10 +7477,10 @@  AT_CHECK([tail -1 stdout], [0],
 ])
 
 dnl An 170 byte packet
-AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000808004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000800004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
 
 AT_CHECK([ovs-ofctl parse-pcap p1.pcap], [0], [dnl
-icmp,in_port=ANY,vlan_tci=0x0000,dl_src=00:50:56:c0:00:08,dl_dst=00:0c:29:c8:a0:a4,nw_src=192.168.218.1,nw_dst=192.168.218.100,nw_tos=0,nw_ecn=0,nw_ttl=64,nw_frag=no,icmp_type=8,icmp_code=0
+in_port=ANY,vlan_tci=0x0000,dl_src=00:50:56:c0:00:08,dl_dst=00:0c:29:c8:a0:a4,dl_type=0x05ff
 ])
 
 AT_CHECK([ovs-appctl revalidator/purge], [0])
@@ -7509,7 +7509,7 @@  AT_CHECK([tail -1 stdout], [0],
 ])
 
 dnl An 170 byte packet
-AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000808004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '000c29c8a0a4005056c0000800004500009cb4a6000040019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
 
 AT_CHECK([ovs-appctl revalidator/purge], [0])
 dnl packet size: 64 + 128 + 170 = 362
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 2889f81fb171..435f0f45762c 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -6627,11 +6627,11 @@  AT_CHECK([ovs-ofctl del-flows br0])
 AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 
 dnl send a proto 0 packet to try and poison the DP flow path
-AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::3,proto=0,tclass=0,hlimit=64,frag=no)'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 '50540000000c50540000000b86dd600000000024004020010db800000000000000000000000101110db80000000000000000000000033a000502000001008f00321b0000000103000000ff020000000000000000000000010003'])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows], [0], [dnl
 flow-dump from the main thread:
-recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::3,proto=0,hlimit=0,frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535))
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::3,proto=58,hlimit=64,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(hlimit=63)),2
 ])
 
 dnl Send ICMP for mod nw_src and mod nw_dst
@@ -6655,8 +6655,8 @@  AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:
 
 AT_CHECK([ovs-appctl dpctl/dump-flows | sort], [0], [dnl
 flow-dump from the main thread:
-recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::3,proto=0,hlimit=0,frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535))
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::3,proto=1,hlimit=64,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(hlimit=63)),2
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::3,proto=58,hlimit=64,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(hlimit=63)),2
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::4,proto=1,hlimit=64,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(hlimit=8)),2
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::5,proto=1,tclass=0/0x3,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(tclass=0x2/0x3)),2
 recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x86dd),ipv6(dst=111:db8::6,proto=1,tclass=0/0xfc,frag=no), packets:0, bytes:0, used:never, actions:set(ipv6(tclass=0x40/0xfc)),2
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 508737c53ec6..37ae520b8e2e 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -557,7 +557,7 @@  ovs-appctl coverage/read-counter drop_action_congestion
 
 
 dnl Check GREL3 only accepts non-fragmented packets?
-AT_CHECK([ovs-appctl netdev-dummy/receive p0 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c8fe71d883724fbeb6f4e1494a080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p0 'aa55aa550000001b213cab6408004500007e79464000402fba550101025c0101025820000800000001c84e710062724fbeb6f4e14217080045000054ba200000400184861e0000011e00000200004227e75400030af3195500000000f265010000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637'])
 
 ovs-appctl time/warp 1000
 ovs-appctl time/warp 1000