Re: [PATCH iwl v3] ice: retry reading NVM if admin queue returns EBUSY

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

>> 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 <grzegorz.nitka@intel.com>

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