Re: [net,2/2] enic: match mailbox replies to request numbers

[netdev-bot+sashiko]

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

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

- [High] enic_mbox_vf_handle_reply()
  (drivers/net/ethernet/cisco/enic/enic_mbox.c:403) now requires the…

--

enic: match mailbox replies to request numbers

This adds a message-number match to the VF mailbox reply path: outgoing
replies now reuse the initiating message number, the VF records both the
expected reply type and the expected message number, and a reply is only
accepted when both match.  The expected-reply state is moved under a new
spinlock and the message counter is kept monotonic across an admin channel
reopen.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index ad79d3951f3d..5c93ca49552a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c

[ ... ]

> @@ -68,7 +71,21 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
>  		goto unlock;
>  	}
>  
> -	enic_mbox_fill_hdr(enic, buf, msg_type, dst_vnic_id, total_len);
> +	/* Replies reuse the initiating message number.  Requests and
> +	 * notifications allocate a new one.
> +	 */
> +	if (!reuse_msg_num)
> +		msg_num = ++enic->mbox_msg_num;
> +	if (expected_reply) {
> +		reinit_completion(&enic->mbox_comp);
> +		spin_lock_bh(&enic->mbox_state_lock);
> +		enic->mbox_expected_reply = expected_reply;
> +		enic->mbox_expected_msg_num = msg_num;
> +		spin_unlock_bh(&enic->mbox_state_lock);
> +		reply_expected = true;
> +	}

[ ... ]

> @@ -178,8 +243,8 @@ static int enic_mbox_pf_handle_capability(struct enic *enic, void *msg,
>  	reply.reply.ret_major = cpu_to_le16(0);
>  	reply.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
>  
> -	return enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
> -				  &reply, sizeof(reply));
> +	return enic_mbox_send_reply(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
> +				    &reply, sizeof(reply), msg_num);
>  }

[ ... ]

> @@ -324,75 +389,57 @@ static void enic_mbox_pf_process_msg(struct enic *enic,
> +static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
> +				      void *payload, u64 msg_num)
>  {
> +	struct enic_mbox_generic_reply *reply = payload;
> +	u16 ret_major = le16_to_cpu(reply->ret_major);
> +	u64 expected_msg_num;
> +	u8 expected_type;
> +
> +	spin_lock_bh(&enic->mbox_state_lock);
> +	expected_type = enic->mbox_expected_reply;
> +	expected_msg_num = enic->mbox_expected_msg_num;
> +	if (expected_type != reply_type || expected_msg_num != msg_num) {
> +		spin_unlock_bh(&enic->mbox_state_lock);
>  		netdev_warn(enic->netdev,
> -			    "MBOX: stale capability reply (expected %u), drop\n",
> -			    READ_ONCE(enic->mbox_expected_reply));
> +			    "MBOX: stale reply %u/%llu (expected %u/%llu), drop\n",
> +			    reply_type, (unsigned long long)msg_num,
> +			    expected_type, (unsigned long long)expected_msg_num);
>  		return;
>  	}

[Severity: High]
What happens when the VF runs this code against a PF that does not echo
msg_num?

The expected number armed in enic_mbox_send_msg_id() is the VF's own
private counter value:

	if (!reuse_msg_num)
		msg_num = ++enic->mbox_msg_num;
	if (expected_reply) {
		...
		enic->mbox_expected_msg_num = msg_num;

so acceptance now depends on the peer returning that exact value.  The
mismatch branch in enic_mbox_vf_handle_reply() returns without calling
complete(&enic->mbox_comp), so enic_mbox_wait_reply() runs out its 3000 ms
and returns -ETIMEDOUT for every reply that carries a different number.

The only PF implementation that can be inspected is the in-tree one added
by the commit named in the Fixes: tag, and before this patch it stamped a
fresh number on every outgoing message, replies included:

drivers/net/ethernet/cisco/enic/enic_mbox.c:enic_mbox_fill_hdr() {
	...
	hdr->msg_num = cpu_to_le64(++enic->mbox_msg_num);
}

That is also why this patch has to convert
enic_mbox_pf_handle_capability(), enic_mbox_pf_handle_register() and
enic_mbox_pf_handle_unregister() over to enic_mbox_send_reply(..., msg_num).

Given that, is this statement in the commit message accurate?

    Reply-number echo is part of the established version-1 protocol, so this
    remains compatible with deployed V2-capable PF implementations that
    already echo msg_num.

For a VF with this patch talking to a PF running the pre-patch in-tree
driver (older host kernel than guest, or a partial stable backport that
picks up only the VF-side hunks), does the capability handshake fail
outright?

enic_mbox_vf_capability_check() -> enic_mbox_wait_reply() returns
-ETIMEDOUT, and in enic_probe() that error is fatal:

		err = enic_mbox_vf_capability_check(enic);
		if (err) {
			dev_err(dev, "MBOX capability check failed: %d\n", err);
			goto err_out_admin_close;
		}

so the SR-IOV V2 VF netdev is never registered.  On the reset path,
enic_admin_chan_reopen() calls enic_admin_channel_close() on the same
error, leaving the VF without PF-driven link state or registration.

Would it make sense to keep a fallback here, for example accepting a reply
whose msg_num is 0 (or otherwise not echoed), or gating the strict match on
the negotiated pf_cap_version, so that the VF does not depend on peer
behaviour that this same patch introduces?

> -	if (le16_to_cpu(reply->reply.ret_major) == 0)
> -		enic->pf_cap_version = le32_to_cpu(reply->version);
> -	else
> -		netdev_warn(enic->netdev,
> -			    "MBOX: PF rejected capability request: %u/%u\n",
> -			    le16_to_cpu(reply->reply.ret_major),
> -			    le16_to_cpu(reply->reply.ret_minor));
> -	complete(&enic->mbox_comp);
> -}

[ ... ]

> @@ -571,32 +620,31 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
>  int enic_mbox_vf_capability_check(struct enic *enic)
>  {
>  	struct enic_mbox_vf_capability_msg req = {};
> +	u32 version;
>  	int err;
>  
> -	enic->pf_cap_version = 0;
> -	reinit_completion(&enic->mbox_comp);
> -	WRITE_ONCE(enic->mbox_expected_reply, ENIC_MBOX_VF_CAPABILITY_REPLY);
> +	WRITE_ONCE(enic->pf_cap_version, 0);
>  	req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
>  
> -	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REQUEST,
> -				 ENIC_MBOX_DST_PF, &req, sizeof(req));
> -	if (err) {
> -		WRITE_ONCE(enic->mbox_expected_reply, 0);
> +	err = enic_mbox_vf_send_request(enic,
> +					ENIC_MBOX_VF_CAPABILITY_REQUEST,
> +					ENIC_MBOX_VF_CAPABILITY_REPLY,
> +					&req, sizeof(req));
> +	if (err)
>  		return err;
> -	}
>  
>  	err = enic_mbox_wait_reply(enic, 3000);
> -	WRITE_ONCE(enic->mbox_expected_reply, 0);
> +	version = READ_ONCE(enic->pf_cap_version);
>  	if (err) {
>  		netdev_warn(enic->netdev,
>  			    "MBOX: no capability reply from PF\n");
>  		return err;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830-b4-enic-v2-mbox-fixes-net-v1-0-23adf9bfd426%40cisco.com