Cherry-picked commits from R3.0

Deadlock in agent.

Problem:
It was seen that if a packet is trapped for flow setup with same source IP and
destination IP is classified as nat flow then rflow key becomes similar to flow
key.
This in turn used to enter in deadlock as agent will try to attempt lock both on
flow and rflow located using above keys. (Both will be same).

Mentioned bug exposed this issue.
In bug ICMP TTL expired was received for which agent generated a packet to
switch to VM-port from where packet with TTL 1 was generated. This packet
resulted in flow trap and agent processed the same. (agent packet was
                                                     generated
                                                     with
                                                     same
                                                     SIP
                                                     and
                                                     DIP).
Deadlock results in flows going to hold state.

This fix does not cover agent generating packet with same SIP and DIP and
flow trap for agent generated packet.
It only fixes deadlock.

Solution:
In packet flow when flow and rflow keys are created, match them and if they are
same, nullify rflow and mark flow as short flow.

Partial-bug: #1556290

Conflicts:
src/vnsw/agent/pkt/flow_table.cc

Conflicts:
src/vnsw/agent/pkt/flow_entry.cc
src/vnsw/agent/pkt/flow_entry.h
src/vnsw/agent/pkt/flow_table.cc
src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc

BGP service sessio gets reset intermittently.

Problem:
All BGP as service flows program flows with loose policy. This
is to enable flow
lookup on non-tunneled traffic coming from fabric.
Say there is a VM and it has two bgp-sessions to CN1 and CN2.
Both the session
will have reverse flow(fabric) which will have same nat-sport
and
dport(bgp-port) with different destination IP. For loose policy
vrouter programs
this nat-sport to bitmap which it uses to identify fabric
traffic for flow
processing. When traffic comes from fabric it checks dport and
if it matches to
nat port it has stored in bitmap vrouter pushes it for flow
processing else dump
it to host interface.
Now if one session is teared down say CN2 in this case, reverse
flow gets
aged out and in turn vrouter removes the nat port from bitmap.
However for CN1
this reservation was still needed. In its absence packet coming
from CN1 to VM
gets dumped to host interface(even though flow is present).

Solution:
Dont age the flow of bgp service and let it get deleted by
config change of bgp
service object or vm interface deletion.

Closes-bug: 1551576

Conflicts:
src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc

Send BGP flag to retain same nat port across flows.

In BGP as service same nat port is used for different CN
peers.
Now if one CN is going down agent will send delete for flow,
    which in turn will
    reset the port still in use by second CN. Now because of
    this reset packets from
    second CN will start going to vhost. This will cause session
    reset for second CN
    and in turn other issues arise.

    Solution:
    New flag tells vrouter to retain the port for BGP flows even
    if flow is deleted.

    Closes-bug: #1551576

Change-Id: I79cff1e70a2a3560d408e4d8f74cfde10018f33d

src/vnsw/agent/kstate/flow_kstate.cc

@@ -154,6 +154,8 @@ const std::string FlowKState::DropCodeToStr(uint8_t drop_code) const {
         return "RevSG";
     case VR_FLOW_DR_REVERSE_OUT_SG:
         return "RevOutSG";
+    case VR_FLOW_DR_SAME_FLOW_RFLOW_KEY:
+        return "SameFlowRflowKey";
     default:
         return "Unknown";
     }

src/vnsw/agent/pkt/flow_entry.cc

@@ -81,6 +81,7 @@ const std::map<uint16_t, const char*>
         ((uint16_t)SHORT_INVALID_L2_FLOW,    "Short flow invalid L2 flow")
         ((uint16_t)SHORT_FLOW_ON_TSN,        "Short flow TSN flow")
         ((uint16_t)SHORT_NO_MIRROR_ENTRY,     "Short flow No mirror entry ")
+        ((uint16_t)SHORT_SAME_FLOW_RFLOW_KEY,"Short flow same flow and rflow")
         ((uint16_t)DROP_POLICY,              "Flow drop Policy")
         ((uint16_t)DROP_OUT_POLICY,          "Flow drop Out Policy")
         ((uint16_t)DROP_SG,                  "Flow drop SG")

