linux-kernel - Re: [PATCH] swiotlb-xen: override common mmap and get

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Jun 2021 15:47:25 +0300
From:   Roman Skakun <rm.skakun@...il.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:     Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        xen-devel@...ts.xenproject.org, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>,
        Volodymyr Babchuk <volodymyr_babchuk@...m.com>,
        Roman Skakun <roman_skakun@...m.com>,
        Andrii Anisov <andrii_anisov@...m.com>
Subject: Re: [PATCH] swiotlb-xen: override common mmap and get_sgtable dma ops

Hey, Boris!
Thanks for the review.

I have an additional question about current implementation that disturbed me.
I think, that we can have cases when mapped memory can not be
physically contiguous.
In order to proceed this correctly need to apply some additional steps
to current implementation as well.

In mmap() :
1. Is the memory region physically contiguous?
2. Remap multiple ranges if it is not.

In get_sgtable() :
1. Is the memory region physically contiguous?
2. Create sgt that will be included multiple contiguous ranges if it is not.

What do you think about it?

Cheers!
Roman


пт, 11 июн. 2021 г. в 18:20, Boris Ostrovsky <boris.ostrovsky@...cle.com>:
>
>
> On 6/11/21 5:55 AM, Roman Skakun wrote:
> >
> > +static int
> > +xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > +             void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > +             unsigned long attrs)
> > +{
> > +     unsigned long user_count = vma_pages(vma);
> > +     unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> > +     unsigned long off = vma->vm_pgoff;
> > +     struct page *page;
> > +
> > +     if (is_vmalloc_addr(cpu_addr))
> > +             page = vmalloc_to_page(cpu_addr);
> > +     else
> > +             page = virt_to_page(cpu_addr);
> > +
> > +     vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
> > +
> > +     if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
> > +             return -ENXIO;
> > +
> > +     if (off >= count || user_count > count - off)
> > +             return -ENXIO;
> > +
> > +     return remap_pfn_range(vma, vma->vm_start,
> > +                     page_to_pfn(page) + vma->vm_pgoff,
> > +                     user_count << PAGE_SHIFT, vma->vm_page_prot);
> > +}
>
>
> I suggest you create a helper for computing page value and then revert 922659ea771b3fd728149262c5ea15608fab9719 and pass result of the helper instead of cpu_addr. Here and in xen_swiotlb_dma_get_sgtable().
>
>
> And use this new helper in xen_swiotlb_free_coherent() too. I am curious though why this was not a problem when Stefano was looking at the problem that introduced this vmalloc check (i.e. 8b1e868f66076490189a36d984fcce286cdd6295). Stefano?
>
>
> -boris



-- 
Best Regards, Roman.