diff mbox series

[ovs-dev,v3,2/2] system-traffic: Check frozen state handling with TLV map change

Message ID 1586457459-30517-2-git-send-email-pkusunyifeng@gmail.com
State Accepted
Commit 134e6831acca48f10df3d59b8e1567c24dd925d2
Headers show
Series None | expand

Commit Message

Yifeng Sun April 9, 2020, 6:37 p.m. UTC 
This patch enhances a system traffic test to prevent regression on
the tunnel metadata table (tun_table) handling with frozen state.
Without a proper fix this test can crash ovs-vswitchd due to a
use-after-free bug on tun_table.

These are the timed sequence of how this bug is triggered:

- Adds an OpenFlow rule in OVS that matches Geneve tunnel metadata that
contains a controller action.
- When the first packet matches the aforementioned OpenFlow rule,
during the miss upcall, OVS stores a pointer to the tun_table (that
decodes the Geneve tunnel metadata) in a frozen state and pushes down
a datapath flow into kernel datapath.
- Issues a add-tlv-map command to reprogram the tun_table on OVS.
OVS frees the old tun_table and create a new tun_table.
- A subsequent packet hits the kernel datapath flow again. Since
there is a controller action associated with that flow, it triggers
slow path controller upcall.
- In the slow path controller upcall, OVS derives the tun_table
from the frozen state, which points to the old tun_table that is
already being freed at this time point.
- In order to access the tunnel metadata, OVS uses the invalid
pointer that points to the old tun_table and triggers the core dump.

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Co-authored-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
v1-v2: Improve the test based on William's review, thanks.

 tests/system-traffic.at | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

William Tu April 10, 2020, 9:54 p.m. UTC | #1 
On Thu, Apr 09, 2020 at 11:37:39AM -0700, Yifeng Sun wrote:
> This patch enhances a system traffic test to prevent regression on
> the tunnel metadata table (tun_table) handling with frozen state.
> Without a proper fix this test can crash ovs-vswitchd due to a
> use-after-free bug on tun_table.
> 
> These are the timed sequence of how this bug is triggered:
> 
> - Adds an OpenFlow rule in OVS that matches Geneve tunnel metadata that
> contains a controller action.
> - When the first packet matches the aforementioned OpenFlow rule,
> during the miss upcall, OVS stores a pointer to the tun_table (that
> decodes the Geneve tunnel metadata) in a frozen state and pushes down
> a datapath flow into kernel datapath.
> - Issues a add-tlv-map command to reprogram the tun_table on OVS.
> OVS frees the old tun_table and create a new tun_table.
> - A subsequent packet hits the kernel datapath flow again. Since
> there is a controller action associated with that flow, it triggers
> slow path controller upcall.
> - In the slow path controller upcall, OVS derives the tun_table
> from the frozen state, which points to the old tun_table that is
> already being freed at this time point.
> - In order to access the tunnel metadata, OVS uses the invalid
> pointer that points to the old tun_table and triggers the core dump.
> 
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> Co-authored-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---

Applied, thanks
William
William Tu April 10, 2020, 10:11 p.m. UTC | #2 
On Fri, Apr 10, 2020 at 02:54:41PM -0700, William Tu wrote:
> On Thu, Apr 09, 2020 at 11:37:39AM -0700, Yifeng Sun wrote:
> > This patch enhances a system traffic test to prevent regression on
> > the tunnel metadata table (tun_table) handling with frozen state.
> > Without a proper fix this test can crash ovs-vswitchd due to a
> > use-after-free bug on tun_table.
> > 
> > These are the timed sequence of how this bug is triggered:
> > 
> > - Adds an OpenFlow rule in OVS that matches Geneve tunnel metadata that
> > contains a controller action.
> > - When the first packet matches the aforementioned OpenFlow rule,
> > during the miss upcall, OVS stores a pointer to the tun_table (that
> > decodes the Geneve tunnel metadata) in a frozen state and pushes down
> > a datapath flow into kernel datapath.
> > - Issues a add-tlv-map command to reprogram the tun_table on OVS.
> > OVS frees the old tun_table and create a new tun_table.
> > - A subsequent packet hits the kernel datapath flow again. Since
> > there is a controller action associated with that flow, it triggers
> > slow path controller upcall.
> > - In the slow path controller upcall, OVS derives the tun_table
> > from the frozen state, which points to the old tun_table that is
> > already being freed at this time point.
> > - In order to access the tunnel metadata, OVS uses the invalid
> > pointer that points to the old tun_table and triggers the core dump.
> > 
> > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > Co-authored-by: Yi-Hung Wei <yihung.wei@gmail.com>
> > ---
> 
> Applied, thanks
> William
I also backport to 2.13.
diff mbox series

Patch

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4a39c929c207..3ed03d92b566 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -611,6 +611,16 @@  NS_CHECK_EXEC([at_ns0], [ping -q -c 3 10.1.1.100 | FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
+dnl Test OVS handles TLV map modifictions properly when restores frozen state.
+NS_CHECK_EXEC([at_ns0], [ping 10.1.1.100 > /dev/null &])
+
+AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0x88,len=4}->tun_metadata1"])
+sleep 1
+AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0x99,len=4}->tun_metadata2"])
+sleep 1
+AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0xaa,len=4}->tun_metadata3"])
+sleep 1
+
 OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP