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

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@intel.com>
---
 lib/netdev-native-tnl.c | 3 +++
 1 file changed, 3 insertions(+)

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@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.

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@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.

- Bhanuprakash.

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@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.