2021-05-21 13:31:44

by Jarod Wilson

Subject: [PATCH net-next v2 4/4] bonding/balance-alb: put all slaves into promisc

Unlike most other modes with a primary interface, ALB mode bonding can
receive on all slaves. That includes traffic destined for a non-local MAC
behind a bridge on top of the bond. Such traffic gets dropped if the
interface isn't in promiscuous mode. Therefore, it would seem to make
sense to put all slaves into promisc.

Cc: Jay Vosburgh <[email protected]>
Cc: Veaceslav Falico <[email protected]>
Cc: Andy Gospodarek <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Thomas Davis <[email protected]>
Cc: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
drivers/net/bonding/bond_main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 32785e9d0295..6d95f9e46059 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -647,9 +647,10 @@ static int bond_check_dev_link(struct bonding *bond,
static int bond_set_promiscuity(struct bonding *bond, int inc)
{
struct list_head *iter;
- int err = 0;
+ int mode, err = 0;

- if (bond_uses_primary(bond)) {
+ mode = BOND_MODE(bond);
+ if (mode == BOND_MODE_ACTIVEBACKUP || mode == BOND_MODE_TLB) {
struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);

if (curr_active)
--
2.30.2

2021-05-21 20:23:55

by Jay Vosburgh

Subject: Re: [PATCH net-next v2 4/4] bonding/balance-alb: put all slaves into promisc

Jarod Wilson <[email protected]> wrote:

>Unlike most other modes with a primary interface, ALB mode bonding can
>receive on all slaves. That includes traffic destined for a non-local MAC
>behind a bridge on top of the bond. Such traffic gets dropped if the
>interface isn't in promiscuous mode. Therefore, it would seem to make
>sense to put all slaves into promisc.

I'm still confused by this description and the actual changes of
the patch; the description above reads to me as you're intending that
all slaves of an alb bond should be promisc all the time, but the patch
below doesn't do that (it causes all alb mode slaves to be set to
promisc when the bond itself is set to promisc mode). Could you
clarify?

Also, your phrasing that "it would seem to make sense" suggests
to me that this change is speculative. Do you have a specific example
of when the prior behavior causes an issue?

-J

>Cc: Jay Vosburgh <[email protected]>
>Cc: Veaceslav Falico <[email protected]>
>Cc: Andy Gospodarek <[email protected]>
>Cc: "David S. Miller" <[email protected]>
>Cc: Jakub Kicinski <[email protected]>
>Cc: Thomas Davis <[email protected]>
>Cc: [email protected]
>Signed-off-by: Jarod Wilson <[email protected]>
>---
> drivers/net/bonding/bond_main.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 32785e9d0295..6d95f9e46059 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -647,9 +647,10 @@ static int bond_check_dev_link(struct bonding *bond,
> static int bond_set_promiscuity(struct bonding *bond, int inc)
> {
> struct list_head *iter;
>- int err = 0;
>+ int mode, err = 0;
>
>- if (bond_uses_primary(bond)) {
>+ mode = BOND_MODE(bond);
>+ if (mode == BOND_MODE_ACTIVEBACKUP || mode == BOND_MODE_TLB) {
> struct slave *curr_active = rtnl_dereference(bond->curr_active_slave);
>
> if (curr_active)
>--
>2.30.2


---
-Jay Vosburgh, [email protected]