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:   Sun, 25 Feb 2018 14:42:31 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Netdev <netdev@...r.kernel.org>,
        Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
Subject: Re: [Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC
 address filters specifying source addresses

On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
<vinicius.gomes@...el.com> wrote:
> Makes it possible to direct packets to queues based on their source
> address. Documents the expected usage of the 'flags' parameter.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
>  drivers/net/ethernet/intel/igb/igb.h           |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
>  3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index 573bf177fd08..c6f552de30dd 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -490,6 +490,7 @@
>   * manageability enabled, allowing us room for 15 multicast addresses.
>   */
>  #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
> +#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
>  #define E1000_RAH_QSEL_ENABLE 0x10000000
>  #define E1000_RAL_MAC_ADDR_LEN 4
>  #define E1000_RAH_MAC_ADDR_LEN 2
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 1c6b8d9176a8..d5cd5f6708d9 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -472,6 +472,7 @@ struct igb_mac_addr {
>
>  #define IGB_MAC_STATE_DEFAULT  0x1
>  #define IGB_MAC_STATE_IN_USE   0x2
> +#define IGB_MAC_STATE_SRC_ADDR  0x4
>
>  /* board specific private data structure */
>  struct igb_adapter {
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 543aa99892eb..db66b697fe3b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
>         igb_rar_set_index(adapter, 0);
>  }
>
> +/* Add a MAC filter for 'addr' directing matching traffic to 'queue',
> + * 'flags' is used to indicate what kind of match is made, match is by
> + * default for the destination address, if matching by source address
> + * is desired the flag IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>
>                 ether_addr_copy(adapter->mac_table[i].addr, addr);
>                 adapter->mac_table[i].queue = queue;
> -               adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);

More unneeded parenthesis.

>
>                 igb_rar_set_index(adapter, i);
>                 return i;
> @@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>         return -ENOSPC;
>  }
>
> +/* Remove a MAC filter for 'addr' directing matching traffic to
> + * 'queue', 'flags' is used to indicate what kind of match need to be
> + * removed, match is by default for the destination address, if
> + * matching by source address is to be removed the flag
> + * IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>          * for the VF MAC addresses.
>          */
>         for (i = 0; i < rar_entries; i++) {
> -               if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
> +               if (!(adapter->mac_table[i].state &
> +                     (IGB_MAC_STATE_IN_USE | flags)))

Shouldn't these be two separate checks? If the address isn't in use
why would I care what the flags state is? It probably isn't valid.

>                         continue;
>                 if (adapter->mac_table[i].queue != queue)
>                         continue;
>                 if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>                         continue;
>
> -               adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);

Maybe instead of just clearing the specific flags we should just clear
the state and reset it back to 0.

>                 memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>                 adapter->mac_table[i].queue = 0;
>
> @@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
>         struct igb_adapter *adapter = netdev_priv(netdev);
>         int ret;
>
> -       ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       ret = igb_add_mac_filter(adapter, addr,
> +                                adapter->vfs_allocated_count, 0);

Instead of having to add a 0 to all these functions it might be better
to rename the original igb_add_mac_filter, and then add a new function
named the same as the original that just automatically passes the 0.
It will take a lot of noise out of this code.

>
>         return min_t(int, ret, 0);
>  }
> @@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>  {
>         struct igb_adapter *adapter = netdev_priv(netdev);
>
> -       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
>
>         return 0;
>  }
> @@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         if (entry->vf == vf) {
>                                 entry->vf = -1;
>                                 entry->free = true;
> -                               igb_del_mac_filter(adapter, entry->vf_mac, vf);
> +                               igb_del_mac_filter(adapter,
> +                                                  entry->vf_mac, vf, 0);
>                         }
>                 }
>                 break;
> @@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         entry->vf = vf;
>                         ether_addr_copy(entry->vf_mac, addr);
>
> -                       ret = igb_add_mac_filter(adapter, addr, vf);
> +                       ret = igb_add_mac_filter(adapter, addr, vf, 0);
>                         ret = min_t(int, ret, 0);
>                 } else {
>                         ret = -ENOSPC;
> @@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
>                 if (is_valid_ether_addr(addr))
>                         rar_high |= E1000_RAH_AV;
>
> +               if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
> +                       rar_high |= E1000_RAH_ASEL_SRC_ADDR;
> +
>                 switch (hw->mac.type) {
>                 case e1000_82575:
>                 case e1000_i210:
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Powered by blists - more mailing lists