[syncd] Add workaround for port error status notification (#1430)

Adding new field in port status notification is breaking
change and can cause some issues when getting notification
from older SAI library, this will add workaround to handle
that case

proxylib/Proxy.cpp

@@ -9,6 +9,7 @@
 #include "meta/ZeroMQSelectableChannel.h"
 
 #include "syncd/ZeroMQNotificationProducer.h"
+#include "syncd/Workaround.h"
 
 #include <inttypes.h>
 
@@ -35,7 +36,8 @@ Proxy::Proxy(
     m_vendorSai(vendorSai),
     m_options(options),
     m_apiInitialized(false),
-    m_notificationsSentCount(0)
+    m_notificationsSentCount(0),
+    m_apiVersion(SAI_VERSION(0,0,0))
 {
     SWSS_LOG_ENTER();
 
@@ -79,6 +81,17 @@ Proxy::Proxy(
 
         SWSS_LOG_NOTICE("api initialized success");
     }
+
+    auto st = m_vendorSai->queryApiVersion(&m_apiVersion);
+
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_WARN("failed to obtain libsai api version: %s", sai_serialize_status(st).c_str());
+    }
+    else
+    {
+        SWSS_LOG_NOTICE("libsai api version: %lu", m_apiVersion);
+    }
 }
 
 Proxy::~Proxy()
@@ -1113,7 +1126,9 @@ void Proxy::onPortStateChange(
 {
     SWSS_LOG_ENTER();
 
-    auto s = sai_serialize_port_oper_status_ntf(count, data);
+    auto ntfdata = syncd::Workaround::convertPortOperStatusNotification(count, data, m_apiVersion);
+
+    auto s = sai_serialize_port_oper_status_ntf((uint32_t)ntfdata.size(), ntfdata.data());
 
     sendNotification(SAI_SWITCH_NOTIFICATION_NAME_PORT_STATE_CHANGE, s);
 }

proxylib/Proxy.h

@@ -206,5 +206,7 @@ namespace saiproxy
              * notifications.
              */
             uint64_t m_notificationsSentCount;
+
+            sai_api_version_t m_apiVersion;
     };
 }

syncd/NotificationHandler.cpp

@@ -1,4 +1,5 @@
 #include "NotificationHandler.h"
+#include "Workaround.h"
 #include "sairediscommon.h"
 
 #include "swss/logger.h"
@@ -10,8 +11,10 @@
 using namespace syncd;
 
 NotificationHandler::NotificationHandler(
-        _In_ std::shared_ptr<NotificationProcessor> processor):
-    m_processor(processor)
+        _In_ std::shared_ptr<NotificationProcessor> processor,
+        _In_ sai_api_version_t apiVersion):
+    m_processor(processor),
+    m_apiVersion(apiVersion)
 {
     SWSS_LOG_ENTER();
 
@@ -27,6 +30,21 @@ NotificationHandler::~NotificationHandler()
     // empty
 }
 
+void NotificationHandler::setApiVersion(
+        _In_ sai_api_version_t apiVersion)
+{
+    SWSS_LOG_ENTER();
+
+    m_apiVersion = apiVersion;
+}
+
+sai_api_version_t NotificationHandler::getApiVersion() const
+{
+    SWSS_LOG_ENTER();
+
+    return m_apiVersion;
+}
+
 void NotificationHandler::setSwitchNotifications(
         _In_ const sai_switch_notifications_t& switchNotifications)
 {
@@ -93,7 +111,9 @@ void NotificationHandler::onPortStateChange(
 {
     SWSS_LOG_ENTER();
 
-    auto s = sai_serialize_port_oper_status_ntf(count, data);
+    auto ntfdata = Workaround::convertPortOperStatusNotification(count, data, m_apiVersion);
+
+    auto s = sai_serialize_port_oper_status_ntf((uint32_t)ntfdata.size(), ntfdata.data());
 
     enqueueNotification(SAI_SWITCH_NOTIFICATION_NAME_PORT_STATE_CHANGE, s);
 }

syncd/NotificationHandler.h

@@ -20,7 +20,8 @@ namespace syncd
         public:
 
             NotificationHandler(
-                    _In_ std::shared_ptr<NotificationProcessor> processor);
+                    _In_ std::shared_ptr<NotificationProcessor> processor,
+                    _In_ sai_api_version_t apiVersion = SAI_VERSION(0,0,0));
 
             virtual ~NotificationHandler();
 
@@ -35,6 +36,11 @@ namespace syncd
                     _In_ uint32_t attr_count,
                     _In_ sai_attribute_t *attr_list) const;
 
