diff mbox series

teamd: fix possible race in master ifname callback

Message ID 20200203110749.8912-1-jiri@resnulli.us (mailing list archive)
State New
Headers show
Series teamd: fix possible race in master ifname callback | expand

Commit Message

Jiri Pirko Feb. 3, 2020, 11:07 a.m. UTC
From: Jiri Pirko <jiri@mellanox.com>

In teamd the messages from kernel are processed in order:
options
ifinfo
ports

However, kernel sends the messages in a different order. See:
team_upper_dev_link() which generates ifinfo notification
__team_port_change_port_added() which generates ports notification
__team_options_change_check() which generates options notification

So there is a chance that the teamd processed only the port without
options and right after that it processes ifinfo.

Fix this by introducing teamd_port_enabled_check() which does not log
error and ignore the port in case this call fails. This is safe
as this is going to be handled by future
link_watch_enabled_option_changed() call.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Fixes: 5f351666409f ("teamd: add port_master_ifindex_changed for link_watch_port_watch_ops")
Tested-by: Stepan Blyshchak <stepanb@mellanox.com>
---
 teamd/teamd.h            |  2 ++
 teamd/teamd_link_watch.c | 13 ++++++++++---
 teamd/teamd_per_port.c   | 24 +++++++++++++++++++-----
 3 files changed, 31 insertions(+), 8 deletions(-)

Comments

Xin Long Feb. 3, 2020, 3:54 p.m. UTC | #1
On Mon, Feb 3, 2020 at 7:07 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@mellanox.com>
>
> In teamd the messages from kernel are processed in order:
> options
> ifinfo
> ports
>
> However, kernel sends the messages in a different order. See:
> team_upper_dev_link() which generates ifinfo notification
> __team_port_change_port_added() which generates ports notification
> __team_options_change_check() which generates options notification
>
> So there is a chance that the teamd processed only the port without
> options and right after that it processes ifinfo.
>
> Fix this by introducing teamd_port_enabled_check() which does not log
> error and ignore the port in case this call fails. This is safe
> as this is going to be handled by future
> link_watch_enabled_option_changed() call.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Fixes: 5f351666409f ("teamd: add port_master_ifindex_changed for link_watch_port_watch_ops")
> Tested-by: Stepan Blyshchak <stepanb@mellanox.com>
Reviewed-by: Xin Long <lucien.xin@gmail.com>

> ---
>  teamd/teamd.h            |  2 ++
>  teamd/teamd_link_watch.c | 13 ++++++++++---
>  teamd/teamd_per_port.c   | 24 +++++++++++++++++++-----
>  3 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/teamd/teamd.h b/teamd/teamd.h
> index fb2872efd0ac..f94c918cfe52 100644
> --- a/teamd/teamd.h
> +++ b/teamd/teamd.h
> @@ -325,6 +325,8 @@ int teamd_port_remove_all(struct teamd_context *ctx);
>  void teamd_port_obj_remove_all(struct teamd_context *ctx);
>  int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
>                        bool *enabled);
> +int teamd_port_enabled_check(struct teamd_context *ctx,
> +                            struct teamd_port *tdport, bool *enabled);
>  int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport);
>  int teamd_port_check_enable(struct teamd_context *ctx,
>                             struct teamd_port *tdport,
> diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
> index 5c626eb3e636..cae6549d613a 100644
> --- a/teamd/teamd_link_watch.c
> +++ b/teamd/teamd_link_watch.c
> @@ -423,9 +423,16 @@ static int link_watch_refresh_forced_send(struct teamd_context *ctx)
>         int err;
>
>         teamd_for_each_tdport(tdport, ctx) {
> -               err = teamd_port_enabled(ctx, tdport, &port_enabled);
> -               if (err)
> -                       return err;
> +               err = teamd_port_enabled_check(ctx, tdport, &port_enabled);
> +               if (err) {
> +                       /* Looks like the options are not ready for this port.
> +                        * This can happen when called from
> +                        * link_watch_port_master_ifindex_changed(). Skip this
> +                        * for now, let it be handled by future call of
> +                        * link_watch_enabled_option_changed().
> +                        */
> +                       continue;
> +               }
>                 __set_forced_send_for_port(tdport, port_enabled);
>                 if (port_enabled)
>                         enabled_port_count++;
> diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
> index 327aefbe9a44..166da576ef7c 100644
> --- a/teamd/teamd_per_port.c
> +++ b/teamd/teamd_per_port.c
> @@ -389,19 +389,21 @@ int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name)
>         return teamd_port_remove(ctx, tdport);
>  }
>
> -int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
> -                      bool *enabled)
> +int __teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
> +                        bool *enabled, bool may_fail)
>  {
>         struct team_option *option;
>
>         option = team_get_option(ctx->th, "np", "enabled", tdport->ifindex);
>         if (!option) {
> -               teamd_log_err("%s: Failed to find \"enabled\" option.",
> -                             tdport->ifname);
> +               if (!may_fail)
> +                       teamd_log_err("%s: Failed to find \"enabled\" option.",
> +                                     tdport->ifname);
>                 return -ENOENT;
>         }
>         if (team_get_option_type(option) != TEAM_OPTION_TYPE_BOOL) {
> -               teamd_log_err("Unexpected type of \"enabled\" option.");
> +               if (!may_fail)
> +                       teamd_log_err("Unexpected type of \"enabled\" option.");
>                 return -EINVAL;
>         }
>
> @@ -409,6 +411,18 @@ int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
>         return 0;
>  }
>
> +int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
> +                      bool *enabled)
> +{
> +       return __teamd_port_enabled(ctx, tdport, enabled, false);
> +}
> +
> +int teamd_port_enabled_check(struct teamd_context *ctx,
> +                            struct teamd_port *tdport, bool *enabled)
> +{
> +       return __teamd_port_enabled(ctx, tdport, enabled, true);
> +}
> +
>  int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport)
>  {
>         int prio;
> --
> 2.21.0
>
Jiri Pirko Feb. 4, 2020, 5:18 p.m. UTC | #2
Mon, Feb 03, 2020 at 04:54:35PM CET, lucien.xin@gmail.com wrote:
>On Mon, Feb 3, 2020 at 7:07 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> In teamd the messages from kernel are processed in order:
>> options
>> ifinfo
>> ports
>>
>> However, kernel sends the messages in a different order. See:
>> team_upper_dev_link() which generates ifinfo notification
>> __team_port_change_port_added() which generates ports notification
>> __team_options_change_check() which generates options notification
>>
>> So there is a chance that the teamd processed only the port without
>> options and right after that it processes ifinfo.
>>
>> Fix this by introducing teamd_port_enabled_check() which does not log
>> error and ignore the port in case this call fails. This is safe
>> as this is going to be handled by future
>> link_watch_enabled_option_changed() call.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> Fixes: 5f351666409f ("teamd: add port_master_ifindex_changed for link_watch_port_watch_ops")
>> Tested-by: Stepan Blyshchak <stepanb@mellanox.com>
>Reviewed-by: Xin Long <lucien.xin@gmail.com>

applied, thanks!
diff mbox series

Patch

diff --git a/teamd/teamd.h b/teamd/teamd.h
index fb2872efd0ac..f94c918cfe52 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -325,6 +325,8 @@  int teamd_port_remove_all(struct teamd_context *ctx);
 void teamd_port_obj_remove_all(struct teamd_context *ctx);
 int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
 		       bool *enabled);
