[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