Re: [Intel-wired-lan] [PATCH net v2] i40e: limit the DDP profile count returned by the firmware

[Linkui Xiao <xiaolinkui@126.com>]

Thanks for the review. Both findings are addressed in v3.

- [Medium] Incomplete fix in i40e_ddp_does_profile_exist() and
   i40e_ddp_does_profile_overlap()

   Agreed on the clamping half. Silently limiting the count and then
   returning 0 hands i40e_ddp_load() a definite answer derived from a list
   that was only partially read: the add path proceeds without having
   examined every profile the firmware reported, and the is_add == false
   path reports a profile as missing that may in fact be loaded. Both
   helpers already have an error return that i40e_ddp_load() turns into
   "Failed to fetch loaded profiles." and aborts the operation, so v3
   rejects the list instead: if the firmware reports more profiles than
   I40E_PROFILE_LIST_SIZE can hold, the helpers return -EIO and no scan
   happens at all.

   I did not plumb the response datalen out of i40e_aq_get_ddp_list().
   For this command the driver has to set desc.datalen to the buffer size
   it passes in, and i40e_aq_get_ddp_list() does not report the writeback
   descriptor back to its caller, so the length would have to come either
   from a new output parameter on that helper (declared in
   i40e_prototype.h) or from a wb_desc routed through cmd_details. Either
   way it adds a second firmware supplied number to validate on top of the
   one this patch is about, and if the firmware leaves the field at the
   requested 772 bytes the derived bound is exactly the bound we have
   today. That is a bigger change to the common AQ path than a net fix
   should carry, so I would rather bound the scan by the size of the
   buffer the driver owns.

   The remaining half - comparing against p_info[] slots the firmware
   never filled - cannot be detected independently of the count, because
   the driver is not told how many records were actually written. If the
   firmware reports a count of 16 or less while writing fewer records, the
   unread slots are indistinguishable from real entries. Zero
   initialization makes the outcome deterministic (an unwritten entry is
   all zeroes rather than stale stack) but does not make it correct;
   deriving the bound from the response length would be needed for that,
   and that is the larger helper change described above.

- [Medium, pre-existing] uninitialized buff[] handed to
   i40e_aq_get_ddp_list()

   Agreed, and fixed as suggested: buff[] is now zero initialized in both
   helpers. i40e_asq_send_command_atomic_exec() copies the full buff_size
   into the DMA bounce buffer and copies the full buff_size back on
   completion, so those 772 bytes of stack were both readable by the
   firmware and compared against afterwards. Both hunks touch these
   declarations and the root commit is the same (cdc594e00370), so it is
   handled here rather than as a separate patch; I can split it out if you
   would rather have it on its own.

v3 keeps the unsigned loop counter from v2, and drops the Reviewed-by tag,
as the code changed after the review.

pw-bot: cr