| Message ID | alpine.GSO.2.20.1804271256090.9078@kekkonen.niksula.hut.fi (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
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.
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.
> 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.
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!
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");
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