2021-07-05 15:18:31

by Anatoly Pugachev

[permalink] [raw]
Subject: [sparc64] locking/atomic, kernel OOPS on running stress-ng

Hello!

latest sparc64 git kernel produces the following OOPS on running stress-ng as :

$ stress-ng -v --mmap 1 -t 30s

kernel OOPS (console logs):

[ 27.276719] Unable to handle kernel NULL pointer dereference
[ 27.276782] tsk->{mm,active_mm}->context = 00000000000003cb
[ 27.276818] tsk->{mm,active_mm}->pgd = fff800003a2a0000
[ 27.276853] \|/ ____ \|/
[ 27.276853] "@'/ .. \`@"
[ 27.276853] /_| \__/ |_\
[ 27.276853] \__U_/
[ 27.276927] stress-ng(928): Oops [#1]
[ 27.276961] CPU: 0 PID: 928 Comm: stress-ng Tainted: G E
5.13.0-rc1-00111-g8e6a4b3afe64 #257
[ 27.277021] TSTATE: 0000009911001603 TPC: 000000000044f3c4 TNPC:
000000000044f3c8 Y: 00000000 Tainted: G E
[ 27.277084] TPC: <pmdp_invalidate+0x24/0xc0>
[ 27.277129] g0: 0000000000000000 g1: 3d0000004d800653 g2:
0000000000000000 g3: 0006000000000000
[ 27.277180] g4: fff80000370a9c00 g5: fff8000229666000 g6:
fff8000047404000 g7: 0000000000090014
[ 27.277231] o0: 0000000000000001 o1: 0000000000000000 o2:
fff80000370aa4b8 o3: 0000000000000000
[ 27.277283] o4: 2ec9091000000000 o5: 0000000000f86c00 sp:
fff8000047406ec1 ret_pc: 00000000004d197c
[ 27.277337] RPC: <lock_acquire+0x1bc/0x200>
[ 27.277377] l0: 000000000119b1c0 l1: 0000000000000000 l2:
0000000001205e48 l3: 81123ddb8627a322
[ 27.277432] l4: c269484aab0b613a l5: 000000000148f800 l6:
ffffffff7c8086d1 l7: 0000000001204788
[ 27.277487] i0: fff8000043c9ce40 i1: fff8000104800000 i2:
fff80000424d9048 i3: bd0000004d800653
[ 27.277540] i4: 3d0000004d800653 i5: bd0000004d800653 i6:
fff8000047406f71 i7: 000000000069e4ac
[ 27.277593] I7: <__split_huge_pmd_locked+0x1ec/0x5e0>
[ 27.277639] Call Trace:
[ 27.277662] [<000000000069e4ac>] __split_huge_pmd_locked+0x1ec/0x5e0
[ 27.277708] [<000000000069ff48>] __split_huge_pmd+0x288/0x2e0
[ 27.277751] [<00000000006a0638>] split_huge_pmd_address+0x78/0xa0
[ 27.277797] [<00000000006a0780>] vma_adjust_trans_huge+0x120/0x160
[ 27.277843] [<000000000065ac70>] __vma_adjust+0x1d0/0xb00
[ 27.277887] [<000000000065bfb0>] __split_vma+0xf0/0x180
[ 27.277927] [<0000000000675704>] madvise_behavior+0x224/0x2a0
[ 27.277972] [<0000000000677418>] do_madvise+0x478/0x600
[ 27.278011] [<0000000000677778>] sys_madvise+0x18/0x40
[ 27.278050] [<0000000000406274>] linux_sparc_syscall+0x34/0x44
[ 27.278100] Disabling lock debugging due to kernel taint
[ 27.278112] Caller[000000000069e4ac]: __split_huge_pmd_locked+0x1ec/0x5e0
[ 27.278130] Caller[000000000069ff48]: __split_huge_pmd+0x288/0x2e0
[ 27.278146] Caller[00000000006a0638]: split_huge_pmd_address+0x78/0xa0
[ 27.278161] Caller[00000000006a0780]: vma_adjust_trans_huge+0x120/0x160
[ 27.278177] Caller[000000000065ac70]: __vma_adjust+0x1d0/0xb00
[ 27.278191] Caller[000000000065bfb0]: __split_vma+0xf0/0x180
[ 27.278208] Caller[0000000000675704]: madvise_behavior+0x224/0x2a0
[ 27.278222] Caller[0000000000677418]: do_madvise+0x478/0x600
[ 27.278237] Caller[0000000000677778]: sys_madvise+0x18/0x40
[ 27.278253] Caller[0000000000406274]: linux_sparc_syscall+0x34/0x44
[ 27.278267] Caller[00000100000bd02c]: 0x100000bd02c
[ 27.278281] Instruction DUMP:
[ 27.278285] ba10001b
[ 27.278295] 8210001c
[ 27.278304] 84102000
[ 27.278313] <c3f0901d>
[ 27.278321] 80a0401d
[ 27.278330] 22600004
[ 27.278338] d05e2040
[ 27.278346] 106ffffa
[ 27.278354] fa5e8000
[ 27.278363]

tried to bisect this OOPS, but was unable to find the latest commit
id, without cherry-pick:

linux-2.6$ git describe
v5.13-rc1-111-gb9b12978a8e9

linux-2.6$ make
CC kernel/bounds.s
In file included from ./include/linux/atomic.h:87,
from ./include/asm-generic/bitops/lock.h:5,
from ./arch/sparc/include/asm/bitops_64.h:52,
from ./arch/sparc/include/asm/bitops.h:5,
from ./include/linux/bitops.h:32,
from ./include/linux/kernel.h:12,
from ./include/asm-generic/bug.h:20,
from ./arch/sparc/include/asm/bug.h:25,
from ./include/linux/bug.h:5,
from ./include/linux/page-flags.h:10,
from kernel/bounds.c:10:
./include/asm-generic/atomic-long.h: In function ‘atomic_long_add_return’:
./include/asm-generic/atomic-long.h:59:9: error: implicit declaration
of function ‘atomic64_add_return’; did you mean ‘atomic64_dec_return’?
[-Werror=implicit-function-declaration]
59 | return atomic64_add_return(i, v);
| ^~~~~~~~~~~~~~~~~~~
| atomic64_dec_return
./include/asm-generic/atomic-long.h: In function ‘atomic_long_fetch_add’:
./include/asm-generic/atomic-long.h:83:9: error: implicit declaration
of function ‘atomic64_fetch_add’; did you mean ‘atomic64_fetch_dec’?
[-Werror=implicit-function-declaration]
83 | return atomic64_fetch_add(i, v);
| ^~~~~~~~~~~~~~~~~~
| atomic64_fetch_dec


$ git bisect log
# bad: [3dbdb38e286903ec220aaf1fb29a8d94297da246] Merge branch
'for-5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
# good: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect start '3dbdb38e2869' 'v5.13' 'mm/'
# bad: [a6eaf3850cb171c328a8b0db6d3c79286a1eba9d] Merge tag
'sched-urgent-2021-06-30' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad a6eaf3850cb171c328a8b0db6d3c79286a1eba9d
# bad: [31e798fd6f0ff0acdc49c1a358b581730936a09a] Merge tag
'media/v5.14-1' of
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect bad 31e798fd6f0ff0acdc49c1a358b581730936a09a
# bad: [9840cfcb97fc8b6aa7b36cec3cc3fd763f14052e] Merge tag
'arm64-upstream' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
git bisect bad 9840cfcb97fc8b6aa7b36cec3cc3fd763f14052e
# good: [b89c07dea16137696d0f2d479ef665ef7c1022ab] Merge tags
'objtool-urgent-2021-06-28' and 'objtool-core-2021-06-28' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good b89c07dea16137696d0f2d479ef665ef7c1022ab
# bad: [8e4d7a78f08a788a839bd88a2710ba7a71a86e24] Merge tag
'x86-cleanups-2021-06-28' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 8e4d7a78f08a788a839bd88a2710ba7a71a86e24
# good: [adf3c31e18b765ea24eba7b0c1efc076b8ee3d55] sched/doc: Update
the CPU capacity asymmetry bits
git bisect good adf3c31e18b765ea24eba7b0c1efc076b8ee3d55
# bad: [a15286c63d113d4296c58867994cd266a28f5d6d] Merge tag
'locking-core-2021-06-28' of
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad a15286c63d113d4296c58867994cd266a28f5d6d
# good: [c7b5fd6faa1dc6cdc721a978d9d122cd31bbd7b1] locking/atomic:
mips: move to ARCH_ATOMIC
git bisect good c7b5fd6faa1dc6cdc721a978d9d122cd31bbd7b1
# bad: [b8e00abe7d9fe21dd13609e2e3a707e38902b105] locking/lockdep:
Reduce LOCKDEP dependency list
git bisect bad b8e00abe7d9fe21dd13609e2e3a707e38902b105
# good: [9efbb355831014ca004d241db8ede182c019b9bf] locking/atomic:
riscv: move to ARCH_ATOMIC
git bisect good 9efbb355831014ca004d241db8ede182c019b9bf

$ git desc b8e00abe7d9fe21dd13609e2e3a707e38902b105
v5.13-rc1-115-gb8e00abe7d9f
$ git desc 9efbb355831014ca004d241db8ede182c019b9bf
v5.13-rc1-108-g9efbb3558310

$ git log --oneline 9efbb3558310..b8e00abe7d9f
b8e00abe7d9f (refs/bisect/bad) locking/lockdep: Reduce LOCKDEP dependency list
fab6216fafdd locking/lockdep,doc: Improve readability of the block matrix
bccf1ec369ac locking/atomics: atomic-instrumented: simplify ifdeffery
3c1885187bc1 locking/atomic: delete !ARCH_ATOMIC remnants
b9b12978a8e9 (HEAD) locking/atomic: xtensa: move to ARCH_ATOMIC
ff5b4f1ed580 locking/atomic: sparc: move to ARCH_ATOMIC
8c6417551309 locking/atomic: sh: move to ARCH_ATOMIC

so ff5b4f1ed580 does not compile without cherry-picking
bccf1ec369ac... and booting to this kernel (ff5b4f1ed580 +
bccf1ec369ac) introduces stress-ng OOPS (shown above)

Can someone please look at this commit ids?
Thanks!

PS: kernel config
https://github.com/mator/sparc64-dmesg/blob/master/config-5.13.0.gz


2021-07-05 19:58:27

by Mark Rutland

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Mon, Jul 05, 2021 at 06:16:49PM +0300, Anatoly Pugachev wrote:
> Hello!

Hi Anatoly,

> latest sparc64 git kernel produces the following OOPS on running stress-ng as :
>
> $ stress-ng -v --mmap 1 -t 30s
>
> kernel OOPS (console logs):
>
> [ 27.276719] Unable to handle kernel NULL pointer dereference
> [ 27.276782] tsk->{mm,active_mm}->context = 00000000000003cb
> [ 27.276818] tsk->{mm,active_mm}->pgd = fff800003a2a0000
> [ 27.276853] \|/ ____ \|/
> [ 27.276853] "@'/ .. \`@"
> [ 27.276853] /_| \__/ |_\
> [ 27.276853] \__U_/
> [ 27.276927] stress-ng(928): Oops [#1]

I can reproduce this under QEMU; following your bisection (and working
around the missing ifdeferry that breaks bisection), I can confirm that
the first broken commit is:

ff5b4f1ed580 ("locking/atomic: sparc: move to ARCH_ATOMIC")

Sorry about this.

> Can someone please look at this commit ids?

From digging into this, I can't spot an obvious bug in the commit above.

It looks like this happens when some of the xchg/cmpxchg variants are
wrapped by <asm-generic/atomic-instrumented.h>, but I can't immediately
explain why. This might be a latent bug that's being tickled by the
structure of the wrappers, or some subtlety with the typecasting that
happens in the wrappers.

Starting with:

ff5b4f1ed580 ("locking/atomic: sparc: move to ARCH_ATOMIC")

... and atop that, cherry-picking:

bccf1ec369ac ("locking/atomics: atomic-instrumented: simplify ifdeffery")

... the below hack seems to make the stress-ng run pass without issue,
even after running for multiple minutes (when it would usually fail in a
few seconds).

In case this is a codegen issue, I'm using the kernel.org GCC 10.3.0
cross toolchain.

Thanks,
Mark.

---->8----
From 9a77ebd7005a9d4492686c45207642eeb4d13a8c Mon Sep 17 00:00:00 2001
From: Mark Rutland <[email protected]>
Date: Mon, 5 Jul 2021 20:38:06 +0100
Subject: [PATCH] HACK: disable instrumentation of xchg/cmpxchg

Signed-off-by: Mark Rutland <[email protected]>
---
include/asm-generic/atomic-instrumented.h | 86 ++++++++++++++++++++++++++++++-
scripts/atomic/gen-atomic-instrumented.sh | 10 ++++
2 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
index bc45af52c93b..7d0c38091c82 100644
--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -1177,90 +1177,139 @@ atomic64_dec_if_positive(atomic64_t *v)
return arch_atomic64_dec_if_positive(v);
}

+#if 0
#define xchg(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_xchg(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define xchg arch_xchg
+#endif

+#if 0
#define xchg_acquire(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_xchg_acquire(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define xchg_acquire arch_xchg_acquire
+#endif

+#if 0
#define xchg_release(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_xchg_release(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define xchg_release arch_xchg_release
+#endif

+#if 0
#define xchg_relaxed(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_xchg_relaxed(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define xchg_relaxed arch_xchg_relaxed
+#endif

+#if 0
#define cmpxchg(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg arch_cmpxchg
+#endif

+#if 0
#define cmpxchg_acquire(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_acquire(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_acquire arch_cmpxchg_acquire
+#endif

+#if 0
#define cmpxchg_release(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_release(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_release arch_cmpxchg_release
+#endif

+#if 0
#define cmpxchg_relaxed(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_relaxed(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_relaxed arch_cmpxchg_relaxed
+#endif

+#if 0
#define cmpxchg64(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg64(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg64 arch_cmpxchg64
+#endif

+#if 0
#define cmpxchg64_acquire(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg64_acquire(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg64_acquire arch_cmpxchg64_acquire
+#endif

+#if 0
#define cmpxchg64_release(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg64_release(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg64_release arch_cmpxchg64_release
+#endif

+#if 0
#define cmpxchg64_relaxed(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg64_relaxed(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg64_relaxed arch_cmpxchg64_relaxed
+#endif

+#if 0
#define try_cmpxchg(ptr, oldp, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
@@ -1269,7 +1318,11 @@ atomic64_dec_if_positive(atomic64_t *v)
instrument_atomic_write(__ai_oldp, sizeof(*__ai_oldp)); \
arch_try_cmpxchg(__ai_ptr, __ai_oldp, __VA_ARGS__); \
})
+#else
+#define try_cmpxchg arch_try_cmpxchg
+#endif

