Re: [PATCH net-next 4/6] enic: validate V2 VF configuration replies

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 4 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] enic_mbox_vf_handle_add_del_mac_reply()
  (drivers/net/ethernet/cisco/enic/enic_mbox.c:725-726) unconditionally…
- [Medium] enic_mbox_vf_handle_reply() now ends with `if (recovery !=
  ENIC_MBOX_VF_REPLY_OK) enic_mbox_vf_kick_recovery(enic);`, which fires…
- [Low] ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF (12) / _ACK (13) are added to
  the message enum and the notification type is listed in…
- [Low] The three new externally visible VF entry points
  (enic_mbox_vf_add_del_macs(), enic_mbox_vf_add_del_mac(),…
- [Low] enic_mbox_vf_malformed_msg() turns any single mis-framed admin-
  channel message attributed to the PF into a full protocol fault…
- [Low] enic_mbox_vf_reply_claim()
  (drivers/net/ethernet/cisco/enic/enic_mbox.c:381) returns with…

Pre-existing issues:
- [Low] The VF now emits ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST (10) and
  ENIC_MBOX_VF_SET_PKT_FILTER_REQUEST (14), but…

--

enic: validate V2 VF configuration replies

This adds the V2 mailbox operations for MAC filters, administrative-MAC
notifications and packet-filter settings on the VF side, and keeps the
request buffer alive so per-entry reply results can be validated. On a
malformed or contradictory reply, or when the PF reports the VF is no longer
registered, the VF forces a new registration before accepting traffic again.

A few questions about the new recovery escalation and the lock handoff
between the claim and completion helpers.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 2d77d41577a3a..3f73072170b91 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c

[ ... ]

> @@ -376,6 +377,82 @@ static void enic_mbox_vf_request_finish(struct enic *enic)
>  	mutex_unlock(&enic->vf_mbox_request_lock);
>  }
>  
> +/* Return with mbox_state_lock held when this handler owns the reply. */
> +static bool enic_mbox_vf_reply_claim(struct enic *enic, u8 reply_type,
> +				     u64 msg_num, u8 *expected)
> +{
> +	spin_lock_bh(&enic->mbox_state_lock);
> +	*expected = enic->mbox_expected_reply;
> +	if (*expected == reply_type &&
> +	    enic->mbox_expected_msg_num == msg_num)
> +		return true;
> +	spin_unlock_bh(&enic->mbox_state_lock);
> +
> +	return false;
> +}
> +
> +enum enic_mbox_vf_reply_recovery {
> +	ENIC_MBOX_VF_REPLY_OK,
> +	ENIC_MBOX_VF_REPLY_RECONNECT,
> +	ENIC_MBOX_VF_REPLY_REGISTRATION_LOST,
> +};
> +
> +static int
> +enic_mbox_vf_classify_reply(bool malformed, u16 ret_major,
> +			    enum enic_mbox_vf_reply_recovery *recovery)
> +{
> +	*recovery = ENIC_MBOX_VF_REPLY_OK;
> +	if (malformed) {
> +		*recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> +		return -EIO;
> +	}
> +	/* Some deployed peers return a negative errno in this 16-bit field.
> +	 * Interpret protocol bits only when no unknown bits are present; otherwise
> +	 * an errno such as -EINVAL could accidentally look like registration loss.
> +	 */
> +	if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
> +	    (ret_major & ENIC_MBOX_ERR_VF_NOT_REGISTERED)) {
> +		*recovery = ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
> +		return -ENODEV;
> +	}
> +	if (!(ret_major & ~ENIC_MBOX_ERR_MASK) &&
> +	    (ret_major & ENIC_MBOX_ERR_MSG_NOT_SUPPORTED))
> +		return -EOPNOTSUPP;
> +	if (ret_major)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static void
> +enic_mbox_vf_recover_reply_locked(struct enic *enic,
> +				  enum enic_mbox_vf_reply_recovery recovery)
> +{
> +	bool registration_lost;
> +
> +	lockdep_assert_held(&enic->mbox_state_lock);
> +
> +	if (recovery != ENIC_MBOX_VF_REPLY_OK) {
> +		registration_lost =
> +			recovery == ENIC_MBOX_VF_REPLY_REGISTRATION_LOST;
> +		enic_mbox_vf_mark_reconnect_locked(enic, registration_lost);
> +	}
> +}
> +
> +static void enic_mbox_vf_reply_complete(struct enic *enic)
> +{
> +	lockdep_assert_held(&enic->mbox_state_lock);
> +	enic->mbox_expected_reply = 0;
> +	enic->mbox_expected_msg_num = 0;
> +	/* Publish completion before releasing the state lock.  A waiter that
> +	 * hit the timeout boundary may otherwise see the claimed state, finish the
> +	 * request, and let a new request reinitialize this completion before the
> +	 * old handler signals it.
> +	 */
> +	complete(&enic->mbox_comp);
> +	spin_unlock_bh(&enic->mbox_state_lock);
> +}

