[libteam PATCH] teamd: update ctx->hwaddr after setting ctx->ifindex to new hwaddr

[Hangbin Liu]

When we add the first slave to team port, we will update ctx->ifindex
with new hwaddr in function

teamd_event_watch_port_added()
  - teamd_hwaddr_check_change(),

But we didn't update the ctx->hwaddr, which will cause the first added
slave set to team's init hwaddr again later. e.g. in the following functions

lacp_port_set_mac()
lb_event_watch_port_added()
ab_hwaddr_policy_same_all_port_added().

The tdport's hwaddr will be reset based on ctx->hwaddr. Fix it by updating
ctx->hwaddr when set ctx->ifindex to new hwaddr.

Note: function teamd_set_hwaddr() is not considered as it will set
ctx->hwaddr_explicit = true.

Signed-off-by: Hangbin Liu 
---
 teamd/teamd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/teamd/teamd.c b/teamd/teamd.c
index 6c47312..9622da1 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -867,7 +867,7 @@ static int teamd_add_ports(struct teamd_context *ctx)
 static int teamd_hwaddr_check_change(struct teamd_context *ctx,
    			     struct teamd_port *tdport)
 {
-	const char *hwaddr;
+	char *hwaddr;
    unsigned char hwaddr_len;
    int err;

@@ -885,6 +885,8 @@ static int teamd_hwaddr_check_change(struct teamd_context *ctx,
    	teamd_log_err("Failed to set team device hardware address.");
    	return err;
    }
+	ctx->hwaddr = hwaddr;
+	ctx->hwaddr_len = hwaddr_len;
    return 0;
 }

[Jiri Pirko]

Tue, Nov 19, 2019 at 11:31:00AM CET, [e-mail] wrote:
> When we add the first slave to team port, we will update ctx->ifindex
> with new hwaddr in function

I don't understand this sentence :(
> teamd_event_watch_port_added()
> teamd_hwaddr_check_change(),
>
> But we didn't update the ctx->hwaddr, which will cause the first added
> slave set to team's init hwaddr again later. e.g. in the following functions

I don't understand this either :/
> lacp_port_set_mac()
> lb_event_watch_port_added()
> ab_hwaddr_policy_same_all_port_added().
>
> The tdport's hwaddr will be reset based on ctx->hwaddr. Fix it by updating
> ctx->hwaddr when set ctx->ifindex to new hwaddr.

"set ifindex to new hwaddr"

I think I know (from the code) what you want to do, but the description
didn't help me. On contratry, it makes me confused.

Could you please fix the description?
> Note: function teamd_set_hwaddr() is not considered as it will set
> ctx->hwaddr_explicit = true.
> Signed-off-by: Hangbin Liu
>
> teamd/teamd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/teamd/teamd.c b/teamd/teamd.c
> index 6c47312..9622da1 100644
> --- a/teamd/teamd.c
> +++ b/teamd/teamd.c
> @@ -867,7 +867,7 @@ static int teamd_add_ports(struct teamd_context *ctx)
> static int teamd_hwaddr_check_change(struct teamd_context *ctx,
>    			     struct teamd_port *tdport)
> {
> const char *hwaddr;
> char *hwaddr;
> unsigned char hwaddr_len;
> int err;
>
> @@ -885,6 +885,8 @@ static int teamd_hwaddr_check_change(struct teamd_context *ctx,
>    	teamd_log_err("Failed to set team device hardware address.");
>    	return err;
>    }
> ctx->hwaddr = hwaddr;
> ctx->hwaddr_len = hwaddr_len;
> return 0;
>
> }
>
> -- 
> 2.19.2

[libteam PATCHv2] teamd: update ctx->hwaddr after setting team dev to new hwaddr

[Hangbin Liu]

When adding the first slave to team dev, the team dev's hwaddr will
be updated to this slave's hwaddr in function:
  teamd_event_watch_port_added()
    - teamd_hwaddr_check_change(),
But we didn't update the ctx->hwaddr, which is still the team's init hwaddr.

Later in the following functions:
  lacp_port_set_mac()
  lb_event_watch_port_added()
  ab_hwaddr_policy_same_all_port_added()
