| 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
| ||
|
Message-ID: <CAKOOJTz4A2ER8MQE1dW27Spocds09SYafjeuLcFDJ0nL6mKyOw@mail.gmail.com>
Date: Thu, 23 Sep 2021 14:11:40 -0700
From: Edwin Peer <edwin.peer@...adcom.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Leon Romanovsky <leonro@...dia.com>,
Alexander Lobakin <alobakin@...me>,
Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>,
Ariel Elior <aelior@...vell.com>,
GR-everest-linux-l2@...vell.com,
GR-QLogic-Storage-Upstream@...vell.com,
Igor Russkikh <irusskikh@...vell.com>,
intel-wired-lan@...ts.osuosl.org,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
Javed Hasan <jhasan@...vell.com>,
Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
Jesse Brandeburg <jesse.brandeburg@...el.com>,
Jiri Pirko <jiri@...dia.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-scsi@...r.kernel.org,
"Martin K. Petersen" <martin.petersen@...cle.com>,
Michael Chan <michael.chan@...adcom.com>,
Michal Kalderon <michal.kalderon@...vell.com>,
netdev <netdev@...r.kernel.org>,
Sathya Perla <sathya.perla@...adcom.com>,
Saurav Kashyap <skashyap@...vell.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
Vasundhara Volam <vasundhara-v.volam@...adcom.com>
Subject: Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and
registration status
On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> From: Leon Romanovsky <leonro@...dia.com>
>
> devlink is a software interface that doesn't depend on any hardware
> capabilities. The failure in SW means memory issues, wrong parameters,
> programmer error e.t.c.
>
> Like any other such interface in the kernel, the returned status of
> devlink APIs should be checked and propagated further and not ignored.
>
> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
> 3 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 037767b370d5..4c483fd91dbe 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -13370,7 +13370,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
>
> bnxt_inv_fw_health_reg(bp);
> - bnxt_dl_register(bp);
> + rc = bnxt_dl_register(bp);
> + if (rc)
> + goto init_err_dl;
>
> rc = register_netdev(dev);
> if (rc)
> @@ -13390,6 +13392,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> init_err_cleanup:
> bnxt_dl_unregister(bp);
> +init_err_dl:
> bnxt_shutdown_tc(bp);
> bnxt_clear_int_mode(bp);
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index bf7d3c17049b..dc0851f709f5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -134,7 +134,7 @@ void bnxt_dl_fw_reporters_create(struct bnxt *bp)
> {
> struct bnxt_fw_health *health = bp->fw_health;
>
> - if (!bp->dl || !health)
> + if (!health)
> return;
>
> if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET) || health->fw_reset_reporter)
> @@ -188,7 +188,7 @@ void bnxt_dl_fw_reporters_destroy(struct bnxt *bp, bool all)
> {
> struct bnxt_fw_health *health = bp->fw_health;
>
> - if (!bp->dl || !health)
> + if (!health)
> return;
>
> if ((all || !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) &&
> @@ -781,6 +781,7 @@ int bnxt_dl_register(struct bnxt *bp)
> {
> const struct devlink_ops *devlink_ops;
> struct devlink_port_attrs attrs = {};
> + struct bnxt_dl *bp_dl;
> struct devlink *dl;
> int rc;
>
> @@ -795,7 +796,9 @@ int bnxt_dl_register(struct bnxt *bp)
> return -ENOMEM;
> }
>
> - bnxt_link_bp_to_dl(bp, dl);
> + bp->dl = dl;
> + bp_dl = devlink_priv(dl);
> + bp_dl->bp = bp;
>
> /* Add switchdev eswitch mode setting, if SRIOV supported */
> if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
> @@ -826,7 +829,6 @@ int bnxt_dl_register(struct bnxt *bp)
> err_dl_port_unreg:
> devlink_port_unregister(&bp->dl_port);
> err_dl_free:
> - bnxt_link_bp_to_dl(bp, NULL);
> devlink_free(dl);
> return rc;
> }
> @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
> {
> struct devlink *dl = bp->dl;
>
> - if (!dl)
> - return;
> -
minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.
> if (BNXT_PF(bp)) {
> bnxt_dl_params_unregister(bp);
> devlink_port_unregister(&bp->dl_port);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> index d889f240da2b..406dc655a5fc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> @@ -20,19 +20,6 @@ static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
> return ((struct bnxt_dl *)devlink_priv(dl))->bp;
> }
>
> -/* To clear devlink pointer from bp, pass NULL dl */
> -static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
> -{
> - bp->dl = dl;
> -
> - /* add a back pointer in dl to bp */
> - if (dl) {
> - struct bnxt_dl *bp_dl = devlink_priv(dl);
> -
> - bp_dl->bp = bp;
> - }
> -}
> -
> #define NVM_OFF_MSIX_VEC_PER_PF_MAX 108
> #define NVM_OFF_MSIX_VEC_PER_PF_MIN 114
> #define NVM_OFF_IGNORE_ARI 164
> --
> 2.31.1
>
Reviewed-by: Edwin Peer <edwin.peer@...adcom.com>
Regards,
Edwin Peer
Powered by blists - more mailing lists