[Severity: Low]
Should this conditional acquire be annotated?  enic_mbox_vf_reply_claim()
returns with enic->mbox_state_lock held on the true path and unlocked on
the false path, and the matching release happens in a different function,
enic_mbox_vf_reply_complete().

Neither helper carries __cond_acquires()/__acquires()/__releases(), and
spin_lock_bh() is annotated __acquires(lock) in this tree, so sparse and
clang context analysis cannot pair the acquire with the release for the
claim helper or for its callers (enic_mbox_vf_handle_reply,
enic_mbox_vf_handle_add_del_mac_reply,
enic_mbox_vf_handle_set_pkt_filter_reply, enic_mbox_vf_malformed_msg).

I traced all four callers and every true-branch path does reach
enic_mbox_vf_reply_complete(), so this is balanced at runtime today.  Would
__cond_acquires(true, &enic->mbox_state_lock) on the claim helper and
__releases(&enic->mbox_state_lock) on the completion helper be preferable,
or could the lock be taken and dropped inside a single function?  The
longest region between claim and complete walks up to
ENIC_MBOX_MAX_MAC_OPS (130) entries under spin_lock_bh, so any future early
return added there would leak the lock with BHs disabled.

> @@ -551,23 +628,21 @@ 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;
> +	enum enic_mbox_vf_reply_recovery recovery;
>  	u16 ret_major = le16_to_cpu(reply->ret_major);
> -	u64 expected_msg_num;
> -	u8 expected_type;
> +	u8 expected;
> +	int status;
>  
> +	status = enic_mbox_vf_classify_reply(false, ret_major, &recovery);
> +	if (!enic_mbox_vf_reply_claim(enic, reply_type, msg_num, &expected)) {

[ ... ]

> @@ -584,16 +659,168 @@ static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
>  			break;
>  		}
>  	}
> -	enic->mbox_expected_reply = 0;
> -	enic->mbox_expected_msg_num = 0;
> -	complete(&enic->mbox_comp);
> -	spin_unlock_bh(&enic->mbox_state_lock);
> +	enic_mbox_vf_recover_reply_locked(enic, recovery);
> +	WRITE_ONCE(enic->mbox_reply_status, status);
> +	enic_mbox_vf_reply_complete(enic);
>  
>  	if (ret_major)
>  		netdev_warn(enic->netdev,
>  			    "MBOX: PF rejected reply type %u: %u/%u\n",
>  			    reply_type, ret_major,
>  			    le16_to_cpu(reply->ret_minor));
> +	if (recovery != ENIC_MBOX_VF_REPLY_OK)
> +		enic_mbox_vf_kick_recovery(enic);
> +}

[Severity: Medium]
Can this new kick re-queue the reset work from inside the reset work?

The CAPABILITY and REGISTER requests are issued from
enic_admin_chan_reopen(), which runs in the enic_reset() work item, and
enic_reset() only returns early when !netif_running(), which stays true
across an internal reset.  If the reply carries
ENIC_MBOX_ERR_VF_NOT_REGISTERED, enic_mbox_vf_classify_reply() reports
ENIC_MBOX_VF_REPLY_REGISTRATION_LOST, so:

