* [PATCH] qed: fix qed_fill_link() error handling @ 2016-05-30 15:46 Arnd Bergmann 2016-05-30 16:24 ` Yuval Mintz 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2016-05-30 15:46 UTC (permalink / raw) To: Yuval Mintz, Ariel Elior, everest-linux-l2 Cc: Arnd Bergmann, David S. Miller, Manish Chopra, Sudarsana Reddy Kalluru, netdev, linux-kernel 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(¶ms, 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, ¶ms); - qed_vf_get_link_state(hwfn, &link); - qed_vf_get_link_caps(hwfn, &link_caps); + if (qed_get_link_data(hwfn, ¶ms, &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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH] qed: fix qed_fill_link() error handling 2016-05-30 15:46 [PATCH] qed: fix qed_fill_link() error handling Arnd Bergmann @ 2016-05-30 16:24 ` Yuval Mintz 2016-05-31 21:20 ` David Miller 0 siblings, 1 reply; 10+ messages in thread From: Yuval Mintz @ 2016-05-30 16:24 UTC (permalink / raw) To: Arnd Bergmann Cc: David Miller, Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior > + 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qed: fix qed_fill_link() error handling 2016-05-30 16:24 ` Yuval Mintz @ 2016-05-31 21:20 ` David Miller 2016-05-31 22:34 ` Arnd Bergmann 0 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2016-05-31 21:20 UTC (permalink / raw) To: Yuval.Mintz Cc: arnd, manish.chopra, Sudarsana.Kalluru, netdev, linux-kernel, Ariel.Elior From: Yuval Mintz <Yuval.Mintz@qlogic.com> Date: Mon, 30 May 2016 16:24:07 +0000 >> + 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. I think both solutions are equally valid/elegant. Arnd? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qed: fix qed_fill_link() error handling 2016-05-31 21:20 ` David Miller @ 2016-05-31 22:34 ` Arnd Bergmann 2016-06-01 10:55 ` Yuval Mintz 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2016-05-31 22:34 UTC (permalink / raw) To: David Miller Cc: Yuval.Mintz, manish.chopra, Sudarsana.Kalluru, netdev, linux-kernel, Ariel.Elior On Tuesday, May 31, 2016 2:20:46 PM CEST David Miller wrote: > From: Yuval Mintz <Yuval.Mintz@qlogic.com> > Date: Mon, 30 May 2016 16:24:07 +0000 > > >> + 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. Adding a memset() to those functions would add a bit of overhead in code size because that ends up being unused in practice without a way for the compiler to know, I added the IS_ENABLED() check to reduce the object code size here by also eliminating the check for IS_PF(). > I think both solutions are equally valid/elegant. > > Arnd? I think we can just remove the IS_ENABLED() check there and define the IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set, like some other drivers do diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c index 287f61c20c19..756176525cf9 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_main.c +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn, { void *p; - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) { + if (!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); diff --git a/drivers/net/ethernet/qlogic/qed/qed_sriov.h b/drivers/net/ethernet/qlogic/qed/qed_sriov.h index c8667c65e685..c90b2b6ad969 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h @@ -12,11 +12,13 @@ #include "qed_vf.h" #define QED_VF_ARRAY_LENGTH (3) +#ifdef CONFIG_QED_SRIOV #define IS_VF(cdev) ((cdev)->b_is_vf) #define IS_PF(cdev) (!((cdev)->b_is_vf)) -#ifdef CONFIG_QED_SRIOV #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info)) #else +#define IS_VF(cdev) (0) +#define IS_PF(cdev) (1) #define IS_PF_SRIOV(p_hwfn) (0) #endif #define IS_PF_SRIOV_ALLOC(p_hwfn) (!!((p_hwfn)->pf_iov_info)) I don't see why that isn't already the case actually. If this is ok, I'll send an updated patch. For the PF case, we still need to fix the qed_mcp_get_link_params() failure case, so the rest of my patch is needed anyway, regardless of how we address the warning. Arnd ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH] qed: fix qed_fill_link() error handling 2016-05-31 22:34 ` Arnd Bergmann @ 2016-06-01 10:55 ` Yuval Mintz 2016-06-01 11:03 ` Arnd Bergmann 0 siblings, 1 reply; 10+ messages in thread From: Yuval Mintz @ 2016-06-01 10:55 UTC (permalink / raw) To: Arnd Bergmann, David Miller Cc: Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior > > I think both solutions are equally valid/elegant. > > > > Arnd? > > I think we can just remove the IS_ENABLED() check there and define the > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set, > like some other drivers do > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c > b/drivers/net/ethernet/qlogic/qed/qed_main.c > index 287f61c20c19..756176525cf9 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_main.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c > @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn, > { > void *p; > > - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) { > + if (!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); diff --git > a/drivers/net/ethernet/qlogic/qed/qed_sriov.h > b/drivers/net/ethernet/qlogic/qed/qed_sriov.h > index c8667c65e685..c90b2b6ad969 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h > +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h > @@ -12,11 +12,13 @@ > #include "qed_vf.h" > #define QED_VF_ARRAY_LENGTH (3) > > +#ifdef CONFIG_QED_SRIOV > #define IS_VF(cdev) ((cdev)->b_is_vf) > #define IS_PF(cdev) (!((cdev)->b_is_vf)) > -#ifdef CONFIG_QED_SRIOV > #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info)) > #else > +#define IS_VF(cdev) (0) > +#define IS_PF(cdev) (1) > #define IS_PF_SRIOV(p_hwfn) (0) > #endif > #define IS_PF_SRIOV_ALLOC(p_hwfn) (!!((p_hwfn)->pf_iov_info)) > > I don't see why that isn't already the case actually. If this is ok, I'll send an > updated patch. > > For the PF case, we still need to fix the qed_mcp_get_link_params() failure case, > so the rest of my patch is needed anyway, regardless of how we address the > warning. > > Arnd I think that would be unsafe with current qede - qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that how it goes]. Without changing this, if for some reason we'd have an assigned VF to a VM whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config], that VM is likely to miserably crash. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qed: fix qed_fill_link() error handling 2016-06-01 10:55 ` Yuval Mintz @ 2016-06-01 11:03 ` Arnd Bergmann 2016-06-01 11:10 ` Yuval Mintz 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2016-06-01 11:03 UTC (permalink / raw) To: Yuval Mintz Cc: David Miller, Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior On Wednesday, June 1, 2016 10:55:02 AM CEST Yuval Mintz wrote: > > > I think both solutions are equally valid/elegant. > > > > > > Arnd? > > > > I think we can just remove the IS_ENABLED() check there and define the > > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set, > > like some other drivers do > > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c > > b/drivers/net/ethernet/qlogic/qed/qed_main.c > > index 287f61c20c19..756176525cf9 100644 > > --- a/drivers/net/ethernet/qlogic/qed/qed_main.c > > +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c > > @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn, > > { > > void *p; > > > > - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) { > > + if (!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); diff --git > > a/drivers/net/ethernet/qlogic/qed/qed_sriov.h > > b/drivers/net/ethernet/qlogic/qed/qed_sriov.h > > index c8667c65e685..c90b2b6ad969 100644 > > --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h > > +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h > > @@ -12,11 +12,13 @@ > > #include "qed_vf.h" > > #define QED_VF_ARRAY_LENGTH (3) > > > > +#ifdef CONFIG_QED_SRIOV > > #define IS_VF(cdev) ((cdev)->b_is_vf) > > #define IS_PF(cdev) (!((cdev)->b_is_vf)) > > -#ifdef CONFIG_QED_SRIOV > > #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info)) > > #else > > +#define IS_VF(cdev) (0) > > +#define IS_PF(cdev) (1) > > #define IS_PF_SRIOV(p_hwfn) (0) > > #endif > > #define IS_PF_SRIOV_ALLOC(p_hwfn) (!!((p_hwfn)->pf_iov_info)) > > > > I don't see why that isn't already the case actually. If this is ok, I'll send an > > updated patch. > > > > For the PF case, we still need to fix the qed_mcp_get_link_params() failure case, > > so the rest of my patch is needed anyway, regardless of how we address the > > warning. > > > > I think that would be unsafe with current qede - > qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, > even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that > how it goes]. > Without changing this, if for some reason we'd have an assigned VF to a VM > whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config], > that VM is likely to miserably crash. Wouldn't it crash anyway if the code to handle VF devices is not present? E.g. the warning we got here tells us that qed_get_link_data() operates on uninitialized data when called on a VF device and SRIOV support is not built into the driver. I haven't looked if all the other functions handle that right, but my guess is that there are other functions with similar problems. Maybe it's best to remove the PCI IDs fort the virtual devices from the table if they are not supported by the configuration. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] qed: fix qed_fill_link() error handling 2016-06-01 11:03 ` Arnd Bergmann @ 2016-06-01 11:10 ` Yuval Mintz 2016-06-01 13:29 ` [PATCH v2] " Arnd Bergmann 0 siblings, 1 reply; 10+ messages in thread From: Yuval Mintz @ 2016-06-01 11:10 UTC (permalink / raw) To: Arnd Bergmann Cc: David Miller, Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior > > > I think we can just remove the IS_ENABLED() check there and define > > > the > > > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is > > > not set, like some other drivers do > > > > I think that would be unsafe with current qede - qede currently > > publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, even > > if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but > > that how it goes]. > > Without changing this, if for some reason we'd have an assigned VF to > > a VM whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an > > odd config], that VM is likely to miserably crash. > > Wouldn't it crash anyway if the code to handle VF devices is not present? > E.g. the warning we got here tells us that qed_get_link_data() operates on > uninitialized data when called on a VF device and SRIOV support is not built into > the driver. I haven't looked if all the other functions handle that right, but my > guess is that there are other functions with similar problems. > > Maybe it's best to remove the PCI IDs fort the virtual devices from the table if > they are not supported by the configuration. Actually, I think VF probe should gracefully fail in that case, as qed_vf_hw_prepare() would simply return -EINVAL. But I can honestly say I've never tested this flow, and I agree there's no reason to allow VF probe in case we're not supporting SRIOV. So I guess removing the PCI ID and defining IS_PF to be true in case CONFIG_QED_SRIOV isn't set is the right way to go. Do you want to revise your patch, or do you want me to do it? Thanks, Yuval ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] qed: fix qed_fill_link() error handling 2016-06-01 11:10 ` Yuval Mintz @ 2016-06-01 13:29 ` Arnd Bergmann 2016-06-01 13:36 ` Yuval Mintz 0 siblings, 1 reply; 10+ messages in thread From: Arnd Bergmann @ 2016-06-01 13:29 UTC (permalink / raw) To: Yuval Mintz Cc: David Miller, Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior 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. As discussed, we also use a compile-time check to ensure we never use any of the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is updated to no longer bind to virtual functions in that configuration. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote: > Actually, I think VF probe should gracefully fail in that case, > as qed_vf_hw_prepare() would simply return -EINVAL. > But I can honestly say I've never tested this flow, and I agree there's > no reason to allow VF probe in case we're not supporting SRIOV. ok > So I guess removing the PCI ID and defining IS_PF to be true in case > CONFIG_QED_SRIOV isn't set is the right way to go. > Do you want to revise your patch, or do you want me to do it? I've done the patch below now, please either Ack or modify it the way you like and forward it. Thanks, Arnd diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c index 753064679bde..61cc6869fa65 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_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(¶ms, 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, ¶ms); - qed_vf_get_link_state(hwfn, &link); - qed_vf_get_link_caps(hwfn, &link_caps); + if (qed_get_link_data(hwfn, ¶ms, &link, &link_caps)) { + dev_warn(&hwfn->cdev->pdev->dev, "no link data available\n"); + return; } /* Set the link parameters to pass to protocol driver */ diff --git a/drivers/net/ethernet/qlogic/qed/qed_sriov.h b/drivers/net/ethernet/qlogic/qed/qed_sriov.h index c8667c65e685..c90b2b6ad969 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h @@ -12,11 +12,13 @@ #include "qed_vf.h" #define QED_VF_ARRAY_LENGTH (3) +#ifdef CONFIG_QED_SRIOV #define IS_VF(cdev) ((cdev)->b_is_vf) #define IS_PF(cdev) (!((cdev)->b_is_vf)) -#ifdef CONFIG_QED_SRIOV #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info)) #else +#define IS_VF(cdev) (0) +#define IS_PF(cdev) (1) #define IS_PF_SRIOV(p_hwfn) (0) #endif #define IS_PF_SRIOV_ALLOC(p_hwfn) (!!((p_hwfn)->pf_iov_info)) diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c index 5d00d1404bfc..5733d1888223 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_main.c +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c @@ -87,7 +87,9 @@ static const struct pci_device_id qede_pci_tbl[] = { {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_100), QEDE_PRIVATE_PF}, {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_50), QEDE_PRIVATE_PF}, {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_25), QEDE_PRIVATE_PF}, +#ifdef CONFIG_QED_SRIOV {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_IOV), QEDE_PRIVATE_VF}, +#endif { 0 } }; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v2] qed: fix qed_fill_link() error handling 2016-06-01 13:29 ` [PATCH v2] " Arnd Bergmann @ 2016-06-01 13:36 ` Yuval Mintz 2016-06-02 5:06 ` David Miller 0 siblings, 1 reply; 10+ messages in thread From: Yuval Mintz @ 2016-06-01 13:36 UTC (permalink / raw) To: Arnd Bergmann, David Miller Cc: Manish Chopra, Sudarsana Kalluru, netdev, linux-kernel, Ariel Elior > 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. > > As discussed, we also use a compile-time check to ensure we never use any of > the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is > updated to no longer bind to virtual functions in that configuration. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote: > > Actually, I think VF probe should gracefully fail in that case, as > > qed_vf_hw_prepare() would simply return -EINVAL. > > But I can honestly say I've never tested this flow, and I agree > > there's no reason to allow VF probe in case we're not supporting SRIOV. > > ok > > > So I guess removing the PCI ID and defining IS_PF to be true in case > > CONFIG_QED_SRIOV isn't set is the right way to go. > > Do you want to revise your patch, or do you want me to do it? > > I've done the patch below now, please either Ack or modify it the way you like > and forward it. > > Thanks, > > Arnd Perhaps it would have made more sense as a 2-part series; But I'm content with the changes themselves. I'd let Dave decide whether he wants it split. Thanks for taking the time fixing this. Acked-by: Yuval Mintz <Yuval.Mintz@qlogic.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] qed: fix qed_fill_link() error handling 2016-06-01 13:36 ` Yuval Mintz @ 2016-06-02 5:06 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2016-06-02 5:06 UTC (permalink / raw) To: Yuval.Mintz Cc: arnd, manish.chopra, Sudarsana.Kalluru, netdev, linux-kernel, Ariel.Elior From: Yuval Mintz <Yuval.Mintz@qlogic.com> Date: Wed, 1 Jun 2016 13:36:38 +0000 >> 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. >> >> As discussed, we also use a compile-time check to ensure we never use any of >> the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is >> updated to no longer bind to virtual functions in that configuration. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> --- >> On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote: >> > Actually, I think VF probe should gracefully fail in that case, as >> > qed_vf_hw_prepare() would simply return -EINVAL. >> > But I can honestly say I've never tested this flow, and I agree >> > there's no reason to allow VF probe in case we're not supporting SRIOV. >> >> ok >> >> > So I guess removing the PCI ID and defining IS_PF to be true in case >> > CONFIG_QED_SRIOV isn't set is the right way to go. >> > Do you want to revise your patch, or do you want me to do it? >> >> I've done the patch below now, please either Ack or modify it the way you like >> and forward it. >> >> Thanks, >> >> Arnd > > Perhaps it would have made more sense as a 2-part series; But I'm content with > the changes themselves. I'd let Dave decide whether he wants it split. > > Thanks for taking the time fixing this. > > Acked-by: Yuval Mintz <Yuval.Mintz@qlogic.com> I think this is fine as one change, because the two changes are inetricably linked together in my opinion. Either you nop out these VF code paths and SRIOV is disabled and don't match the VF IDs, or you don't. There really isn't a valid intermediate state. So, applied, thanks everyone. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-02 5:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-30 15:46 [PATCH] qed: fix qed_fill_link() error handling Arnd Bergmann 2016-05-30 16:24 ` Yuval Mintz 2016-05-31 21:20 ` David Miller 2016-05-31 22:34 ` Arnd Bergmann 2016-06-01 10:55 ` Yuval Mintz 2016-06-01 11:03 ` Arnd Bergmann 2016-06-01 11:10 ` Yuval Mintz 2016-06-01 13:29 ` [PATCH v2] " Arnd Bergmann 2016-06-01 13:36 ` Yuval Mintz 2016-06-02 5:06 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).