netdev - Re: [PATCH net-next 6/8] bonding: add arp_ip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <C580A3C6-BF26-4316-A6F5-6DB02319B100@cumulusnetworks.com>
Date:	Thu, 7 Nov 2013 00:56:35 -1000
From:	Scott Feldman <sfeldma@...ulusnetworks.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	vfalico@...hat.com, fubar@...ibm.com, andy@...yhouse.net,
	netdev@...r.kernel.org, shm <shm@...ulusnetworks.com>
Subject: Re: [PATCH net-next 6/8] bonding: add arp_ip_target netlink support


On Nov 7, 2013, at 12:16 AM, Jiri Pirko <jiri@...nulli.us> wrote:

> Thu, Nov 07, 2013 at 10:43:30AM CET, sfeldma@...ulusnetworks.com wrote:
>> Add IFLA_BOND_ARP_IP_TARGET to allow get/set of bonding parameter
>> arp_ip_target via netlink.
>> 
>> Signed-off-by: Scott Feldman <sfeldma@...ulusnetworks.com>
>> ---
>> drivers/net/bonding/bond_netlink.c |   43 ++++++++++++++-
>> drivers/net/bonding/bond_options.c |  102 ++++++++++++++++++++++++++++++++++++
>> drivers/net/bonding/bond_sysfs.c   |   77 +++------------------------
>> drivers/net/bonding/bonding.h      |    3 +
>> include/uapi/linux/if_link.h       |    1 
>> 5 files changed, 155 insertions(+), 71 deletions(-)
>> 
>> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>> index 9c50165..e4673ba 100644
>> --- a/drivers/net/bonding/bond_netlink.c
>> +++ b/drivers/net/bonding/bond_netlink.c
>> @@ -29,6 +29,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
>> 	[IFLA_BOND_DOWNDELAY]		= { .type = NLA_U32 },
>> 	[IFLA_BOND_USE_CARRIER]		= { .type = NLA_U8 },
>> 	[IFLA_BOND_ARP_INTERVAL]	= { .type = NLA_U32 },
>> +	[IFLA_BOND_ARP_IP_TARGET]	= { .type = NLA_NESTED },
>> };
>> 
>> static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
>> @@ -109,6 +110,21 @@ static int bond_changelink(struct net_device *bond_dev,
>> 		if (err)
>> 			return err;
>> 	}
>> +	if (data[IFLA_BOND_ARP_IP_TARGET]) {
>> +		struct nlattr *attr;
>> +		int rem;
>> +
>> +		err = bond_option_arp_ip_target_clear_all(bond);
>> +		if (err)
>> +			return err;
>> +
> 
> Hmm. I think this is not correct. I think you should not clear all. They
> might be needed to be used but for a brief moment, they disappear.
> 
> Just set or unset them one by one.

That’s a good catch.  I should compare existing set with new set and retain entries entries common to both sets (and add/remove uncommon ones).

Just to be clear, for netlink, the idea is to push a set of ip targets addrs as a nested attribute and process as a set.  This is consistent with arp_ip_target documentation in Documentation/networking/bonding.txt where arp_ip_target is passed as a comma-separated list of addrs.  However, it’s not consistent with the sysfs interface where addrs are added/removed one at a time using “+” or “-“ prefix.

e.g.:

ip link add bond7 type bond mode 1
ip link set dev bond7 type bond arp_ip_target 10.0.0.1,10.0.0.5
ip link set dev bond7 type bond arp_ip_target 10.0.0.1


>> @@ -141,6 +159,8 @@ static int bond_fill_info(struct sk_buff *skb,
>> {
>> 	struct bonding *bond = netdev_priv(bond_dev);
>> 	struct net_device *slave_dev = bond_option_active_slave_get(bond);
>> +	struct nlattr *targets;
>> +	int i, targets_added;
>> 
>> 	if (nla_put_u8(skb, IFLA_BOND_MODE, bond->params.mode))
>> 		goto nla_put_failure;
>> @@ -152,10 +172,12 @@ static int bond_fill_info(struct sk_buff *skb,
>> 	if (nla_put_u32(skb, IFLA_BOND_MIIMON, bond->params.miimon))
>> 		goto nla_put_failure;
>> 
>> -	if (nla_put_u32(skb, IFLA_BOND_UPDELAY, bond->params.updelay))
>> +	if (nla_put_u32(skb, IFLA_BOND_UPDELAY,
>> +		bond->params.updelay * bond->params.miimon))
>> 		goto nla_put_failure;
> 
> 	This bit should probably no be here :)

Drats, that change goes with earlier patch.  I’ll fix on patch set resend for issue above.

-scott--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html