+            void setApiVersion(
+                    _In_ sai_api_version_t apiVersion);
+
+            sai_api_version_t getApiVersion() const;
+
         public: // members reflecting SAI callbacks
 
             void onFdbEvent(
@@ -99,5 +105,7 @@ namespace syncd
             std::shared_ptr<NotificationQueue> m_notificationQueue;
 
             std::shared_ptr<NotificationProcessor> m_processor;
+
+            sai_api_version_t m_apiVersion;
     };
 }

syncd/Syncd.cpp

@@ -206,6 +206,21 @@ Syncd::Syncd(
         abort();
     }
 
+    sai_api_version_t apiVersion = SAI_VERSION(0,0,0); // invalid version
+
+    status = m_vendorSai->queryApiVersion(&apiVersion);
+
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_WARN("failed to obtain libsai api version: %s", sai_serialize_status(status).c_str());
+    }
+    else
+    {
+        SWSS_LOG_NOTICE("libsai api version: %lu", apiVersion);
+    }
+
+    m_handler->setApiVersion(apiVersion);
+
     m_breakConfig = BreakConfigParser::parseBreakConfig(m_commandLineOptions->m_breakConfig);
 
     SWSS_LOG_NOTICE("syncd started");

syncd/Workaround.cpp

@@ -6,6 +6,11 @@
 
 using namespace syncd;
 
+/**
+ * @def Maximum port count on port notification
+ */
+#define MAX_PORT_COUNT (4096)
+
 /**
  * @brief Determines whether attribute is "workaround" attribute for SET API.
  *
@@ -57,3 +62,43 @@ bool Workaround::isSetAttributeWorkaround(
 
     return false;
 }
+
+std::vector<sai_port_oper_status_notification_t> Workaround::convertPortOperStatusNotification(
+        _In_ const uint32_t count,
+        _In_ const sai_port_oper_status_notification_t* data,
+        _In_ sai_api_version_t version)
+{
+    SWSS_LOG_ENTER();
+
+    std::vector<sai_port_oper_status_notification_t> ntf;
+
+    if (data == nullptr || count > MAX_PORT_COUNT)
+    {
+        SWSS_LOG_ERROR("invalid notification parameters: data: %p, count: %d", data, count);
+
+        return ntf;
+    }
+
+    if (version > SAI_VERSION(1,14,0))
+    {
+        // structure is compatible, no need for change
+
+        ntf.assign(data, data + count);
+
+        return ntf;
+    }
+
+    SWSS_LOG_INFO("converting sai_port_oper_status_notification_t data from %lu to %u", version, SAI_API_VERSION);
+
+    const sai_port_oper_status_notification_v1_14_0_t* dataold =
+        reinterpret_cast<const sai_port_oper_status_notification_v1_14_0_t*>(data);
+
+    for (uint32_t i = 0; i < count; i++)
+    {
+        sai_port_oper_status_notification_t item{ dataold[i].port_id, dataold[i].port_state, (sai_port_error_status_t)0};
+
+        ntf.push_back(item);
+    }
+
+    return ntf;
+}

syncd/Workaround.h

@@ -4,6 +4,8 @@ extern "C" {
 #include "saimetadata.h"
 }
 
+#include <vector>
+
 namespace syncd
 {
     class Workaround
@@ -31,5 +33,38 @@ namespace syncd
                     _In_ sai_object_type_t objectType,
                     _In_ sai_attr_id_t attrId,
                     _In_ sai_status_t status);
+
+            /**
+             * @brief Convert port status notification from older version.
+             *
+             * This method will convert port status notification from SAI
+             * v1.14.0 to version v1.15.0, since from that version a new
+             * structure field was added, and there is possibility that syncd
+             * compiled against headers v1.15.0 will receive notification from
+             * sai library v1.14.0 or older, and since that change is not
+             * backward compatible, it can cause invalid memory read or garbage
+             * memory read. To prevent that, we will convert structures.
+             *
+             * Similar thing can happen if older syncd will receive
+             * notification from newer version of sai library.
+             */
+            static std::vector<sai_port_oper_status_notification_t> convertPortOperStatusNotification(
+                    _In_ const uint32_t count,
+                    _In_ const sai_port_oper_status_notification_t* data,
+                    _In_ sai_api_version_t version);
+        public:
+
+            /**
+             * @brief Port operational status notification as in SAI version v1.14.0
+             *
+             * This will imitate structure size needed for notification conversion.
+             */
+            typedef struct _sai_port_oper_status_notification_v1_14_0_t
+            {
+                sai_object_id_t port_id;
+
+                sai_port_oper_status_t port_state;
+
+            } sai_port_oper_status_notification_v1_14_0_t;
     };
 }