+#if 0
#define try_cmpxchg_acquire(ptr, oldp, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
@@ -1278,7 +1331,11 @@ atomic64_dec_if_positive(atomic64_t *v)
instrument_atomic_write(__ai_oldp, sizeof(*__ai_oldp)); \
arch_try_cmpxchg_acquire(__ai_ptr, __ai_oldp, __VA_ARGS__); \
})
+#else
+#define try_cmpxchg_acquire arch_try_cmpxchg_acquire
+#endif

+#if 0
#define try_cmpxchg_release(ptr, oldp, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
@@ -1287,7 +1344,11 @@ atomic64_dec_if_positive(atomic64_t *v)
instrument_atomic_write(__ai_oldp, sizeof(*__ai_oldp)); \
arch_try_cmpxchg_release(__ai_ptr, __ai_oldp, __VA_ARGS__); \
})
+#else
+#define try_cmpxchg_release arch_try_cmpxchg_release
+#endif

+#if 0
#define try_cmpxchg_relaxed(ptr, oldp, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
@@ -1296,42 +1357,65 @@ atomic64_dec_if_positive(atomic64_t *v)
instrument_atomic_write(__ai_oldp, sizeof(*__ai_oldp)); \
arch_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
})
+#else
+#define try_cmpxchg_relaxed arch_try_cmpxchg_relaxed
+#endif

