RE: [PATCH net-next 03/15] net: enetc: add link status message support to PF driver

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c

b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c

quoted

--- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c

@@ -458,6 +458,8 @@ int enetc_init_sriov_resources(struct enetc_pf *pf)
 	for (int i = 0; i < pf->total_vfs; i++)
 		mutex_init(&pf->vf_state[i].lock);

+	mutex_init(&pf->msg_lock);
+
 	return 0;
 }

[Severity: High]
If the device does not support SR-IOV or total_vfs is 0,
enetc_init_sriov_resources() returns early:

        pf->total_vfs = pci_sriov_get_totalvfs(pf->si->pdev);
        if (!pf->total_vfs)
                return 0;

This skips the mutex_init() for msg_lock.

However, the phylink callbacks enetc4_pl_mac_link_up() and
enetc4_pl_mac_link_down() unconditionally call enetc_pf_notify_vf_link_up()
and enetc_pf_notify_vf_link_down().

These functions eventually reach enetc_pf_notify_vf_link_status(), which
attempts to acquire this uninitialized lock:

enetc_pf_notify_vf_link_status()
    mutex_lock(&pf->msg_lock);

Could this cause a lockdep warning or a crash when the device link comes
up or goes down and total_vfs is 0?

This is a bug, will fix it in v2.

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c

b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

quoted

--- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

[ ... ]

quoted

+static int enetc_pf_send_msg(struct enetc_pf *pf, u32 msg_code, u16

ms_mask)

quoted

+{
+	struct enetc_si *si = pf->si;
+	u32 val;
+
+	enetc_wr(&si->hw, ENETC_PSIMSGSR,
+		 FIELD_PREP(PSIMSGSR_MC, msg_code) | ms_mask);
+
+	return read_poll_timeout(enetc_rd, val, !(val & ms_mask), 1000,
+				 200000, false, &si->hw, ENETC_PSIMSGSR);
+}
+
+static void enetc_msg_notify_vf_link_status(struct enetc_pf *pf, u16

ms_mask,

quoted

+					    bool link_up)
+{

[ ... ]

quoted

+	if (enetc_pf_send_msg(pf, pf_msg, ms_mask))
+		dev_err_ratelimited(&pf->si->pdev->dev,
+				    "PF notifies link status failed\n");
+}

[Severity: High]
If a VF is malicious, crashed, or improperly configured and fails to clear
its bit in ENETC_PSIMSGSR, enetc_pf_send_msg() will block the phylink worker
thread for its maximum 200ms polling timeout.

Because enetc_msg_notify_vf_link_status() only logs an error and does not
remove the unresponsive VF from pf->link_status_ms_mask, won't every
subsequent link state change also incur this 200ms stall?

This problem is unavoidable. We cannot determine whether VF is malicious
or simply not responding due to busy conditions. Removing it from
pf->link_status_ms_mask will cause VF to malfunction.

Please ignore this warning.

Additionally, a malicious VF could spam
ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER
messages, forcing the PF to repeatedly call enetc_pf_send_msg() while holding
pf->msg_lock. Could this stall the shared msg_task workqueue and starve
other VFs?

I don't think that's the case, since msg_task doesn't only handle messages from
malicious VFs.

quoted

@@ -361,6 +471,7 @@ int enetc_sriov_configure(struct pci_dev *pdev, int

num_vfs)

quoted

 	if (!num_vfs) {
 		pci_disable_sriov(pdev);
 		enetc_msg_psi_free(pf);
+		pf->link_status_ms_mask = 0;
 		pf->num_vfs = 0;
 	} else {

[Severity: Medium]
When disabling SR-IOV, pf->link_status_ms_mask is cleared outside of
pf->msg_lock and after the VFs have been disabled via pci_disable_sriov().

If a physical link state change occurs concurrently, could
enetc_pf_notify_vf_link_status() acquire the lock, observe a stale non-zero
ms_mask, and attempt to notify destroyed VFs?

Since the VFs are no longer active, they wouldn't clear their bits, causing
enetc_pf_send_msg() to inevitably timeout after 200ms. Should this mask be
cleared under the msg_lock before disabling SR-IOV?

I don't think this needs to be improved. First, this kind of problem is very rare,
and second, the timeout is normal and will not have a functional impact.

And the problem could not be solved even before moving to
pci_disable_sriov(). Because VF may still set its mask via message during the
window between clearing the mask and VF actually closing.

--
pw-bot: cr