Re: [PATCH] ixgbe: fix incomplete ACI command buffer backup on retry

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Commit message overstates the impact of the fixed bug.
- [Low] Commit message names the wrong ice function as the reference
  implementation.

--

ixgbe: fix incomplete ACI command buffer backup on retry

The patch replaces the kmalloc() plus single-byte store in
ixgbe_aci_send_cmd() with kmemdup() so the whole indirect command buffer
is saved before an EBUSY retry, skips the backup when buf_size is 0, and
keys the restore off buf_cpy instead of buf. The code change itself looks
correct; the questions below are about the changelog only, so the diff
hunks are snipped.

> So every retry hands the firmware a buffer whose content, apart from the
> first byte, is uninitialized slab memory, and on a read command the
> caller's buffer is overwritten with that garbage when the retry finally
> gives up.

[Severity: Medium]
Can the firmware actually see that garbage?  In
ixgbe_aci_send_cmd_execute() the buffer only reaches the HIBA registers
when the descriptor carries LIBIE_AQ_FLAG_RD:

drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:ixgbe_aci_send_cmd_execute() {
	...
	if (desc->flags & cpu_to_le16(LIBIE_AQ_FLAG_RD)) {
		for (i = 0; i < buf_size / 4; i++)
			IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), ((u32 *)buf)[i]);
	...
}

The backup/restore path only runs for the opcodes listed in
ixgbe_should_retry_aci_send_cmd_execute(): disable_rxen, get_phy_caps,
get_link_status and get_link_topo.  disable_rxen and get_link_topo pass
(NULL, 0), and the two that do pass a buffer (ixgbe_aci_get_phy_caps()
and ixgbe_aci_get_link_info()) build their descriptor with
ixgbe_fill_dflt_direct_cmd_desc(), which sets only LIBIE_AQ_FLAG_SI.
LIBIE_AQ_FLAG_RD is OR-ed in only by set_phy_cfg, the NVM write,
nvm_pkg_data and nvm_pass_component_tbl paths, none of which are
retryable.

Also, on both EBUSY exits ixgbe_aci_send_cmd_execute() returns before the
"Write a response values to a buf" block, so buf is not touched by an
EBUSY attempt and the restore is effectively a no-op there.

Would it be more accurate to describe the effect as the caller's output
buffer being filled with uninitialized slab bytes once retries are
exhausted, rather than as garbage being handed to the firmware on every
retry?  As written the message suggests a functional firmware
interaction problem, which affects how this reads for stable triage.

> Use kmemdup() to copy the whole buffer, exactly like the equivalent
> ice_sq_send_cmd() already does.

[Severity: Low]
Is ice_sq_send_cmd() the right reference here?  That function is the
low-level control queue submit routine, the counterpart of
ixgbe_aci_send_cmd_execute(), and it has no buffer backup.  The
kmemdup()-based backup and restore-on-EBUSY logic lives in
ice_sq_send_cmd_retry() in drivers/net/ethernet/intel/ice/ice_common.c:

drivers/net/ethernet/intel/ice/ice_common.c:ice_sq_send_cmd_retry() {
	...
	if (is_cmd_for_retry) {
		if (buf) {
			buf_cpy = kmemdup(buf, buf_size, GFP_KERNEL);
			if (!buf_cpy)
				return -ENOMEM;
		}
	...
		if (buf_cpy)
			memcpy(buf, buf_cpy, buf_size);
	...
}

Could the changelog name ice_sq_send_cmd_retry() instead?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912013403.2818191-1-xiaolinkui%40126.com