On Fri, 21 Jun 2019 at 15:44, Arnd Bergmann [off-list ref] wrote:

On Fri, Jun 21, 2019 at 3:32 PM Ard Biesheuvel
[off-list ref] wrote:

quoted

On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann [off-list ref] wrote:

quoted

On Thu, Jun 20, 2019 at 7:36 PM Kees Cook [off-list ref] wrote:

quoted

On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:

quoted

The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
leads to much larger kernel stack usage, as seen from the warnings
about functions that now exceed the 2048 byte limit:

Is the preference that this go into v5.2 (there's not much time left),
or should this be v5.3? (You didn't mark it as Cc: stable?)

Having it in 5.2 would be great. I had not done much build testing in the last
months, so I didn't actually realize that your patch was merged a while ago
rather than only in linux-next.

BTW, I have now run into a small number of files that are still affected
by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
to come up with patches for those as well, we can probably do it in a way
that also improves the affected drivers. I'll put you on Cc when I
find another one.

There is something fundamentally wrong here, though. BYREF_ALL only
initializes variables that have their address taken, which does not
explain why the size of the stack frame should increase (since in
order to have an address in the first place, the variable must already
have a stack slot assigned)

So I suspect that BYREF_ALL is defeating some optimizations where.
e.g., the call involving the address of the variable is optimized
away, but the the initialization remains, thus forcing the variable to
be allocated in the stack frame even though the initializer is the
only thing that references it.

One pattern I have seen here is temporary variables from macros or
inline functions whose lifetime now extends over the entire function
rather than just the basic block in which they are defined, see e.g.
lpfc_debug_dump_qe() being inlined multiple times into
lpfc_debug_dump_all_queues(). Each instance of the local
"char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
one now, where the behavior without the structleak plugin is that
they don't.

Right, that seems to be due to the fact that this code

/* split the first bb where we can put the forced initializers */
gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
if (!single_pred_p(bb)) {
    split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
    gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
}

puts all the initializers at the beginning of the function rather than
inside the scope of the definition.