netdev - Re: [PATCH net-next] net: bridge: add support for sticky fdb entries

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]
Date:   Tue, 11 Sep 2018 02:55:10 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        davem@...emloft.net, bridge@...ts.linux-foundation.org
Subject: Re: [PATCH net-next] net: bridge: add support for sticky fdb entries

On 11/09/18 02:22, Grygorii Strashko wrote:
> 
> 
> On 09/10/2018 04:18 PM, Stephen Hemminger wrote:
>> On Mon, 10 Sep 2018 13:16:01 +0300
>> Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:
>>
>>> Add support for entries which are "sticky", i.e. will not change their port
>>> if they show up from a different one. A new ndm flag is introduced for that
>>> purpose - NTF_STICKY. We allow to set it only to non-local entries.
>>
>> Is there a name for this in other network switch API's?
> 
> In TI CPSW hardware we have following definition for similar FDB/ALE entries
> (AM437x TRM - 15.3.2.7.1.4 Unicast Address Table Entry):
> 
> Block (BLOCK)  – The block bit indicates that a packet with a matching source or
> destination address should be dropped (block the address).
> 0 – Address is not blocked.
> 1 – Drop a packet with a matching source or destination address (secure must be zero)
>     If block and secure are both set, then they no longer mean block and secure.
> When both are set, the block and secure bits indicate that the packet is 
> a unicast supervisory (super) packet and they determine the unicast forward state test criteria. 
> If both bits are set then the packet is forwarded if the receive port is in
>    the Forwarding/Blocking/Learning state.
> If both bits are not set then the packet is forwarded if the receive port is in
>    the Forwarding state.
> 
> Secure (SECURE) - This bit indicates that a packet with a matching source address
> should be dropped if the received port number is not equal to the table entry port_number.
> 0 – Received port number is a don’t care.
> 1 – Drop the packet if the received port is not the secure port for the source
> address and do not update the address (block must be zero)
> 
> Updating Process:
> if ((source address found) and (receive port number != port_number) and (secure or block))
> then do not update address
> 
> "sticky" would mean SECURE+BLOCK = supervisory (super) packet
> 

That could work. but we haven't really made any arrangements w.r.t. to current implementations.
So if IUC secure+block should be: 
   - block:
      1 – Drop a packet with a matching source or destination address (secure must be zero)
      If block and secure are both set, then they no longer mean block and secure.

    - secure:
      1 – Drop the packet if the received port is not the secure port for the source
          address and do not update the address (block must be zero)

These could mean a very different config based on their description. Feel free to add.

>>
>>>
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>>> ---
>>> I'll send the selftest for sticky with the iproute2 patch if this one is
>>> accepted. We've had multiple requests to support such flag and now it's
>>> also needed for some eVPN and clag setups.
>>>
>>>   include/uapi/linux/neighbour.h |  1 +
>>>   net/bridge/br_fdb.c            | 19 ++++++++++++++++---
>>>   net/bridge/br_private.h        |  1 +
>>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
>>> index 904db6148476..998155444e0d 100644
>>> --- a/include/uapi/linux/neighbour.h
>>> +++ b/include/uapi/linux/neighbour.h
>>> @@ -43,6 +43,7 @@ enum {
>>>   #define NTF_PROXY	0x08	/* == ATF_PUBL */
>>>   #define NTF_EXT_LEARNED	0x10
>>>   #define NTF_OFFLOADED   0x20
>>> +#define NTF_STICKY	0x40
>>>   #define NTF_ROUTER	0x80
>>>   
>>>   /*
>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>> index 502f66349530..26569ed06a4d 100644
>>> --- a/net/bridge/br_fdb.c
>>> +++ b/net/bridge/br_fdb.c
>>> @@ -584,7 +584,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>>   			unsigned long now = jiffies;
>>>   
>>>   			/* fastpath: update of existing entry */
>>> -			if (unlikely(source != fdb->dst)) {
>>> +			if (unlikely(source != fdb->dst && !fdb->is_sticky)) {
>>>   				fdb->dst = source;
>>>   				fdb_modified = true;
>>>   				/* Take over HW learned entry */
>>> @@ -656,6 +656,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
>>>   		ndm->ndm_flags |= NTF_OFFLOADED;
>>>   	if (fdb->added_by_external_learn)
>>>   		ndm->ndm_flags |= NTF_EXT_LEARNED;
>>> +	if (fdb->is_sticky)
>>> +		ndm->ndm_flags |= NTF_STICKY;
>>>   
>>>   	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
>>>   		goto nla_put_failure;
>>> @@ -772,7 +774,8 @@ int br_fdb_dump(struct sk_buff *skb,
>>>   
>>>   /* Update (create or replace) forwarding database entry */
>>>   static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
>>> -			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
>>> +			 const u8 *addr, u16 state, u16 flags, u16 vid,
>>> +			 u8 is_sticky)
>>
>> Why not change the API to take a full ndm flags, someone is sure to add more later.

Sure, I've added a similar "fix" on my bridge todo list which is getting very long by the way,

but it is not the point of this patch.