[PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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(-)

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);
 }
-- 
2.35.1

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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(-)

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);
 }
-- 
2.35.1

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Maciej Fijalkowski]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Maciej Fijalkowski]

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 at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Maciej Fijalkowski]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Maciej Fijalkowski]

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 at osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Alexander Lobakin]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Alexander Lobakin]

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

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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

RE: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Keller, Jacob E]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Keller, Jacob E]

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

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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 at osuosl.org
>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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
>

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Tony Nguyen]

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
>

Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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

[Intel-wired-lan] [PATCH net] ice: Protect vf_state check by cfg_lock in ice_vc_process_vf_msg()

[Ivan Vecera]

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