netdev - Re: [PATCH linux v1 net-next 4/4] bonding: add netlink support for sys prio, actor sys mac, and port key

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] [day] [month] [year] [list]

Message-ID: <CAF2d9jhNFJUhJab0AXZ01BLjy-nRt=X0TyqdytNXQdYG2+pqjg@mail.gmail.com>
Date:	Wed, 6 May 2015 00:07:07 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Jonathan Toppins <jtoppins@...ulusnetworks.com>
Cc:	linux-netdev <netdev@...r.kernel.org>,
	Jay Vosburgh <j.vosburgh@...il.com>,
	Veaceslav Falico <vfalico@...il.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>,
	David Miller <davem@...emloft.net>,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH linux v1 net-next 4/4] bonding: add netlink support for
 sys prio, actor sys mac, and port key

On Tue, May 5, 2015 at 6:07 PM, Jonathan Toppins
<jtoppins@...ulusnetworks.com> wrote:
> On 5/4/15 4:48 PM, Jonathan Toppins wrote:
>>
>> On 5/1/15 7:15 PM, Mahesh Bandewar wrote:
>>>>
>>>>
>>>> @@ -548,6 +585,19 @@ static int bond_fill_info(struct sk_buff *skb,
>>>>                         bond->params.ad_select))
>>>>                  goto nla_put_failure;
>>>>
>>>> +       if (nla_put_u16(skb, IFLA_BOND_AD_ACTOR_SYS_PRIO,
>>>> +                       bond->params.ad_actor_sys_prio))
>>>> +               goto nla_put_failure;
>>>> +
>>>> +       if (nla_put_u16(skb, IFLA_BOND_AD_USER_PORT_KEY,
>>>> +                       bond->params.ad_user_port_key))
>>>> +               goto nla_put_failure;
>>>> +
>>>> +       if (nla_put(skb, IFLA_BOND_AD_ACTOR_SYSTEM,
>>>> +                   sizeof(bond->params.ad_actor_system),
>>>> +                   &bond->params.ad_actor_system))
>>>> +               goto nla_put_failure;
>>>> +
>>>
>>> I think this does not make sense for MODE != 8023AD. Shouldn't this be
>>> inside next block which is for the 802.3ad mode?
>
>
> Agreed, the kernel should filter attributes not useful for that mode. Will
> move the sending of these attributes to be inside the mode check below.
>
>>>
>>>>          if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>>>>                  struct ad_info info;
>>>>
>>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>>> index d9cd192..6d6e502 100644
>>>> --- a/include/uapi/linux/if_link.h
>>>> +++ b/include/uapi/linux/if_link.h
>>>> @@ -417,6 +417,9 @@ enum {
>>>>          IFLA_BOND_AD_LACP_RATE,
>>>>          IFLA_BOND_AD_SELECT,
>>>>          IFLA_BOND_AD_INFO,
>>>> +       IFLA_BOND_AD_ACTOR_SYS_PRIO,
>>>> +       IFLA_BOND_AD_USER_PORT_KEY,
>>>> +       IFLA_BOND_AD_ACTOR_SYSTEM,
>>>
>>> Even though this is available / stored in bond->param, I feel that
>>> these belong to IFLA_BOND_AD_INFO_, no?
>>
>>
>> Can see it fitting in there. Not sure of the history of the AD_INFO_*
>> object. Evaluating implementation, will respond tomorrow with conclusion.
>
>
> So I did complete a rough implementation of this in both the kernel and
> iproute2 [1][2].
>
> After having implemented the example code it became clear that we should
> continue to divide based on write permissions. That being all current
> bonding attributes that are writable exist in "IFLA_BOND_*" and all
> "IFLA_BOND_AD_INFO*" attributes are read-only. By dividing this way it makes
> consumers of the API (f.e. iprotue2) pretty straight forward to implement in
> both the write and read cases. Also all IFLA_BOND_AD_INFO attributes I do
> not see getting converted to writable, as they are generated as a result of
> lacp negotiation with input from these new ad_ attributes, agree?
>
So far what we had was only LACP negotiated values but now that we
have variables that we are setting and are used for LACP negotiations
makes it little different.

I'm fine if the overall command structure makes sense and the
read-values are kept separate from write-values as you have mentioned
and is easier to parse.

> In the implementation code for iproute2 [2] a "submode" for defining ad_info
> attributes was needed to keep the parsing simple. This felt awkward because
> all other attributes just need to be listed, with no special submode beyond
> the link type.
>
> Summary, will send a v2 with the current implementation and change
> bond_fill_info() to only send these new ad attributes if the bond is in mode
> 4, will not modify lacp_rate or ad_select at this time. Will post v2
> tomorrow if no strong disagreements.
>
> Thanks!
>
> [1] https://github.com/jtoppins/net-next/tree/ad_actor-patches-v2
> [2] https://github.com/jtoppins/iproute2/tree/ad_actor-patches-v2
>
>>
>>>
>>>>          __IFLA_BOND_MAX,
>>>>   };
>>>>
>>>> --
>>>> 1.7.10.4
>>>>
>>>> --
>>>> 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
>>
>>
>
--
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