+#if 0
#define cmpxchg_local(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_local(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_local arch_cmpxchg_local
+#endif

+#if 0
#define cmpxchg64_local(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg64_local(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg64_local arch_cmpxchg64_local
+#endif

+#if 0
#define sync_cmpxchg(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_sync_cmpxchg(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define sync_cmpxchg arch_sync_cmpxchg
+#endif

+#if 0
#define cmpxchg_double(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, 2 * sizeof(*__ai_ptr)); \
arch_cmpxchg_double(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_double arch_cmpxchg_double
+#endif


+#if 0
#define cmpxchg_double_local(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, 2 * sizeof(*__ai_ptr)); \
arch_cmpxchg_double_local(__ai_ptr, __VA_ARGS__); \
})
+#else
+#define cmpxchg_double_local arch_cmpxchg_double_local
+#endif

#endif /* _ASM_GENERIC_ATOMIC_INSTRUMENTED_H */
-// 1d7c3a25aca5c7fb031c307be4c3d24c7b48fcd5
+// 2a4279557c0aea18c2784cefd4a26d58e6ee66d0
diff --git a/scripts/atomic/gen-atomic-instrumented.sh b/scripts/atomic/gen-atomic-instrumented.sh
index b0c45aee19d7..bfadca4046fb 100755
--- a/scripts/atomic/gen-atomic-instrumented.sh
+++ b/scripts/atomic/gen-atomic-instrumented.sh
@@ -80,6 +80,7 @@ gen_xchg()
if [ "${xchg%${xchg#try_cmpxchg}}" = "try_cmpxchg" ] ; then

