| Message ID | 20210521132756.1811620-3-jarod@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/4] bonding: add pure source-mac-based tx hashing option | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Jarod Wilson <jarod@redhat.com> wrote: >With a virtual machine behind a bridge that directly incorporates a >balance-alb bond as one of its ports, outgoing traffic should retain the >VM's source MAC. That works fine most of the time, until doing a failover, >and then the MAC gets rewritten to the bond slave's MAC, and the return >traffic gets dropped. If we don't rewrite the MAC there, we don't lose the >traffic. Comparing the description above to the patch, is the erroneous behavior really related to failover (i.e., bond slave goes down, bond reshuffles various things as it is wont to do), or is it related to either a TX side rebalance or even simply that specific traffic is being sent on a slave that isn't the curr_active_slave? One more comment, below. >Cc: Jay Vosburgh <j.vosburgh@gmail.com> >Cc: Veaceslav Falico <vfalico@gmail.com> >Cc: Andy Gospodarek <andy@greyhouse.net> >Cc: "David S. Miller" <davem@davemloft.net> >Cc: Jakub Kicinski <kuba@kernel.org> >Cc: Thomas Davis <tadavis@lbl.gov> >Cc: netdev@vger.kernel.org >Signed-off-by: Jarod Wilson <jarod@redhat.com> >--- > drivers/net/bonding/bond_alb.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >index 3455f2cc13f2..c57f62e43328 100644 >--- a/drivers/net/bonding/bond_alb.c >+++ b/drivers/net/bonding/bond_alb.c >@@ -1302,6 +1302,23 @@ void bond_alb_deinitialize(struct bonding *bond) > rlb_deinitialize(bond); > } > >+static bool bond_alb_bridged_mac(struct bonding *bond, struct ethhdr *eth_data) >+{ >+ if (BOND_MODE(bond) != BOND_MODE_ALB) >+ return false; >+ >+ /* Don't modify source MACs that do not originate locally >+ * (e.g.,arrive via a bridge). >+ */ >+ if (!netif_is_bridge_port(bond->dev)) >+ return false; Repeating my comment (from my response to the v1 patch) that hasn't been addressed: I believe this logic will fail if the plumbing is, e.g., bond -> vlan -> bridge, as netif_is_bridge_port() would not return true for the bond in that case. Making this reliable is tricky at best, and may be impossible to be correct for all possible cases. As such, I think the comment above should reflect the limited scope of what is actually being checked here (i.e., the bond itself is directly a bridge port). Ideally, the bonding.rst documentation should describe the special behaviors of alb mode when configured as a bridge port. -J >+ >+ if (bond_slave_has_mac_rx(bond, eth_data->h_source)) >+ return false; >+ >+ return true; >+} >+ > static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond, > struct slave *tx_slave) > { >@@ -1316,7 +1333,8 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond, > } > > if (tx_slave && bond_slave_can_tx(tx_slave)) { >- if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) { >+ if (tx_slave != rcu_access_pointer(bond->curr_active_slave) && >+ !bond_alb_bridged_mac(bond, eth_data)) { > ether_addr_copy(eth_data->h_source, > tx_slave->dev->dev_addr); > } >-- >2.30.2 --- -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index 3455f2cc13f2..c57f62e43328 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -1302,6 +1302,23 @@ void bond_alb_deinitialize(struct bonding *bond) rlb_deinitialize(bond); } +static bool bond_alb_bridged_mac(struct bonding *bond, struct ethhdr *eth_data) +{ + if (BOND_MODE(bond) != BOND_MODE_ALB) + return false; + + /* Don't modify source MACs that do not originate locally + * (e.g.,arrive via a bridge). + */ + if (!netif_is_bridge_port(bond->dev)) + return false; + + if (bond_slave_has_mac_rx(bond, eth_data->h_source)) + return false; + + return true; +} + static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond, struct slave *tx_slave) { @@ -1316,7 +1333,8 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond, } if (tx_slave && bond_slave_can_tx(tx_slave)) { - if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) { + if (tx_slave != rcu_access_pointer(bond->curr_active_slave) && + !bond_alb_bridged_mac(bond, eth_data)) { ether_addr_copy(eth_data->h_source, tx_slave->dev->dev_addr); }
With a virtual machine behind a bridge that directly incorporates a balance-alb bond as one of its ports, outgoing traffic should retain the VM's source MAC. That works fine most of the time, until doing a failover, and then the MAC gets rewritten to the bond slave's MAC, and the return traffic gets dropped. If we don't rewrite the MAC there, we don't lose the traffic. Cc: Jay Vosburgh <j.vosburgh@gmail.com> Cc: Veaceslav Falico <vfalico@gmail.com> Cc: Andy Gospodarek <andy@greyhouse.net> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Thomas Davis <tadavis@lbl.gov> Cc: netdev@vger.kernel.org Signed-off-by: Jarod Wilson <jarod@redhat.com> --- drivers/net/bonding/bond_alb.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)