netdev - Re: [PATCH net-next 02/11] qed/qede: make devlink survive recovery

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200821102814.00003002@intel.com>
Date:   Fri, 21 Aug 2020 10:28:14 -0700
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     Igor Russkikh <irusskikh@...vell.com>
Cc:     <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ariel Elior <aelior@...vell.com>,
        Michal Kalderon <mkalderon@...vell.com>,
        "Alexander Lobakin" <alobakin@...vell.com>,
        Michal Kalderon <michal.kalderon@...vell.com>
Subject: Re: [PATCH net-next 02/11] qed/qede: make devlink survive recovery

Igor Russkikh wrote:

> Before that, devlink instance lifecycle was linked to qed_dev object,
> that causes devlink to be recreated on each recovery.
> 
> Changing it by making higher level driver (qede) responsible for its
> life. This way devlink will survive recoveries.
> 
> qede will store devlink structure pointer as a part of its device
> object, devlink private data contains a linkage structure, it'll
> contain extra devlink related content in following patches.
> 
> The same lifecycle should be applied to storage drivers (qedf/qedi) later.
> 
> Signed-off-by: Igor Russkikh <irusskikh@...vell.com>
> Signed-off-by: Alexander Lobakin <alobakin@...vell.com>
> Signed-off-by: Michal Kalderon <michal.kalderon@...vell.com>

<snip>

> diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 1aaae3203f5a..7c2d948b2035 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -1172,10 +1172,23 @@ static int __qede_probe(struct pci_dev *pdev, u32 dp_module, u8 dp_level,
>  			rc = -ENOMEM;
>  			goto err2;
>  		}
> +
> +		edev->devlink = qed_ops->common->devlink_register(cdev);
> +		if (IS_ERR(edev->devlink)) {
> +			DP_NOTICE(edev, "Cannot register devlink\n");
> +			edev->devlink = NULL;
> +			/* Go on, we can live without devlink */
> +		}
>  	} else {
>  		struct net_device *ndev = pci_get_drvdata(pdev);
>  
>  		edev = netdev_priv(ndev);
> +
> +		if (edev && edev->devlink) {

cppcheck notes that the edev check here before the && is either
unnecessary, or the original code had a possible null pointer
dereference.  I figure the code should just be
		if (edev->devlink) {

And I recommend that you try to run cppcheck --enable=all on your code,
it finds several style violations and a few other null pointer checks
that can be fixed up.

> +			struct qed_devlink *qdl = devlink_priv(edev->devlink);
> +
> +			qdl->cdev = cdev;
> +		}
>  		edev->cdev = cdev;
>  		memset(&edev->stats, 0, sizeof(edev->stats));
>  		memcpy(&edev->dev_info, &dev_info, sizeof(dev_info));