[PATCH] libteam: do not destroy the ifinfo of current unregistered dev enslaved into team

[Xin Long]

When a dev is being unregistered and the corresp team port is being
removed, the ifinfo should be delayed to destroy when doing dellink
next time, because later on the dbg log still needs it to print the
ifname saved in this ifinfo:
  ...
  <port_list>
  -1450: veth1: down 0Mbit HD <--
  </port_list>
  ...

The smilar process is also used on the team_port's destruction.

However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo
after lost RTNLGRP_LINK notifications"), it would destroy all those
ifinfo with REMVOED flag, including the one of current unregistered
dev enslaved into team. It would cause a crash later when trying to
print the dbg log.

A simple way to reproduce:

  # ip link add veth1 type veth peer name veth1_peer
  # sleep 5 && ip link del veth1 &
  # teamd -g -c
'{"device":"team0","runner":{"name":"activebackup"},
    "ports":{"veth1":{}}}'

This patch is to avoid it by simply checking !ifinfo->port before
calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in
ifinfo_clear_changed(), so that the ifinfo of current unregistered
dev enslaved into team will be delayed to destroy next time.

Fixes: 046fb6ba0aec ("libteam: resynchronize ifinfo after lost RTNLGRP_LINK
notifications")
Signed-off-by: Xin Long <lucien.xin(a)gmail.com&gt;
---
 libteam/ifinfo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
index 5c32a9c..d47c2bf 100644
--- a/libteam/ifinfo.c
+++ b/libteam/ifinfo.c
@@ -211,7 +211,8 @@ void ifinfo_clear_changed(struct team_handle *th)
 	struct team_ifinfo *ifinfo;
 
 	list_for_each_node_entry(ifinfo, &th->ifinfo_list, list)
-		clear_changed(ifinfo);
+		if (!ifinfo->port)
+			clear_changed(ifinfo);
 }
 
 static struct team_ifinfo *ifinfo_find_create(struct team_handle *th,
@@ -245,7 +246,7 @@ void ifinfo_destroy_removed(struct team_handle *th)
 	struct team_ifinfo *ifinfo, *tmp;
 
 	list_for_each_node_entry_safe(ifinfo, tmp, &th->ifinfo_list, list) {
-		if (is_changed(ifinfo, CHANGED_REMOVED))
+		if (is_changed(ifinfo, CHANGED_REMOVED) && !ifinfo->port)
 			ifinfo_destroy(ifinfo);
 	}
 }
-- 
2.1.0

[Jiri Pirko]

Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin(a)gmail.com wrote:
> When a dev is being unregistered and the corresp team port is being
> removed, the ifinfo should be delayed to destroy when doing dellink
> next time, because later on the dbg log still needs it to print the
> ifname saved in this ifinfo:
>   ...
>   <port_list>
>   -1450: veth1: down 0Mbit HD <--
>   </port_list>
>   ...
>
> The smilar process is also used on the team_port's destruction.
>
> However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo
> after lost RTNLGRP_LINK notifications"), it would destroy all those
> ifinfo with REMVOED flag, including the one of current unregistered
> dev enslaved into team. It would cause a crash later when trying to
> print the dbg log.
>
> A simple way to reproduce:
>
>   # ip link add veth1 type veth peer name veth1_peer
>   # sleep 5 && ip link del veth1 &
>   # teamd -g -c
> '{"device":"team0","runner":{"name":"activebackup"},
>     "ports":{"veth1":{}}}'
>
> This patch is to avoid it by simply checking !ifinfo->port before
> calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in
> ifinfo_clear_changed(), so that the ifinfo of current unregistered
> dev enslaved into team will be delayed to destroy next time. 
What is "next time"?


Please take case of the typos like:
REMVOED, smilar

Also, please be imparative in the patch description like you would be
telling the codebase what to do.

[Xin Long]

On Thu, Apr 19, 2018 at 10:46 PM, Jiri Pirko <jiri(a)resnulli.us&gt; wrote:
>  Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin(a)gmail.com wrote:
> >When a dev is being unregistered and the corresp team port is being
> >removed, the ifinfo should be delayed to destroy when doing dellink
> >next time, because later on the dbg log still needs it to print the
> >ifname saved in this ifinfo:
> >  ...
> >  <port_list>
> >  -1450: veth1: down 0Mbit HD <--
> >  </port_list>
> >  ...
> >
> >The smilar process is also used on the team_port's destruction.
> >
> >However, after Commit 046fb6ba0aec ("libteam: resynchronize ifinfo
> >after lost RTNLGRP_LINK notifications"), it would destroy all those
> >ifinfo with REMVOED flag, including the one of current unregistered
> >dev enslaved into team. It would cause a crash later when trying to
> >print the dbg log.
> >
> >A simple way to reproduce:
> >
> >  # ip link add veth1 type veth peer name veth1_peer
> >  # sleep 5 && ip link del veth1 &
> >  # teamd -g -c
> '{"device":"team0","runner":{"name":"activebackup"},
> >    "ports":{"veth1":{}}}'
> >
> >This patch is to avoid it by simply checking !ifinfo->port before
> >calling ifinfo_destroy in ifinfo_destroy_removed(), as well as in
> >ifinfo_clear_changed(), so that the ifinfo of current unregistered
> >dev enslaved into team will be delayed to destroy next time.
>
>  What is "next time"? 
next time when ifinfo_destroy_removed is called for
another
ifinfo's change.

>  Please take case of the typos like:
>  REMVOED, smilar 
Sorry.

>  Also, please be imparative in the patch description like you would be
>  telling the codebase what to do. 
Copy, 'be imparative',  will improve the
changelog and post v2.

Thanks.