| 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
| ||
|
Date: Tue, 5 Nov 2019 05:23:59 +0000
From: Arkady Gilinsky <arkady.gilinsky@...monicinc.com>
To: "Creeley, Brett" <brett.creeley@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC: Arkady Gilinsky <arcadyg@...il.com>
Subject: Re: [EXTERNAL] RE: [PATCH net] i40e/iavf: Fix msg interface between
VF and PF
On Mon, 2019-11-04 at 23:43 +0000, Creeley, Brett wrote:
> > -----Original Message-----
> > From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On Behalf Of Arkady Gilinsky
> > Sent: Sunday, November 3, 2019 9:32 PM
> > To: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>
> > Cc: Arkady Gilinsky <arcadyg@...il.com>
> > Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> >
> > From af5ab2aaa51882bb7111b026882fe217ed81c15b Mon Sep 17 00:00:00 2001
> > From: Arkady Gilinsky <arkady.gilinsky@...monicinc
> > .com>
> > Date: Sun, 3 Nov 2019 20:37:21 +0200
> > Subject: [PATCH net] i40e/iavf: Fix msg interface between VF and PF
> >
> > * The original issue was that iavf driver passing TX/RX queues
> > as bitmap in iavf_disable_queues and the i40e driver
> > interprets this message as an absolute number in
> > i40e_vc_disable_queues_msg, so the validation in the
> > latter function always fail.
>
> The VIRTCHNL interface expects the tx_queues and rx_queues to be sent as
> a bitmap so this should/can not be changed.
The fields [t|r]x_queues through whole driver code contains the number
and not bitmap (as far as I understand), so I thought that keeping
this convention is more important for better code readability,
saving in these fields something different is very confusing, IMHO,
and potentially leads to such issues that this commit is trying to solve.
>
> I think something like the following would be better becase the change is
> completely contained inside the i40e driver and it should completely
> address the issue you are seeing. Of course, this would have to be
> in both the enable and disable queue flow. Also, with this change it might
> be best to check and make sure the sizeof(vqs->[r|t]x_queues) equal
> sizeof(u32) in case those members ever change from u32 to u64, which
> will cause this check to always fail.
>
> static bool i40e_vc_verify_vqs_bitmaps(struct virtchnl_queue_select *vqs)
> {
> /* this will catch any changes made to the virtchnl_queue_select bitmap */
> if (sizeof(vqs->rx_queues) != sizeof(u32) ||
> sizeof(vqs->tx_queues) != sizeof(u32))
> return false;
If so, then is it better to check the type of the fields in compile-time rather than in runtime ?
Something like this:
BUILD_BUG_ON(sizeof(vqs->rx_queues) != sizeof(u32));
BUILD_BUG_ON(sizeof(vqs->tx_queues) != sizeof(u32));
This is not required comparison each time when function is called and made code more optimized.
>
> if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> hweight32(vqs->rx_queues) > I40E_MAX_VF_QUEUES ||
> hweight32(vqs->tx_queues) > I40E_MAX_VF_QUEUES)
> return false;
Again, from optimization POV it is better to have constant changed than variable,
since it is compile time and not run time action:
if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
vqs->rx_queues >= (BIT(I40E_MAX_VF_QUEUES)) ||
vqs->tx_queues >= (BIT(I40E_MAX_VF_QUEUES)))
return false;
> return true;
> }
>
> if (i40e_vc_verify_vqs_bitmaps(vqs)) {
> aq_ret = I40E_ERR_PARAM;
> goto error_param;
> }
>
> > This commit reorganize the msg interface between i40e and iavf
> > between the iavf_disable_queues and i40e_vc_disable_queues_msg
> > functions (also for iavf_enable_queues and i40e_vc_enable_queues_msg).
> > Now both drivers operate with number of queues instead of
> > bitmap. Also the commit introduces range check in
> > i40e_vc_enable_queues_msg function.
> >
> > Signed-off-by: Arkady Gilinsky <arkady.gilinsky@...monicinc.com>
> > ---
> > drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 23 ++++++++++++++++------
> > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 6 ++++--
> > 2 files changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > index 3d2440838822..c650eb91982a 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> > @@ -2352,13 +2352,22 @@ static int i40e_vc_enable_queues_msg(struct i40e_vf *vf, u8 *msg)
> > goto error_param;
> > }
> >
> > - /* Use the queue bit map sent by the VF */
> > - if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> > + if ((vqs->rx_queues == 0 && vqs->tx_queues == 0) ||
> > + vqs->rx_queues > I40E_MAX_VF_QUEUES ||
> > + vqs->tx_queues > I40E_MAX_VF_QUEUES) {
> > + aq_ret = I40E_ERR_PARAM;
> > + goto error_param;
> > + }
> > +
> > + /* Build the queue bit map from value sent by the VF */
> > + if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> > + BIT(vqs->rx_queues) - 1,
> > true)) {
> > aq_ret = I40E_ERR_TIMEOUT;
> > goto error_param;
> > }
> > - if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> > + if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> > + BIT(vqs->tx_queues) - 1,
> > true)) {
> > aq_ret = I40E_ERR_TIMEOUT;
> > goto error_param;
> > @@ -2416,13 +2425,15 @@ static int i40e_vc_disable_queues_msg(struct i40e_vf *vf, u8 *msg)
> > goto error_param;
> > }
> >
> > - /* Use the queue bit map sent by the VF */
> > - if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx], vqs->tx_queues,
> > + /* Build the queue bit map from value sent by the VF */
> > + if (i40e_ctrl_vf_tx_rings(pf->vsi[vf->lan_vsi_idx],
> > + BIT(vqs->tx_queues) - 1,
> > false)) {
> > aq_ret = I40E_ERR_TIMEOUT;
> > goto error_param;
> > }
> > - if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx], vqs->rx_queues,
> > + if (i40e_ctrl_vf_rx_rings(pf->vsi[vf->lan_vsi_idx],
> > + BIT(vqs->rx_queues) - 1,
> > false)) {
> > aq_ret = I40E_ERR_TIMEOUT;
> > goto error_param;
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > index c46770eba320..271f144bf05b 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> > @@ -308,7 +308,8 @@ void iavf_enable_queues(struct iavf_adapter *adapter)
> > }
> > adapter->current_op = VIRTCHNL_OP_ENABLE_QUEUES;
> > vqs.vsi_id = adapter->vsi_res->vsi_id;
> > - vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> > + /* Send the queues number to PF */
> > + vqs.tx_queues = adapter->num_active_queues;
> > vqs.rx_queues = vqs.tx_queues;
> > adapter->aq_required &= ~IAVF_FLAG_AQ_ENABLE_QUEUES;
> > iavf_send_pf_msg(adapter, VIRTCHNL_OP_ENABLE_QUEUES,
> > @@ -333,7 +334,8 @@ void iavf_disable_queues(struct iavf_adapter *adapter)
> > }
> > adapter->current_op = VIRTCHNL_OP_DISABLE_QUEUES;
> > vqs.vsi_id = adapter->vsi_res->vsi_id;
> > - vqs.tx_queues = BIT(adapter->num_active_queues) - 1;
> > + /* Send the queues number to PF */
> > + vqs.tx_queues = adapter->num_active_queues;
> > vqs.rx_queues = vqs.tx_queues;
> > adapter->aq_required &= ~IAVF_FLAG_AQ_DISABLE_QUEUES;
> > iavf_send_pf_msg(adapter, VIRTCHNL_OP_DISABLE_QUEUES,
> > --
> > 2.11.0
>
--
Best regards,
Arkady Gilinsky
------------------------------------------
Powered by blists - more mailing lists