| Message ID | 20221007201140.1744961-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/select: mark do_select noinline_for_stack for 32b | expand |
On Fri, Oct 07, 2022 at 01:11:40PM -0700, Nick Desaulniers wrote: > Effectively a revert of > commit ad312f95d41c ("fs/select: avoid clang stack usage warning") > > Various configs can still push the stack useage of core_sys_select() > over the CONFIG_FRAME_WARN threshold (1024B on 32b targets). > > fs/select.c:619:5: error: stack frame size of 1048 bytes in function > 'core_sys_select' [-Werror,-Wframe-larger-than=] Btw, I also see a warning here with all my KASAN x86_64 gcc builds.
On Mon, Oct 10, 2022 at 12:44 AM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Oct 07, 2022 at 01:11:40PM -0700, Nick Desaulniers wrote: > > Effectively a revert of > > commit ad312f95d41c ("fs/select: avoid clang stack usage warning") > > > > Various configs can still push the stack useage of core_sys_select() > > over the CONFIG_FRAME_WARN threshold (1024B on 32b targets). > > > > fs/select.c:619:5: error: stack frame size of 1048 bytes in function > > 'core_sys_select' [-Werror,-Wframe-larger-than=] > > Btw, I also see a warning here with all my KASAN x86_64 gcc builds. Thanks for the report. That might be another interesting data point; I haven't been able to reproduce that locally though: $ make -j$(nproc) defconfig $ ./scripts/config -e KASAN $ make -j$(nproc) olddefconfig fs/select.o $ gcc --version | head -n1 gcc (Debian 12.2.0-1) 12.2.0 I also tried enabling CONFIG_KASAN_INLINE=y but couldn't reproduce. Mind sending me your .config that reproduces this?
On Fri, Oct 7, 2022, at 10:11 PM, Nick Desaulniers wrote: > +#ifdef CONFIG_64BIT > +#define noinline_for_stack_32b > +#else > +#define noinline_for_stack_32b noinline_for_stack > +#endif > + > +noinline_for_stack_32b I don't see much value in making it behave differently for 32 bit: it doesn't reduce the total frame size on 32-bit machines but only hides the warning. The bug you are working around also looks i386 specific (because of limited number of registers vs ubsan needing a lot of them), so just make it a simple 'noinline_for_stack'. Arnd
Hi Nick,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd]
url: https://github.com/intel-lab-lkp/linux/commits/Nick-Desaulniers/fs-select-mark-do_select-noinline_for_stack-for-32b/20221008-041539
base: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd
patch link: https://lore.kernel.org/r/20221007201140.1744961-1-ndesaulniers%40google.com
patch subject: [PATCH] fs/select: mark do_select noinline_for_stack for 32b
config: powerpc-randconfig-c003-20221012
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/58064d22945d0fed666adc4cef463401981e2719
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nick-Desaulniers/fs-select-mark-do_select-noinline_for_stack-for-32b/20221008-041539
git checkout 58064d22945d0fed666adc4cef463401981e2719
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> fs/select.c:981:12: warning: stack frame size (1056) exceeds limit (1024) in 'do_sys_poll' [-Wframe-larger-than]
static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds,
^
1 warning generated.
vim +/do_sys_poll +981 fs/select.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 977
70674f95c0a2ea Andi Kleen 2006-03-28 978 #define N_STACK_PPS ((sizeof(stack_pps) - sizeof(struct poll_list)) / \
70674f95c0a2ea Andi Kleen 2006-03-28 979 sizeof(struct pollfd))
70674f95c0a2ea Andi Kleen 2006-03-28 980
e99ca56ce03dd9 Al Viro 2017-04-08 @981 static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds,
766b9f928bd5b9 Deepa Dinamani 2016-05-19 982 struct timespec64 *end_time)
^1da177e4c3f41 Linus Torvalds 2005-04-16 983 {
^1da177e4c3f41 Linus Torvalds 2005-04-16 984 struct poll_wqueues table;
43e11fa2d1d3b6 Gustavo A. R. Silva 2019-07-16 985 int err = -EFAULT, fdcount, len;
30c14e40ed8546 Jes Sorensen 2006-03-31 986 /* Allocate small arguments on the stack to save memory and be
30c14e40ed8546 Jes Sorensen 2006-03-31 987 faster - use long to make sure the buffer is aligned properly
30c14e40ed8546 Jes Sorensen 2006-03-31 988 on 64 bit archs to avoid unaligned access */
30c14e40ed8546 Jes Sorensen 2006-03-31 989 long stack_pps[POLL_STACK_ALLOC/sizeof(long)];
252e5725cfb55a Oleg Nesterov 2007-10-16 990 struct poll_list *const head = (struct poll_list *)stack_pps;
252e5725cfb55a Oleg Nesterov 2007-10-16 991 struct poll_list *walk = head;
252e5725cfb55a Oleg Nesterov 2007-10-16 992 unsigned long todo = nfds;
^1da177e4c3f41 Linus Torvalds 2005-04-16 993
d554ed895dc8f2 Jiri Slaby 2010-03-05 994 if (nfds > rlimit(RLIMIT_NOFILE))
^1da177e4c3f41 Linus Torvalds 2005-04-16 995 return -EINVAL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 996
252e5725cfb55a Oleg Nesterov 2007-10-16 997 len = min_t(unsigned int, nfds, N_STACK_PPS);
252e5725cfb55a Oleg Nesterov 2007-10-16 998 for (;;) {
252e5725cfb55a Oleg Nesterov 2007-10-16 999 walk->next = NULL;
252e5725cfb55a Oleg Nesterov 2007-10-16 1000 walk->len = len;
252e5725cfb55a Oleg Nesterov 2007-10-16 1001 if (!len)
252e5725cfb55a Oleg Nesterov 2007-10-16 1002 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1003
252e5725cfb55a Oleg Nesterov 2007-10-16 1004 if (copy_from_user(walk->entries, ufds + nfds-todo,
252e5725cfb55a Oleg Nesterov 2007-10-16 1005 sizeof(struct pollfd) * walk->len))
^1da177e4c3f41 Linus Torvalds 2005-04-16 1006 goto out_fds;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1007
252e5725cfb55a Oleg Nesterov 2007-10-16 1008 todo -= walk->len;
252e5725cfb55a Oleg Nesterov 2007-10-16 1009 if (!todo)
252e5725cfb55a Oleg Nesterov 2007-10-16 1010 break;
252e5725cfb55a Oleg Nesterov 2007-10-16 1011
252e5725cfb55a Oleg Nesterov 2007-10-16 1012 len = min(todo, POLLFD_PER_PAGE);
43e11fa2d1d3b6 Gustavo A. R. Silva 2019-07-16 1013 walk = walk->next = kmalloc(struct_size(walk, entries, len),
0bcfe68b876748 Linus Torvalds 2021-09-07 1014 GFP_KERNEL);
252e5725cfb55a Oleg Nesterov 2007-10-16 1015 if (!walk) {
252e5725cfb55a Oleg Nesterov 2007-10-16 1016 err = -ENOMEM;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1017 goto out_fds;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1018 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 1019 }
9f72949f679df0 David Woodhouse 2006-01-18 1020
252e5725cfb55a Oleg Nesterov 2007-10-16 1021 poll_initwait(&table);
ccec5ee302d5cb Mateusz Guzik 2016-01-06 1022 fdcount = do_poll(head, &table, end_time);
252e5725cfb55a Oleg Nesterov 2007-10-16 1023 poll_freewait(&table);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1024
ef0ba05538299f Linus Torvalds 2021-01-07 1025 if (!user_write_access_begin(ufds, nfds * sizeof(*ufds)))
ef0ba05538299f Linus Torvalds 2021-01-07 1026 goto out_fds;
ef0ba05538299f Linus Torvalds 2021-01-07 1027
252e5725cfb55a Oleg Nesterov 2007-10-16 1028 for (walk = head; walk; walk = walk->next) {
^1da177e4c3f41 Linus Torvalds 2005-04-16 1029 struct pollfd *fds = walk->entries;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1030 int j;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1031
ef0ba05538299f Linus Torvalds 2021-01-07 1032 for (j = walk->len; j; fds++, ufds++, j--)
ef0ba05538299f Linus Torvalds 2021-01-07 1033 unsafe_put_user(fds->revents, &ufds->revents, Efault);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1034 }
ef0ba05538299f Linus Torvalds 2021-01-07 1035 user_write_access_end();
252e5725cfb55a Oleg Nesterov 2007-10-16 1036
^1da177e4c3f41 Linus Torvalds 2005-04-16 1037 err = fdcount;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1038 out_fds:
252e5725cfb55a Oleg Nesterov 2007-10-16 1039 walk = head->next;
252e5725cfb55a Oleg Nesterov 2007-10-16 1040 while (walk) {
252e5725cfb55a Oleg Nesterov 2007-10-16 1041 struct poll_list *pos = walk;
252e5725cfb55a Oleg Nesterov 2007-10-16 1042 walk = walk->next;
252e5725cfb55a Oleg Nesterov 2007-10-16 1043 kfree(pos);
^1da177e4c3f41 Linus Torvalds 2005-04-16 1044 }
252e5725cfb55a Oleg Nesterov 2007-10-16 1045
^1da177e4c3f41 Linus Torvalds 2005-04-16 1046 return err;
ef0ba05538299f Linus Torvalds 2021-01-07 1047
ef0ba05538299f Linus Torvalds 2021-01-07 1048 Efault:
ef0ba05538299f Linus Torvalds 2021-01-07 1049 user_write_access_end();
ef0ba05538299f Linus Torvalds 2021-01-07 1050 err = -EFAULT;
ef0ba05538299f Linus Torvalds 2021-01-07 1051 goto out_fds;
^1da177e4c3f41 Linus Torvalds 2005-04-16 1052 }
9f72949f679df0 David Woodhouse 2006-01-18 1053
diff --git a/fs/select.c b/fs/select.c index 0ee55af1a55c..945d04b9cf5a 100644 --- a/fs/select.c +++ b/fs/select.c @@ -476,6 +476,13 @@ static inline void wait_key_set(poll_table *wait, unsigned long in, wait->_key |= POLLOUT_SET; } +#ifdef CONFIG_64BIT +#define noinline_for_stack_32b +#else +#define noinline_for_stack_32b noinline_for_stack +#endif + +noinline_for_stack_32b static int do_select(int n, fd_set_bits *fds, struct timespec64 *end_time) { ktime_t expire, *to = NULL; diff --git a/include/linux/poll.h b/include/linux/poll.h index a9e0e1c2d1f2..d1ea4f3714a8 100644 --- a/include/linux/poll.h +++ b/include/linux/poll.h @@ -14,11 +14,7 @@ /* ~832 bytes of stack space used max in sys_select/sys_poll before allocating additional memory. */ -#ifdef __clang__ -#define MAX_STACK_ALLOC 768 -#else #define MAX_STACK_ALLOC 832 -#endif #define FRONTEND_STACK_ALLOC 256 #define SELECT_STACK_ALLOC FRONTEND_STACK_ALLOC #define POLL_STACK_ALLOC FRONTEND_STACK_ALLOC
Effectively a revert of commit ad312f95d41c ("fs/select: avoid clang stack usage warning") Various configs can still push the stack useage of core_sys_select() over the CONFIG_FRAME_WARN threshold (1024B on 32b targets). fs/select.c:619:5: error: stack frame size of 1048 bytes in function 'core_sys_select' [-Werror,-Wframe-larger-than=] core_sys_select() has a large stack allocation for `stack_fds` where it tries to do something equivalent to "small string optimization" to potentially avoid a malloc. core_sys_select() calls do_select() which has another potentially large stack allocation, `table`. Both of these values depend on FRONTEND_STACK_ALLOC. Mix those two large allocation with register spills which are exacerbated by various configs and compiler versions and we can just barely exceed the 1024B limit. Rather than keep trying to find the right value of MAX_STACK_ALLOC or FRONTEND_STACK_ALLOC, mark do_select() as noinline_for_stack for 32b targets. The intent of FRONTEND_STACK_ALLOC is to help potentially avoid a dynamic memory allocation. In that spirit, restore the previous threshold but separate the stack frames for 32b targets. Link: https://lore.kernel.org/lkml/20221006222124.aabaemy7ofop7ccz@google.com/ Fixes: ad312f95d41c ("fs/select: avoid clang stack usage warning") Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- fs/select.c | 7 +++++++ include/linux/poll.h | 4 ---- 2 files changed, 7 insertions(+), 4 deletions(-) base-commit: 93ed07a23fd08b8613f64cf0a15d7fbdaca010fd