* [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. @ 2019-04-17 12:35 Boyang Zhou 2019-04-18 11:28 ` Mark Rutland 0 siblings, 1 reply; 6+ messages in thread From: Boyang Zhou @ 2019-04-17 12:35 UTC (permalink / raw) To: catalin.marinas Cc: will.deacon, haozhu, mark.rutland, linux, linux-arm-kernel, linux-kernel, Boyang Zhou The error information is that “offset value too large for defined data type”. Reason: On the X86 platform, the data type of “off" is unsigned long; but on the ARM64 platform, the data type is defined as off_t, and off_t is by type long instead of unsigned long. When the off right shifts in the function “sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT)"on ARM64, high address of off is filled with sign bit 1instead of 0. In our case, we mmap GPU doorbell on both platform. On the x86 platform, the value of off is f009c00000000000, after shift the value becomes f009c00000000; while on the ARM64, the value of off changes from ed35c00000000000 to fffed35c00000000. This value is treated as unsigned long in later functions. So it is too big for off and the error happened. We have tested the patchs in Huawei ARM64 server with a couples of AMD GPUs. Signed-off-by: Boyang Zhou <zhouby_cn@126.com> --- arch/arm64/kernel/sys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c index b44065f..6f91e81 100644 --- a/arch/arm64/kernel/sys.c +++ b/arch/arm64/kernel/sys.c @@ -31,7 +31,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, unsigned long, prot, unsigned long, flags, - unsigned long, fd, off_t, off) + unsigned long, fd, unsigned long, off) { if (offset_in_page(off) != 0) return -EINVAL; -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. 2019-04-17 12:35 [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform Boyang Zhou @ 2019-04-18 11:28 ` Mark Rutland 2019-04-23 17:04 ` Will Deacon 2019-04-24 8:31 ` David Howells 0 siblings, 2 replies; 6+ messages in thread From: Mark Rutland @ 2019-04-18 11:28 UTC (permalink / raw) To: Boyang Zhou Cc: catalin.marinas, will.deacon, haozhu, linux, linux-arm-kernel, linux-kernel, linux-arch, arnd, viro, akpm [adding linux-arch and relevant folk] On Wed, Apr 17, 2019 at 08:35:25PM +0800, Boyang Zhou wrote: > The error information is that “offset value too large for defined data type”. > Reason: > On the X86 platform, the data type of “off" is unsigned long; but on the ARM64 platform, the data type is defined as off_t, and off_t is by type long instead of unsigned long. > When the off right shifts in the function “sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT)"on ARM64, high address of off is filled with sign bit 1instead of 0. > In our case, we mmap GPU doorbell on both platform. On the x86 platform, the value of off is f009c00000000000, after shift the value becomes f009c00000000; while on the ARM64, the value of off changes from ed35c00000000000 to fffed35c00000000. This value is treated as unsigned long in later functions. So it is too big for off and the error happened. > We have tested the patchs in Huawei ARM64 server with a couples of AMD GPUs. It looks like the generic mmap uses unsigned long, as do sparc and x86. However, arm64, microblase, powerpc and riscv all use off_t. Should those all be using unsigned long? If so, that seems like it should be a treewide cleanup. Similar applies to pgoff for mmap2. Thanks, Mark. > > Signed-off-by: Boyang Zhou <zhouby_cn@126.com> > --- > arch/arm64/kernel/sys.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c > index b44065f..6f91e81 100644 > --- a/arch/arm64/kernel/sys.c > +++ b/arch/arm64/kernel/sys.c > @@ -31,7 +31,7 @@ > > SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, > unsigned long, prot, unsigned long, flags, > - unsigned long, fd, off_t, off) > + unsigned long, fd, unsigned long, off) > { > if (offset_in_page(off) != 0) > return -EINVAL; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. 2019-04-18 11:28 ` Mark Rutland @ 2019-04-23 17:04 ` Will Deacon 2019-04-23 17:20 ` Mark Rutland 2019-04-24 8:31 ` David Howells 1 sibling, 1 reply; 6+ messages in thread From: Will Deacon @ 2019-04-23 17:04 UTC (permalink / raw) To: Mark Rutland Cc: Boyang Zhou, catalin.marinas, haozhu, linux, linux-arm-kernel, linux-kernel, linux-arch, arnd, viro, akpm On Thu, Apr 18, 2019 at 12:28:18PM +0100, Mark Rutland wrote: > [adding linux-arch and relevant folk] > > On Wed, Apr 17, 2019 at 08:35:25PM +0800, Boyang Zhou wrote: > > The error information is that “offset value too large for defined data type”. > > Reason: > > On the X86 platform, the data type of “off" is unsigned long; but on the ARM64 platform, the data type is defined as off_t, and off_t is by type long instead of unsigned long. > > When the off right shifts in the function “sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT)"on ARM64, high address of off is filled with sign bit 1instead of 0. > > In our case, we mmap GPU doorbell on both platform. On the x86 platform, the value of off is f009c00000000000, after shift the value becomes f009c00000000; while on the ARM64, the value of off changes from ed35c00000000000 to fffed35c00000000. This value is treated as unsigned long in later functions. So it is too big for off and the error happened. > > We have tested the patchs in Huawei ARM64 server with a couples of AMD GPUs. > > It looks like the generic mmap uses unsigned long, as do sparc and x86. > > However, arm64, microblase, powerpc and riscv all use off_t. > > Should those all be using unsigned long? If so, that seems like it > should be a treewide cleanup. This is more than just a cleanup, since it changes the behaviour of the system call and I'd much rather each architecture made this choice themselves. I also don't think we should change the type of the parameter, so something like the following instead: diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c index b44065fb1616..77dc5f006bc4 100644 --- a/arch/arm64/kernel/sys.c +++ b/arch/arm64/kernel/sys.c @@ -31,8 +31,10 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, unsigned long, prot, unsigned long, flags, - unsigned long, fd, off_t, off) + unsigned long, fd, off_t, pgoff) { + unsigned long off = pgoff; + if (offset_in_page(off) != 0) return -EINVAL; > Similar applies to pgoff for mmap2. Does it? Looks like unsigned long is used consistently there afaict. Did you spot something I missed? Will ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. 2019-04-23 17:04 ` Will Deacon @ 2019-04-23 17:20 ` Mark Rutland 0 siblings, 0 replies; 6+ messages in thread From: Mark Rutland @ 2019-04-23 17:20 UTC (permalink / raw) To: Will Deacon Cc: Boyang Zhou, catalin.marinas, haozhu, linux, linux-arm-kernel, linux-kernel, linux-arch, arnd, viro, akpm On Tue, Apr 23, 2019 at 06:04:45PM +0100, Will Deacon wrote: > On Thu, Apr 18, 2019 at 12:28:18PM +0100, Mark Rutland wrote: > > [adding linux-arch and relevant folk] > > > > On Wed, Apr 17, 2019 at 08:35:25PM +0800, Boyang Zhou wrote: > > > The error information is that “offset value too large for defined data type”. > > > Reason: > > > On the X86 platform, the data type of “off" is unsigned long; but on the ARM64 platform, the data type is defined as off_t, and off_t is by type long instead of unsigned long. > > > When the off right shifts in the function “sys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT)"on ARM64, high address of off is filled with sign bit 1instead of 0. > > > In our case, we mmap GPU doorbell on both platform. On the x86 platform, the value of off is f009c00000000000, after shift the value becomes f009c00000000; while on the ARM64, the value of off changes from ed35c00000000000 to fffed35c00000000. This value is treated as unsigned long in later functions. So it is too big for off and the error happened. > > > We have tested the patchs in Huawei ARM64 server with a couples of AMD GPUs. > > > > It looks like the generic mmap uses unsigned long, as do sparc and x86. > > > > However, arm64, microblase, powerpc and riscv all use off_t. > > > > Should those all be using unsigned long? If so, that seems like it > > should be a treewide cleanup. > > This is more than just a cleanup, since it changes the behaviour of the > system call and I'd much rather each architecture made this choice > themselves. I also don't think we should change the type of the parameter, > so something like the following instead: > > diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c > index b44065fb1616..77dc5f006bc4 100644 > --- a/arch/arm64/kernel/sys.c > +++ b/arch/arm64/kernel/sys.c > @@ -31,8 +31,10 @@ > > SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len, > unsigned long, prot, unsigned long, flags, > - unsigned long, fd, off_t, off) > + unsigned long, fd, off_t, pgoff) > { > + unsigned long off = pgoff; > + > if (offset_in_page(off) != 0) > return -EINVAL; To be honest, I think changing the parameter itself would be cleaner, but I'm not the maintainer. :) > > Similar applies to pgoff for mmap2. > > Does it? Looks like unsigned long is used consistently there afaict. Did > you spot something I missed? Sorry, I meant tree-wide where csky and riscv use off_t, and all others use unsigned long. Thanks, Mark. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. 2019-04-18 11:28 ` Mark Rutland 2019-04-23 17:04 ` Will Deacon @ 2019-04-24 8:31 ` David Howells 2019-04-24 13:50 ` Will Deacon 1 sibling, 1 reply; 6+ messages in thread From: David Howells @ 2019-04-24 8:31 UTC (permalink / raw) To: Will Deacon Cc: dhowells, Mark Rutland, Boyang Zhou, catalin.marinas, haozhu, linux, linux-arm-kernel, linux-kernel, linux-arch, arnd, viro, akpm Will Deacon <will.deacon@arm.com> wrote: > - unsigned long, fd, off_t, off) > + unsigned long, fd, off_t, pgoff) > { > + unsigned long off = pgoff; > + Would loff_t be better? David ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform. 2019-04-24 8:31 ` David Howells @ 2019-04-24 13:50 ` Will Deacon 0 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2019-04-24 13:50 UTC (permalink / raw) To: David Howells Cc: Mark Rutland, Boyang Zhou, catalin.marinas, haozhu, linux, linux-arm-kernel, linux-kernel, linux-arch, arnd, viro, akpm On Wed, Apr 24, 2019 at 09:31:28AM +0100, David Howells wrote: > Will Deacon <will.deacon@arm.com> wrote: > > > - unsigned long, fd, off_t, off) > > + unsigned long, fd, off_t, pgoff) > > { > > + unsigned long off = pgoff; > > + > > Would loff_t be better? I don't think so, isn't that still signed? I also think it's the same size as off_t on arm64 (long long vs long). Will ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-24 13:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-17 12:35 [PATCH] The patch solves the type error of the parameter “off” in syscall mmap on the ARM64 platform Boyang Zhou 2019-04-18 11:28 ` Mark Rutland 2019-04-23 17:04 ` Will Deacon 2019-04-23 17:20 ` Mark Rutland 2019-04-24 8:31 ` David Howells 2019-04-24 13:50 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).