[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <607248b2-bfb2-08a2-3d17-67c5c28840fc@intel.com>
Date: Mon, 18 Apr 2022 11:10:30 -0700
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Ivan Vecera <ivecera@...hat.com>
CC: Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
Fei Liu <feliu@...hat.com>, <netdev@...r.kernel.org>,
<mschmidt@...hat.com>, "Brett Creeley" <brett@...sando.io>,
open list <linux-kernel@...r.kernel.org>,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@...ts.osuosl.org>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH net] ice: Protect vf_state check by
cfg_lock in ice_vc_process_vf_msg()
On 4/16/2022 4:30 AM, Ivan Vecera wrote:
> On Fri, 15 Apr 2022 13:55:06 -0700
> Tony Nguyen <anthony.l.nguyen@...el.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
>
Powered by blists - more mailing lists