[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4BE90FE5.9050609@panasas.com>
Date: Tue, 11 May 2010 11:05:57 +0300
From: Boaz Harrosh <bharrosh@...asas.com>
To: Andrew Morton <akpm@...ux-foundation.org>
CC: Dan Carpenter <error27@...il.com>,
Benny Halevy <bhalevy@...asas.com>,
Avishay Traeger <avishay@...il.com>, osd-dev@...n-osd.org,
linux-kernel@...r.kernel.org
Subject: Re: [patch] exofs: confusion between kmap() and kmap_atomic() api
On 05/11/2010 02:03 AM, Andrew Morton wrote:
> On Sun, 09 May 2010 13:16:38 +0300
> Boaz Harrosh <bharrosh@...asas.com> wrote:
>
>> On 05/07/2010 12:05 PM, Dan Carpenter wrote:
>>> For kmap_atomic() we call kunmap_atomic() on the returned pointer.
>>> That's different from kmap() and kunmap() and so it's easy to get them
>>> backwards.
>>>
>>> Signed-off-by: Dan Carpenter <error27@...il.com>
>>>
>>
>> Thank you Dan, I'll push it ASAP.
>
>> Looks like a bad bug. So this is actually a leak, right? kunmap_atomic
>> would detect the bad pointer and do nothing?
>
> void kunmap_atomic(void *kvaddr, enum km_type type)
> {
> unsigned long vaddr = (unsigned long) kvaddr & PAGE_MASK;
> enum fixed_addresses idx = type + KM_TYPE_NR*smp_processor_id();
>
> /*
> * Force other mappings to Oops if they'll try to access this pte
> * without first remap it. Keeping stale mappings around is a bad idea
> * also, in case the page changes cacheability attributes or becomes
> * a protected page in a hypervisor.
> */
> if (vaddr == __fix_to_virt(FIX_KMAP_BEGIN+idx))
> kpte_clear_flush(kmap_pte-idx, vaddr);
> else {
> #ifdef CONFIG_DEBUG_HIGHMEM
> BUG_ON(vaddr < PAGE_OFFSET);
> BUG_ON(vaddr >= (unsigned long)high_memory);
> #endif
> }
>
> pagefault_enable();
> }
>
> if CONFIG_DEBUG_HIGHMEM=y, kunmap_atomic() will go BUG.
>
> if CONFIG_DEBUG_HIGHMEM=n, kunmap_atomic() will do nothing, leaving the
> pte pointing at the old page. Next time someone tries to use that
> kmap_atomic() slot,
>
> void *kmap_atomic_prot(struct page *page, enum km_type type, pgprot_t prot)
> {
> enum fixed_addresses idx;
> unsigned long vaddr;
>
> /* even !CONFIG_PREEMPT needs this, for in_atomic in do_page_fault */
> pagefault_disable();
>
> if (!PageHighMem(page))
> return page_address(page);
>
> debug_kmap_atomic(type);
>
> idx = type + KM_TYPE_NR*smp_processor_id();
> vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
> BUG_ON(!pte_none(*(kmap_pte-idx)));
> set_pte(kmap_pte-idx, mk_pte(page, prot));
>
> return (void *)vaddr;
> }
>
> kmap_atomic_prot() will go BUG because the pte wasn't cleared.
>
>
> I can only assume that this code has never been run on i386. I'd suggest
> adding a "Cc: <stable@...nel.org>" to the changelog if you have
> expectations that anyone will try to run it on i386.
>
Right! Everyone I know runs 64bit. I will add the Cc: <stable@...nel.org>
to the patch. Thanks.
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists