Fixes: aff850393200 ("powerpc: add system call table generation support")
Signed-off-by: Michal Suchanek <[email protected]>
---
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 @@
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
--
2.22.0
On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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 <[email protected]> wrote:
> On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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 <[email protected]> wrote:
> On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <[email protected]> wrote:
> > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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 <[email protected]> wrote:
> On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <[email protected]> wrote:
> > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <[email protected]> wrote:
> > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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 <[email protected]> wrote:
> On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann <[email protected]> wrote:
> > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <[email protected]> wrote:
> > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <[email protected]> wrote:
> > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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 <[email protected]> wrote:
> On Thu, Aug 29, 2019 at 4:19 PM Michal Suchánek <[email protected]> wrote:
> > On Thu, 29 Aug 2019 14:57:39 +0200 Arnd Bergmann <[email protected]> wrote:
> > > On Thu, Aug 29, 2019 at 2:37 PM Michal Suchánek <[email protected]> wrote:
> > > > On Thu, 29 Aug 2019 14:19:46 +0200 Arnd Bergmann <[email protected]> wrote:
> > > > > On Thu, Aug 29, 2019 at 12:23 PM Michal Suchanek <[email protected]> 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