netdev - Re: [net-next 12/14] i40e: allow i40e_update_filter

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, 12 Feb 2017 13:17:34 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     Jacob Keller <jacob.e.keller@...el.com>, netdev@...r.kernel.org,
        nhorman@...hat.com, sassmann@...hat.com, jogreene@...hat.com
Subject: Re: [net-next 12/14] i40e: allow i40e_update_filter_state to skip
 broadcast filters

Hello!

On 2/12/2017 8:30 AM, Jeff Kirsher wrote:

> From: Jacob Keller <jacob.e.keller@...el.com>
>
> Fix a bug where we modified the mac_filter_hash while outside a lock,
> when handling addition of broadcast filters.
>
> Normally, we add filters to firmware by batching the additions into
> lists and issuing 1 update for every few filters. Broadcast filters are
> handled differently, by instead setting the broadcast promiscuous mode
> flags. In order to make sure the 1<->1 mapping of filters in our
> addition array lined up with filters in the hlist tmp_add_list, we had
> to remove the filter and move it back to the main hash. However, we
> didn't do this under lock, which could cause consistency problems for
> the list.
>
> Fix this by updating i40e_update_filter_state logic so that it knows to
> avoid broadcast filters. This ensures that we don't have to remove the
> filter separately, and can put it back using the normal flow.
>
> Change-ID: Id288fade80b3e3a9a54b68cc249188cb95147518
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 37 ++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index fa4a04d..06c80d4 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -1843,6 +1843,31 @@ static void i40e_undo_filter_entries(struct i40e_vsi *vsi,
>  }
>
>  /**
> + * i40e_next_entry - Get the next non-broadcast filter from a list
> + * @f: pointer to filter in list
> + *
> + * Returns the next non-broadcast filter in the list. Required so that we
> + * ignore broadcast filters within the list, since these are not handled via
> + * the normal firmware update path.
> + */
> +static struct i40e_mac_filter *i40e_next_filter(struct i40e_mac_filter *f)
> +{
> +	while (f) {
> +		f = hlist_entry(f->hlist.next,
> +				typeof(struct i40e_mac_filter),
> +				hlist);
> +
> +		/* keep going if we found a broadcast filter */
> +		if (f && is_broadcast_ether_addr(f->macaddr))
> +			continue;
> +
> +		break;

    Isn't it simpler to *break* on an inverted condition above?
This way, *continue* isn;'t needed...

[...]

MBR, Sergei