Re: [linux-next:master] BUILD REGRESSION 088b9c375534d905a4d337c78db3b3bfbb52c4a0
On Thu, Jul 07, 2022 at 07:02:58AM -0700, Guenter Roeck wrote: > and the NULL > dereferences in the binder driver are at the very least suspicious. The NULL dereferences in binder are just nonsense Sparse annotations. They don't affect runtime. drivers/android/binder.c:1481:19-23: ERROR: from is NULL but dereferenced. drivers/android/binder.c:2920:29-33: ERROR: target_thread is NULL but dereferenced. drivers/android/binder.c:353:25-35: ERROR: node -> proc is NULL but dereferenced. drivers/android/binder.c:4888:16-20: ERROR: t is NULL but dereferenced. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] scsi: core: Cap shost max_sectors according to DMA optimum mapping limits
Hi John,
url:
https://github.com/intel-lab-lkp/linux/commits/John-Garry/DMA-mapping-changes-for-SCSI-core/20220520-163049
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: s390-randconfig-m031-20220519
(https://download.01.org/0day-ci/archive/20220521/202205210545.gks834ds-...@intel.com/config)
compiler: s390-linux-gcc (GCC) 11.3.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/scsi/hosts.c:243 scsi_add_host_with_dma() warn: variable dereferenced
before check 'dma_dev' (see line 228)
vim +/dma_dev +243 drivers/scsi/hosts.c
d139b9bd0e52dd James Bottomley 2009-11-05 209 int
scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
d139b9bd0e52dd James Bottomley 2009-11-05 210
struct device *dma_dev)
^1da177e4c3f41 Linus Torvalds2005-04-16 211 {
^1da177e4c3f41 Linus Torvalds2005-04-16 212struct
scsi_host_template *sht = shost->hostt;
^1da177e4c3f41 Linus Torvalds2005-04-16 213int error = -EINVAL;
^1da177e4c3f41 Linus Torvalds2005-04-16 214
91921e016a2199 Hannes Reinecke 2014-06-25 215shost_printk(KERN_INFO,
shost, "%s\n",
^1da177e4c3f41 Linus Torvalds2005-04-16 216
sht->info ? sht->info(shost) : sht->name);
^1da177e4c3f41 Linus Torvalds2005-04-16 217
^1da177e4c3f41 Linus Torvalds2005-04-16 218if (!shost->can_queue) {
91921e016a2199 Hannes Reinecke 2014-06-25 219
shost_printk(KERN_ERR, shost,
91921e016a2199 Hannes Reinecke 2014-06-25 220
"can_queue = 0 no longer supported\n");
542bd1377a9630 James Bottomley 2008-04-21 221goto fail;
^1da177e4c3f41 Linus Torvalds2005-04-16 222}
^1da177e4c3f41 Linus Torvalds2005-04-16 223
50b6cb3516365c Dexuan Cui2021-10-07 224/* Use min_t(int, ...)
in case shost->can_queue exceeds SHRT_MAX */
50b6cb3516365c Dexuan Cui2021-10-07 225shost->cmd_per_lun =
min_t(int, shost->cmd_per_lun,
ea2f0f77538c50 John Garry2021-05-19 226
shost->can_queue);
ea2f0f77538c50 John Garry2021-05-19 227
2ad7ba6ca08593 John Garry2022-05-20 @228if (dma_dev->dma_mask) {
^
The patch adds a new unchecked dereference
2ad7ba6ca08593 John Garry2022-05-20 229
shost->max_sectors = min_t(unsigned int, shost->max_sectors,
2ad7ba6ca08593 John Garry2022-05-20 230
dma_opt_mapping_size(dma_dev) >> SECTOR_SHIFT);
2ad7ba6ca08593 John Garry2022-05-20 231}
2ad7ba6ca08593 John Garry2022-05-20 232
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03 233error =
scsi_init_sense_cache(shost);
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03 234if (error)
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03 235goto fail;
0a6ac4ee7c2109 Christoph Hellwig 2017-01-03 236
d285203cf647d7 Christoph Hellwig 2014-01-17 237error =
scsi_mq_setup_tags(shost);
542bd1377a9630 James Bottomley 2008-04-21 238if (error)
542bd1377a9630 James Bottomley 2008-04-21 239goto fail;
d285203cf647d7 Christoph Hellwig 2014-01-17 240
^1da177e4c3f41 Linus Torvalds2005-04-16 241if
(!shost->shost_gendev.parent)
^1da177e4c3f41 Linus Torvalds2005-04-16 242
shost->shost_gendev.parent = dev ? dev : &platform_bus;
3c8d9a957d0ae6 James Bottomley 2012-05-04 @243if (!dma_dev)
The old code checked for NULL
3c8d9a957d0ae6 James Bottomley 2012-05-04 244dma_dev =
shost->shost_gendev.parent;
3c8d9a957d0ae6 James Bottomley 2012-05-04 245
d139b9bd0e52dd James Bottomley 2009-11-05 246shost->dma_dev =
dma_dev;
^1da177e4c3f41 Linus Torvalds2005-04-16 247
5c6fab9d558470 Mika Westerberg 2016-02-18 248/*
5c6fab9d558470 Mika Westerberg 2016-02-18 249 * Increase usage count
temporarily here so that calling
5c6fab9d558470 Mika Westerberg 2016-02-18 250 *
scsi_autopm_put_host() will trigger runtime idle if there is
5c6fab9d558470 Mika Westerberg 2016-02-18 251 * nothing else
preventing suspending the device.
5c6fab9d558470 Mika Westerberg 2016-02-18 252 */
5c6fab9d558470 Mika Westerberg 2016-02-18 253
pm_runtime_get_noresume(&shost->shost_gendev);
bc4f24014de58f Alan Stern2010-06-17 254
pm_runtime_set_active(&shost->shost_gendev);
bc4f24014de58f Alan Stern2010-06-17 255
pm_runtime_enable(&shost->shost_gende
Re: [PATCH 1/4] iommu/mediatek: Use dev_err_probe to mute probe_defer err log
On Wed, May 11, 2022 at 02:49:17PM +0800, Yong Wu wrote:
> Fixes: d2e9a1102cfc ("iommu/mediatek: Contain MM IOMMU flow with the MM TYPE")
> Signed-off-by: Yong Wu
> ---
> The Fixes tag commit-id is from linux-next.
This is fine. The commit hash will not change unless the maintainer
rebases the tree.
When maintainers rebase their trees it's their responsibility to deal
with the Fixes tags. Often they just fold the fix into the original
commit so the issue is moot. Stephen Rothwell checks that Fixes tags
point to a valid commit and there are probably other people who have
checks for that as well.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Improve amd_iommu_v2_exit()
Hello Suravee Suthikulpanit,
The patch d46c04558fdd: "iommu/amd: Improve amd_iommu_v2_exit()" from
Mar 1, 2022, leads to the following Smatch static checker warning:
drivers/iommu/amd/iommu_v2.c:133 free_device_state()
warn: sleeping in atomic context
drivers/iommu/amd/iommu_v2.c
955static void __exit amd_iommu_v2_exit(void)
956{
957struct device_state *dev_state, *next;
958unsigned long flags;
959
960if (!amd_iommu_v2_supported())
961return;
962
963amd_iommu_unregister_ppr_notifier(&ppr_nb);
964
965flush_workqueue(iommu_wq);
966
967/*
968 * The loop below might call flush_workqueue(), so call
969 * destroy_workqueue() after it
970 */
971spin_lock_irqsave(&state_lock, flags);
Holding a spin lock.
972
973list_for_each_entry_safe(dev_state, next, &state_list,
list) {
974WARN_ON_ONCE(1);
975
976put_device_state(dev_state);
977list_del(&dev_state->list);
978free_device_state(dev_state);
Calling a sleeping function (wait_event()).
979}
980
981spin_unlock_irqrestore(&state_lock, flags);
982
983destroy_workqueue(iommu_wq);
984}
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Improve error handling for amd_iommu_init_pci
Hello Suravee Suthikulpanit,
The patch 06687a03805e: "iommu/amd: Improve error handling for
amd_iommu_init_pci" from Mar 1, 2022, leads to the following Smatch
static checker warning:
drivers/iommu/amd/init.c:1989 amd_iommu_init_pci()
warn: duplicate check 'ret' (previous on line 1978)
drivers/iommu/amd/init.c
1951 static int __init amd_iommu_init_pci(void)
1952 {
1953 struct amd_iommu *iommu;
1954 int ret;
1955
1956 for_each_iommu(iommu) {
1957 ret = iommu_init_pci(iommu);
1958 if (ret) {
1959 pr_err("IOMMU%d: Failed to initialize IOMMU
Hardware (error=%d)!\n",
1960iommu->index, ret);
1961 goto out;
1962 }
1963 /* Need to setup range after PCI init */
1964 iommu_set_cwwb_range(iommu);
1965 }
1966
1967 /*
1968 * Order is important here to make sure any unity map
requirements are
1969 * fulfilled. The unity mappings are created and written to
the device
1970 * table during the amd_iommu_init_api() call.
1971 *
1972 * After that we call init_device_table_dma() to make sure any
1973 * uninitialized DTE will block DMA, and in the end we flush
the caches
1974 * of all IOMMUs to make sure the changes to the device table
are
1975 * active.
1976 */
1977 ret = amd_iommu_init_api();
1978 if (ret) {
The patch moved the error handling up here
1979 pr_err("IOMMU: Failed to initialize IOMMU-API
interface (error=%d)!\n",
1980ret);
1981 goto out;
1982 }
1983
1984 init_device_table_dma();
1985
1986 for_each_iommu(iommu)
1987 iommu_flush_all_caches(iommu);
1988
--> 1989 if (!ret)
Where before we just checked for errors here. This condition is
impossible now.
1990 print_iommu_info();
1991
1992 out:
1993 return ret;
1994 }
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/vt-d: Fix unmap_pages support
Hello Alex Williamson,
The patch edad96db58d2: "iommu/vt-d: Fix unmap_pages support" from
Nov 22, 2021, leads to the following Smatch static checker warning:
drivers/iommu/intel/iommu.c:1369 dma_pte_clear_level()
error: uninitialized symbol 'level_pfn'.
drivers/iommu/intel/iommu.c
1330 static struct page *dma_pte_clear_level(struct dmar_domain *domain,
int level,
1331 struct dma_pte *pte, unsigned
long pfn,
1332 unsigned long start_pfn,
1333 unsigned long last_pfn,
1334 struct page *freelist)
1335 {
1336 struct dma_pte *first_pte = NULL, *last_pte = NULL;
1337
1338 pfn = max(start_pfn, pfn);
1339 pte = &pte[pfn_level_offset(pfn, level)];
1340
1341 do {
1342 unsigned long level_pfn;
1343
1344 if (!dma_pte_present(pte))
1345 goto next;
^^
If we ever hit this goto then there is going to be a bug.
1346
1347 level_pfn = pfn & level_mask(level);
1348
1349 /* If range covers entire pagetable, free it */
1350 if (start_pfn <= level_pfn &&
1351 last_pfn >= level_pfn + level_size(level) - 1) {
1352 /* These suborbinate page tables are going
away entirely. Don't
1353bother to clear them; we're just going to
*free* them. */
1354 if (level > 1 && !dma_pte_superpage(pte))
1355 freelist =
dma_pte_list_pagetables(domain, level - 1, pte, freelist);
1356
1357 dma_clear_pte(pte);
1358 if (!first_pte)
1359 first_pte = pte;
1360 last_pte = pte;
1361 } else if (level > 1) {
1362 /* Recurse down into a level that isn't
*entirely* obsolete */
1363 freelist = dma_pte_clear_level(domain, level -
1,
1364
phys_to_virt(dma_pte_addr(pte)),
1365level_pfn,
start_pfn, last_pfn,
1366freelist);
1367 }
1368 next:
--> 1369 pfn = level_pfn + level_size(level);
^
1370 } while (!first_pte_in_page(++pte) && pfn <= last_pfn);
1371
1372 if (first_pte)
1373 domain_flush_cache(domain, first_pte,
1374(void *)++last_pte - (void
*)first_pte);
1375
1376 return freelist;
1377 }
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 6/6] dma-iommu: Pass iova len for IOVA domain init
Hi John,
url:
https://github.com/0day-ci/linux/commits/John-Garry/iommu-Allow-IOVA-rcache-range-be-configured/20210714-184328
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: ia64-randconfig-m031-20210714 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/iommu/dma-iommu.c:384 iommu_dma_init_domain() warn: variable
dereferenced before check 'dev' (see line 374)
vim +/dev +384 drivers/iommu/dma-iommu.c
06d60728ff5c01 Christoph Hellwig 2019-05-20 332 static int
iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
ac6d704679d343 Jean-Philippe Brucker 2021-06-18 333
dma_addr_t limit, struct device *dev)
0db2e5d18f76a6 Robin Murphy 2015-10-01 334 {
fdbe574eb69312 Robin Murphy 2017-01-19 335struct iommu_dma_cookie
*cookie = domain->iova_cookie;
c61a4633a56aaa Shaokun Zhang 2019-01-24 336unsigned long order,
base_pfn;
6b0c54e7f27159 Yunsheng Lin 2019-08-24 337struct iova_domain
*iovad;
de4ba360c3e4ed John Garry2021-07-14 338size_t max_opt_dma_size;
de4ba360c3e4ed John Garry2021-07-14 339unsigned long iova_len
= 0;
0db2e5d18f76a6 Robin Murphy 2015-10-01 340
fdbe574eb69312 Robin Murphy 2017-01-19 341if (!cookie ||
cookie->type != IOMMU_DMA_IOVA_COOKIE)
fdbe574eb69312 Robin Murphy 2017-01-19 342return -EINVAL;
0db2e5d18f76a6 Robin Murphy 2015-10-01 343
6b0c54e7f27159 Yunsheng Lin 2019-08-24 344iovad = &cookie->iovad;
6b0c54e7f27159 Yunsheng Lin 2019-08-24 345
0db2e5d18f76a6 Robin Murphy 2015-10-01 346/* Use the smallest
supported page size for IOVA granularity */
d16e0faab911cc Robin Murphy 2016-04-07 347order =
__ffs(domain->pgsize_bitmap);
0db2e5d18f76a6 Robin Murphy 2015-10-01 348base_pfn =
max_t(unsigned long, 1, base >> order);
0db2e5d18f76a6 Robin Murphy 2015-10-01 349
0db2e5d18f76a6 Robin Murphy 2015-10-01 350/* Check the domain
allows at least some access to the device... */
0db2e5d18f76a6 Robin Murphy 2015-10-01 351if
(domain->geometry.force_aperture) {
0db2e5d18f76a6 Robin Murphy 2015-10-01 352if (base >
domain->geometry.aperture_end ||
ac6d704679d343 Jean-Philippe Brucker 2021-06-18 353limit <
domain->geometry.aperture_start) {
0db2e5d18f76a6 Robin Murphy 2015-10-01 354
pr_warn("specified DMA range outside IOMMU capability\n");
0db2e5d18f76a6 Robin Murphy 2015-10-01 355return
-EFAULT;
0db2e5d18f76a6 Robin Murphy 2015-10-01 356}
0db2e5d18f76a6 Robin Murphy 2015-10-01 357/* ...then
finally give it a kicking to make sure it fits */
0db2e5d18f76a6 Robin Murphy 2015-10-01 358base_pfn =
max_t(unsigned long, base_pfn,
0db2e5d18f76a6 Robin Murphy 2015-10-01 359
domain->geometry.aperture_start >> order);
0db2e5d18f76a6 Robin Murphy 2015-10-01 360}
0db2e5d18f76a6 Robin Murphy 2015-10-01 361
f51d7bb79c1124 Robin Murphy 2017-01-16 362/* start_pfn is always
nonzero for an already-initialised domain */
0db2e5d18f76a6 Robin Murphy 2015-10-01 363if (iovad->start_pfn) {
0db2e5d18f76a6 Robin Murphy 2015-10-01 364if (1UL <<
order != iovad->granule ||
f51d7bb79c1124 Robin Murphy 2017-01-16 365base_pfn !=
iovad->start_pfn) {
0db2e5d18f76a6 Robin Murphy 2015-10-01 366
pr_warn("Incompatible range for DMA domain\n");
0db2e5d18f76a6 Robin Murphy 2015-10-01 367return
-EFAULT;
0db2e5d18f76a6 Robin Murphy 2015-10-01 368}
7c1b058c8b5a31 Robin Murphy 2017-03-16 369
7c1b058c8b5a31 Robin Murphy 2017-03-16 370return 0;
0db2e5d18f76a6 Robin Murphy 2015-10-01 371}
7c1b058c8b5a31 Robin Murphy 2017-03-16 372
de4ba360c3e4ed John Garry2021-07-14 373
de4ba360c3e4ed John Garry2021-07-14 @374max_opt_dma_size =
iommu_group_get_max_opt_dma_size(dev->iommu_group);
New unchecked dereference
de4ba360c3e4ed John Garry2021-07-14 375if (max_opt_dma_size) {
de4ba360c3e4ed John Garry2021-07-14 376unsigned long
shift = __ffs(1UL << order);
de4ba360c3e4ed John Garry2021-07-14 377
d
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote:
>
> 在 2021/7/14 下午4:05, Dan Carpenter 写道:
> > On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote:
> > > 在 2021/7/13 下午7:31, Dan Carpenter 写道:
> > > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote:
> > > > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa
> > > > > *v, u64 iova, u64 size)
> > > > > }
> > > > >}
> > > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,
> > > > > -struct vhost_iotlb_msg *msg)
> > > > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
> > > > > + u64 iova, u64 size, u64 uaddr, u32 perm)
> > > > >{
> > > > > struct vhost_dev *dev = &v->vdev;
> > > > > - struct vhost_iotlb *iotlb = dev->iotlb;
> > > > > struct page **page_list;
> > > > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
> > > > > unsigned int gup_flags = FOLL_LONGTERM;
> > > > > unsigned long npages, cur_base, map_pfn, last_pfn = 0;
> > > > > unsigned long lock_limit, sz2pin, nchunks, i;
> > > > > - u64 iova = msg->iova;
> > > > > + u64 start = iova;
> > > > > long pinned;
> > > > > int ret = 0;
> > > > > - if (msg->iova < v->range.first ||
> > > > > - msg->iova + msg->size - 1 > v->range.last)
> > > > > - return -EINVAL;
> > > > This is not related to your patch, but can the "msg->iova + msg->size"
> > > > addition can have an integer overflow. From looking at the callers it
> > > > seems like it can. msg comes from:
> > > > vhost_chr_write_iter()
> > > > --> dev->msg_handler(dev, &msg);
> > > > --> vhost_vdpa_process_iotlb_msg()
> > > >--> vhost_vdpa_process_iotlb_update()
> > >
> > > Yes.
> > >
> > >
> > > > If I'm thinking of the right thing then these are allowed to overflow to
> > > > 0 because of the " - 1" but not further than that. I believe the check
> > > > needs to be something like:
> > > >
> > > > if (msg->iova < v->range.first ||
> > > > msg->iova - 1 > U64_MAX - msg->size ||
> > >
> > > I guess we don't need - 1 here?
> > The - 1 is important. The highest address is 0x. So it goes
> > start + size = 0 and then start + size - 1 == 0x.
>
>
> Right, so actually
>
> msg->iova = 0xfffe, msg->size=2 is valid.
I believe so, yes. It's inclusive of 0xfffe and 0x.
(Not an expert).
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote:
>
> 在 2021/7/13 下午7:31, Dan Carpenter 写道:
> > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote:
> > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v,
> > > u64 iova, u64 size)
> > > }
> > > }
> > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,
> > > -struct vhost_iotlb_msg *msg)
> > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
> > > + u64 iova, u64 size, u64 uaddr, u32 perm)
> > > {
> > > struct vhost_dev *dev = &v->vdev;
> > > - struct vhost_iotlb *iotlb = dev->iotlb;
> > > struct page **page_list;
> > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
> > > unsigned int gup_flags = FOLL_LONGTERM;
> > > unsigned long npages, cur_base, map_pfn, last_pfn = 0;
> > > unsigned long lock_limit, sz2pin, nchunks, i;
> > > - u64 iova = msg->iova;
> > > + u64 start = iova;
> > > long pinned;
> > > int ret = 0;
> > > - if (msg->iova < v->range.first ||
> > > - msg->iova + msg->size - 1 > v->range.last)
> > > - return -EINVAL;
> > This is not related to your patch, but can the "msg->iova + msg->size"
> > addition can have an integer overflow. From looking at the callers it
> > seems like it can. msg comes from:
> >vhost_chr_write_iter()
> >--> dev->msg_handler(dev, &msg);
> >--> vhost_vdpa_process_iotlb_msg()
> > --> vhost_vdpa_process_iotlb_update()
>
>
> Yes.
>
>
> >
> > If I'm thinking of the right thing then these are allowed to overflow to
> > 0 because of the " - 1" but not further than that. I believe the check
> > needs to be something like:
> >
> > if (msg->iova < v->range.first ||
> > msg->iova - 1 > U64_MAX - msg->size ||
>
>
> I guess we don't need - 1 here?
The - 1 is important. The highest address is 0x. So it goes
start + size = 0 and then start + size - 1 == 0x.
I guess we could move the - 1 to the other side?
msg->iova > U64_MAX - msg->size + 1 ||
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace
On Tue, Jul 13, 2021 at 04:46:55PM +0800, Xie Yongji wrote:
> +static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name)
> +{
> + struct vduse_vdpa *vdev;
> + int ret;
> +
> + if (dev->vdev)
> + return -EEXIST;
> +
> + vdev = vdpa_alloc_device(struct vduse_vdpa, vdpa, dev->dev,
> + &vduse_vdpa_config_ops, name, true);
> + if (!vdev)
> + return -ENOMEM;
This should be an IS_ERR() check instead of a NULL check.
The vdpa_alloc_device() macro is doing something very complicated but
I'm not sure what. It calls container_of() and that looks buggy until
you spot the BUILD_BUG_ON_ZERO() compile time assert which ensures that
the container_of() is a no-op.
Only one of the callers checks for error pointers correctly so maybe
it's too complicated or maybe there should be better documentation.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote:
> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64
> iova, u64 size)
> }
> }
>
> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,
> -struct vhost_iotlb_msg *msg)
> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
> + u64 iova, u64 size, u64 uaddr, u32 perm)
> {
> struct vhost_dev *dev = &v->vdev;
> - struct vhost_iotlb *iotlb = dev->iotlb;
> struct page **page_list;
> unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
> unsigned int gup_flags = FOLL_LONGTERM;
> unsigned long npages, cur_base, map_pfn, last_pfn = 0;
> unsigned long lock_limit, sz2pin, nchunks, i;
> - u64 iova = msg->iova;
> + u64 start = iova;
> long pinned;
> int ret = 0;
>
> - if (msg->iova < v->range.first ||
> - msg->iova + msg->size - 1 > v->range.last)
> - return -EINVAL;
This is not related to your patch, but can the "msg->iova + msg->size"
addition can have an integer overflow. From looking at the callers it
seems like it can. msg comes from:
vhost_chr_write_iter()
--> dev->msg_handler(dev, &msg);
--> vhost_vdpa_process_iotlb_msg()
--> vhost_vdpa_process_iotlb_update()
If I'm thinking of the right thing then these are allowed to overflow to
0 because of the " - 1" but not further than that. I believe the check
needs to be something like:
if (msg->iova < v->range.first ||
msg->iova - 1 > U64_MAX - msg->size ||
msg->iova + msg->size - 1 > v->range.last)
But writing integer overflow check correctly is notoriously difficult.
Do you think you could send a fix for that which is separate from the
patcheset? We'd want to backport it to stable.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 07/17] virtio: Don't set FAILED status bit on device index allocation failure
On Tue, Jul 13, 2021 at 04:46:46PM +0800, Xie Yongji wrote: > We don't need to set FAILED status bit on device index allocation > failure since the device initialization hasn't been started yet. The commit message should say what the effect of this change is to the user. Is this a bugfix? Will it have any effect on runtime at all? To me, hearing your thoughts on this is valuable even if you have to guess. "I noticed this mistake during review and I don't think it will affect runtime." regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
On Mon, Jul 05, 2021 at 12:21:38PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
> > On 2021-07-05 11:23, Dan Carpenter wrote:
> > > [ Ancient code, but the bug seems real enough still. -dan ]
> > >
> > > Hello Upinder Malhi,
> > >
> > > The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> > > driver" from Sep 10, 2013, leads to the following static checker
> > > warning:
> > >
> > > drivers/iommu/iommu.c:2482 iommu_map()
> > > warn: sleeping in atomic context
> > >
> > > drivers/infiniband/hw/usnic/usnic_uiom.c
> > > 244 static int usnic_uiom_map_sorted_intervals(struct list_head
> > > *intervals,
> > > 245 struct
> > > usnic_uiom_reg *uiomr)
> > >
> > > This function is always called from usnic_uiom_reg_get() which is holding
> > > spin_lock(&pd->lock); so it can't sleep.
> >
> > FWIW back in those days it wasn't really well defined whether iommu_map()
> > was callable from non-sleeping contexts (the arch/arm DMA API code relied on
> > it, for instance). It was only formalised 2 years ago by 781ca2de89ba
> > ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
> > might_sleep() check that's firing there. I guess these calls want to be
> > updated to iommu_map_atomic() now.
>
> Does this mean this driver doesn't work at all upstream? I would be
> quite interested to delete it.
It just means it hasn't been used with CONFIG_DEBUG_ATOMIC_SLEEP enabled
within the past two years.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] IB/usnic: Add Cisco VIC low-level hardware driver
*/ 320 chunk = list_first_entry(&chunk->list, 321 struct usnic_uiom_chunk, 322 list); 323 goto iter_chunk; 324 } 325 } 326 327 return 0; 328 329 err_out: 330 usnic_uiom_unmap_sorted_intervals(intervals, pd); 331 return err; 332 } regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/vt-d: Allocate/register iopf queue for sva devices
Hello Lu Baolu, This is a semi-automatic email about new static checker warnings. The patch 4c82b88696ac: "iommu/vt-d: Allocate/register iopf queue for sva devices" from Jun 10, 2021, leads to the following Smatch complaint: drivers/iommu/intel/iommu.c:5335 intel_iommu_enable_sva() warn: variable dereferenced before check 'info' (see line 5332) drivers/iommu/intel/iommu.c 5331 struct device_domain_info *info = get_domain_info(dev); 5332 struct intel_iommu *iommu = info->iommu; ^^^ Dereferenced 5333 int ret; 5334 5335 if (!info || !iommu || dmar_disabled) ^ Checked too late. 5336 return -EINVAL; 5337 regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 04/12] virtio-blk: Add validation for block size in config space
On Wed, May 19, 2021 at 09:39:20PM +0800, Yongji Xie wrote: > On Mon, May 17, 2021 at 5:56 PM Xie Yongji wrote: > > > > This ensures that we will not use an invalid block size > > in config space (might come from an untrusted device). I looked at if I should add this as an untrusted function so that Smatch could find these sorts of bugs but this is reading data from the host so there has to be some level of trust... I should add some more untrusted data kvm functions to Smatch. Right now I only have kvm_register_read() and I've added kvm_read_guest_virt() just now. > > > > Signed-off-by: Xie Yongji > > --- > > drivers/block/virtio_blk.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > index ebb4d3fe803f..c848aa36d49b 100644 > > --- a/drivers/block/virtio_blk.c > > +++ b/drivers/block/virtio_blk.c > > @@ -826,7 +826,7 @@ static int virtblk_probe(struct virtio_device *vdev) > > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, > >struct virtio_blk_config, blk_size, > >&blk_size); > > - if (!err) > > + if (!err && blk_size > 0 && blk_size <= max_size) > > The check here is incorrect. I will use PAGE_SIZE as the maximum > boundary in the new version. What does this bug look like to the user? A minimum block size of 1 seems pretty crazy. Surely the minimum should be higher? regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/vt-d: check for allocation failure in aux_detach_device()
In current kernels small allocations never fail, but checking for
allocation failure is the correct thing to do.
Fixes: 18abda7a2d55 ("iommu/vt-d: Fix general protection fault in
aux_detach_device()")
Signed-off-by: Dan Carpenter
---
drivers/iommu/intel/iommu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 708f430af1c4..9a7b79b5af18 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4606,6 +4606,8 @@ static int auxiliary_link_device(struct dmar_domain
*domain,
if (!sinfo) {
sinfo = kzalloc(sizeof(*sinfo), GFP_ATOMIC);
+ if (!sinfo)
+ return -ENOMEM;
sinfo->domain = domain;
sinfo->pdev = dev;
list_add(&sinfo->link_phys, &info->subdevices);
--
2.30.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC v1 PATCH 1/3] drivers: soc: add support for soc_device_match returning -EPROBE_DEFER
On Mon, Apr 19, 2021 at 10:20:13AM +0200, Geert Uytterhoeven wrote:
> Hi Alice,
>
> CC Arnd (soc_device_match() author)
>
> On Mon, Apr 19, 2021 at 6:28 AM Alice Guo (OSS) wrote:
> > From: Alice Guo
> >
> > In i.MX8M boards, the registration of SoC device is later than caam
> > driver which needs it. Caam driver needs soc_device_match to provide
> > -EPROBE_DEFER when no SoC device is registered and no
> > early_soc_dev_attr.
>
> I'm wondering if this is really a good idea: soc_device_match() is a
> last-resort low-level check, and IMHO should be made available early on,
> so there is no need for -EPROBE_DEFER.
>
> >
> > Signed-off-by: Alice Guo
>
> Thanks for your patch!
>
> > --- a/drivers/base/soc.c
> > +++ b/drivers/base/soc.c
> > @@ -110,6 +110,7 @@ static void soc_release(struct device *dev)
> > }
> >
> > static struct soc_device_attribute *early_soc_dev_attr;
> > +static bool soc_dev_attr_init_done = false;
>
> Do you need this variable?
>
> >
> > struct soc_device *soc_device_register(struct soc_device_attribute
> > *soc_dev_attr)
> > {
> > @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct
> > soc_device_attribute *soc_dev_attr
> > return ERR_PTR(ret);
> > }
> >
> > + soc_dev_attr_init_done = true;
> > return soc_dev;
> >
> > out3:
> > @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match(
> > if (!matches)
> > return NULL;
> >
> > + if (!soc_dev_attr_init_done && !early_soc_dev_attr)
>
> if (!soc_bus_type.p && !early_soc_dev_attr)
There is one place checking this already. We could wrap it in a helper
function:
static bool device_init_done(void)
{
return soc_bus_type.p ? true : false;
}
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH][next] iommu/mediatek: Fix unsigned domid comparison with less than zero
On Thu, Feb 04, 2021 at 09:25:58AM +, Will Deacon wrote:
> On Wed, Feb 03, 2021 at 01:59:36PM +, Colin King wrote:
> > From: Colin Ian King
> >
> > Currently the check for domid < 0 is always false because domid
> > is unsigned. Fix this by making it signed.
> >
> > Addresses-CoverityL ("Unsigned comparison against 0")
>
> Typo here ('L' instead of ':')
>
> > Fixes: ab1d5281a62b ("iommu/mediatek: Add iova reserved function")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/iommu/mtk_iommu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 0ad14a7604b1..823d719945b2 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device
> > *dev,
> >struct list_head *head)
> > {
> > struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
> > - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i;
> > + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i;
>
> Not sure if it's intentional, but this also makes 'i' signed. It probably
> should remain 'unsigned' to match 'iova_region_nr' in
> 'struct mtk_iommu_plat_data'.
iova_region_nr is either 1 or 5 so unsigned doesn't matter.
I once almost introduced a bug where the iterator was supposed to be
size_t. I fixed a bug by making it signed but I ended up introducing a
new bug. But generally that's pretty rare. The more common case is
that making iterators unsigned introduces bugs.
It's better to default to "int i;" and if more complicated types are
required that should stand out. "size_t pg_idx;" or whatever.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/mediatek: Fix error code in probe()
This error path is supposed to return -EINVAL. It used to return
directly but we added some clean up and accidentally removed the
error code. Also I fixed a typo in the error message.
Fixes: c0b57581b73b ("iommu/mediatek: Add power-domain operation")
Signed-off-by: Dan Carpenter
---
drivers/iommu/mtk_iommu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 0ad14a7604b1..5f78ac0dc30e 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -886,7 +886,8 @@ static int mtk_iommu_probe(struct platform_device *pdev)
link = device_link_add(data->smicomm_dev, dev,
DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
if (!link) {
- dev_err(dev, "Unable link %s.\n", dev_name(data->smicomm_dev));
+ dev_err(dev, "Unable to link %s.\n",
dev_name(data->smicomm_dev));
+ ret = -EINVAL;
goto out_runtime_disable;
}
--
2.30.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/3] iommu/vt-d: Fix ineffective devTLB invalidation for subdevices
Hi Liu,
url:
https://github.com/0day-ci/linux/commits/Liu-Yi-L/iommu-vt-d-Misc-fixes-on-scalable-mode/20201229-113203
base:5c8fe583cce542aa0b84adc939ce85293de36e5e
config: i386-randconfig-m021-20201229 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
New smatch warnings:
drivers/iommu/intel/iommu.c:1471 domain_update_iotlb() error: we previously
assumed 'info' could be null (see line 1472)
Old smatch warnings:
drivers/iommu/intel/iommu.c:920 device_to_iommu() error: we previously assumed
'pdev' could be null (see line 893)
drivers/iommu/intel/iommu.c:3764 intel_iommu_add() warn: should '(1 << sp)' be
a 64 bit type?
vim +/info +1471 drivers/iommu/intel/iommu.c
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1464 static
void domain_update_iotlb(struct dmar_domain *domain)
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1465 {
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1466
struct device_domain_info *info;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1467
bool has_iotlb_device = false;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1468
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1469
assert_spin_locked(&device_domain_lock);
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1470
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 @1471
list_for_each_entry(info, &domain->devices, link)
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 @1472
if (info && info->ats_enabled) {
"info" is the list iterator so it can't ever be NULL. Just delete the
NULL check.
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1473
has_iotlb_device = true;
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1474
break;
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1475
}
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1476
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1477
if (!has_iotlb_device) {
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1478
struct subdev_domain_info *sinfo;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1479
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1480
list_for_each_entry(sinfo, &domain->subdevices, link_domain) {
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1481
info = get_domain_info(sinfo->pdev);
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1482
if (info && info->ats_enabled) {
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1483
has_iotlb_device = true;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1484
break;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1485
}
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1486
}
1aaf68f8927fb5c drivers/iommu/intel/iommu.c Liu Yi L 2020-12-29 1487
}
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1488
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1489
domain->has_iotlb_device = has_iotlb_device;
0824c5920b16fe1 drivers/iommu/intel-iommu.c Omer Peleg 2016-04-20 1490 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[kbuild] Re: [PATCH 1/7] vfio: iommu_type1: Clear added dirty bit when unwind pin
Hi Keqian,
url:
https://github.com/0day-ci/linux/commits/Keqian-Zhu/vfio-iommu_type1-Some-fixes-and-optimization/20201210-154322
base: https://github.com/awilliam/linux-vfio.git next
config: x86_64-randconfig-m001-20201215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/vfio/vfio_iommu_type1.c:648 vfio_iommu_type1_pin_pages() warn: variable
dereferenced before check 'iommu' (see line 640)
vim +/iommu +648 drivers/vfio/vfio_iommu_type1.c
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 631 static int
vfio_iommu_type1_pin_pages(void *iommu_data,
95fc87b44104d9a Kirti Wankhede 2020-05-29 632
struct iommu_group *iommu_group,
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 633
unsigned long *user_pfn,
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 634
int npage, int prot,
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 635
unsigned long *phys_pfn)
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 636 {
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 637 struct vfio_iommu
*iommu = iommu_data;
95fc87b44104d9a Kirti Wankhede 2020-05-29 638 struct vfio_group
*group;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 639 int i, j, ret;
2b172c0ea2a6daf Keqian Zhu 2020-12-10 @640 unsigned long pgshift =
__ffs(iommu->pgsize_bitmap);
The patch introduces a new dereference.
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 641 unsigned long
remote_vaddr;
2b172c0ea2a6daf Keqian Zhu 2020-12-10 642 unsigned long
bitmap_offset;
2b172c0ea2a6daf Keqian Zhu 2020-12-10 643 unsigned long
*bitmap_added;
2b172c0ea2a6daf Keqian Zhu 2020-12-10 644 dma_addr_t iova;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 645 struct vfio_dma *dma;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 646 bool do_accounting;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 647
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 @648 if (!iommu || !user_pfn
|| !phys_pfn)
^^
Checked too late.
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 649 return -EINVAL;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 650
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 651 /* Supported for v2
version only */
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 652 if (!iommu->v2)
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 653 return -EACCES;
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 654
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 655
mutex_lock(&iommu->lock);
a54eb55045ae9b3 Kirti Wankhede 2016-11-17 656
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
kbuild mailing list -- kbu...@lists.01.org
To unsubscribe send an email to kbuild-le...@lists.01.org
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [bug report] dma-mapping: add benchmark support for streaming DMA APIs
On Wed, Dec 09, 2020 at 10:01:49AM +, Song Bao Hua (Barry Song) wrote:
>
>
> > -Original Message-
> > From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> > Sent: Wednesday, December 9, 2020 8:00 PM
> > To: Song Bao Hua (Barry Song)
> > Cc: iommu@lists.linux-foundation.org
> > Subject: [bug report] dma-mapping: add benchmark support for streaming DMA
> > APIs
> >
> > Hello Barry Song,
> >
> > The patch 65789daa8087: "dma-mapping: add benchmark support for
> > streaming DMA APIs" from Nov 16, 2020, leads to the following static
> > checker warning:
> >
> > kernel/dma/map_benchmark.c:241 map_benchmark_ioctl()
> > error: undefined (user controlled) shift '1 << (map->bparam.dma_bits)'
> >
> > kernel/dma/map_benchmark.c
> >191 static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
> >192 unsigned long arg)
> >193 {
> >194 struct map_benchmark_data *map = file->private_data;
> >195 void __user *argp = (void __user *)arg;
> >196 u64 old_dma_mask;
> >197
> >198 int ret;
> >199
> >200 if (copy_from_user(&map->bparam, argp, sizeof(map->bparam)))
> >^
> > Comes from the user
> >
> >201 return -EFAULT;
> >202
> >203 switch (cmd) {
> >204 case DMA_MAP_BENCHMARK:
> >205 if (map->bparam.threads == 0 ||
> >206 map->bparam.threads > DMA_MAP_MAX_THREADS) {
> >207 pr_err("invalid thread number\n");
> >208 return -EINVAL;
> >209 }
> >210
> >211 if (map->bparam.seconds == 0 ||
> >212 map->bparam.seconds > DMA_MAP_MAX_SECONDS) {
> >213 pr_err("invalid duration seconds\n");
> >214 return -EINVAL;
> >215 }
> >216
> >217 if (map->bparam.node != NUMA_NO_NODE &&
> >218 !node_possible(map->bparam.node)) {
> >219 pr_err("invalid numa node\n");
> >220 return -EINVAL;
> >221 }
> >222
> >223 switch (map->bparam.dma_dir) {
> >224 case DMA_MAP_BIDIRECTIONAL:
> >225 map->dir = DMA_BIDIRECTIONAL;
> >226 break;
> >227 case DMA_MAP_FROM_DEVICE:
> >228 map->dir = DMA_FROM_DEVICE;
> >229 break;
> >230 case DMA_MAP_TO_DEVICE:
> >231 map->dir = DMA_TO_DEVICE;
> >232 break;
> >233 default:
> >234 pr_err("invalid DMA direction\n");
> >235 return -EINVAL;
> >236 }
> >237
> >238 old_dma_mask = dma_get_mask(map->dev);
> >239
> >240 ret = dma_set_mask(map->dev,
> >241
> > DMA_BIT_MASK(map->bparam.dma_bits));
> >
> > ^^
> > If this is more than 31 then the behavior is undefined (but in real life
> > it will shift wrap).
>
> Guess it should be less than 64?
> For 64, it would be ~0ULL, otherwise, it will be 1ULL< 64 is undefined, not 31 as I said.
>
> In test app,
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=7679325702
>
>
> I have some code like:
> + /* suppose the mininum DMA zone is 1MB in the world */
> + if (bits < 20 || bits > 64) {
> + fprintf(stderr, "invalid dma mask bit, must be in 20-64\n");
> + exit(1);
> + }
>
> Maybe I should do the same thing in kernel as well.
Sounds good!
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] dma-mapping: add benchmark support for streaming DMA APIs
Hello Barry Song,
The patch 65789daa8087: "dma-mapping: add benchmark support for
streaming DMA APIs" from Nov 16, 2020, leads to the following static
checker warning:
kernel/dma/map_benchmark.c:241 map_benchmark_ioctl()
error: undefined (user controlled) shift '1 << (map->bparam.dma_bits)'
kernel/dma/map_benchmark.c
191 static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
192 unsigned long arg)
193 {
194 struct map_benchmark_data *map = file->private_data;
195 void __user *argp = (void __user *)arg;
196 u64 old_dma_mask;
197
198 int ret;
199
200 if (copy_from_user(&map->bparam, argp, sizeof(map->bparam)))
^
Comes from the user
201 return -EFAULT;
202
203 switch (cmd) {
204 case DMA_MAP_BENCHMARK:
205 if (map->bparam.threads == 0 ||
206 map->bparam.threads > DMA_MAP_MAX_THREADS) {
207 pr_err("invalid thread number\n");
208 return -EINVAL;
209 }
210
211 if (map->bparam.seconds == 0 ||
212 map->bparam.seconds > DMA_MAP_MAX_SECONDS) {
213 pr_err("invalid duration seconds\n");
214 return -EINVAL;
215 }
216
217 if (map->bparam.node != NUMA_NO_NODE &&
218 !node_possible(map->bparam.node)) {
219 pr_err("invalid numa node\n");
220 return -EINVAL;
221 }
222
223 switch (map->bparam.dma_dir) {
224 case DMA_MAP_BIDIRECTIONAL:
225 map->dir = DMA_BIDIRECTIONAL;
226 break;
227 case DMA_MAP_FROM_DEVICE:
228 map->dir = DMA_FROM_DEVICE;
229 break;
230 case DMA_MAP_TO_DEVICE:
231 map->dir = DMA_TO_DEVICE;
232 break;
233 default:
234 pr_err("invalid DMA direction\n");
235 return -EINVAL;
236 }
237
238 old_dma_mask = dma_get_mask(map->dev);
239
240 ret = dma_set_mask(map->dev,
241 DMA_BIT_MASK(map->bparam.dma_bits));
^^
If this is more than 31 then the behavior is undefined (but in real life
it will shift wrap).
242 if (ret) {
243 pr_err("failed to set dma_mask on device %s\n",
244 dev_name(map->dev));
245 return -EINVAL;
246 }
247
248 ret = do_map_benchmark(map);
249
250 /*
251 * restore the original dma_mask as many devices'
dma_mask are
252 * set by architectures, acpi, busses. When we bind
them back
253 * to their original drivers, those drivers shouldn't
see
254 * dma_mask changed by benchmark
255 */
256 dma_set_mask(map->dev, old_dma_mask);
257 break;
258 default:
259 return -EINVAL;
260 }
261
262 if (copy_to_user(argp, &map->bparam, sizeof(map->bparam)))
263 return -EFAULT;
264
265 return ret;
266 }
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu: fix a check in iommu_check_bind_data()
The "data->flags" variable is a u64 so if one of the high 32 bits is
set the original code will allow it, but it should be rejected. The
fix is to declare "mask" as a u64 instead of a u32.
Fixes: d90573812eea ("iommu/uapi: Handle data and argsz filled by users")
Signed-off-by: Dan Carpenter
---
drivers/iommu/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c470f451a32..b53446bb8c6b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2071,7 +2071,7 @@ EXPORT_SYMBOL_GPL(iommu_uapi_cache_invalidate);
static int iommu_check_bind_data(struct iommu_gpasid_bind_data *data)
{
- u32 mask;
+ u64 mask;
int i;
if (data->version != IOMMU_GPASID_BIND_VERSION_1)
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Restore IRTE.RemapEn bit after programming IRTE
Hello Suravee Suthikulpanit, This is a semi-automatic email about new static checker warnings. The patch 26e495f34107: "iommu/amd: Restore IRTE.RemapEn bit after programming IRTE" from Sep 3, 2020, leads to the following Smatch complaint: drivers/iommu/amd/iommu.c:3870 amd_iommu_deactivate_guest_mode() warn: variable dereferenced before check 'entry' (see line 3867) drivers/iommu/amd/iommu.c 3866 struct irq_cfg *cfg = ir_data->cfg; 3867 u64 valid = entry->lo.fields_remap.valid; The patch adds a new dereference 3868 3869 if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || 3870 !entry || !entry->lo.fields_vapic.guest_mode) ^^ before "entry" has been checked for NULL. 3871 return 0; 3872 regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] dma-pool: Fix an uninitialized variable bug in atomic_pool_expand()
The "page" pointer can be used with out being initialized.
Fixes: d7e673ec2c8e ("dma-pool: Only allocate from CMA when in same memory
zone")
Signed-off-by: Dan Carpenter
---
kernel/dma/pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 06582b488e31..1281c0f0442b 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -84,7 +84,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t
pool_size,
gfp_t gfp)
{
unsigned int order;
- struct page *page;
+ struct page *page = NULL;
void *addr;
int ret = -ENOMEM;
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/vt-d:Add support for probing ACPI device in RMRR
On Wed, Aug 26, 2020 at 06:27:51AM -0400, FelixCuioc wrote:
> After acpi device in RMRR is detected,it is necessary
> to establish a mapping for these devices.
> In acpi_device_create_direct_mappings(),create a mapping
> for the acpi device in RMRR.
> Add a helper to achieve the acpi namespace device can
> access the RMRR region.
>
> Signed-off-by: FelixCuioc
> ---
> drivers/iommu/intel/iommu.c | 27 +++
> drivers/iommu/iommu.c | 6 ++
> include/linux/iommu.h | 3 +++
> 3 files changed, 36 insertions(+)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index f774ef63d473..b31f02f41c96 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4797,6 +4797,20 @@ static int __init platform_optin_force_iommu(void)
>
> return 1;
> }
> +static int acpi_device_create_direct_mappings(struct device *pn_dev, struct
> device *acpi_device)
Blank line.
> +{
> + struct iommu_group *group;
> +
> + acpi_device->bus->iommu_ops = &intel_iommu_ops;
> + group = iommu_group_get(pn_dev);
> + if (!group) {
> + pr_warn("ACPI name space devices create direct mappings
> wrong!\n");
> + return -1;
Use a proper error code. -ENOMEM? -EINVAL?
> + }
> + __acpi_device_create_direct_mappings(group, acpi_device);
> +
> + return 0;
> +}
>
> static int __init probe_acpi_namespace_devices(void)
> {
> @@ -4812,6 +4826,7 @@ static int __init probe_acpi_namespace_devices(void)
> struct acpi_device_physical_node *pn;
> struct iommu_group *group;
> struct acpi_device *adev;
> + struct device *pn_dev = NULL;
>
> if (dev->bus != &acpi_bus_type)
> continue;
> @@ -4822,6 +4837,7 @@ static int __init probe_acpi_namespace_devices(void)
> &adev->physical_node_list, node) {
> group = iommu_group_get(pn->dev);
> if (group) {
> + pn_dev = pn->dev;
> iommu_group_put(group);
> continue;
> }
> @@ -4830,7 +4846,18 @@ static int __init probe_acpi_namespace_devices(void)
> ret = iommu_probe_device(pn->dev);
> if (ret)
> break;
> + pn_dev = pn->dev;
> + }
> + if (pn_dev == NULL) {
Run checkpatch.pl --strict on this patch. Use "if (!pn_dev) {".
> + dev->bus->iommu_ops = &intel_iommu_ops;
> + ret = iommu_probe_device(dev);
> + if (ret) {
> + pr_err("acpi_device probe fail!
> ret:%d\n", ret);
> + return ret;
^^
This should be goto unlock;
> + }
> + return 0;
> }
> + ret = acpi_device_create_direct_mappings(pn_dev, dev);
unlock:
> mutex_unlock(&adev->physical_node_lock);
^^^
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/3] iommu/vt-d:Add mutex_unlock() before returning
On Wed, Aug 26, 2020 at 06:27:52AM -0400, FelixCuioc wrote: > In the probe_acpi_namespace_devices function,when the physical > node of the acpi device is NULL,the unlock function is missing. > Add mutex_unlock(&adev->physical_node_lock). > > Reported-by: Dan Carpenter > Signed-off-by: FelixCuioc Oh... Crap. I wondered why I was being CC'd on this patchset. Just fold this into the ealier patch. Don't worry about the Reported-by. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] iommu/vt-d:Add support for detecting ACPI device in RMRR
gt; - for (scope = (void *)(drhd + 1);
> - (unsigned long)scope < ((unsigned long)drhd) +
> drhd->header.length;
> - scope = ((void *)scope) + scope->length) {
> - if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
> - continue;
> - if (scope->enumeration_id != device_number)
> - continue;
> + return ret;
>
> - path = (void *)(scope + 1);
> - pr_info("ACPI device \"%s\" under DMAR at %llx as
> %02x:%02x.%d\n",
> - dev_name(&adev->dev), dmaru->reg_base_addr,
> - scope->bus, path->device, path->function);
> - for_each_dev_scope(dmaru->devices, dmaru->devices_cnt,
> i, tmp)
> - if (tmp == NULL) {
> - dmaru->devices[i].bus = scope->bus;
> - dmaru->devices[i].devfn =
> PCI_DEVFN(path->device,
> -
> path->function);
> -
> rcu_assign_pointer(dmaru->devices[i].dev,
> -
> get_device(&adev->dev));
> - return;
> - }
> - BUG_ON(i >= dmaru->devices_cnt);
> - }
> - }
> - pr_warn("No IOMMU scope found for ANDD enumeration ID %d (%s)\n",
> - device_number, dev_name(&adev->dev));
> }
>
> static int __init dmar_acpi_dev_scope_init(void)
> @@ -766,7 +774,7 @@ static int __init dmar_acpi_dev_scope_init(void)
> andd->device_name);
> continue;
> }
> - dmar_acpi_insert_dev_scope(andd->device_number, adev);
> + dmar_acpi_bus_add_dev(andd->device_number, adev);
> }
> }
> return 0;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index ca557d351518..f774ef63d473 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4507,6 +4507,24 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
>
> return ret;
> }
> +int dmar_rmrr_add_acpi_dev(u8 device_number, struct acpi_device *adev)
Please add a blank line between functions.
> +{
> + int ret;
> + struct dmar_rmrr_unit *rmrru;
> + struct acpi_dmar_reserved_memory *rmrr;
> +
> + list_for_each_entry(rmrru, &dmar_rmrr_units, list) {
> + rmrr = container_of(rmrru->hdr,
> + struct acpi_dmar_reserved_memory,
> + header);
> + ret = dmar_acpi_insert_dev_scope(device_number, adev, (void
> *)(rmrr + 1),
> + ((void *)rmrr) +
> rmrr->header.length,
> + rmrru->devices,
> rmrru->devices_cnt);
> + if (ret)
> + break;
> + }
> + return 0;
> +}
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d:Add support for probing ACPI device in RMRR
Hi FelixCuioc,
Thank you for the patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/FelixCuioc/iommu-vt-d-Add-support-for-probing-ACPI-device-in-RMRR/20200818-104920
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-m001-20200820 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
New smatch warnings:
drivers/iommu/intel/iommu.c:4850 probe_acpi_namespace_devices() warn:
inconsistent returns 'adev->physical_node_lock'.
Old smatch warnings:
drivers/iommu/intel/iommu.c:836 device_to_iommu() error: we previously assumed
'pdev' could be null (see line 809)
drivers/iommu/intel/iommu.c:2272 __domain_mapping() error: we previously
assumed 'sg' could be null (see line 2263)
drivers/iommu/intel/iommu.c:4371 intel_iommu_add() warn: should '(1 << sp)' be
a 64 bit type?
#
https://github.com/0day-ci/linux/commit/3adfd53a32c52833a34033fabaa5c1e65dfca688
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
FelixCuioc/iommu-vt-d-Add-support-for-probing-ACPI-device-in-RMRR/20200818-104920
git checkout 3adfd53a32c52833a34033fabaa5c1e65dfca688
vim +4850 drivers/iommu/intel/iommu.c
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4797 static
int __init probe_acpi_namespace_devices(void)
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4798 {
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4799
struct dmar_drhd_unit *drhd;
af88ec3962010e3 drivers/iommu/intel-iommu.c Qian Cai 2019-06-03 4800
/* To avoid a -Wunused-but-set-variable warning. */
af88ec3962010e3 drivers/iommu/intel-iommu.c Qian Cai 2019-06-03 4801
struct intel_iommu *iommu __maybe_unused;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4802
struct device *dev;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4803
int i, ret = 0;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4804
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4805
for_each_active_iommu(iommu, drhd) {
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4806
for_each_active_dev_scope(drhd->devices,
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4807
drhd->devices_cnt, i, dev) {
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4808
struct acpi_device_physical_node *pn;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4809
struct iommu_group *group;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4810
struct acpi_device *adev;
3adfd53a32c5283 drivers/iommu/intel/iommu.c FelixCuioc 2020-08-17 4811
struct device *pn_dev = NULL;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4812
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4813
if (dev->bus != &acpi_bus_type)
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4814
continue;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4815
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4816
adev = to_acpi_device(dev);
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4817
mutex_lock(&adev->physical_node_lock);
^
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4818
list_for_each_entry(pn,
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4819
&adev->physical_node_list, node) {
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4820
group = iommu_group_get(pn->dev);
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4821
if (group) {
3adfd53a32c5283 drivers/iommu/intel/iommu.c FelixCuioc 2020-08-17 4822
pn_dev = pn->dev;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4823
iommu_group_put(group);
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4824
continue;
fa212a97f3a366a drivers/iommu/intel-iommu.c Lu Baolu 2019-05-25 4825
Re: Passing NULL dev to dma_alloc_coherent() allowed or not?
On Sat, Jun 27, 2020 at 01:45:16PM +0200, Richard Weinberger wrote:
> Hi!
>
> While porting on an old out-of-tree driver I noticed that dma_alloc_coherent()
> was used with dev being NULL.
>
> commit 148a97d5a02a62f81b5d6176f871c94a65e1f3af
> Author: Dan Carpenter
> Date: Wed Apr 24 17:24:37 2019 +0300
>
> dma-mapping: remove an unnecessary NULL check
>
> We already dereferenced "dev" when we called get_dma_ops() so this NULL
> check is too late. We're not supposed to pass NULL "dev" pointers to
> dma_alloc_attrs().
>
> Signed-off-by: Dan Carpenter
> Signed-off-by: Christoph Hellwig
>
> says that dma_alloc_attrs() with dev being NULL is not allowed, but in
> include/linux/dma-mapping.h we have:
>
> static inline void *dma_alloc_coherent(struct device *dev, size_t size,
> dma_addr_t *dma_handle, gfp_t gfp)
> {
>
> return dma_alloc_attrs(dev, size, dma_handle, gfp,
> (gfp & __GFP_NOWARN) ? DMA_ATTR_NO_WARN : 0);
> }
>
> In Linus' tree I see at least three callers of dma_alloc_coherent() with a
> NULL device.
> drivers/staging/emxx_udc/emxx_udc.c:2596: ep->virt_buf
> = dma_alloc_coherent(NULL, PAGE_SIZE,
> drivers/tty/synclink.c:3667:info->buffer_list =
> dma_alloc_coherent(NULL, BUFFERLISTSIZE, &info->buffer_list_dma_addr,
> GFP_KERNEL);
> drivers/tty/synclink.c:3777:BufferList[i].virt_addr =
> dma_alloc_coherent(NULL, DMABUFFERSIZE, &BufferList[i].dma_addr, GFP_KERNEL);
>
> I think these callers are wrong.
> Can you please clarify?
The are wrong. It was slightly worse when I originally sent the patch
but we fixed comedi.
https://lkml.org/lkml/2019/4/24/668
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[kbuild] Re: [PATCH v7 33/36] rapidio: fix common struct sg_table related issues
Hi Marek,
url:
https://github.com/0day-ci/linux/commits/Marek-Szyprowski/DRM-fix-struct-sg_table-nents-vs-orig_nents-misuse/20200619-184302
base:ce2cc8efd7a40cbd17841add878cb691d0ce0bba
config: i386-randconfig-m021-20200623 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/rapidio/devices/rio_mport_cdev.c:939 rio_dma_transfer() error:
uninitialized symbol 'nents'.
#
https://github.com/0day-ci/linux/commit/c99597eab54307f46248273962da0c23a9a88b76
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c99597eab54307f46248273962da0c23a9a88b76
vim +/nents +939 drivers/rapidio/devices/rio_mport_cdev.c
e8de370188d098 Alexandre Bounine 2016-03-22 804 static int
4e1016dac1ccce Alexandre Bounine 2016-05-05 805 rio_dma_transfer(struct file
*filp, u32 transfer_mode,
e8de370188d098 Alexandre Bounine 2016-03-22 806enum
rio_transfer_sync sync, enum dma_data_direction dir,
e8de370188d098 Alexandre Bounine 2016-03-22 807struct
rio_transfer_io *xfer)
e8de370188d098 Alexandre Bounine 2016-03-22 808 {
e8de370188d098 Alexandre Bounine 2016-03-22 809 struct mport_cdev_priv
*priv = filp->private_data;
e8de370188d098 Alexandre Bounine 2016-03-22 810 unsigned long nr_pages
= 0;
e8de370188d098 Alexandre Bounine 2016-03-22 811 struct page **page_list
= NULL;
e8de370188d098 Alexandre Bounine 2016-03-22 812 struct mport_dma_req
*req;
e8de370188d098 Alexandre Bounine 2016-03-22 813 struct mport_dev *md =
priv->md;
e8de370188d098 Alexandre Bounine 2016-03-22 814 struct dma_chan *chan;
67446283d89467 John Hubbard 2020-06-04 815 int ret;
e8de370188d098 Alexandre Bounine 2016-03-22 816 int nents;
^
e8de370188d098 Alexandre Bounine 2016-03-22 817
e8de370188d098 Alexandre Bounine 2016-03-22 818 if (xfer->length == 0)
e8de370188d098 Alexandre Bounine 2016-03-22 819 return -EINVAL;
e8de370188d098 Alexandre Bounine 2016-03-22 820 req =
kzalloc(sizeof(*req), GFP_KERNEL);
e8de370188d098 Alexandre Bounine 2016-03-22 821 if (!req)
e8de370188d098 Alexandre Bounine 2016-03-22 822 return -ENOMEM;
e8de370188d098 Alexandre Bounine 2016-03-22 823
e8de370188d098 Alexandre Bounine 2016-03-22 824 ret =
get_dma_channel(priv);
e8de370188d098 Alexandre Bounine 2016-03-22 825 if (ret) {
e8de370188d098 Alexandre Bounine 2016-03-22 826 kfree(req);
e8de370188d098 Alexandre Bounine 2016-03-22 827 return ret;
e8de370188d098 Alexandre Bounine 2016-03-22 828 }
c5157b76869ba9 Ioan Nicu 2018-04-20 829 chan = priv->dmach;
c5157b76869ba9 Ioan Nicu 2018-04-20 830
c5157b76869ba9 Ioan Nicu 2018-04-20 831
kref_init(&req->refcount);
c5157b76869ba9 Ioan Nicu 2018-04-20 832
init_completion(&req->req_comp);
c5157b76869ba9 Ioan Nicu 2018-04-20 833 req->dir = dir;
c5157b76869ba9 Ioan Nicu 2018-04-20 834 req->filp = filp;
c5157b76869ba9 Ioan Nicu 2018-04-20 835 req->priv = priv;
c5157b76869ba9 Ioan Nicu 2018-04-20 836 req->dmach = chan;
c5157b76869ba9 Ioan Nicu 2018-04-20 837 req->sync = sync;
e8de370188d098 Alexandre Bounine 2016-03-22 838
e8de370188d098 Alexandre Bounine 2016-03-22 839 /*
e8de370188d098 Alexandre Bounine 2016-03-22 840* If parameter
loc_addr != NULL, we are transferring data from/to
e8de370188d098 Alexandre Bounine 2016-03-22 841* data buffer
allocated in user-space: lock in memory user-space
e8de370188d098 Alexandre Bounine 2016-03-22 842* buffer pages and
build an SG table for DMA transfer request
e8de370188d098 Alexandre Bounine 2016-03-22 843*
e8de370188d098 Alexandre Bounine 2016-03-22 844* Otherwise (loc_addr
== NULL) contiguous kernel-space buffer is
e8de370188d098 Alexandre Bounine 2016-03-22 845* used for DMA data
transfers: build single entry SG table using
e8de370188d098 Alexandre Bounine 2016-03-22 846* offset within the
internal buffer specified by handle parameter.
e8de370188d098 Alexandre Bounine 2016-03-22 847*/
e8de370188d098 Alexandre Bounine 2016-03-22 848 if (xfer->loc_addr) {
c4860ad6056483 Tvrtko Ursulin 2017-07-31 849 unsigned int
offset;
e8de370188d098 Alexandre Bounine 2016-03-22 850 long pinned;
e8de370188d098 Alexandre Bounine 2016-03-22 851
c4860ad6056483 Tvrtko Ursulin 2017-07-31 852 offset =
lower_32_bits(offset_in_page(xfer->loc_
Re: [PATCH 1/3] dma-direct: provide the ability to reserve per-numa CMA
On Fri, Jun 05, 2020 at 06:04:31AM +, Song Bao Hua (Barry Song) wrote: > > > > -Original Message- > > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > > Sent: Thursday, June 4, 2020 11:37 PM > > To: kbu...@lists.01.org; Song Bao Hua (Barry Song) > > ; h...@lst.de; m.szyprow...@samsung.com; > > robin.mur...@arm.com; catalin.mari...@arm.com > > Cc: l...@intel.com; Dan Carpenter ; > > kbuild-...@lists.01.org; iommu@lists.linux-foundation.org; > > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org; Linuxarm > > ; Jonathan Cameron > > ; John Garry > > Subject: Re: [PATCH 1/3] dma-direct: provide the ability to reserve per-numa > > CMA > > > > Hi Barry, > > > > url: > > https://github.com/0day-ci/linux/commits/Barry-Song/support-per-numa-CM > > A-for-ARM-server/20200603-104821 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git > > for-next/core > > config: x86_64-randconfig-m001-20200603 (attached as .config) > > compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot > > Reported-by: Dan Carpenter > > Dan, thanks! Good catch! > as this patch has not been in mainline yet, is it correct to add these > "reported-by" in patch v2? These are just an automated email from the zero day bot. I look over the Smatch warnings and then forward them on. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets
On Thu, Jun 04, 2020 at 09:48:49AM -0400, Jim Quinlan wrote: > > > + r = devm_kcalloc(dev, 1, sizeof(struct dma_pfn_offset_region), > > > + GFP_KERNEL); > > > > Use:r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL); > Will fix. > > > > > > > > + if (!r) > > > + return -ENOMEM; > > > + > > > + r->uniform_offset = true; > > > + r->pfn_offset = pfn_offset; > > > + > > > + return 0; > > > +} > > > > This function doesn't seem to do anything useful. Is part of it > > missing? > No, the uniform pfn offset is a special case. Sorry, I wasn't clear. We're talking about different things. The code does: r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL); if (!r) return -ENOMEM; r->uniform_offset = true; r->pfn_offset = pfn_offset; return 0; The code allocates "r" and then doesn't save it anywhere so there is no point. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] dma-direct: provide the ability to reserve per-numa CMA
Hi Barry,
url:
https://github.com/0day-ci/linux/commits/Barry-Song/support-per-numa-CMA-for-ARM-server/20200603-104821
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
for-next/core
config: x86_64-randconfig-m001-20200603 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
kernel/dma/contiguous.c:274 dma_alloc_contiguous() warn: variable dereferenced
before check 'dev' (see line 272)
#
https://github.com/0day-ci/linux/commit/adb919e972c1cac3d8b11905d5258d23d3aac6a4
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout adb919e972c1cac3d8b11905d5258d23d3aac6a4
vim +/dev +274 kernel/dma/contiguous.c
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 267
struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 268
{
90ae409f9eb3bc kernel/dma/contiguous.c Christoph Hellwig 2019-08-20 269
size_t count = size >> PAGE_SHIFT;
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 270
struct page *page = NULL;
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 271
struct cma *cma = NULL;
adb919e972c1ca kernel/dma/contiguous.c Barry Song2020-06-03 @272
int nid = dev_to_node(dev);
^^^
Dereferenced inside function.
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 273
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 @274
if (dev && dev->cma_area)
^^^
Too late.
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 275
cma = dev->cma_area;
adb919e972c1ca kernel/dma/contiguous.c Barry Song2020-06-03 276
else if ((nid != NUMA_NO_NODE) && dma_contiguous_pernuma_area[nid]
adb919e972c1ca kernel/dma/contiguous.c Barry Song2020-06-03 277
&& !(gfp & (GFP_DMA | GFP_DMA32)))
adb919e972c1ca kernel/dma/contiguous.c Barry Song2020-06-03 278
cma = dma_contiguous_pernuma_area[nid];
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 279
else if (count > 1)
bd2e75633c8012 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 280
cma = dma_contiguous_default_area;
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 281
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 282
/* CMA can be used only in the context which permits sleeping */
b1d2dc009dece4 kernel/dma/contiguous.c Nicolin Chen 2019-05-23 283
if (cma && gfpflags_allow_blocking(gfp)) {
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 09/13] device core: Introduce multiple dma pfn offsets
}
> + return 0;
> +}
> +
> +
> +
> +/**
> + * attach_dma_pfn_offset - Assign scalar offset for all addresses.
> + * @dev: device pointer; only needed for a corner case.
> + * @dma_pfn_offset: offset to apply when converting from phys addr
^^^
This parameter name does not match.
> + * to dma addr and vice versa.
> + *
> + * It returns -ENOMEM if out of memory, otherwise 0.
It can also return -ENODEV. Why are we passing NULL dev pointers to
all these functions anyway?
> + */
> +int attach_uniform_dma_pfn_offset(struct device *dev, unsigned long
> pfn_offset)
> +{
> + struct dma_pfn_offset_region *r;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + if (!pfn_offset)
> + return 0;
> +
> + r = devm_kcalloc(dev, 1, sizeof(struct dma_pfn_offset_region),
> + GFP_KERNEL);
Use:r = devm_kzalloc(dev, sizeof(*r), GFP_KERNEL);
> + if (!r)
> + return -ENOMEM;
> +
> + r->uniform_offset = true;
> + r->pfn_offset = pfn_offset;
> +
> + return 0;
> +}
This function doesn't seem to do anything useful. Is part of it
missing?
> +EXPORT_SYMBOL_GPL(attach_uniform_dma_pfn_offset);
> +
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Unlock on error paths
On Thu, Mar 12, 2020 at 08:02:41PM +0800, Lu Baolu wrote:
> On 2020/3/12 19:37, Dan Carpenter wrote:
> > There were a couple places where we need to unlock before returning.
> >
> > Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected")
> > Signed-off-by: Dan Carpenter
> > ---
> > drivers/iommu/intel-iommu-debugfs.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-iommu-debugfs.c
> > b/drivers/iommu/intel-iommu-debugfs.c
> > index 8d24c4d85cc2..6a495b103972 100644
> > --- a/drivers/iommu/intel-iommu-debugfs.c
> > +++ b/drivers/iommu/intel-iommu-debugfs.c
> > @@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct
> > seq_file *m, void *unused)
> > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
> > if (!(sts & DMA_GSTS_TES)) {
> > seq_puts(m, "DMA Remapping is not enabled\n");
> > - return 0;
> > + goto unlock;
> > }
> > root_tbl_walk(m, iommu);
> > seq_putc(m, '\n');
> > }
> > +unlock:
> > rcu_read_unlock();
> > return 0;
> > @@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file
> > *m, void *unused)
> > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
> > if (!(sts & DMA_GSTS_IRES)) {
> > seq_puts(m, "Interrupt Remapping is not enabled\n");
> > - return 0;
> > + goto unlock;
> > }
> > if (iommu->ir_table) {
> > @@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file
> > *m, void *unused)
> > }
> > seq_putc(m, '\n');
> > }
> > +unlock:
> > rcu_read_unlock();
> > return 0;
> >
>
> Thanks a lot for the catch. I think it could be further cleanup. How
> about below changes?
Obviously that solves the issues with forgetting to drop the lock but
I'm not qualified to comment on the rest. (And I can't really review
it anyway because the patch was damaged in sending the email).
regards,
dan carepnter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/vt-d: Unlock on error paths
There were a couple places where we need to unlock before returning.
Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected")
Signed-off-by: Dan Carpenter
---
drivers/iommu/intel-iommu-debugfs.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/intel-iommu-debugfs.c
b/drivers/iommu/intel-iommu-debugfs.c
index 8d24c4d85cc2..6a495b103972 100644
--- a/drivers/iommu/intel-iommu-debugfs.c
+++ b/drivers/iommu/intel-iommu-debugfs.c
@@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct seq_file
*m, void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_TES)) {
seq_puts(m, "DMA Remapping is not enabled\n");
- return 0;
+ goto unlock;
}
root_tbl_walk(m, iommu);
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
return 0;
@@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file *m,
void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_IRES)) {
seq_puts(m, "Interrupt Remapping is not enabled\n");
- return 0;
+ goto unlock;
}
if (iommu->ir_table) {
@@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file *m,
void *unused)
}
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
return 0;
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Pass gfp flags to iommu_map_page() in amd_iommu_map()
Did you get a chance to look at iommu_dma_alloc_remap() as well?
drivers/iommu/dma-iommu.c
584 static void *iommu_dma_alloc_remap(struct device *dev, size_t size,
585 dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
^
586 {
587 struct iommu_domain *domain = iommu_get_dma_domain(dev);
588 struct iommu_dma_cookie *cookie = domain->iova_cookie;
589 struct iova_domain *iovad = &cookie->iovad;
590 bool coherent = dev_is_dma_coherent(dev);
591 int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent,
attrs);
592 pgprot_t prot = dma_pgprot(dev, PAGE_KERNEL, attrs);
593 unsigned int count, min_size, alloc_sizes =
domain->pgsize_bitmap;
594 struct page **pages;
595 struct sg_table sgt;
596 dma_addr_t iova;
597 void *vaddr;
598
599 *dma_handle = DMA_MAPPING_ERROR;
600
601 if (unlikely(iommu_dma_deferred_attach(dev, domain)))
602 return NULL;
603
604 min_size = alloc_sizes & -alloc_sizes;
605 if (min_size < PAGE_SIZE) {
606 min_size = PAGE_SIZE;
607 alloc_sizes |= PAGE_SIZE;
608 } else {
609 size = ALIGN(size, min_size);
610 }
611 if (attrs & DMA_ATTR_ALLOC_SINGLE_PAGES)
612 alloc_sizes = min_size;
613
614 count = PAGE_ALIGN(size) >> PAGE_SHIFT;
615 pages = __iommu_dma_alloc_pages(dev, count, alloc_sizes >>
PAGE_SHIFT,
616 gfp);
617 if (!pages)
618 return NULL;
619
620 size = iova_align(iovad, size);
621 iova = iommu_dma_alloc_iova(domain, size,
dev->coherent_dma_mask, dev);
622 if (!iova)
623 goto out_free_pages;
624
625 if (sg_alloc_table_from_pages(&sgt, pages, count, 0, size,
GFP_KERNEL))
^^
gfp here instead of GFP_KERNEL?
626 goto out_free_iova;
627
628 if (!(ioprot & IOMMU_CACHE)) {
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Pass gfp-flags to iommu_map_page()
Hello Joerg Roedel,
The patch b911b89b6d01: "iommu/amd: Pass gfp-flags to
iommu_map_page()" from Jul 5, 2016, leads to the following static
checker warning:
drivers/iommu/amd_iommu.c:2545 amd_iommu_map()
warn: use 'gfp' here instead of GFP_XXX?
drivers/iommu/amd_iommu.c
2529 static int amd_iommu_map(struct iommu_domain *dom, unsigned long iova,
2530 phys_addr_t paddr, size_t page_size, int
iommu_prot,
2531 gfp_t gfp)
^
I don't know why I'm suddenly getting this warning even though the code
is three years old...
2532 {
2533 struct protection_domain *domain = to_pdomain(dom);
2534 int prot = 0;
2535 int ret;
2536
2537 if (domain->mode == PAGE_MODE_NONE)
2538 return -EINVAL;
2539
2540 if (iommu_prot & IOMMU_READ)
2541 prot |= IOMMU_PROT_IR;
2542 if (iommu_prot & IOMMU_WRITE)
2543 prot |= IOMMU_PROT_IW;
2544
2545 ret = iommu_map_page(domain, iova, paddr, page_size, prot,
GFP_KERNEL);
^^
But it does seem like maybe gfp was intended here?
2546
2547 domain_flush_np_cache(domain, iova, page_size);
2548
2549 return ret;
2550 }
See also:
drivers/iommu/dma-iommu.c:625 iommu_dma_alloc_remap() warn: use 'gfp' here
instead of GFP_XXX?
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/qcom: Simplify a test in 'qcom_iommu_add_device()'
On Mon, Sep 16, 2019 at 10:29:36PM +0200, Christophe JAILLET wrote:
> 'iommu_group_get_for_dev()' never returns NULL, so this test can be
> simplified a bit.
>
It used to until commit 72dcac633475 ("iommu: Warn once when device_group
callback returns NULL").
Reviewed-by: Dan Carpenter
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: use exact allocation for dma coherent memory
I once wrote a Smatch check based on a commit message that said we can't
pass dma_alloc_coherent() pointers to virt_to_phys(). But then I never
felt like I understood the rules enough to actually report the warnings
as bugs.
drivers/platform/x86/dcdbas.c:108 smi_data_buf_realloc() error: 'buf' came from
dma_alloc_coherent() so we can't do virt_to_phys()
drivers/net/caif/caif_virtio.c:414 cfv_create_genpool() error:
'cfv->alloc_addr' came from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:135 alloc_host_sq() error: 'sq->queue' came
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:272 create_qp() error: 'wq->rq.queue' came
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c:2628 alloc_srq_queue() error: 'wq->queue' came
from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:494 ocrdma_alloc_ucontext() error:
'ctx->ah_tbl.va' came from dma_alloc_coherent() so we can't do virt_to_phys()
drivers/infiniband/hw/cxgb4/qp.c
129 static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
130 {
131 sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev),
sq->memsize,
132 &(sq->dma_addr), GFP_KERNEL);
133 if (!sq->queue)
134 return -ENOMEM;
135 sq->phys_addr = virt_to_phys(sq->queue);
136 dma_unmap_addr_set(sq, mapping, sq->dma_addr);
137 return 0;
138 }
Is this a bug?
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] dma-mapping: remove an unnecessary NULL check
We already dereferenced "dev" when we called get_dma_ops() so this NULL check is too late. We're not supposed to pass NULL "dev" pointers to dma_alloc_attrs(). Signed-off-by: Dan Carpenter --- There are still at least two drivers which do pass a NULL unfortunately. drivers/staging/comedi/drivers/comedi_isadma.c:195 comedi_isadma_alloc() error: NULL dereference inside function 'dma_alloc_coherent()' drivers/staging/comedi/drivers/comedi_isadma.c:227 comedi_isadma_free() error: NULL dereference inside function 'dma_free_coherent()' drivers/tty/synclink.c:3667 mgsl_alloc_buffer_list_memory() error: NULL dereference inside function 'dma_alloc_coherent()' drivers/tty/synclink.c:3738 mgsl_free_buffer_list_memory() error: NULL dereference inside function 'dma_free_coherent()' drivers/tty/synclink.c:3777 mgsl_alloc_frame_memory() error: NULL dereference inside function 'dma_alloc_coherent()' drivers/tty/synclink.c:3811 mgsl_free_frame_memory() error: NULL dereference inside function 'dma_free_coherent()' If I remember right there are one or two others which sometimes pass a NULL, but it's harder to grep for those warnings. kernel/dma/mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index 685a53f2a793..f7afdadb6770 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -244,7 +244,7 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle, const struct dma_map_ops *ops = get_dma_ops(dev); void *cpu_addr; - WARN_ON_ONCE(dev && !dev->coherent_dma_mask); + WARN_ON_ONCE(!dev->coherent_dma_mask); if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) return cpu_addr; -- 2.18.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [bug report] iommu/io-pgtable-arm: Fix pgtable allocation in selftest
On Wed, Apr 10, 2019 at 10:44:03AM +0100, Robin Murphy wrote:
> Hi Dan,
>
> On 10/04/2019 10:34, Dan Carpenter wrote:
> > Hello Jean-Philippe Brucker,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch fac83d29d954: "iommu/io-pgtable-arm: Fix pgtable allocation
> > in selftest" from Jun 18, 2018, leads to the following Smatch
> > complaint:
> >
> > drivers/iommu/io-pgtable-arm.c:246 __arm_lpae_alloc_pages()
> > error: we previously assumed 'dev' could be null (see line 239)
> >
> > drivers/iommu/io-pgtable-arm.c
> > 238 VM_BUG_ON((gfp & __GFP_HIGHMEM));
> > 239 p = alloc_pages_node(dev ? dev_to_node(dev) :
> > NUMA_NO_NODE,
> > ^^^
> > We added a NULL check here
> >
> > 240 gfp | __GFP_ZERO, order);
> > 241 if (!p)
> > 242 return NULL;
> > 243
> > 244 pages = page_address(p);
> > 245 if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA)) {
>
> The selftests *should* always set this quirk such that they never get to the
> DMA mapping calls (that was one of the reasons for implementing things that
> way) - I guess that might be a bit too sneaky for Smatch, but I can take a
> look to double-check that the flow is working correctly such that this
> really is a false-positive.
Ah, thanks.
I have a plan for fixing these false positives caused by not tracking
bit masks. It's not that complicated to fix, it just takes time to
write the code.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/io-pgtable-arm: Fix pgtable allocation in selftest
Hello Jean-Philippe Brucker,
This is a semi-automatic email about new static checker warnings.
The patch fac83d29d954: "iommu/io-pgtable-arm: Fix pgtable allocation
in selftest" from Jun 18, 2018, leads to the following Smatch
complaint:
drivers/iommu/io-pgtable-arm.c:246 __arm_lpae_alloc_pages()
error: we previously assumed 'dev' could be null (see line 239)
drivers/iommu/io-pgtable-arm.c
238 VM_BUG_ON((gfp & __GFP_HIGHMEM));
239 p = alloc_pages_node(dev ? dev_to_node(dev) : NUMA_NO_NODE,
^^^
We added a NULL check here
240 gfp | __GFP_ZERO, order);
241 if (!p)
242 return NULL;
243
244 pages = page_address(p);
245 if (!(cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA)) {
246 dma = dma_map_single(dev, pages, size, DMA_TO_DEVICE);
^^^
But dma_map_single() can't take a NULL argument either.
247 if (dma_mapping_error(dev, dma))
248 goto out_free;
I don't know why this warning is only showing up now, almost a year
later. Sorry about that.
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] x86/Hyper-V: Set x2apic destination mode to physical when x2apic is available
On Thu, Jan 31, 2019 at 06:17:31PM +0800, lantianyu1...@gmail.com wrote: > > This comment needs to be indented one tab or it looks like we're outside the funciton. > +/* > + * Hyper-V doesn't provide irq remapping for IO-APIC. To enable x2apic, > + * set x2apic destination mode to physcial mode when x2apic is available > + * and Hyper-V IOMMU driver makes sure cpus assigned with IO-APIC irqs > + * have 8-bit APIC id. > + */ > +# if IS_ENABLED(CONFIG_HYPERV_IOMMU) > + if (x2apic_supported()) > + x2apic_phys = 1; > +# endif The IS_ENABLED() macro is really magical. You could write this like so: if (IS_ENABLED(CONFIG_HYPERV_IOMMU) && x2apic_supported()) x2apic_phys = 1; It works the same and is slightly more pleasant to look at. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/7] vfio: add sdmdev support
Hi Kenneth,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.19-rc2 next-20180905]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Kenneth-Lee/A-General-Accelerator-Framework-WarpDrive/20180903-162733
base:
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
smatch warnings:
drivers/vfio/sdmdev/vfio_sdmdev.c:78 iommu_type_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:91 dma_flag_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:127 flags_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:128 name_show() error: 'sdmdev' dereferencing
possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:130 device_api_show() error: 'sdmdev'
dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:138 available_instances_show() error:
'sdmdev' dereferencing possible ERR_PTR()
drivers/vfio/sdmdev/vfio_sdmdev.c:178 vfio_sdmdev_mdev_remove() warn: if();
#
https://github.com/0day-ci/linux/commit/1e47d5e608652b4a2c813dbeaf5aa6811f6ceaf7
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 1e47d5e608652b4a2c813dbeaf5aa6811f6ceaf7
vim +/sdmdev +78 drivers/vfio/sdmdev/vfio_sdmdev.c
1e47d5e6 Kenneth Lee 2018-09-03 69
1e47d5e6 Kenneth Lee 2018-09-03 70 static ssize_t iommu_type_show(struct
device *dev,
1e47d5e6 Kenneth Lee 2018-09-03 71 struct
device_attribute *attr, char *buf)
1e47d5e6 Kenneth Lee 2018-09-03 72 {
1e47d5e6 Kenneth Lee 2018-09-03 73struct vfio_sdmdev *sdmdev =
vfio_sdmdev_pdev_sdmdev(dev);
^^^
Presumably this returns error pointers instead of NULL?
1e47d5e6 Kenneth Lee 2018-09-03 74
1e47d5e6 Kenneth Lee 2018-09-03 75if (!sdmdev)
1e47d5e6 Kenneth Lee 2018-09-03 76return -ENODEV;
1e47d5e6 Kenneth Lee 2018-09-03 77
1e47d5e6 Kenneth Lee 2018-09-03 @78return sprintf(buf, "%d\n",
sdmdev->iommu_type);
1e47d5e6 Kenneth Lee 2018-09-03 79 }
1e47d5e6 Kenneth Lee 2018-09-03 80
1e47d5e6 Kenneth Lee 2018-09-03 81 static DEVICE_ATTR_RO(iommu_type);
1e47d5e6 Kenneth Lee 2018-09-03 82
1e47d5e6 Kenneth Lee 2018-09-03 83 static ssize_t dma_flag_show(struct
device *dev,
1e47d5e6 Kenneth Lee 2018-09-03 84 struct
device_attribute *attr, char *buf)
1e47d5e6 Kenneth Lee 2018-09-03 85 {
1e47d5e6 Kenneth Lee 2018-09-03 86struct vfio_sdmdev *sdmdev =
vfio_sdmdev_pdev_sdmdev(dev);
1e47d5e6 Kenneth Lee 2018-09-03 87
1e47d5e6 Kenneth Lee 2018-09-03 88if (!sdmdev)
1e47d5e6 Kenneth Lee 2018-09-03 89return -ENODEV;
1e47d5e6 Kenneth Lee 2018-09-03 90
1e47d5e6 Kenneth Lee 2018-09-03 @91return sprintf(buf, "%d\n",
sdmdev->dma_flag);
1e47d5e6 Kenneth Lee 2018-09-03 92 }
1e47d5e6 Kenneth Lee 2018-09-03 93
1e47d5e6 Kenneth Lee 2018-09-03 94 static DEVICE_ATTR_RO(dma_flag);
1e47d5e6 Kenneth Lee 2018-09-03 95
1e47d5e6 Kenneth Lee 2018-09-03 96 /* mdev->dev_attr_groups */
1e47d5e6 Kenneth Lee 2018-09-03 97 static struct attribute
*vfio_sdmdev_attrs[] = {
1e47d5e6 Kenneth Lee 2018-09-03 98&dev_attr_iommu_type.attr,
1e47d5e6 Kenneth Lee 2018-09-03 99&dev_attr_dma_flag.attr,
1e47d5e6 Kenneth Lee 2018-09-03 100NULL,
1e47d5e6 Kenneth Lee 2018-09-03 101 };
1e47d5e6 Kenneth Lee 2018-09-03 102 static const struct attribute_group
vfio_sdmdev_group = {
1e47d5e6 Kenneth Lee 2018-09-03 103.name =
VFIO_SDMDEV_PDEV_ATTRS_GRP_NAME,
1e47d5e6 Kenneth Lee 2018-09-03 104.attrs = vfio_sdmdev_attrs,
1e47d5e6 Kenneth Lee 2018-09-03 105 };
1e47d5e6 Kenneth Lee 2018-09-03 106 const struct attribute_group
*vfio_sdmdev_groups[] = {
1e47d5e6 Kenneth Lee 2018-09-03 107&vfio_sdmdev_group,
1e47d5e6 Kenneth Lee 2018-09-03 108NULL,
1e47d5e6 Kenneth Lee 2018-09-03 109 };
1e47d5e6 Kenneth Lee 2018-09-03 110
1e47d5e6 Kenneth Lee 2018-09-03 111 /* default attributes for
mdev->supported_type_groups, used by registerer*/
1e47d5e6 Kenneth Lee 2018-09-03 112 #define MDEV_TYPE_ATTR_RO_EXPORT(name) \
1e47d5e6 Kenneth Lee 2018-09-03 113MDEV_TYPE_ATTR_RO(name); \
1e47d5e6 Kenneth Lee 2018-09-03 114
EXPORT_SYMBOL_GPL(mdev_type_attr_##name);
1e47d5e6 Kenneth Lee 2018-09-03 115
1e47d5e6 Kenneth Lee 2018-09-03 116 #define DEF_SIMPLE_SDMDEV_ATTR(_name,
sdmdev_member, format) \
1e47d5e6 Kenneth Lee 2018-09-03 117 static ssize_t _name##_show(struct
kobject *kobj, struct device *dev, \
1e47d5e6 Kenneth Lee 2018-09-03 118char *buf) \
1e47d5e6 Kenneth Lee 2018-09-03 119 { \
1e47d5e6 Kenneth Lee 2018-09-03 120
[PATCH] swiotlb: remove an unecessary NULL check
Smatch complains here: lib/swiotlb.c:730 swiotlb_alloc_buffer() warn: variable dereferenced before check 'dev' (see line 716) "dev" isn't ever NULL in this function so we can just remove the check. Signed-off-by: Dan Carpenter diff --git a/lib/swiotlb.c b/lib/swiotlb.c index 47aeb04c1997..51dde88e50a3 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -727,7 +727,7 @@ swiotlb_alloc_buffer(struct device *dev, size_t size, dma_addr_t *dma_handle, out_unmap: dev_warn(dev, "hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016Lx\n", - (unsigned long long)(dev ? dev->coherent_dma_mask : 0), + (unsigned long long)dev->coherent_dma_mask, (unsigned long long)*dma_handle); /* ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/arm-smmu: Make use of the iommu_register interface
Hello Joerg Roedel, This is a semi-automatic email about new static checker warnings. The patch 9648cbc9625b: "iommu/arm-smmu: Make use of the iommu_register interface" from Feb 1, 2017, leads to the following Smatch complaint: ./drivers/iommu/arm-smmu-v3.c:1957 arm_smmu_remove_device() warn: variable dereferenced before check 'master' (see line 1956) ./drivers/iommu/arm-smmu-v3.c 1955 master = fwspec->iommu_priv; 1956 smmu = master->smmu; Pach adds unchecked dereference 1957 if (master && master->ste.assigned) ^^ The old code assumes "master" can be NULL. 1958 arm_smmu_detach_dev(dev); 1959 iommu_group_remove_device(dev); regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
On Thu, Aug 24, 2017 at 08:47:33PM +0800, Baoquan He wrote: > On 08/24/17 at 03:32pm, Dan Carpenter wrote: > > Take a look at this code for example. But all the places which call > > get_domain() are the same: > > > > drivers/iommu/amd_iommu.c > > 2648 page = virt_to_page(virt_addr); > > 2649 size = PAGE_ALIGN(size); > > 2650 > > 2651 domain = get_domain(dev); > > ^^ > > imagined get_domain() returns NULL. > > > > 2652 if (IS_ERR(domain)) > > 2653 goto free_mem; > > 2654 > > 2655 dma_dom = to_dma_ops_domain(domain); > > ^ > > This will Oops. > > I see, it's a problem. Thanks for telling! > > How about below change? But I am not very sure which errno should be > picked, seems the latter one, EBUSY is better since it has passed the > check_device() checking. Looks good to me. You know better than I do which errno is best, so I'll leave that to you. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
Take a look at this code for example. But all the places which call get_domain() are the same: drivers/iommu/amd_iommu.c 2648 page = virt_to_page(virt_addr); 2649 size = PAGE_ALIGN(size); 2650 2651 domain = get_domain(dev); ^^ imagined get_domain() returns NULL. 2652 if (IS_ERR(domain)) 2653 goto free_mem; 2654 2655 dma_dom = to_dma_ops_domain(domain); ^ This will Oops. 2656 regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Check if domain is NULL before dereference it
On Thu, Aug 24, 2017 at 07:56:47PM +0800, Baoquan He wrote: > In get_domain(), 'domain' could still be NULL before it's passed to > dma_ops_domain() to dereference. For safety, check if 'domain' is > NULL before passing to dma_ops_domain(). > > Reported-by: Dan Carpenter > Signed-off-by: Baoquan He > --- > drivers/iommu/amd_iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c > index 16f1e6af00b0..2e2d5e6a13b3 100644 > --- a/drivers/iommu/amd_iommu.c > +++ b/drivers/iommu/amd_iommu.c > @@ -2262,7 +2262,7 @@ static struct protection_domain *get_domain(struct > device *dev) > domain = to_pdomain(io_domain); > attach_device(dev, domain); > } > - if (!dma_ops_domain(domain)) > + if (domain && !dma_ops_domain(domain)) > return ERR_PTR(-EBUSY); > > return domain; This still doesn't look right. None of the callers can handle a NULL domain. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Use is_attach_deferred call-back
Hello Baoquan He,
This is a semi-automatic email about new static checker warnings.
The patch df3f7a6e8e85: "iommu/amd: Use is_attach_deferred call-back"
from Aug 9, 2017, leads to the following Smatch complaint:
drivers/iommu/amd_iommu.c:2265 get_domain()
error: we previously assumed 'domain' could be null (see line 2259)
drivers/iommu/amd_iommu.c
2258 domain = get_dev_data(dev)->domain;
2259 if (domain == NULL && get_dev_data(dev)->defer_attach) {
^^
The patch adds a new check for NULL.
2260 get_dev_data(dev)->defer_attach = false;
2261 io_domain = iommu_get_domain_for_dev(dev);
2262 domain = to_pdomain(io_domain);
2263 attach_device(dev, domain);
2264 }
2265 if (!dma_ops_domain(domain))
^^
Existing unchecked dereference inside the function.
2266 return ERR_PTR(-EBUSY);
2267
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids
Hello Magnus Damm,
This is a semi-automatic email about new static checker warnings.
The patch 7b2d59611fef: "iommu/ipmmu-vmsa: Replace local utlb code
with fwspec ids" from Jul 17, 2017, leads to the following Smatch
complaint:
drivers/iommu/ipmmu-vmsa.c:555 ipmmu_attach_device()
warn: variable dereferenced before check 'priv' (see line 549)
drivers/iommu/ipmmu-vmsa.c
548 struct iommu_fwspec *fwspec = dev->iommu_fwspec;
549 struct ipmmu_vmsa_device *mmu = priv->mmu;
^
Old dereference.
550 struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
551 unsigned long flags;
552 unsigned int i;
553 int ret = 0;
554
555 if (!priv || !priv->mmu) {
Patch adds new check. Should this be checking fwspec?
556 dev_err(dev, "Cannot attach to IPMMU\n");
557 return -ENXIO;
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/arm-smmu: Make use of the iommu_register interface
Hello Joerg Roedel, This is a semi-automatic email about new static checker warnings. The patch 9648cbc9625b: "iommu/arm-smmu: Make use of the iommu_register interface" from Feb 1, 2017, leads to the following Smatch complaint: drivers/iommu/arm-smmu-v3.c:1810 arm_smmu_remove_device() warn: variable dereferenced before check 'master' (see line 1809) drivers/iommu/arm-smmu-v3.c 1808 master = fwspec->iommu_priv; 1809 smmu = master->smmu; New dereference. 1810 if (master && master->ste.valid) ^^ Old code checked for NULL. 1811 arm_smmu_detach_dev(dev); 1812 iommu_group_remove_device(dev); regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch] iommu: silence an uninintialized variable warning
Never mind. Eric just sent his own patch. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu: silence an uninintialized variable warning
My static checker complains that we return an uninitialized scalar if
the list is empty. If that's the case then we should return zero.
Fixes: 6c65fb318e8b ("iommu: iommu_get_group_resv_regions")
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b752c3d91cfc..0e1750481ebc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -210,9 +210,9 @@ iommu_insert_device_resv_regions(struct list_head
*dev_resv_regions,
list_for_each_entry(entry, dev_resv_regions, list) {
ret = iommu_insert_resv_region(entry, group_resv_regions);
if (ret)
- break;
+ return ret;
}
- return ret;
+ return 0;
}
int iommu_get_group_resv_regions(struct iommu_group *group,
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu: iommu_get_group_resv_regions
Hello Eric Auger,
The patch 6c65fb318e8b: "iommu: iommu_get_group_resv_regions" from
Jan 19, 2017, leads to the following static checker warning:
drivers/iommu/iommu.c:215 iommu_insert_device_resv_regions()
error: uninitialized symbol 'ret'.
drivers/iommu/iommu.c
203 static int
204 iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
205 struct list_head *group_resv_regions)
206 {
207 struct iommu_resv_region *entry;
208 int ret;
209
210 list_for_each_entry(entry, dev_resv_regions, list) {
211 ret = iommu_insert_resv_region(entry,
group_resv_regions);
212 if (ret)
213 break;
214 }
215 return ret;
On the one hand, it probably doesn't make sense that the dev_resv_regions
would ever be empty, but on the other hand, there some code that assumes
it is possible. What I mean is that iommu_get_resv_regions() can
basically do nothing if ->get_resv_regions() isn't implemented.
I guess we should probably set ret = -EINVAL here?
216 }
217
218 int iommu_get_group_resv_regions(struct iommu_group *group,
219 struct list_head *head)
220 {
221 struct iommu_device *device;
222 int ret = 0;
223
224 mutex_lock(&group->mutex);
225 list_for_each_entry(device, &group->devices, list) {
226 struct list_head dev_resv_regions;
227
228 INIT_LIST_HEAD(&dev_resv_regions);
229 iommu_get_resv_regions(device->dev, &dev_resv_regions);
230 ret =
iommu_insert_device_resv_regions(&dev_resv_regions, head);
231 iommu_put_resv_regions(device->dev, &dev_resv_regions);
232 if (ret)
233 break;
234 }
235 mutex_unlock(&group->mutex);
236 return ret;
237 }
238 EXPORT_SYMBOL_GPL(iommu_get_group_resv_regions);
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: Missing error code in amd_iommu_init_device()
We should set "ret" to -EINVAL if iommu_group_get() fails.
Fixes: 55c99a4dc50f ("iommu/amd: Use iommu_attach_group()")
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 594849a..f8ed8c9 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -805,8 +805,10 @@ int amd_iommu_init_device(struct pci_dev *pdev, int pasids)
goto out_free_domain;
group = iommu_group_get(&pdev->dev);
- if (!group)
+ if (!group) {
+ ret = -EINVAL;
goto out_free_domain;
+ }
ret = iommu_attach_group(dev_state->domain, group);
if (ret != 0)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu: Implement common IOMMU ops for DMA mapping
Hello Robin Murphy,
The patch 0db2e5d18f76: "iommu: Implement common IOMMU ops for DMA
mapping" from Oct 1, 2015, leads to the following static checker
warning:
drivers/iommu/dma-iommu.c:377 iommu_dma_alloc()
warn: use 'gfp' here instead of GFP_XXX?
drivers/iommu/dma-iommu.c
343 struct page **iommu_dma_alloc(struct device *dev, size_t size, gfp_t
gfp,
^
344 unsigned long attrs, int prot, dma_addr_t *handle,
345 void (*flush_page)(struct device *, const void *,
phys_addr_t))
346 {
347 struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
348 struct iova_domain *iovad = cookie_iovad(domain);
349 struct iova *iova;
350 struct page **pages;
351 struct sg_table sgt;
352 dma_addr_t dma_addr;
353 unsigned int count, min_size, alloc_sizes =
domain->pgsize_bitmap;
354
355 *handle = DMA_ERROR_CODE;
356
357 min_size = alloc_sizes & -alloc_sizes;
358 if (min_size < PAGE_SIZE) {
359 min_size = PAGE_SIZE;
360 alloc_sizes |= PAGE_SIZE;
361 } else {
362 size = ALIGN(size, min_size);
363 }
364 if (attrs & DMA_ATTR_ALLOC_SINGLE_PAGES)
365 alloc_sizes = min_size;
366
367 count = PAGE_ALIGN(size) >> PAGE_SHIFT;
368 pages = __iommu_dma_alloc_pages(count, alloc_sizes >>
PAGE_SHIFT, gfp);
Here we use gfp.
369 if (!pages)
370 return NULL;
371
372 iova = __alloc_iova(domain, size, dev->coherent_dma_mask);
373 if (!iova)
374 goto out_free_pages;
375
376 size = iova_align(iovad, size);
377 if (sg_alloc_table_from_pages(&sgt, pages, count, 0, size,
GFP_KERNEL))
^^
Here we're using GFP_KERNEL. I don't know the code well enough to say
if it was intentional.
378 goto out_free_iova;
379
380 if (!(prot & IOMMU_CACHE)) {
381 struct sg_mapping_iter miter;
382 /*
383 * The CPU-centric flushing implied by SG_MITER_TO_SG
isn't
384 * sufficient here, so skip it by using the "wrong"
direction.
385 */
386 sg_miter_start(&miter, sgt.sgl, sgt.orig_nents,
SG_MITER_FROM_SG);
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/vt-d: Fix IOMMU lookup for SR-IOV Virtual Functions
Hello Ashok Raj,
This is a semi-automatic email about new static checker warnings.
The patch 1c387188c60f: "iommu/vt-d: Fix IOMMU lookup for SR-IOV
Virtual Functions" from Oct 21, 2016, leads to the following Smatch
complaint:
drivers/iommu/intel-iommu.c:918 device_to_iommu()
error: we previously assumed 'pdev' could be null (see line 908)
drivers/iommu/intel-iommu.c
907 for_each_active_iommu(iommu, drhd) {
908 if (pdev && segment != drhd->segment)
Other code assumes pdev can be NULL.
909 continue;
910
911 for_each_active_dev_scope(drhd->devices,
912drhd->devices_cnt, i, tmp) {
913 if (tmp == dev) {
914 /* For a VF use its original BDF# not
that of the PF
915 * which we used for the IOMMU lookup.
Strictly speaking
916 * we could do this for all PCI
devices; we only need to
917 * get the BDF# from the scope table
for ACPI matches. */
918 if (pdev->is_virtfn)
^^^
Not checked.
919 goto got_pdev;
920
921 *bus = drhd->devices[i].bus;
922 *devfn = drhd->devices[i].devfn;
923 goto out;
924 }
925
926 if (!pdev || !dev_is_pci(tmp))
Checked here as well.
927 continue;
928
929 ptmp = to_pci_dev(tmp);
930 if (ptmp->subordinate &&
931 ptmp->subordinate->number <=
pdev->bus->number &&
932 ptmp->subordinate->busn_res.end >=
pdev->bus->number)
933 goto got_pdev;
934 }
935
936 if (pdev && drhd->include_all) {
937 got_pdev:
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: Signedness bug in acpihid_device_group()
"devid" needs to be signed for the error handling to work.
Fixes:b097d11a0fa3f ('iommu/amd: Manage iommu_group for ACPI HID devices')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index c430c10..12f7779 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -283,7 +283,7 @@ static struct iommu_dev_data *get_dev_data(struct device
*dev)
static struct iommu_group *acpihid_device_group(struct device *dev)
{
struct acpihid_map_entry *p, *entry = NULL;
- u16 devid;
+ int devid;
devid = get_acpihid_device_id(dev, &entry);
if (devid < 0)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: silence an uninitialized variable warning
My static checker complains that "dma_alias" is uninitialized unless we
are dealing with a pci device. This is true but harmless. Anyway, we
can flip the condition around to silence the warning.
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a2e1b7f..e1852e8 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2458,7 +2458,7 @@ static struct dmar_domain *get_domain_for_dev(struct
device *dev, int gaw)
}
/* register PCI DMA alias device */
- if (req_id != dma_alias && dev_is_pci(dev)) {
+ if (dev_is_pci(dev) && req_id != dma_alias) {
tmp = dmar_insert_one_dev_info(iommu, PCI_BUS_NUM(dma_alias),
dma_alias & 0xff, NULL, domain);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch 2/2] iommu/mediatek: checking for IS_ERR() instead of NULL
of_platform_device_create() returns NULL on error, it never returns
error pointers.
Fixes: 0df4fabe208d ('iommu/mediatek: Add mt8173 IOMMU driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 1a4022c..4682da4 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -628,7 +628,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_platform_device_create(
larbnode, NULL,
platform_bus_type.dev_root);
- if (IS_ERR(plarbdev))
+ if (!plarbdev)
return -EPROBE_DEFER;
}
data->smi_imu.larb_imu[i].dev = &plarbdev->dev;
@@ -721,8 +721,8 @@ static int mtk_iommu_init_fn(struct device_node *np)
struct platform_device *pdev;
pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
- if (IS_ERR(pdev))
- return PTR_ERR(pdev);
+ if (!pdev)
+ return -ENOMEM;
ret = platform_driver_register(&mtk_iommu_driver);
if (ret) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch 1/2] iommu/mediatek: signedness bug in probe function
"larb_nr" needs to be signed for the error handling to work. "i" can
be int as well.
Fixes: 0df4fabe208d ('iommu/mediatek: Add mt8173 IOMMU driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 721ffdb..1a4022c 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -578,7 +578,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
struct resource *res;
struct component_match *match = NULL;
void*protect;
- unsigned inti, larb_nr;
+ int i, larb_nr;
int ret;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/exynos: checking for IS_ERR() instead of NULL
of_platform_device_create() returns NULL on error, it never returns
error pointers.
Fixes: 8ed55c812fa8 ('iommu/exynos: Init from dt-specific callback instead of
initcall')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b066504..cb57bda 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1347,8 +1347,8 @@ static int __init exynos_iommu_of_setup(struct
device_node *np)
exynos_iommu_init();
pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
- if (IS_ERR(pdev))
- return PTR_ERR(pdev);
+ if (!pdev)
+ return -ENOMEM;
/*
* use the first registered sysmmu device for performing
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: fix a loop in prq_event_thread()
There is an extra semi-colon on this if statement so we always break on
the first iteration.
Fixes: 0204a4960982 ('iommu/vt-d: Add callback to device driver on page faults')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 99a7803..dd070e6 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -536,7 +536,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
/* Accounting for major/minor faults? */
rcu_read_lock();
list_for_each_entry_rcu(sdev, &svm->devs, list) {
- if (sdev->sid == PCI_DEVID(req->bus, req->devfn));
+ if (sdev->sid == PCI_DEVID(req->bus, req->devfn))
break;
}
/* Other devices can go away, but the drivers are not permitted
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
re: iommu/vt-d: Implement page request handling
Hello David Woodhouse,
This is a semi-automatic email about new static checker warnings.
The patch 0b9252a34858: "iommu/vt-d: Implement page request handling"
from Oct 8, 2015, leads to the following Smatch complaint:
drivers/iommu/intel-svm.c:452 prq_event_thread()
error: we previously assumed 'svm' could be null (see line 412)
drivers/iommu/intel-svm.c
411 svm = idr_find(&iommu->pasid_idr, req->pasid);
412 if (!svm) {
The patch adds a NULL check here.
413 pr_err("%s: Page request for invalid
PASID %d: %08llx %08llx\n",
414 iommu->name, req->pasid,
((unsigned long long *)req)[0],
415 ((unsigned long long *)req)[1]);
416 mutex_unlock(&pasid_mutex);
417 goto inval_req;
418 }
419 /* Strictly speaking, we shouldn't need this.
It is forbidden
420 to unbind the PASID while there may still be
requests in
421 flight. But let's do it anyway. */
422 kref_get(&svm->kref);
423 mutex_unlock(&pasid_mutex);
424 }
425
426 result = QI_RESP_FAILURE;
427 down_read(&svm->mm->mmap_sem);
428 vma = find_extend_vma(svm->mm, address);
429 if (!vma || address < vma->vm_start)
430 goto hard_fault;
431
432 ret = handle_mm_fault(svm->mm, vma, address,
433req->wr_req ? FAULT_FLAG_WRITE :
0);
434 if (ret & VM_FAULT_ERROR)
435 goto hard_fault;
436
437 result = QI_RESP_SUCCESS;
438 hard_fault:
439 up_read(&svm->mm->mmap_sem);
440 inval_req:
441 /* Accounting for major/minor faults? */
442
443 if (req->lpig) {
Perhaps this check ensures that "svm" is not NULL but I don't know the
code well enough to say.
444 /* Page Group Response */
445 resp.low = QI_PGRP_PASID(req->pasid) |
446 QI_PGRP_DID((req->bus << 8) |
req->devfn) |
447 QI_PGRP_PASID_P(req->pasid_present) |
448 QI_PGRP_RESP_TYPE;
449 resp.high = QI_PGRP_IDX(req->prg_index) |
450 QI_PGRP_PRIV(req->private) |
QI_PGRP_RESP_CODE(result);
451
452 qi_submit_sync(&resp, svm->iommu);
^^
The patch introduces a NULL dereference here.
453 } else if (req->srr) {
454 /* Page Stream Response */
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: shift wrapping bug in prq_event_thread()
The "req->addr" variable is a bit field declared as "u64 addr:52;".
The "address" variable is a u64. We need to cast "req->addr" to a u64
before the shift or the result is truncated to 52 bits.
Fixes: 0b9252a34858 ('iommu/vt-d: Implement page request handling')
Signed-off-by: Dan Carpenter
---
Also does this code work if PAGE_SHIFT is more than 12? (I am a newbie
so this is not rhetorical, I don't know the answer).
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index ba9..19aa67b 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -395,7 +395,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
req = &iommu->prq[head / sizeof(*req)];
result = QI_RESP_INVALID;
- address = req->addr << PAGE_SHIFT;
+ address = (u64)req->addr << PAGE_SHIFT;
if (!req->pasid_present) {
pr_err("%s: Page request without PASID: %08llx
%08llx\n",
iommu->name, ((unsigned long long *)req)[0],
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: iommu/amd: Implement dm_region call-backs
On Thu, Jun 11, 2015 at 09:43:27AM +0200, Joerg Roedel wrote: > On Wed, Jun 10, 2015 at 06:39:20PM +0300, Dan Carpenter wrote: > > Hello Joerg Roedel, > > > > The patch b61238c4a5e1: "iommu/amd: Implement dm_region call-backs" > > from May 28, 2015, leads to the following static checker warning: > > > > drivers/iommu/amd_iommu.c:3153 amd_iommu_get_dm_regions() > > error: potential null dereference 'region'. (kzalloc returns null) > > Thanks for the report, I added a check for the return value and folded > it back into the original patch. > > What static checker do you use, btw? I'm using Smatch. The ERR_PTR stuff requires that you run smatch_scripts/build_kernel_data.sh At the start and that takes a couple hours. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
re: iommu/amd: Implement dm_region call-backs
Hello Joerg Roedel,
The patch b61238c4a5e1: "iommu/amd: Implement dm_region call-backs"
from May 28, 2015, leads to the following static checker warning:
drivers/iommu/amd_iommu.c:3153 amd_iommu_get_dm_regions()
error: potential null dereference 'region'. (kzalloc returns null)
drivers/iommu/amd_iommu.c
3138 static void amd_iommu_get_dm_regions(struct device *dev,
3139 struct list_head *head)
3140 {
3141 struct unity_map_entry *entry;
3142 u16 devid;
3143
3144 devid = get_device_id(dev);
3145
3146 list_for_each_entry(entry, &amd_iommu_unity_map, list) {
3147 struct iommu_dm_region *region;
3148
3149 if (devid < entry->devid_start || devid >
entry->devid_end)
3150 continue;
3151
3152 region = kzalloc(sizeof(*region), GFP_KERNEL);
No check here. We may not care and in that case just ignore this
email. Or add GFP_NOFAIL to silence the warning if you want.
3153 region->start = entry->address_start;
3154 region->length = entry->address_end -
entry->address_start;
3155 if (entry->prot & IOMMU_PROT_IR)
3156 region->prot |= IOMMU_READ;
3157 if (entry->prot & IOMMU_PROT_IW)
3158 region->prot |= IOMMU_WRITE;
3159
3160 list_add_tail(®ion->list, head);
3161 }
3162 }
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
re: iommu/amd: Put IOMMUv2 devices in a direct mapped domain
Hello Joerg Roedel,
This is a semi-automatic email about new static checker warnings.
The patch 0a3da4517107: "iommu/amd: Put IOMMUv2 devices in a direct
mapped domain" from May 28, 2015, leads to the following Smatch
complaint:
drivers/iommu/amd_iommu.c:2285 amd_iommu_add_device()
error: we previously assumed 'dev_data' could be null (see line 2279)
drivers/iommu/amd_iommu.c
2278 dev_data = get_dev_data(dev);
2279 if (dev_data && dev_data->iommu_v2)
Check.
2280 iommu_request_dm_for_dev(dev);
2281
2282 /* Domains are initialized for this device - have a look what
we ended up with */
2283 domain = iommu_get_domain_for_dev(dev);
2284 if (domain->type == IOMMU_DOMAIN_IDENTITY) {
2285 dev_data->passthrough = true;
^^
Unchecked dereference.
2286 dev->archdata.dma_ops = &nommu_dma_ops;
2287 } else {
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
re: iommu: Introduce iommu_request_dm_for_dev()
Hello Joerg Roedel,
This is a semi-automatic email about new static checker warnings.
The patch eeae3fba3afe: "iommu: Introduce iommu_request_dm_for_dev()"
from May 28, 2015, leads to the following Smatch complaint:
drivers/iommu/iommu.c:1581 iommu_request_dm_for_dev()
error: we previously assumed 'group->default_domain' could be null
(see line 1558)
drivers/iommu/iommu.c
1557 /* Check if the default domain is already direct mapped */
1558 ret = 0;
1559 if (group->default_domain &&
^
Check for NULL.
1560 group->default_domain->type == IOMMU_DOMAIN_IDENTITY)
1561 goto out;
1562
1563 /* Don't change mappings of existing devices */
1564 ret = -EBUSY;
1565 if (iommu_group_device_count(group) != 1)
1566 goto out;
1567
1568 /* Allocate a direct mapped domain */
1569 ret = -ENOMEM;
1570 dm_domain = __iommu_domain_alloc(dev->bus,
IOMMU_DOMAIN_IDENTITY);
1571 if (!dm_domain)
1572 goto out;
1573
1574 /* Attach the device to the domain */
1575 ret = __iommu_attach_group(dm_domain, group);
1576 if (ret) {
1577 iommu_domain_free(dm_domain);
1578 goto out;
1579 }
1580
1581 /* Make the direct mapped domain the default for this group */
1582 iommu_domain_free(group->default_domain);
^
Dereferenced inside function.
1583 group->default_domain = dm_domain;
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch -next] iommu: checking for NULL instead of IS_ERR
The iommu_group_alloc() and iommu_group_get_for_dev() functions return
error pointers, they never return NULL.
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c520c0c..9c25e6be 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -788,15 +788,16 @@ static struct iommu_group
*iommu_group_get_for_pci_dev(struct pci_dev *pdev)
/* No shared group found, allocate new */
group = iommu_group_alloc();
- if (group) {
- /*
-* Try to allocate a default domain - needs support from the
-* IOMMU driver.
-*/
- group->default_domain = __iommu_domain_alloc(pdev->dev.bus,
-IOMMU_DOMAIN_DMA);
- group->domain = group->default_domain;
- }
+ if (IS_ERR(group))
+ return NULL;
+
+ /*
+* Try to allocate a default domain - needs support from the
+* IOMMU driver.
+*/
+ group->default_domain = __iommu_domain_alloc(pdev->dev.bus,
+IOMMU_DOMAIN_DMA);
+ group->domain = group->default_domain;
return group;
}
@@ -1548,8 +1549,8 @@ int iommu_request_dm_for_dev(struct device *dev)
/* Device must already be in a group before calling this function */
group = iommu_group_get_for_dev(dev);
- if (!group)
- return -EINVAL;
+ if (IS_ERR(group))
+ return PTR_ERR(group);
mutex_lock(&group->mutex);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch] iommu/vt-d: unlock on error in intel_iommu_load_translation_tables()
On Tue, Jun 02, 2015 at 03:45:18PM +0200, Joerg Roedel wrote:
> On Tue, Jun 02, 2015 at 01:09:58PM +0300, Dan Carpenter wrote:
> > Smatch found some error paths that don't unlock. Also we can return
> > -ENOMEM instead of -1 if we don't have an old root entry.
> >
> > Fixes: 5908f10af4b9 ('iommu/vt-d: datatypes and functions used for kdump')
> > Signed-off-by: Dan Carpenter
>
> Applied, thanks.
>
> > Releasing the lock is good, but we might need to do other error handling
> > as well. Presumably this function always succeeds anyway? It seems
> > like it might be essential for booting.
>
> What do you mean by 'other error handling'? In error case a negative
> value is returned and the caller checks that.
I was just worried we should call __iommu_free_mapped_mem() or
something. I don't know this code very well is what I'm saying...
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: unlock on error in intel_iommu_load_translation_tables()
Smatch found some error paths that don't unlock. Also we can return
-ENOMEM instead of -1 if we don't have an old root entry.
Fixes: 5908f10af4b9 ('iommu/vt-d: datatypes and functions used for kdump')
Signed-off-by: Dan Carpenter
---
Releasing the lock is good, but we might need to do other error handling
as well. Presumably this function always succeeds anyway? It seems
like it might be essential for booting.
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2926907..0e2e635 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5151,22 +5151,24 @@ static int intel_iommu_load_translation_tables(struct
intel_iommu *iommu)
iommu->root_entry =
(struct root_entry *)alloc_pgtable_page(iommu->node);
if (!iommu->root_entry) {
- spin_unlock_irqrestore(&iommu->lock, flags);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_unlock;
}
}
iommu->root_entry_old_phys = q & VTD_PAGE_MASK;
if (!iommu->root_entry_old_phys) {
pr_err("Could not read old root entry address.");
- return -1;
+ ret = -ENOMEM;
+ goto err_unlock;
}
iommu->root_entry_old_virt = ioremap_cache(iommu->root_entry_old_phys,
VTD_PAGE_SIZE);
if (!iommu->root_entry_old_virt) {
pr_err("Could not map the old root entry.");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_unlock;
}
__iommu_load_old_root_entry(iommu);
@@ -5179,6 +5181,10 @@ static int intel_iommu_load_translation_tables(struct
intel_iommu *iommu)
__iommu_free_mapped_mem();
return ret;
+
+err_unlock:
+ spin_unlock_irqrestore(&iommu->lock, flags);
+ return ret;
}
static void unmap_device_dma(struct dmar_domain *domain,
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: small cleanup in mn_release()
"pasid_state->device_state" and "dev_state" are the same, but it's nicer to use dev_state consistently. Signed-off-by: Dan Carpenter diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c index 6d5a5c4..a1cbba9 100644 --- a/drivers/iommu/amd_iommu_v2.c +++ b/drivers/iommu/amd_iommu_v2.c @@ -417,7 +417,7 @@ static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm) dev_state = pasid_state->device_state; run_inv_ctx_cb = !pasid_state->invalid; - if (run_inv_ctx_cb && pasid_state->device_state->inv_ctx_cb) + if (run_inv_ctx_cb && dev_state->inv_ctx_cb) dev_state->inv_ctx_cb(dev_state->pdev, pasid_state->pasid); unbind_pasid(pasid_state); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: fix a small leak in irq_remapping_alloc()
There is a memory leak here that was detected by Smatch:
drivers/iommu/amd_iommu.c:4261 irq_remapping_alloc()
warn: possible memory leak of 'data'
It happens if you hit the "if (!irq_data || !cfg) {" on the first
iteration through the loop. The original code was a bit weird. For
example, it treated the first allocation as a special case for some
reason. Anyway I cleaned it up a bit.
Fixes: ecf87b38d902 ('iommu/amd: Enhance AMD IR driver to suppport hierarchy
irqdomain')
Signed-off-by: Dan Carpenter
---
Please review this carefully. I haven't tested it.
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 35a38db..3bb69e4 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -4208,11 +4208,6 @@ static int irq_remapping_alloc(struct irq_domain
*domain, unsigned int virq,
if (ret < 0)
return ret;
- ret = -ENOMEM;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- goto out_free_parent;
-
if (info->type == X86_IRQ_ALLOC_TYPE_IOAPIC) {
if (get_irq_table(devid, true))
index = info->ioapic_pin;
@@ -4223,7 +4218,6 @@ static int irq_remapping_alloc(struct irq_domain *domain,
unsigned int virq,
}
if (index < 0) {
pr_warn("Failed to allocate IRTE\n");
- kfree(data);
goto out_free_parent;
}
@@ -4232,14 +4226,16 @@ static int irq_remapping_alloc(struct irq_domain
*domain, unsigned int virq,
cfg = irqd_cfg(irq_data);
if (!irq_data || !cfg) {
ret = -EINVAL;
- goto out_free_data;
+ goto out_unwind_loop;
}
- if (i > 0) {
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- goto out_free_data;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto out_unwind_loop;
}
+
irq_data->hwirq = (devid << 16) + i;
irq_data->chip_data = data;
irq_data->chip = &amd_ir_chip;
@@ -4248,8 +4244,8 @@ static int irq_remapping_alloc(struct irq_domain *domain,
unsigned int virq,
}
return 0;
-out_free_data:
- for (i--; i >= 0; i--) {
+out_unwind_loop:
+ while (--i >= 0) {
irq_data = irq_domain_get_irq_data(domain, virq + i);
if (irq_data)
kfree(irq_data->chip_data);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: returning free pointer in get_domain_for_dev()
If we hit this error condition then we want to return a NULL pointer and
not a freed variable.
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6fbce01..69fa7da 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2257,6 +2257,7 @@ static struct dmar_domain *get_domain_for_dev(struct
device *dev, int gaw)
goto error;
if (iommu_attach_domain(domain, iommu)) {
free_domain_mem(domain);
+ domain = NULL;
goto error;
}
free = domain;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/vt-d: signedness bug in alloc_irte()
"index" needs to be signed for the error handling to work. I deleted a
little bit of obsolete cruft related to "index" and "start_index" as
well.
Fixes: 360eb3c5687e ('iommu/vt-d: use dedicated bitmap to track remapping entry
allocation status')
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/intel_irq_remapping.c
b/drivers/iommu/intel_irq_remapping.c
index 78fba6212ed2..ef5f65dbafe9 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -71,18 +71,13 @@ static int alloc_irte(struct intel_iommu *iommu, int irq,
u16 count)
struct ir_table *table = iommu->ir_table;
struct irq_2_iommu *irq_iommu = irq_2_iommu(irq);
struct irq_cfg *cfg = irq_get_chip_data(irq);
- u16 index, start_index;
unsigned int mask = 0;
unsigned long flags;
+ int index;
if (!count || !irq_iommu)
return -1;
- /*
-* start the IRTE search from index 0.
-*/
- index = start_index = 0;
-
if (count > 1) {
count = __roundup_pow_of_two(count);
mask = ilog2(count);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: use after free in get_irq_table()
We should return NULL on error instead of the freed pointer.
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index e78b8a4..a636d68 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3867,6 +3867,7 @@ static struct irq_remap_table *get_irq_table(u16 devid,
bool ioapic)
table->table = kmem_cache_alloc(amd_iommu_irq_cache, GFP_ATOMIC);
if (!table->table) {
kfree(table);
+ table = NULL;
goto out;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Request VFIO inclusion in linux-next
On Wed, Jun 27, 2012 at 01:23:23PM -0600, Alex Williamson wrote: > On Wed, 2012-06-27 at 15:37 +0300, Dan Carpenter wrote: > > On Mon, Jun 25, 2012 at 10:55:52PM -0600, Alex Williamson wrote: > > > Hi, > > > > > > VFIO has been kicking around for well over a year now and has been > > > posted numerous times for review. The pre-requirements are finally > > > available in linux-next (or will be in the 20120626 build) so I'd like > > > to request a new branch be included in linux-next with a goal of being > > > accepted into v3.6. > > > > > > > Could you run Sparse over the driver? > > http://lwn.net/Articles/205624/ > > > > It reports a bunch of endian problems. Some are definitely bugs > > like: > > *prev |= cpu_to_le32((u32)epos << 20); > > Done. Mostly just adding annotation, but it did find a bug or two. > Should be cleaned up in tomorrow's next. Thanks for the report, > Cool. I have some Smatch fixes as well. I've updated them to apply on top of today's linux-next. regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Request VFIO inclusion in linux-next
On Mon, Jun 25, 2012 at 10:55:52PM -0600, Alex Williamson wrote: > Hi, > > VFIO has been kicking around for well over a year now and has been > posted numerous times for review. The pre-requirements are finally > available in linux-next (or will be in the 20120626 build) so I'd like > to request a new branch be included in linux-next with a goal of being > accepted into v3.6. > Could you run Sparse over the driver? http://lwn.net/Articles/205624/ It reports a bunch of endian problems. Some are definitely bugs like: *prev |= cpu_to_le32((u32)epos << 20); regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch 2/2 -resend] iommu/amd: fix type bug in flush code
write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory. Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.
Signed-off-by: Dan Carpenter
---
Originally sent on Fri, 2 Mar 2012. This is the other patch where I
guess I was supposed to modify debugfs_create_bool() to take bool
pointers.
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 243..c1b1d48 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -652,7 +652,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
* If true, the addresses will be flushed on unmap time, not when
* they are reused
*/
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
/* Smallest number of PASIDs supported by any IOMMU in the system */
extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4127b56..92b9267 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -451,7 +451,7 @@ static void amd_iommu_stats_init(void)
return;
de_fflush = debugfs_create_bool("fullflush", 0444, stats_dir,
-(u32 *)&amd_iommu_unmap_flush);
+&amd_iommu_unmap_flush);
amd_iommu_stats_add(&compl_wait);
amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c04ddca..a33612f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf; /* largest PCI
device id we have
to handle */
LIST_HEAD(amd_iommu_unity_map);/* a list of required unity
mappings
we find in ACPI */
-bool amd_iommu_unmap_flush;/* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush; /* if true, flush on every unmap */
LIST_HEAD(amd_iommu_list); /* list of all AMD IOMMUs in the
system */
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[patch] iommu/amd: fix type bug in flush code
write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory. Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.
Signed-off-by: Dan Carpenter
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 2452f3b..f0469e0 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -649,7 +649,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
* If true, the addresses will be flushed on unmap time, not when
* they are reused
*/
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
/* Smallest number of PASIDs supported by any IOMMU in the system */
extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index daa333f..f5fbce2 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -402,7 +402,7 @@ static void amd_iommu_stats_init(void)
return;
de_fflush = debugfs_create_bool("fullflush", 0444, stats_dir,
-(u32 *)&amd_iommu_unmap_flush);
+&amd_iommu_unmap_flush);
amd_iommu_stats_add(&compl_wait);
amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index bdea288..7d159fc 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf; /* largest PCI
device id we have
to handle */
LIST_HEAD(amd_iommu_unity_map);/* a list of required unity
mappings
we find in ACPI */
-bool amd_iommu_unmap_flush;/* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush; /* if true, flush on every unmap */
LIST_HEAD(amd_iommu_list); /* list of all AMD IOMMUs in the
system */
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
