libteam: don't crash when trying to print unregistered device name
diff mbox

Message ID alpine.GSO.2.20.1804271256090.9078@kekkonen.niksula.hut.fi
State New
Headers show

Commit Message

Tiainen, Antti April 27, 2018, 10:02 a.m. UTC
team_port_str() will crash when trying to print port name that was
just unregistered, if dellink event is handled before port removal
event.

This is regression from Commit 046fb6ba0aec ("libteam: resynchronize
ifinfo after lost RTNLGRP_LINK notifications"), which made it free
all removed interfaces after ifinfo handlers are called.

Put the ifinfo_destroy_removed() back to dellink/newlink handlers as
it was before that commit. Clean up the ifinfo list after change handlers
only if it refreshed the entire ifinfo list after lost events.

There's still a rare possibility that dellink event is missed due to
full socket receive buffer, which would cause ifinfo refresh and clearing
removed interfaces. For this, add NULL check to team_port_str() so it
doesn't try to print port device name in this situation.

Signed-off-by: Antti Tiainen <atiainen@forcepoint.com>

---
  include/team.h      | 1 +
  libteam/ifinfo.c    | 7 ++++++-
  libteam/libteam.c   | 2 +-
  libteam/stringify.c | 3 ++-
  4 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.16.1

Comments

Jiri Pirko May 2, 2018, 10:22 a.m. UTC | #1
Fri, Apr 27, 2018 at 12:02:21PM CEST, atiainen@forcepoint.com wrote:
>team_port_str() will crash when trying to print port name that was

>just unregistered, if dellink event is handled before port removal

>event.

>

>This is regression from Commit 046fb6ba0aec ("libteam: resynchronize

>ifinfo after lost RTNLGRP_LINK notifications"), which made it free

>all removed interfaces after ifinfo handlers are called.

>

>Put the ifinfo_destroy_removed() back to dellink/newlink handlers as

>it was before that commit. Clean up the ifinfo list after change handlers

>only if it refreshed the entire ifinfo list after lost events.

>

>There's still a rare possibility that dellink event is missed due to

>full socket receive buffer, which would cause ifinfo refresh and clearing

>removed interfaces. For this, add NULL check to team_port_str() so it

>doesn't try to print port device name in this situation.

>

>Signed-off-by: Antti Tiainen <atiainen@forcepoint.com>


Xin, what do you think about this patch. Is it working for you?

Thanks.
Xin Long May 2, 2018, 10:46 a.m. UTC | #2
On Wed, May 2, 2018 at 6:22 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Apr 27, 2018 at 12:02:21PM CEST, atiainen@forcepoint.com wrote:

>>team_port_str() will crash when trying to print port name that was

>>just unregistered, if dellink event is handled before port removal

>>event.

>>

>>This is regression from Commit 046fb6ba0aec ("libteam: resynchronize

>>ifinfo after lost RTNLGRP_LINK notifications"), which made it free

>>all removed interfaces after ifinfo handlers are called.

>>

>>Put the ifinfo_destroy_removed() back to dellink/newlink handlers as

>>it was before that commit. Clean up the ifinfo list after change handlers

>>only if it refreshed the entire ifinfo list after lost events.

>>

>>There's still a rare possibility that dellink event is missed due to

>>full socket receive buffer, which would cause ifinfo refresh and clearing

>>removed interfaces. For this, add NULL check to team_port_str() so it

>>doesn't try to print port device name in this situation.

>>

>>Signed-off-by: Antti Tiainen <atiainen@forcepoint.com>

>

> Xin, what do you think about this patch. Is it working for you?

Putting !ifinfo check in __team_port_str() is not nice,
but well, !ifinfo->port check in ifinfo_destroy_removed()
in the other patch is not nice either.

But both will fix the issue. it's a matter of which ugliness we can bear :)
So you decide please, either way to go is fine to me.

Thanks.
Tiainen, Antti May 2, 2018, 12:26 p.m. UTC | #3
> Putting !ifinfo check in __team_port_str() is not nice,


I don't think it's so bad idea to be there in any case, as ifinfo is updated 
from events to different socket than what's triggering teamd to call this.

Actually, I think the possibility of crash to NULL ifinfo has always
been there, with e.g. two ports unregistered at the same time and
messages arriving to two different sockets in specific order.

-antti

From: Xin Long <lucien.xin@gmail.com>

Sent: Wednesday, May 2, 2018 1:46 PM
To: Jiri Pirko
Cc: Tiainen, Antti; libteam@lists.fedorahosted.org
Subject: EXTERNAL: Re: [PATCH] libteam: don't crash when trying to print unregistered device name
  

On Wed, May 2, 2018 at 6:22 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Apr 27, 2018 at 12:02:21PM CEST, atiainen@forcepoint.com wrote:

>>team_port_str() will crash when trying to print port name that was

>>just unregistered, if dellink event is handled before port removal

>>event.

>>

>>This is regression from Commit 046fb6ba0aec ("libteam: resynchronize

>>ifinfo after lost RTNLGRP_LINK notifications"), which made it free

>>all removed interfaces after ifinfo handlers are called.

>>

>>Put the ifinfo_destroy_removed() back to dellink/newlink handlers as

>>it was before that commit. Clean up the ifinfo list after change handlers

>>only if it refreshed the entire ifinfo list after lost events.

>>

>>There's still a rare possibility that dellink event is missed due to

>>full socket receive buffer, which would cause ifinfo refresh and clearing

>>removed interfaces. For this, add NULL check to team_port_str() so it

>>doesn't try to print port device name in this situation.

>>

>>Signed-off-by: Antti Tiainen <atiainen@forcepoint.com>

>

> Xin, what do you think about this patch. Is it working for you?

Putting !ifinfo check in __team_port_str() is not nice,
but well, !ifinfo->port check in ifinfo_destroy_removed()
in the other patch is not nice either.

But both will fix the issue. it's a matter of which ugliness we can bear :)
So you decide please, either way to go is fine to me.

Thanks.
Jiri Pirko May 3, 2018, 2:24 p.m. UTC | #4
Fri, Apr 27, 2018 at 12:02:21PM CEST, atiainen@forcepoint.com wrote:
>team_port_str() will crash when trying to print port name that was

>just unregistered, if dellink event is handled before port removal

>event.

>

>This is regression from Commit 046fb6ba0aec ("libteam: resynchronize

>ifinfo after lost RTNLGRP_LINK notifications"), which made it free

>all removed interfaces after ifinfo handlers are called.

>

>Put the ifinfo_destroy_removed() back to dellink/newlink handlers as

>it was before that commit. Clean up the ifinfo list after change handlers

>only if it refreshed the entire ifinfo list after lost events.

>

>There's still a rare possibility that dellink event is missed due to

>full socket receive buffer, which would cause ifinfo refresh and clearing

>removed interfaces. For this, add NULL check to team_port_str() so it

>doesn't try to print port device name in this situation.

>

>Signed-off-by: Antti Tiainen <atiainen@forcepoint.com>


This patch is corrupter, probably by your email client.
Could you please send it using "git sent-email"?
Please see file SubmittingPatches for the info how to do so.

Thanks!

Patch
diff mbox

diff --git a/include/team.h b/include/team.h
index 9ae517d..b31c8d8 100644
--- a/include/team.h
+++ b/include/team.h
@@ -223,6 +223,7 @@  enum {
  	TEAM_PORT_CHANGE	= 0x1,
  	TEAM_OPTION_CHANGE	= 0x2,
  	TEAM_IFINFO_CHANGE	= 0x4,
+	TEAM_IFINFO_REFRESH	= 0x8,
  	TEAM_ANY_CHANGE		= TEAM_PORT_CHANGE |
  				  TEAM_OPTION_CHANGE |
  				  TEAM_IFINFO_CHANGE,
diff --git a/libteam/ifinfo.c b/libteam/ifinfo.c
index 5c32a9c..46d56a2 100644
--- a/libteam/ifinfo.c
+++ b/libteam/ifinfo.c
@@ -258,6 +258,8 @@  static void obj_input_newlink(struct nl_object *obj, void *arg, bool event)
  	uint32_t ifindex;
  	int err;

+	ifinfo_destroy_removed(th);
+
  	link = (struct rtnl_link *) obj;

  	ifindex = rtnl_link_get_ifindex(link);
@@ -294,6 +296,8 @@  static void event_handler_obj_input_dellink(struct nl_object *obj, void *arg)
  	uint32_t ifindex;
  	int err;

+	ifinfo_destroy_removed(th);
+
  	link = (struct rtnl_link *) obj;

  	ifindex = rtnl_link_get_ifindex(link);
@@ -412,7 +416,8 @@  int get_ifinfo_list(struct team_handle *th)
  		}
  	}

-	ret = check_call_change_handlers(th, TEAM_IFINFO_CHANGE);
+	ret = check_call_change_handlers(th, TEAM_IFINFO_CHANGE |
+					     TEAM_IFINFO_REFRESH);
  	if (ret < 0)
  		err(th, "get_ifinfo_list: check_call_change_handers failed");
  	return ret;
diff --git a/libteam/libteam.c b/libteam/libteam.c
index 77a06dd..ce0467e 100644
--- a/libteam/libteam.c
+++ b/libteam/libteam.c
@@ -236,7 +236,7 @@  int check_call_change_handlers(struct team_handle *th,
  				break;
  		}
  	}
-	if (call_type_mask & TEAM_IFINFO_CHANGE) {
+	if (call_type_mask & TEAM_IFINFO_REFRESH) {
  		ifinfo_destroy_removed(th);
  		ifinfo_clear_changed(th);
  	}
diff --git a/libteam/stringify.c b/libteam/stringify.c
index 38f4788..f1faf90 100644
--- a/libteam/stringify.c
+++ b/libteam/stringify.c
@@ -344,7 +344,8 @@  static bool __team_port_str(struct team_port *port,
  			    team_is_port_removed(port) ? "-" :
  				team_is_port_changed(port) ? "*" : " ",
  			    ifindex,
-			    team_get_ifinfo_ifname(ifinfo),
+			    ifinfo ? team_get_ifinfo_ifname(ifinfo) :
+				"(removed)",
  			    team_is_port_link_up(port) ? "up": "down",
  			    team_get_port_speed(port),
  			    team_get_port_duplex(port) ? "FD" : "HD");