[ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action

[Anju Thomas]

Hi Ben,

We are facing multiple such crashes on different computes in our deployments. Seems to be a pretty common problem in our setup.  As you suggested, it would be good if we can make the below changes as well.    How do you suggest we move forward regarding the approaches?

Regards & thanks
Anju

-----Original Message-----
From: Anju Thomas 
Sent: Tuesday, June 05, 2018 10:26 AM
To: Ben Pfaff <blp at ovn.org>
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action


Hi ,

Any suggestions ?

Will it be ok if  we merge the tunnel push change that we have discussed below while I can work on the other changes in parallel?

Regards & thanks
Anju
-----Original Message-----
From: Anju Thomas
Sent: Thursday, May 17, 2018 3:52 PM
To: 'Ben Pfaff' <blp at ovn.org>
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action

Hi Ben,
I was working on the code changes and I can think of two approaches we can take to prevent this crash in the dpdk datapath.

1. Today the dp_packet module that we have is never return any error . The only error handling it is by assert using OVS_NOT_REACHED(). Should we go ahead and restructure the entire dp_packet module to do some error handling and  return (refer to openvswitchlib/dp-packet.c). This would require a more detailed effort to ensure all flows are handled.
2.  Do not call the  dp_packet_uninit without properly checking we have adequate headroom for DPBUF_DPDK buffers. 

Any suggestions on which approach we should go ahead with  ?

Regards & Thanks
Anju
-----Original Message-----
From: Ben Pfaff [mailto:blp at ovn.org]
Sent: Friday, May 11, 2018 2:00 AM
To: Anju Thomas <anju.thomas at ericsson.com>
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action

On Thu, May 10, 2018 at 08:51:03AM +0000, Anju Thomas wrote:
> It looks like your commit message describes at least two other bugs in 
> OVS, though.  First, if OVS crashes when it pushes tunnel headers, 
> even if there's not enough headroom, that's really bad.  At worst, it 
> should drop the packet.  Do you know where the crash occurs?  We 
> should fix the problem, since it might recur in some other context.
> 
> [Anju] OVS was actually crashing in the dpdk datapath. The crash is a manual assert in our case.
> The rootcause is that dp receives the actions after the upcall (say with >=3 tunnel pushes ) . Now as part of action processing , since it is a tunnel push action , we try to find space in the dpdk mbuf packet headroom (which Is 128 bytes). By the time we try to process the third tunnel push , there is no headroom left since each tunnel header is of 50 bytes (50 *3 > 128 bytes headroom). Hence it manually asserts .  This assert is to catch any unexpected code flow . Do you think that in this case we should still go ahead and  prevent the crash ? 

I don't understand why it's OK to crash in this case.  Why do you think so?

> Second, it's a little shocking to hear that an encapsulation action without a following output action causes a memory leak.  We also need to fix that.  Do you have any more details?
> [Anju] Now as explained above, the crash happens because we run out of headroom. But in case we have say 2 or less than 2 tunnel pushes we will have a mem leak as packet is never freed because the tnl push is the dp last action and there is no other output action or any other action like recirc that may translate to output action in the end leading  to packet buffer not being freed.
> Are you proposing that we have some sort of preventive fix in the dp to handle an incorrect action list from the upcall handling? 

Yes.  It's unacceptable to leak memory because there's a packet modification without an output action.  The kernel datapath never does this, for example.