* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-13 20:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160113204844.GV6357@twins.programming.kicks-ass.net>
On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
>
>> I ask HW team about it but I have a question - has it any relationship with
>> replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
> Of course. If you cannot explain the semantics of the primitives you
> introduce, how can we judge the patch.
>
>
You missed a point - it is a question about replacement of SYNC with
lightweight primitives. It is NOT a question about multithread system
behavior without any SYNC. The answer on a latest Will's question lies
in different area.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-13 22:26 UTC (permalink / raw)
To: Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160113104516.GE25458@arm.com>
On 01/13/2016 02:45 AM, Will Deacon wrote:
>>
> I don't think the address dependency is enough on its own. By that
> reasoning, the following variant (WRC+addr+addr) would work too:
>
>
> P0:
> Wx = 1
>
> P1:
> Rx == 1
> <address dep>
> Wy = 1
>
> P2:
> Ry == 1
> <address dep>
> Rx = 0
>
>
> So are you saying that this is also forbidden?
> Imagine that P0 and P1 are two threads that share a store buffer. What
> then?
OK, I collected answers and it is:
In MIPS R6 this test passes OK, I mean - P2: Rx = 1 if Ry is read
as 1. By design.
However, it is unclear that happens in MIPS R2 1004K.
Moreover, there are voices against guarantee that it will be in
future and that voices point me to Documentation/memory-barriers.txt
section "DATA DEPENDENCY BARRIERS" examples which require SYNC_RMB
between loading address/index and using that for loading data based on
that address or index for shared data (look on CPU2 pseudo-code):
> To deal with this, a data dependency barrier or better must be inserted
> between the address load and the data load:
>
> CPU 1 CPU 2
> =============== ===============
> { A == 1, B == 2, C = 3, P == &A, Q == &C }
> B = 4;
> <write barrier>
> WRITE_ONCE(P, &B);
> Q = READ_ONCE(P);
> <data dependency barrier> <-----------
> SYNC_RMB is here
> D = *Q;
...
> Another example of where data dependency barriers might be required is
> where a
> number is read from memory and then used to calculate the index for an
> array
> access:
>
> CPU 1 CPU 2
> =============== ===============
> { M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
> M[1] = 4;
> <write barrier>
> WRITE_ONCE(P, 1);
> Q = READ_ONCE(P);
> <data dependency barrier> <------------
> SYNC_RMB is here
> D = M[Q];
That voices say that there is a legitimate reason to relax HW here for
performance if SYNC_RMB is needed anyway to work with this sequence of
shared data.
And all that is out-of-topic here in my mind. I just want to be sure
that this patchset still provides a use of a specific lightweight SYNCs
on MIPS vs bold and heavy generalized "SYNC 0" in any case.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Michael S. Tsirkin @ 2016-01-14 9:24 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, linux-sh, Peter Zijlstra, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <5696CF08.8080700@imgtec.com>
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
> And all that is out-of-topic here in my mind. I just want to be sure that
> this patchset still provides a use of a specific lightweight SYNCs on MIPS
> vs bold and heavy generalized "SYNC 0" in any case.
>
> - Leonid.
Of course it does. All this patchset does is rename smp_mb/rmb/wmb
to __smp_mb()/__smp_rmb()/__smp_wmb()
and then asm-generic does #define smp_mb __smp_mb
or #define smp_mb barrier depending on CONFIG_SMP.
Why is that needed? So we can implement
[PATCH v3 28/41] asm-generic: implement virt_xxx memory barriers
--
MST
^ permalink raw reply
* Re: [PATCH v3 0/4] x86: faster mb()+documentation tweaks
From: Borislav Petkov @ 2016-01-14 11:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Davidlohr Bueso, Davidlohr Bueso, Peter Zijlstra,
the arch/x86 maintainers, linux-kernel, virtualization,
H. Peter Anvin, Thomas Gleixner, Paul E. McKenney, Linus Torvalds,
Ingo Molnar
In-Reply-To: <1452715911-12067-1-git-send-email-mst@redhat.com>
On Wed, Jan 13, 2016 at 10:12:22PM +0200, Michael S. Tsirkin wrote:
> mb() typically uses mfence on modern x86, but a micro-benchmark shows that it's
> 2 to 3 times slower than lock; addl that we use on older CPUs.
>
> So let's use the locked variant everywhere.
>
> While I was at it, I found some inconsistencies in comments in
> arch/x86/include/asm/barrier.h
>
> The documentation fixes are included first - I verified that
> they do not change the generated code at all. They should be
> safe to apply directly.
>
> The last patch changes mb() to lock addl. I was unable to
> measure a speed difference on a macro benchmark,
> but I noted that even doing
> #define mb() barrier()
> seems to make no difference for most benchmarks
> (it causes hangs sometimes, of course).
>
> HPA asked that the last patch is deferred until we hear back from
> intel, which makes sense of course. So it needs HPA's ack.
>
> I hope I'm not splitting this up too much - the reason is I wanted to isolate
> the code changes (that people might want to test for performance)
> from comment changes approved by Linus, from (so far unreviewed) changes
> I came up with myself.
>
> Changes from v2:
> add patch adding cc clobber for addl
> tweak commit log for patch 2
> use addl at SP-4 (as opposed to SP) to reduce data dependencies
>
> Michael S. Tsirkin (4):
> x86: add cc clobber for addl
> x86: drop a comment left over from X86_OOSTORE
> x86: tweak the comment about use of wmb for IO
First three look ok to me regardless of what happens with 4. So applied.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-14 12:04 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <5696BA6E.4070508@imgtec.com>
On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
> On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
> >On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
> >
> >>I ask HW team about it but I have a question - has it any relationship with
> >>replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
> >Of course. If you cannot explain the semantics of the primitives you
> >introduce, how can we judge the patch.
> >
> >
> You missed a point - it is a question about replacement of SYNC with
> lightweight primitives. It is NOT a question about multithread system
> behavior without any SYNC. The answer on a latest Will's question lies in
> different area.
The reason we (Peter and I) care about this isn't because we enjoy being
obstructive. It's because there is a whole load of core (i.e. portable)
kernel code that is written to the *kernel* memory model. For example,
the scheduler, RCU, mutex implementations, perf, drivers, you name it.
Consequently, it's important that the architecture back-ends implement
these portable primitives (e.g. smp_mb()) in a way that satisfies the
kernel memory model so that core code doesn't need to worry about the
underlying architecture for synchronisation purposes. You could turn
around and say "but if MIPS gets it wrong, then that's MIPS's problem",
but actually not having a general understanding of the ordering guarantees
provided by each architecture makes it very difficult for us to extend
the kernel memory model in such a way that it can be implemented
efficiently across the board *and* relied upon by core code.
The virtio patch at the start of the thread doesn't particularly concern
me. It's the other patches you linked to that implement acquire/release
that have me worried.
Will
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Will Deacon @ 2016-01-14 12:14 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <5696CF08.8080700@imgtec.com>
On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
> On 01/13/2016 02:45 AM, Will Deacon wrote:
> >>
> >I don't think the address dependency is enough on its own. By that
> >reasoning, the following variant (WRC+addr+addr) would work too:
> >
> >
> >P0:
> >Wx = 1
> >
> >P1:
> >Rx == 1
> ><address dep>
> >Wy = 1
> >
> >P2:
> >Ry == 1
> ><address dep>
> >Rx = 0
> >
> >
> >So are you saying that this is also forbidden?
> >Imagine that P0 and P1 are two threads that share a store buffer. What
> >then?
>
> OK, I collected answers and it is:
>
> In MIPS R6 this test passes OK, I mean - P2: Rx = 1 if Ry is read as 1.
> By design.
>
> However, it is unclear that happens in MIPS R2 1004K.
How can it be unclear? If, for example, the outcome is permitted on that
CPU, then your original reasoning for the WRC+sync+addr doesn't apply
there and SYNC is not transitive. That's what I'm trying to get to the
bottom of.
Does the MIPS kernel target a particular CPU at compile time?
> Moreover, there are voices against guarantee that it will be in future
> and that voices point me to Documentation/memory-barriers.txt section "DATA
> DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
> address/index and using that for loading data based on that address or index
> for shared data (look on CPU2 pseudo-code):
> >To deal with this, a data dependency barrier or better must be inserted
> >between the address load and the data load:
> >
> > CPU 1 CPU 2
> > =============== ===============
> > { A == 1, B == 2, C = 3, P == &A, Q == &C }
> > B = 4;
> > <write barrier>
> > WRITE_ONCE(P, &B);
> > Q = READ_ONCE(P);
> > <data dependency barrier> <-----------
> >SYNC_RMB is here
> > D = *Q;
> ...
> >Another example of where data dependency barriers might be required is
> >where a
> >number is read from memory and then used to calculate the index for an
> >array
> >access:
> >
> > CPU 1 CPU 2
> > =============== ===============
> > { M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
> > M[1] = 4;
> > <write barrier>
> > WRITE_ONCE(P, 1);
> > Q = READ_ONCE(P);
> > <data dependency barrier> <------------
> >SYNC_RMB is here
> > D = M[Q];
>
> That voices say that there is a legitimate reason to relax HW here for
> performance if SYNC_RMB is needed anyway to work with this sequence of
> shared data.
Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
> And all that is out-of-topic here in my mind. I just want to be sure that
> this patchset still provides a use of a specific lightweight SYNCs on MIPS
> vs bold and heavy generalized "SYNC 0" in any case.
We may be highjacking the thread slightly, but there are much bigger
issues at play here if you want to start using lightweight barriers to
implement relaxed kernel primitives such as smp_load_acquire and
smp_store_release.
Will
^ permalink raw reply
* Possible memory corruption in virtio-pci driver.
From: Jerome Marchand @ 2016-01-14 13:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 19160 bytes --]
Hi Michael,
KASan detected a use-after-free error in virtio-pci remove code. In
virtio_pci_remove(), vp_dev is still used after being freed in
unregister_virtio_device() (in virtio_pci_release_dev() more
precisely). I don't know the proper way to fix this.
Here is the KASan output:
[ 467.987227] ==================================================================
[ 467.990023] BUG: KASAN: use-after-free in virtio_pci_remove+0x37/0x70 [virtio_pci] at addr ffff880035364540
[ 467.993397] Read of size 8 by task modprobe/12507
[ 467.995656] =============================================================================
[ 467.998737] BUG kmalloc-2048 (Tainted: G B D C ): kasan: bad access detected
[ 468.001317] -----------------------------------------------------------------------------
[ 468.001317]
[ 468.006025] INFO: Allocated in virtio_pci_probe+0x33/0x190 [virtio_pci] age=461605 cpu=1 pid=297
[ 468.009567] ___slab_alloc+0x511/0x580
[ 468.012192] __slab_alloc+0x51/0x90
[ 468.014659] kmem_cache_alloc_trace+0x1c8/0x210
[ 468.017456] virtio_pci_probe+0x33/0x190 [virtio_pci]
[ 468.020354] local_pci_probe+0x7a/0xd0
[ 468.022288] pci_device_probe+0x1a4/0x1f0
[ 468.024381] driver_probe_device+0x16b/0x640
[ 468.026629] __driver_attach+0xbd/0xc0
[ 468.029025] bus_for_each_dev+0xeb/0x150
[ 468.031773] driver_attach+0x2b/0x30
[ 468.034422] bus_add_driver+0x30a/0x3d0
[ 468.037126] driver_register+0xd3/0x190
[ 468.039878] __pci_register_driver+0xb4/0xc0
[ 468.042700] locks_end_grace+0x1e/0x50 [grace]
[ 468.045394] do_one_initcall+0x10c/0x270
[ 468.048180] do_init_module+0xf4/0x329
[ 468.051019] INFO: Freed in virtio_pci_release_dev+0x12/0x20 [virtio_pci] age=0 cpu=0 pid=12507
[ 468.054736] __slab_free+0x175/0x280
[ 468.057449] kfree+0x1a6/0x1c0
[ 468.060003] virtio_pci_release_dev+0x12/0x20 [virtio_pci]
[ 468.062743] device_release+0x4a/0xf0
[ 468.064653] kobject_release+0xad/0x200
[ 468.066405] kobject_put+0x30/0x60
[ 468.068327] device_unregister+0x2c/0x70
[ 468.071058] unregister_virtio_device+0x20/0x40 [virtio]
[ 468.074097] virtio_pci_remove+0x2b/0x70 [virtio_pci]
[ 468.076138] pci_device_remove+0x61/0x100
[ 468.078689] __device_release_driver+0xec/0x200
[ 468.080772] driver_detach+0x117/0x120
[ 468.082520] bus_remove_driver+0x98/0x160
[ 468.084323] driver_unregister+0x43/0x70
[ 468.086099] pci_unregister_driver+0x2a/0x90
[ 468.087912] virtio_pci_driver_exit+0x10/0x289 [virtio_pci]
[ 468.089980] INFO: Slab 0xffffea0000d4d800 objects=13 used=11 fp=0xffff880035361260 flags=0x3ffc0000004080
[ 468.092800] INFO: Object 0xffff880035364050 @offset=16464 fp=0x (null)
[ 468.092800]
[ 468.096436] Bytes b4 ffff880035364040: 01 00 00 00 2d 01 00 00 17 84 fb ff 00 00 00 00 ....-...........
[ 468.099330] Object ffff880035364050: 00 00 00 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
[ 468.102188] Object ffff880035364060: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.105248] Object ffff880035364070: 00 2f 02 a0 ff ff ff ff 30 01 28 83 ff ff ff ff ./......0.(.....
[ 468.108942] Object ffff880035364080: 00 00 00 00 00 00 00 00 40 12 02 a0 ff ff ff ff ........@.......
[ 468.112687] Object ffff880035364090: 58 45 1d 6c 00 88 ff ff f0 aa 39 35 00 88 ff ff XE.l......95....
[ 468.116582] Object ffff8800353640a0: 00 40 9a 34 00 88 ff ff a8 40 36 35 00 88 ff ff .@.4.....@65....
[ 468.120388] Object ffff8800353640b0: a8 40 36 35 00 88 ff ff 00 00 00 00 00 00 00 00 .@65............
[ 468.123544] Object ffff8800353640c0: 10 1d 66 6c 00 88 ff ff a0 12 72 82 ff ff ff ff ..fl......r.....
[ 468.126375] Object ffff8800353640d0: 00 00 00 00 00 00 00 00 00 00 00 00 0d 00 00 00 ................
[ 468.129040] Object ffff8800353640e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.132341] Object ffff8800353640f0: 01 00 00 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
[ 468.135056] Object ffff880035364100: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.137552] Object ffff880035364110: 80 5a af 82 ff ff ff ff 00 00 00 00 00 00 00 00 .Z..............
[ 468.140670] Object ffff880035364120: 00 00 00 00 00 00 00 00 00 46 e6 81 ff ff ff ff .........F......
[ 468.143868] Object ffff880035364130: 30 41 36 35 00 88 ff ff 30 41 36 35 00 88 ff ff 0A65....0A65....
[ 468.147565] Object ffff880035364140: 00 00 00 00 00 00 00 00 f0 40 36 35 00 88 ff ff .........@65....
[ 468.151229] Object ffff880035364150: 40 5b af 82 ff ff ff ff e0 1b 24 83 ff ff ff ff @[........$.....
[ 468.153883] Object ffff880035364160: 00 00 00 00 00 00 00 00 c0 6c 03 82 ff ff ff ff .........l......
[ 468.157109] Object ffff880035364170: 00 20 02 a0 ff ff ff ff 00 00 00 00 00 00 00 00 . ..............
[ 468.160940] Object ffff880035364180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.164250] Object ffff880035364190: 00 00 00 00 80 00 00 00 00 00 00 00 ad 4e ad de .............N..
[ 468.168223] Object ffff8800353641a0: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.172228] Object ffff8800353641b0: 20 c1 8a 83 ff ff ff ff 90 1d 24 83 ff ff ff ff .........$.....
[ 468.176178] Object ffff8800353641c0: 00 00 00 00 00 00 00 00 40 6d 03 82 ff ff ff ff ........@m......
[ 468.180054] Object ffff8800353641d0: d0 41 36 35 00 88 ff ff d0 41 36 35 00 88 ff ff .A65.....A65....
[ 468.183932] Object ffff8800353641e0: fe ff ff ff 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
[ 468.187798] Object ffff8800353641f0: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.191511] Object ffff880035364200: a0 c9 8a 83 ff ff ff ff 10 10 24 83 ff ff ff ff ..........$.....
[ 468.194598] Object ffff880035364210: 00 00 00 00 00 00 00 00 40 e2 03 82 ff ff ff ff ........@.......
[ 468.196912] Object ffff880035364220: 20 42 36 35 00 88 ff ff 20 42 36 35 00 88 ff ff B65.... B65....
[ 468.199678] Object ffff880035364230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.203724] Object ffff880035364240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.207643] Object ffff880035364250: 00 00 00 00 00 00 00 00 10 f2 7b 81 ff ff ff ff ..........{.....
[ 468.211475] Object ffff880035364260: 90 40 36 35 00 88 ff ff 01 00 00 00 ff ff ff ff .@65............
[ 468.214824] Object ffff880035364270: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.218710] Object ffff880035364280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.221866] Object ffff880035364290: 20 c9 8a 83 ff ff ff ff 00 00 00 00 00 00 00 00 ...............
[ 468.225708] Object ffff8800353642a0: 00 00 00 00 00 00 00 00 e0 d4 03 82 ff ff ff ff ................
[ 468.229567] Object ffff8800353642b0: 00 00 00 00 00 00 00 00 e0 ff ff ff 0f 00 00 00 ................
[ 468.233707] Object ffff8800353642c0: c0 42 36 35 00 88 ff ff c0 42 36 35 00 88 ff ff .B65.....B65....
[ 468.236995] Object ffff8800353642d0: 40 f8 7b 81 ff ff ff ff 60 c9 8a 83 ff ff ff ff @.{.....`.......
[ 468.239469] Object ffff8800353642e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.242092] Object ffff8800353642f0: a0 d4 03 82 ff ff ff ff 00 00 00 00 ad 4e ad de .............N..
[ 468.246171] Object ffff880035364300: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.250155] Object ffff880035364310: e0 c8 8a 83 ff ff ff ff 00 00 00 00 00 00 00 00 ................
[ 468.254108] Object ffff880035364320: 00 00 00 00 00 00 00 00 20 d5 03 82 ff ff ff ff ........ .......
[ 468.257405] Object ffff880035364330: 30 43 36 35 00 88 ff ff 30 43 36 35 00 88 ff ff 0C65....0C65....
[ 468.261159] Object ffff880035364340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.264029] Object ffff880035364350: 82 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 ................
[ 468.266641] Object ffff880035364360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.269249] Object ffff880035364370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.271800] Object ffff880035364380: 32 85 fb ff 00 00 00 00 00 00 00 00 00 00 00 00 2...............
[ 468.274368] Object ffff880035364390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.276918] Object ffff8800353643a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.279446] Object ffff8800353643b0: 00 00 00 00 00 00 00 00 b8 43 36 35 00 88 ff ff .........C65....
[ 468.281959] Object ffff8800353643c0: b8 43 36 35 00 88 ff ff ff ff ff ff 00 00 00 00 .C65............
[ 468.284498] Object ffff8800353643d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.288159] Object ffff8800353643e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.291900] Object ffff8800353643f0: f0 43 36 35 00 88 ff ff f0 43 36 35 00 88 ff ff .C65.....C65....
[ 468.294442] Object ffff880035364400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.296938] Object ffff880035364410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.299392] Object ffff880035364420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.301821] Object ffff880035364430: 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
[ 468.304262] Object ffff880035364440: ff ff ff ff ff ff ff ff 60 c1 8a 83 ff ff ff ff ........`.......
[ 468.306697] Object ffff880035364450: 30 28 25 83 ff ff ff ff 00 00 00 00 00 00 00 00 0(%.............
[ 468.309180] Object ffff880035364460: 00 6d 03 82 ff ff ff ff 68 44 36 35 00 88 ff ff .m......hD65....
[ 468.311634] Object ffff880035364470: 68 44 36 35 00 88 ff ff 00 00 00 00 00 00 00 00 hD65............
[ 468.315349] Object ffff880035364480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.319247] Object ffff880035364490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.323239] Object ffff8800353644a0: 00 00 00 00 00 00 00 00 90 9f 04 a0 ff ff ff ff ................
[ 468.327047] Object ffff8800353644b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.330824] Object ffff8800353644c0: 05 00 00 00 f4 1a 00 00 20 ca 04 a0 ff ff ff ff ........ .......
[ 468.334530] Object ffff8800353644d0: 00 00 00 00 00 00 00 00 d8 44 36 35 00 88 ff ff .........D65....
[ 468.338324] Object ffff8800353644e0: d8 44 36 35 00 88 ff ff 02 00 00 30 00 00 00 00 .D65.......0....
[ 468.342082] Object ffff8800353644f0: 20 37 a2 5f 00 88 ff ff c0 44 1d 6c 00 88 ff ff 7._.....D.l....
[ 468.345679] Object ffff880035364500: 13 c1 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.349365] Object ffff880035364510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.353272] Object ffff880035364520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.356041] Object ffff880035364530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.359712] Object ffff880035364540: 00 c1 01 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
[ 468.363386] Object ffff880035364550: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
[ 468.367082] Object ffff880035364560: 40 dd 04 a0 ff ff ff ff c0 f8 27 83 ff ff ff ff @.........'.....
[ 468.370743] Object ffff880035364570: 00 00 00 00 00 00 00 00 20 c6 04 a0 ff ff ff ff ........ .......
[ 468.373920] Object ffff880035364580: 80 45 36 35 00 88 ff ff 80 45 36 35 00 88 ff ff .E65.....E65....
[ 468.377400] Object ffff880035364590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.380959] Object ffff8800353645a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.384465] Object ffff8800353645b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.387889] Object ffff8800353645c0: 00 00 00 00 00 00 00 00 70 b6 04 a0 ff ff ff ff ........p.......
[ 468.390421] Object ffff8800353645d0: c0 b4 04 a0 ff ff ff ff e0 b5 04 a0 ff ff ff ff ................
[ 468.392643] Object ffff8800353645e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.394914] Object ffff8800353645f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.397226] Object ffff880035364600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.400344] Object ffff880035364610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.402957] Object ffff880035364620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.405496] Object ffff880035364630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.408076] Object ffff880035364640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.410591] Object ffff880035364650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.413755] Object ffff880035364660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.416388] Object ffff880035364670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.418917] Object ffff880035364680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.421442] Object ffff880035364690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.423979] Object ffff8800353646a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.427009] Object ffff8800353646b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.430256] Object ffff8800353646c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.433458] Object ffff8800353646d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.436006] Object ffff8800353646e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.438457] Object ffff8800353646f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.440878] Object ffff880035364700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.443925] Object ffff880035364710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.446409] Object ffff880035364720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.449451] Object ffff880035364730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.452441] Object ffff880035364740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.455046] Object ffff880035364750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.458026] Object ffff880035364760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.460966] Object ffff880035364770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.463946] Object ffff880035364780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.466956] Object ffff880035364790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.469865] Object ffff8800353647a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.472684] Object ffff8800353647b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.475187] Object ffff8800353647c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.477277] Object ffff8800353647d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.480034] Object ffff8800353647e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.482730] Object ffff8800353647f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.485410] Object ffff880035364800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.488062] Object ffff880035364810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.490731] Object ffff880035364820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.493298] Object ffff880035364830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.495887] Object ffff880035364840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 468.498467] CPU: 0 PID: 12507 Comm: modprobe Tainted: G B D C 4.4.0kasan+ #275
[ 468.500762] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 468.502698] ffffea0000d4d800 ffff880048edfc28 ffffffff815bfaa7 ffff88006cc02d80
[ 468.505094] ffff880048edfc58 ffffffff8130db19 ffff88006cc02d80 ffffea0000d4d800
[ 468.507477] ffff880035364050 ffff88006c1d4518 ffff880048edfc80 ffffffff81313a54
[ 468.509854] Call Trace:
[ 468.511217] [<ffffffff815bfaa7>] dump_stack+0x44/0x5d
[ 468.513091] [<ffffffff8130db19>] print_trailer+0xf9/0x150
[ 468.515016] [<ffffffff81313a54>] object_err+0x34/0x40
[ 468.516831] [<ffffffff81316142>] kasan_report_error+0x212/0x520
[ 468.518148] [<ffffffff8115f71e>] ? do_raw_spin_lock+0x10e/0x1a0
[ 468.519364] [<ffffffff81157866>] ? trace_hardirqs_on_caller+0x16/0x290
[ 468.520692] [<ffffffff81316849>] kasan_report+0x39/0x40
[ 468.521876] [<ffffffffa004a3b7>] ? virtio_pci_remove+0x37/0x70 [virtio_pci]
[ 468.524219] [<ffffffff8131531d>] __asan_load8+0x5d/0x70
[ 468.526269] [<ffffffffa004a3b7>] virtio_pci_remove+0x37/0x70 [virtio_pci]
[ 468.528614] [<ffffffff816380a1>] pci_device_remove+0x61/0x100
[ 468.530842] [<ffffffff817adbac>] __device_release_driver+0xec/0x200
[ 468.533206] [<ffffffff817aebd7>] driver_detach+0x117/0x120
[ 468.535566] [<ffffffff817ad2b8>] bus_remove_driver+0x98/0x160
[ 468.537794] [<ffffffff817af733>] driver_unregister+0x43/0x70
[ 468.540103] [<ffffffff8163568a>] pci_unregister_driver+0x2a/0x90
[ 468.542476] [<ffffffffa004bd87>] virtio_pci_driver_exit+0x10/0x289 [virtio_pci]
[ 468.545189] [<ffffffff811b5230>] SyS_delete_module+0x260/0x2b0
[ 468.547626] [<ffffffff811b4fd0>] ? free_module+0x450/0x450
[ 468.550061] [<ffffffff8115314d>] ? trace_hardirqs_off+0xd/0x10
[ 468.552521] [<ffffffff81157866>] ? trace_hardirqs_on_caller+0x16/0x290
[ 468.555142] [<ffffffff81c4f972>] entry_SYSCALL_64_fastpath+0x12/0x76
[ 468.557138] Memory state around the buggy address:
[ 468.558965] ffff880035364400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 468.561771] ffff880035364480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 468.564716] >ffff880035364500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 468.567593] ^
[ 468.570169] ffff880035364580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 468.573101] ffff880035364600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 468.575705] ==================================================================
Thanks,
Jerome
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio_pci: fix use after free on release
From: Michael S. Tsirkin @ 2016-01-14 14:04 UTC (permalink / raw)
To: linux-kernel; +Cc: Sasha Levin, Jerome Marchand, stable, virtualization
KASan detected a use-after-free error in virtio-pci remove code. In
virtio_pci_remove(), vp_dev is still used after being freed in
unregister_virtio_device() (in virtio_pci_release_dev() more
precisely).
To fix, keep a reference until cleanup is done.
Fixes: 63bd62a08ca4 ("virtio_pci: defer kfree until release callback")
Reported-by: Jerome Marchand <jmarchan@redhat.com>
Cc: stable@vger.kernel.org
Cc: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_pci_common.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 36205c2..f6bed86 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -545,6 +545,7 @@ err_enable_device:
static void virtio_pci_remove(struct pci_dev *pci_dev)
{
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+ struct device *dev = get_device(&vp_dev->vdev.dev);
unregister_virtio_device(&vp_dev->vdev);
@@ -554,6 +555,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
virtio_pci_modern_remove(vp_dev);
pci_disable_device(pci_dev);
+ put_device(dev);
}
static struct pci_driver virtio_pci_driver = {
--
MST
^ permalink raw reply related
* Re: Possible memory corruption in virtio-pci driver.
From: Michael S. Tsirkin @ 2016-01-14 14:38 UTC (permalink / raw)
To: Jerome Marchand; +Cc: linux-kernel, virtualization
In-Reply-To: <5697A1DE.1000607@redhat.com>
On Thu, Jan 14, 2016 at 02:25:50PM +0100, Jerome Marchand wrote:
> Hi Michael,
>
> KASan detected a use-after-free error in virtio-pci remove code. In
> virtio_pci_remove(), vp_dev is still used after being freed in
> unregister_virtio_device() (in virtio_pci_release_dev() more
> precisely). I don't know the proper way to fix this.
Thanks a lot for the report, I posted a patch - would
appreciate a Tested-by tag.
> Here is the KASan output:
>
> [ 467.987227] ==================================================================
> [ 467.990023] BUG: KASAN: use-after-free in virtio_pci_remove+0x37/0x70 [virtio_pci] at addr ffff880035364540
> [ 467.993397] Read of size 8 by task modprobe/12507
> [ 467.995656] =============================================================================
> [ 467.998737] BUG kmalloc-2048 (Tainted: G B D C ): kasan: bad access detected
> [ 468.001317] -----------------------------------------------------------------------------
> [ 468.001317]
> [ 468.006025] INFO: Allocated in virtio_pci_probe+0x33/0x190 [virtio_pci] age=461605 cpu=1 pid=297
> [ 468.009567] ___slab_alloc+0x511/0x580
> [ 468.012192] __slab_alloc+0x51/0x90
> [ 468.014659] kmem_cache_alloc_trace+0x1c8/0x210
> [ 468.017456] virtio_pci_probe+0x33/0x190 [virtio_pci]
> [ 468.020354] local_pci_probe+0x7a/0xd0
> [ 468.022288] pci_device_probe+0x1a4/0x1f0
> [ 468.024381] driver_probe_device+0x16b/0x640
> [ 468.026629] __driver_attach+0xbd/0xc0
> [ 468.029025] bus_for_each_dev+0xeb/0x150
> [ 468.031773] driver_attach+0x2b/0x30
> [ 468.034422] bus_add_driver+0x30a/0x3d0
> [ 468.037126] driver_register+0xd3/0x190
> [ 468.039878] __pci_register_driver+0xb4/0xc0
> [ 468.042700] locks_end_grace+0x1e/0x50 [grace]
> [ 468.045394] do_one_initcall+0x10c/0x270
> [ 468.048180] do_init_module+0xf4/0x329
> [ 468.051019] INFO: Freed in virtio_pci_release_dev+0x12/0x20 [virtio_pci] age=0 cpu=0 pid=12507
> [ 468.054736] __slab_free+0x175/0x280
> [ 468.057449] kfree+0x1a6/0x1c0
> [ 468.060003] virtio_pci_release_dev+0x12/0x20 [virtio_pci]
> [ 468.062743] device_release+0x4a/0xf0
> [ 468.064653] kobject_release+0xad/0x200
> [ 468.066405] kobject_put+0x30/0x60
> [ 468.068327] device_unregister+0x2c/0x70
> [ 468.071058] unregister_virtio_device+0x20/0x40 [virtio]
> [ 468.074097] virtio_pci_remove+0x2b/0x70 [virtio_pci]
> [ 468.076138] pci_device_remove+0x61/0x100
> [ 468.078689] __device_release_driver+0xec/0x200
> [ 468.080772] driver_detach+0x117/0x120
> [ 468.082520] bus_remove_driver+0x98/0x160
> [ 468.084323] driver_unregister+0x43/0x70
> [ 468.086099] pci_unregister_driver+0x2a/0x90
> [ 468.087912] virtio_pci_driver_exit+0x10/0x289 [virtio_pci]
> [ 468.089980] INFO: Slab 0xffffea0000d4d800 objects=13 used=11 fp=0xffff880035361260 flags=0x3ffc0000004080
> [ 468.092800] INFO: Object 0xffff880035364050 @offset=16464 fp=0x (null)
> [ 468.092800]
> [ 468.096436] Bytes b4 ffff880035364040: 01 00 00 00 2d 01 00 00 17 84 fb ff 00 00 00 00 ....-...........
> [ 468.099330] Object ffff880035364050: 00 00 00 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
> [ 468.102188] Object ffff880035364060: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.105248] Object ffff880035364070: 00 2f 02 a0 ff ff ff ff 30 01 28 83 ff ff ff ff ./......0.(.....
> [ 468.108942] Object ffff880035364080: 00 00 00 00 00 00 00 00 40 12 02 a0 ff ff ff ff ........@.......
> [ 468.112687] Object ffff880035364090: 58 45 1d 6c 00 88 ff ff f0 aa 39 35 00 88 ff ff XE.l......95....
> [ 468.116582] Object ffff8800353640a0: 00 40 9a 34 00 88 ff ff a8 40 36 35 00 88 ff ff .@.4.....@65....
> [ 468.120388] Object ffff8800353640b0: a8 40 36 35 00 88 ff ff 00 00 00 00 00 00 00 00 .@65............
> [ 468.123544] Object ffff8800353640c0: 10 1d 66 6c 00 88 ff ff a0 12 72 82 ff ff ff ff ..fl......r.....
> [ 468.126375] Object ffff8800353640d0: 00 00 00 00 00 00 00 00 00 00 00 00 0d 00 00 00 ................
> [ 468.129040] Object ffff8800353640e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.132341] Object ffff8800353640f0: 01 00 00 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
> [ 468.135056] Object ffff880035364100: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.137552] Object ffff880035364110: 80 5a af 82 ff ff ff ff 00 00 00 00 00 00 00 00 .Z..............
> [ 468.140670] Object ffff880035364120: 00 00 00 00 00 00 00 00 00 46 e6 81 ff ff ff ff .........F......
> [ 468.143868] Object ffff880035364130: 30 41 36 35 00 88 ff ff 30 41 36 35 00 88 ff ff 0A65....0A65....
> [ 468.147565] Object ffff880035364140: 00 00 00 00 00 00 00 00 f0 40 36 35 00 88 ff ff .........@65....
> [ 468.151229] Object ffff880035364150: 40 5b af 82 ff ff ff ff e0 1b 24 83 ff ff ff ff @[........$.....
> [ 468.153883] Object ffff880035364160: 00 00 00 00 00 00 00 00 c0 6c 03 82 ff ff ff ff .........l......
> [ 468.157109] Object ffff880035364170: 00 20 02 a0 ff ff ff ff 00 00 00 00 00 00 00 00 . ..............
> [ 468.160940] Object ffff880035364180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.164250] Object ffff880035364190: 00 00 00 00 80 00 00 00 00 00 00 00 ad 4e ad de .............N..
> [ 468.168223] Object ffff8800353641a0: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.172228] Object ffff8800353641b0: 20 c1 8a 83 ff ff ff ff 90 1d 24 83 ff ff ff ff .........$.....
> [ 468.176178] Object ffff8800353641c0: 00 00 00 00 00 00 00 00 40 6d 03 82 ff ff ff ff ........@m......
> [ 468.180054] Object ffff8800353641d0: d0 41 36 35 00 88 ff ff d0 41 36 35 00 88 ff ff .A65.....A65....
> [ 468.183932] Object ffff8800353641e0: fe ff ff ff 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
> [ 468.187798] Object ffff8800353641f0: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.191511] Object ffff880035364200: a0 c9 8a 83 ff ff ff ff 10 10 24 83 ff ff ff ff ..........$.....
> [ 468.194598] Object ffff880035364210: 00 00 00 00 00 00 00 00 40 e2 03 82 ff ff ff ff ........@.......
> [ 468.196912] Object ffff880035364220: 20 42 36 35 00 88 ff ff 20 42 36 35 00 88 ff ff B65.... B65....
> [ 468.199678] Object ffff880035364230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.203724] Object ffff880035364240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.207643] Object ffff880035364250: 00 00 00 00 00 00 00 00 10 f2 7b 81 ff ff ff ff ..........{.....
> [ 468.211475] Object ffff880035364260: 90 40 36 35 00 88 ff ff 01 00 00 00 ff ff ff ff .@65............
> [ 468.214824] Object ffff880035364270: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.218710] Object ffff880035364280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.221866] Object ffff880035364290: 20 c9 8a 83 ff ff ff ff 00 00 00 00 00 00 00 00 ...............
> [ 468.225708] Object ffff8800353642a0: 00 00 00 00 00 00 00 00 e0 d4 03 82 ff ff ff ff ................
> [ 468.229567] Object ffff8800353642b0: 00 00 00 00 00 00 00 00 e0 ff ff ff 0f 00 00 00 ................
> [ 468.233707] Object ffff8800353642c0: c0 42 36 35 00 88 ff ff c0 42 36 35 00 88 ff ff .B65.....B65....
> [ 468.236995] Object ffff8800353642d0: 40 f8 7b 81 ff ff ff ff 60 c9 8a 83 ff ff ff ff @.{.....`.......
> [ 468.239469] Object ffff8800353642e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.242092] Object ffff8800353642f0: a0 d4 03 82 ff ff ff ff 00 00 00 00 ad 4e ad de .............N..
> [ 468.246171] Object ffff880035364300: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.250155] Object ffff880035364310: e0 c8 8a 83 ff ff ff ff 00 00 00 00 00 00 00 00 ................
> [ 468.254108] Object ffff880035364320: 00 00 00 00 00 00 00 00 20 d5 03 82 ff ff ff ff ........ .......
> [ 468.257405] Object ffff880035364330: 30 43 36 35 00 88 ff ff 30 43 36 35 00 88 ff ff 0C65....0C65....
> [ 468.261159] Object ffff880035364340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.264029] Object ffff880035364350: 82 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00 ................
> [ 468.266641] Object ffff880035364360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.269249] Object ffff880035364370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.271800] Object ffff880035364380: 32 85 fb ff 00 00 00 00 00 00 00 00 00 00 00 00 2...............
> [ 468.274368] Object ffff880035364390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.276918] Object ffff8800353643a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.279446] Object ffff8800353643b0: 00 00 00 00 00 00 00 00 b8 43 36 35 00 88 ff ff .........C65....
> [ 468.281959] Object ffff8800353643c0: b8 43 36 35 00 88 ff ff ff ff ff ff 00 00 00 00 .C65............
> [ 468.284498] Object ffff8800353643d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.288159] Object ffff8800353643e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.291900] Object ffff8800353643f0: f0 43 36 35 00 88 ff ff f0 43 36 35 00 88 ff ff .C65.....C65....
> [ 468.294442] Object ffff880035364400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.296938] Object ffff880035364410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.299392] Object ffff880035364420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.301821] Object ffff880035364430: 00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00 .....N..........
> [ 468.304262] Object ffff880035364440: ff ff ff ff ff ff ff ff 60 c1 8a 83 ff ff ff ff ........`.......
> [ 468.306697] Object ffff880035364450: 30 28 25 83 ff ff ff ff 00 00 00 00 00 00 00 00 0(%.............
> [ 468.309180] Object ffff880035364460: 00 6d 03 82 ff ff ff ff 68 44 36 35 00 88 ff ff .m......hD65....
> [ 468.311634] Object ffff880035364470: 68 44 36 35 00 88 ff ff 00 00 00 00 00 00 00 00 hD65............
> [ 468.315349] Object ffff880035364480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.319247] Object ffff880035364490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.323239] Object ffff8800353644a0: 00 00 00 00 00 00 00 00 90 9f 04 a0 ff ff ff ff ................
> [ 468.327047] Object ffff8800353644b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.330824] Object ffff8800353644c0: 05 00 00 00 f4 1a 00 00 20 ca 04 a0 ff ff ff ff ........ .......
> [ 468.334530] Object ffff8800353644d0: 00 00 00 00 00 00 00 00 d8 44 36 35 00 88 ff ff .........D65....
> [ 468.338324] Object ffff8800353644e0: d8 44 36 35 00 88 ff ff 02 00 00 30 00 00 00 00 .D65.......0....
> [ 468.342082] Object ffff8800353644f0: 20 37 a2 5f 00 88 ff ff c0 44 1d 6c 00 88 ff ff 7._.....D.l....
> [ 468.345679] Object ffff880035364500: 13 c1 01 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.349365] Object ffff880035364510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.353272] Object ffff880035364520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.356041] Object ffff880035364530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.359712] Object ffff880035364540: 00 c1 01 00 00 00 00 00 00 00 00 00 ad 4e ad de .............N..
> [ 468.363386] Object ffff880035364550: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff ................
> [ 468.367082] Object ffff880035364560: 40 dd 04 a0 ff ff ff ff c0 f8 27 83 ff ff ff ff @.........'.....
> [ 468.370743] Object ffff880035364570: 00 00 00 00 00 00 00 00 20 c6 04 a0 ff ff ff ff ........ .......
> [ 468.373920] Object ffff880035364580: 80 45 36 35 00 88 ff ff 80 45 36 35 00 88 ff ff .E65.....E65....
> [ 468.377400] Object ffff880035364590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.380959] Object ffff8800353645a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.384465] Object ffff8800353645b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.387889] Object ffff8800353645c0: 00 00 00 00 00 00 00 00 70 b6 04 a0 ff ff ff ff ........p.......
> [ 468.390421] Object ffff8800353645d0: c0 b4 04 a0 ff ff ff ff e0 b5 04 a0 ff ff ff ff ................
> [ 468.392643] Object ffff8800353645e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.394914] Object ffff8800353645f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.397226] Object ffff880035364600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.400344] Object ffff880035364610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.402957] Object ffff880035364620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.405496] Object ffff880035364630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.408076] Object ffff880035364640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.410591] Object ffff880035364650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.413755] Object ffff880035364660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.416388] Object ffff880035364670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.418917] Object ffff880035364680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.421442] Object ffff880035364690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.423979] Object ffff8800353646a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.427009] Object ffff8800353646b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.430256] Object ffff8800353646c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.433458] Object ffff8800353646d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.436006] Object ffff8800353646e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.438457] Object ffff8800353646f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.440878] Object ffff880035364700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.443925] Object ffff880035364710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.446409] Object ffff880035364720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.449451] Object ffff880035364730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.452441] Object ffff880035364740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.455046] Object ffff880035364750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.458026] Object ffff880035364760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.460966] Object ffff880035364770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.463946] Object ffff880035364780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.466956] Object ffff880035364790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.469865] Object ffff8800353647a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.472684] Object ffff8800353647b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.475187] Object ffff8800353647c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.477277] Object ffff8800353647d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.480034] Object ffff8800353647e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.482730] Object ffff8800353647f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.485410] Object ffff880035364800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.488062] Object ffff880035364810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.490731] Object ffff880035364820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.493298] Object ffff880035364830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.495887] Object ffff880035364840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> [ 468.498467] CPU: 0 PID: 12507 Comm: modprobe Tainted: G B D C 4.4.0kasan+ #275
> [ 468.500762] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [ 468.502698] ffffea0000d4d800 ffff880048edfc28 ffffffff815bfaa7 ffff88006cc02d80
> [ 468.505094] ffff880048edfc58 ffffffff8130db19 ffff88006cc02d80 ffffea0000d4d800
> [ 468.507477] ffff880035364050 ffff88006c1d4518 ffff880048edfc80 ffffffff81313a54
> [ 468.509854] Call Trace:
> [ 468.511217] [<ffffffff815bfaa7>] dump_stack+0x44/0x5d
> [ 468.513091] [<ffffffff8130db19>] print_trailer+0xf9/0x150
> [ 468.515016] [<ffffffff81313a54>] object_err+0x34/0x40
> [ 468.516831] [<ffffffff81316142>] kasan_report_error+0x212/0x520
> [ 468.518148] [<ffffffff8115f71e>] ? do_raw_spin_lock+0x10e/0x1a0
> [ 468.519364] [<ffffffff81157866>] ? trace_hardirqs_on_caller+0x16/0x290
> [ 468.520692] [<ffffffff81316849>] kasan_report+0x39/0x40
> [ 468.521876] [<ffffffffa004a3b7>] ? virtio_pci_remove+0x37/0x70 [virtio_pci]
> [ 468.524219] [<ffffffff8131531d>] __asan_load8+0x5d/0x70
> [ 468.526269] [<ffffffffa004a3b7>] virtio_pci_remove+0x37/0x70 [virtio_pci]
> [ 468.528614] [<ffffffff816380a1>] pci_device_remove+0x61/0x100
> [ 468.530842] [<ffffffff817adbac>] __device_release_driver+0xec/0x200
> [ 468.533206] [<ffffffff817aebd7>] driver_detach+0x117/0x120
> [ 468.535566] [<ffffffff817ad2b8>] bus_remove_driver+0x98/0x160
> [ 468.537794] [<ffffffff817af733>] driver_unregister+0x43/0x70
> [ 468.540103] [<ffffffff8163568a>] pci_unregister_driver+0x2a/0x90
> [ 468.542476] [<ffffffffa004bd87>] virtio_pci_driver_exit+0x10/0x289 [virtio_pci]
> [ 468.545189] [<ffffffff811b5230>] SyS_delete_module+0x260/0x2b0
> [ 468.547626] [<ffffffff811b4fd0>] ? free_module+0x450/0x450
> [ 468.550061] [<ffffffff8115314d>] ? trace_hardirqs_off+0xd/0x10
> [ 468.552521] [<ffffffff81157866>] ? trace_hardirqs_on_caller+0x16/0x290
> [ 468.555142] [<ffffffff81c4f972>] entry_SYSCALL_64_fastpath+0x12/0x76
> [ 468.557138] Memory state around the buggy address:
> [ 468.558965] ffff880035364400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 468.561771] ffff880035364480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 468.564716] >ffff880035364500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 468.567593] ^
> [ 468.570169] ffff880035364580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 468.573101] ffff880035364600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 468.575705] ==================================================================
>
>
> Thanks,
> Jerome
>
^ permalink raw reply
* Re: [PATCH] virtio_pci: fix use after free on release
From: Jerome Marchand @ 2016-01-14 14:55 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: Sasha Levin, stable, virtualization
In-Reply-To: <1452780236-27981-1-git-send-email-mst@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 1525 bytes --]
On 01/14/2016 03:04 PM, Michael S. Tsirkin wrote:
> KASan detected a use-after-free error in virtio-pci remove code. In
> virtio_pci_remove(), vp_dev is still used after being freed in
> unregister_virtio_device() (in virtio_pci_release_dev() more
> precisely).
>
> To fix, keep a reference until cleanup is done.
That does make KASan happy.
>
> Fixes: 63bd62a08ca4 ("virtio_pci: defer kfree until release callback")
> Reported-by: Jerome Marchand <jmarchan@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jerome Marchand <jmarchan@redhat.com>
Thanks,
Jerome
> ---
> drivers/virtio/virtio_pci_common.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 36205c2..f6bed86 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -545,6 +545,7 @@ err_enable_device:
> static void virtio_pci_remove(struct pci_dev *pci_dev)
> {
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> + struct device *dev = get_device(&vp_dev->vdev.dev);
>
> unregister_virtio_device(&vp_dev->vdev);
>
> @@ -554,6 +555,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> virtio_pci_modern_remove(vp_dev);
>
> pci_disable_device(pci_dev);
> + put_device(dev);
> }
>
> static struct pci_driver virtio_pci_driver = {
>
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 16:16 UTC (permalink / raw)
To: Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160114120445.GB15828@arm.com>
On Thu, Jan 14, 2016 at 12:04:45PM +0000, Will Deacon wrote:
> On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
> > On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
> > >On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
> > >
> > >>I ask HW team about it but I have a question - has it any relationship with
> > >>replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
> > >Of course. If you cannot explain the semantics of the primitives you
> > >introduce, how can we judge the patch.
> > >
> > >
> > You missed a point - it is a question about replacement of SYNC with
> > lightweight primitives. It is NOT a question about multithread system
> > behavior without any SYNC. The answer on a latest Will's question lies in
> > different area.
>
> The reason we (Peter and I) care about this isn't because we enjoy being
> obstructive. It's because there is a whole load of core (i.e. portable)
> kernel code that is written to the *kernel* memory model. For example,
> the scheduler, RCU, mutex implementations, perf, drivers, you name it.
>
> Consequently, it's important that the architecture back-ends implement
> these portable primitives (e.g. smp_mb()) in a way that satisfies the
> kernel memory model so that core code doesn't need to worry about the
> underlying architecture for synchronisation purposes. You could turn
> around and say "but if MIPS gets it wrong, then that's MIPS's problem",
> but actually not having a general understanding of the ordering guarantees
> provided by each architecture makes it very difficult for us to extend
> the kernel memory model in such a way that it can be implemented
> efficiently across the board *and* relied upon by core code.
What Will said!
Yes, you can cut corners within MIPS architecture-specific code,
but primitives that are used in the core kernel really do need to
work as expected.
Thanx, Paul
> The virtio patch at the start of the thread doesn't particularly concern
> me. It's the other patches you linked to that implement acquire/release
> that have me worried.
>
> Will
>
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 19:28 UTC (permalink / raw)
To: Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160114121449.GC15828@arm.com>
On 01/14/2016 04:14 AM, Will Deacon wrote:
> On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
>
>> Moreover, there are voices against guarantee that it will be in future
>> and that voices point me to Documentation/memory-barriers.txt section "DATA
>> DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
>> address/index and using that for loading data based on that address or index
>> for shared data (look on CPU2 pseudo-code):
>>> To deal with this, a data dependency barrier or better must be inserted
>>> between the address load and the data load:
>>>
>>> CPU 1 CPU 2
>>> =============== ===============
>>> { A == 1, B == 2, C = 3, P == &A, Q == &C }
>>> B = 4;
>>> <write barrier>
>>> WRITE_ONCE(P, &B);
>>> Q = READ_ONCE(P);
>>> <data dependency barrier> <-----------
>>> SYNC_RMB is here
>>> D = *Q;
>> ...
>>> Another example of where data dependency barriers might be required is
>>> where a
>>> number is read from memory and then used to calculate the index for an
>>> array
>>> access:
>>>
>>> CPU 1 CPU 2
>>> =============== ===============
>>> { M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
>>> M[1] = 4;
>>> <write barrier>
>>> WRITE_ONCE(P, 1);
>>> Q = READ_ONCE(P);
>>> <data dependency barrier> <------------
>>> SYNC_RMB is here
>>> D = M[Q];
>> That voices say that there is a legitimate reason to relax HW here for
>> performance if SYNC_RMB is needed anyway to work with this sequence of
>> shared data.
> Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
It is not me, it is Documentation/memory-barriers.txt from kernel sources.
HW team can't work on voice statements, it should do a work on written
documents. If that is written (see above the lines which I marked by
"SYNC_RMB") then anybody should use it and never mind how many
CPUs/Threads are in play. This examples explicitly requires to insert
"data dependency barrier" between reading a shared pointer/index and
using it to fetch a shared data. So, your WRC+addr+addr test is a
violation of that recommendation.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 19:42 UTC (permalink / raw)
To: paulmck, Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel, And
In-Reply-To: <20160114161604.GT3818@linux.vnet.ibm.com>
On 01/14/2016 08:16 AM, Paul E. McKenney wrote:
> On Thu, Jan 14, 2016 at 12:04:45PM +0000, Will Deacon wrote:
>> On Wed, Jan 13, 2016 at 12:58:22PM -0800, Leonid Yegoshin wrote:
>>> On 01/13/2016 12:48 PM, Peter Zijlstra wrote:
>>>> On Wed, Jan 13, 2016 at 11:02:35AM -0800, Leonid Yegoshin wrote:
>>>>
>>>>> I ask HW team about it but I have a question - has it any relationship with
>>>>> replacing MIPS SYNC with lightweight SYNCs (SYNC_WMB etc)?
>>>> Of course. If you cannot explain the semantics of the primitives you
>>>> introduce, how can we judge the patch.
>>>>
>>>>
>>> You missed a point - it is a question about replacement of SYNC with
>>> lightweight primitives. It is NOT a question about multithread system
>>> behavior without any SYNC. The answer on a latest Will's question lies in
>>> different area.
> What Will said!
>
> Yes, you can cut corners within MIPS architecture-specific code,
> but primitives that are used in the core kernel really do need to
> work as expected.
>
> Thanx, Paul
>
>
Absolutelly! Please use SYNC - right now it is not.
An the only point - please use an appropriate SYNC_* barriers instead of
heavy bold hammer. That stuff was design explicitly to support the
requirements of Documentation/memory-barriers.txt
It is easy - just use smp_acquire instead of plain smp_mb
insmp_load_acquire, at least for MIPS.
- Leonid.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 20:12 UTC (permalink / raw)
To: Will Deacon
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, Paul McKenney, linux-xtensa, james.hogan,
Arnd Bergmann, Stefano Stabellini, adi-buildroot-devel,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160114120445.GB15828@arm.com>
On 01/14/2016 04:04 AM, Will Deacon wrote:
> Consequently, it's important that the architecture back-ends implement
> these portable primitives (e.g. smp_mb()) in a way that satisfies the
> kernel memory model so that core code doesn't need to worry about the
> underlying architecture for synchronisation purposes.
It seems you don't listen me. I said multiple times - MIPS
implementation of SYNC_RMB/SYNC_WMB/SYNC_MB/SYNC_ACQUIRE/SYNC_RELEASE
instructions matches the description of
smp_rmb/smp_wmb/smp_mb/sync_acquire/sync_release from
Documentation/memory-barriers.txt file.
What else do you want from me - RTL or microArch design for that?
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-14 20:15 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, paulmck, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <5697FA0A.6040601@imgtec.com>
On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
> An the only point - please use an appropriate SYNC_* barriers instead of
> heavy bold hammer. That stuff was design explicitly to support the
> requirements of Documentation/memory-barriers.txt
That's madness. That document changes from version to version as to what
we _think_ the actual hardware does. It is _NOT_ a specification.
You cannot design hardware from that. Its incomplete and fails to
specify a bunch of things. It not a mathematically sound definition of a
memory model.
Please stop referring to that document for what a particular barrier
_should_ do. Explain what MIPS does, so we can attempt to integrate
this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
upon our understanding of hardware and improve the Linux memory model.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 20:34 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <5697F6D2.60409@imgtec.com>
On Thu, Jan 14, 2016 at 11:28:18AM -0800, Leonid Yegoshin wrote:
> On 01/14/2016 04:14 AM, Will Deacon wrote:
> >On Wed, Jan 13, 2016 at 02:26:16PM -0800, Leonid Yegoshin wrote:
> >
> >> Moreover, there are voices against guarantee that it will be in future
> >>and that voices point me to Documentation/memory-barriers.txt section "DATA
> >>DEPENDENCY BARRIERS" examples which require SYNC_RMB between loading
> >>address/index and using that for loading data based on that address or index
> >>for shared data (look on CPU2 pseudo-code):
> >>>To deal with this, a data dependency barrier or better must be inserted
> >>>between the address load and the data load:
> >>>
> >>> CPU 1 CPU 2
> >>> =============== ===============
> >>> { A == 1, B == 2, C = 3, P == &A, Q == &C }
> >>> B = 4;
> >>> <write barrier>
> >>> WRITE_ONCE(P, &B);
> >>> Q = READ_ONCE(P);
> >>> <data dependency barrier> <-----------
> >>>SYNC_RMB is here
> >>> D = *Q;
> >>...
> >>>Another example of where data dependency barriers might be required is
> >>>where a
> >>>number is read from memory and then used to calculate the index for an
> >>>array
> >>>access:
> >>>
> >>> CPU 1 CPU 2
> >>> =============== ===============
> >>> { M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
> >>> M[1] = 4;
> >>> <write barrier>
> >>> WRITE_ONCE(P, 1);
> >>> Q = READ_ONCE(P);
> >>> <data dependency barrier> <------------
> >>>SYNC_RMB is here
> >>> D = M[Q];
> >>That voices say that there is a legitimate reason to relax HW here for
> >>performance if SYNC_RMB is needed anyway to work with this sequence of
> >>shared data.
> >Are you saying that MIPS needs to implement [smp_]read_barrier_depends?
>
> It is not me, it is Documentation/memory-barriers.txt from kernel sources.
>
> HW team can't work on voice statements, it should do a work on
> written documents. If that is written (see above the lines which I
> marked by "SYNC_RMB") then anybody should use it and never mind how
> many CPUs/Threads are in play. This examples explicitly requires to
> insert "data dependency barrier" between reading a shared
> pointer/index and using it to fetch a shared data. So, your
> WRC+addr+addr test is a violation of that recommendation.
Perhaps Documentation/memory-barriers.txt needs additional clarification.
It would not be the first time.
If your CPU implicitly maintains ordering based on address and
data dependencies, then you don't need any instructions for
<data dependency barrier>.
The WRC+addr+addr is OK because data dependencies are not required to be
transitive, in other words, they are not required to flow from one CPU to
another without the help of an explicit memory barrier. Transitivity is
instead supplied by smp_mb() and by smp_store_release()-smp_load_acquire()
chains. Here is the Linux kernel code for WRC+addr+addr, give or take
(and no, I have no idea why anyone would want to write code like this):
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
WRITE_ONCE(p->a, &x);
}
void cpu2(void)
{
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
It is legal to end the run with x==&f and r1==&x. To prevent this outcome,
we do the following:
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
smp_store_release(&p->a, &x); /* Additional ordering. */
}
void cpu2(void)
{
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
And I still don't know why anyone would need this sort of code. ;-)
Alternatively, we pull cpu2() into cpu1():
struct foo {
struct foo **a;
};
struct foo b;
struct foo c;
struct foo d;
struct foo e;
struct foo f = { &d };
struct foo g = { &e };
struct foo *x = &b;
void cpu0(void)
{
WRITE_ONCE(x, &f);
}
void cpu1(void)
{
struct foo *p;
p = lockless_dereference(x);
WRITE_ONCE(p->a, &x);
r1 = lockless_dereference(f.a);
WRITE_ONCE(*r1, &c);
}
The ordering is now enforced by being within a single thread. In fact,
the second lockless_dereference() can be READ_ONCE().
So, does MIPS maintain ordering within a given CPU based on address and
data dependencies? If so, you don't need to emit memory-barrier instructions
for read_barrier_depends().
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 20:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, Leonid Yegoshin,
ddaney.cavm, Thomas Gleixner, linux-metag
In-Reply-To: <20160114201513.GI6357@twins.programming.kicks-ass.net>
On Thu, Jan 14, 2016 at 09:15:13PM +0100, Peter Zijlstra wrote:
> On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
> > An the only point - please use an appropriate SYNC_* barriers instead of
> > heavy bold hammer. That stuff was design explicitly to support the
> > requirements of Documentation/memory-barriers.txt
>
> That's madness. That document changes from version to version as to what
> we _think_ the actual hardware does. It is _NOT_ a specification.
There is work in progress on a specification, but please don't hold
your breath. And I am not as optimistic as I might be about any formal
specification keeping up with the Linux kernel or with the hardware that
it supports. But it seems worth a good try.
> You cannot design hardware from that. Its incomplete and fails to
> specify a bunch of things. It not a mathematically sound definition of a
> memory model.
>
> Please stop referring to that document for what a particular barrier
> _should_ do. Explain what MIPS does, so we can attempt to integrate
> this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
> upon our understanding of hardware and improve the Linux memory model.
Please!
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Peter Zijlstra @ 2016-01-14 20:46 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, paulmck, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <20160114201513.GI6357@twins.programming.kicks-ass.net>
On Thu, Jan 14, 2016 at 09:15:13PM +0100, Peter Zijlstra wrote:
> On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
> > An the only point - please use an appropriate SYNC_* barriers instead of
> > heavy bold hammer. That stuff was design explicitly to support the
> > requirements of Documentation/memory-barriers.txt
>
> That's madness. That document changes from version to version as to what
> we _think_ the actual hardware does. It is _NOT_ a specification.
>
> You cannot design hardware from that. Its incomplete and fails to
> specify a bunch of things. It not a mathematically sound definition of a
> memory model.
>
> Please stop referring to that document for what a particular barrier
> _should_ do. Explain what MIPS does, so we can attempt to integrate
> this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
> upon our understanding of hardware and improve the Linux memory model.
That is, if you'd managed to read that file at the right point in time,
you might have through we'd be OK with requiring a barrier for
control dependencies.
We got rid of that mistake. It was based on a flawed reading of the
Alpha docs. See: 105ff3cbf225 ("atomic: remove all traces of
READ_ONCE_CTRL() and atomic*_read_ctrl()")
Similarly, while the document goes to great length to explain the
read_barrier_depends thing, nobody actually thinks its a brilliant idea
to have. Ideally we'd kill the thing the moment we drop Alpha support.
Again, memory-barriers.txt is _NOT_, I repeat, _NOT_ a hardware spec, it
is not even a recommendation. It are our best effort (but flawed)
scribbles of what we think is makes sense given the huge amount of
actual hardware we have to run on.
As to the ACQUIRE/RELEASE semantics, ARM64 actually has
multi-copy-atomic acquire/release (as does ia64, although in reality it
doesn't actually have acquire/release). PPC otoh does _NOT_ have this,
and is currently the only arch to suffer RCpc locks.
Now for a long long time we assumed our locks were RCsc, and we've
written code assuming UNLOCK x + LOCK y was in fact a full barrier with
transitiviy. Then we figured out PPC didn't actually match that. RCU is
the only piece of code we _know_ relied on that, but there might be more
out there...
So we document, for new code, that UNLOCK+LOCK isn't a MB, while at the
same time we lobby PPC to stick a full barrier in and get rid of this
stuff.
Nobody really likes RCpc locks, esp. given the history we have of
assuming RCsc.
The current document allowing for RCpc is not an endorsement thereof.
Ideally we'd _NOT_ have to worry about that. We can do without these
head-aches.
So again, stop referring to our document as a spec. Also please don't
make MIPS push the limits of weak memory models, we really can do
without the pain.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 20:46 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Will Deacon,
virtualization, H. Peter Anvin, sparclinux, Ingo Molnar,
linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, paulmck, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag, linux-arm-kernel
In-Reply-To: <20160114201513.GI6357@twins.programming.kicks-ass.net>
On 01/14/2016 12:15 PM, Peter Zijlstra wrote:
> On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
>> An the only point - please use an appropriate SYNC_* barriers instead of
>> heavy bold hammer. That stuff was design explicitly to support the
>> requirements of Documentation/memory-barriers.txt
> That's madness. That document changes from version to version as to what
> we _think_ the actual hardware does. It is _NOT_ a specification.
>
> You cannot design hardware from that. Its incomplete and fails to
> specify a bunch of things. It not a mathematically sound definition of a
> memory model.
>
> Please stop referring to that document for what a particular barrier
> _should_ do. Explain what MIPS does, so we can attempt to integrate
> this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
> upon our understanding of hardware and improve the Linux memory model.
I am afraid I can't help you here. It is very complicated stuff and a
model is actually doesn't fit your assumptions about CPUs well without
some simplifications which are based on what you want to have.
I say that SYNC_ACQUIRE/etc follows what you expect for smp_acquire etc
(basing on that document). And at least two CPU models were tested with
my patches (see it in LMO) for that last year and that instructions are
implemented now in engineering kernel.
If you have something else in mind, you can ask me. But I prefer to do
not deviate too much from Documentation/memory-barriers.txt, for exam -
if it asks to have memory barrier somewhere, then I assume the code
should have it, and please - don't ask me a test which violates the
current version of document recommendations.
For a moment I don't see a significant changes in this document for MIPS
Arch at least 1.5 year, and the only significant point is that MIPS CPU
Arch doesn't have yet smp_read_barrier_depends() and smp_rmb() should be
used instead.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 20:48 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <56980145.5030901@imgtec.com>
On Thu, Jan 14, 2016 at 12:12:53PM -0800, Leonid Yegoshin wrote:
> On 01/14/2016 04:04 AM, Will Deacon wrote:
> >Consequently, it's important that the architecture back-ends
> >implement these portable primitives (e.g. smp_mb()) in a way that
> >satisfies the kernel memory model so that core code doesn't need
> >to worry about the underlying architecture for synchronisation
> >purposes.
>
> It seems you don't listen me. I said multiple times - MIPS
> implementation of
> SYNC_RMB/SYNC_WMB/SYNC_MB/SYNC_ACQUIRE/SYNC_RELEASE instructions
> matches the description of
> smp_rmb/smp_wmb/smp_mb/sync_acquire/sync_release from
> Documentation/memory-barriers.txt file.
>
> What else do you want from me - RTL or microArch design for that?
I suspect that it is more likely that we are talking past each other.
This stuff is subtle and although we have better ways of talking about
it than (say) ten years ago, it is subtle. Two ways of talking about
it are herd and ppcmem.
The overview of ppcmem (AKA armmem and cppmem) is here:
https://www.cl.cam.ac.uk/~pes20/ppcmem/help.html
The intro to herd is here: http://arxiv.org/pdf/1308.6810v5.pdf
It may be downloaded here: http://diy.inria.fr/herd/
As a very rough rule of thumb, herd is faster and easier to use
and ppcmem is more precise.
So SYNC_RMB is intended to implement smp_rmb(), correct?
You could use SYNC_ACQUIRE() to implement read_barrier_depends() and
smp_read_barrier_depends(), but SYNC_RMB probably does not suffice.
The reason for this is that smp_read_barrier_depends() must order the
pointer load against any subsequent read or write through a dereference
of that pointer. For example:
p = READ_ONCE(gp);
smp_rmb();
r1 = p->a; /* ordered by smp_rmb(). */
p->b = 42; /* NOT ordered by smp_rmb(), BUG!!! */
r2 = x; /* ordered by smp_rmb(), but doesn't need to be. */
In contrast:
p = READ_ONCE(gp);
smp_read_barrier_depends();
r1 = p->a; /* ordered by smp_read_barrier_depends(). */
p->b = 42; /* ordered by smp_read_barrier_depends(). */
r2 = x; /* not ordered by smp_read_barrier_depends(), which is OK. */
Again, if your hardware maintains local ordering for address
and data dependencies, you can have read_barrier_depends() and
smp_read_barrier_depends() be no-ops like they are for most
architectures.
Does that help?
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 21:01 UTC (permalink / raw)
To: paulmck
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <20160114203430.GC3818@linux.vnet.ibm.com>
I need some time to understand your test examples. However,
On 01/14/2016 12:34 PM, Paul E. McKenney wrote:
>
>
> The WRC+addr+addr is OK because data dependencies are not required to be
> transitive, in other words, they are not required to flow from one CPU to
> another without the help of an explicit memory barrier.
I don't see any reliable way to fit WRC+addr+addr into "DATA DEPENDENCY
BARRIERS" section recommendation to have data dependency barrier between
read of a shared pointer/index and read the shared data based on that
pointer. If you have this two reads, it doesn't matter the rest of
scenario, you should put the dependency barrier in code anyway. If you
don't do it in WRC+addr+addr scenario then after years it can be easily
changed to different scenario which fits some of scenario in "DATA
DEPENDENCY BARRIERS" section and fails.
> Transitivity is
Peter Zijlstra recently wrote: "In particular we're very much all
'confused' about the various notions of transitivity". I am confused
too, so - please use some more simple way to explain your words. Sorry,
but we need a common ground first.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 21:24 UTC (permalink / raw)
To: paulmck
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <20160114204827.GE3818@linux.vnet.ibm.com>
On 01/14/2016 12:48 PM, Paul E. McKenney wrote:
>
> So SYNC_RMB is intended to implement smp_rmb(), correct?
Yes.
>
> You could use SYNC_ACQUIRE() to implement read_barrier_depends() and
> smp_read_barrier_depends(), but SYNC_RMB probably does not suffice.
If smp_read_barrier_depends() is used to separate not only two reads but
read pointer and WRITE basing on that pointer (example below) - yes. I
just doesn't see any example of this in famous
Documentation/memory-barriers.txt and had no chance to know what you use
it in this way too.
> The reason for this is that smp_read_barrier_depends() must order the
> pointer load against any subsequent read or write through a dereference
> of that pointer.
I can't see that requirement anywhere in Documents directory. I mean -
the words "write through a dereference of that pointer" or similar for
smp_read_barrier_depends.
> For example:
>
> p = READ_ONCE(gp);
> smp_rmb();
> r1 = p->a; /* ordered by smp_rmb(). */
> p->b = 42; /* NOT ordered by smp_rmb(), BUG!!! */
> r2 = x; /* ordered by smp_rmb(), but doesn't need to be. */
>
> In contrast:
>
> p = READ_ONCE(gp);
> smp_read_barrier_depends();
> r1 = p->a; /* ordered by smp_read_barrier_depends(). */
> p->b = 42; /* ordered by smp_read_barrier_depends(). */
> r2 = x; /* not ordered by smp_read_barrier_depends(), which is OK. */
>
> Again, if your hardware maintains local ordering for address
> and data dependencies, you can have read_barrier_depends() and
> smp_read_barrier_depends() be no-ops like they are for most
> architectures.
It is not so simple, I mean "local ordering for address and data
dependencies". Local ordering is NOT enough. It happens that current
MIPS R6 doesn't require in your example smp_read_barrier_depends() but
in discussion it comes out that it may not. Because without
smp_read_barrier_depends() your example can be a part of Will's
WRC+addr+addr and we found some design which easily can bump into this
test. And that design actually performs "local ordering for address and
data dependencies" too.
- Leonid.
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 21:29 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <56980C91.1010403@imgtec.com>
On Thu, Jan 14, 2016 at 01:01:05PM -0800, Leonid Yegoshin wrote:
> I need some time to understand your test examples. However,
Understood.
> On 01/14/2016 12:34 PM, Paul E. McKenney wrote:
> >
> >
> >The WRC+addr+addr is OK because data dependencies are not required to be
> >transitive, in other words, they are not required to flow from one CPU to
> >another without the help of an explicit memory barrier.
>
> I don't see any reliable way to fit WRC+addr+addr into "DATA
> DEPENDENCY BARRIERS" section recommendation to have data dependency
> barrier between read of a shared pointer/index and read the shared
> data based on that pointer. If you have this two reads, it doesn't
> matter the rest of scenario, you should put the dependency barrier
> in code anyway. If you don't do it in WRC+addr+addr scenario then
> after years it can be easily changed to different scenario which
> fits some of scenario in "DATA DEPENDENCY BARRIERS" section and
> fails.
The trick is that lockless_dereference() contains an
smp_read_barrier_depends():
#define lockless_dereference(p) \
({ \
typeof(p) _________p1 = READ_ONCE(p); \
smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
(_________p1); \
})
Or am I missing your point?
> > Transitivity is
>
> Peter Zijlstra recently wrote: "In particular we're very much all
> 'confused' about the various notions of transitivity". I am confused
> too, so - please use some more simple way to explain your words.
> Sorry, but we need a common ground first.
OK, how about an example? (Z6.3 in the ppcmem naming scheme.)
int x, y, z;
void cpu0(void)
{
WRITE_ONCE(x, 1);
smp_wmb();
WRITE_ONCE(y, 1);
}
void cpu1(void)
{
WRITE_ONCE(y, 2);
smp_wmb();
WRITE_ONCE(z, 1);
}
void cpu2(void)
{
r1 = READ_ONCE(z);
smp_rmb();
r2 = read_once(x);
}
If smp_rmb() and smp_wmb() provided transitive ordering, then cpu2()
would see cpu0()'s ordering. But they do not, so the ordering is
visible at best to the adjacent CPU. This means that the final value
of y can be 2, while at the same time r1==1 && r2==0.
Now the full barrier, smp_mb(), does provide transitive ordering,
so if the three barriers in the above example are replaced with
smp_mb() the y==2 && r1==1 && r2==0 outcome will be prohibited.
So smp_mb() provides transitivity, as do pairs of smp_store_release()
and smp_read_acquire(), as do RCU grace periods. The exact interactions
between transitive and non-transitive ordering is a work in progress.
That said, if a series of transitive segments ends in a write, which
connects to a single non-transitive segment starting with a read,
you should be good. And in fact in the example above, you can replace
the smp_wmb()s with smp_mb() and leave the smp_rmb() and still
prohibit the "cyclic" outcome.
If you want a more formal definition, I must refer you back to the
ppcmem and herd references.
Does that help?
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Paul E. McKenney @ 2016-01-14 21:34 UTC (permalink / raw)
To: Leonid Yegoshin
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <56980933.2020801@imgtec.com>
On Thu, Jan 14, 2016 at 12:46:43PM -0800, Leonid Yegoshin wrote:
> On 01/14/2016 12:15 PM, Peter Zijlstra wrote:
> >On Thu, Jan 14, 2016 at 11:42:02AM -0800, Leonid Yegoshin wrote:
> >>An the only point - please use an appropriate SYNC_* barriers instead of
> >>heavy bold hammer. That stuff was design explicitly to support the
> >>requirements of Documentation/memory-barriers.txt
> >That's madness. That document changes from version to version as to what
> >we _think_ the actual hardware does. It is _NOT_ a specification.
> >
> >You cannot design hardware from that. Its incomplete and fails to
> >specify a bunch of things. It not a mathematically sound definition of a
> >memory model.
> >
> >Please stop referring to that document for what a particular barrier
> >_should_ do. Explain what MIPS does, so we can attempt to integrate
> >this knowledge with our knowledge of PPC/ARM/Alpha/x86/etc. and improve
> >upon our understanding of hardware and improve the Linux memory model.
>
> I am afraid I can't help you here. It is very complicated stuff and
> a model is actually doesn't fit your assumptions about CPUs well
> without some simplifications which are based on what you want to
> have.
>
> I say that SYNC_ACQUIRE/etc follows what you expect for smp_acquire
> etc (basing on that document). And at least two CPU models were
> tested with my patches (see it in LMO) for that last year and that
> instructions are implemented now in engineering kernel.
>
> If you have something else in mind, you can ask me. But I prefer to
> do not deviate too much from Documentation/memory-barriers.txt, for
> exam - if it asks to have memory barrier somewhere, then I assume
> the code should have it, and please - don't ask me a test which
> violates the current version of document recommendations.
>
> For a moment I don't see a significant changes in this document for
> MIPS Arch at least 1.5 year, and the only significant point is that
> MIPS CPU Arch doesn't have yet smp_read_barrier_depends() and
> smp_rmb() should be used instead.
Is SYNC_ACQUIRE a memory-barrier instruction that orders prior loads
against later loads and stores? If so, and if MIPS does not do
ordering based on address and data dependencies, I suggest making
read_barrier_depends() be a SYNC_ACQUIRE rather than SYNC_RMB.
Thanx, Paul
^ permalink raw reply
* Re: [v3,11/41] mips: reuse asm-generic/barrier.h
From: Leonid Yegoshin @ 2016-01-14 21:36 UTC (permalink / raw)
To: paulmck
Cc: linux-mips, linux-ia64, Michael S. Tsirkin, Peter Zijlstra,
Will Deacon, virtualization, H. Peter Anvin, sparclinux,
Ingo Molnar, linux-arch, linux-s390, Russell King - ARM Linux,
user-mode-linux-devel, linux-sh, Michael Ellerman, x86, xen-devel,
Ingo Molnar, linux-xtensa, james.hogan, Arnd Bergmann,
Stefano Stabellini, adi-buildroot-devel, ddaney.cavm,
Thomas Gleixner, linux-metag
In-Reply-To: <20160114212913.GF3818@linux.vnet.ibm.com>
On 01/14/2016 01:29 PM, Paul E. McKenney wrote:
>
>> On 01/14/2016 12:34 PM, Paul E. McKenney wrote:
>>>
>>> The WRC+addr+addr is OK because data dependencies are not required to be
>>> transitive, in other words, they are not required to flow from one CPU to
>>> another without the help of an explicit memory barrier.
>> I don't see any reliable way to fit WRC+addr+addr into "DATA
>> DEPENDENCY BARRIERS" section recommendation to have data dependency
>> barrier between read of a shared pointer/index and read the shared
>> data based on that pointer. If you have this two reads, it doesn't
>> matter the rest of scenario, you should put the dependency barrier
>> in code anyway. If you don't do it in WRC+addr+addr scenario then
>> after years it can be easily changed to different scenario which
>> fits some of scenario in "DATA DEPENDENCY BARRIERS" section and
>> fails.
> The trick is that lockless_dereference() contains an
> smp_read_barrier_depends():
>
> #define lockless_dereference(p) \
> ({ \
> typeof(p) _________p1 = READ_ONCE(p); \
> smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> (_________p1); \
> })
>
> Or am I missing your point?
WRC+addr+addr has no any barrier. lockless_dereference() has a barrier.
I don't see a common points between this and that in your answer, sorry.
- Leonid.
^ 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