| 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: <20260130113756.00002279@huawei.com>
Date: Fri, 30 Jan 2026 11:37:56 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>
CC: <jgg@...pe.ca>, <michael.chan@...adcom.com>,
<linux-kernel@...r.kernel.org>, <dave.jiang@...el.com>, <saeedm@...dia.com>,
<gospo@...adcom.com>, <selvin.xavier@...adcom.com>, <leon@...nel.org>,
<kalesh-anakkur.purayil@...adcom.com>
Subject: Re: [PATCH v3 fwctl 2/5] fwctl/bnxt_en: Refactor aux bus functions
to be more generic
On Thu, 29 Jan 2026 07:54:50 -0800
Pavan Chebbi <pavan.chebbi@...adcom.com> wrote:
> Up until now there was only one auxiliary device that bnxt
> created and that was for RoCE driver. bnxt fwctl is also
> going to use an aux bus device that bnxt should create.
> This requires some nomenclature changes and refactoring of
> the existing bnxt aux dev functions.
Minor thing but why wrap to 60 ish chars? Convention for much of the kernel
is 75 (to give a bit of room for git tools deciding to indent it
a little).
>
> Convert 'aux_priv' and 'edev' members of struct bnxt into
> arrays where each element contains supported auxbus device's
> data. Move struct bnxt_aux_priv from bnxt.h to ulp.h because
> that is where it belongs. Make aux bus init/uninit/add/del
> functions more generic which will loop through all the aux
> device types. Make bnxt_ulp_start/stop functions (the only
> other common functions applicable to any aux device) loop
> through the aux devices to update their config and states.
> Make callers of bnxt_ulp_start() call it only when there
> are no errors.
>
> Also, as an improvement in code, bnxt_register_dev() can skip
> unnecessary dereferencing of edev from bp, instead use the
> edev pointer from the function parameter.
>
> Future patches will reuse these functions to add an aux bus
> device for fwctl.
>
> Reviewed-by: Andy Gospodarek <gospo@...adcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
Hi Pavan,
Various comments inline. Please also make sure to run a few
static analysis tools over the code. There seems to be a use
after free that is pretty obvious in here, so I'd expect that
to be picked up. That stuff is easier for tooling that humans!
Jonathan
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 47 ++-
> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 19 +-
> .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 8 +-
> .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 2 +-
> drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 339 +++++++++++-------
> include/linux/bnxt/ulp.h | 25 +-
> 6 files changed, 273 insertions(+), 167 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 4481d80cdfc2..a9bfbfabf121 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> static int bnxt_init_board(struct pci_dev *pdev, struct net_device *dev)
> @@ -16211,12 +16214,13 @@ static void bnxt_remove_one(struct pci_dev *pdev)
> if (BNXT_PF(bp))
> __bnxt_sriov_disable(bp);
>
> - bnxt_rdma_aux_device_del(bp);
> + bnxt_aux_devices_del(bp);
>
> unregister_netdev(dev);
> bnxt_ptp_clear(bp);
>
> - bnxt_rdma_aux_device_uninit(bp);
> + bnxt_aux_devices_uninit(bp);
> + bnxt_auxdev_id_free(bp, bp->auxdev_id);
I'm not following why there is a mix of devices and single
device ID freeing going on here.
If we have a set of devices, how come there is only one of those ids?
Maybe it's just a naming thing and auxdev_id is only meant to be used
for this one auxdev that is already in the driver?
>
> bnxt_free_l2_filters(bp, true);
> bnxt_free_ntp_fltrs(bp, true);
> @@ -16802,7 +16806,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> bnxt_set_tpa_flags(bp);
> bnxt_init_ring_params(bp);
> bnxt_set_ring_params(bp);
> - bnxt_rdma_aux_device_init(bp);
> + mutex_init(&bp->auxdev_lock);
> + if (!bnxt_auxdev_id_alloc(bp))
> + bnxt_aux_devices_init(bp);
> rc = bnxt_set_dflt_rings(bp, true);
> if (rc) {
> if (BNXT_VF(bp) && rc == -ENODEV) {
> @@ -16866,7 +16872,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> bnxt_dl_fw_reporters_create(bp);
>
> - bnxt_rdma_aux_device_add(bp);
> + bnxt_aux_devices_add(bp);
>
> bnxt_print_device_info(bp);
>
> @@ -16874,7 +16880,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> return 0;
> init_err_cleanup:
> - bnxt_rdma_aux_device_uninit(bp);
> + bbnxt_aux_devices_uninitnxt_aux_devices_uninit(bp);
> + bnxt_auxdev_id_free(bp, bp->auxdev_id);
> bnxt_dl_unregister(bp);
> init_err_dl:
> bnxt_shutdown_tc(bp);
> @@ -17008,9 +17015,10 @@ static int bnxt_resume(struct device *device)
>
> resume_exit:
> netdev_unlock(bp->dev);
> - bnxt_ulp_start(bp, rc);
> - if (!rc)
> + if (!rc) {
> + bnxt_ulp_start(bp);
> bnxt_reenable_sriov(bp);
> + }
> return rc;
> }
>
> @@ -17190,9 +17198,10 @@ static void bnxt_io_resume(struct pci_dev *pdev)
> netif_device_attach(netdev);
>
> netdev_unlock(netdev);
> - bnxt_ulp_start(bp, err);
> - if (!err)
> + if (!err) {
> + bnxt_ulp_start(bp);
> bnxt_reenable_sriov(bp);
> + }
> }
>
> static const struct pci_error_handlers bnxt_err_handler = {
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index fa513892db50..3097fc5755e6 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +
> +static struct bnxt_aux_device bnxt_aux_devices[__BNXT_AUXDEV_MAX] = {{
const? If not I'd be concerned that this will break badly if there
are multiple devices.
> + .name = "rdma",
> +}};
> @@ -227,20 +264,27 @@ EXPORT_SYMBOL(bnxt_send_msg);
>
> void bnxt_ulp_stop(struct bnxt *bp)
> {
> - struct bnxt_aux_priv *aux_priv = bp->aux_priv;
> - struct bnxt_en_dev *edev = bp->edev;
> + int i;
Similar to later comment, could move that into the loop init.
(assuming not some local netdev thing about not doing that?)
>
> - if (!edev)
> - return;
> -
> - mutex_lock(&edev->en_dev_lock);
> - if (!bnxt_ulp_registered(edev) ||
> - (edev->flags & BNXT_EN_FLAG_ULP_STOPPED))
> - goto ulp_stop_exit;
> -
> - edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
> - if (aux_priv) {
> + mutex_lock(&bp->auxdev_lock);
> + for (i = 0; i < __BNXT_AUXDEV_MAX; i++) {
...
> static void bnxt_aux_dev_release(struct device *dev)
> @@ -408,20 +462,25 @@ static void bnxt_aux_dev_release(struct device *dev)
> container_of(dev, struct bnxt_aux_priv, aux_dev.dev);
> struct bnxt *bp = netdev_priv(aux_priv->edev->net);
>
> - ida_free(&bnxt_aux_dev_ids, aux_priv->id);
> kfree(aux_priv->edev->ulp_tbl);
> - bp->edev = NULL;
> + bp->edev[aux_priv->id] = NULL;
> kfree(aux_priv->edev);
> kfree(aux_priv);
> - bp->aux_priv = NULL;
> + bp->aux_priv[aux_priv->id] = NULL;
UAF of aux_priv.
You'll need a local copy of aux_priv->id.
> }
>
> -void bnxt_rdma_aux_device_del(struct bnxt *bp)
> +void bnxt_aux_devices_del(struct bnxt *bp)
> {
> - if (!bp->edev)
> - return;
> + int idx;
>
> - auxiliary_device_delete(&bp->aux_priv->aux_dev);
> + mutex_lock(&bp->auxdev_lock);
> + for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
> + if (bnxt_auxdev_is_active(bp, idx)) {
> + auxiliary_device_delete(&bp->aux_priv[idx]->aux_dev);
> + bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_INIT);
> + }
> + }
> + mutex_unlock(&bp->auxdev_lock);
> }
>
> static void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
> @@ -451,83 +510,105 @@ static void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
> edev->bar0 = bp->bar0;
> }
>
> -void bnxt_rdma_aux_device_add(struct bnxt *bp)
> +void bnxt_aux_devices_add(struct bnxt *bp)
> {
> struct auxiliary_device *aux_dev;
> - int rc;
> -
> - if (!bp->edev)
> - return;
> -
> - aux_dev = &bp->aux_priv->aux_dev;
> - rc = auxiliary_device_add(aux_dev);
> - if (rc) {
> - netdev_warn(bp->dev, "Failed to add auxiliary device for ROCE\n");
> - auxiliary_device_uninit(aux_dev);
> - bp->flags &= ~BNXT_FLAG_ROCE_CAP;
> + int rc, idx;
> +
> + mutex_lock(&bp->auxdev_lock);
> + for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
> + if (bnxt_auxdev_is_init(bp, idx)) {
I haven't checked later patches, but if you can I'd do
if (!bnxt_auxdev_is_init(bp, idx))
continue;
aux_dev = ..
Just to reduce indent without it making much different otherwise to readability.
> + aux_dev = &bp->aux_priv[idx]->aux_dev;
> + rc = auxiliary_device_add(aux_dev);
> + if (rc) {
> + netdev_warn(bp->dev, "Failed to add auxiliary device for ROCE\n");
> + auxiliary_device_uninit(aux_dev);
> + if (idx == BNXT_AUXDEV_RDMA)
> + bp->flags &= ~BNXT_FLAG_ROCE_CAP;
Similar to below. I'd like this type specific stuff wrapped up in callbacks or
data.
> + continue;
> + }
> + bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_ADD);
> + }
> }
> + mutex_unlock(&bp->auxdev_lock);
> }
>
> -void bnxt_rdma_aux_device_init(struct bnxt *bp)
> +void bnxt_aux_devices_init(struct bnxt *bp)
> {
> struct auxiliary_device *aux_dev;
> struct bnxt_aux_priv *aux_priv;
> struct bnxt_en_dev *edev;
> struct bnxt_ulp *ulp;
> - int rc;
> + int rc, idx;
I'm not certain for net, but for most of the kernel it's now
acceptable to do
for (int idx = 0; ...
to limit the scope of idx.
> +
> + mutex_lock(&bp->auxdev_lock);
> + for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
It might be worth considering a precursor patch to factor out
a bnxt_aux_devices_init_one()
that would be a no operation changes patch, then do this
on top. Would make things a lot easier to follow than this diff.
> + bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_NONE);
> +
> + if (idx == BNXT_AUXDEV_RDMA &&
> + !(bp->flags & BNXT_FLAG_ROCE_CAP))
> + continue;
I'd find the code easier to read if the few bits of device
specific stuff are via callbacks or even better data in struct bnxt_aux_device.
Having a mix of stuff that is in there and checks against specific IDs
in the code just tends to end up messy if more devices get added over time.
> +
> + aux_priv = kzalloc(sizeof(*aux_priv), GFP_KERNEL);
> + if (!aux_priv)
> + goto next_auxdev;
> +
> + aux_dev = &aux_priv->aux_dev;
> + aux_dev->id = bp->auxdev_id;
> + aux_dev->name = bnxt_aux_devices[idx].name;
> + aux_dev->dev.parent = &bp->pdev->dev;
> + aux_dev->dev.release = bnxt_aux_dev_release;
Could skip the zeroing at allocation in favor of the follow.
For me this is easier to read but that's just personal preference
so up to you.
aux_priv = (struct bnxt_aux_priv) {
.aux_dev = {
.id = bp->auxdev_id,
.name = bnxt_aux_devices[idx].name,
.dev = {
.parent = &bp->pdev->dev,
.release = bnxt_aux_dev_release,
},
},
};
> +
> + rc = auxiliary_device_init(aux_dev);
> + if (rc) {
> + kfree(aux_priv);
> + goto next_auxdev;
> + }
> + bp->aux_priv[idx] = aux_priv;
>
> - if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
> - return;
> + /* From this point, all cleanup will happen via the .release
> + * callback & any error unwinding will need to include a call
> + * to auxiliary_device_uninit.
> + */
> + edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> + if (!edev)
> + goto aux_dev_uninit;
>
> - aux_priv = kzalloc(sizeof(*bp->aux_priv), GFP_KERNEL);
> - if (!aux_priv)
> - goto exit;
> + aux_priv->edev = edev;
> + bnxt_set_edev_info(edev, bp);
>
> - aux_priv->id = ida_alloc(&bnxt_aux_dev_ids, GFP_KERNEL);
> - if (aux_priv->id < 0) {
> - netdev_warn(bp->dev,
> - "ida alloc failed for ROCE auxiliary device\n");
> - kfree(aux_priv);
> - goto exit;
> - }
> + ulp = kzalloc(sizeof(*ulp), GFP_KERNEL);
> + if (!ulp)
> + goto aux_dev_uninit;
>
> - aux_dev = &aux_priv->aux_dev;
> - aux_dev->id = aux_priv->id;
> - aux_dev->name = "rdma";
> - aux_dev->dev.parent = &bp->pdev->dev;
> - aux_dev->dev.release = bnxt_aux_dev_release;
> + edev->ulp_tbl = ulp;
> + bp->edev[idx] = edev;
> + if (idx == BNXT_AUXDEV_RDMA)
Another thing I'd like to see as a callback registered in the per
auxdev type data alongside the name etc.
> + bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);
> + aux_priv->id = idx;
> + bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_INIT);
>
> - rc = auxiliary_device_init(aux_dev);
> - if (rc) {
> - ida_free(&bnxt_aux_dev_ids, aux_priv->id);
> - kfree(aux_priv);
> - goto exit;
> + continue;
> +aux_dev_uninit:
> + auxiliary_device_uninit(aux_dev);
> +next_auxdev:
> + if (idx == BNXT_AUXDEV_RDMA)
> + bp->flags &= ~BNXT_FLAG_ROCE_CAP;
Similar to above, I'd prefer this to be either data or a callback that
is specific to the idx.
> }
> - bp->aux_priv = aux_priv;
> -
> - /* From this point, all cleanup will happen via the .release callback &
> - * any error unwinding will need to include a call to
> - * auxiliary_device_uninit.
> - */
> - edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> - if (!edev)
> - goto aux_dev_uninit;
> -
> - aux_priv->edev = edev;
> -
> - ulp = kzalloc(sizeof(*ulp), GFP_KERNEL);
> - if (!ulp)
> - goto aux_dev_uninit;
> + mutex_unlock(&bp->auxdev_lock);
> +}
>
> - edev->ulp_tbl = ulp;
> - bp->edev = edev;
> - bnxt_set_edev_info(edev, bp);
> - bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);
Powered by blists - more mailing lists