| Message ID | 061a0de2042156669303f95526ec13476bf490c7.1567072270.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Disable compat cruft on ppc64le v4 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (6ed37b3ef6aef8647b8dad925affa92c4c8cc8a3) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > Fixes: aff850393200 ("powerpc: add system call table generation support") This patch needs a proper explanation. The Fixes tag doesn't seem right here, since ppc64 has had llseek since the start in 2002 commit 3939e37587e7 ("Add ppc64 support. This includes both pSeries (RS/6000) and iSeries (AS/400)."). > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl > index 010b9f445586..53e427606f6c 100644 > --- a/arch/powerpc/kernel/syscalls/syscall.tbl > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl > @@ -188,7 +188,7 @@ > 137 common afs_syscall sys_ni_syscall > 138 common setfsuid sys_setfsuid > 139 common setfsgid sys_setfsgid > -140 common _llseek sys_llseek > +140 32 _llseek sys_llseek > 141 common getdents sys_getdents compat_sys_getdents > 142 common _newselect sys_select compat_sys_select > 143 common flock sys_flock In particular, I don't see why you single out llseek here, but leave other syscalls that are not needed on 64-bit machines such as pread64(). ARnd
On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > > > Fixes: aff850393200 ("powerpc: add system call table generation support") > > This patch needs a proper explanation. The Fixes tag doesn't seem right > here, since ppc64 has had llseek since the start in 2002 commit 3939e37587e7 > ("Add ppc64 support. This includes both pSeries (RS/6000) and iSeries > (AS/400)."). > > > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl > > index 010b9f445586..53e427606f6c 100644 > > --- a/arch/powerpc/kernel/syscalls/syscall.tbl > > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl > > @@ -188,7 +188,7 @@ > > 137 common afs_syscall sys_ni_syscall > > 138 common setfsuid sys_setfsuid > > 139 common setfsgid sys_setfsgid > > -140 common _llseek sys_llseek > > +140 32 _llseek sys_llseek > > 141 common getdents sys_getdents compat_sys_getdents > > 142 common _newselect sys_select compat_sys_select > > 143 common flock sys_flock > > In particular, I don't see why you single out llseek here, but leave other > syscalls that are not needed on 64-bit machines such as pread64(). Because llseek is not built in fs/ when building 64bit only causing a link error. I initially posted patch to build it always but it was pointed out it is not needed, and the interface does not make sense on 64bit, and that platforms that don't have it on 64bit now don't want that useless code. Thanks Michal
On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <msuchanek@suse.de> wrote: > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > In particular, I don't see why you single out llseek here, but leave other > > syscalls that are not needed on 64-bit machines such as pread64(). > > Because llseek is not built in fs/ when building 64bit only causing a > link error. > > I initially posted patch to build it always but it was pointed out it > is not needed, and the interface does not make sense on 64bit, and > that platforms that don't have it on 64bit now don't want that useless > code. Ok, please put that into the changeset description then. I looked at uses of __NR__llseek in debian code search and found this one: https://codesearch.debian.net/show?file=umview_0.8.2-1.2%2Fxmview%2Fum_mmap.c&line=328 It looks like this application will try to use llseek instead of lseek when built against kernel headers that define __NR_llseek. Changing the powerpc kernel not to provide that to user space may break it unless the program gets recompiled against the latest headers. Arnd
On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <msuchanek@suse.de> wrote: > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > > In particular, I don't see why you single out llseek here, but leave other > > > syscalls that are not needed on 64-bit machines such as pread64(). > > > > Because llseek is not built in fs/ when building 64bit only causing a > > link error. > > > > I initially posted patch to build it always but it was pointed out it > > is not needed, and the interface does not make sense on 64bit, and > > that platforms that don't have it on 64bit now don't want that useless > > code. > > Ok, please put that into the changeset description then. > > I looked at uses of __NR__llseek in debian code search and > found this one: > > https://codesearch.debian.net/show?file=umview_0.8.2-1.2%2Fxmview%2Fum_mmap.c&line=328 > > It looks like this application will try to use llseek instead of lseek > when built against kernel headers that define __NR_llseek. > The available documentation says this syscall is for 32bit only so using it on 64bit is undefined. The interface is not well-defined in that case either. Thanks Michal
On Thu, Aug 29, 2019 at 4:19 PM Michal Suchánek <msuchanek@suse.de> wrote: > On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <msuchanek@suse.de> wrote: > > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > > > In particular, I don't see why you single out llseek here, but leave other > > > > syscalls that are not needed on 64-bit machines such as pread64(). > > > > > > Because llseek is not built in fs/ when building 64bit only causing a > > > link error. > > > > > > I initially posted patch to build it always but it was pointed out it > > > is not needed, and the interface does not make sense on 64bit, and > > > that platforms that don't have it on 64bit now don't want that useless > > > code. > > > > Ok, please put that into the changeset description then. > > > > I looked at uses of __NR__llseek in debian code search and > > found this one: > > > > https://codesearch.debian.net/show?file=umview_0.8.2-1.2%2Fxmview%2Fum_mmap.c&line=328 > > > > It looks like this application will try to use llseek instead of lseek > > when built against kernel headers that define __NR_llseek. > > > > The available documentation says this syscall is for 32bit only so > using it on 64bit is undefined. The interface is not well-defined in > that case either. That's generally not how it works. If there is an existing application that relies on the behavior of the system call interface, we should not change it in a way that breaks the application, regardless of what the documentation says. Presumably nobody cares about umview on powerpc64, but there might be other applications doing the same thing. It looks like sparc64 and parisc64 do the same thing as powerpc64, and provide llseek() calls that may or may not be used by applications. I think your original approach of always building sys_llseek on powerpc64 is the safe choice here. Arnd
On Thu, 29 Aug 2019 16:32:50 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Aug 29, 2019 at 4:19 PM Michal Suchánek <msuchanek@suse.de> wrote: > > On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <msuchanek@suse.de> wrote: > > > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <msuchanek@suse.de> wrote: > > > > > In particular, I don't see why you single out llseek here, but leave other > > > > > syscalls that are not needed on 64-bit machines such as pread64(). > > > > > > > > Because llseek is not built in fs/ when building 64bit only causing a > > > > link error. > > > > > > > > I initially posted patch to build it always but it was pointed out it > > > > is not needed, and the interface does not make sense on 64bit, and > > > > that platforms that don't have it on 64bit now don't want that useless > > > > code. > > > > > > Ok, please put that into the changeset description then. > > > > > > I looked at uses of __NR__llseek in debian code search and > > > found this one: > > > > > > https://codesearch.debian.net/show?file=umview_0.8.2-1.2%2Fxmview%2Fum_mmap.c&line=328 > > > > > > It looks like this application will try to use llseek instead of lseek > > > when built against kernel headers that define __NR_llseek. > > > > > > > The available documentation says this syscall is for 32bit only so > > using it on 64bit is undefined. The interface is not well-defined in > > that case either. > > That's generally not how it works. If there is an existing application > that relies on the behavior of the system call interface, we should not > change it in a way that breaks the application, regardless of what the > documentation says. Presumably nobody cares about umview on > powerpc64, but there might be other applications doing the same > thing. Actually the umview headers go out of their way to define the llseek syscall as invalid on x86_64 so that the non-llseek path is taken. mview-os/xmview/defs_x86_64_um.h:#define __NR__llseek __NR_doesnotexist It is probably an oversight that this is not done on non-x86. I am not even sure this builds on non-x86 out of the box. > It looks like sparc64 and parisc64 do the same thing as powerpc64, > and provide llseek() calls that may or may not be used by > applications. And if they are supposed to build with !compat it should be removed there as well. > > I think your original approach of always building sys_llseek on > powerpc64 is the safe choice here. That's safe but adds junk to the kernel as pointed out in the reply to that patch. Thanks Michal
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index 010b9f445586..53e427606f6c 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -188,7 +188,7 @@ 137 common afs_syscall sys_ni_syscall 138 common setfsuid sys_setfsuid 139 common setfsgid sys_setfsgid -140 common _llseek sys_llseek +140 32 _llseek sys_llseek 141 common getdents sys_getdents compat_sys_getdents 142 common _newselect sys_select compat_sys_select 143 common flock sys_flock
Fixes: aff850393200 ("powerpc: add system call table generation support") Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- arch/powerpc/kernel/syscalls/syscall.tbl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)