linux-kernel - RE: [PATCH] qed: fix qed_fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 1 Jun 2016 10:55:02 +0000
From:	Yuval Mintz <Yuval.Mintz@...gic.com>
To:	Arnd Bergmann <arnd@...db.de>, David Miller <davem@...emloft.net>
CC:	Manish Chopra <manish.chopra@...gic.com>,
	Sudarsana Kalluru <Sudarsana.Kalluru@...gic.com>,
	netdev <netdev@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Ariel Elior <Ariel.Elior@...gic.com>
Subject: RE: [PATCH] qed: fix qed_fill_link() error handling

> > 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.