netdev - Re: [PATCH net-next] eth: bnxt: fix backward compatibility with older devices

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] [day] [month] [year] [list]
Message-ID: <CACKFLinuMQ-0riznftx=dhWVnw76D=ek3YNFjbsBJeCjrNir7w@mail.gmail.com>
Date: Thu, 12 Oct 2023 20:53:33 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com
Subject: Re: [PATCH net-next] eth: bnxt: fix backward compatibility with older devices

On Thu, Oct 12, 2023 at 3:41 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Recent FW interface update bumped the size of struct hwrm_func_cfg_input
> above 128B which is the max some devices support.
>
> Probe on Stratus (BCM957452) with FW 20.8.3.11 fails with:
>
>    bnxt_en ...: Unable to reserve tx rings
>    bnxt_en ...: 2nd rings reservation failed.
>    bnxt_en ...: Not enough rings available.
>
> Once probe is fixed other error pop up:
>
>    bnxt_en ...: Failed to set async event completion ring.

Thanks for catching this.  We need to do more compatibility testing
with older firmware.

>
> This is because __hwrm_send() rejects requests larger than
> bp->hwrm_max_ext_req_len with -E2BIG. Since the driver doesn't
> actually access any of the new fields, yet, trim the length.
> It should be safe.
>
> Similar workaround exists for backing_store_cfg_input,
> although that one mins() to a constant of 256?!

Because the backing store cfg command is supported by relatively newer
firmware that will accept 256 bytes at least.

The HWRM_FUNC_CFG is used quite a bit in bnxt_sriov.c and also in
bnxt_devlink.c.  I think we should apply the same treatment to all of
them.

Thanks.

>
> To make debugging easier in the future add a warning
> for oversized requests.
>
> Fixes: 754fbf604ff6 ("bnxt_en: Update firmware interface to 1.10.2.171")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: michael.chan@...adcom.com
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 21 +++++++++++++++----
>  .../net/ethernet/broadcom/bnxt/bnxt_hwrm.c    |  2 ++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 5c2afcd5ce80..10dc680f022e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -5855,6 +5855,19 @@ static int hwrm_ring_alloc_send_msg(struct bnxt *bp,
>         return rc;
>  }
>
> +/* Older devices can only support req length of 128.
> + * Requests which don't need fields starting at num_quic_tx_key_ctxs
> + * can use this helper to avoid getting -E2BIG.
> + */
> +static int bnxt_hwrm_func_cfg_short_req_init(struct bnxt *bp,
> +                                            struct hwrm_func_cfg_input **req)
> +{
> +       u32 req_len;
> +
> +       req_len = min_t(u32, sizeof(**req), bp->hwrm_max_ext_req_len);
> +       return __hwrm_req_init(bp, (void **)req, HWRM_FUNC_CFG, req_len);
> +}
> +
>  static int bnxt_hwrm_set_async_event_cr(struct bnxt *bp, int idx)
>  {
>         int rc;
> @@ -5862,7 +5875,7 @@ static int bnxt_hwrm_set_async_event_cr(struct bnxt *bp, int idx)
>         if (BNXT_PF(bp)) {
>                 struct hwrm_func_cfg_input *req;
>
> -               rc = hwrm_req_init(bp, req, HWRM_FUNC_CFG);
> +               rc = bnxt_hwrm_func_cfg_short_req_init(bp, &req);
>                 if (rc)
>                         return rc;
>
> @@ -6273,7 +6286,7 @@ __bnxt_hwrm_reserve_pf_rings(struct bnxt *bp, int tx_rings, int rx_rings,
>         struct hwrm_func_cfg_input *req;
>         u32 enables = 0;
>
> -       if (hwrm_req_init(bp, req, HWRM_FUNC_CFG))
> +       if (bnxt_hwrm_func_cfg_short_req_init(bp, &req))
>                 return NULL;
>
>         req->fid = cpu_to_le16(0xffff);
> @@ -8618,7 +8631,7 @@ static int bnxt_hwrm_set_br_mode(struct bnxt *bp, u16 br_mode)
>         else
>                 return -EINVAL;
>
> -       rc = hwrm_req_init(bp, req, HWRM_FUNC_CFG);
> +       rc = bnxt_hwrm_func_cfg_short_req_init(bp, &req);
>         if (rc)
>                 return rc;
>
> @@ -8636,7 +8649,7 @@ static int bnxt_hwrm_set_cache_line_size(struct bnxt *bp, int size)
>         if (BNXT_VF(bp) || bp->hwrm_spec_code < 0x10803)
>                 return 0;
>
> -       rc = hwrm_req_init(bp, req, HWRM_FUNC_CFG);
> +       rc = bnxt_hwrm_func_cfg_short_req_init(bp, &req);
>         if (rc)
>                 return rc;
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> index 132442f16fe6..1df3d56cc4b5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c
> @@ -485,6 +485,8 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx)
>
>         if (msg_len > BNXT_HWRM_MAX_REQ_LEN &&
>             msg_len > bp->hwrm_max_ext_req_len) {
> +               netdev_warn(bp->dev, "oversized hwrm request, req_type 0x%x",
> +                           req_type);
>                 rc = -E2BIG;
>                 goto exit;
>         }
> --
> 2.41.0
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)