linux-kernel - Re: [PATCH 4/9] exofs: address_space

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 31 Dec 2008 17:35:44 +0200
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	avishay@...il.com, jeff@...zik.org, viro@...IV.linux.org.uk,
	linux-fsdevel@...r.kernel.org, osd-dev@...n-osd.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/9] exofs: address_space_operations

Andrew Morton wrote:
>> +
>> +	kaddr = page_address(page);
>> +
>> +	req = prepare_osd_write(sbi->s_dev, sbi->s_pid,
>> +			      inode->i_ino + EXOFS_OBJ_OFF, len, start, 0,
>> +			      kaddr);
> 
> Does prepare_osd_write() modify the memory at *kaddr?  If so, does it
> do the needed flush_dcache_page()?
> 

kaddr is not modified by CPU. This is just a very BAD API left from the old
osd-initiator days.The address is used for preparing a BIO and submitted
to HW later. I will change all these places to receive a page* directly.
(It was meant to be changed in the future where I want to support read/write 
 page* array).

<snip>
>> +	} else if (ret == -EFAULT) {
>> +		char *kaddr;
>> +
>> +		/* In this case we were trying to read something that wasn't on
>> +		 * disk yet - return a page full of zeroes.  This should be OK,
>> +		 * because the object should be empty (if there was a write
>> +		 * before this read, the read would be waiting with the page
>> +		 * locked */
>> +		kaddr = page_address(page);
>> +		memset(kaddr, 0, PAGE_CACHE_SIZE);
> 
> There is I think a missing flsh_dcache_page() here.  Use of the
> (somewhat misnamed) zero_user() would be an appropriate fix and
> cleanup.
> 

What happened here is that the HW actually never touched the page in question,
and it is returned to CPU, do I need to flsh_dcache_page anyway?

But this is not relevant since I will use zero_user() as you suggested.
Should I use clear_highpage as this is a clear of a full page?

<snip>
>> +
>> +	/* this will be out of bounds, or doesn't exist yet */
>> +	if ((page->index >= end_index + 1) || !ObjCreated(oi) || !amount
>> +	    /*|| (i_start >= oi->i_commit_size)*/) {
>> +		kaddr = kmap_atomic(page, KM_USER0);
>> +		memset(kaddr, 0, PAGE_CACHE_SIZE);
>> +		flush_dcache_page(page);
>> +		kunmap_atomic(page, KM_USER0);
> 
> There's a flush_dcache_page() ;)
> 
> Could use clear_highpage() here.
> 

Thanks, sounds much better.

>> +		SetPageUptodate(page);
>> +		if (PageError(page))
>> +			ClearPageError(page);
>> +		if (is_async_unlock)
>> +			unlock_page(page);
>> +		goto out;
>> +	}
>> +
>> +	if (amount != PAGE_CACHE_SIZE) {
>> +		kaddr = kmap_atomic(page, KM_USER0);
>> +		memset(kaddr + amount, 0, PAGE_CACHE_SIZE - amount);
>> +		flush_dcache_page(page);
>> +		kunmap_atomic(page, KM_USER0);
> 
> Use zero_user()?
> 

Will change

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/