lkml.org 
[lkml]   [2019]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: READ_ONCE() + STACKPROTECTOR_STRONG == :/ (was Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.5-2 tag (topic/kasan-bitops))
    Date
    Peter Zijlstra <peterz@infradead.org> writes:
    > On Thu, Dec 12, 2019 at 10:07:56AM +0000, Will Deacon wrote:
    >
    >> > So your proposed change _should_ be fine. Will, I'm assuming you never
    >> > saw this on your ARGH64 builds when you did this code ?
    >>
    >> I did see it, but (a) looking at the code out-of-line makes it look a lot
    >> worse than it actually is (so the ext4 example is really helpful -- thanks
    >> Michael!) and (b) I chalked it up to a crappy compiler.
    >>
    >> However, see this comment from Arnd on my READ_ONCE series from the other
    >> day:
    >>
    >> https://lore.kernel.org/lkml/CAK8P3a0f=WvSQSBQ4t0FmEkcFE_mC3oARxaeTviTSkSa-D2qhg@mail.gmail.com
    >>
    >> In which case, I'm thinking that we should be doing better in READ_ONCE()
    >> for non-buggy compilers which would also keep the KCSAN folks happy for this
    >> code (and would help with [1] too).
    >
    > So something like this then? Although I suppose that should be moved
    > into compiler-gcc.h and then guarded by #ifndef READ_ONCE or so.

    I tried this:

    > @@ -295,6 +296,23 @@ void __write_once_size(volatile void *p, void *res, int size)
    > */
    > #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
    >
    > +#else /* GCC_VERSION < 40800 */
    > +
    > +#define READ_ONCE_NOCHECK(x) \
    > +({ \
    > + typeof(x) __x = *(volatile typeof(x))&(x); \

    Didn't compile, needed:

    typeof(x) __x = *(volatile typeof(&x))&(x); \


    > + smp_read_barrier_depends(); \
    > + __x;
    > +})


    And that works for me. No extra stack check stuff.

    I guess the question is does that version of READ_ONCE() implement the
    read once semantics. Do we have a good way to test that?

    The only differences are because of the early return in the generic
    test_and_set_bit_lock():

    1 <ext4_resize_begin_generic>: 1 <ext4_resize_begin_ppc>:
    2 addis r2,r12,281 2 addis r2,r12,281
    3 addi r2,r2,-22368 3 addi r2,r2,-22064
    4 mflr r0 4 mflr r0
    5 bl <_mcount> 5 bl <_mcount>
    6 mflr r0 6 mflr r0
    7 std r31,-8(r1) 7 std r31,-8(r1)
    8 std r30,-16(r1) 8 std r30,-16(r1)
    9 mr r31,r3 9 mr r31,r3
    10 li r3,24 10 li r3,24
    11 std r0,16(r1) 11 std r0,16(r1)
    12 stdu r1,-128(r1) 12 stdu r1,-112(r1)
    13 ld r30,920(r31) 13 ld r30,920(r31)
    14 bl <capable+0x8> 14 bl <capable+0x8>
    15 nop 15 nop
    16 cmpdi cr7,r3,0 16 cmpdi cr7,r3,0
    17 beq cr7,<ext4_resize_begin_generic+0xf0> 17 beq cr7,<ext4_resize_begin_ppc+0xc0>
    18 ld r9,920(r31) 18 ld r9,920(r31)
    19 ld r10,96(r30) 19 ld r10,96(r30)
    20 lwz r7,84(r30) 20 lwz r7,84(r30)
    21 ld r8,104(r9) 21 ld r8,104(r9)
    22 ld r10,24(r10) 22 ld r10,24(r10)
    23 lwz r8,20(r8) 23 lwz r8,20(r8)
    24 srd r10,r10,r7 24 srd r10,r10,r7
    25 cmpd cr7,r10,r8 25 cmpd cr7,r10,r8
    26 bne cr7,<ext4_resize_begin_generic+0x128> 26 bne cr7,<ext4_resize_begin_ppc+0xf8>
    27 lhz r10,160(r9) 27 lhz r10,160(r9)
    28 andi. r10,r10,2 28 andi. r10,r10,2
    29 bne <ext4_resize_begin_generic+0x100>
    30 ld r10,560(r9)
    31 std r10,104(r1)
    32 ld r10,104(r1)
    33 andi. r10,r10,1
    34 bne <ext4_resize_begin_generic+0xd0> 29 bne <ext4_resize_begin_ppc+0xd0>
    35 addi r7,r9,560 30 addi r9,r9,560
    36 li r8,1 31 li r10,1
    37 ldarx r10,0,r7 32 ldarx r3,0,r9,1
    38 or r6,r8,r10 33 or r8,r3,r10
    39 stdcx. r6,0,r7 34 stdcx. r8,0,r9
    40 bne- <ext4_resize_begin_generic+0x8c> 35 bne- <ext4_resize_begin_ppc+0x78>
    41 isync 36 isync
    37 clrldi r3,r3,63
    42 andi. r9,r10,1 38 addi r3,r3,-1
    43 li r3,0 39 rlwinm r3,r3,0,27,27
    44 bne <ext4_resize_begin_generic+0xd0> 40 addi r3,r3,-16
    45 addi r1,r1,128 41 addi r1,r1,112
    46 ld r0,16(r1) 42 ld r0,16(r1)
    47 ld r30,-16(r1) 43 ld r30,-16(r1)
    48 ld r31,-8(r1) 44 ld r31,-8(r1)
    49 mtlr r0 45 mtlr r0
    50 blr 46 blr
    51 nop 47 nop
    52 nop 48 nop
    53 nop 49 nop
    54 addi r1,r1,128
    55 li r3,-16
    56 ld r0,16(r1)
    57 ld r30,-16(r1)
    58 ld r31,-8(r1)
    59 mtlr r0
    60 blr
    61 nop
    62 li r3,-1 50 li r3,-1
    63 b <ext4_resize_begin_generic+0xac> 51 b <ext4_resize_begin_ppc+0x9c>
    64 nop 52 nop
    65 nop 53 nop
    66 addis r6,r2,-117 54 addis r6,r2,-117
    67 addis r4,r2,-140 55 addis r4,r2,-140
    68 mr r3,r31 56 mr r3,r31
    69 li r5,146 57 li r5,83
    70 addi r6,r6,-32736 58 addi r6,r6,-32736
    71 addi r4,r4,3088 59 addi r4,r4,3064
    72 bl <__ext4_warning+0x8> 60 bl <__ext4_warning+0x8>
    73 nop 61 nop
    74 li r3,-1 62 li r3,-1
    75 b <ext4_resize_begin_generic+0xac> 63 b <ext4_resize_begin_ppc+0x9c>
    76 ld r9,96(r9) 64 ld r9,96(r9)
    77 addis r6,r2,-118 65 addis r6,r2,-118
    78 addis r4,r2,-140 66 addis r4,r2,-140
    79 mr r3,r31 67 mr r3,r31
    80 li r5,136 68 li r5,73
    81 addi r6,r6,32752 69 addi r6,r6,32752
    82 addi r4,r4,3088 70 addi r4,r4,3064
    83 ld r7,24(r9) 71 ld r7,24(r9)
    84 bl <__ext4_warning+0x8> 72 bl <__ext4_warning+0x8>
    85 nop 73 nop
    86 li r3,-1 74 li r3,-1
    87 b <ext4_resize_begin_generic+0xac> 75 b <ext4_resize_begin_ppc+0x9c>


    cheers

    \
     
     \ /
      Last update: 2019-12-13 13:08    [W:4.762 / U:0.284 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site