On Fri, Jan 12, 2018 at 07:33:30PM +0000, Bodireddy, Bhanuprakash wrote: > 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.
OK, thanks. I applied this to master. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