+int teamd_port_enabled_check(struct teamd_context *ctx,
+			     struct teamd_port *tdport, bool *enabled);
 int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport);
 int teamd_port_check_enable(struct teamd_context *ctx,
 			    struct teamd_port *tdport,
diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
index 5c626eb3e636..cae6549d613a 100644
--- a/teamd/teamd_link_watch.c
+++ b/teamd/teamd_link_watch.c
@@ -423,9 +423,16 @@  static int link_watch_refresh_forced_send(struct teamd_context *ctx)
 	int err;
 
 	teamd_for_each_tdport(tdport, ctx) {
-		err = teamd_port_enabled(ctx, tdport, &port_enabled);
-		if (err)
-			return err;
+		err = teamd_port_enabled_check(ctx, tdport, &port_enabled);
+		if (err) {
+			/* Looks like the options are not ready for this port.
+			 * This can happen when called from
+			 * link_watch_port_master_ifindex_changed(). Skip this
+			 * for now, let it be handled by future call of
+			 * link_watch_enabled_option_changed().
+			 */
+			continue;
+		}
 		__set_forced_send_for_port(tdport, port_enabled);
 		if (port_enabled)
 			enabled_port_count++;
diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 327aefbe9a44..166da576ef7c 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -389,19 +389,21 @@  int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name)
 	return teamd_port_remove(ctx, tdport);
 }
 
-int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
-		       bool *enabled)
+int __teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
+			 bool *enabled, bool may_fail)
 {
 	struct team_option *option;
 
 	option = team_get_option(ctx->th, "np", "enabled", tdport->ifindex);
 	if (!option) {
-		teamd_log_err("%s: Failed to find \"enabled\" option.",
-			      tdport->ifname);
+		if (!may_fail)
+			teamd_log_err("%s: Failed to find \"enabled\" option.",
+				      tdport->ifname);
 		return -ENOENT;
 	}
 	if (team_get_option_type(option) != TEAM_OPTION_TYPE_BOOL) {
-		teamd_log_err("Unexpected type of \"enabled\" option.");
+		if (!may_fail)
+			teamd_log_err("Unexpected type of \"enabled\" option.");
 		return -EINVAL;
 	}
 
@@ -409,6 +411,18 @@  int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
 	return 0;
 }
 
+int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
+		       bool *enabled)
+{
+	return __teamd_port_enabled(ctx, tdport, enabled, false);
+}
+
+int teamd_port_enabled_check(struct teamd_context *ctx,
+			     struct teamd_port *tdport, bool *enabled)
+{
+	return __teamd_port_enabled(ctx, tdport, enabled, true);
+}
+
 int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport)
 {
 	int prio;