Skip to content

[FDB] [202012] Fix fbdorch to properly handle syncd FDB FLUSH Notif #2401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 16, 2022
Merged

[FDB] [202012] Fix fbdorch to properly handle syncd FDB FLUSH Notif #2401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d8af09
[FDB] Fix fbdorch to properly handle syncd FDB FLUSH Notif (#2254)
vivekrnv May 18, 2022
ed50d20
[fdborch] fix heap-use-after-free in clearFdbEntry() (#2353)
Yakiv-Huryk Jun 24, 2022
0de0d1c
Merge branch '202012' into fdb_rm_vlan_2012
liat-grozovik Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 116 additions & 102 deletions orchagent/fdborch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
extern sai_fdb_api_t *sai_fdb_api;

extern sai_object_id_t gSwitchId;
extern PortsOrch* gPortsOrch;
extern CrmOrch * gCrmOrch;
extern Directory<Orch*> gDirectory;

Expand Down Expand Up @@ -151,6 +150,104 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
}
}

/*
clears stateDb and decrements corresponding internal fdb counters
*/
void FdbOrch::clearFdbEntry(const FdbEntry& entry)
{
FdbUpdate update;
update.entry = entry;
update.add = false;

/* Fetch Vlan and decrement the counter */
Port temp_vlan;
if (m_portsOrch->getPort(entry.bv_id, temp_vlan))
{
m_portsOrch->decrFdbCount(temp_vlan.m_alias, 1);
}

/* Decrement port fdb_counter */
m_portsOrch->decrFdbCount(entry.port_name, 1);

/* Remove the FdbEntry from the internal cache, update state DB and CRM counter */
storeFdbEntryState(update);
notify(SUBJECT_TYPE_FDB_CHANGE, &update);

SWSS_LOG_INFO("FdbEntry removed from internal cache, MAC: %s , port: %s, BVID: 0x%" PRIx64,
update.entry.mac.to_string().c_str(), update.entry.port_name.c_str(), update.entry.bv_id);
}

