Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1735886 > unrolled thread

Re: [PATCH v5 03/10] swiotlb: Map the buffer if it was unmapped by XPFO

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 03/10] swiotlb: Map the buffer if it was unmapped by  XPFO Dave Hansen <dave.hansen@intel.com> - 2017-09-20 18:30 +0200
    Re: [PATCH v5 03/10] swiotlb: Map the buffer if it was unmapped by  XPFO Tycho Andersen <tycho@docker.com> - 2017-09-21 00:50 +0200
      Re: [PATCH v5 03/10] swiotlb: Map the buffer if it was unmapped by  XPFO Dave Hansen <dave.hansen@intel.com> - 2017-09-21 01:30 +0200

#1735886 — Re: [PATCH v5 03/10] swiotlb: Map the buffer if it was unmapped by XPFO

On 08/09/2017 01:07 PM, Tycho Andersen wrote:
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -420,8 +420,9 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
>  {
>  	unsigned long pfn = PFN_DOWN(orig_addr);
>  	unsigned char *vaddr = phys_to_virt(tlb_addr);
> +	struct page *page = pfn_to_page(pfn);
>  
> -	if (PageHighMem(pfn_to_page(pfn))) {
> +	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {
>  		/* The buffer does not have a mapping.  Map it in and copy */
>  		unsigned int offset = orig_addr & ~PAGE_MASK;
>  		char *buffer;

This is a little scary.  I wonder how many more of these are in the
kernel, like:

> static inline void *skcipher_map(struct scatter_walk *walk)
> {
>         struct page *page = scatterwalk_page(walk);
> 
>         return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
>                offset_in_page(walk->offset);
> }

Is there any better way to catch these?  Like, can we add some debugging
to check for XPFO pages in __va()?

[toc] | [next] | [standalone]

#1736216

On Wed, Sep 20, 2017 at 09:19:56AM -0700, Dave Hansen wrote:
> On 08/09/2017 01:07 PM, Tycho Andersen wrote:
> > --- a/lib/swiotlb.c
> > +++ b/lib/swiotlb.c
> > @@ -420,8 +420,9 @@ static void swiotlb_bounce(phys_addr_t orig_addr, phys_addr_t tlb_addr,
> >  {
> >  	unsigned long pfn = PFN_DOWN(orig_addr);
> >  	unsigned char *vaddr = phys_to_virt(tlb_addr);
> > +	struct page *page = pfn_to_page(pfn);
> >  
> > -	if (PageHighMem(pfn_to_page(pfn))) {
> > +	if (PageHighMem(page) || xpfo_page_is_unmapped(page)) {
> >  		/* The buffer does not have a mapping.  Map it in and copy */
> >  		unsigned int offset = orig_addr & ~PAGE_MASK;
> >  		char *buffer;
> 
> This is a little scary.  I wonder how many more of these are in the
> kernel, like:

I don't know, but I assume several :)

> > static inline void *skcipher_map(struct scatter_walk *walk)
> > {
> >         struct page *page = scatterwalk_page(walk);
> > 
> >         return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
> >                offset_in_page(walk->offset);
> > }
> 
> Is there any better way to catch these?  Like, can we add some debugging
> to check for XPFO pages in __va()?

Yes, and perhaps also a debugging check in PageHighMem? Would __va
have caught either of the two cases you've pointed out?

Tycho

[toc] | [prev] | [next] | [standalone]

#1736237

On 09/20/2017 03:47 PM, Tycho Andersen wrote:
> 
>>> static inline void *skcipher_map(struct scatter_walk *walk)
>>> {
>>>         struct page *page = scatterwalk_page(walk);
>>>
>>>         return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
>>>                offset_in_page(walk->offset);
>>> }
>> Is there any better way to catch these?  Like, can we add some debugging
>> to check for XPFO pages in __va()?
> Yes, and perhaps also a debugging check in PageHighMem?

I'm not sure what PageHighMem() would check.  It's OK to use as long as
you don't depend on the contents of the page.
		
> Would __va have caught either of the two cases you've pointed out?
Yes.  __va() is what is eventually called by lowmem_page_address(),
which is only OK to call on things that are actually mapped into the kernel.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web