src/vnsw/agent/pkt/flow_entry.h

@@ -406,6 +406,7 @@ class FlowEntry {
         SHORT_INVALID_L2_FLOW,
         SHORT_FLOW_ON_TSN,
         SHORT_NO_MIRROR_ENTRY,
+        SHORT_SAME_FLOW_RFLOW_KEY,
         SHORT_MAX
     };
 

src/vnsw/agent/pkt/flow_event.cc

@@ -196,15 +196,15 @@ void FlowEventQueueBase::ProcessDone(FlowEvent *event, bool update_rev_flow) {
 
     case FlowEvent::DELETE_DBENTRY:
     case FlowEvent::DELETE_FLOW: {
-        FLOW_LOCK(flow, rflow);
+        FLOW_LOCK(flow, rflow, event->event());
         flow->GetPendingAction()->ResetDelete();
         if (rflow)
             rflow->GetPendingAction()->ResetDelete();
         break;
     }
 
     case FlowEvent::FLOW_MESSAGE: {
-        FLOW_LOCK(flow, rflow);
+        FLOW_LOCK(flow, rflow, event->event());
         flow->GetPendingAction()->ResetRecompute();
         if (rflow)
             rflow->GetPendingAction()->ResetRecompute();
@@ -218,7 +218,7 @@ void FlowEventQueueBase::ProcessDone(FlowEvent *event, bool update_rev_flow) {
     }
 
     case FlowEvent::REVALUATE_DBENTRY: {
-        FLOW_LOCK(flow, rflow);
+        FLOW_LOCK(flow, rflow, event->event());
         flow->GetPendingAction()->ResetRevaluate();
         if (rflow)
             rflow->GetPendingAction()->ResetRevaluate();

src/vnsw/agent/pkt/flow_table.cc

@@ -156,7 +156,7 @@ void FlowTable::Add(FlowEntry *flow, FlowEntry *rflow) {
     FlowEntry *new_flow = Locate(flow, time);
     FlowEntry *new_rflow = (rflow != NULL) ? Locate(rflow, time) : NULL;
 
-    FLOW_LOCK(new_flow, new_rflow);
+    FLOW_LOCK(new_flow, new_rflow, FlowEvent::FLOW_MESSAGE);
     AddInternal(flow, new_flow, rflow, new_rflow, false, false);
 }
 
@@ -172,7 +172,7 @@ void FlowTable::Update(FlowEntry *flow, FlowEntry *rflow) {
         rev_flow_update = false;
     }
 
-    FLOW_LOCK(new_flow, new_rflow);
+    FLOW_LOCK(new_flow, new_rflow, FlowEvent::FLOW_MESSAGE);
     AddInternal(flow, new_flow, rflow, new_rflow, fwd_flow_update,
                 rev_flow_update);
 }
@@ -394,7 +394,7 @@ bool FlowTable::Delete(const FlowKey &key, bool del_reverse_flow) {
     FlowEntry *rflow = NULL;
 
     PopulateFlowEntriesUsingKey(key, del_reverse_flow, &flow, &rflow);
-    FLOW_LOCK(flow, rflow);
+    FLOW_LOCK(flow, rflow, FlowEvent::DELETE_FLOW);
     return DeleteUnLocked(del_reverse_flow, flow, rflow);
 }
 
@@ -411,7 +411,7 @@ void FlowTable::DeleteAll() {
             reverse_entry = it->second;
             ++it;
         }
-        FLOW_LOCK(entry, reverse_entry);
+        FLOW_LOCK(entry, reverse_entry, FlowEvent::DELETE_FLOW);
         DeleteUnLocked(true, entry, reverse_entry);
     }
 }
@@ -755,7 +755,7 @@ void FlowTable::ProcessKSyncFlowEvent(const FlowEventKSync *req,
 bool FlowTable::ProcessFlowEvent(const FlowEvent *req, FlowEntry *flow,
                                  FlowEntry *rflow) {
     //Take lock
-    FLOW_LOCK(flow, rflow);
+    FLOW_LOCK(flow, rflow, req->event());
 
     if (flow)
         flow->set_last_event(req->event());

src/vnsw/agent/pkt/flow_table.h

@@ -56,13 +56,24 @@ class FlowTableKSyncObject;
 class FlowEvent;
 class FlowEventKSync;
 
-#define FLOW_LOCK(flow, rflow) \
+#define FLOW_LOCK(flow, rflow, flow_event) \
+    bool is_flow_rflow_key_same = false; \
+    if (flow == rflow) { \
+        if (flow_event == FlowEvent::DELETE_FLOW) { \
+            assert(0); \
+        } \
+        is_flow_rflow_key_same = true; \
+        rflow = NULL; \
+    } \
     tbb::mutex tmp_mutex1, tmp_mutex2, *mutex_ptr_1, *mutex_ptr_2; \
-    FlowTable::GetMutexSeq(flow ? flow->mutex() : tmp_mutex1,      \
-                           rflow ? rflow->mutex() : tmp_mutex2,    \
-                           &mutex_ptr_1, &mutex_ptr_2);            \
-    tbb::mutex::scoped_lock lock1(*mutex_ptr_1);                   \
-    tbb::mutex::scoped_lock lock2(*mutex_ptr_2);
+    FlowTable::GetMutexSeq(flow ? flow->mutex() : tmp_mutex1, \
+                           rflow ? rflow->mutex() : tmp_mutex2, \
+                           &mutex_ptr_1, &mutex_ptr_2); \
+    tbb::mutex::scoped_lock lock1(*mutex_ptr_1); \
+    tbb::mutex::scoped_lock lock2(*mutex_ptr_2); \
+    if (is_flow_rflow_key_same) { \
+        flow->MakeShortFlow(FlowEntry::SHORT_SAME_FLOW_RFLOW_KEY); \
+    }
 
 /////////////////////////////////////////////////////////////////////////////
 // Class to manage free-list of flow-entries

src/vnsw/agent/pkt/pkt_flow_info.cc

@@ -1658,12 +1658,7 @@ void PktFlowInfo::Add(const PktInfo *pkt, PktControlInfo *in,
     }
 
     // Allocate reverse flow
-    if (in->intf_ == out->intf_) {
-        // we are trying to loopback to the same interface,
-        // this is not supported, mark the flow as short flow
-        short_flow = true;
-        short_flow_reason = FlowEntry::SHORT_IPV4_FWD_DIS;
-    } else if (nat_done) {
+    if (nat_done) {
         FlowKey rkey(out->nh_, nat_ip_daddr, nat_ip_saddr, pkt->ip_proto,
                      r_sport, r_dport);
         rflow = FlowEntry::Allocate(rkey, flow_table);

src/vnsw/agent/pkt/test/test_pkt_fip.cc

@@ -1767,7 +1767,25 @@ TEST_F(FlowTest, vm_packet_to_own_floating_ip) {
                             1, 0, 0, vnet[1]->flow_key_nh()->id());
 
     EXPECT_TRUE(fe != NULL && fe->is_flags_set(FlowEntry::ShortFlow) == true &&
-                fe->short_flow_reason() == FlowEntry::SHORT_IPV4_FWD_DIS);
+                fe->short_flow_reason() == FlowEntry::SHORT_SAME_FLOW_RFLOW_KEY);
+
+    //cleanup
+    client->EnqueueFlowFlush();
+    client->WaitForIdle(2);
+    WAIT_FOR(1000, 100, (0U == flow_proto_->FlowCount()));
+}
+
+// negative test scenario where packet with same source and dest IP is received.
+TEST_F(FlowTest, invalid_nat_flow_1) {
+    TxIpPacket(vnet[1]->id(), "2.1.1.100", "2.1.1.100", 1);
+    client->WaitForIdle();
+
+    // verify flow created as short flow
+    FlowEntry *fe = FlowGet(vnet[1]->vrf()->vrf_id(), "2.1.1.100", "2.1.1.100",
+                            1, 0, 0, vnet[1]->flow_key_nh()->id());
+
+    EXPECT_TRUE(fe != NULL);
+    EXPECT_TRUE(fe->is_flags_set(FlowEntry::ShortFlow) == true);
 
     //cleanup
     client->EnqueueFlowFlush();

src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc

@@ -143,6 +143,7 @@ void FlowStatsCollector::UpdateEntriesToVisit() {
 bool FlowStatsCollector::ShouldBeAged(FlowExportInfo *info,
                                       const vr_flow_entry *k_flow,
                                       uint64_t curr_time) {
+    FlowEntry *flow = info->flow();
     //If both forward and reverse flow are marked
     //as TCP closed then immediately remote the flow
     if (k_flow != NULL) {
@@ -161,6 +162,11 @@ bool FlowStatsCollector::ShouldBeAged(FlowExportInfo *info,
     if (diff_time < flow_age_time_intvl()) {
         return false;
     }
+
+    if (flow->is_flags_set(FlowEntry::BgpRouterService)) {
+        return false;
+    }
+
     return true;
 }
 
@@ -372,7 +378,7 @@ void FlowStatsCollector::UpdateAndExportInternalLocked(FlowExportInfo *info,
                                                    const RevFlowDepParams *p) {
     FlowEntry *flow = info->flow();
     FlowEntry *rflow = info->reverse_flow();
-    FLOW_LOCK(flow, rflow);
+    FLOW_LOCK(flow, rflow, FlowEvent::FLOW_MESSAGE);
     UpdateAndExportInternal(info, bytes, oflow_bytes, pkts, oflow_pkts, time,
                             teardown_time, p);
 }
@@ -670,7 +676,7 @@ void FlowStatsCollector::ExportFlowLocked(FlowExportInfo *info,
                                           const RevFlowDepParams *params) {
     FlowEntry *flow = info->flow();
     FlowEntry *rflow = info->reverse_flow();
-    FLOW_LOCK(flow, rflow);
+    FLOW_LOCK(flow, rflow, FlowEvent::FLOW_MESSAGE);
     ExportFlow(info, diff_bytes, diff_pkts, params);
 }
 
@@ -894,7 +900,7 @@ bool FlowStatsCollector::RequestHandler(boost::shared_ptr<FlowExportReq> req) {
     const FlowExportInfo &info = req->info();
     FlowEntry *flow = info.flow();
     FlowEntry *rflow = info.reverse_flow();
-    FLOW_LOCK(flow, rflow);
+    FLOW_LOCK(flow, rflow, FlowEvent::FLOW_MESSAGE);
 
     switch (req->event()) {
     case FlowExportReq::ADD_FLOW: {

src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc

@@ -79,6 +79,8 @@ static uint16_t GetDropReason(uint16_t dr) {
         return VR_FLOW_DR_LINKLOCAL_SRC_NAT;
     case FlowEntry::SHORT_NO_MIRROR_ENTRY:
         return VR_FLOW_DR_NO_MIRROR_ENTRY;
+    case FlowEntry::SHORT_SAME_FLOW_RFLOW_KEY:
+        return VR_FLOW_DR_SAME_FLOW_RFLOW_KEY;
     case FlowEntry::DROP_POLICY:
         return VR_FLOW_DR_POLICY;
     case FlowEntry::DROP_OUT_POLICY:
@@ -328,6 +330,9 @@ int FlowTableKSyncEntry::Encode(sandesh_op::type op, char *buf, int buf_len) {
             if (nat_flow->is_flags_set(FlowEntry::LinkLocalBindLocalSrcPort) ||
                 nat_flow->is_flags_set(FlowEntry::BgpRouterService)) {
                 flags |= VR_FLOW_FLAG_LINK_LOCAL;
+                if (nat_flow->is_flags_set(FlowEntry::BgpRouterService)) {
+                    flags |= VR_FLOW_BGP_SERVICE;
+                }
             }
 
             flags |= VR_FLOW_FLAG_VRFT;