libteam: do not destroy the ifinfo of current unregistered dev enslaved into team
diff mbox

Message ID 09f171c8308cfaec4620c038b651cd6886f74bd8.1523601814.git.lucien.xin@gmail.com
State New
Headers show

Commit Message

Xin Long April 13, 2018, 6:43 a.m. UTC
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@gmail.com>

---
 libteam/ifinfo.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.1.0

Comments

Jiri Pirko April 19, 2018, 2:46 p.m. UTC | #1
Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin@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 April 21, 2018, 11:58 a.m. UTC | #2
On Thu, Apr 19, 2018 at 10:46 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Apr 13, 2018 at 08:43:34AM CEST, lucien.xin@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.

Patch
diff mbox

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);
 	}
 }