This is an AI-generated review of your patch. The human sending this
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
+ break;
+ ice_debug(hw, ICE_DBG_NVM,
+ "NVM read EBUSY error, retry %d\n",
+ retry_cnt + 1);
+ last_cmd = false;
+ ice_release_nvm(hw);
+ msleep(ICE_SQ_SEND_DELAY_TIME_MS);
The review is correct that this allows one extra execution. However, I
intentionally kept this verbatim from the OOT driver (1.15.4) to
This OOT change was made entirely by Grzegorz, for E825C. Would be great
to acknowledge his authorship (+ SoB ofc, below).
Signed-off-by: Grzegorz Nitka <[email protected]>
What was the platform that you have encountered the bug on?
retain the significant test exposure it has there. The absolute worst
case is one harmless extra retry before giving up. If you strongly
prefer strict adherence to ICE_SQ_SEND_MAX_EXECUTE, I can update the
condition in a v4.
Please update (to have one attempt less).
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
I don't think this is an issue. The NVM resource (req_res 0x0008) is
an ownership lease, not a content snapshot. The FW grants it with a
hold timeout (see the timeout field handling in ice_aq_req_res). It
never guaranteed a point-in-time view of the flash.
Very similar behavior already exists in ice_devlink_nvm_snapshot(),
which reads the NVM in chunks via ice_read_flat_nvm() and deliberately
acquires/releases the lock between chunks (with a comment explaining
the FW reclaim).
Additionally, the lock is only dropped on the failure path, where
neither bytes_read nor offset have been advanced. The driver simply
re-reads the failed chunk after re-acquiring. So no data that is
returned to the caller is committed across the drop window.
Overall the lock drop does not weaken any guarantee callers actually
have. If we wanted to introduce a true snapshot guarantee, that would
probably require different approach and separate patch.
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
Agreed, this might be a real bug, and the one of the three I think is
worth investigating.
If ice_acquire_nvm() fails after the drop, ice_read_flat_nvm() returns
with the lock unheld while callers unconditionally call
ice_release_nvm(), so a stray release is issued.
On probability, though, the window is very small. Reaching it requires
sustained EBUSY across the retry budget plus a failed re-acquire
(which itself polls up to ICE_NVM_TIMEOUT), and concurrently another
requester taking the lock. Most reads happen during init (ice_probe,
and reset/rebuild via ice_init_nvm), and NVM writes only happen on an
already initialized driver. The devlink/ethtool nvm_read paths are
also exposed, but hitting this race would require precise timing
against a concurrent NVM owner on the device.
I'd prefer to keep the scope of this patch limited to the EBUSY retry
path and not take on the unbalanced-release fix here. A proper fix
should change the lock-ownership contract of ice_read_flat_nvm() (on
error, the lock must be released by ice_read_flat_nvm(), callers
release only on success) and update all callers. Code change sould be
simple for all callers but ice_discover_flash_size(), it intentionally
holds one lease across a read loop and would need to re-acquire after
each expected boundary failure.
Given how small the original window is, I'd rather not trade tested
OOT behavior for the risk of a complex unbalanced NVM lock fix. I
actually have a patch mostly ready that fixes the lock-ownership
contract, but I really don't like it. It changes the design of
ice_read_flat_nvm(), making it less intuitive for callers. More
importantly, I just don't have the resources or test coverage right
now to properly verify such change.
However, I can modify the failure path for ice_acquire_nvm inside
ice_read_flat_nvm. Instead of bailing out immediately, we can just
retry it within the existing retry budget. In this case, the
probability of leaving ice_read_flat_nvm without holding the lock is
reduced even further without needing a refactor.
Please let me know what you think about my thought process on this.
I think that both AI-reported issues against the lock are valid
concerns.
I think that sleep was the actual fix, and re-locking were merely
a necessity due to their expiration (as you said).
A proper fix would be to just increase lock-timeout to accommodate all
attempts (and still do the retries&sleep, but without unlocking).
+ retry_cnt++;
+ } else {
+ bytes_read += read_size;
+ offset += read_size;
+ retry_cnt = 0;
+ }
} while (!last_cmd);
*length = bytes_read;
Thanks,
Robert