they will set the first slave's hwaddr to team's init hwaddr(ctx->hwaddr).

This will cause that the first slave(most likely the active slave)'s hwaddr
changes to team dev's original hwaddr, and later back to its old hwaddr
again, which would make the LACPDUs have different Actor System IDs.

Fix it by updating ctx->hwaddr when set ctx->ifindex to new hwaddr.

Note that teamd_set_hwaddr() doesn't need this fix as it will set
ctx->hwaddr_explicit = true.

v2: update commit description, no code changes.

Signed-off-by: Hangbin Liu 
---
 teamd/teamd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/teamd/teamd.c b/teamd/teamd.c
index 6c47312..9622da1 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -867,7 +867,7 @@ static int teamd_add_ports(struct teamd_context *ctx)
 static int teamd_hwaddr_check_change(struct teamd_context *ctx,
    			     struct teamd_port *tdport)
 {
-	const char *hwaddr;
+	char *hwaddr;
    unsigned char hwaddr_len;
    int err;

@@ -885,6 +885,8 @@ static int teamd_hwaddr_check_change(struct teamd_context *ctx,
    	teamd_log_err("Failed to set team device hardware address.");
    	return err;
    }
+	ctx->hwaddr = hwaddr;
+	ctx->hwaddr_len = hwaddr_len;
    return 0;
 }

[libteam PATCHv2] teamd: update ctx->hwaddr after setting team dev to new hwaddr

[Jiri Pirko]

Wed, Dec 04, 2019 at 08:17:11AM CET, [e-mail] wrote:
> When adding the first slave to team dev, the team dev's hwaddr will
> be updated to this slave's hwaddr in function:
>  teamd_event_watch_port_added()
> teamd_hwaddr_check_change(),
>
> But we didn't update the ctx->hwaddr, which is still the team's init hwaddr.
>
> Later in the following functions:
>  lacp_port_set_mac()
>  lb_event_watch_port_added()
>  ab_hwaddr_policy_same_all_port_added()
> they will set the first slave's hwaddr to team's init hwaddr(ctx->hwaddr).
>
> This will cause that the first slave(most likely the active slave)'s hwaddr
> changes to team dev's original hwaddr, and later back to its old hwaddr
> again, which would make the LACPDUs have different Actor System IDs.
>
> Fix it by updating ctx->hwaddr when set ctx->ifindex to new hwaddr.
>
> Note that teamd_set_hwaddr() doesn't need this fix as it will set
> ctx->hwaddr_explicit = true.
>
> v2: update commit description, no code changes.
>
> Signed-off-by: Hangbin Liu

applied, thanks.

Next time, please put the "v2" notes under "---"

[libteam PATCHv2] teamd: update ctx->hwaddr after setting team dev to new hwaddr

[Hangbin Liu]

On Wed, Dec 11, 2019 at 11:44:47AM +0100, Jiri Pirko wrote:
> Wed, Dec 04, 2019 at 08:17:11AM CET, [e-mail] wrote:
> > When adding the first slave to team dev, the team dev's hwaddr will
> > be updated to this slave's hwaddr in function:
> >  teamd_event_watch_port_added()
> > teamd_hwaddr_check_change(),
> >
> > But we didn't update the ctx->hwaddr, which is still the team's init hwaddr.
> >
> > Later in the following functions:
> >  lacp_port_set_mac()
> >  lb_event_watch_port_added()
> >  ab_hwaddr_policy_same_all_port_added()
> > they will set the first slave's hwaddr to team's init hwaddr(ctx->hwaddr).
> >
> > This will cause that the first slave(most likely the active slave)'s hwaddr
> > changes to team dev's original hwaddr, and later back to its old hwaddr
> > again, which would make the LACPDUs have different Actor System IDs.
> >
> > Fix it by updating ctx->hwaddr when set ctx->ifindex to new hwaddr.
> >
> > Note that teamd_set_hwaddr() doesn't need this fix as it will set
> > ctx->hwaddr_explicit = true.
> >
> > v2: update commit description, no code changes.
> >
> > Signed-off-by: Hangbin Liu
>
> applied, thanks.
>
> Next time, please put the "v2" notes under "---"

Thanks for the reviewing, and thanks for the tips.

Regards
Hangbin