* Re: C vdso
From: Christophe Leroy @ 2020-10-24 11:16 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <874kmkx7gi.fsf@mpe.ellerman.id.au>
Le 24/10/2020 à 12:07, Michael Ellerman a écrit :
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Le 24/09/2020 à 15:17, Christophe Leroy a écrit :
>>>> Le 17/09/2020 à 14:33, Michael Ellerman a écrit :
>>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>>>
>>>>>> What is the status with the generic C vdso merge ?
>>>>>> In some mail, you mentionned having difficulties getting it working on
>>>>>> ppc64, any progress ? What's the problem ? Can I help ?
>>>>>
>>>>> Yeah sorry I was hoping to get time to work on it but haven't been able
>>>>> to.
>>>>>
>>>>> It's causing crashes on ppc64 ie. big endian.
>> ...
>>>>
>>>> Can you tell what defconfig you are using ? I have been able to setup a full glibc PPC64 cross
>>>> compilation chain and been able to test it under QEMU with success, using Nathan's vdsotest tool.
>>>
>>> What config are you using ?
>>
>> ppc64_defconfig + guest.config
>>
>> Or pseries_defconfig.
>>
>> I'm using Ubuntu GCC 9.3.0 mostly, but it happens with other toolchains too.
>
> I'm also seeing warnings because of the feature fixups:
>
>
> That's happening because the 32-bit VDSO is built with CONFIG_PPC32=y,
> due to config-fake32.h, and that causes the feature fixup entries to be
> the wrong size.
>
> See the logic in feature-fixup.h:
>
>
>
> We expect the fixup entries to still use 64-bit values, even for the
> 32-bit VDSO in a 64-bit kernel.
>
> TBH I'm not sure how config-fake32.h can work long term, it's so fragile
> to be defining/redefining a handful of CONFIG symbols like that.
I took the idea from mips (arch/mips/vdso/config-n32-o32-env.c) after struggling in different
direction. At that time the generic VDSO code was far less careful and was including several linux
headers IIRC.
I agree with you that it's rather fragile.
>
> The generic VDSO code is fairly careful to only include uapi and vdso
> headers, not linux ones. So I think we need to better split our headers
> so that we can build the VDSO code with few or no linux headers, and so
> avoid the need to define any (or most) CONFIG symbols.
>
I'll revisit it when I'm back from vacation (I'm leaving now for two weeks).
Christophe
^ permalink raw reply
* [GIT PULL] Please pull powerpc/linux.git powerpc-5.10-2 tag
From: Michael Ellerman @ 2020-10-24 10:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: mikey, srikar, aneesh.kumar, linux-kernel, hegdevasant, ganeshgr,
jniethe5, oohall, linuxppc-dev
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Hi Linus,
Please pull powerpc fixes for 5.10:
The following changes since commit ffd0b25ca049a477cb757e5bcf2d5e1664d12e5d:
Revert "powerpc/pci: unmap legacy INTx interrupts when a PHB is removed" (2020-10-15 13:42:49 +1100)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.10-2
for you to fetch changes up to 4ff753feab021242144818b9a3ba011238218145:
powerpc/pseries: Avoid using addr_to_pfn in real mode (2020-10-22 14:34:45 +1100)
- ------------------------------------------------------------------
powerpc fixes for 5.10 #2
A fix for undetected data corruption on Power9 Nimbus <= DD2.1 in the emulation
of VSX loads. The affected CPUs were not widely available.
Two fixes for machine check handling in guests under PowerVM.
A fix for our recent changes to SMP setup, when CONFIG_CPUMASK_OFFSTACK=y.
Three fixes for races in the handling of some of our powernv sysfs attributes.
One change to remove TM from the set of Power10 CPU features.
A couple of other minor fixes.
Thanks to:
Aneesh Kumar K.V, Christophe Leroy, Ganesh Goudar, Jordan Niethe, Mahesh
Salgaonkar, Michael Neuling, Oliver O'Halloran, Qian Cai, Srikar Dronamraju,
Vasant Hegde.
- ------------------------------------------------------------------
Aneesh Kumar K.V (1):
powerpc/opal_elog: Handle multiple writes to ack attribute
Christophe Leroy (1):
powerpc/uaccess: Don't use "m<>" constraint with GCC 4.9
Ganesh Goudar (2):
powerpc/mce: Avoid nmi_enter/exit in real mode on pseries hash
powerpc/pseries: Avoid using addr_to_pfn in real mode
Jordan Niethe (1):
powerpc/64s: Remove TM from Power10 features
Michael Neuling (2):
powerpc: Fix undetected data corruption with P9N DD2.1 VSX CI load emulation
selftests/powerpc: Make alignment handler test P9N DD2.1 vector CI load workaround
Oliver O'Halloran (1):
powerpc/eeh: Fix eeh_dev_check_failure() for PE#0
Srikar Dronamraju (2):
powerpc/smp: Remove unnecessary variable
powerpc/smp: Use GFP_ATOMIC while allocating tmp mask
Vasant Hegde (2):
powerpc/powernv/dump: Fix race while processing OPAL dump
powerpc/powernv/dump: Handle multiple writes to ack attribute
arch/powerpc/include/asm/asm-const.h | 13 +++
arch/powerpc/include/asm/cputable.h | 2 +-
arch/powerpc/include/asm/uaccess.h | 4 +-
arch/powerpc/kernel/cputable.c | 13 ++-
arch/powerpc/kernel/eeh.c | 5 -
arch/powerpc/kernel/mce.c | 7 +-
arch/powerpc/kernel/smp.c | 70 ++++++------
arch/powerpc/kernel/traps.c | 2 +-
arch/powerpc/platforms/powernv/opal-dump.c | 52 ++++++---
arch/powerpc/platforms/powernv/opal-elog.c | 11 +-
arch/powerpc/platforms/pseries/ras.c | 118 ++++++++++++--------
tools/testing/selftests/powerpc/alignment/alignment_handler.c | 8 +-
12 files changed, 185 insertions(+), 120 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAl+UBeUACgkQUevqPMjh
pYBHUA//THLt6DJlSPPqn8LQZQGT76Gx82cKyy9DQ7/Elcl13xcuq3XbhHD5asi0
QbJGbLhRqpRhtmj3c8BCYAygi5FXZWH4IeN6FK8xoZGR2bi/gY7VkhIUSzFAHnRi
PFXafzb8eWVS7O5k8xbxrjxdOAu8SjEzywG5I8PPn5IWFwhUwjGosv81QtxJOLVc
V9WwuTBK87nfvoMdfcl3YJXRs+4vKOQQ0Gqa5vTVTUmgdbJOqJi1MvLULnSnKxTJ
G+XplOeSI1N3gk+E2cycPasghTYziTtzEyrTHe0Uufgx+9t6VuN1g2zL81kDA7U9
10Oqqry4Z66V2BhGrDMnXKYGeQNGRO8vNLT2DuuZd5XTN/LpV0knJHm/9F2E+5zl
T+GgQwS0IhXDcbS70TcbxXHPyBe2/eXRH1+rkSlAEjl656JVbKefgi1VUsqSzkjH
JBF2+zCodYelbbnRP89Aj5/03t+VeHbNC/1jixoYDHR7drXiU2XQqjfFZeCHvxOQ
YCznpoC84gcDupGC5q4op3tHBmvULJn0KaHYWryaAEWlCxjdVcjBis48B+GQVww8
JnDMC5WGVvAAxPHc74EkyEvdROx4Q+8UeOj+TXnrRlowEF8Wymxcvy7NUn2Bqq2J
VsRCUzLIReKCckdJQ/+SxG8eb9JUxQRWG76+Q9zzTHbdaBSWuMc=
=9Oxg
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] powerpc/eeh: Fix eeh_dev_check_failure() for PE#0
From: Michael Ellerman @ 2020-10-24 10:27 UTC (permalink / raw)
To: linuxppc-dev, Oliver O'Halloran
In-Reply-To: <20201021232554.1434687-1-oohall@gmail.com>
On Thu, 22 Oct 2020 10:25:54 +1100, Oliver O'Halloran wrote:
> In commit 269e583357df ("powerpc/eeh: Delete eeh_pe->config_addr") the
> following simplification was made:
>
> - if (!pe->addr && !pe->config_addr) {
> + if (!pe->addr) {
> eeh_stats.no_cfg_addr++;
> return 0;
> }
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/eeh: Fix eeh_dev_check_failure() for PE#0
https://git.kernel.org/powerpc/c/99f6e9795a68fe23f96a2b5b0be07a3dd9457f99
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/64s: Remove TM from Power10 features
From: Michael Ellerman @ 2020-10-24 10:27 UTC (permalink / raw)
To: linuxppc-dev, Jordan Niethe
In-Reply-To: <20200827035529.900-1-jniethe5@gmail.com>
On Thu, 27 Aug 2020 13:55:29 +1000, Jordan Niethe wrote:
> ISA v3.1 removes transactional memory and hence it should not be present
> in cpu_features or cpu_user_features2. Remove CPU_FTR_TM_COMP from
> CPU_FTRS_POWER10. Remove PPC_FEATURE2_HTM_COMP and
> PPC_FEATURE2_HTM_NOSC_COMP from COMMON_USER2_POWER10.
Applied to powerpc/fixes.
[1/1] powerpc/64s: Remove TM from Power10 features
https://git.kernel.org/powerpc/c/ec613a57fa1d57381f890c3166175fe68cf43f12
cheers
^ permalink raw reply
* Re: [PATCH v2 1/3] powerpc/uaccess: Don't use "m<>" constraint with GCC 4.9
From: Michael Ellerman @ 2020-10-24 10:27 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Paul Mackerras,
Benjamin Herrenschmidt, mathieu.desnoyers
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <212d3bc4a52ca71523759517bb9c61f7e477c46a.1603179582.git.christophe.leroy@csgroup.eu>
On Tue, 20 Oct 2020 07:40:07 +0000 (UTC), Christophe Leroy wrote:
> GCC 4.9 sometimes fails to build with "m<>" constraint in
> inline assembly.
>
> CC lib/iov_iter.o
> In file included from ./arch/powerpc/include/asm/cmpxchg.h:6:0,
> from ./arch/powerpc/include/asm/atomic.h:11,
> from ./include/linux/atomic.h:7,
> from ./include/linux/crypto.h:15,
> from ./include/crypto/hash.h:11,
> from lib/iov_iter.c:2:
> lib/iov_iter.c: In function 'iovec_from_user.part.30':
> ./arch/powerpc/include/asm/uaccess.h:287:2: error: 'asm' operand has impossible constraints
> __asm__ __volatile__( \
> ^
> ./include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
> # define unlikely(x) __builtin_expect(!!(x), 0)
> ^
> ./arch/powerpc/include/asm/uaccess.h:583:34: note: in expansion of macro 'unsafe_op_wrap'
> #define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
> ^
> ./arch/powerpc/include/asm/uaccess.h:329:10: note: in expansion of macro '__get_user_asm'
> case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
> ^
> ./arch/powerpc/include/asm/uaccess.h:363:3: note: in expansion of macro '__get_user_size_allowed'
> __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
> ^
> ./arch/powerpc/include/asm/uaccess.h:100:2: note: in expansion of macro '__get_user_nocheck'
> __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
> ^
> ./arch/powerpc/include/asm/uaccess.h:583:49: note: in expansion of macro '__get_user_allowed'
> #define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
> ^
> lib/iov_iter.c:1663:3: note: in expansion of macro 'unsafe_get_user'
> unsafe_get_user(len, &uiov[i].iov_len, uaccess_end);
> ^
> make[1]: *** [scripts/Makefile.build:283: lib/iov_iter.o] Error 1
>
> [...]
Patch 1 applied to powerpc/fixes.
[1/3] powerpc/uaccess: Don't use "m<>" constraint with GCC 4.9
https://git.kernel.org/powerpc/c/592bbe9c505d9a0ef69260f8c8263df47da2698e
cheers
^ permalink raw reply
* Re: [PATCH v4] powerpc/pseries: Avoid using addr_to_pfn in real mode
From: Michael Ellerman @ 2020-10-24 10:27 UTC (permalink / raw)
To: linuxppc-dev, mpe, Ganesh Goudar; +Cc: aneesh.kumar, npiggin, mahesh
In-Reply-To: <20200724063946.21378-1-ganeshgr@linux.ibm.com>
On Fri, 24 Jul 2020 12:09:46 +0530, Ganesh Goudar wrote:
> When an UE or memory error exception is encountered the MCE handler
> tries to find the pfn using addr_to_pfn() which takes effective
> address as an argument, later pfn is used to poison the page where
> memory error occurred, recent rework in this area made addr_to_pfn
> to run in real mode, which can be fatal as it may try to access
> memory outside RMO region.
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/pseries: Avoid using addr_to_pfn in real mode
https://git.kernel.org/powerpc/c/4ff753feab021242144818b9a3ba011238218145
cheers
^ permalink raw reply
* Re: C vdso
From: Michael Ellerman @ 2020-10-24 10:07 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <877drhxeg8.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 24/09/2020 à 15:17, Christophe Leroy a écrit :
>>> Le 17/09/2020 à 14:33, Michael Ellerman a écrit :
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>>
>>>>> What is the status with the generic C vdso merge ?
>>>>> In some mail, you mentionned having difficulties getting it working on
>>>>> ppc64, any progress ? What's the problem ? Can I help ?
>>>>
>>>> Yeah sorry I was hoping to get time to work on it but haven't been able
>>>> to.
>>>>
>>>> It's causing crashes on ppc64 ie. big endian.
> ...
>>>
>>> Can you tell what defconfig you are using ? I have been able to setup a full glibc PPC64 cross
>>> compilation chain and been able to test it under QEMU with success, using Nathan's vdsotest tool.
>>
>> What config are you using ?
>
> ppc64_defconfig + guest.config
>
> Or pseries_defconfig.
>
> I'm using Ubuntu GCC 9.3.0 mostly, but it happens with other toolchains too.
I'm also seeing warnings because of the feature fixups:
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at arch/powerpc/lib/feature-fixups.c:109 .do_feature_fixups+0x80/0xc0
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc2-00261-g107a86292cc4 #11
NIP: c0000000000a3660 LR: c0000000000a362c CTR: 0000000000000000
REGS: c00000007e3a3790 TRAP: 0700 Not tainted (5.9.0-rc2-00261-g107a86292cc4)
MSR: 8000000002029032 <SF,VEC,EE,ME,IR,DR,RI> CR: 48000422 XER: 00000000
CFAR: c0000000000a3630 IRQMASK: 0
GPR00: c0000000011e8964 c00000007e3a3a20 c000000001bb2b00 0000000000000001
GPR04: c000000001bc0bc0 c000000001bc0bf0 0000000066736574 00000000fffffe00
GPR08: 0000000300000004 0000000000000000 0000000000000000 0000000000000db8
GPR12: 0000000028000224 c000000001dc0000 c000000000012d40 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: c000000001aae0f0 c000000001063f08 c000000001063f18 c000000001063f28
GPR28: c00000000106e188 000000eb8f4d91a7 c000000001bc0bf0 c000000001bc0bc0
NIP [c0000000000a3660] .do_feature_fixups+0x80/0xc0
LR [c0000000000a362c] .do_feature_fixups+0x4c/0xc0
Call Trace:
[c00000007e3a3a20] [c0000000000a362c] .do_feature_fixups+0x4c/0xc0 (unreliable)
[c00000007e3a3ab0] [c0000000011e8964] .vdso_init+0x498/0x95c
[c00000007e3a3bd0] [c000000000012780] .do_one_initcall+0x60/0x2b8
[c00000007e3a3cb0] [c0000000011e4d8c] .kernel_init_freeable+0x2d8/0x370
[c00000007e3a3da0] [c000000000012d64] .kernel_init+0x24/0x150
[c00000007e3a3e20] [c00000000000e24c] .ret_from_kernel_thread+0x58/0x6c
Instruction dump:
40820030 3bff0030 7c3ef840 4181ffe4 38210090 e8010010 eb81ffe0 eba1ffe8
ebc1fff0 7c0803a6 ebe1fff8 4e800020 <0fe00000> e8ff0028 e8df0020 7f83e378
---[ end trace ece1c957ca5bd6e9 ]---
Unable to patch feature section at bffffffd01bc0bbc - c000000001bc0bc0 with bffffd9101bc0958 - bfffffe501bc0ba4
That's happening because the 32-bit VDSO is built with CONFIG_PPC32=y,
due to config-fake32.h, and that causes the feature fixup entries to be
the wrong size.
See the logic in feature-fixup.h:
#if defined(CONFIG_PPC64) && !defined(__powerpc64__)
/* 64 bits kernel, 32 bits code (ie. vdso32) */
#define FTR_ENTRY_LONG .8byte
#define FTR_ENTRY_OFFSET .long 0xffffffff; .long
#elif defined(CONFIG_PPC64)
#define FTR_ENTRY_LONG .8byte
#define FTR_ENTRY_OFFSET .8byte
#else
#define FTR_ENTRY_LONG .long
#define FTR_ENTRY_OFFSET .long
#endif
We expect the fixup entries to still use 64-bit values, even for the
32-bit VDSO in a 64-bit kernel.
TBH I'm not sure how config-fake32.h can work long term, it's so fragile
to be defining/redefining a handful of CONFIG symbols like that.
The generic VDSO code is fairly careful to only include uapi and vdso
headers, not linux ones. So I think we need to better split our headers
so that we can build the VDSO code with few or no linux headers, and so
avoid the need to define any (or most) CONFIG symbols.
cheers
^ permalink raw reply
* Re: [PATCH] net: ucc_geth: Drop extraneous parentheses in comparison
From: patchwork-bot+netdevbpf @ 2020-10-24 1:50 UTC (permalink / raw)
To: Michael Ellerman
Cc: netdev, linux-kernel, leoyang.li, linuxppc-dev, kuba, davem
In-Reply-To: <20201023033236.3296988-1-mpe@ellerman.id.au>
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Fri, 23 Oct 2020 14:32:36 +1100 you wrote:
> Clang warns about the extra parentheses in this comparison:
>
> drivers/net/ethernet/freescale/ucc_geth.c:1361:28:
> warning: equality comparison with extraneous parentheses
> if ((ugeth->phy_interface == PHY_INTERFACE_MODE_SGMII))
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> [...]
Here is the summary with links:
- net: ucc_geth: Drop extraneous parentheses in comparison
https://git.kernel.org/netdev/net/c/dab234227cbd
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH] net: ucc_geth: Drop extraneous parentheses in comparison
From: Jakub Kicinski @ 2020-10-24 1:45 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, leoyang.li, davem, linux-kernel, netdev
In-Reply-To: <20201023033236.3296988-1-mpe@ellerman.id.au>
On Fri, 23 Oct 2020 14:32:36 +1100 Michael Ellerman wrote:
> Clang warns about the extra parentheses in this comparison:
>
> drivers/net/ethernet/freescale/ucc_geth.c:1361:28:
> warning: equality comparison with extraneous parentheses
> if ((ugeth->phy_interface == PHY_INTERFACE_MODE_SGMII))
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> It seems clear the intent here is to do a comparison not an
> assignment, so drop the extra parentheses to avoid any confusion.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Applied, thanks!
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-23 21:28 UTC (permalink / raw)
To: 'Segher Boessenkool', Al Viro
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, 'Greg KH', Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201023182713.GG2672@gate.crashing.org>
From: Segher Boessenkool
> Sent: 23 October 2020 19:27
>
> On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
> > On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
> >
> > > Now, I am not a compiler expert, but as I already cited, at least on
> > > x86-64 clang expects that the high bits were cleared by the caller - in
> > > contrast to gcc. I suspect it's the same on arm64, but again, I am no
> > > compiler expert.
> > >
> > > If what I said and cites for x86-64 is correct, if the function expects
> > > an "unsigned int", it will happily use 64bit operations without further
> > > checks where valid when assuming high bits are zero. That's why even
> > > converting everything to "unsigned int" as proposed by me won't work on
> > > clang - it assumes high bits are zero (as indicated by Nick).
> > >
> > > As I am neither a compiler experts (did I mention that already? ;) ) nor
> > > an arm64 experts, I can't tell if this is a compiler BUG or not.
> >
> > On arm64 when callee expects a 32bit argument, the caller is *not* responsible
> > for clearing the upper half of 64bit register used to pass the value - it only
> > needs to store the actual value into the lower half. The callee must consider
> > the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
> > https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
> > ); AFAICS, the relevant bit is
> > "Unlike in the 32-bit AAPCS, named integral values must be narrowed by
> > the callee rather than the caller."
>
> Or the formal rule:
>
> C.9 If the argument is an Integral or Pointer Type, the size of the
> argument is less than or equal to 8 bytes and the NGRN is less
> than 8, the argument is copied to the least significant bits in
> x[NGRN]. The NGRN is incremented by one. The argument has now
> been allocated.
So, in essence, if the value is in a 64bit register the calling
code is independent of the actual type of the formal parameter.
Clearly a value might need explicit widening.
I've found a copy of the 64 bit arm instruction set.
Unfortunately it is alpha sorted and repetitive so shows none
of the symmetry and makes things difficult to find.
But, contrary to what someone suggested most register writes
(eg from arithmetic) seem to zero/extend the high bits.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
From: Miguel Ojeda @ 2020-10-23 20:03 UTC (permalink / raw)
To: Joe Perches
Cc: clang-built-linux, Nick Desaulniers, Linus Torvalds, linux-kernel,
linuxppc-dev
In-Reply-To: <64b49cd3680f45808dad286b408e7b196c31ec79.camel@perches.com>
On Fri, Oct 23, 2020 at 10:03 AM Joe Perches <joe@perches.com> wrote:
>
> Thanks Miguel, but IMO it doesn't need time in next.
You're welcome! It never hurts to keep things for a bit there.
> Applying it just before an rc1 minimizes conflicts.
There shouldn't be many conflicts after -rc1. The amount of changes is
reasonable too, so no need to apply the script directly. In any case,
if you prefer that Linus picks it up himself right away for this -rc1,
it looks good to me (with the caveat that it isn't tested):
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Cheers,
Miguel
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Segher Boessenkool @ 2020-10-23 18:27 UTC (permalink / raw)
To: Al Viro
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, 'Greg KH', Nick Desaulniers,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201023175857.GA3576660@ZenIV.linux.org.uk>
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote:
> On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
>
> > Now, I am not a compiler expert, but as I already cited, at least on
> > x86-64 clang expects that the high bits were cleared by the caller - in
> > contrast to gcc. I suspect it's the same on arm64, but again, I am no
> > compiler expert.
> >
> > If what I said and cites for x86-64 is correct, if the function expects
> > an "unsigned int", it will happily use 64bit operations without further
> > checks where valid when assuming high bits are zero. That's why even
> > converting everything to "unsigned int" as proposed by me won't work on
> > clang - it assumes high bits are zero (as indicated by Nick).
> >
> > As I am neither a compiler experts (did I mention that already? ;) ) nor
> > an arm64 experts, I can't tell if this is a compiler BUG or not.
>
> On arm64 when callee expects a 32bit argument, the caller is *not* responsible
> for clearing the upper half of 64bit register used to pass the value - it only
> needs to store the actual value into the lower half. The callee must consider
> the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
> https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
> ); AFAICS, the relevant bit is
> "Unlike in the 32-bit AAPCS, named integral values must be narrowed by
> the callee rather than the caller."
Or the formal rule:
C.9 If the argument is an Integral or Pointer Type, the size of the
argument is less than or equal to 8 bytes and the NGRN is less
than 8, the argument is copied to the least significant bits in
x[NGRN]. The NGRN is incremented by one. The argument has now
been allocated.
Segher
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Al Viro @ 2020-10-23 17:58 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, 'Greg KH',
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <935f7168-c2f5-dd14-7124-412b284693a2@redhat.com>
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote:
> Now, I am not a compiler expert, but as I already cited, at least on
> x86-64 clang expects that the high bits were cleared by the caller - in
> contrast to gcc. I suspect it's the same on arm64, but again, I am no
> compiler expert.
>
> If what I said and cites for x86-64 is correct, if the function expects
> an "unsigned int", it will happily use 64bit operations without further
> checks where valid when assuming high bits are zero. That's why even
> converting everything to "unsigned int" as proposed by me won't work on
> clang - it assumes high bits are zero (as indicated by Nick).
>
> As I am neither a compiler experts (did I mention that already? ;) ) nor
> an arm64 experts, I can't tell if this is a compiler BUG or not.
On arm64 when callee expects a 32bit argument, the caller is *not* responsible
for clearing the upper half of 64bit register used to pass the value - it only
needs to store the actual value into the lower half. The callee must consider
the contents of the upper half of that register as undefined. See AAPCS64 (e.g.
https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules
); AFAICS, the relevant bit is
"Unlike in the 32-bit AAPCS, named integral values must be narrowed by
the callee rather than the caller."
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-23 16:33 UTC (permalink / raw)
To: 'Greg KH', David Laight
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201023144718.GA2525489@kroah.com>
On 23.10.20 16:47, 'Greg KH' wrote:
> On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
>> From: David Hildenbrand
>>> Sent: 23 October 2020 15:33
>> ...
>>> I just checked against upstream code generated by clang 10 and it
>>> properly discards the upper 32bit via a mov w23 w2.
>>>
>>> So at least clang 10 indeed properly assumes we could have garbage and
>>> masks it off.
>>>
>>> Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
>>> behaves differently.
>>
>> We'll need the disassembly from a failing kernel image.
>> It isn't that big to hand annotate.
>
> I've worked around the merge at the moment in the android tree, but it
> is still quite reproducable, and will try to get a .o file to
> disassemble on Monday or so...
I just compiled pre and post fb041b598997d63c0f7d7305dfae70046bf66fe1 with
clang version 11.0.0 (Fedora 11.0.0-0.2.rc1.fc33)
for aarch64 with defconfig and extracted import_iovec and
rw_copy_check_uvector (skipping the compat things)
Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec
-> https://pastebin.com/LtnYMLJt
Post fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec
-> https://pastebin.com/BWPmXrAf
Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector
-> https://pastebin.com/4nSBYRbf
Post fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector
-> https://pastebin.com/hPtEgaEW
I'm only able to spot minor differences ... less gets inlined than I
would have expected. But there are some smaller differences.
Maybe someone wants to have a look before we have object files as used
by Greg ...
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: 'Greg KH' @ 2020-10-23 14:47 UTC (permalink / raw)
To: David Laight
Cc: linux-aio@kvack.org, 'David Hildenbrand',
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, Arnd Bergmann,
linux-block@vger.kernel.org, Al Viro, io-uring@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Jens Axboe,
linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <35d0ec90ef4f4a35a75b9df7d791f719@AcuMS.aculab.com>
On Fri, Oct 23, 2020 at 02:39:24PM +0000, David Laight wrote:
> From: David Hildenbrand
> > Sent: 23 October 2020 15:33
> ...
> > I just checked against upstream code generated by clang 10 and it
> > properly discards the upper 32bit via a mov w23 w2.
> >
> > So at least clang 10 indeed properly assumes we could have garbage and
> > masks it off.
> >
> > Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
> > behaves differently.
>
> We'll need the disassembly from a failing kernel image.
> It isn't that big to hand annotate.
I've worked around the merge at the moment in the android tree, but it
is still quite reproducable, and will try to get a .o file to
disassemble on Monday or so...
thanks,
greg k-h
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-23 14:39 UTC (permalink / raw)
To: 'David Hildenbrand', 'Greg KH'
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <999e2926-9a75-72fd-007a-1de0af341292@redhat.com>
From: David Hildenbrand
> Sent: 23 October 2020 15:33
...
> I just checked against upstream code generated by clang 10 and it
> properly discards the upper 32bit via a mov w23 w2.
>
> So at least clang 10 indeed properly assumes we could have garbage and
> masks it off.
>
> Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
> behaves differently.
We'll need the disassembly from a failing kernel image.
It isn't that big to hand annotate.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-23 14:33 UTC (permalink / raw)
To: David Laight, 'Greg KH'
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <935f7168-c2f5-dd14-7124-412b284693a2@redhat.com>
On 23.10.20 15:09, David Hildenbrand wrote:
> On 23.10.20 14:46, David Laight wrote:
>> From: Greg KH <gregkh@linuxfoundation.org>
>>> Sent: 22 October 2020 14:51
>>
>> I've rammed the code into godbolt.
>>
>> https://godbolt.org/z/9v5PPW
>>
>> Definitely a clang bug.
>>
>> Search for [wx]24 in the clang output.
>> nr_segs comes in as w2 and the initial bound checks are done on w2.
>> w24 is loaded from w2 - I don't believe this changes the high bits.
>> There are no references to w24, just x24.
>> So the kmalloc_array() is passed 'huge' and will fail.
>> The iov_iter_init also gets the 64bit value.
>>
>> Note that the gcc code has a sign-extend copy of w2.
>
> Do we have a result from using "unsigned long" in the base function and
> explicitly masking of the high bits? That should definitely work.
>
> Now, I am not a compiler expert, but as I already cited, at least on
> x86-64 clang expects that the high bits were cleared by the caller - in
> contrast to gcc. I suspect it's the same on arm64, but again, I am no
> compiler expert.
>
> If what I said and cites for x86-64 is correct, if the function expects
> an "unsigned int", it will happily use 64bit operations without further
> checks where valid when assuming high bits are zero. That's why even
> converting everything to "unsigned int" as proposed by me won't work on
> clang - it assumes high bits are zero (as indicated by Nick).
>
> As I am neither a compiler experts (did I mention that already? ;) ) nor
> an arm64 experts, I can't tell if this is a compiler BUG or not.
>
I just checked against upstream code generated by clang 10 and it
properly discards the upper 32bit via a mov w23 w2.
So at least clang 10 indeed properly assumes we could have garbage and
masks it off.
Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11
behaves differently.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
From: Dmitry Safonov @ 2020-10-23 13:29 UTC (permalink / raw)
To: Christophe Leroy, Will Deacon
Cc: nathanl, linux-arch, Arnd Bergmann, open list, Paul Mackerras,
Andy Lutomirski, Thomas Gleixner, Vincenzo Frascino, linuxppc-dev
In-Reply-To: <284cacf4-9811-4b67-385c-2783a7cd9b31@csgroup.eu>
Hi Christophe, Will,
On 10/23/20 12:57 PM, Christophe Leroy wrote:
>
>
> Le 23/10/2020 à 13:25, Will Deacon a écrit :
>> On Fri, Oct 23, 2020 at 01:22:04PM +0200, Christophe Leroy wrote:
>>> Hi Dmitry,
[..]
>>> I haven't seen the patches, did you sent them out finally ?
I was working on .close() hook, but while cooking it, I thought it may
be better to make tracking of user landing generic. Note that the vdso
base address is mostly needed by kernel as an address to land in
userspace after processing a signal.
I have some raw patches that add
+#ifdef CONFIG_ARCH_HAS_USER_LANDING
+ struct vm_area_struct *user_landing;
+#endif
inside mm_struct and I plan to finish them after rc1 gets released.
While working on that, I noticed that arm32 and some other architectures
track vdso position in mm.context with the only reason to add
AT_SYSINFO_EHDR in the elf header that's being loaded. That's quite
overkill to have a pointer in mm.context that rather can be a local
variable in elf binfmt loader. Also, I found some issues with mremap
code. The patches series mentioned are at the base of the branch with
generic user landing. I have sent only those patches not the full branch
as I remember there was a policy that during merge window one should
send only fixes, rather than refactoring/new code.
>> I think it's this series:
>>
>> https://lore.kernel.org/r/20201013013416.390574-1-dima@arista.com
>>
>> but they look really invasive to me, so I may cook a small hack for arm64
>> in the meantine / for stable.
I don't mind small hacks, but I'm concerned that the suggested fix which
sets `mm->context.vdso_base = 0` on munmap() may have it's issue: that
won't work if a user for whatever-broken-reason will mremap() vdso on 0
address. As the fix supposes to fix an issue that hasn't fired for
anyone yet, it probably shouldn't introduce another. That's why I've
used vm_area_struct to track vdso position in the patches set.
Probably, temporary, you could use something like:
#define BAD_VDSO_ADDRESS (-1)UL
Or non-page-aligned address.
But the signal code that checks if it can land on vdso/sigpage should be
also aligned with the new definition.
> Not sure we are talking about the same thing.
>
> I can't see any new .close function added to vm_special_mapping in order
> to replace arch_unmap() hook.
Thanks,
Dmitry
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-23 13:28 UTC (permalink / raw)
To: 'Arnd Bergmann'
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAK8P3a1n+b8hOMhNQSDzgic03dyXbmpccfTJ3C1bGKvzsgMXbg@mail.gmail.com>
From: Arnd Bergmann
> Sent: 23 October 2020 14:23
>
> On Fri, Oct 23, 2020 at 2:46 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: 22 October 2020 14:51
> >
> > I've rammed the code into godbolt.
> >
> > https://godbolt.org/z/9v5PPW
> >
> > Definitely a clang bug.
> >
> > Search for [wx]24 in the clang output.
> > nr_segs comes in as w2 and the initial bound checks are done on w2.
> > w24 is loaded from w2 - I don't believe this changes the high bits.
>
> You believe wrong, "mov w24, w2" is a zero-extending operation.
Ah ok, but gcc uses utxw for the same task.
I guess they could be the same opcode.
Last time I wrote ARM thumb didn't really exist - never mind 64bit
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: C vdso
From: Michael Ellerman @ 2020-10-23 13:24 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <be21c7c8-6828-b757-064d-20f74e5c1a31@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 24/09/2020 à 15:17, Christophe Leroy a écrit :
>> Le 17/09/2020 à 14:33, Michael Ellerman a écrit :
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>
>>>> What is the status with the generic C vdso merge ?
>>>> In some mail, you mentionned having difficulties getting it working on
>>>> ppc64, any progress ? What's the problem ? Can I help ?
>>>
>>> Yeah sorry I was hoping to get time to work on it but haven't been able
>>> to.
>>>
>>> It's causing crashes on ppc64 ie. big endian.
...
>>
>> Can you tell what defconfig you are using ? I have been able to setup a full glibc PPC64 cross
>> compilation chain and been able to test it under QEMU with success, using Nathan's vdsotest tool.
>
> What config are you using ?
ppc64_defconfig + guest.config
Or pseries_defconfig.
I'm using Ubuntu GCC 9.3.0 mostly, but it happens with other toolchains too.
At a minimum we're seeing relocations in the output, which is a problem:
$ readelf -r build\~/arch/powerpc/kernel/vdso64/vdso64.so
Relocation section '.rela.dyn' at offset 0x12a8 contains 8 entries:
Offset Info Type Sym. Value Sym. Name + Addend
000000001368 000000000016 R_PPC64_RELATIVE 7c0
000000001370 000000000016 R_PPC64_RELATIVE 9300
000000001380 000000000016 R_PPC64_RELATIVE 970
000000001388 000000000016 R_PPC64_RELATIVE 9300
000000001398 000000000016 R_PPC64_RELATIVE a90
0000000013a0 000000000016 R_PPC64_RELATIVE 9300
0000000013b0 000000000016 R_PPC64_RELATIVE b20
0000000013b8 000000000016 R_PPC64_RELATIVE 9300
cheers
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann @ 2020-10-23 13:23 UTC (permalink / raw)
To: David Laight
Cc: linux-aio@kvack.org, David Hildenbrand,
linux-mips@vger.kernel.org, David Howells, linux-mm@kvack.org,
keyrings@vger.kernel.org, sparclinux@vger.kernel.org,
Christoph Hellwig, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org,
kernel-team@android.com, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <134f162d711d466ebbd88906fae35b33@AcuMS.aculab.com>
On Fri, Oct 23, 2020 at 2:46 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: 22 October 2020 14:51
>
> I've rammed the code into godbolt.
>
> https://godbolt.org/z/9v5PPW
>
> Definitely a clang bug.
>
> Search for [wx]24 in the clang output.
> nr_segs comes in as w2 and the initial bound checks are done on w2.
> w24 is loaded from w2 - I don't believe this changes the high bits.
You believe wrong, "mov w24, w2" is a zero-extending operation.
Arnd
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-23 13:12 UTC (permalink / raw)
To: Al Viro, Nick Desaulniers
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, Greg KH,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, David Laight,
netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Andrew Morton, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022192458.GV3576660@ZenIV.linux.org.uk>
On 22.10.20 21:24, Al Viro wrote:
> On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
>
>> Passing an `unsigned long` as an `unsigned int` does no such
>> narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
>> calls, no masking instructions).
>> So if rw_copy_check_uvector() is inlined into import_iovec() (looking
>> at the mainline@1028ae406999), then children calls of
>> `rw_copy_check_uvector()` will be interpreting the `nr_segs` register
>> unmodified, ie. garbage in the upper 32b.
>
> FWIW,
>
> void f(unsinged long v)
> {
> if (v != 1)
> printf("failed\n");
> }
>
> void g(unsigned int v)
> {
> f(v);
> }
>
> void h(unsigned long v)
> {
> g(v);
> }
>
> main()
> {
> h(0x100000001);
> }
>
> must not produce any output on a host with 32bit int and 64bit long, regardless of
> the inlining, having functions live in different compilation units, etc.
>
> Depending upon the calling conventions, compiler might do truncation in caller or
> in a callee, but it must be done _somewhere_.
The interesting case is having g() in a separate compilation unit and
force-calling g() with 0x100000001 via inline ASM. So forcing garbage
into high bits.
I'll paly with it.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand @ 2020-10-23 13:09 UTC (permalink / raw)
To: David Laight, 'Greg KH'
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <134f162d711d466ebbd88906fae35b33@AcuMS.aculab.com>
On 23.10.20 14:46, David Laight wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
>> Sent: 22 October 2020 14:51
>
> I've rammed the code into godbolt.
>
> https://godbolt.org/z/9v5PPW
>
> Definitely a clang bug.
>
> Search for [wx]24 in the clang output.
> nr_segs comes in as w2 and the initial bound checks are done on w2.
> w24 is loaded from w2 - I don't believe this changes the high bits.
> There are no references to w24, just x24.
> So the kmalloc_array() is passed 'huge' and will fail.
> The iov_iter_init also gets the 64bit value.
>
> Note that the gcc code has a sign-extend copy of w2.
Do we have a result from using "unsigned long" in the base function and
explicitly masking of the high bits? That should definitely work.
Now, I am not a compiler expert, but as I already cited, at least on
x86-64 clang expects that the high bits were cleared by the caller - in
contrast to gcc. I suspect it's the same on arm64, but again, I am no
compiler expert.
If what I said and cites for x86-64 is correct, if the function expects
an "unsigned int", it will happily use 64bit operations without further
checks where valid when assuming high bits are zero. That's why even
converting everything to "unsigned int" as proposed by me won't work on
clang - it assumes high bits are zero (as indicated by Nick).
As I am neither a compiler experts (did I mention that already? ;) ) nor
an arm64 experts, I can't tell if this is a compiler BUG or not.
Main issue seems to be garbage in high bits as originally suggested by me.
--
Thanks,
David / dhildenb
^ permalink raw reply
* RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Laight @ 2020-10-23 12:46 UTC (permalink / raw)
To: 'Greg KH', David Hildenbrand
Cc: linux-aio@kvack.org, linux-mips@vger.kernel.org, David Howells,
linux-mm@kvack.org, keyrings@vger.kernel.org,
sparclinux@vger.kernel.org, Christoph Hellwig,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Arnd Bergmann, linux-block@vger.kernel.org, Al Viro,
io-uring@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jens Axboe, linux-parisc@vger.kernel.org, netdev@vger.kernel.org,
Nick Desaulniers, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20201022135036.GA1787470@kroah.com>
From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 22 October 2020 14:51
I've rammed the code into godbolt.
https://godbolt.org/z/9v5PPW
Definitely a clang bug.
Search for [wx]24 in the clang output.
nr_segs comes in as w2 and the initial bound checks are done on w2.
w24 is loaded from w2 - I don't believe this changes the high bits.
There are no references to w24, just x24.
So the kmalloc_array() is passed 'huge' and will fail.
The iov_iter_init also gets the 64bit value.
Note that the gcc code has a sign-extend copy of w2.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] x86/mpx: fix recursive munmap() corruption
From: Christophe Leroy @ 2020-10-23 12:28 UTC (permalink / raw)
To: Laurent Dufour, Michael Ellerman
Cc: mhocko, rguenther, linux-mm, Dave Hansen, x86, stable, LKML,
Dave Hansen, Thomas Gleixner, luto, linuxppc-dev, Andrew Morton,
vbabka
In-Reply-To: <9c2b2826-4083-fc9c-5a4d-c101858dd560@linux.vnet.ibm.com>
Hi Laurent
Le 07/05/2019 à 18:35, Laurent Dufour a écrit :
> Le 01/05/2019 à 12:32, Michael Ellerman a écrit :
>> Laurent Dufour <ldufour@linux.vnet.ibm.com> writes:
>>> Le 23/04/2019 à 18:04, Dave Hansen a écrit :
>>>> On 4/23/19 4:16 AM, Laurent Dufour wrote:
>> ...
>>>>> There are 2 assumptions here:
>>>>> 1. 'start' and 'end' are page aligned (this is guaranteed by __do_munmap().
>>>>> 2. the VDSO is 1 page (this is guaranteed by the union vdso_data_store on powerpc)
>>>>
>>>> Are you sure about #2? The 'vdso64_pages' variable seems rather
>>>> unnecessary if the VDSO is only 1 page. ;)
>>>
>>> Hum, not so sure now ;)
>>> I got confused, only the header is one page.
>>> The test is working as a best effort, and don't cover the case where
>>> only few pages inside the VDSO are unmmapped (start >
>>> mm->context.vdso_base). This is not what CRIU is doing and so this was
>>> enough for CRIU support.
>>>
>>> Michael, do you think there is a need to manage all the possibility
>>> here, since the only user is CRIU and unmapping the VDSO is not a so
>>> good idea for other processes ?
>>
>> Couldn't we implement the semantic that if any part of the VDSO is
>> unmapped then vdso_base is set to zero? That should be fairly easy, eg:
>>
>> if (start < vdso_end && end >= mm->context.vdso_base)
>> mm->context.vdso_base = 0;
>>
>>
>> We might need to add vdso_end to the mm->context, but that should be OK.
>>
>> That seems like it would work for CRIU and make sense in general?
>
> Sorry for the late answer, yes this would make more sense.
>
> Here is a patch doing that.
>
In your patch, the test seems overkill:
+ if ((start <= vdso_base && vdso_end <= end) || /* 1 */
+ (vdso_base <= start && start < vdso_end) || /* 3,4 */
+ (vdso_base < end && end <= vdso_end)) /* 2,3 */
+ mm->context.vdso_base = mm->context.vdso_end = 0;
What about
if (start < vdso_end && vdso_start < end)
mm->context.vdso_base = mm->context.vdso_end = 0;
This should cover all cases, or am I missing something ?
And do we really need to store vdso_end in the context ?
I think it should be possible to re-calculate it: the size of the VDSO should be (&vdso32_end -
&vdso32_start) + PAGE_SIZE for 32 bits VDSO, and (&vdso64_end - &vdso64_start) + PAGE_SIZE for the
64 bits VDSO.
Christophe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox