Re: PROBLEM: fanotify_mark EFAULT on x86
On Thu, Nov 26, 2020 at 11:48:27AM +0100, Jan Kara wrote: > I'd prefer that as well but if nobody pops up, I'll just push this to my > tree next week and will see what breaks :) Right. You could send a proper patch and Cc the usual suspects as now it is buried in some thread which people might not read. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: PROBLEM: fanotify_mark EFAULT on x86
On Thu 26-11-20 01:01:30, Naresh Kamboju wrote: > On Tue, 24 Nov 2020 at 15:50, Jan Kara wrote: > > > > On Tue 24-11-20 09:45:07, Borislav Petkov wrote: > > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > > > > On 23/11/20, Jan Kara wrote: > > > > > OK, with a help of Boris Petkov I think I have a fix that looks > > > > > correct > > > > > (attach). Can you please try whether it works for you? Thanks! > > > > > > > > > Thanks for checking! I didn't realize I needed to change the ifdefs as well > > (I missed that bit in 121b32a58a3a). So do I understand correctly that > > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are > > passed just fine regardless of whether the userspace is 32-bit or not? > > > > Also how about other 32-bit archs? Because I now realized that > > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by > > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g. > > in sparc, powerpc, and other args). So I probably need to actually keep > > that for other archs but do the modification only for x86, don't I? > > > > So something like attached patch? > > I have tested the attached patch on i386 and qemu_i386 and the reported > problem > got fixed. > > Test links, > https://lkft.validation.linaro.org/scheduler/job/1985236#L1176 > https://lkft.validation.linaro.org/scheduler/job/1985238#L801 Thanks for testing! I've added your tested-by tag. Honza -- Jan Kara SUSE Labs, CR
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue 24-11-20 11:28:14, Borislav Petkov wrote: > On Tue, Nov 24, 2020 at 11:20:33AM +0100, Jan Kara wrote: > > On Tue 24-11-20 09:45:07, Borislav Petkov wrote: > > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > > > > On 23/11/20, Jan Kara wrote: > > > > > OK, with a help of Boris Petkov I think I have a fix that looks > > > > > correct > > > > > (attach). Can you please try whether it works for you? Thanks! > > > > > > > > Unfortunately I am getting a linker error. > > > > > > > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to > > > > `__ia32_sys_ia32_fanotify_mark' > > > > > > Because CONFIG_COMPAT is not set in your .config. > > > > > > Jan, look at 121b32a58a3a and especially this hunk > > > > > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > > index 9b294c13809a..b8f89f78b8cd 100644 > > > --- a/arch/x86/kernel/Makefile > > > +++ b/arch/x86/kernel/Makefile > > > @@ -53,6 +53,8 @@ obj-y += setup.o x86_init.o i8259.o > > > irqinit.o > > > obj-$(CONFIG_JUMP_LABEL) += jump_label.o > > > obj-$(CONFIG_IRQ_WORK) += irq_work.o > > > obj-y += probe_roms.o > > > +obj-$(CONFIG_X86_32) += sys_ia32.o > > > +obj-$(CONFIG_IA32_EMULATION) += sys_ia32.o > > > > > > how it enables the ia32 syscalls depending on those config items. Now, > > > you have > > > > > > #ifdef CONFIG_COMPAT > > > -COMPAT_SYSCALL_DEFINE6(fanotify_mark, > > > +SYSCALL_DEFINE6(ia32_fanotify_mark, > > > > > > which is under CONFIG_COMPAT which is not set in Paweł's config. > > > > > > config COMPAT > > > def_bool y > > > depends on IA32_EMULATION || X86_X32 > > > > > > but it depends on those two config items. > > > > > > However, it looks to me like ia32_fanotify_mark's definition should be > > > simply under CONFIG_X86_32 because: > > > > > > IA32_EMULATION is 32-bit emulation on 64-bit kernels > > > X86_X32 is for the x32 ABI > > > > Thanks for checking! I didn't realize I needed to change the ifdefs as well > > (I missed that bit in 121b32a58a3a). So do I understand correctly that > > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are > > passed just fine regardless of whether the userspace is 32-bit or not? > > > > Also how about other 32-bit archs? Because I now realized that > > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by > > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g. > > in sparc, powerpc, and other args). So I probably need to actually keep > > that for other archs but do the modification only for x86, don't I? > > Hmm, you raise a good point and looking at that commit again, the > intention is to supply those ia32 wrappers as both 32-bit native *and* > 32-bit emulation ones. > > So I think this > > > -#ifdef CONFIG_COMPAT > > +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32) > > +#ifdef CONFIG_X86_32 > > +SYSCALL_DEFINE6(ia32_fanotify_mark, > > +#elif CONFIG_COMPAT > > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > > +#endif > > should be > > if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) > SYSCALL_DEFINE6(ia32_fanotify_mark, > #elif CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > #endif > > or so. > > Meaning that 32-bit native or 32-bit emulation supplies > ia32_fanotify_mark() as a syscall wrapper and other arches doing > CONFIG_COMPAT, still do the COMPAT_SYSCALL_DEFINE6() thing. Yeah, looking again at what those config options mean I agree. Patch updated. > But I'd prefer if someone more knowledgeable than me in that whole > syscall macros muck to have a look... I'd prefer that as well but if nobody pops up, I'll just push this to my tree next week and will see what breaks :) Honza -- Jan Kara SUSE Labs, CR
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue, 24 Nov 2020 at 15:50, Jan Kara wrote: > > On Tue 24-11-20 09:45:07, Borislav Petkov wrote: > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > > > On 23/11/20, Jan Kara wrote: > > > > OK, with a help of Boris Petkov I think I have a fix that looks correct > > > > (attach). Can you please try whether it works for you? Thanks! > > > > > Thanks for checking! I didn't realize I needed to change the ifdefs as well > (I missed that bit in 121b32a58a3a). So do I understand correctly that > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are > passed just fine regardless of whether the userspace is 32-bit or not? > > Also how about other 32-bit archs? Because I now realized that > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g. > in sparc, powerpc, and other args). So I probably need to actually keep > that for other archs but do the modification only for x86, don't I? > > So something like attached patch? I have tested the attached patch on i386 and qemu_i386 and the reported problem got fixed. Test links, https://lkft.validation.linaro.org/scheduler/job/1985236#L1176 https://lkft.validation.linaro.org/scheduler/job/1985238#L801 -- Linaro LKFT https://lkft.linaro.org
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue, Nov 24, 2020 at 11:20:33AM +0100, Jan Kara wrote: > On Tue 24-11-20 09:45:07, Borislav Petkov wrote: > > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > > > On 23/11/20, Jan Kara wrote: > > > > OK, with a help of Boris Petkov I think I have a fix that looks correct > > > > (attach). Can you please try whether it works for you? Thanks! > > > > > > Unfortunately I am getting a linker error. > > > > > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to > > > `__ia32_sys_ia32_fanotify_mark' > > > > Because CONFIG_COMPAT is not set in your .config. > > > > Jan, look at 121b32a58a3a and especially this hunk > > > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > index 9b294c13809a..b8f89f78b8cd 100644 > > --- a/arch/x86/kernel/Makefile > > +++ b/arch/x86/kernel/Makefile > > @@ -53,6 +53,8 @@ obj-y += setup.o x86_init.o i8259.o > > irqinit.o > > obj-$(CONFIG_JUMP_LABEL) += jump_label.o > > obj-$(CONFIG_IRQ_WORK) += irq_work.o > > obj-y += probe_roms.o > > +obj-$(CONFIG_X86_32) += sys_ia32.o > > +obj-$(CONFIG_IA32_EMULATION) += sys_ia32.o > > > > how it enables the ia32 syscalls depending on those config items. Now, > > you have > > > > #ifdef CONFIG_COMPAT > > -COMPAT_SYSCALL_DEFINE6(fanotify_mark, > > +SYSCALL_DEFINE6(ia32_fanotify_mark, > > > > which is under CONFIG_COMPAT which is not set in Paweł's config. > > > > config COMPAT > > def_bool y > > depends on IA32_EMULATION || X86_X32 > > > > but it depends on those two config items. > > > > However, it looks to me like ia32_fanotify_mark's definition should be > > simply under CONFIG_X86_32 because: > > > > IA32_EMULATION is 32-bit emulation on 64-bit kernels > > X86_X32 is for the x32 ABI > > Thanks for checking! I didn't realize I needed to change the ifdefs as well > (I missed that bit in 121b32a58a3a). So do I understand correctly that > whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are > passed just fine regardless of whether the userspace is 32-bit or not? > > Also how about other 32-bit archs? Because I now realized that > CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by > other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g. > in sparc, powerpc, and other args). So I probably need to actually keep > that for other archs but do the modification only for x86, don't I? Hmm, you raise a good point and looking at that commit again, the intention is to supply those ia32 wrappers as both 32-bit native *and* 32-bit emulation ones. So I think this > -#ifdef CONFIG_COMPAT > +#if defined(CONFIG_COMPAT) || defined(CONFIG_X86_32) > +#ifdef CONFIG_X86_32 > +SYSCALL_DEFINE6(ia32_fanotify_mark, > +#elif CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > +#endif should be if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION) SYSCALL_DEFINE6(ia32_fanotify_mark, #elif CONFIG_COMPAT COMPAT_SYSCALL_DEFINE6(fanotify_mark, #endif or so. Meaning that 32-bit native or 32-bit emulation supplies ia32_fanotify_mark() as a syscall wrapper and other arches doing CONFIG_COMPAT, still do the COMPAT_SYSCALL_DEFINE6() thing. But I'd prefer if someone more knowledgeable than me in that whole syscall macros muck to have a look... Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue 24-11-20 09:45:07, Borislav Petkov wrote: > On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > > On 23/11/20, Jan Kara wrote: > > > OK, with a help of Boris Petkov I think I have a fix that looks correct > > > (attach). Can you please try whether it works for you? Thanks! > > > > Unfortunately I am getting a linker error. > > > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to > > `__ia32_sys_ia32_fanotify_mark' > > Because CONFIG_COMPAT is not set in your .config. > > Jan, look at 121b32a58a3a and especially this hunk > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 9b294c13809a..b8f89f78b8cd 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -53,6 +53,8 @@ obj-y += setup.o x86_init.o i8259.o > irqinit.o > obj-$(CONFIG_JUMP_LABEL) += jump_label.o > obj-$(CONFIG_IRQ_WORK) += irq_work.o > obj-y += probe_roms.o > +obj-$(CONFIG_X86_32) += sys_ia32.o > +obj-$(CONFIG_IA32_EMULATION) += sys_ia32.o > > how it enables the ia32 syscalls depending on those config items. Now, > you have > > #ifdef CONFIG_COMPAT > -COMPAT_SYSCALL_DEFINE6(fanotify_mark, > +SYSCALL_DEFINE6(ia32_fanotify_mark, > > which is under CONFIG_COMPAT which is not set in Paweł's config. > > config COMPAT > def_bool y > depends on IA32_EMULATION || X86_X32 > > but it depends on those two config items. > > However, it looks to me like ia32_fanotify_mark's definition should be > simply under CONFIG_X86_32 because: > > IA32_EMULATION is 32-bit emulation on 64-bit kernels > X86_X32 is for the x32 ABI Thanks for checking! I didn't realize I needed to change the ifdefs as well (I missed that bit in 121b32a58a3a). So do I understand correctly that whenever the kernel is 64-bit, 64-bit syscall args (e.g. defined as u64) are passed just fine regardless of whether the userspace is 32-bit or not? Also how about other 32-bit archs? Because I now realized that CONFIG_COMPAT as well as the COMPAT_SYSCALL_DEFINE6() is also utilized by other 32-bit archs (I can see a reference to compat_sys_fanotify_mark e.g. in sparc, powerpc, and other args). So I probably need to actually keep that for other archs but do the modification only for x86, don't I? So something like attached patch? Honza -- Jan Kara SUSE Labs, CR >From 20d2ddf37c01e91ca18d415a59b3488a394acd8f Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 23 Nov 2020 17:37:00 +0100 Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit x86 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Suggested-by: Borislav Petkov Reported-by: Paweł Jasiak Reported-by: Naresh Kamboju Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls taking 64-bit arguments") CC: sta...@vger.kernel.org Signed-off-by: Jan Kara --- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/notify/fanotify/fanotify_user.c | 6 +- 2 files changed, 6 insertions(+), 2 deletions(-) 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..54a36d4bd116 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1292,8 +1292,12 @@ 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) +#ifdef CONFIG_X86_32 +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
Re: PROBLEM: fanotify_mark EFAULT on x86
On Mon, Nov 23, 2020 at 11:46:51PM +0100, Paweł Jasiak wrote: > On 23/11/20, Jan Kara wrote: > > OK, with a help of Boris Petkov I think I have a fix that looks correct > > (attach). Can you please try whether it works for you? Thanks! > > Unfortunately I am getting a linker error. > > ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to > `__ia32_sys_ia32_fanotify_mark' Because CONFIG_COMPAT is not set in your .config. Jan, look at 121b32a58a3a and especially this hunk diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 9b294c13809a..b8f89f78b8cd 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -53,6 +53,8 @@ obj-y += setup.o x86_init.o i8259.o irqinit.o obj-$(CONFIG_JUMP_LABEL) += jump_label.o obj-$(CONFIG_IRQ_WORK) += irq_work.o obj-y += probe_roms.o +obj-$(CONFIG_X86_32) += sys_ia32.o +obj-$(CONFIG_IA32_EMULATION) += sys_ia32.o how it enables the ia32 syscalls depending on those config items. Now, you have #ifdef CONFIG_COMPAT -COMPAT_SYSCALL_DEFINE6(fanotify_mark, +SYSCALL_DEFINE6(ia32_fanotify_mark, which is under CONFIG_COMPAT which is not set in Paweł's config. config COMPAT def_bool y depends on IA32_EMULATION || X86_X32 but it depends on those two config items. However, it looks to me like ia32_fanotify_mark's definition should be simply under CONFIG_X86_32 because: IA32_EMULATION is 32-bit emulation on 64-bit kernels X86_X32 is for the x32 ABI But I could be missing an angle... HTH. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: PROBLEM: fanotify_mark EFAULT on x86
On 23/11/20, Jan Kara wrote: > OK, with a help of Boris Petkov I think I have a fix that looks correct > (attach). Can you please try whether it works for you? Thanks! Unfortunately I am getting a linker error. ld: arch/x86/entry/syscall_32.o:(.rodata+0x54c): undefined reference to `__ia32_sys_ia32_fanotify_mark' > -- > Jan Kara > SUSE Labs, CR > From fc9104a50a774ec198c1e3a145372cde77df7967 Mon Sep 17 00:00:00 2001 > From: Jan Kara > Date: Mon, 23 Nov 2020 17:37:00 +0100 > Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit archs > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > 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 ABI. Add missed conversion. > > CC: Brian Gerst > Suggested-by: Borislav Petkov > Reported-by: Paweł Jasiak > Reported-by: Naresh Kamboju > Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls > taking 64-bit arguments") > CC: sta...@vger.kernel.org > Signed-off-by: Jan Kara > --- > arch/x86/entry/syscalls/syscall_32.tbl | 2 +- > fs/notify/fanotify/fanotify_user.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > 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 i386perf_event_open sys_perf_event_open > 337 i386recvmmsgsys_recvmmsg_time32 > compat_sys_recvmmsg_time32 > 338 i386fanotify_init sys_fanotify_init > -339 i386fanotify_mark sys_fanotify_mark > compat_sys_fanotify_mark > +339 i386fanotify_mark sys_ia32_fanotify_mark > 340 i386prlimit64 sys_prlimit64 > 341 i386name_to_handle_at sys_name_to_handle_at > 342 i386open_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..e20e7b53a87f 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1293,7 +1293,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, > unsigned int, flags, > } > > #ifdef CONFIG_COMPAT > -COMPAT_SYSCALL_DEFINE6(fanotify_mark, > +SYSCALL_DEFINE6(ia32_fanotify_mark, > int, fanotify_fd, unsigned int, flags, > __u32, mask0, __u32, mask1, int, dfd, > const char __user *, pathname) > -- > 2.16.4 > -- Paweł Jasiak
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue 03-11-20 22:17:47, Paweł Jasiak wrote: > I have written small patch that fixes problem for me and doesn't break > x86_64. OK, with a help of Boris Petkov I think I have a fix that looks correct (attach). Can you please try whether it works for you? Thanks! Honza > > diff --git a/fs/notify/fanotify/fanotify_user.c > b/fs/notify/fanotify/fanotify_user.c > index 3e01d8f2ab90..cf0b97309975 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned > int flags, __u64 mask, > return ret; > } > > +#if defined(CONFIG_X86) && !defined(CONFIG_64BIT) > +SYSCALL_DEFINE6(fanotify_mark, > + int, fanotify_fd, unsigned int, flags, __u32, mask0, > + __u32, mask1, int, dfd, const char __user *, pathname) > +{ > + return do_fanotify_mark(fanotify_fd, flags, > +#ifdef __BIG_ENDIAN > + ((__u64)mask0 << 32) | mask1, > +#else > + ((__u64)mask1 << 32) | mask0, > +#endif > + dfd, pathname); > +} > +#else > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > __u64, mask, int, dfd, > const char __user *, pathname) > { > return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname); > } > +#endif > > #ifdef CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > > > -- > > Paweł Jasiak -- Jan Kara SUSE Labs, CR >From fc9104a50a774ec198c1e3a145372cde77df7967 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 23 Nov 2020 17:37:00 +0100 Subject: [PATCH] fanotify: Fix fanotify_mark() on 32-bit archs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ABI. Add missed conversion. CC: Brian Gerst Suggested-by: Borislav Petkov Reported-by: Paweł Jasiak Reported-by: Naresh Kamboju Fixes: 121b32a58a3a ("x86/entry/32: Use IA32-specific wrappers for syscalls taking 64-bit arguments") CC: sta...@vger.kernel.org Signed-off-by: Jan Kara --- arch/x86/entry/syscalls/syscall_32.tbl | 2 +- fs/notify/fanotify/fanotify_user.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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..e20e7b53a87f 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1293,7 +1293,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, } #ifdef CONFIG_COMPAT -COMPAT_SYSCALL_DEFINE6(fanotify_mark, +SYSCALL_DEFINE6(ia32_fanotify_mark, int, fanotify_fd, unsigned int, flags, __u32, mask0, __u32, mask1, int, dfd, const char __user *, pathname) -- 2.16.4
Re: PROBLEM: fanotify_mark EFAULT on x86
On Tue 03-11-20 22:17:47, Paweł Jasiak wrote: > I have written small patch that fixes problem for me and doesn't break > x86_64. Yeah, that looks sensible, thanks for the patch. But I'm waiting for some explanation from x86 folks when compat handlers are really needed and why it wasn't needed before syscall wrapper rewrite in 5.7-rc1 and is needed now. Brian, Andy, Thomas? Honza > > diff --git a/fs/notify/fanotify/fanotify_user.c > b/fs/notify/fanotify/fanotify_user.c > index 3e01d8f2ab90..cf0b97309975 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned > int flags, __u64 mask, > return ret; > } > > +#if defined(CONFIG_X86) && !defined(CONFIG_64BIT) > +SYSCALL_DEFINE6(fanotify_mark, > + int, fanotify_fd, unsigned int, flags, __u32, mask0, > + __u32, mask1, int, dfd, const char __user *, pathname) > +{ > + return do_fanotify_mark(fanotify_fd, flags, > +#ifdef __BIG_ENDIAN > + ((__u64)mask0 << 32) | mask1, > +#else > + ((__u64)mask1 << 32) | mask0, > +#endif > + dfd, pathname); > +} > +#else > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > __u64, mask, int, dfd, > const char __user *, pathname) > { > return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname); > } > +#endif > > #ifdef CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > > > -- > > Paweł Jasiak -- Jan Kara SUSE Labs, CR
Re: PROBLEM: fanotify_mark EFAULT on x86
I have written small patch that fixes problem for me and doesn't break x86_64. diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 3e01d8f2ab90..cf0b97309975 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1285,12 +1285,27 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, return ret; } +#if defined(CONFIG_X86) && !defined(CONFIG_64BIT) +SYSCALL_DEFINE6(fanotify_mark, + int, fanotify_fd, unsigned int, flags, __u32, mask0, + __u32, mask1, int, dfd, const char __user *, pathname) +{ + return do_fanotify_mark(fanotify_fd, flags, +#ifdef __BIG_ENDIAN + ((__u64)mask0 << 32) | mask1, +#else + ((__u64)mask1 << 32) | mask0, +#endif +dfd, pathname); +} +#else SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, __u64, mask, int, dfd, const char __user *, pathname) { return do_fanotify_mark(fanotify_fd, flags, mask, dfd, pathname); } +#endif #ifdef CONFIG_COMPAT COMPAT_SYSCALL_DEFINE6(fanotify_mark, -- Paweł Jasiak
Re: PROBLEM: fanotify_mark EFAULT on x86
On 02/11/20, Jan Kara wrote: > Strange. Thanks for report. Looks like some issue got created / exposed > somewhere between 5.5 and 5.9 (actually probably between 5.5 and 5.7 > because the Linaro report you mentioned [1] is from 5.7-rc6). There were > no changes in this area in fanotify, I think it must have been some x86 > change that triggered this. Hum, looking into x86 changelog in that time > range there was a series rewriting 32-bit ABI [2] that got merged into > 5.7-rc1. Can you perhaps check whether 5.6 is good and 5.7-rc1 is bad? 5.6 works. 5.7-rc1 doesn't work. -- Paweł Jasiak
Re: PROBLEM: fanotify_mark EFAULT on x86
On Sun 01-11-20 22:27:38, Paweł Jasiak wrote: > I am trying to run examples from man fanotify.7 but fanotify_mark always > fail with errno = EFAULT. > > fanotify_mark declaration is > > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > __u64, mask, int, dfd, > const char __user *, pathname) > > When > > fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR, > FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de) > > is called on kernel side I can see in do_syscall_32_irqs_on that CPU > context is > bx = 0x4= 4 > cx = 0x9= FAN_MARK_ADD | FAN_MARK_ONLYDIR, > dx = 0x4100 = FAN_CREATE | FAN_ONDIR > si = 0x0 > di = 0xff9c = AT_FDCWD > bp = 0xdeadc0de > ax = 0xffda > orix_ax = 0x153 > > I am not sure if it is ok because third argument is uint64_t so if I > understand correctly mask should be divided into two registers (dx and > si). > > But in fanotify_mark we get > fanotify_fd = 4 = bx > flags = 0x9= cx > mask= 0x4100 = dx > dfd = 0 = si > pathname= 0xff9c = di > > I believe that correct order is > fanotify_fd = 4 = bx > flags = 0x9= cx > mask= 0x4100 = (si << 32) | dx > dfd = 0xff9c = di > pathname= 0xdeadc0de = bp > > I think that we should call COMPAT version of fanotify_mark here > > COMPAT_SYSCALL_DEFINE6(fanotify_mark, > int, fanotify_fd, unsigned int, flags, > __u32, mask0, __u32, mask1, int, dfd, > const char __user *, pathname) > > or something wrong is with 64-bits arguments. > > I am running Linux 5.9.2 i686 on Pentium III (Coppermine). > For tests I am using Debian sid on qemu with 5.9.2 and default kernel > from repositories. > > Everything works fine on 5.5 and 5.4. Strange. Thanks for report. Looks like some issue got created / exposed somewhere between 5.5 and 5.9 (actually probably between 5.5 and 5.7 because the Linaro report you mentioned [1] is from 5.7-rc6). There were no changes in this area in fanotify, I think it must have been some x86 change that triggered this. Hum, looking into x86 changelog in that time range there was a series rewriting 32-bit ABI [2] that got merged into 5.7-rc1. Can you perhaps check whether 5.6 is good and 5.7-rc1 is bad? Brian, any idea whether your series could regress fanotify_mark(2) syscall? Do we have somewhere documented which syscalls need compat wrappers and how they should look like? Honza [1] https://lists.linux.it/pipermail/ltp/2020-June/017436.html [2] https://lore.kernel.org/lkml/20200313195144.164260-1-brge...@gmail.com/ -- Jan Kara SUSE Labs, CR
Re: PROBLEM: fanotify_mark EFAULT on x86
On 01/11/20, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 10:27:38PM +0100, Paweł Jasiak wrote: > > I am trying to run examples from man fanotify.7 but fanotify_mark always > > fail with errno = EFAULT. > > > > fanotify_mark declaration is > > > > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > > __u64, mask, int, dfd, > > const char __user *, pathname) > > Don't worry about that. You aren't calling the SYSCALL, you're calling > glibc and glibc is turning it into a syscall. > > extern int fanotify_mark (int __fanotify_fd, unsigned int __flags, > uint64_t __mask, int __dfd, const char *__pathname) > > > When > > > > fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR, > > FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de) > > The last argument is supposed to be a pointer to a string. I'm guessing > there's no string at 0xdeadc0de. You are right but it's not a problem. 0xdeadc0de is just a _well known_ address here only for debug purpose. pathname inside kernel should be a pointer to string located in user space at 0xdeadc0de but it is equal to 0xff9c which is AT_FDCWD. If you call fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_ONLYDIR, FAN_CREATE | FAN_ONDIR, AT_FDCWD, argv[1]); from example with *valid* pointer at argv[1] you still get EFAULT because pathname is equal to AT_FDCWD in kernel space -- last argument is not used. In my example in user space we have fanotify_fd = 4 flags = 0x9 mask= 0x4100 dfd = 0xff9c pathname= 0xdeadc0de and in kernel space we have fanotify_fd = 4 flags = 0x9 mask= 0x4100 dfd = 0 pathname= 0xff9c So all arguments after __u64 mask are shifted by one. It looks similar to https://lists.linux.it/pipermail/ltp/2020-June/017436.html -- Paweł Jasiak signature.asc Description: PGP signature
Re: PROBLEM: fanotify_mark EFAULT on x86
On Sun, Nov 01, 2020 at 10:27:38PM +0100, Paweł Jasiak wrote: > I am trying to run examples from man fanotify.7 but fanotify_mark always > fail with errno = EFAULT. > > fanotify_mark declaration is > > SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, > __u64, mask, int, dfd, > const char __user *, pathname) Don't worry about that. You aren't calling the SYSCALL, you're calling glibc and glibc is turning it into a syscall. extern int fanotify_mark (int __fanotify_fd, unsigned int __flags, uint64_t __mask, int __dfd, const char *__pathname) > When > > fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR, > FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de) The last argument is supposed to be a pointer to a string. I'm guessing there's no string at 0xdeadc0de.
PROBLEM: fanotify_mark EFAULT on x86
I am trying to run examples from man fanotify.7 but fanotify_mark always fail with errno = EFAULT. fanotify_mark declaration is SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, __u64, mask, int, dfd, const char __user *, pathname) When fanotify_mark(4, FAN_MARK_ADD | FAN_MARK_ONLYDIR, FAN_CREATE | FAN_ONDIR, AT_FDCWD, 0xdeadc0de) is called on kernel side I can see in do_syscall_32_irqs_on that CPU context is bx = 0x4= 4 cx = 0x9= FAN_MARK_ADD | FAN_MARK_ONLYDIR, dx = 0x4100 = FAN_CREATE | FAN_ONDIR si = 0x0 di = 0xff9c = AT_FDCWD bp = 0xdeadc0de ax = 0xffda orix_ax = 0x153 I am not sure if it is ok because third argument is uint64_t so if I understand correctly mask should be divided into two registers (dx and si). But in fanotify_mark we get fanotify_fd = 4 = bx flags = 0x9= cx mask= 0x4100 = dx dfd = 0 = si pathname= 0xff9c = di I believe that correct order is fanotify_fd = 4 = bx flags = 0x9= cx mask= 0x4100 = (si << 32) | dx dfd = 0xff9c = di pathname= 0xdeadc0de = bp I think that we should call COMPAT version of fanotify_mark here COMPAT_SYSCALL_DEFINE6(fanotify_mark, int, fanotify_fd, unsigned int, flags, __u32, mask0, __u32, mask1, int, dfd, const char __user *, pathname) or something wrong is with 64-bits arguments. I am running Linux 5.9.2 i686 on Pentium III (Coppermine). For tests I am using Debian sid on qemu with 5.9.2 and default kernel from repositories. Everything works fine on 5.5 and 5.4. -- Paweł Jasiak signature.asc Description: PGP signature