netdev - Re: [net-next 08/12] i40e: Missing response checks in driver when starting/stopping FW LLDP

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: <4157b306-b663-e20a-8c24-2d91a049e7fb@cogentembedded.com>
Date:   Fri, 14 Jun 2019 12:42:52 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     Piotr Marczak <piotr.marczak@...el.com>, netdev@...r.kernel.org,
        nhorman@...hat.com, sassmann@...hat.com,
        Andrew Bowers <andrewx.bowers@...el.com>
Subject: Re: [net-next 08/12] i40e: Missing response checks in driver when
 starting/stopping FW LLDP

Hello!

On 13.06.2019 21:53, Jeff Kirsher wrote:

> From: Piotr Marczak <piotr.marczak@...el.com>
> 
> Driver did not check response on LLDP flag change and always returned
> SUCCESS.
> 
> This patch now checks for an error and returns an error code and has
> additional information in the log.
> 
> Signed-off-by: Piotr Marczak <piotr.marczak@...el.com>
> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> ---
>   .../net/ethernet/intel/i40e/i40e_ethtool.c    | 27 +++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 7f7d04ab1515..0837c6b3e15e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
[...]
> @@ -5013,7 +5015,28 @@ static int i40e_set_priv_flags(struct net_device *dev, u32 flags)
>   			dcbcfg->pfc.willing = 1;
>   			dcbcfg->pfc.pfccap = I40E_MAX_TRAFFIC_CLASS;
>   		} else {
> -			i40e_aq_start_lldp(&pf->hw, false, NULL);
> +			status = i40e_aq_start_lldp(&pf->hw, false, NULL);
> +			if (status) {
> +				adq_err = pf->hw.aq.asq_last_status;
> +				switch (adq_err) {
> +				case I40E_AQ_RC_EEXIST:
> +					dev_warn(&pf->pdev->dev,
> +						 "FW LLDP agent is already running\n");
> +					return 0;
> +				case I40E_AQ_RC_EPERM:
> +					dev_warn(&pf->pdev->dev,
> +						 "Device configuration forbids SW from starting the LLDP agent.\n");
> +					return (-EINVAL);

    () not needed. None was used above, so why have them here?

> +				default:
> +					dev_warn(&pf->pdev->dev,
> +						 "Starting FW LLDP agent failed: error: %s, %s\n",
> +						 i40e_stat_str(&pf->hw,
> +							       status),
> +						 i40e_aq_str(&pf->hw,
> +							     adq_err));
> +					return (-EINVAL);

    Neither they are needed here...

[...]

MBR, Sergei