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

[Ben Pfaff]

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.