enic_reset()				/* holds rtnl_lock */
  enic_admin_chan_reopen()
    enic_mbox_vf_capability_check()
      enic_mbox_vf_handle_reply()
        enic_mbox_vf_kick_recovery()
          schedule_work(&enic->reset);	/* re-queues the running work */

The work bit is cleared before the callback runs, so this re-queues rather
than being a no-op.  With a PF that keeps answering the handshake with
VF_NOT_REGISTERED, does the device then cycle reset -> soft reset -> reopen
-> handshake failure -> reset with no fault budget or back-off, where the
baseline simply left the device down?  Each iteration also holds rtnl_lock
across up to two 3000 ms mailbox waits.

This one depends on peer behaviour I could not confirm against real PF
firmware, so it may not be reachable in practice, but there is no
structural termination for the loop in the code.

> +
> +static bool enic_mbox_vf_mac_reply_matches(const struct enic_mac_addr *request,
> +					   const struct enic_mac_addr *reply)
> +{

[ ... ]

> +static void enic_mbox_vf_handle_add_del_mac_reply(struct enic *enic,
> +						  void *payload, u16 msg_len,
> +						  u64 msg_num)
> +{
> +	struct enic_mbox_vf_add_del_mac_reply_msg *reply = payload;
> +	enum enic_mbox_vf_reply_recovery recovery;

[ ... ]

> +	if (msg_len < sizeof(*reply)) {
> +		status = enic_mbox_vf_classify_reply(true, 0, &recovery);
> +	} else {
> +		ret_major = le16_to_cpu(reply->reply.ret_major);
> +		ret_minor = le16_to_cpu(reply->reply.ret_minor);
> +		status = enic_mbox_vf_classify_reply(false, ret_major,
> +						     &recovery);
> +		if (status == -EIO)
> +			recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> +	}

[Severity: Medium]
Does this override also escalate an ordinary policy rejection?

For a well-formed reply whose ret_major carries only
ENIC_MBOX_ERR_GENERIC, enic_mbox_vf_classify_reply() takes its last arm:

	if (ret_major)
		return -EIO;

leaving *recovery == ENIC_MBOX_VF_REPLY_OK.  The unconditional
"if (status == -EIO)" above then rewrites that to
ENIC_MBOX_VF_REPLY_RECONNECT, so enic_mbox_vf_recover_reply_locked() ->
enic_mbox_vf_mark_reconnect_locked() sets mbox_send_disabled and
vf_rx_quarantined, bumps vf_mbox_fault_generation, and
enic_mbox_vf_kick_recovery() schedules enic->reset.

That appears to contradict the comment further down in this same function:

	/* After a malformed reply, the VF cannot trust that its state matches the
	 * PF. VF_NOT_REGISTERED means the PF removed all VF state. Both require a
	 * new registration; an ordinary policy rejection does not.
	 */

and the commit message, which says "Return stable policy errors to callers
while retaining retry semantics for operations that made no state change."

The sibling handler added by this same patch escalates only for ret_major
bits outside the protocol mask:

		if (status && (ret_major & ~ENIC_MBOX_ERR_MASK))
			recovery = ENIC_MBOX_VF_REPLY_RECONNECT;

Should the MAC handler use the same narrower rule?  Later in the series the
station-address install (enic_vf_station_addr_replace()) re-issues this
request on every open/reopen, so a PF that stably answers ERR_GENERIC would
keep re-arming the soft reset under rtnl_lock with RX quarantined.

[ ... ]

> +static void enic_mbox_vf_handle_set_pkt_filter_reply(struct enic *enic,
> +						     void *payload, u16 msg_len,
> +						     u64 msg_num)
> +{

[ ... ]

> +	if (msg_len < sizeof(*reply)) {
> +		status = enic_mbox_vf_classify_reply(true, 0, &recovery);
> +	} else {
> +		ret_major = le16_to_cpu(reply->reply.ret_major);
> +		status = enic_mbox_vf_classify_reply(false, ret_major,
> +						     &recovery);
> +		if (status && (ret_major & ~ENIC_MBOX_ERR_MASK))
> +			recovery = ENIC_MBOX_VF_REPLY_RECONNECT;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com