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

2024-05-06 Thread Roi Dayan via dev 



On 06/05/2024 14:05, Ilya Maximets wrote:
> On 5/5/24 08:42, Roi Dayan via dev wrote:
>> From: Eli Britstein 
>>
>> 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. 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-05-06 Thread Ilya Maximets 
On 5/5/24 08:42, Roi Dayan via dev wrote:
> From: Eli Britstein 
> 
> 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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-05-05 Thread 0-day Robot 
Bleep bloop.  Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: The subject summary should end with a dot.
Subject: dpif-netdev: Drop invalid parsed packets
Lines checked: 129, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2024-05-04 Thread Roi Dayan via dev 
From: Eli Britstein 

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 
Acked-by: Roi Dayan 
---
 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(-)

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="f8bc124658e0f8bc124434b68847000161404518000140007ae6"
 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/0xf,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/0xf,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 
'000c29c8a0a4005056c80800459cb4a640019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'000c29c8a0a4005056c8459cb4a640019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
 
 AT_CHECK([ovs-ofctl parse-pcap p1.pcap], [0], [dnl
-icmp,in_port=ANY,vlan_tci=0x,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=0x,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 
'000c29c8a0a4005056c80800459cb4a640019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'000c29c8a0a4005056c8459cb4a640019003c0a8da01c0a8da640800cb5fa762000556f431ad0009388e08090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f404142434445464748494a4b4c4d4e4f505152535455565758595a5b5c5d5e5f606162636465666768696a6b6c6d6e6f707172737475767778797a7b7c7d7e7f'])
 
 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