/*
Handles the SAI_FDB_EVENT_FLUSHED notification recieved from syncd
*/
void FdbOrch::handleSyncdFlushNotif(const sai_object_id_t& bv_id,
const sai_object_id_t& bridge_port_id,
const MacAddress& mac)
{
// Consolidated flush will have a zero mac
MacAddress flush_mac("00:00:00:00:00:00");

/* TODO: Read the SAI_FDB_FLUSH_ATTR_ENTRY_TYPE attr from the flush notif
and clear the entries accordingly, currently only non-static entries are flushed
*/
if (bridge_port_id == SAI_NULL_OBJECT_ID && bv_id == SAI_NULL_OBJECT_ID)
{
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
else if (bv_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on PORT */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->second.bridge_port_id == bridge_port_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on BV_ID */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->first.bv_id == bv_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
else
{
/* FLUSH based on port and VLAN */
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto curr = itr++;
if (curr->first.bv_id == bv_id && curr->second.bridge_port_id == bridge_port_id)
{
if (curr->second.type != "static" && (curr->first.mac == mac || mac == flush_mac))
{
clearFdbEntry(curr->first);
}
}
}
}
}

void FdbOrch::update(sai_fdb_event_t type,
const sai_fdb_entry_t* entry,
sai_object_id_t bridge_port_id)
Expand All @@ -168,24 +265,29 @@ void FdbOrch::update(sai_fdb_event_t type,
type, update.entry.mac.to_string().c_str(),
entry->bv_id, bridge_port_id);


if (bridge_port_id &&
!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
{
if (type == SAI_FDB_EVENT_FLUSHED)
{
/* In case of flush - can be ignored due to a race.
There are notifications about FDB FLUSH (syncd/sai_redis) on port,
which was already removed by orchagent as a result of
removeVlanMember action (removeBridgePort) */
/* There are notifications about FDB FLUSH (syncd/sai_redis) on port,
which was already removed by orchagent as a result of removeVlanMember
action (removeBridgePort). But the internal cleanup of statedb and
internal counters is yet to be performed, thus continue
*/
SWSS_LOG_INFO("Flush event: Failed to get port by bridge port ID 0x%" PRIx64 ".",
bridge_port_id);

} else {
SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64 ".",
bridge_port_id);

return;
}
}

if (entry->bv_id &&
!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch notification type %d: Failed to locate vlan port from bv_id 0x%" PRIx64, type, entry->bv_id);
return;
}

Expand All @@ -195,12 +297,6 @@ void FdbOrch::update(sai_fdb_event_t type,
{
SWSS_LOG_INFO("Received LEARN event for bvid=0x%" PRIx64 "mac=%s port=0x%" PRIx64, entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_ERROR("FdbOrch LEARN notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}

// we already have such entries
auto existing_entry = m_entries.find(update.entry);
if (existing_entry != m_entries.end())
Expand Down Expand Up @@ -229,11 +325,6 @@ void FdbOrch::update(sai_fdb_event_t type,
SWSS_LOG_INFO("Received AGE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch AGE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
}

auto existing_entry = m_entries.find(update.entry);
// we don't have such entries
if (existing_entry == m_entries.end())
Expand Down Expand Up @@ -326,12 +417,6 @@ void FdbOrch::update(sai_fdb_event_t type,
SWSS_LOG_INFO("Received MOVE event for bvid=0x%" PRIx64 " mac=%s port=0x%" PRIx64,
entry->bv_id, update.entry.mac.to_string().c_str(), bridge_port_id);

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_ERROR("FdbOrch MOVE notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}

// We should already have such entry
if (existing_entry == m_entries.end())
{
Expand Down Expand Up @@ -369,86 +454,15 @@ void FdbOrch::update(sai_fdb_event_t type,
bridge_port_id);

string vlanName = "-";
if (entry->bv_id) {
Port vlan;

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch notification: Failed to locate vlan\
port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}
if (!vlan.m_alias.empty()) {
vlanName = "Vlan" + to_string(vlan.m_vlan_info.vlan_id);
}

SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }", update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());

if (bridge_port_id == SAI_NULL_OBJECT_ID &&
entry->bv_id == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(), vlanName.c_str());
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
/*
TODO: here should only delete the dynamic fdb entries,
but unfortunately in structure FdbEntry currently have
no member to indicate the fdb entry type,
if there is static mac added, here will have issue.
*/
update.entry.mac = itr->first.mac;
update.entry.bv_id = itr->first.bv_id;
update.add = false;
itr++;

storeFdbEntryState(update);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
}
else if (entry->bv_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on port */
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());

for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
auto next_item = std::next(itr);
if (itr->first.port_name == update.port.m_alias)
{
update.entry.mac = itr->first.mac;
update.entry.bv_id = itr->first.bv_id;
update.add = false;

storeFdbEntryState(update);
handleSyncdFlushNotif(entry->bv_id, bridge_port_id, update.entry.mac);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
itr = next_item;
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID)
{
/* FLUSH based on VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(),
vlanName.c_str());

}
else
{
/* FLUSH based on port and VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());
}
break;
}

Expand Down Expand Up @@ -524,7 +538,7 @@ void FdbOrch::doTask(Consumer& consumer)
{
SWSS_LOG_ENTER();

if (!gPortsOrch->allPortsReady())
if (!m_portsOrch->allPortsReady())
{
return;
}
Expand Down Expand Up @@ -681,7 +695,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
{
SWSS_LOG_ENTER();

if (!gPortsOrch->allPortsReady())
if (!m_portsOrch->allPortsReady())
{
return;
}
Expand Down
3 changes: 3 additions & 0 deletions orchagent/fdborch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ class FdbOrch: public Orch, public Subject, public Observer

bool storeFdbEntryState(const FdbUpdate& update);
void notifyTunnelOrch(Port& port);

void clearFdbEntry(const FdbEntry&);
void handleSyncdFlushNotif(const sai_object_id_t&, const sai_object_id_t&, const MacAddress& );
};

#endif /* SWSS_FDBORCH_H */
14 changes: 14 additions & 0 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5355,3 +5355,17 @@ std::unordered_set<std::string> PortsOrch::generateCounterStats(const string& ty
}
return counter_stats;
}

bool PortsOrch::decrFdbCount(const std::string& alias, int count)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see it being done in the previous flow. Why is it newly done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the m_fdb_count is decremented during the SAI_FDB_EVENT_AGED notification. So, for the SAI_FDB_EVENT_FLUSHED i think the same is required. https://github.com/sonic-net/sonic-swss/blob/202012/orchagent/fdborch.cpp#L227

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm if this is tested on 202012? I see the ref count as a new change and not part of the FLUSH handling itself for 202012.

Copy link
Contributor Author

@vivekrnv vivekrnv Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, when the corresponding fdb entry expires/removed, some vendor SDK's generate AGED notification and some vendors generate FLUSH notification.

In 202012, we have the code which decrements the m_fdb_count when there is an AGED notification https://github.com/sonic-net/sonic-swss/blob/202012/orchagent/fdborch.cpp#L311, so when the capability to handle FLUSH notification is added, i see no issues in decrementing m_fdb_count as this keeps the implementation consistent.

As for testing this change, do you mean for me to check if m_fdb_count is decremented when there is an AGED notification?

{
auto itr = m_portList.find(alias);
if (itr == m_portList.end())
{
return false;
}
else
{
itr->second.m_fdb_count -= count;
}
return true;
}
1 change: 1 addition & 0 deletions orchagent/portsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class PortsOrch : public Orch, public Subject
bool removeVlanMember(Port &vlan, Port &port);
bool isVlanMember(Port &vlan, Port &port);

bool decrFdbCount(const string& alias, int count);
private:
unique_ptr<Table> m_counterTable;
unique_ptr<Table> m_counterLagTable;
Expand Down
1 change: 1 addition & 0 deletions tests/mock_tests/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ LDADD_GTEST = -L/usr/src/gtest

tests_SOURCES = aclorch_ut.cpp \
portsorch_ut.cpp \
fdborch/flush_syncd_notif_ut.cpp \
saispy_ut.cpp \
consumer_ut.cpp \
ut_saihelper.cpp \
Expand Down
Loading