Hi Ben, >On Fri, Jan 12, 2018 at 05:43:13PM +0000, Bhanuprakash Bodireddy wrote: >> During tunnel decapsulation the below steps are performed: >> [1] Tunnel information is populated in packet metadata i.e packet->md- >>tunnel. >> [2] Outer header gets popped. >> [3] Packet is recirculated. >> >> For [1] to work, the dp_packet L3 and L4 header offsets should be valid. >> The offsets in the dp_packet are set as part of miniflow extraction. >> >> If offsets are accidentally reset (or) the pop header operation is >> performed prior to miniflow extraction, step [1] fails silently and >> creates issues that are harder to debug. Add the assertion to check if >> the offsets are valid. >> >> Signed-off-by: Bhanuprakash Bodireddy >> <[email protected]> >> --- >> lib/netdev-native-tnl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index >> 9ce8567..fb5eab0 100644 >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -508,6 +508,9 @@ netdev_vxlan_pop_header(struct dp_packet >*packet) >> ovs_be32 vx_flags; >> enum packet_type next_pt = PT_ETH; >> >> + ovs_assert(packet->l3_ofs > 0); >> + ovs_assert(packet->l4_ofs > 0); >> + >> pkt_metadata_init_tnl(md); >> if (VXLAN_HLEN > dp_packet_l4_size(packet)) { >> goto err; > >Thanks for working to make OVS more reliable. > >How much risk do you think there is of these assertions triggering? Are you >debugging an issue where they would trigger, and has that been fixed? I'm >trying to figure out whether it makes more sense to put assertions here or >whether something closer to a log message plus a jump to "err" would be >better. It's not great for OVS to assert-fail, but on the other hand if it >indicates >a genuine bug then sometimes it's the best thing to do.
I was working on a RFC patch to skip recirculation for vxlan decap side. I posted today @ https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343103.html In that implementation vxlan header is popped before the Miniflow extraction and that's when I ran in to above mentioned problem. Also I found that dp_packet_reset_packet() and dp_packet_reset_offsets() when accidentally called will clear the offsets and any later invocation of *vxlan_pop_header() or for that matter any code that uses the dp_packet L3/L4 offsets will fail. So I added an assertion to make it more explicit for vxlans. Please note that there isn't any bug on the master code and this was done as a precautionary measure to improve debugging. - Bhanuprakash. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
