| Message ID | 20220413072259.3189386-1-ivecera@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net |
On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: > Previous patch labelled "ice: Fix incorrect locking in > ice_vc_process_vf_msg()" fixed an issue with ignored messages tiny tiny nit: double space after " Also, has mentioned patch landed onto some tree so that we could provide SHA-1 of it? If not, then maybe squashing this one with the mentioned one would make sense? > sent by VF driver but a small race window still left. > > Recently caught trace during 'ip link set ... vf 0 vlan ...' operation: > > [ 7332.995625] ice 0000:3b:00.0: Clearing port VLAN on VF 0 > [ 7333.001023] iavf 0000:3b:01.0: Reset indication received from the PF > [ 7333.007391] iavf 0000:3b:01.0: Scheduling reset task > [ 7333.059575] iavf 0000:3b:01.0: PF returned error -5 (IAVF_ERR_PARAM) to our request 3 > [ 7333.059626] ice 0000:3b:00.0: Invalid message from VF 0, opcode 3, len 4, error -1 > > Setting of VLAN for VF causes a reset of the affected VF using > ice_reset_vf() function that runs with cfg_lock taken: > > 1. ice_notify_vf_reset() informs IAVF driver that reset is needed and > IAVF schedules its own reset procedure > 2. Bit ICE_VF_STATE_DIS is set in vf->vf_state > 3. Misc initialization steps > 4. ice_sriov_post_vsi_rebuild() -> ice_vf_set_initialized() and that > clears ICE_VF_STATE_DIS in vf->vf_state > > Step 3 is mentioned race window because IAVF reset procedure runs in > parallel and one of its step is sending of VIRTCHNL_OP_GET_VF_RESOURCES > message (opcode==3). This message is handled in ice_vc_process_vf_msg() > and if it is received during the mentioned race window then it's > marked as invalid and error is returned to VF driver. > > Protect vf_state check in ice_vc_process_vf_msg() by cfg_lock to avoid > this race condition. > > Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling and VF ndo ops") > Tested-by: Fei Liu <feliu@redhat.com> > Signed-off-by: Ivan Vecera <ivecera@redhat.com> > --- > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 38 +++++++++---------- > 1 file changed, 17 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index 5612c032f15a..553287a75b50 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) > return; > } > > + mutex_lock(&vf->cfg_lock); > + > /* Check if VF is disabled. */ > if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { > err = -EPERM; > - goto error_handler; > - } > - > - ops = vf->virtchnl_ops; > - > - /* Perform basic checks on the msg */ > - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); > - if (err) { > - if (err == VIRTCHNL_STATUS_ERR_PARAM) > - err = -EPERM; > - else > - err = -EINVAL; > + } else { > + /* Perform basic checks on the msg */ > + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, > + msglen); > + if (err) { > + if (err == VIRTCHNL_STATUS_ERR_PARAM) > + err = -EPERM; > + else > + err = -EINVAL; > + } The chunk above feels a bit like unnecessary churn, no? Couldn't this patch be simply focused only on extending critical section? > } > - > -error_handler: > if (err) { > ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_PARAM, > NULL, 0); > dev_err(dev, "Invalid message from VF %d, opcode %d, len %d, error %d\n", > vf_id, v_opcode, msglen, err); > - ice_put_vf(vf); > - return; > + goto finish; > } > > - mutex_lock(&vf->cfg_lock); > - > if (!ice_vc_is_opcode_allowed(vf, v_opcode)) { > ice_vc_send_msg_to_vf(vf, v_opcode, > VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL, > 0); > - mutex_unlock(&vf->cfg_lock); > - ice_put_vf(vf); > - return; > + goto finish; > } > > + ops = vf->virtchnl_ops; > + > switch (v_opcode) { > case VIRTCHNL_OP_VERSION: > err = ops->get_ver_msg(vf, msg); > @@ -3773,6 +3768,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) > vf_id, v_opcode, err); > } > > +finish: > mutex_unlock(&vf->cfg_lock); > ice_put_vf(vf); > } > -- > 2.35.1 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Fri, Apr 15, 2022 at 01:55:10PM +0200, Maciej Fijalkowski wrote: > On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: > > Previous patch labelled "ice: Fix incorrect locking in > > ice_vc_process_vf_msg()" fixed an issue with ignored messages > > tiny tiny nit: double space after " > Also, has mentioned patch landed onto some tree so that we could provide > SHA-1 of it? If not, then maybe squashing this one with the mentioned one > would make sense? Again, Brett's Intel address is bouncing, so: CC: Brett Creeley <brett@pensando.io> > > > sent by VF driver but a small race window still left. > > > > Recently caught trace during 'ip link set ... vf 0 vlan ...' operation: > > > > [ 7332.995625] ice 0000:3b:00.0: Clearing port VLAN on VF 0 > > [ 7333.001023] iavf 0000:3b:01.0: Reset indication received from the PF > > [ 7333.007391] iavf 0000:3b:01.0: Scheduling reset task > > [ 7333.059575] iavf 0000:3b:01.0: PF returned error -5 (IAVF_ERR_PARAM) to our request 3 > > [ 7333.059626] ice 0000:3b:00.0: Invalid message from VF 0, opcode 3, len 4, error -1 > > > > Setting of VLAN for VF causes a reset of the affected VF using > > ice_reset_vf() function that runs with cfg_lock taken: > > > > 1. ice_notify_vf_reset() informs IAVF driver that reset is needed and > > IAVF schedules its own reset procedure > > 2. Bit ICE_VF_STATE_DIS is set in vf->vf_state > > 3. Misc initialization steps > > 4. ice_sriov_post_vsi_rebuild() -> ice_vf_set_initialized() and that > > clears ICE_VF_STATE_DIS in vf->vf_state > > > > Step 3 is mentioned race window because IAVF reset procedure runs in > > parallel and one of its step is sending of VIRTCHNL_OP_GET_VF_RESOURCES > > message (opcode==3). This message is handled in ice_vc_process_vf_msg() > > and if it is received during the mentioned race window then it's > > marked as invalid and error is returned to VF driver. > > > > Protect vf_state check in ice_vc_process_vf_msg() by cfg_lock to avoid > > this race condition. > > > > Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling and VF ndo ops") > > Tested-by: Fei Liu <feliu@redhat.com> > > Signed-off-by: Ivan Vecera <ivecera@redhat.com> > > --- > > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 38 +++++++++---------- > > 1 file changed, 17 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > index 5612c032f15a..553287a75b50 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > > @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) > > return; > > } > > > > + mutex_lock(&vf->cfg_lock); > > + > > /* Check if VF is disabled. */ > > if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { > > err = -EPERM; > > - goto error_handler; > > - } > > - > > - ops = vf->virtchnl_ops; > > - > > - /* Perform basic checks on the msg */ > > - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); > > - if (err) { > > - if (err == VIRTCHNL_STATUS_ERR_PARAM) > > - err = -EPERM; > > - else > > - err = -EINVAL; > > + } else { > > + /* Perform basic checks on the msg */ > > + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, > > + msglen); > > + if (err) { > > + if (err == VIRTCHNL_STATUS_ERR_PARAM) > > + err = -EPERM; > > + else > > + err = -EINVAL; > > + } > > The chunk above feels a bit like unnecessary churn, no? > Couldn't this patch be simply focused only on extending critical section? > > > } > > - > > -error_handler: > > if (err) { > > ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_PARAM, > > NULL, 0); > > dev_err(dev, "Invalid message from VF %d, opcode %d, len %d, error %d\n", > > vf_id, v_opcode, msglen, err); > > - ice_put_vf(vf); > > - return; > > + goto finish; > > } > > > > - mutex_lock(&vf->cfg_lock); > > - > > if (!ice_vc_is_opcode_allowed(vf, v_opcode)) { > > ice_vc_send_msg_to_vf(vf, v_opcode, > > VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL, > > 0); > > - mutex_unlock(&vf->cfg_lock); > > - ice_put_vf(vf); > > - return; > > + goto finish; > > } > > > > + ops = vf->virtchnl_ops; > > + > > switch (v_opcode) { > > case VIRTCHNL_OP_VERSION: > > err = ops->get_ver_msg(vf, msg); > > @@ -3773,6 +3768,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) > > vf_id, v_opcode, err); > > } > > > > +finish: > > mutex_unlock(&vf->cfg_lock); > > ice_put_vf(vf); > > } > > -- > > 2.35.1 > > > > _______________________________________________ > > Intel-wired-lan mailing list > > Intel-wired-lan@osuosl.org > > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: Fri, 15 Apr 2022 13:57:37 +0200 > On Fri, Apr 15, 2022 at 01:55:10PM +0200, Maciej Fijalkowski wrote: > > On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: > > > Previous patch labelled "ice: Fix incorrect locking in > > > ice_vc_process_vf_msg()" fixed an issue with ignored messages > > > > tiny tiny nit: double space after " > > Also, has mentioned patch landed onto some tree so that we could provide > > SHA-1 of it? If not, then maybe squashing this one with the mentioned one > > would make sense? > > Again, Brett's Intel address is bouncing, so: > CC: Brett Creeley <brett@pensando.io> Cc: Jacob Keller <jacob.e.keller@intel.com> > > > > > > sent by VF driver but a small race window still left. --- 8< --- > > > -- > > > 2.35.1 Al
On Fri, 15 Apr 2022 13:55:02 +0200 Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: > > Previous patch labelled "ice: Fix incorrect locking in > > ice_vc_process_vf_msg()" fixed an issue with ignored messages > > tiny tiny nit: double space after " > Also, has mentioned patch landed onto some tree so that we could provide > SHA-1 of it? If not, then maybe squashing this one with the mentioned one > would make sense? Well, that commit were already tested and now it is present in Tony's queue but not in upstream yet. It is not problem to squash together but the first was about ignored VF messages and this one is about race and I didn't want to make single patch with huge description that cover both issues. But as I said, no problem to squash if needed. Thx, Ivan
> -----Original Message----- > From: Ivan Vecera <ivecera@redhat.com> > Sent: Friday, April 15, 2022 9:39 AM > To: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> > Cc: netdev@vger.kernel.org; Fei Liu <feliu@redhat.com>; moderated list:INTEL > ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; mschmidt > <mschmidt@redhat.com>; Brett Creeley <brett.creeley@intel.com>; open list > <linux-kernel@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; David S. Miller <davem@davemloft.net> > Subject: Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in > ice_vc_process_vf_msg() > > On Fri, 15 Apr 2022 13:55:02 +0200 > Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > > On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: > > > Previous patch labelled "ice: Fix incorrect locking in > > > ice_vc_process_vf_msg()" fixed an issue with ignored messages > > > > tiny tiny nit: double space after " > > Also, has mentioned patch landed onto some tree so that we could provide > > SHA-1 of it? If not, then maybe squashing this one with the mentioned one > > would make sense? > > Well, that commit were already tested and now it is present in Tony's queue > but not in upstream yet. It is not problem to squash together but the first > was about ignored VF messages and this one is about race and I didn't want > to make single patch with huge description that cover both issues. > But as I said, no problem to squash if needed. > > Thx, > Ivan I'm fine with either squashing or keeping them as separate changes.
On 4/15/2022 11:31 AM, Keller, Jacob E wrote: > >> -----Original Message----- >> From: Ivan Vecera <ivecera@redhat.com> >> Sent: Friday, April 15, 2022 9:39 AM >> To: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> >> Cc: netdev@vger.kernel.org; Fei Liu <feliu@redhat.com>; moderated list:INTEL >> ETHERNET DRIVERS <intel-wired-lan@lists.osuosl.org>; mschmidt >> <mschmidt@redhat.com>; Brett Creeley <brett.creeley@intel.com>; open list >> <linux-kernel@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni >> <pabeni@redhat.com>; David S. Miller <davem@davemloft.net> >> Subject: Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in >> ice_vc_process_vf_msg() >> >> On Fri, 15 Apr 2022 13:55:02 +0200 >> Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: >> >>> On Wed, Apr 13, 2022 at 09:22:59AM +0200, Ivan Vecera wrote: >>>> Previous patch labelled "ice: Fix incorrect locking in >>>> ice_vc_process_vf_msg()" fixed an issue with ignored messages >>> tiny tiny nit: double space after " >>> Also, has mentioned patch landed onto some tree so that we could provide >>> SHA-1 of it? If not, then maybe squashing this one with the mentioned one >>> would make sense? >> Well, that commit were already tested and now it is present in Tony's queue >> but not in upstream yet. It is not problem to squash together but the first >> was about ignored VF messages and this one is about race and I didn't want >> to make single patch with huge description that cover both issues. >> But as I said, no problem to squash if needed. >> >> Thx, >> Ivan > I'm fine with either squashing or keeping them as separate changes. Either way sounds ok to me as they are different types of changes. Thanks, Tony
On 4/15/2022 4:57 AM, Maciej Fijalkowski wrote: <snip> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>> index 5612c032f15a..553287a75b50 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>> @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) >>> return; >>> } >>> >>> + mutex_lock(&vf->cfg_lock); >>> + >>> /* Check if VF is disabled. */ >>> if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { >>> err = -EPERM; >>> - goto error_handler; >>> - } >>> - >>> - ops = vf->virtchnl_ops; >>> - >>> - /* Perform basic checks on the msg */ >>> - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); >>> - if (err) { >>> - if (err == VIRTCHNL_STATUS_ERR_PARAM) >>> - err = -EPERM; >>> - else >>> - err = -EINVAL; >>> + } else { >>> + /* Perform basic checks on the msg */ >>> + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, >>> + msglen); >>> + if (err) { >>> + if (err == VIRTCHNL_STATUS_ERR_PARAM) >>> + err = -EPERM; >>> + else >>> + err = -EINVAL; >>> + } >> The chunk above feels a bit like unnecessary churn, no? >> Couldn't this patch be simply focused only on extending critical section? Agree, this doesn't seem related to the fix. Thanks, Tony >>> } >>> - >>> -error_handler: >>> if (err) { >>> ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_PARAM, >>> NULL, 0); >>> dev_err(dev, "Invalid message from VF %d, opcode %d, len %d, error %d\n", >>> vf_id, v_opcode, msglen, err); >>> - ice_put_vf(vf); >>> - return; >>> + goto finish; >>> } >>> >>> - mutex_lock(&vf->cfg_lock); >>> - >>> if (!ice_vc_is_opcode_allowed(vf, v_opcode)) { >>> ice_vc_send_msg_to_vf(vf, v_opcode, >>> VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL, >>> 0); >>> - mutex_unlock(&vf->cfg_lock); >>> - ice_put_vf(vf); >>> - return; >>> + goto finish; >>> } >>> >>> + ops = vf->virtchnl_ops; >>> + >>> switch (v_opcode) { >>> case VIRTCHNL_OP_VERSION: >>> err = ops->get_ver_msg(vf, msg); >>> @@ -3773,6 +3768,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) >>> vf_id, v_opcode, err); >>> } >>> >>> +finish: >>> mutex_unlock(&vf->cfg_lock); >>> ice_put_vf(vf); >>> } >>> -- >>> 2.35.1 >>> >>> _______________________________________________ >>> Intel-wired-lan mailing list >>> Intel-wired-lan@osuosl.org >>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Fri, 15 Apr 2022 13:55:06 -0700 Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > >>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > >>> index 5612c032f15a..553287a75b50 100644 > >>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > >>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > >>> @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) > >>> return; > >>> } > >>> > >>> + mutex_lock(&vf->cfg_lock); > >>> + > >>> /* Check if VF is disabled. */ > >>> if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { > >>> err = -EPERM; > >>> - goto error_handler; > >>> - } > >>> - > >>> - ops = vf->virtchnl_ops; > >>> - > >>> - /* Perform basic checks on the msg */ > >>> - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); > >>> - if (err) { > >>> - if (err == VIRTCHNL_STATUS_ERR_PARAM) > >>> - err = -EPERM; > >>> - else > >>> - err = -EINVAL; > >>> + } else { > >>> + /* Perform basic checks on the msg */ > >>> + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, > >>> + msglen); > >>> + if (err) { > >>> + if (err == VIRTCHNL_STATUS_ERR_PARAM) > >>> + err = -EPERM; > >>> + else > >>> + err = -EINVAL; > >>> + } > >> The chunk above feels a bit like unnecessary churn, no? > >> Couldn't this patch be simply focused only on extending critical section? > > Agree, this doesn't seem related to the fix. > > Thanks, > > Tony Yes, it is not directly related but it's just a conversion of following snippet to avoid ugly and unnecessary 'goto': if (A) { err = ... goto error_handler; } if (B) { err = ... ... } if (err) { ... } to if (A) { err = ... } else { if (B) { ... } } if (err) { ... } If you want to leave the code as is and remove this from the patch let me know and I will send v2. Thanks, Ivan
On 4/16/2022 4:30 AM, Ivan Vecera wrote: > On Fri, 15 Apr 2022 13:55:06 -0700 > Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > >>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>>>> index 5612c032f15a..553287a75b50 100644 >>>>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>>>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c >>>>> @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) >>>>> return; >>>>> } >>>>> >>>>> + mutex_lock(&vf->cfg_lock); >>>>> + >>>>> /* Check if VF is disabled. */ >>>>> if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { >>>>> err = -EPERM; >>>>> - goto error_handler; >>>>> - } >>>>> - >>>>> - ops = vf->virtchnl_ops; >>>>> - >>>>> - /* Perform basic checks on the msg */ >>>>> - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); >>>>> - if (err) { >>>>> - if (err == VIRTCHNL_STATUS_ERR_PARAM) >>>>> - err = -EPERM; >>>>> - else >>>>> - err = -EINVAL; >>>>> + } else { >>>>> + /* Perform basic checks on the msg */ >>>>> + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, >>>>> + msglen); >>>>> + if (err) { >>>>> + if (err == VIRTCHNL_STATUS_ERR_PARAM) >>>>> + err = -EPERM; >>>>> + else >>>>> + err = -EINVAL; >>>>> + } >>>> The chunk above feels a bit like unnecessary churn, no? >>>> Couldn't this patch be simply focused only on extending critical section? >> Agree, this doesn't seem related to the fix. >> >> Thanks, >> >> Tony > Yes, it is not directly related but it's just a conversion of following snippet > to avoid ugly and unnecessary 'goto': > > if (A) { > err = ... > goto error_handler; > } > if (B) { > err = ... > ... > } > if (err) { > ... > } > > to > > if (A) { > err = ... > } else { > if (B) { > ... > } > } > if (err) { > ... > } > > If you want to leave the code as is and remove this from the patch > let me know and I will send v2. The change itself looks ok to me, but for net patches, we should fix the issue without introducing other changes. A v2 without this change would be great; feel free to submit this change to -next after I've applied the v2 for this patch. Thanks, Tony > Thanks, > Ivan >
On Mon, 18 Apr 2022 11:10:30 -0700 Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > If you want to leave the code as is and remove this from the patch > > let me know and I will send v2. > > The change itself looks ok to me, but for net patches, we should fix the > issue without introducing other changes. A v2 without this change would > be great; feel free to submit this change to -next after I've applied > the v2 for this patch. > > Thanks, > > Tony Agree, sending v2. Thanks, Ivan
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index 5612c032f15a..553287a75b50 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -3625,44 +3625,39 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) return; } + mutex_lock(&vf->cfg_lock); + /* Check if VF is disabled. */ if (test_bit(ICE_VF_STATE_DIS, vf->vf_states)) { err = -EPERM; - goto error_handler; - } - - ops = vf->virtchnl_ops; - - /* Perform basic checks on the msg */ - err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, msglen); - if (err) { - if (err == VIRTCHNL_STATUS_ERR_PARAM) - err = -EPERM; - else - err = -EINVAL; + } else { + /* Perform basic checks on the msg */ + err = virtchnl_vc_validate_vf_msg(&vf->vf_ver, v_opcode, msg, + msglen); + if (err) { + if (err == VIRTCHNL_STATUS_ERR_PARAM) + err = -EPERM; + else + err = -EINVAL; + } } - -error_handler: if (err) { ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_PARAM, NULL, 0); dev_err(dev, "Invalid message from VF %d, opcode %d, len %d, error %d\n", vf_id, v_opcode, msglen, err); - ice_put_vf(vf); - return; + goto finish; } - mutex_lock(&vf->cfg_lock); - if (!ice_vc_is_opcode_allowed(vf, v_opcode)) { ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL, 0); - mutex_unlock(&vf->cfg_lock); - ice_put_vf(vf); - return; + goto finish; } + ops = vf->virtchnl_ops; + switch (v_opcode) { case VIRTCHNL_OP_VERSION: err = ops->get_ver_msg(vf, msg); @@ -3773,6 +3768,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) vf_id, v_opcode, err); } +finish: mutex_unlock(&vf->cfg_lock); ice_put_vf(vf); }
Previous patch labelled "ice: Fix incorrect locking in ice_vc_process_vf_msg()" fixed an issue with ignored messages sent by VF driver but a small race window still left. Recently caught trace during 'ip link set ... vf 0 vlan ...' operation: [ 7332.995625] ice 0000:3b:00.0: Clearing port VLAN on VF 0 [ 7333.001023] iavf 0000:3b:01.0: Reset indication received from the PF [ 7333.007391] iavf 0000:3b:01.0: Scheduling reset task [ 7333.059575] iavf 0000:3b:01.0: PF returned error -5 (IAVF_ERR_PARAM) to our request 3 [ 7333.059626] ice 0000:3b:00.0: Invalid message from VF 0, opcode 3, len 4, error -1 Setting of VLAN for VF causes a reset of the affected VF using ice_reset_vf() function that runs with cfg_lock taken: 1. ice_notify_vf_reset() informs IAVF driver that reset is needed and IAVF schedules its own reset procedure 2. Bit ICE_VF_STATE_DIS is set in vf->vf_state 3. Misc initialization steps 4. ice_sriov_post_vsi_rebuild() -> ice_vf_set_initialized() and that clears ICE_VF_STATE_DIS in vf->vf_state Step 3 is mentioned race window because IAVF reset procedure runs in parallel and one of its step is sending of VIRTCHNL_OP_GET_VF_RESOURCES message (opcode==3). This message is handled in ice_vc_process_vf_msg() and if it is received during the mentioned race window then it's marked as invalid and error is returned to VF driver. Protect vf_state check in ice_vc_process_vf_msg() by cfg_lock to avoid this race condition. Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling and VF ndo ops") Tested-by: Fei Liu <feliu@redhat.com> Signed-off-by: Ivan Vecera <ivecera@redhat.com> --- drivers/net/ethernet/intel/ice/ice_virtchnl.c | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-)