[ovs-dev] [PATCH] netdev-native-tnl: Add assertion in vxlan_pop_header.

Ben Pfaff blp at ovn.org
Fri Jan 12 21:05:32 UTC 2018


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
> >> <bhanuprakash.bodireddy at intel.com>
> >> ---
> >>  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.


More information about the dev mailing list