Re: [PATCH net-next v5 09/15] idpf: refactor idpf to use libie control queues

Sashiko says:

quoted

+void idpf_send_vf_reset_msg(struct idpf_adapter *adapter)
+{
+	struct libie_ctlq_info *ctlq = adapter->asq;
+
+	/* Forcefully claim send queue slot */
+	idpf_mb_clean(adapter, ctlq, true);
+
+	scoped_guard(spinlock, &ctlq->lock) {
+		*ctlq->tx_msg[ctlq->next_to_use] = (struct libie_ctlq_msg) {
+			.opcode = LIBIE_CTLQ_SEND_MSG_TO_CP,
+			.chnl_opcode = VIRTCHNL2_OP_RESET_VF,
+		};
+
+		libie_ctlq_send(adapter->asq, 1);
+	}
+}

Compared to the old call, which routed through idpf_send_mb_msg() and
first ran idpf_is_reset_detected() (via the arq registers) to bail out
early if the mailbox was gone, this new helper unconditionally
dereferences adapter->asq and immediately calls idpf_mb_clean() which
takes &ctlq->lock.
Can adapter->asq be NULL here?
idpf_deinit_dflt_mbx() sets asq = NULL after libie_ctlq_xn_deinit(),
and idpf_init_dflt_mbx() can fail with asq/arq still NULL if
libie_find_ctlq() returns NULL. If a hard reset flow is entered on
such an error path, ctlq->lock would fault.
Also, is bypassing the xn manager and reaching directly into libie
internals (ctlq->tx_msg[ctlq->next_to_use], ctlq->lock, and
libie_ctlq_send()) intentional? This is the only send path in the
refactored driver that pokes libie state directly, and it is fragile
against future changes to how tx_msg[] slots are allocated or
lifecycled after a forced idpf_mb_clean().
Finally, is it safe to writel() to the mailbox tail via
libie_ctlq_send() when hardware is already in reset? The old
idpf_is_reset_detected() gate specifically avoided that.

VF reset is done only in one flow, where it is performed only
if no other reset is going on.

[...]

quoted

+int idpf_send_mb_msg_kfree(struct idpf_adapter *adapter,
+			   struct libie_ctlq_xn_send_params *xn_params,
+			   void *send_buf, size_t send_buf_size)
+{
+	int err = idpf_send_mb_msg(adapter, xn_params, send_buf, send_buf_size);
+
+	if (libie_cp_can_send_onstack(send_buf_size))
+		kfree(send_buf);
+
+	return err;
+}

Is the stack-vs-heap discrimination here safe as a long-term invariant?
The reset-detected branch decides to kfree(send_buf) based purely on
libie_cp_can_send_onstack(send_buf_size), and rel_tx_buf is set to
kfree unconditionally so libie_ctlq_xn_send() will also invoke kfree
on the send buffer when the send fails and the size crosses
LIBIE_CP_TX_COPYBREAK (128 bytes).
Many call sites pass the address of a stack-allocated struct:
    struct virtchnl2_get_capabilities caps = {};
    ... idpf_send_mb_msg(adapter, &xn_params, &caps, sizeof(caps));
    struct virtchnl2_vport_stats stats_msg = {};
    ... idpf_send_mb_msg(np->adapter, &xn_params, &stats_msg,
                         sizeof(stats_msg));
virtchnl2_vport_stats is exactly 128 bytes today, so it sits right at
the LIBIE_CP_TX_COPYBREAK boundary. Adding one field to it, or any
similar stack-passed structure growing past 128 bytes, would silently
cause kfree() to be called on a stack address in the reset-detected
or send-failure path, corrupting the slab allocator.
Would a BUILD_BUG_ON at each stack-based call site, or a typed helper
that only accepts on-stack buffers up to LIBIE_CP_TX_COPYBREAK, be
worth adding so this constraint is enforced at compile time rather
than by convention?

I would very much prefer to go the callback route right away, like in ixd,
but the diff would be even more unmanageable then.


[...]

quoted

@@ -1586,33 +1168,29 @@ int idpf_send_create_vport_msg(struct idpf_adapter *adapter,
 	}
 
 	if (!adapter->vport_params_recvd[idx]) {
-		adapter->vport_params_recvd[idx] = kzalloc(IDPF_CTLQ_MAX_BUF_LEN,
-							   GFP_KERNEL);
+		adapter->vport_params_recvd[idx] =
+			kzalloc(LIBIE_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
 		if (!adapter->vport_params_recvd[idx]) {
 			err = -ENOMEM;
 			goto rel_buf;
 		}
 	}
 
-	xn_params.vc_op = VIRTCHNL2_OP_CREATE_VPORT;
-	xn_params.send_buf.iov_base = vport_msg;
-	xn_params.send_buf.iov_len = buf_size;
-	xn_params.recv_buf.iov_base = adapter->vport_params_recvd[idx];
-	xn_params.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN;
-	xn_params.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
-	reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
-	if (reply_sz < 0) {
-		err = reply_sz;
-		goto free_vport_params;
+	err = idpf_send_mb_msg_kfree(adapter, &xn_params, vport_msg,
+				     sizeof(*vport_msg));
+	if (err) {
+		kfree(adapter->vport_params_recvd[idx]);
+		adapter->vport_params_recvd[idx] = NULL;
+		return err;
 	}
 
-	kfree(vport_msg);
+	memcpy(adapter->vport_params_recvd[idx], xn_params.recv_mem.iov_base,
+	       xn_params.recv_mem.iov_len);

Is the memcpy() length here guaranteed to be bounded?
adapter->vport_params_recvd[idx] is a fixed LIBIE_CTLQ_MAX_BUF_LEN
allocation, but the copy length is taken directly from
xn_params.recv_mem.iov_len without a defensive cap. This function

libie_cp does cap the iov_len at LIBIE_CTLQ_MAX_BUF_LEN in
libie_ctlq_fill_rx_msg(), so this is fine.

relies on libie's Rx path always delivering iov_len <=
LIBIE_CTLQ_MAX_BUF_LEN. If that invariant ever changes (larger RX
buffers, multi-descriptor delivery, or a missing bounds check against
the descriptor's data_len in libie), this becomes an unchecked heap
overflow driven by CP firmware.
Other call sites in the same patch use bounded copies, for example
idpf_send_get_caps_msg():
    memcpy(&adapter->caps, xn_params.recv_mem.iov_base,
           sizeof(adapter->caps));
Would using min_t(size_t, LIBIE_CTLQ_MAX_BUF_LEN,
xn_params.recv_mem.iov_len) match the pattern used elsewhere in this
file?