Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409244 > unrolled thread

[PATCH] qed: fix qed_fill_link() error handling

Back to article view | Back to linux.kernel

Contents

  [PATCH] qed: fix qed_fill_link() error handling Arnd Bergmann <arnd@arndb.de> - 2016-05-30 17:50 +0200
    RE: [PATCH] qed: fix qed_fill_link() error handling Yuval Mintz <Yuval.Mintz@qlogic.com> - 2016-05-30 18:40 +0200

#1409244 — [PATCH] qed: fix qed_fill_link() error handling

gcc warns about qed_fill_link possibly accessing uninitialized data:

drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link':
drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be used uninitialized in this function [-Werror=maybe-uninitialized]

While this warning is only about the specific case of CONFIG_QED_SRIOV
being disabled but the function getting called for a VF (which should
never happen), another possibility is that qed_mcp_get_*() fails without
returning data.

This rearranges the code so we bail out in either of the two cases
and print a warning instead of accessing the uninitialized data.

The qed_link_output structure remains untouched in this case, but
all callers first call memset() on it, so at least we are not leaking
stack data then.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 45 ++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 753064679bde..68f87a4f9316 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1105,6 +1105,39 @@ static int qed_get_port_type(u32 media_type)
 	return port_type;
 }
 
+static int qed_get_link_data(struct qed_hwfn *hwfn,
+			     struct qed_mcp_link_params *params,
+			     struct qed_mcp_link_state *link,
+			     struct qed_mcp_link_capabilities *link_caps)
+{
+	void *p;
+
+	if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
+		qed_vf_get_link_params(hwfn, params);
+		qed_vf_get_link_state(hwfn, link);
+		qed_vf_get_link_caps(hwfn, link_caps);
+
+		return 0;
+	}
+
+	p = qed_mcp_get_link_params(hwfn);
+	if (!p)
+		return -ENXIO;
+	memcpy(params, p, sizeof(*params));
+
+	p = qed_mcp_get_link_state(hwfn);
+	if (!p)
+		return -ENXIO;
+	memcpy(link, p, sizeof(*link));
+
+	p = qed_mcp_get_link_capabilities(hwfn);
+	if (!p)
+		return -ENXIO;
+	memcpy(link_caps, p, sizeof(*link_caps));
+
+	return 0;
+}
+
 static void qed_fill_link(struct qed_hwfn *hwfn,
 			  struct qed_link_output *if_link)
 {
@@ -1116,15 +1149,9 @@ static void qed_fill_link(struct qed_hwfn *hwfn,
 	memset(if_link, 0, sizeof(*if_link));
 
 	/* Prepare source inputs */
-	if (IS_PF(hwfn->cdev)) {
-		memcpy(&params, qed_mcp_get_link_params(hwfn), sizeof(params));
-		memcpy(&link, qed_mcp_get_link_state(hwfn), sizeof(link));
-		memcpy(&link_caps, qed_mcp_get_link_capabilities(hwfn),
-		       sizeof(link_caps));
-	} else {
-		qed_vf_get_link_params(hwfn, &params);
-		qed_vf_get_link_state(hwfn, &link);
-		qed_vf_get_link_caps(hwfn, &link_caps);
+	if (qed_get_link_data(hwfn, &params, &link, &link_caps)) {
+		dev_warn(&hwfn->cdev->pdev->dev, "no link data available\n");
+		return;
 	}
 
 	/* Set the link parameters to pass to protocol driver */
-- 
2.7.0

[toc] | [next] | [standalone]

#1409273

> +	if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) {
> +		qed_vf_get_link_params(hwfn, params);
> +		qed_vf_get_link_state(hwfn, link);
> +		qed_vf_get_link_caps(hwfn, link_caps);
> +
> +		return 0;
> +	}

The IS_ENABLED here seems a bit wasteful to me - we have empty implementation
under qed_vf.h just for this case [I.e., that SRIOV isn't enabled for qed].
If all we're trying achieve is removing these gcc warnings, I think we can simply
memset the structs in the currently-empty qed_vf_get_link_* functions.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web