| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Ytov9VdwIv5HVsbr@infradead.org>
Date: Thu, 21 Jul 2022 22:04:53 -0700
From: "hch@...radead.org" <hch@...radead.org>
To: "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
Cc: Tianyu Lan <ltykernel@...il.com>,
"hch@...radead.org" <hch@...radead.org>,
"m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
"robin.murphy@....com" <robin.murphy@....com>,
Tianyu Lan <Tianyu.Lan@...rosoft.com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>
Subject: Re: [PATCH] swiotlb: Clean up some coding style and minor issues
On Fri, Jul 22, 2022 at 04:03:21AM +0000, Michael Kelley (LINUX) wrote:
> I think you missed one of the bugs I pointed out in my previous
> comments. In the function rmem_swiotlb_device_init(), the two
> calls to kfree() in the error path are in the wrong order. It's a
> path that will probably never happen, but it still should be fixed.
That is already fixed in the dma-mapping tree:
http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/4a97739474c402e0a14cf6a432f1920262f6811c
> The other fixes look good to me.
Can you make that a formal Reviewed-by?
>
> Michael
>
> > Fixes: 26ffb91fa5e0 ("swiotlb: split up the global swiotlb lock")
> > Signed-off-by: Tianyu Lan <tiala@...rosoft.com>
> > ---
> > .../admin-guide/kernel-parameters.txt | 3 +-
> > kernel/dma/swiotlb.c | 42 ++++++++++++-------
> > 2 files changed, 30 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 4a6ad177d4b8..ddca09550f76 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5907,7 +5907,8 @@
> > Format: { <int> [,<int>] | force | noforce }
> > <int> -- Number of I/O TLB slabs
> > <int> -- Second integer after comma. Number of swiotlb
> > - areas with their own lock. Must be power of 2.
> > + areas with their own lock. Will be rounded up
> > + to a power of 2.
> > force -- force using of bounce buffers even if they
> > wouldn't be automatically used by the kernel
> > noforce -- Never use bounce buffers (for debugging)
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c39483bf067d..5752db98a1f2 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -96,7 +96,13 @@ struct io_tlb_slot {
> >
> > static void swiotlb_adjust_nareas(unsigned int nareas)
> > {
> > - if (!is_power_of_2(nareas))
> > + /*
> > + * Set area number to 1 when input area number
> > + * is zero.
> > + */
> > + if (!nareas)
> > + nareas = 1;
> > + else if (!is_power_of_2(nareas))
> > nareas = roundup_pow_of_two(nareas);
> >
> > default_nareas = nareas;
> > @@ -270,6 +276,7 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem,
> > phys_addr_t start,
> > for (i = 0; i < mem->nareas; i++) {
> > spin_lock_init(&mem->areas[i].lock);
> > mem->areas[i].index = 0;
> > + mem->areas[i].used = 0;
> > }
> >
> > for (i = 0; i < mem->nslabs; i++) {
> > @@ -353,8 +360,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned
> > int flags,
> > panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> > __func__, alloc_size, PAGE_SIZE);
> >
> > - mem->areas = memblock_alloc(sizeof(struct io_tlb_area) *
> > - default_nareas, SMP_CACHE_BYTES);
> > + mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
> > + default_nareas), SMP_CACHE_BYTES);
> > if (!mem->areas)
> > panic("%s: Failed to allocate mem->areas.\n", __func__);
> >
> > @@ -479,7 +486,7 @@ void __init swiotlb_exit(void)
> > free_pages((unsigned long)mem->slots, get_order(slots_size));
> > } else {
> > memblock_free_late(__pa(mem->areas),
> > - mem->nareas * sizeof(struct io_tlb_area));
> > + array_size(sizeof(*mem->areas), mem->nareas));
> > memblock_free_late(mem->start, tbl_size);
> > memblock_free_late(__pa(mem->slots), slots_size);
> > }
> > @@ -593,11 +600,12 @@ static unsigned int wrap_area_index(struct io_tlb_mem
> > *mem, unsigned int index)
> > * Find a suitable number of IO TLB entries size that will fit this request and
> > * allocate a buffer from that IO TLB pool.
> > */
> > -static int swiotlb_do_find_slots(struct io_tlb_mem *mem,
> > - struct io_tlb_area *area, int area_index,
> > - struct device *dev, phys_addr_t orig_addr,
> > +static int swiotlb_do_find_slots(struct device *dev,
> > + int area_index, phys_addr_t orig_addr,
> > size_t alloc_size, unsigned int alloc_align_mask)
> > {
> > + struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > + struct io_tlb_area *area = mem->areas + area_index;
> > unsigned long boundary_mask = dma_get_seg_boundary(dev);
> > dma_addr_t tbl_dma_addr =
> > phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> > @@ -686,13 +694,12 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t
> > orig_addr,
> > size_t alloc_size, unsigned int alloc_align_mask)
> > {
> > struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > - int start = raw_smp_processor_id() & ((1U << __fls(mem->nareas)) - 1);
> > + int start = raw_smp_processor_id() & (mem->nareas - 1);
> > int i = start, index;
> >
> > do {
> > - index = swiotlb_do_find_slots(mem, mem->areas + i, i,
> > - dev, orig_addr, alloc_size,
> > - alloc_align_mask);
> > + index = swiotlb_do_find_slots(dev, i, orig_addr,
> > + alloc_size, alloc_align_mask);
> > if (index >= 0)
> > return index;
> > if (++i >= mem->nareas)
> > @@ -903,17 +910,24 @@ bool is_swiotlb_active(struct device *dev)
> > }
> > EXPORT_SYMBOL_GPL(is_swiotlb_active);
> >
> > +static int io_tlb_used_get(void *data, u64 *val)
> > +{
> > + *val = mem_used(&io_tlb_default_mem);
> > +
> > + return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_io_tlb_used, io_tlb_used_get, NULL, "%llu\n");
> > +
> > static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem,
> > const char *dirname)
> > {
> > - unsigned long used = mem_used(mem);
> > -
> > mem->debugfs = debugfs_create_dir(dirname, io_tlb_default_mem.debugfs);
> > if (!mem->nslabs)
> > return;
> >
> > debugfs_create_ulong("io_tlb_nslabs", 0400, mem->debugfs, &mem->nslabs);
> > - debugfs_create_ulong("io_tlb_used", 0400, mem->debugfs, &used);
> > + debugfs_create_file_unsafe("io_tlb_used", 0400, mem->debugfs, NULL,
> > + &fops_io_tlb_used);
> > }
> >
> > static int __init __maybe_unused swiotlb_create_default_debugfs(void)
> > --
> > 2.25.1
>
---end quoted text---
Powered by blists - more mailing lists