[RFC net-next 0/2] bnxt_en: Recover from failed TX RING_FREE

[Joe Damato]

Greetings:

This series makes a failed TX HWRM_RING_FREE recoverable instead of silently
releasing ring memory the FW might still own to address a use after free I
noticed on a production system.

I'm posting this as an RFC because:
  - I am not sure if patch 2 is correct. Maybe Broadcom can let me know ?
  - I am not sure if this is a Fixes or not. I guess if it was always like
    this then this is net-next material?

The core issue is that bnxt_hwrm_tx_ring_free discards the return value of
hwrm_ring_free_send_msg and sets fw_ring_id = INVALID_HW_RING_ID
unconditionally. When a TX RING_FREE sent over a completion ring times out,
the driver logs:

  hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  Resp cmpl intr err msg: 0x51

and then __bnxt_close_nic() calls bnxt_free_mem(), unmapping ring memory that
the FW was never told to release.

bnxt_queue_stop mentions something about this in a comment:

  "HWRM_RING_FREE completion is handled in NAPI to guarantee no more DMA
   on that ring after seeing the completion."

But... if the completion ring is dead, the completion never arrives.

This is reachable on production systems today:

  - TX completions stop
  - netdev watchdog fires
  - reset closes the device
  - every RING_FREE routed through that dead completion ring times out
  - ring memory freed by the driver but still in use by the FW

The result on an IOMMU host is IO_PAGE_FAULT or DMAR fault against freed
pages.

I tried to test the code in patch 2 on a BCM57504 with FW 235.1.208.0/pkg
235.1.208.0.

I hacked something together to inject a failure to test the reset paths on my
device. It seems like HWRM_RING_RESET ring_type=TX is accepted by thte FW and
the polled RING_FREE also succeeds, but in my testing the TX ring was idle. I
never tested a reset against a ring with descriptors in flight. Which leads me
to my questions.....

1. Does HWRM_RING_RESET with ring_type=TX cause the FW to abandon work already
outstanding on that ring and stop DMA ? If not .... then this code is wrong :(
and maybe see question (3) below.

2. Is a TX ring reset supposed to post a completion ring entry? In my testing
it seemed like the FW may have written success into the response DMA buffer
but never posted the entry, so the request times out with -EBUSY even though
it succeeded. Is that intentional? If so, maybe only polled transport mode
works on this FW?

3. Maybe the TX reset isn't necessary at all? Maybe instead the code should
retry the RING_FREE over polled transport and that's good enough? This depends
on the answer to question (1) above, but I guess it would simplify the code if
a polled RING_FREE is enough?

Thanks,
Joe

Joe Damato (2):
  bnxt_en: return status from bnxt_hwrm_tx_ring_free
  bnxt_en: recover a failed TX RING_FREE with a ring reset

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 55 ++++++++++++++++++++---
 1 file changed, 49 insertions(+), 6 deletions(-)


base-commit: 5ccdfb2c3203207deb17e7c5b0db8c7f475639a7
-- 
2.53.0-Meta