teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

judyjoseph · 2019-12-07T01:14:32Z

What I did

Added additional checks in the netlink message handling of teamsyncd to make sure the message is for a valid interface with ifindex and name. This is using the netlink cache infrastructure to check if the interface name retrieved from cache with the ifindex is matching with the interfaceName in the netlink NEW LINK create message. If the interface name is not same, we drop the message.

Why I did it

Have seen random cases where teamsyncd fails and the stack trace was showing interface name as NULL and the ifindex an incorrect one.

How I verified it

Could not get the exact reproduction scenario, but induced these errors manually and found the teamsyncd staying up.

Details if related

…id before addLag is called. Have seen cases where the interface name is NULL and the ifindex is an incorrect one.

madhukar-kamarapu · 2019-12-08T01:03:51Z

Hi judyjoseph,

Could you please share the teamsyncd related logs when the issue is seen (OR when you introduced the issue manually).

root@sonic:/var/log# pwd
/var/log
root@sonic:/var/log# zgrep -ne teamsyncd *

By any chance are the logs similar to the one's reported in #763 ?

Rgds,
Madhukar

pavel-shirshov · 2019-12-09T20:37:28Z

teamsyncd/teamsync.cpp

+    {
+        SWSS_LOG_ERROR("Unable to allocate netlink socket");
+    } else {
+        nl_connect(m_nl_sock, NETLINK_ROUTE);


check error code?

Sure will add an explicit check.

pavel-shirshov · 2019-12-09T20:38:05Z

teamsyncd/teamsync.cpp

+    m_nl_sock = nl_socket_alloc();
+    if (!m_nl_sock)
+    {
+        SWSS_LOG_ERROR("Unable to allocate netlink socket");


I think it's better to crash it. Otherwise your check will not work

I had thought before about introducing a throw statement and crashing if the socket_alloc() fails. But didn't do so as I have seen these failure messages of "Unable to initialize team socket" with the team_alloc()/team_init() in other scenario's like back to back config reload command issued multiple times. So didn't want teamsyncd to fail when we try to spawn the process because if this additional check we are introducing.

But I think on a second thought -- I agree with you, it is good to throw error and crash as this is not a good scenario. Shall update, thanks.

pavel-shirshov · 2019-12-09T20:41:33Z

teamsyncd/teamsync.cpp

+    }
+
+    /* Refill cache and check for ifindex */
+    nl_cache_refill(m_nl_sock ,m_link_cache);


could return not zero as error indication
Also. Would it be to much to refill all cache for every ifindex to name operation?

I will do a bit of profiling and see how much time it takes will all the max number of Port channels created. We need to do a refill before checking -- as I found cache incosistency issues in case when we do config reload command back to back.

Also I see similar implementations in
RouteSync::onRouteMsg() --> calling RouteSync::getIfName()
NeighSync::onMsg() --> LinkCache::getInstance().ifindexToName()

I did more profiling for the refill cache, and found it was done in the same second ( it took around 1ms for refill API to complete execution ) - which looked ok.

teamsyncd/teamsync.cpp

pavel-shirshov · 2019-12-09T20:42:41Z

teamsyncd/teamsync.cpp

+    if (rtnl_link_i2name(m_link_cache, ifindex, ifName, TeamPortSync::MAX_IFNAME) == NULL)
+    {
+        /* Returns ifindex as string / */
+        return to_string(ifindex);


the same as before

The idea was to give back the ifindex itself as the interface name and hence the caller function match will fail and ignore the NETLINK update, instead of proceeding with the TeamSync::addLag() API.

I changed the API format so that now it checks the interface name in the API checkIfindexToName() and returns true/false depending on the interface name is present in kernel cache or not.

pavel-shirshov

as comments

pavel-shirshov · 2019-12-09T20:46:59Z

judyjoseph · 2019-12-10T00:56:13Z

Hi Madhukar,
This fix is to try address a segmentation fault crash in teamsyncd which was seen when a config reload command is issued and not for the logs you pointed out.

regards,
Judy

judyjoseph · 2019-12-10T01:02:47Z

Hi Pavel,
I am not able to reproduce this segmentation fault in teamsyncd with multiple back to back config reloads. This issue occurred earlier after a config reload.

I have tried the patch b931751 -- but the error message "TeamPortSync: Failed to initialize team handler" comes again randomly on back to back config reloads and now it happens thrice.

thanks,
Judy

…link message is valid

madhukar-kamarapu · 2019-12-11T14:43:36Z

The reason for the error "TeamPortSync: Failed to initialize team handler" during CONFIG-RELOAD is having the old port-channel netdevices in the kernel.
Fix: Delete the old port-channel netdevices from the kernel before teamsyncd is started. You may add the logic in TeamSync::TeamSync().
Quick fix to validate: Delete all the port-channel netdevices using the command "ip link del dev PortChannelXXX" before teamd docker starts.

I'll be submitting this PR later with more details; you may use the above fix in the meanwhile.
This fix should also solve your teamsyncd crash.
Note: In sonic-buildimage/dockers/docker-teamd/start.sh, start teamsyncd 1st followed with teammgrd. This helps in addressing few issues.

madhukar-kamarapu · 2019-12-11T23:06:04Z

@judyjoseph - can you please let me know if the suggested solution works for you?

lguohan · 2019-12-13T16:07:35Z

teamsyncd/teamsync.cpp

@@ -102,13 +161,38 @@ void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj)
    if ((nlmsg_type != RTM_NEWLINK) && (nlmsg_type != RTM_DELLINK))
        return;

-    string lagName = rtnl_link_get_name(link);
+    /* Introduce check if the interface name is NULL */
+    char *ifName = rtnl_link_get_name(link);


I do not understand why you change the previous lagName to ifName here. seems ifName is not used anymore afterwards

The main reason behind this fix was the observation of a crash in teamsyncd segmentation fault with invalid ifindex and mtu value

#7 swss::TeamSync::addLag (this=0x3e8, lagName=..., ifindex=-374612960, admin_state=, oper_state=, mtu=3920352592)

So the plan here was to check if the ifName is null … to make sure we don't proceed.

lguohan · 2019-12-13T16:10:19Z

teamsyncd/teamsync.cpp

+     * We are here because this is a NEW link create netlink message from kernel. 
+     * Fetch and compare the interface name using ifindex from kernel cache. 
+     */
+    if(!checkIfindexToName(ifindex, lagName)) {


under what conditions do we see ifindex does not match lagName? if we ignore such messages, what is the consequence here? Are we losing some validate netlink messages?

From my testing I have seen messages with the older if indices coming to teamsyncd on doing a config reload. So the idea was to make sure we proceed to add Lag only the ifindices --> interface name and the "Name" which came in netlink message is same. This check should not resulting in losing valid messages.

lguohan · 2019-12-13T16:20:33Z

@madhukar-kamarapu , teammgrd will recreate those port channel devices. is there a race condition between teamsync and teammgrd? maybe we should put some explicit dependency between teamsync and teammgrd?

judyjoseph

@madhukar-kamarapu, Thank you for your comments. From what I understand from code and from testing scenario's - the issues I see of invalid netlink messages is when we do commands like config reload/ load minigraph where we stop start the services. I agree with you that one approach could be to delete all the port-channels in the constructor, but then for this fix to work, we will have to change process startup order to start teamsyncd first and then teammgrd which could have other impacts ( to be analyzed )

The fix I am working on now is to do a good clean of the interfaces when the teamd docker goes down, like clean up the portchannel interfaces in teammgrd signal handler. Additionally I am looking into checking if we don't wait enough on doing a docker stop. Are we spawning the teamd docker again too quickly ? I find this as the root-cause of invalid netlink messages coming in teamSync::onMsg() -- your thoughts.

madhukar-kamarapu · 2019-12-17T01:33:02Z

@judyjoseph - if teammgrd is launched before teamsyncd, I've observed NLE_DUMP_INTR error on the netlink socket opened by teamsyncd. Reason - during config-reload OR cold-reboot, all the netdevices (front-panel-ports, port-channel, VLAN, VTEP, etc) are being created in the kernel. Due to this, the DUMP_REQUEST sent by teamsyncd fails. Note: This is purely a timing issue though.

Upon receiving NLE_DUMP_INTR, we are supposed to retry the DUMP_REQUEST (already done on teamd - https://lists.fedoraproject.org/archives/list/libteam@lists.fedorahosted.org/thread/SMLV7U2FCYL7UUAYVDBGQMHHSI3YWEK7/).
Currently teamsyncd does not have this infrastructure to retry for NLE_DUMP_INTR.

As long as we delete the stale (old) port-channel netdevices in the kernel during config-reload, I don't see any problem by starting teamsyncd 1st followed with teammgrd.

We can add dependency though - program an entry TeamsyncdInitDone in APP_DB (similar to PortInitDone in PORT_TABLE of APP_DB).

madhukar-kamarapu · 2019-12-17T01:48:57Z

@judyjoseph - Adding logic to clean-up the port-channel interfaces in teammgrd signal handlers seems fine. One another thought - how about terminating all the teamd processes; it should take care of removing the port-channel netdevices in the kernel.
@lguohan - can you please comment on this.

Spawning teamd docker quickly is fine - all we need to ensure that proper clean-up is done before we bring up the teamd docker.
As I mentioned earlier, invalid netlink messages is mainly due to the stale (old) port-channel netdevices in the kernel. You may print/dump the interface number when you receive the invalid netlink message, it clearly shows that its the port-channel netdevice created before the config-reload was issued.

When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process.

madhukar-kamarapu

Can you please update the questionnaire:

What I did
Why I did it
How I verified it

judyjoseph · 2019-12-23T23:40:35Z

Thanks for taking a look. I too find difficult to update this PR .. hence I had opened a new one #1159. Can you take a look ? The details are updated already there.

judyjoseph · 2019-12-23T23:41:44Z

Yes I have verified that the teamd processes exits cleanly and the Port channel interfaces are removed on config reload/ docker restart - I would need to try a docker upgrade though

judyjoseph · 2019-12-23T23:44:09Z

I did some study on this and found that the teamd processes were not getting the SIGTERM signal from supervisord on issuing the "docker stop". Can you take a look at the PR #1159. This change gets things cleaned and has least impact.

madhukar-kamarapu · 2019-12-23T23:52:17Z

Thanks. Please update the docker upgrade results in the new PR #1159.
Note: I am sure it would work (since docker-restart worked); but wanted to double confirm.

madhukar-kamarapu · 2019-12-23T23:53:32Z

@judyjoseph - I guess you're going to abandon the current PR (#1144). Please confirm.
I will give further comments (if any) in the PR#1159

Depends on sonic-net#1453 - What I did Added support to query and clear headroom pool watermark counters Added unit test for the headroom pool watermark counters - How I did it Modified watermarkstat script to query/clear headroom pool watermark counters Added show and clear commands - How to verify it Send traffic such that it treks into the headroom pool and check for the headroom pool usage using the show command below Set polling interval to 30s and issue clear commands mentioned below and verify that counters are cleared - New command output (if the output of a command-line utility has changed) Show commands admin@sonic:~$ show headroom-pool watermark Headroom pool maximum occupancy: Pool Bytes --------------------- ------- ingress_lossless_pool 12480 admin@sonic:~$ show headroom-pool persistent-watermark Headroom pool maximum occupancy: Pool Bytes --------------------- ------- ingress_lossless_pool 12480 Clear commands admin@sonic:~$ sudo sonic-clear headroom-pool watermark admin@sonic:~$ sudo sonic-clear headroom-pool persistent-watermark Signed-off-by: Neetha John <nejo@microsoft.com>

Additional checks put inplace to make sure the netlink message is val…

ab9795b

…id before addLag is called. Have seen cases where the interface name is NULL and the ifindex is an incorrect one.

pavel-shirshov self-requested a review December 9, 2019 20:32

pavel-shirshov self-assigned this Dec 9, 2019

pavel-shirshov reviewed Dec 9, 2019

View reviewed changes

teamsyncd/teamsync.cpp Outdated Show resolved Hide resolved

pavel-shirshov reviewed Dec 9, 2019

View reviewed changes

[Part 2]teamsyncd::Additional checks put inplace to make sure the net…

c5dedab

…link message is valid

lguohan reviewed Dec 13, 2019

View reviewed changes

judyjoseph commented Dec 13, 2019

View reviewed changes

Send explicit signal to the teamd processes whenthe teamd docker exits.

94977e3

When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process.

madhukar-kamarapu reviewed Dec 23, 2019

View reviewed changes

judyjoseph closed this Dec 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

judyjoseph commented Dec 7, 2019

madhukar-kamarapu commented Dec 8, 2019 •

edited

Loading

pavel-shirshov Dec 9, 2019

judyjoseph Dec 10, 2019

pavel-shirshov Dec 9, 2019

judyjoseph Dec 10, 2019

pavel-shirshov Dec 9, 2019

judyjoseph Dec 10, 2019

judyjoseph Dec 11, 2019

pavel-shirshov Dec 9, 2019

judyjoseph Dec 10, 2019

judyjoseph Dec 11, 2019

pavel-shirshov left a comment

pavel-shirshov commented Dec 9, 2019

judyjoseph commented Dec 10, 2019

judyjoseph commented Dec 10, 2019

madhukar-kamarapu commented Dec 11, 2019

madhukar-kamarapu commented Dec 11, 2019

lguohan Dec 13, 2019

judyjoseph Dec 13, 2019

lguohan Dec 13, 2019

judyjoseph Dec 13, 2019

lguohan commented Dec 13, 2019

judyjoseph left a comment

madhukar-kamarapu commented Dec 17, 2019

madhukar-kamarapu commented Dec 17, 2019

madhukar-kamarapu left a comment

judyjoseph commented Dec 23, 2019

judyjoseph commented Dec 23, 2019

judyjoseph commented Dec 23, 2019

madhukar-kamarapu commented Dec 23, 2019

madhukar-kamarapu commented Dec 23, 2019

teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

Conversation

judyjoseph commented Dec 7, 2019

madhukar-kamarapu commented Dec 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-shirshov left a comment

Choose a reason for hiding this comment

pavel-shirshov commented Dec 9, 2019

judyjoseph commented Dec 10, 2019

judyjoseph commented Dec 10, 2019

madhukar-kamarapu commented Dec 11, 2019

madhukar-kamarapu commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Dec 13, 2019

judyjoseph left a comment

Choose a reason for hiding this comment

madhukar-kamarapu commented Dec 17, 2019

madhukar-kamarapu commented Dec 17, 2019

madhukar-kamarapu left a comment

Choose a reason for hiding this comment

judyjoseph commented Dec 23, 2019

judyjoseph commented Dec 23, 2019

judyjoseph commented Dec 23, 2019

madhukar-kamarapu commented Dec 23, 2019

madhukar-kamarapu commented Dec 23, 2019

madhukar-kamarapu commented Dec 8, 2019 •

edited

Loading