From mboxrd@z Thu Jan 1 00:00:00 1970 From: Veaceslav Falico Subject: Re: [PATCH net-next 0/6] bonding: only rely on arp packets if arp monitor is used Date: Fri, 17 Jan 2014 07:57:18 +0100 Message-ID: <20140117065718.GA5699@redhat.com> References: <1389837916-5377-1-git-send-email-vfalico@redhat.com> <15764.1389848997@death.nxdomain> <20140116084102.GM1867@redhat.com> <21868.1389911939@death.nxdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: netdev@vger.kernel.org, Andy Gospodarek , "David S. Miller" To: Jay Vosburgh Return-path: Received: from mx1.redhat.com ([209.132.183.28]:7956 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751079AbaAQHA2 (ORCPT ); Fri, 17 Jan 2014 02:00:28 -0500 Content-Disposition: inline In-Reply-To: <21868.1389911939@death.nxdomain> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jan 16, 2014 at 02:38:59PM -0800, Jay Vosburgh wrote: =2E..snip... > I think the bottom line here is pretty simple: > > Using the ARP monitor with the loadbalance modes is not a common >configuration in my experience, and making it work is tricky. However= , >anyone using it today will be relying on the current behavior, which w= e >therefore must not change. Yep, agreed. It might be against the documentation, these use cases mig= ht be weird/illogical - but they (kind of) work, and we both agree that th= is change might break them, so it's definitely a no go. OTOH, I'd still like to help people who have some kind of broadcast tra= ffic (STP, CDP, some routing etc.) running over network and keeping their sl= aves up (and those that cannot/don't want to use arp_validate=3D3). What do you think about this*? It's on top of this series, extends arp_validate to (not) filter out ARPs on not-validated slaves and permi= ts it to be used in non-AB mode (also, we don't need that bond->lock, we'r= e always under RCU). *: diff --git a/Documentation/networking/bonding.txt b/Documentation/netwo= rking/bonding.txt index a4d925e..7223ef4 100644 --- a/Documentation/networking/bonding.txt +++ b/Documentation/networking/bonding.txt @@ -279,19 +279,45 @@ arp_validate =20 none or 0 =20 - No validation is performed. This is the default. + No validation is performed. This is the default. Any arriving + traffic (arp or non-arp) is considered a proof that the slave + is up. =20 active or 1 =20 - Validation is performed only for the active slave. + Validation is performed only for the active slave. Only ARPs + that arrive from any arp_ip_target are considered legit. The + backup slave still does no validation (as if arp_validate=3D0). =20 backup or 2 =20 - Validation is performed only for backup slaves. + Validation is performed only for backup slaves. Only ARPs + that arrive from any arp_ip_target are considered legit. The + active slave still has no validation (as if arp_validate=3D0). =20 all or 3 =20 - Validation is performed for all slaves. + Validation is performed for all slaves. Only ARPs + that arrive from any arp_ip_target are considered legit. + + arp or 4 + + Any arp packet is accepted as a proof that any slave is up, + but no IP-based validation is made. + + active_arp or 5 + + Validation is performed only for the active slave. Only ARPs + that arrive from any arp_ip_target are considered legit. The + backup slave validates only arp packets, but doesn't check the + source (as if arp_validate=3D4). + + backup_any or 6 + + Validation is performed only for backup slaves. Only ARPs + that arrive from any arp_ip_target are considered legit. The + active slave validates only arp packets, but doesn't check the + source (as if arp_validate=3D4). =20 For the active slave, the validation checks ARP replies to confirm that they were generated by an arp_ip_target. Since diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond= _main.c index 0f613ae..2ef1d5a 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -246,6 +246,9 @@ const struct bond_parm_tbl arp_validate_tbl[] =3D { { "active", BOND_ARP_VALIDATE_ACTIVE}, { "backup", BOND_ARP_VALIDATE_BACKUP}, { "all", BOND_ARP_VALIDATE_ALL}, +{ "arp", BOND_ARP_VALIDATE_ARP}, +{ "active_arp", BOND_ARP_VALIDATE_ACTIVE_ARP}, +{ "backup_arp", BOND_ARP_VALIDATE_BACKUP_ARP}, { NULL, -1}, }; =20 @@ -2284,16 +2287,15 @@ int bond_arp_rcv(const struct sk_buff *skb, str= uct bonding *bond, struct arphdr *arp =3D (struct arphdr *)skb->data; unsigned char *arp_ptr; __be32 sip, tip; - int alen; - - if (skb->protocol !=3D __cpu_to_be16(ETH_P_ARP)) - return RX_HANDLER_ANOTHER; - - read_lock(&bond->lock); + int alen, is_arp =3D skb->protocol =3D=3D __cpu_to_be16(ETH_P_ARP); =20 if (!slave_do_arp_validate(bond, slave)) { - slave->last_arp_rx =3D jiffies; - goto out_unlock; + if ((slave_do_arp_validate_only(bond, slave) && is_arp) || + !slave_do_arp_validate_only(bond, slave)) + slave->last_arp_rx =3D jiffies; + return RX_HANDLER_ANOTHER; + } else if (!is_arp) { + return RX_HANDLER_ANOTHER; } =20 alen =3D arp_hdr_len(bond->dev); @@ -2349,7 +2351,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struc= t bonding *bond, bond_validate_arp(bond, slave, tip, sip); =20 out_unlock: - read_unlock(&bond->lock); if (arp !=3D (struct arphdr *)skb->data) kfree(arp); return RX_HANDLER_ANOTHER; @@ -4181,10 +4182,6 @@ static int bond_check_params(struct bond_params = *params) } =20 if (arp_validate) { - if (bond_mode !=3D BOND_MODE_ACTIVEBACKUP) { - pr_err("arp_validate only supported in active-backup mode\n"); - return -EINVAL; - } if (!arp_interval) { pr_err("arp_validate requires arp_interval\n"); return -EINVAL; diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/b= ond_options.c index 1bab20e..9d6d231 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -434,12 +434,6 @@ int bond_option_arp_validate_set(struct bonding *b= ond, int arp_validate) return -EINVAL; } =20 - if (bond->params.mode !=3D BOND_MODE_ACTIVEBACKUP) { - pr_err("%s: arp_validate only supported in active-backup mode.\n", - bond->dev->name); - return -EINVAL; - } - pr_info("%s: setting arp_validate to %s (%d).\n", bond->dev->name, arp_validate_tbl[arp_validate].modename, arp_validate); diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bondin= g.h index 9f07af1..19eb023 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -319,6 +319,11 @@ static inline bool bond_is_active_slave(struct sla= ve *slave) #define BOND_ARP_VALIDATE_BACKUP (1 << BOND_STATE_BACKUP) #define BOND_ARP_VALIDATE_ALL (BOND_ARP_VALIDATE_ACTIVE | \ BOND_ARP_VALIDATE_BACKUP) +#define BOND_ARP_VALIDATE_ARP (BOND_ARP_VALIDATE_ALL + 1) /* =D0=B1=D0= =BB=D1=8F... */ +#define BOND_ARP_VALIDATE_ACTIVE_ARP (BOND_ARP_VALIDATE_ACTIVE | \ + BOND_ARP_VALIDATE_ARP) +#define BOND_ARP_VALIDATE_BACKUP_ARP (BOND_ARP_VALIDATE_BACKUP | \ + BOND_ARP_VALIDATE_ARP) =20 static inline int slave_do_arp_validate(struct bonding *bond, struct slave *slave) @@ -326,6 +331,12 @@ static inline int slave_do_arp_validate(struct bon= ding *bond, return bond->params.arp_validate & (1 << bond_slave_state(slave)); } =20 +static inline int slave_do_arp_validate_only(struct bonding *bond, + struct slave *slave) +{ + return bond->params.arp_validate & BOND_ARP_VALIDATE_ARP; +} + /* Get the oldest arp which we've received on this slave for bond's * arp_targets. */ > > -J