From: Arnd Bergmann <arnd@arndb.de>
To: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
        linux-arm-kernel@lists.infradead.org, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mtd: avoid stack overflow in MTD CFI code
Date: Fri, 18 Mar 2016 21:25:23 +0100
Message-ID: <10357051.EVa2c1MaZl@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <20160318174419.GA70362@google.com>
References: <1456748436-522413-1-git-send-email-arnd@arndb.de> <2259988.Lj3BC6sPv6@wuerfel> <20160318174419.GA70362@google.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2586
Lines: 53

On Friday 18 March 2016 10:44:19 Brian Norris wrote:
> On Sat, Mar 05, 2016 at 12:25:24AM +0100, Arnd Bergmann wrote:
> > On Friday 04 March 2016 13:21:59 Brian Norris wrote:
> > > 
> > > Looking a little closer at this... why do we need the changes to
> > > include/linux/mtd/map.h again? It should be fine to leave these
> > > definitions as-is, right? They don't contribute to the large stack
> > > usage, do they?
> > > 
> > > Maybe I'm just missing something obvious, so please do enlighten 
> > > 
> > 
> > It's been a while since I created the patch, and the originally
> > failing configuration currently doesn't produce this (probably
> > because something else changed). I remember that it was something
> > rather subtle, but don't exactly remember what happened.
> > 
> > I've reverted the patch now, trying to reproduce it on my
> > randconfig setup, but I might not be able to get back to you
> > in the next week while I'm traveling.
> 
> FWIW, I took a little look at this, and I can reproduce this myself. I
> can get a large frame size on at least 2 of the 3 functions you report.
> I think most of the gain you get with this patch is due to the Kconfig
> change (MTD_COMPLEX_MAPPINGS) because it forces an extra level of
> indirection. The other changes seem to give a modest decrease in size,
> though it's less clear why exactly.
> 
> Anyway, I think the problem isn't primarily with anything you're
> touching here exactly, but with the fact that we're putting several
> copies of the 'map_word' typedef on the stack, and doing assignment to
> it as if it's a typical variable. But with MAP_WIDTH_32, this is a
> 32-byte object, and I assume that these (too) long functions are
> introducing enough complexity that the compiler has to have several
> copies of them. To properly fix all this, it seems like the code could
> use some more attention, and not just the superficial changes here.
> 
> Anyway, I'm interested if you have any more thoughts on this. If you
> still think this is worthwhile, I can probably just take it. (It looks
> OK, despite the above comments.)

I'd like to get this fixed in one way or another, but I'm not
particularly attached to my specific workaround.

If you don't think anyone will be able to reduce the number of
map_word instances on the stack, please add my patch so at least
we get rid of the warning and make it less likely to hit the bug.

An alternative might be to optionally move the map_word_and/clr/or/...
operations out of line for large sizes of map_word, so we don't
have all instances on the stack at once.

	Arnd