[Intel-wired-lan] [PATCH net-next 2/6] ice: always check VF VSI pointer values

[Paul Menzel]

Dear Jacob,


Am 12.04.22 um 19:35 schrieb Jacob Keller:

> On 4/11/2022 10:38 PM, Paul Menzel wrote:

>> Am 12.04.22 um 01:29 schrieb Jacob Keller:
>>> The ice_get_vf_vsi function can return NULL in some cases, such as if
>>> handling messages during a reset where the VSI is being removed and
>>> recreated.
>>>
>>> Several places throughout the driver do not bother to check whether this
>>> VSI pointer is valid. Static analysis tools maybe report issues because
>>> they detect paths where a potentially NULL pointer could be
>>> dereferenced.
>>
>> (side note: scripts/checkpatch.pl checks for 75 characters per line, and
>> you seem to use 72 characters per line.)
> 
> For commit message wrapping? I use some default from a vim plugin which
> I guess decided that 72 was a good choice. Technically thats 8
> characters less than 80 which is one full tabstop if you render tabs as
> 8 spaces, which is sometimes used as a method of indenting commits from
> git tools..

Yes, but the git tools indent with four spaces. See commit 2a076f40d8c9 
(checkpatch, SubmittingPatches: suggest line wrapping commit messages at 
75 columns) [1]:

> Commit messages lines are sometimes overly long.
> 
> Suggest line wrapping at 75 columns so the default git commit log
> indentation of 4 plus the commit message text still fits on an 80 column
> screen.
> 
> Add a checkpatch test for long commit messages lines too.


>>> Fix this by checking the return value of ice_get_vf_vsi everywhere.
>>
>> I didn’t understand, when WARN_ON() is necessary, and when not, but
>> looks fine.
>>
> 
> I tried my best to use WARN_ON in places which have no suitable way to
> report an error back out, but for places where we can simply fail the
> operation I avoided it.
> 
> This was similar to how we handled invalid VF pointers in certain places.
> 
> The WARN_ONs aren't strictly needed, but generally speaking those flows
> shouldn't be hitting an invalid pointer. (Indeed, prior to this patch
> they would have been BUGs instead!)
> 
> Hope that explains why not every place has the WARN_ON.

Yes, thank you for clarifying this.

>>> Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
>>>    drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
>>>    drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
>>>    drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
>>>    drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
>>>    .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
>>>    6 files changed, 77 insertions(+), 7 deletions(-)
>>
>> […]
>>
>> Reviewed-by: Paul Menzel <pmenzel at molgen.mpg.de>


Kind regards,

Paul


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a076f40d8c9be95bee7bcf18436655e1140447f