Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
From: Philipp Stanner
Date: Thu Apr 04 2024 - 03:16:22 EST
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@xxxxxxxxx/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@xxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Closes:
> > https://lore.kernel.org/r/202404040920.QIxhNeMu-lkp@xxxxxxxxx/
>
> 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 }
>