On 25. 3. 6. 20:13, Loktionov, Aleksandr wrote:
>
>
>> -----Original Message-----
>> From: Kyungwook Boo <[email protected]>
>> Sent: Thursday, March 6, 2025 6:26 AM
>> To: Loktionov, Aleksandr <[email protected]>; Kitszel,
>> Przemyslaw <[email protected]>; Nguyen, Anthony L
>> <[email protected]>
>> Cc: [email protected]; [email protected]
>> Subject: [PATCH] i40e: fix MMIO write access to an invalid page in
>> i40e_clear_hw
> Please follow the rules, add v2 to the patch
Hi, Loktionov,
Thank you for reviewing the patch.
Following your guidance, I will update the patch with the correct format and
also include v2.
>>
>> In i40e_clear_hw(), when the device sends a specific input(e.g., 0), an
>> integer
>> underflow in the num_{pf,vf}_int variables can occur, leading to MMIO write
>> access to an invalid page.
>>
>> To fix this, we change the type of the unsigned integer variables
>> num_{pf,vf}_int to signed integers. Additionally, in the for-loop where the
>> integer underflow occurs, we also change the type of the loop variable i to a
>> signed integer.
> Please do follow the linux kernel
If you are referring to the tone of the patch description, I will rewrite it in
the imperative mood.
>>
>> Signed-off-by: Kyungwook Boo <[email protected]>
>> Signed-off-by: Loktionov, Aleksandr <[email protected]>
>> Signed-off-by: Przemek Kitszel <[email protected]>
>> Link: https://lore.kernel.org/lkml/ffc91764-1142-4ba2-91b6-
>> [email protected]/T/
>> ---
> Please up here versions history.
I have noted your request and will add the version history in the next update.
>> drivers/net/ethernet/intel/i40e/i40e_common.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
>> b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> index 370b4bddee44..9a73cb94dc5e 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> @@ -817,8 +817,8 @@ int i40e_pf_reset(struct i40e_hw *hw) void
>> i40e_clear_hw(struct i40e_hw *hw) {
>> u32 num_queues, base_queue;
>> - u32 num_pf_int;
>> - u32 num_vf_int;
>> + s32 num_pf_int;
>> + s32 num_vf_int;
>> u32 num_vfs;
>> u32 i, j;
> What about this vars? Are they used?
i, j are both used.
I think the relevant line to be considered is as follows:
if (val & I40E_PF_VT_PFALLOC_VALID_MASK && j >= i)
num_vfs = (j - i) + 1;
After this, j is not used and
i is used as index of several loops.
My current plan was to change only i to s32 since it is related to the bug.
However, i is also used outside the loop, as in the code above.
Should I proceed with the original plan, or would it be better to separate the
loop variable for clarity? I would appreciate your opinion on this.
>> u32 val;
>> @@ -848,18 +848,18 @@ void i40e_clear_hw(struct i40e_hw *hw)
>> /* stop all the interrupts */
>> wr32(hw, I40E_PFINT_ICR0_ENA, 0);
>> val = 0x3 << I40E_PFINT_DYN_CTLN_ITR_INDX_SHIFT;
>> - for (i = 0; i < num_pf_int - 2; i++)
>> + for (s32 i = 0; i < num_pf_int - 2; i++)
>> wr32(hw, I40E_PFINT_DYN_CTLN(i), val);
>>
>> /* Set the FIRSTQ_INDX field to 0x7FF in PFINT_LNKLSTx */
>> val = eol << I40E_PFINT_LNKLST0_FIRSTQ_INDX_SHIFT;
>> wr32(hw, I40E_PFINT_LNKLST0, val);
>> - for (i = 0; i < num_pf_int - 2; i++)
>> + for (s32 i = 0; i < num_pf_int - 2; i++)
>> wr32(hw, I40E_PFINT_LNKLSTN(i), val);
>> val = eol << I40E_VPINT_LNKLST0_FIRSTQ_INDX_SHIFT;
>> for (i = 0; i < num_vfs; i++)
>> wr32(hw, I40E_VPINT_LNKLST0(i), val);
>> - for (i = 0; i < num_vf_int - 2; i++)
>> + for (s32 i = 0; i < num_vf_int - 2; i++)
>> wr32(hw, I40E_VPINT_LNKLSTN(i), val);
>>
>> /* warn the HW of the coming Tx disables */
>> --
>> 2.25.1
Best,
Kyungwook Boo
