| Message ID | 20201126155246.25961-1-jack@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | fanotify: Fix fanotify_mark() on 32-bit x86 | expand |
On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote: > > Commit converting syscalls taking 64-bit arguments to new scheme of compat > handlers omitted converting fanotify_mark(2) which then broke the > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat > cumbersome since we need to keep the original compat handler for all the > other 32-bit archs. > This is stupendously ugly. I'm not really sure how this is supposed to work on any 32-bit arch. I'm also not sure whether we should expect the SYSCALL_DEFINE macros to figure this out by themselves. At the very least, the native arm 32 and arm64 compat cases should get tested. Al and Christoph, you're probably a lot more familiar than I am with the nasty details of syscall ABI with 64-bit arguments. > CC: Brian Gerst <brgerst@gmail.com> > Suggested-by: Borislav Petkov <bp@suse.de> > Reported-by: Paweł Jasiak <pawel@jasiak.xyz> > Reported-and-tested-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls taking 64-bit arguments") > CC: stable@vger.kernel.org > Signed-off-by: Jan Kara <jack@suse.cz> > --- > arch/x86/entry/syscalls/syscall_32.tbl | 2 +- > fs/notify/fanotify/fanotify_user.c | 7 ++++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > I plan to queue this fix into my tree next week. I'd be happy if someone with > x86 ABI knowledge checks whether I've got the patch right (especially various > config variants) because it was mostly a guesswork of me & Boris ;). Thanks! > > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl > index 0d0667a9fbd7..b2ec6ff88307 100644 > --- a/arch/x86/entry/syscalls/syscall_32.tbl > +++ b/arch/x86/entry/syscalls/syscall_32.tbl > @@ -350,7 +350,7 @@ > 336 i386 perf_event_open sys_perf_event_open > 337 i386 recvmmsg sys_recvmmsg_time32 compat_sys_recvmmsg_time32 > 338 i386 fanotify_init sys_fanotify_init > -339 i386 fanotify_mark sys_fanotify_mark compat_sys_fanotify_mark > +339 i386 fanotify_mark sys_ia32_fanotify_mark > 340 i386 prlimit64 sys_prlimit64 > 341 i386 name_to_handle_at sys_name_to_handle_at > 342 i386 open_by_handle_at sys_open_by_handle_at compat_sys_open_by_handle_at > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 3e01d8f2ab90..ba38f0fec4d0 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1292,8 +1292,13 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname); > } > > -#ifdef CONFIG_COMPAT > +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32) || \ > + defined(CONFIG_IA32_EMULATION) > +#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) > +SYSCALL_DEFINE6(ia32_fanotify_mark, > +#elif CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > +#endif > int, fanotify_fd, unsigned int, flags, > __u32, mask0, __u32, mask1, int, dfd, > const char __user *, pathname) > -- > 2.16.4 >
On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote: > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote: > > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat > > handlers omitted converting fanotify_mark(2) which then broke the > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat > > cumbersome since we need to keep the original compat handler for all the > > other 32-bit archs. > > > > This is stupendously ugly. I'm not really sure how this is supposed > to work on any 32-bit arch. I'm also not sure whether we should > expect the SYSCALL_DEFINE macros to figure this out by themselves. It works on 32-bit arches because the compiler implicitly uses consecutive input registers or stack slots for 64-bit arguments, and some arches have alignment requirements that result in hidden padding. x86-32 is different now because parameters are passed in via pt_regs, and the 64-bit value has to explicitly be reassembled from the high and low 32-bit values, just like in the compat case. I think the simplest way to handle this is add a wrapper in arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit args. That keeps this mess out of general code. -- Brian Gerst
On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote: > > On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote: > > > > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat > > > handlers omitted converting fanotify_mark(2) which then broke the > > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat > > > cumbersome since we need to keep the original compat handler for all the > > > other 32-bit archs. > > > > > > > This is stupendously ugly. I'm not really sure how this is supposed > > to work on any 32-bit arch. I'm also not sure whether we should > > expect the SYSCALL_DEFINE macros to figure this out by themselves. > > It works on 32-bit arches because the compiler implicitly uses > consecutive input registers or stack slots for 64-bit arguments, and > some arches have alignment requirements that result in hidden padding. > x86-32 is different now because parameters are passed in via pt_regs, > and the 64-bit value has to explicitly be reassembled from the high > and low 32-bit values, just like in the compat case. > That was my guess. > I think the simplest way to handle this is add a wrapper in > arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit > args. That keeps this mess out of general code. Want to send a patch? I also wonder if there's a straightforward way to statically check this. Maybe the syscall wrapper macros could be rigged up to avoid emitting the ia32 stubs if there is a u64 or s64 arg, so the build would fail if someone tries to stick one in the syscall tables. I tried to do this, but I got a bit lost in the macro maze and my attempt didn't work. --Andy
On Fri, Nov 27, 2020 at 7:36 PM Andy Lutomirski <luto@kernel.org> wrote: > > On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat > > > > handlers omitted converting fanotify_mark(2) which then broke the > > > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat > > > > cumbersome since we need to keep the original compat handler for all the > > > > other 32-bit archs. > > > > > > > > > > This is stupendously ugly. I'm not really sure how this is supposed > > > to work on any 32-bit arch. I'm also not sure whether we should > > > expect the SYSCALL_DEFINE macros to figure this out by themselves. > > > > It works on 32-bit arches because the compiler implicitly uses > > consecutive input registers or stack slots for 64-bit arguments, and > > some arches have alignment requirements that result in hidden padding. > > x86-32 is different now because parameters are passed in via pt_regs, > > and the 64-bit value has to explicitly be reassembled from the high > > and low 32-bit values, just like in the compat case. > > > > That was my guess. > > > I think the simplest way to handle this is add a wrapper in > > arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit > > args. That keeps this mess out of general code. > > > Want to send a patch? I settled on doing something along the same line as Jan, but in a more generic way that lays the groundwork for converting more of these arch-specific compat wrappers to a generic wrapper. Patch coming soon. -- Brian Gerst
On Mon 30-11-20 17:21:08, Brian Gerst wrote: > On Fri, Nov 27, 2020 at 7:36 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > On Fri, Nov 27, 2020 at 2:30 PM Brian Gerst <brgerst@gmail.com> wrote: > > > > > > On Fri, Nov 27, 2020 at 1:13 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > > > > > On Thu, Nov 26, 2020 at 7:52 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > Commit converting syscalls taking 64-bit arguments to new scheme of compat > > > > > handlers omitted converting fanotify_mark(2) which then broke the > > > > > syscall for 32-bit x86 builds. Add missed conversion. It is somewhat > > > > > cumbersome since we need to keep the original compat handler for all the > > > > > other 32-bit archs. > > > > > > > > > > > > > This is stupendously ugly. I'm not really sure how this is supposed > > > > to work on any 32-bit arch. I'm also not sure whether we should > > > > expect the SYSCALL_DEFINE macros to figure this out by themselves. > > > > > > It works on 32-bit arches because the compiler implicitly uses > > > consecutive input registers or stack slots for 64-bit arguments, and > > > some arches have alignment requirements that result in hidden padding. > > > x86-32 is different now because parameters are passed in via pt_regs, > > > and the 64-bit value has to explicitly be reassembled from the high > > > and low 32-bit values, just like in the compat case. > > > > > > > That was my guess. > > > > > I think the simplest way to handle this is add a wrapper in > > > arch/x86/kernel/sys_ia32.c with the other fs syscalls that need 64-bit > > > args. That keeps this mess out of general code. > > > > > > Want to send a patch? > > I settled on doing something along the same line as Jan, but in a more > generic way that lays the groundwork for converting more of these > arch-specific compat wrappers to a generic wrapper. Cool, thanks for looking into this! > Patch coming soon. Looking forward to it :) Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
Hi Jan,
I love your patch! Yet something to improve:
[auto build test ERROR on ext3/fsnotify]
[also build test ERROR on tip/x86/asm v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jan-Kara/fanotify-Fix-fanotify_mark-on-32-bit-x86/20201126-235659
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: x86_64-kexec (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/83512221cb769f80e071541619033b0dad1b8fa6
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jan-Kara/fanotify-Fix-fanotify_mark-on-32-bit-x86/20201126-235659
git checkout 83512221cb769f80e071541619033b0dad1b8fa6
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> ld: arch/x86/entry/syscall_32.o:(.rodata+0xa98): undefined reference to `__ia32_sys_ia32_fanotify_mark'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl index 0d0667a9fbd7..b2ec6ff88307 100644 --- a/arch/x86/entry/syscalls/syscall_32.tbl +++ b/arch/x86/entry/syscalls/syscall_32.tbl @@ -350,7 +350,7 @@ 336 i386 perf_event_open sys_perf_event_open 337 i386 recvmmsg sys_recvmmsg_time32 compat_sys_recvmmsg_time32 338 i386 fanotify_init sys_fanotify_init -339 i386 fanotify_mark sys_fanotify_mark compat_sys_fanotify_mark +339 i386 fanotify_mark sys_ia32_fanotify_mark 340 i386 prlimit64 sys_prlimit64 341 i386 name_to_handle_at sys_name_to_handle_at 342 i386 open_by_handle_at sys_open_by_handle_at compat_sys_open_by_handle_at diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 3e01d8f2ab90..ba38f0fec4d0 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1292,8 +1292,13 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname); } -#ifdef CONFIG_COMPAT +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32) || \ + defined(CONFIG_IA32_EMULATION) +#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) +SYSCALL_DEFINE6(ia32_fanotify_mark, +#elif CONFIG_COMPAT COMPAT_SYSCALL_DEFINE6(fanotify_mark, +#endif int, fanotify_fd, unsigned int, flags, __u32, mask0, __u32, mask1, int, dfd, const char __user *, pathname)
Commit converting syscalls taking 64-bit arguments to new scheme of compat handlers omitted converting fanotify_mark(2) which then broke the syscall for 32-bit x86 builds. Add missed conversion. It is somewhat cumbersome since we need to keep the original compat handler for all the other 32-bit archs. CC: Brian Gerst <brgerst@gmail.com> Suggested-by: Borislav Petkov <bp@suse.de> Reported-by: Paweł Jasiak <pawel@jasiak.xyz> Reported-and-tested-by: Naresh Kamboju <naresh.kamboju@linaro.org> Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls taking 64-bit arguments") CC: stable@vger.kernel.org Signed-off-by: Jan Kara <jack@suse.cz> --- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/notify/fanotify/fanotify_user.c | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) I plan to queue this fix into my tree next week. I'd be happy if someone with x86 ABI knowledge checks whether I've got the patch right (especially various config variants) because it was mostly a guesswork of me & Boris ;). Thanks!