Messages in this thread | | | Subject | Re: [PATCH] xenbus: avoid stack overflow warning | From | Jürgen Groß <> | Date | Tue, 5 May 2020 18:34:10 +0200 |
| |
On 05.05.20 18:12, Boris Ostrovsky wrote: > > On 5/5/20 12:02 PM, Jürgen Groß wrote: >> On 05.05.20 17:01, Arnd Bergmann wrote: >>> On Tue, May 5, 2020 at 4:34 PM Jürgen Groß <jgross@suse.com> wrote: >>>> On 05.05.20 16:15, Arnd Bergmann wrote: >>>>> The __xenbus_map_ring() function has two large arrays, 'map' and >>>>> 'unmap' on its stack. When clang decides to inline it into its caller, >>>>> xenbus_map_ring_valloc_hvm(), the total stack usage exceeds the >>>>> warning >>>>> limit for stack size on 32-bit architectures. >>>>> >>>>> drivers/xen/xenbus/xenbus_client.c:592:12: error: stack frame size >>>>> of 1104 bytes in function 'xenbus_map_ring_valloc_hvm' >>>>> [-Werror,-Wframe-larger-than=] >>>>> >>>>> As far as I can tell, other compilers don't inline it here, so we get >>>>> no warning, but the stack usage is actually the same. It is possible >>>>> for both arrays to use the same location on the stack, but the >>>>> compiler >>>>> cannot prove that this is safe because they get passed to external >>>>> functions that may end up using them until they go out of scope. >>>>> >>>>> Move the two arrays into separate basic blocks to limit the scope >>>>> and force them to occupy less stack in total, regardless of the >>>>> inlining decision. >>>> >>>> Why don't you put both arrays into a union? >>> >>> I considered that as well, and don't really mind either way. I think >>> it does >>> get a bit ugly whatever we do. If you prefer the union, I can respin the >>> patch that way. >> >> Hmm, thinking more about it I think the real clean solution would be to >> extend struct map_ring_valloc_hvm to cover the pv case, too, to add the >> map and unmap arrays (possibly as a union) to it and to allocate it >> dynamically instead of having it on the stack. >> >> Would you be fine doing this? > > > > Another option might be to factor out/modify code from > xenbus_unmap_ring() and call the resulting code from > __xenbus_map_ring()'s fail path.
This will still allocate large arrays on the stack. If we ever increase the max ring page order it will explode.
Juergen
| |