Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions

[Philipp Stanner <pstanner@redhat.com>]

Hello,

On Thu, 2024-04-04 at 09:29 +0300, Dan Carpenter wrote:
> Hi Philipp,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/PCI-Add-new-set-of-devres-functions/20240403-160932
> base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:   
> https://lore.kernel.org/r/20240403080712.13986-5-pstanner%40redhat.com
> patch subject: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
> config: i386-randconfig-141-20240404
> (https://download.01.org/0day-ci/archive/20240404/202404040920.QIxhNe
> Mu-lkp@intel.com/config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes:
> > https://lore.kernel.org/r/202404040920.QIxhNeMu-lkp@intel.com/
> 
> smatch warnings:
> drivers/pci/devres.c:897 pcim_iomap_regions_request_all() error: we
> previously assumed 'legacy_iomap_table' could be null (see line 894)
> 
> vim +/legacy_iomap_table +897 drivers/pci/devres.c
> 
> acc2364fe66106 Philipp Stanner 2024-01-31  865  int
> pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
> acc2364fe66106 Philipp Stanner 2024-01-31 
> 866                                     const char *name)
> acc2364fe66106 Philipp Stanner 2024-01-31  867  {
> 34e90b966504f3 Philipp Stanner 2024-04-03  868          short bar;
> 34e90b966504f3 Philipp Stanner 2024-04-03  869          int ret;
> 34e90b966504f3 Philipp Stanner 2024-04-03  870          void __iomem
> **legacy_iomap_table;
> acc2364fe66106 Philipp Stanner 2024-01-31  871  
> 34e90b966504f3 Philipp Stanner 2024-04-03  872          ret =
> pcim_request_all_regions(pdev, name);
> 34e90b966504f3 Philipp Stanner 2024-04-03  873          if (ret != 0)
> 34e90b966504f3 Philipp Stanner 2024-04-03 
> 874                  return ret;
> acc2364fe66106 Philipp Stanner 2024-01-31  875  
> 34e90b966504f3 Philipp Stanner 2024-04-03  876          for (bar = 0;
> bar < PCI_STD_NUM_BARS; bar++) {
> 34e90b966504f3 Philipp Stanner 2024-04-03  877                  if
> (!mask_contains_bar(mask, bar))
> 34e90b966504f3 Philipp Stanner 2024-04-03 
> 878                          continue;
> 34e90b966504f3 Philipp Stanner 2024-04-03  879                  if
> (!pcim_iomap(pdev, bar, 0))
> 34e90b966504f3 Philipp Stanner 2024-04-03 
> 880                          goto err;
> 34e90b966504f3 Philipp Stanner 2024-04-03  881          }
> 34e90b966504f3 Philipp Stanner 2024-04-03  882  
> 34e90b966504f3 Philipp Stanner 2024-04-03  883          return 0;
> 34e90b966504f3 Philipp Stanner 2024-04-03  884  
> 34e90b966504f3 Philipp Stanner 2024-04-03  885  err:
> 34e90b966504f3 Philipp Stanner 2024-04-03  886          /*
> 34e90b966504f3 Philipp Stanner 2024-04-03  887           * Here it
> gets tricky: pcim_iomap() above has most likely
> 34e90b966504f3 Philipp Stanner 2024-04-03  888           * failed
> because it got an OOM when trying to create the
> 34e90b966504f3 Philipp Stanner 2024-04-03  889           * legacy-
> table.
> 34e90b966504f3 Philipp Stanner 2024-04-03  890           * We check
> here if that has happened. If not, pcim_iomap()
> 34e90b966504f3 Philipp Stanner 2024-04-03  891           * must have
> failed because of EINVAL.
> 34e90b966504f3 Philipp Stanner 2024-04-03  892           */
> 34e90b966504f3 Philipp Stanner 2024-04-03 
> 893          legacy_iomap_table = (void __iomem
> **)pcim_iomap_table(pdev);
> 34e90b966504f3 Philipp Stanner 2024-04-03 @894          ret =
> legacy_iomap_table ? -EINVAL : -ENOMEM;
>                                                              
> ^^^^^^^^^^^^^^^^^^
> Check for NULL
> 
> 34e90b966504f3 Philipp Stanner 2024-04-03  895  
> 34e90b966504f3 Philipp Stanner 2024-04-03  896          while (--bar
> >= 0)
> 34e90b966504f3 Philipp Stanner 2024-04-03
> @897                  pcim_iounmap(pdev, legacy_iomap_table[bar]);
>                                                                      
>               ^^^^^^^^^^^^^^^^^^
> Unchecked dereference

I think this is fine because `bar` can only be larger 0 if at least one
mapping has been created, thus, when it was possible to create
legacy_iomap_table, which is then valid.
So the second for-loop only executes when it's not NULL.

I guess we could silence the warning by doing

ret = bar > 0 ? -EINVAL : -ENOMEM;

Would even save one line of code


P.


> 
> 34e90b966504f3 Philipp Stanner 2024-04-03  898  
> 34e90b966504f3 Philipp Stanner 2024-04-03 
> 899          pcim_release_all_regions(pdev);
> 34e90b966504f3 Philipp Stanner 2024-04-03  900  
> 34e90b966504f3 Philipp Stanner 2024-04-03  901          return ret;
> acc2364fe66106 Philipp Stanner 2024-01-31  902  }
>