| 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
| ||
|
Message-ID: <4FBC2916.5000305@kernel.org>
Date: Wed, 23 May 2012 09:02:30 +0900
From: Minchan Kim <minchan@...nel.org>
To: Seth Jennings <sjenning@...ux.vnet.ibm.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>,
Dan Magenheimer <dan.magenheimer@...cle.com>,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
Nitin Gupta <ngupta@...are.org>
Subject: Re: [PATCH] zsmalloc: use unsigned long instead of void *
On 05/21/2012 11:19 PM, Seth Jennings wrote:
> On 05/20/2012 09:23 PM, Minchan Kim wrote:
>
>> We should use unsigned long as handle instead of void * to avoid any
>> confusion. Without this, users may just treat zs_malloc return value as
>> a pointer and try to deference it.
>
>
> I wouldn't have agreed with you about the need for this change as people
> should understand a void * to be the address of some data with unknown
> structure.
>
> However, I recently discussed with Dan regarding his RAMster project
> where he assumed that the void * would be an address, and as such,
> 4-byte aligned. So he has masked two bits into the two LSBs of the
> handle for RAMster, which doesn't work with zsmalloc since the handle is
> not an address.
>
> So really we do need to convey as explicitly as possible to the user
> that the handle is an _opaque_ value about which no assumption can be made.
>
> Also, I wanted to test this but is doesn't apply cleanly on
> zsmalloc-main.c on v3.4 or what I have as your latest patch series.
> What is the base for this patch?
It's based on next-20120518.
I have always used linux-next tree for staging.
Greg, What's the convenient tree for you?
Maybe I will resend next spin based on v3.4 today
I hope it doesn't hurt you.
>
>>
>> This patch passed compile test(zram, zcache and ramster) and zram is
>> tested on qemu.
>>
>> Cc: Seth Jennings <sjenning@...ux.vnet.ibm.com>
>> Cc: Dan Magenheimer <dan.magenheimer@...cle.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
>> Cc: Nitin Gupta <ngupta@...are.org>
>> Signed-off-by: Minchan Kim <minchan@...nel.org>
>> ---
>>
>> Nitin, Konrad and I discussed and concluded that we should use 'unsigned long'
>> instead of 'void *'.
>> Look at the lengthy thread if you have a question.
>> http://marc.info/?l=linux-mm&m=133716653118566&w=4
>> Watch out! it has number of noises.
>>
>> drivers/staging/zcache/zcache-main.c | 12 ++++++------
>> drivers/staging/zram/zram_drv.c | 16 ++++++++--------
>> drivers/staging/zram/zram_drv.h | 2 +-
>> drivers/staging/zsmalloc/zsmalloc-main.c | 24 ++++++++++++------------
>> drivers/staging/zsmalloc/zsmalloc.h | 8 ++++----
>> 5 files changed, 31 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
>> index 2734dac..4c218a7 100644
>> --- a/drivers/staging/zcache/zcache-main.c
>> +++ b/drivers/staging/zcache/zcache-main.c
>> @@ -700,7 +700,7 @@ static struct zv_hdr *zv_create(struct zs_pool *pool, uint32_t pool_id,
>> struct zv_hdr *zv;
>> u32 size = clen + sizeof(struct zv_hdr);
>> int chunks = (size + (CHUNK_SIZE - 1)) >> CHUNK_SHIFT;
>> - void *handle = NULL;
>> + unsigned long handle = 0;
>>
>> BUG_ON(!irqs_disabled());
>> BUG_ON(chunks >= NCHUNKS);
>> @@ -718,10 +718,10 @@ static struct zv_hdr *zv_create(struct zs_pool *pool, uint32_t pool_id,
>> memcpy((char *)zv + sizeof(struct zv_hdr), cdata, clen);
>> zs_unmap_object(pool, handle);
>> out:
>> - return handle;
>> + return (struct zv_hdr *)handle;
>
>
> This is kind of weird, and somewhat defeats the point, casting it back
> to a pointer. I know you'd have to change it all the way up the stack.
> Just saying.
Okay.
>
>> }
>>
>> -static void zv_free(struct zs_pool *pool, void *handle)
>> +static void zv_free(struct zs_pool *pool, unsigned long handle)
>> {
>> unsigned long flags;
>> struct zv_hdr *zv;
>> @@ -743,7 +743,7 @@ static void zv_free(struct zs_pool *pool, void *handle)
>> local_irq_restore(flags);
>> }
>>
>> -static void zv_decompress(struct page *page, void *handle)
>> +static void zv_decompress(struct page *page, unsigned long handle)
>> {
>> unsigned int clen = PAGE_SIZE;
>> char *to_va;
>> @@ -1247,7 +1247,7 @@ static int zcache_pampd_get_data(char *data, size_t *bufsize, bool raw,
>> int ret = 0;
>>
>> BUG_ON(is_ephemeral(pool));
>> - zv_decompress((struct page *)(data), pampd);
>> + zv_decompress((struct page *)(data), (unsigned long)pampd);
>> return ret;
>> }
>>
>> @@ -1282,7 +1282,7 @@ static void zcache_pampd_free(void *pampd, struct tmem_pool *pool,
>> atomic_dec(&zcache_curr_eph_pampd_count);
>> BUG_ON(atomic_read(&zcache_curr_eph_pampd_count) < 0);
>> } else {
>> - zv_free(cli->zspool, pampd);
>> + zv_free(cli->zspool, (unsigned long)pampd);
>> atomic_dec(&zcache_curr_pers_pampd_count);
>> BUG_ON(atomic_read(&zcache_curr_pers_pampd_count) < 0);
>> }
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index 685d612..abd69d1 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -135,7 +135,7 @@ static void zram_set_disksize(struct zram *zram, size_t totalram_bytes)
>>
>> static void zram_free_page(struct zram *zram, size_t index)
>> {
>> - void *handle = zram->table[index].handle;
>> + unsigned long handle = zram->table[index].handle;
>
>
> Should we incorporate the union { handle, page } idea we were working on
> earlier before doing this? Might cut down on some the casting below.
Yes. It should be another patch and I don't care it's applied based on
this patch or reverse.
I will try it.
>
>>
>> if (unlikely(!handle)) {
>> /*
>> @@ -150,7 +150,7 @@ static void zram_free_page(struct zram *zram, size_t index)
>> }
>>
>> if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED))) {
>> - __free_page(handle);
>> + __free_page((struct page *)handle);
>> zram_clear_flag(zram, index, ZRAM_UNCOMPRESSED);
>> zram_stat_dec(&zram->stats.pages_expand);
>> goto out;
>> @@ -166,7 +166,7 @@ out:
>> zram->table[index].size);
>> zram_stat_dec(&zram->stats.pages_stored);
>>
>> - zram->table[index].handle = NULL;
>> + zram->table[index].handle = 0;
>> zram->table[index].size = 0;
>> }
>>
>> @@ -189,7 +189,7 @@ static void handle_uncompressed_page(struct zram *zram, struct bio_vec *bvec,
>> unsigned char *user_mem, *cmem;
>>
>> user_mem = kmap_atomic(page);
>> - cmem = kmap_atomic(zram->table[index].handle);
>> + cmem = kmap_atomic((struct page *)zram->table[index].handle);
>>
>> memcpy(user_mem + bvec->bv_offset, cmem + offset, bvec->bv_len);
>> kunmap_atomic(cmem);
>> @@ -317,7 +317,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> int ret;
>> u32 store_offset;
>> size_t clen;
>> - void *handle;
>> + unsigned long handle;
>> struct zobj_header *zheader;
>> struct page *page, *page_store;
>> unsigned char *user_mem, *cmem, *src, *uncmem = NULL;
>> @@ -399,7 +399,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> store_offset = 0;
>> zram_set_flag(zram, index, ZRAM_UNCOMPRESSED);
>> zram_stat_inc(&zram->stats.pages_expand);
>> - handle = page_store;
>> + handle = (unsigned long)page_store;
>> src = kmap_atomic(page);
>> cmem = kmap_atomic(page_store);
>> goto memstore;
>> @@ -592,12 +592,12 @@ void __zram_reset_device(struct zram *zram)
>>
>> /* Free all pages that are still in this zram device */
>> for (index = 0; index < zram->disksize >> PAGE_SHIFT; index++) {
>> - void *handle = zram->table[index].handle;
>> + unsigned long handle = zram->table[index].handle;
>> if (!handle)
>> continue;
>>
>> if (unlikely(zram_test_flag(zram, index, ZRAM_UNCOMPRESSED)))
>> - __free_page(handle);
>> + __free_page((struct page *)handle);
>> else
>> zs_free(zram->mem_pool, handle);
>> }
>> diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
>> index fbe8ac9..7a7e256 100644
>> --- a/drivers/staging/zram/zram_drv.h
>> +++ b/drivers/staging/zram/zram_drv.h
>> @@ -81,7 +81,7 @@ enum zram_pageflags {
>>
>> /* Allocated for each disk page */
>> struct table {
>> - void *handle;
>> + unsigned long handle;
>
>
> Putting the union here, as mentioned above.
>
>> u16 size; /* object size (excluding header) */
>> u8 count; /* object ref count (not yet used) */
>> u8 flags;
>
> <snip>
>
> Thanks,
> Seth
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
>
--
Kind regards,
Minchan Kim
--
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