[patch] exofs: confusion between kmap() and kmap_atomic() api

[patch] exofs: confusion between kmap() and kmap_atomic() api

[Dan Carpenter]

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@gmail.com>

diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
index 4cfab1c..d91e9d8 100644
--- a/fs/exofs/dir.c
+++ b/fs/exofs/dir.c
@@ -608,7 +608,7 @@ int exofs_make_empty(struct inode *inode, struct inode *parent)
 	de->inode_no = cpu_to_le64(parent->i_ino);
 	memcpy(de->name, PARENT_DIR, sizeof(PARENT_DIR));
 	exofs_set_de_type(de, inode);
-	kunmap_atomic(page, KM_USER0);
+	kunmap_atomic(kaddr, KM_USER0);
 	err = exofs_commit_chunk(page, 0, chunk_size);
 fail:
 	page_cache_release(page);

Re: [patch] exofs: confusion between kmap() and kmap_atomic() api

[Boaz Harrosh]

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@gmail.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?

Thanks again
Boaz

> diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
> index 4cfab1c..d91e9d8 100644
> --- a/fs/exofs/dir.c
> +++ b/fs/exofs/dir.c
> @@ -608,7 +608,7 @@ int exofs_make_empty(struct inode *inode, struct inode *parent)
>  	de->inode_no = cpu_to_le64(parent->i_ino);
>  	memcpy(de->name, PARENT_DIR, sizeof(PARENT_DIR));
>  	exofs_set_de_type(de, inode);
> -	kunmap_atomic(page, KM_USER0);
> +	kunmap_atomic(kaddr, KM_USER0);
>  	err = exofs_commit_chunk(page, 0, chunk_size);
>  fail:
>  	page_cache_release(page);

Re: [patch] exofs: confusion between kmap() and kmap_atomic() api

[Dan Carpenter]

On Sun, May 09, 2010 at 01:16:38PM +0300, Boaz Harrosh 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@gmail.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?
> 

I've looked at it, and I think you're right but I'm not the guy to ask...

I saw somenoe else fixing these and wrote a smatch check is all.  I'm at
the "Monkey see, monkey do" level of kernel hacking.

regards,
dan carpenter

 

Re: [patch] exofs: confusion between kmap() and kmap_atomic() api

[Andrew Morton]

On Sun, 09 May 2010 13:16:38 +0300
Boaz Harrosh <bharrosh@panasas.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@gmail.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@kernel.org>" to the changelog if you have
expectations that anyone will try to run it on i386.

Re: [patch] exofs: confusion between kmap() and kmap_atomic() api

[Boaz Harrosh]

On 05/11/2010 02:03 AM, Andrew Morton wrote:
> On Sun, 09 May 2010 13:16:38 +0300
> Boaz Harrosh <bharrosh@panasas.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@gmail.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@kernel.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@kernel.org>
to the patch. Thanks.

Boaz