linux-kernel - Re: [PATCH] mtd: fix mtd_blkdevs problem with caches on some architectures (2.6.31)

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]

Message-Id: <20091122021128.db47e202.isloginov@gmail.com>
Date:	Sun, 22 Nov 2009 02:11:28 +0300
From:	Ilya Loginov <isloginov@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	linux-kernel@...r.kernel.org, Peter Horton <phorton@...box.co.uk>,
	"Ed L. Cashin" <ecashin@...aid.com>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [PATCH] mtd: fix mtd_blkdevs problem with caches on some
 architectures (2.6.31)

> The one somewhat fragile thing is that we'll end up in a situation
> where an architecture could implement a real flush_dcache_page(), but
> would forget to set SOMETHING_LIKE_CONFIG_CPU_HAS_DCACHE_ALIAS.  Or
> vice versa.  To make things reliable it would be good to cause a
> compilation failure in that case.

Yes, I do understand this.

> One way of addressing this is to
> 
> - change every arch/*/include/asm/cacheflush.h to include
> asm-generic/cacheflush.h
> 
> - put #ifndef SOMETHING_LIKE_CONFIG_CPU_HAS_DCACHE_ALIAS wrappers
> around all content in asm-generic/cacheflush
> 
> So it the above mistake happens, we'll get lots of duplicated
> definitions, or no definitions at all (I think).

Actually I don't think that it is the best solution. Let me explain.

1) The problem is that flush_dcache_page is defined for a bunch of
different architectures. (x86, ia64, etc). But in most of the cases
flush_dcache_page is empty, but should be defined anyway. Just see the
review:

	h8300 -- empty. All cache defines like asm-generic, but worse
(without do {} while(0)).
	blackfin -- empty for architectures which do not define
CONFIG_BFIN_EXTMEM_WRITEBACK or CONFIG_BFIN_L2_WRITEBACK.
	frv
	x86 -- empty. All cache defines like asm-generic, but it was
implemented throught inline.
	ia64
	sh -- in case if ARCH_HAS_FLUSH_KERNEL_DCACHE_PAGE was defined
most flash operations are empty, but there no this define anywhere in
kernel. 
	alpha -- empty, like most cache operations.
	avr32 -- empty. But many other operations are defined.
	arm
	sparc
	powerpc
	m32r -- empty for all chips, like most kernel operations.
	microblaze -- empty.
	cris -- empty. cacheflush is equal to asm-generic, except
define change_page_attr function.
	s390 -- empty. cacheflush is equal to asm-generic, except
define kernel_map_pages function if CONFIG_DEBUG_PAGEALLOC was defined.
	mips -- different for different boards and architectures.
	parisc 
	mn10300 -- empty. But there are functions like
mn10300_dcache_flush_page. I think this is bad.
	m68k -- empty if __uClinux__ is using.
	xtensa -- empty if DCACHE_WAY_SIZE < PAGE_SIZE

So you see, it is a real mess. Only in 8 of 20 architectures
this call don't empty! 

2) In the asm-generic/cacheflush.h also was defined a lot of functions
that could have a conflict and we don't want to have this conflict
anyway. So we will meet here the same problem that was described in you
letter:

> we'll get lots of duplicated
> definitions, or no definitions at all

3) I want to mention that flush_dcache_page depends on the type of
process and chipset as well, so in order to make such things workable
developers usually set different flags in Kconfig. For example
SYS_HAS_EARLY_PRINTK or DMA_NONCOHERENT. And this is the most effective
way of doing this, as far as I see this.

I've just checked most of the places in the kernel where
flush_dcache_page is used. See the summary:

1)
drivers/staging/rspiusb/rspiusb.c

for (i = 0; i < pdx->maplist_numPagesMapped[frameInfo]; i++)
	flush_dcache_page(maplist_p[i]);

2)(is equal to 3))
fs/reiserfs/super.c
fs/reiserfs/inode.c
fs/ufs/super.c
fs/ext2/super.c
fs/nilfs2/mdt.c
fs/jfs/super.c
fs/ocfs2/quota_global.c
fs/ext3/super.c
fs/ext4/super.c

This operation is executed into a loop (big enought).
flush_dcache_page(bh->b_page);

3)
in all other cases there is

flush_dcache_page(page);

4)*
fs/bio.c
mm/bounce.c
net/sunrpc/xprtrdma/rpc_rdma.c

These files call flush_dcache_page in bio_for_each_segment. But
there are another work. So we have no pointless empty loops.

I think we should select SYS_HAS_FLUSH_DCACHE_PAGE for those
architectures which requires it(to fix the bug) (and implement empty
flush_dcache_page throught inline like in x86(to avoid pointless do
{} while(0))).

What do you think about this?

-- 
Ilya Loginov <isloginov@...il.com>
--
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/