unittest/syncd/TestNotificationHandler.cpp

@@ -67,3 +67,16 @@ TEST(NotificationHandler, NotificationHandlerTest)
                                                     data,
                                                     description);
 }
+
+TEST(NotificationHandler, setApiVersion)
+{
+    auto np = std::make_shared<NotificationProcessor>(nullptr, nullptr, nullptr);
+
+    auto nh = std::make_shared<NotificationHandler>(np);
+
+    EXPECT_EQ(SAI_VERSION(0,0,0), nh->getApiVersion());
+
+    nh->setApiVersion(SAI_VERSION(1,15,0));
+
+    EXPECT_EQ(SAI_VERSION(1,15,0), nh->getApiVersion());
+}

unittest/syncd/TestWorkaround.cpp

@@ -19,3 +19,51 @@ TEST(Workaround, isSetAttributeWorkaround)
     ASSERT_EQ(Workaround::isSetAttributeWorkaround(SAI_OBJECT_TYPE_PORT, SAI_PORT_ATTR_TYPE, SAI_STATUS_FAILURE), false);
     ASSERT_EQ(Workaround::isSetAttributeWorkaround(SAI_OBJECT_TYPE_PORT, SAI_PORT_ATTR_TYPE, SAI_STATUS_SUCCESS), false);
 }
+
+TEST(Workaround,convertPortOperStatusNotification)
+{
+    sai_port_oper_status_notification_t data[2];
+
+    ASSERT_EQ(Workaround::convertPortOperStatusNotification(0, nullptr, SAI_API_VERSION).size(), 0);
+    ASSERT_EQ(Workaround::convertPortOperStatusNotification(5000, data, SAI_API_VERSION).size(), 0);
+
+    ASSERT_EQ(Workaround::convertPortOperStatusNotification(2, data, SAI_VERSION(1,15,0)).size(), 2);
+    ASSERT_EQ(Workaround::convertPortOperStatusNotification(2, data, SAI_VERSION(1,14,1)).size(), 2);
+
+    // check new structure notifications
+
+    data[0].port_id = 12;
+    data[0].port_state = SAI_PORT_OPER_STATUS_DOWN;
+    data[0].port_error_status = SAI_PORT_ERROR_STATUS_HIGH_BER;
+    data[1].port_id = 22;
+    data[1].port_state = SAI_PORT_OPER_STATUS_UP;
+    data[1].port_error_status = SAI_PORT_ERROR_STATUS_DATA_UNIT_MISALIGNMENT_ERROR;
+
+    auto ntf = Workaround::convertPortOperStatusNotification(2, data, SAI_VERSION(1,14,1));
+
+    ASSERT_EQ(ntf[0].port_id, 12);
+    ASSERT_EQ(ntf[0].port_state, SAI_PORT_OPER_STATUS_DOWN);
+    ASSERT_EQ(ntf[0].port_error_status, SAI_PORT_ERROR_STATUS_HIGH_BER);
+    ASSERT_EQ(ntf[1].port_id, 22);
+    ASSERT_EQ(ntf[1].port_state, SAI_PORT_OPER_STATUS_UP);
+    ASSERT_EQ(ntf[1].port_error_status, SAI_PORT_ERROR_STATUS_DATA_UNIT_MISALIGNMENT_ERROR);
+
+    // check old structure notification
+    Workaround::sai_port_oper_status_notification_v1_14_0_t old[2];
+
+    old[0].port_id = 42;
+    old[0].port_state = SAI_PORT_OPER_STATUS_UP;
+    old[1].port_id = 43;
+    old[1].port_state = SAI_PORT_OPER_STATUS_DOWN;
+
+    auto ntf2 = Workaround::convertPortOperStatusNotification(2, reinterpret_cast<sai_port_oper_status_notification_t*>(old), SAI_VERSION(1,14,0));
+
+    ASSERT_EQ(ntf.size(), 2);
+
+    ASSERT_EQ(ntf2[0].port_id, 42);
+    ASSERT_EQ(ntf2[0].port_state, SAI_PORT_OPER_STATUS_UP);
+    ASSERT_EQ(ntf2[0].port_error_status, 0);
+    ASSERT_EQ(ntf2[1].port_id, 43);
+    ASSERT_EQ(ntf2[1].port_state, SAI_PORT_OPER_STATUS_DOWN);
+    ASSERT_EQ(ntf2[1].port_error_status, 0);
+}