cat <<EOF
+#if 0
#define ${xchg}(ptr, oldp, ...) \\
({ \\
typeof(ptr) __ai_ptr = (ptr); \\
@@ -88,17 +89,26 @@ cat <<EOF
instrument_atomic_write(__ai_oldp, ${mult}sizeof(*__ai_oldp)); \\
arch_${xchg}(__ai_ptr, __ai_oldp, __VA_ARGS__); \\
})
+#else
+#define ${xchg} \
+ arch_${xchg}
+#endif
EOF

else

cat <<EOF
+#if 0
#define ${xchg}(ptr, ...) \\
({ \\
typeof(ptr) __ai_ptr = (ptr); \\
instrument_atomic_write(__ai_ptr, ${mult}sizeof(*__ai_ptr)); \\
arch_${xchg}(__ai_ptr, __VA_ARGS__); \\
})
+#else
+#define ${xchg} \
+ arch_${xchg}
+#endif
EOF

fi
--
2.11.0

2021-07-05 23:26:53

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] HACK: disable instrumentation of xchg/cmpxchg

Hi Mark,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master next-20210701]
[cannot apply to asm-generic/master sparc-next/master v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Mark-Rutland/HACK-disable-instrumentation-of-xchg-cmpxchg/20210706-035817
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git c54b245d011855ea91c5beff07f1db74143ce614
config: xtensa-randconfig-r035-20210705 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/077d46116045ef95775b35f74f003921cb21c955
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mark-Rutland/HACK-disable-instrumentation-of-xchg-cmpxchg/20210706-035817
git checkout 077d46116045ef95775b35f74f003921cb21c955
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=xtensa

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from include/linux/init.h:5,
from lib/atomic64_test.c:10:
lib/atomic64_test.c: In function 'test_atomic':
>> lib/atomic64_test.c:75:9: error: implicit declaration of function 'atomic_arch_xchg'; did you mean 'atomic_long_xchg'? [-Werror=implicit-function-declaration]
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
include/asm-generic/bug.h:183:32: note: in expansion of macro 'if'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~
include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
48 | # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/bug.h:183:36: note: in expansion of macro 'unlikely'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~~~~~~~
lib/atomic64_test.c:75:2: note: in expansion of macro 'BUG_ON'
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
lib/atomic64_test.c:38:2: note: in expansion of macro 'TEST_ARGS'
38 | test(bit, op, ##args); \
| ^~~~
lib/atomic64_test.c:81:2: note: in expansion of macro 'FAMILY_TEST'
81 | FAMILY_TEST(TEST_ARGS, bit, xchg, init, init, new, new); \
| ^~~~~~~~~~~
lib/atomic64_test.c:141:2: note: in expansion of macro 'XCHG_FAMILY_TEST'
141 | XCHG_FAMILY_TEST(, v0, v1);
| ^~~~~~~~~~~~~~~~
>> lib/atomic64_test.c:75:9: error: implicit declaration of function 'atomic_arch_cmpxchg'; did you mean 'atomic_try_cmpxchg'? [-Werror=implicit-function-declaration]
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
include/asm-generic/bug.h:183:32: note: in expansion of macro 'if'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~
include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
48 | # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/bug.h:183:36: note: in expansion of macro 'unlikely'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~~~~~~~
lib/atomic64_test.c:75:2: note: in expansion of macro 'BUG_ON'
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
lib/atomic64_test.c:38:2: note: in expansion of macro 'TEST_ARGS'
38 | test(bit, op, ##args); \
| ^~~~
lib/atomic64_test.c:86:2: note: in expansion of macro 'FAMILY_TEST'
86 | FAMILY_TEST(TEST_ARGS, bit, cmpxchg, \
| ^~~~~~~~~~~
lib/atomic64_test.c:142:2: note: in expansion of macro 'CMPXCHG_FAMILY_TEST'
142 | CMPXCHG_FAMILY_TEST(, v0, v1, onestwos);
| ^~~~~~~~~~~~~~~~~~~
lib/atomic64_test.c: In function 'test_atomic64':
>> lib/atomic64_test.c:75:9: error: implicit declaration of function 'atomic64_arch_xchg'; did you mean 'atomic64_cmpxchg'? [-Werror=implicit-function-declaration]
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
include/asm-generic/bug.h:183:32: note: in expansion of macro 'if'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~
include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
48 | # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/bug.h:183:36: note: in expansion of macro 'unlikely'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~~~~~~~
lib/atomic64_test.c:75:2: note: in expansion of macro 'BUG_ON'
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
lib/atomic64_test.c:38:2: note: in expansion of macro 'TEST_ARGS'
38 | test(bit, op, ##args); \
| ^~~~
lib/atomic64_test.c:81:2: note: in expansion of macro 'FAMILY_TEST'
81 | FAMILY_TEST(TEST_ARGS, bit, xchg, init, init, new, new); \
| ^~~~~~~~~~~
lib/atomic64_test.c:203:2: note: in expansion of macro 'XCHG_FAMILY_TEST'
203 | XCHG_FAMILY_TEST(64, v0, v1);
| ^~~~~~~~~~~~~~~~
>> lib/atomic64_test.c:75:9: error: implicit declaration of function 'atomic64_arch_cmpxchg'; did you mean 'atomic64_try_cmpxchg'? [-Werror=implicit-function-declaration]
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~
include/asm-generic/bug.h:183:32: note: in expansion of macro 'if'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~
include/linux/compiler.h:48:24: note: in expansion of macro '__branch_check__'
48 | # define unlikely(x) (__branch_check__(x, 0, __builtin_constant_p(x)))
| ^~~~~~~~~~~~~~~~
include/asm-generic/bug.h:183:36: note: in expansion of macro 'unlikely'
183 | #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
| ^~~~~~~~
lib/atomic64_test.c:75:2: note: in expansion of macro 'BUG_ON'
75 | BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
| ^~~~~~
lib/atomic64_test.c:38:2: note: in expansion of macro 'TEST_ARGS'
38 | test(bit, op, ##args); \
| ^~~~
lib/atomic64_test.c:86:2: note: in expansion of macro 'FAMILY_TEST'
86 | FAMILY_TEST(TEST_ARGS, bit, cmpxchg, \
| ^~~~~~~~~~~
lib/atomic64_test.c:204:2: note: in expansion of macro 'CMPXCHG_FAMILY_TEST'
204 | CMPXCHG_FAMILY_TEST(64, v0, v1, v2);
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors


vim +75 lib/atomic64_test.c

28aa2bda2211f4 Peter Zijlstra 2016-04-18 71
978e5a3692c3b6 Boqun Feng 2015-11-04 72 #define TEST_ARGS(bit, op, init, ret, expect, args...) \
978e5a3692c3b6 Boqun Feng 2015-11-04 73 do { \
978e5a3692c3b6 Boqun Feng 2015-11-04 74 atomic##bit##_set(&v, init); \
978e5a3692c3b6 Boqun Feng 2015-11-04 @75 BUG_ON(atomic##bit##_##op(&v, ##args) != ret); \
978e5a3692c3b6 Boqun Feng 2015-11-04 76 BUG_ON(atomic##bit##_read(&v) != expect); \
978e5a3692c3b6 Boqun Feng 2015-11-04 77 } while (0)
978e5a3692c3b6 Boqun Feng 2015-11-04 78

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (9.34 kB)
.config.gz (28.49 kB)
Download all attachments

2021-07-06 09:13:22

by Mark Rutland

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Mon, Jul 05, 2021 at 08:56:54PM +0100, Mark Rutland wrote:
> On Mon, Jul 05, 2021 at 06:16:49PM +0300, Anatoly Pugachev wrote:
> > Hello!
>
> Hi Anatoly,
>
> > latest sparc64 git kernel produces the following OOPS on running stress-ng as :
> >
> > $ stress-ng -v --mmap 1 -t 30s
> >
> > kernel OOPS (console logs):
> >
> > [ 27.276719] Unable to handle kernel NULL pointer dereference
> > [ 27.276782] tsk->{mm,active_mm}->context = 00000000000003cb
> > [ 27.276818] tsk->{mm,active_mm}->pgd = fff800003a2a0000
> > [ 27.276853] \|/ ____ \|/
> > [ 27.276853] "@'/ .. \`@"
> > [ 27.276853] /_| \__/ |_\
> > [ 27.276853] \__U_/
> > [ 27.276927] stress-ng(928): Oops [#1]
>
> I can reproduce this under QEMU; following your bisection (and working
> around the missing ifdeferry that breaks bisection), I can confirm that
> the first broken commit is:
>
> ff5b4f1ed580 ("locking/atomic: sparc: move to ARCH_ATOMIC")
>
> Sorry about this.
>
> > Can someone please look at this commit ids?
>
> From digging into this, I can't spot an obvious bug in the commit above.

Looking again with fresh eyes, there is a trivial bug after all.

Could you give the patch below a spin? It works for me locally under
QEMU.

Sorry again about this!

Thanks,
Mark
---->8----
From afb683b2ce749dca426d27f05af3ea08455a52d7 Mon Sep 17 00:00:00 2001
From: Mark Rutland <[email protected]>
Date: Tue, 6 Jul 2021 09:55:56 +0100
Subject: [PATCH] locking/atomic: sparc: fix arch_cmpxchg64_local()

Anatoly reports that since commit:

ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")

... it's possible to reliably trigger an oops by running:

stress-ng -v --mmap 1 -t 30s

... which results in a NULL pointer dereference in
__split_huge_pmd_locked().

The underlying problem is that commit ff5b4f1ed580c59d left
arch_cmpxchg64_local() defined in terms of cmpxchg_local() rather than
arch_cmpxchg_local(). In <asm-generic/atomic-instrumented.h> we wrap
these with macros which use identically-named variables. When
cmpxchg_local() nests inside cmpxchg64_local(), this casues it to use an
unitialized variable as the pointer, which can be NULL.

This can also be seen in pmdp_establish(), where the compiler can
generate the pointer with a `clr` instruction:

0000000000000360 <pmdp_establish>:
360: 9d e3 bf 50 save %sp, -176, %sp
364: fa 5e 80 00 ldx [ %i2 ], %i5
368: 82 10 00 1b mov %i3, %g1
36c: 84 10 20 00 clr %g2
370: c3 f0 90 1d casx [ %g2 ], %i5, %g1
374: 80 a7 40 01 cmp %i5, %g1
378: 32 6f ff fc bne,a %xcc, 368 <pmdp_establish+0x8>
37c: fa 5e 80 00 ldx [ %i2 ], %i5
380: d0 5e 20 40 ldx [ %i0 + 0x40 ], %o0
384: 96 10 00 1b mov %i3, %o3
388: 94 10 00 1d mov %i5, %o2
38c: 92 10 00 19 mov %i1, %o1
390: 7f ff ff 84 call 1a0 <__set_pmd_acct>
394: b0 10 00 1d mov %i5, %i0
398: 81 cf e0 08 return %i7 + 8
39c: 01 00 00 00 nop

This patch fixes the problem by defining arch_cmpxchg64_local() in terms
of arch_cmpxchg_local(), avoiding potential shadowing, and resulting in
working cmpxchg64_local() and variants, e.g.

0000000000000360 <pmdp_establish>:
360: 9d e3 bf 50 save %sp, -176, %sp
364: fa 5e 80 00 ldx [ %i2 ], %i5
368: 82 10 00 1b mov %i3, %g1
36c: c3 f6 90 1d casx [ %i2 ], %i5, %g1
370: 80 a7 40 01 cmp %i5, %g1
374: 32 6f ff fd bne,a %xcc, 368 <pmdp_establish+0x8>
378: fa 5e 80 00 ldx [ %i2 ], %i5
37c: d0 5e 20 40 ldx [ %i0 + 0x40 ], %o0
380: 96 10 00 1b mov %i3, %o3
384: 94 10 00 1d mov %i5, %o2
388: 92 10 00 19 mov %i1, %o1
38c: 7f ff ff 85 call 1a0 <__set_pmd_acct>
390: b0 10 00 1d mov %i5, %i0
394: 81 cf e0 08 return %i7 + 8
398: 01 00 00 00 nop
39c: 01 00 00 00 nop

Fixes: ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")
Signed-off-by: Mark Rutland <[email protected]>
Reported-by: Anatoly Pugachev <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
arch/sparc/include/asm/cmpxchg_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/sparc/include/asm/cmpxchg_64.h b/arch/sparc/include/asm/cmpxchg_64.h
index 8c39a9981187..12d00a42c0a3 100644
--- a/arch/sparc/include/asm/cmpxchg_64.h
+++ b/arch/sparc/include/asm/cmpxchg_64.h
@@ -201,7 +201,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr,
#define arch_cmpxchg64_local(ptr, o, n) \
({ \
BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
- cmpxchg_local((ptr), (o), (n)); \
+ arch_cmpxchg_local((ptr), (o), (n)); \
})
#define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))

--
2.11.0

2021-07-06 12:23:21

by Mark Rutland

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Tue, Jul 06, 2021 at 02:51:06PM +0300, Anatoly Pugachev wrote:
> On Tue, Jul 6, 2021 at 12:11 PM Mark Rutland <[email protected]> wrote:
> > Fixes: ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")
> > Signed-off-by: Mark Rutland <[email protected]>
> > Reported-by: Anatoly Pugachev <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > ---
> > arch/sparc/include/asm/cmpxchg_64.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/sparc/include/asm/cmpxchg_64.h b/arch/sparc/include/asm/cmpxchg_64.h
> > index 8c39a9981187..12d00a42c0a3 100644
> > --- a/arch/sparc/include/asm/cmpxchg_64.h
> > +++ b/arch/sparc/include/asm/cmpxchg_64.h
> > @@ -201,7 +201,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr,
> > #define arch_cmpxchg64_local(ptr, o, n) \
> > ({ \
> > BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
> > - cmpxchg_local((ptr), (o), (n)); \
> > + arch_cmpxchg_local((ptr), (o), (n)); \
> > })
> > #define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))
>
>
> Mark, thanks, fixed...
> tested on git kernel 5.13.0-11788-g79160a603bdb-dirty (dirty - cause
> patch has been applied).

Great! Thanks for confirming.

Peter, are you happy to pick that (full commit in last mail), or should
I send a new copy?

Thanks,
Mark.

2021-07-06 12:25:30

by Anatoly Pugachev

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Tue, Jul 6, 2021 at 12:11 PM Mark Rutland <[email protected]> wrote:
> Fixes: ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")
> Signed-off-by: Mark Rutland <[email protected]>
> Reported-by: Anatoly Pugachev <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> ---
> arch/sparc/include/asm/cmpxchg_64.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/sparc/include/asm/cmpxchg_64.h b/arch/sparc/include/asm/cmpxchg_64.h
> index 8c39a9981187..12d00a42c0a3 100644
> --- a/arch/sparc/include/asm/cmpxchg_64.h
> +++ b/arch/sparc/include/asm/cmpxchg_64.h
> @@ -201,7 +201,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr,
> #define arch_cmpxchg64_local(ptr, o, n) \
> ({ \
> BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
> - cmpxchg_local((ptr), (o), (n)); \
> + arch_cmpxchg_local((ptr), (o), (n)); \
> })
> #define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))


Mark, thanks, fixed...
tested on git kernel 5.13.0-11788-g79160a603bdb-dirty (dirty - cause
patch has been applied).

2021-07-07 07:49:00

by Anatoly Pugachev

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Tue, Jul 6, 2021 at 3:00 PM Mark Rutland <[email protected]> wrote:
> On Tue, Jul 06, 2021 at 02:51:06PM +0300, Anatoly Pugachev wrote:
> > On Tue, Jul 6, 2021 at 12:11 PM Mark Rutland <[email protected]> wrote:
> > > Fixes: ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")
> > > Signed-off-by: Mark Rutland <[email protected]>
> > > Reported-by: Anatoly Pugachev <[email protected]>
> > > Cc: "David S. Miller" <[email protected]>
> > > Cc: Peter Zijlstra <[email protected]>
> > > ---
> > > arch/sparc/include/asm/cmpxchg_64.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/sparc/include/asm/cmpxchg_64.h b/arch/sparc/include/asm/cmpxchg_64.h
> > > index 8c39a9981187..12d00a42c0a3 100644
> > > --- a/arch/sparc/include/asm/cmpxchg_64.h
> > > +++ b/arch/sparc/include/asm/cmpxchg_64.h
> > > @@ -201,7 +201,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr,
> > > #define arch_cmpxchg64_local(ptr, o, n) \
> > > ({ \
> > > BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
> > > - cmpxchg_local((ptr), (o), (n)); \
> > > + arch_cmpxchg_local((ptr), (o), (n)); \
> > > })
> > > #define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))
> >
> >
> > Mark, thanks, fixed...
> > tested on git kernel 5.13.0-11788-g79160a603bdb-dirty (dirty - cause
> > patch has been applied).
>
> Great! Thanks for confirming.
>
> Peter, are you happy to pick that (full commit in last mail), or should
> I send a new copy?

It would be nice if patch could hit the kernel before v5.14-rc1

Thanks.

2021-07-07 08:27:45

by Mark Rutland

[permalink] [raw]
Subject: Re: [sparc64] locking/atomic, kernel OOPS on running stress-ng

On Wed, Jul 07, 2021 at 10:47:06AM +0300, Anatoly Pugachev wrote:
> On Tue, Jul 6, 2021 at 3:00 PM Mark Rutland <[email protected]> wrote:
> > On Tue, Jul 06, 2021 at 02:51:06PM +0300, Anatoly Pugachev wrote:
> > > On Tue, Jul 6, 2021 at 12:11 PM Mark Rutland <[email protected]> wrote:
> > > > Fixes: ff5b4f1ed580c59d ("locking/atomic: sparc: move to ARCH_ATOMIC")
> > > > Signed-off-by: Mark Rutland <[email protected]>
> > > > Reported-by: Anatoly Pugachev <[email protected]>
> > > > Cc: "David S. Miller" <[email protected]>
> > > > Cc: Peter Zijlstra <[email protected]>
> > > > ---
> > > > arch/sparc/include/asm/cmpxchg_64.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/sparc/include/asm/cmpxchg_64.h b/arch/sparc/include/asm/cmpxchg_64.h
> > > > index 8c39a9981187..12d00a42c0a3 100644
> > > > --- a/arch/sparc/include/asm/cmpxchg_64.h
> > > > +++ b/arch/sparc/include/asm/cmpxchg_64.h
> > > > @@ -201,7 +201,7 @@ static inline unsigned long __cmpxchg_local(volatile void *ptr,
> > > > #define arch_cmpxchg64_local(ptr, o, n) \
> > > > ({ \
> > > > BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
> > > > - cmpxchg_local((ptr), (o), (n)); \
> > > > + arch_cmpxchg_local((ptr), (o), (n)); \
> > > > })
> > > > #define arch_cmpxchg64(ptr, o, n) arch_cmpxchg64_local((ptr), (o), (n))
> > >
> > >
> > > Mark, thanks, fixed...
> > > tested on git kernel 5.13.0-11788-g79160a603bdb-dirty (dirty - cause
> > > patch has been applied).
> >
> > Great! Thanks for confirming.
> >
> > Peter, are you happy to pick that (full commit in last mail), or should
> > I send a new copy?
>
> It would be nice if patch could hit the kernel before v5.14-rc1

Absolutely; I'll resend this on it's own so that it's easier for folk to
pick, and I'll poke people about picking it.

Mark.