Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, 29 Aug 2019 16:32:50 +0200 Arnd Bergmann wrote: > On Thu, Aug 29, 2019 at 4:19 PM Michal Suchánek wrote: > > On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann wrote: > > > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek > > > wrote: > > > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann wrote: > > > > > > > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek > > > > > 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=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
Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, Aug 29, 2019 at 4:19 PM Michal Suchánek wrote: > On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann wrote: > > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek wrote: > > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann wrote: > > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek > > > > 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=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
Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann wrote: > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek wrote: > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann wrote: > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek > > > 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=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
Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek wrote: > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann wrote: > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek 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=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
Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, 29 Aug 2019 14:19:46 +0200
Arnd Bergmann wrote:
> On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek 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 @@
> > 137common afs_syscall sys_ni_syscall
> > 138common setfsuidsys_setfsuid
> > 139common setfsgidsys_setfsgid
> > -140common _llseek sys_llseek
> > +14032 _llseek sys_llseek
> > 141common getdentssys_getdents
> > compat_sys_getdents
> > 142common _newselect sys_select
> > compat_sys_select
> > 143common 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
Re: [PATCH v4 1/4] powerpc: make llseek 32bit-only.
On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek 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 @@
> 137common afs_syscall sys_ni_syscall
> 138common setfsuidsys_setfsuid
> 139common setfsgidsys_setfsgid
> -140common _llseek sys_llseek
> +14032 _llseek sys_llseek
> 141common getdentssys_getdents
> compat_sys_getdents
> 142common _newselect sys_select
> compat_sys_select
> 143common 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
[PATCH v4 1/4] powerpc: make llseek 32bit-only.
Fixes: aff850393200 ("powerpc: add system call table generation support")
Signed-off-by: Michal Suchanek
---
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 @@
137common afs_syscall sys_ni_syscall
138common setfsuidsys_setfsuid
139common setfsgidsys_setfsgid
-140common _llseek sys_llseek
+14032 _llseek sys_llseek
141common getdentssys_getdents
compat_sys_getdents
142common _newselect sys_select
compat_sys_select
143common flock sys_flock
--
2.22.0
