* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:43 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205213644.GU7829@linux.vnet.ibm.com>
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > >
> > > [ . . . ]
> > >
> > > > > > and this barrier is no longer paired with anything until
> > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > >
> > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > maybe there are other, better tools now.
> > > > >
> > > > > There are quite a few people who say that smp_store_release() is
> > > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > that they are correct.
> > > >
> > > > OK, but smp_store_release is still not paired with anything since we
> > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > >
> > > Why wouldn't you consider the smp_store_release() to be paired with
> > > the new improved READ_ONCE()?
> >
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
> >
> > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> >
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
>
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located. Would that work for you?
>
> Thanx, Paul
Yes, that's exactly what I did for now.
Thanks!
--
MST
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jesse Brandeburg @ 2017-12-05 21:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jakub Kicinski, Hannes Frederic Sowa, achiad shochat,
Sridhar Samudrala, Anjali Singhai Jain, Alexander Duyck,
virtualization, Achiad, Peter Waskiewicz Jr, netdev,
Shannon Nelson, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171205211603-mutt-send-email-mst@kernel.org>
On Tue, 5 Dec 2017 21:20:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
> > Then we'll have a single solution for both netvsc and virtio (and any
> > other PV device).
> > And we could handle the VF DMA dirt issue agnostically.
>
> For the record, I won't block patches adding this kist to virtio
> on the basis that they must be generic. It's not a lot
> of code, implementation can come first, prettify later.
Thanks, based on this discussion we're going to work on improving
virtio-net first, but some of Achiad's points are good. I don't believe
it should block the virtio work however.
In particular I'm really interested in figuring out how we can get to
the point that virtio is able to make or implement some smart decisions
about which NIC to pick for traffic delivery (it's own paravirt path or
the passthorugh device path), if Achiad wants to develop the idea into
some code, I'd be interested to review it.
> But we do need to have a discussion about how devices are paired.
> I am not sure using just MAC works. E.g. some passthrough
> devices don't give host ability to set the MAC.
> Are these worth worrying about?
I personally don't think that will be much of a problem, if a
certain device has that issue, can't we just have the virtio-net device
pick up the MAC address of the passthrough device? As long as they match
things should work OK. It at least is an initial way to do the
configuration that has at least some traction as workable, as proved by
the Microsoft design.
FWIW, the Intel SR-IOV devices all accept a hypervisor/host provided
MAC address.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 21:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205222857-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
Yeah, so? Complain to the compiler people for forcing us into that.
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
No, the whole point of the exercise was to get away from the fact that
dependent loads are special.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 22:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205234308-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 11:43:41PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > > >
> > > > [ . . . ]
> > > >
> > > > > > > and this barrier is no longer paired with anything until
> > > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > > >
> > > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > > maybe there are other, better tools now.
> > > > > >
> > > > > > There are quite a few people who say that smp_store_release() is
> > > > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > > that they are correct.
> > > > >
> > > > > OK, but smp_store_release is still not paired with anything since we
> > > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > > >
> > > > Why wouldn't you consider the smp_store_release() to be paired with
> > > > the new improved READ_ONCE()?
> > >
> > > READ_ONCE is really all over the place (some code literally replaced all
> > > memory accesses with READ/WRITE ONCE).
> > >
> > > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> > >
> > > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > > and READ_POINTER for symmetry?
> >
> > What we do in some code is to comment the pairings, allowing the other
> > side of the pairing to be easily located. Would that work for you?
>
> Yes, that's exactly what I did for now.
Very good, thank you!
Thanx, Paul
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-05 22:05 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Jakub Kicinski, Hannes Frederic Sowa, achiad shochat,
Sridhar Samudrala, Anjali Singhai Jain, Alexander Duyck,
virtualization, Achiad, Peter Waskiewicz Jr, netdev,
Shannon Nelson, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171205135226.00002b68@intel.com>
On Tue, Dec 05, 2017 at 01:52:26PM -0800, Jesse Brandeburg wrote:
> On Tue, 5 Dec 2017 21:20:07 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
> > > Then we'll have a single solution for both netvsc and virtio (and any
> > > other PV device).
> > > And we could handle the VF DMA dirt issue agnostically.
> >
> > For the record, I won't block patches adding this kist to virtio
> > on the basis that they must be generic. It's not a lot
> > of code, implementation can come first, prettify later.
>
> Thanks, based on this discussion we're going to work on improving
> virtio-net first, but some of Achiad's points are good. I don't believe
> it should block the virtio work however.
>
> In particular I'm really interested in figuring out how we can get to
> the point that virtio is able to make or implement some smart decisions
> about which NIC to pick for traffic delivery (it's own paravirt path or
> the passthorugh device path), if Achiad wants to develop the idea into
> some code, I'd be interested to review it.
>
> > But we do need to have a discussion about how devices are paired.
> > I am not sure using just MAC works. E.g. some passthrough
> > devices don't give host ability to set the MAC.
> > Are these worth worrying about?
>
> I personally don't think that will be much of a problem, if a
> certain device has that issue, can't we just have the virtio-net device
> pick up the MAC address of the passthrough device?
Then what do you do after you have migrated to another box?
The PT device there likely has a different MAC.
> As long as they match
> things should work OK. It at least is an initial way to do the
> configuration that has at least some traction as workable, as proved by
> the Microsoft design.
Yes - that design just implements what people have been doing for years
using bond so of course it's workable.
> FWIW, the Intel SR-IOV devices all accept a hypervisor/host provided
> MAC address.
For VFs you often can program the MAC through the PF, but you typically
can't do this for PFs. Or as another example consider nested virt with a
VF passed through. PF isn't there within L1 guest so can't be used to
program the mac of the VF.
Still, we can always start small and require same mac, add other ways
to address issues later as we come up with them.
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 22:09 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, Michael S. Tsirkin, fweisbec, jiangshanlai,
linux-kernel, rostedt, josh, dhowells, edumazet, netdev,
mathieu.desnoyers, oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205213644.GU7829@linux.vnet.ibm.com>
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located. Would that work for you?
I would say that that is mandatory for any memory ordering code ;-)
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 22:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205215700.GV3165@worktop.lehotels.local>
On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
>
> Yeah, so?
Oh my point was I can't just look for READ_ONCE and go
*that's the pair*. there are too many of these.
At Paul's suggestion I will document the pairing *this read once has a
barrier that is paired with that barrier*.
> Complain to the compiler people for forcing us into that.
In some cases when you end up with all accesses
going through read/write once volatile just might better.
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
>
> No, the whole point of the exercise was to get away from the fact that
> dependent loads are special.
It's a pity that dependent stores are still special.
--
MST
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Jakub Kicinski @ 2017-12-05 22:29 UTC (permalink / raw)
To: achiad shochat
Cc: Hannes Frederic Sowa, Sridhar Samudrala, Michael S. Tsirkin,
Alexander Duyck, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93Kw-GePsraaAkVZNbOwt=rDPMVjmwuU22004LHCAoSSFA@mail.gmail.com>
On Tue, 5 Dec 2017 11:59:17 +0200, achiad shochat wrote:
> >>>> I second Jacob - having a netdev of one device driver enslave a netdev
> >>>> of another device driver is an awkward a-symmetric model.
> >>>> Regardless of whether they share the same backend device.
> >>>> Only I am not sure the Linux Bond is the right choice.
> >>>> e.g one may well want to use the virtio device also when the
> >>>> pass-through device is available, e.g for multicasts, east-west
> >>>> traffic, etc.
> >>>> I'm not sure the Linux Bond fits that functionality.
> >>>> And, as I hear in this thread, it is hard to make it work out of the box.
> >>>> So I think the right thing would be to write a new dedicated module
> >>>> for this purpose.
> >
> > This part I can sort of agree with. What if we were to look at
> > providing a way to somehow advertise that the two devices were meant
> > to be boded for virtualization purposes? For now lets call it a
> > "virt-bond". Basically we could look at providing a means for virtio
> > and VF drivers to advertise that they want this sort of bond. Then it
> > would just be a matter of providing some sort of side channel to
> > indicate where you want things like multicast/broadcast/east-west
> > traffic to go.
>
> I like this approach.
+1 on a separate driver, just enslaving devices to virtio may break
existing setups. If people are bonding from user space today, if they
update their kernel it may surprise them how things get auto-mangled.
Is what Alex is suggesting a separate PV device that says "I would
like to be a bond of those two interfaces"? That would make the HV
intent explicit and kernel decisions more understandable.
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Stephen Hemminger @ 2017-12-05 22:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Hannes Frederic Sowa, achiad shochat, Sridhar Samudrala,
Michael S. Tsirkin, Alexander Duyck, virtualization,
Shannon Nelson, Achiad, Peter Waskiewicz Jr, netdev,
Singhai, Anjali, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171205142928.00f5a51a@cakuba.netronome.com>
On Tue, 5 Dec 2017 14:29:28 -0800
Jakub Kicinski <kubakici@wp.pl> wrote:
> On Tue, 5 Dec 2017 11:59:17 +0200, achiad shochat wrote:
> > >>>> I second Jacob - having a netdev of one device driver enslave a netdev
> > >>>> of another device driver is an awkward a-symmetric model.
> > >>>> Regardless of whether they share the same backend device.
> > >>>> Only I am not sure the Linux Bond is the right choice.
> > >>>> e.g one may well want to use the virtio device also when the
> > >>>> pass-through device is available, e.g for multicasts, east-west
> > >>>> traffic, etc.
> > >>>> I'm not sure the Linux Bond fits that functionality.
> > >>>> And, as I hear in this thread, it is hard to make it work out of the box.
> > >>>> So I think the right thing would be to write a new dedicated module
> > >>>> for this purpose.
> > >
> > > This part I can sort of agree with. What if we were to look at
> > > providing a way to somehow advertise that the two devices were meant
> > > to be boded for virtualization purposes? For now lets call it a
> > > "virt-bond". Basically we could look at providing a means for virtio
> > > and VF drivers to advertise that they want this sort of bond. Then it
> > > would just be a matter of providing some sort of side channel to
> > > indicate where you want things like multicast/broadcast/east-west
> > > traffic to go.
> >
> > I like this approach.
>
> +1 on a separate driver, just enslaving devices to virtio may break
> existing setups. If people are bonding from user space today, if they
> update their kernel it may surprise them how things get auto-mangled.
>
> Is what Alex is suggesting a separate PV device that says "I would
> like to be a bond of those two interfaces"? That would make the HV
> intent explicit and kernel decisions more understandable.
So far, in my experience it still works.
As long as the kernel slaving happens first, it will work.
The attempt to bond an already slaved device will fail and no scripts seem
to check the error return.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 23:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171206000541-mutt-send-email-mst@kernel.org>
On Wed, Dec 06, 2017 at 12:09:36AM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > > READ_ONCE is really all over the place (some code literally replaced all
> > > memory accesses with READ/WRITE ONCE).
> >
> > Yeah, so?
>
> Oh my point was I can't just look for READ_ONCE and go
> *that's the pair*. there are too many of these.
> At Paul's suggestion I will document the pairing *this read once has a
> barrier that is paired with that barrier*.
>
> > Complain to the compiler people for forcing us into that.
>
> In some cases when you end up with all accesses
> going through read/write once volatile just might better.
That is in fact what the jiffies counter does. But you lose READ_ONCE()'s
automatic handling of DEC Alpha when you take that approach.
> > > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > > and READ_POINTER for symmetry?
> >
> > No, the whole point of the exercise was to get away from the fact that
> > dependent loads are special.
>
> It's a pity that dependent stores are still special.
We can make READ_ONCE() not be special at zero cost on non-Alpha
systems, but both smp_wmb() and smp_store_release() are decidedly
not free of added overhead.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH] ptr_ring: add barriers
From: Jason Wang @ 2017-12-06 2:31 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: netdev, George Cherian, davem, edumazet, virtualization
In-Reply-To: <1512501990-30029-1-git-send-email-mst@redhat.com>
On 2017年12月06日 03:29, Michael S. Tsirkin wrote:
> Users of ptr_ring expect that it's safe to give the
> data structure a pointer and have it be available
> to consumers, but that actually requires an smb_wmb
> or a stronger barrier.
>
> In absence of such barriers and on architectures that reorder writes,
> consumer might read an un=initialized value from an skb pointer stored
> in the skb array. This was observed causing crashes.
>
> To fix, add memory barriers. The barrier we use is a wmb, the
> assumption being that producers do not need to read the value so we do
> not need to order these reads.
>
> Reported-by: George Cherian <george.cherian@cavium.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> George, could you pls report whether this patch fixes
> the issue for you?
>
> This seems to be needed in stable as well.
>
>
>
>
> include/linux/ptr_ring.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 37b4bb2..6866df4 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
>
> /* Note: callers invoking this in a loop must use a compiler barrier,
> * for example cpu_relax(). Callers must hold producer_lock.
> + * Callers are responsible for making sure pointer that is being queued
> + * points to a valid data.
> */
> static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
> {
> if (unlikely(!r->size) || r->queue[r->producer])
> return -ENOSPC;
>
> + /* Make sure the pointer we are storing points to a valid data. */
> + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
> + smp_wmb();
> +
> r->queue[r->producer++] = ptr;
> if (unlikely(r->producer >= r->size))
> r->producer = 0;
> @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
> if (ptr)
> __ptr_ring_discard_one(r);
>
> + /* Make sure anyone accessing data through the pointer is up to date. */
> + /* Pairs with smp_wmb in __ptr_ring_produce. */
> + smp_read_barrier_depends();
> return ptr;
> }
>
I was thinking whether or not it's better to move those to the callers.
Then we can save lots of barriers in e.g batch consuming.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] ptr_ring: add barriers
From: Michael S. Tsirkin @ 2017-12-06 2:53 UTC (permalink / raw)
To: Jason Wang
Cc: George Cherian, netdev, linux-kernel, virtualization, edumazet,
davem
In-Reply-To: <96e8cec4-37db-b41d-b02b-767a21d9efba@redhat.com>
On Wed, Dec 06, 2017 at 10:31:39AM +0800, Jason Wang wrote:
>
>
> On 2017年12月06日 03:29, Michael S. Tsirkin wrote:
> > Users of ptr_ring expect that it's safe to give the
> > data structure a pointer and have it be available
> > to consumers, but that actually requires an smb_wmb
> > or a stronger barrier.
> >
> > In absence of such barriers and on architectures that reorder writes,
> > consumer might read an un=initialized value from an skb pointer stored
> > in the skb array. This was observed causing crashes.
> >
> > To fix, add memory barriers. The barrier we use is a wmb, the
> > assumption being that producers do not need to read the value so we do
> > not need to order these reads.
> >
> > Reported-by: George Cherian <george.cherian@cavium.com>
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > George, could you pls report whether this patch fixes
> > the issue for you?
> >
> > This seems to be needed in stable as well.
> >
> >
> >
> >
> > include/linux/ptr_ring.h | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 37b4bb2..6866df4 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
> > /* Note: callers invoking this in a loop must use a compiler barrier,
> > * for example cpu_relax(). Callers must hold producer_lock.
> > + * Callers are responsible for making sure pointer that is being queued
> > + * points to a valid data.
> > */
> > static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
> > {
> > if (unlikely(!r->size) || r->queue[r->producer])
> > return -ENOSPC;
> > + /* Make sure the pointer we are storing points to a valid data. */
> > + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
> > + smp_wmb();
> > +
> > r->queue[r->producer++] = ptr;
> > if (unlikely(r->producer >= r->size))
> > r->producer = 0;
> > @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
> > if (ptr)
> > __ptr_ring_discard_one(r);
> > + /* Make sure anyone accessing data through the pointer is up to date. */
> > + /* Pairs with smp_wmb in __ptr_ring_produce. */
> > + smp_read_barrier_depends();
> > return ptr;
> > }
>
> I was thinking whether or not it's better to move those to the callers. Then
> we can save lots of barriers in e.g batch consuming.
>
> Thanks
Batch consumers only do smp_read_barrier_depends which is free on
non-alpha. I suggest we do the simple thing for stable and reserve
optimizations for later.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] ptr_ring: add barriers
From: Jason Wang @ 2017-12-06 3:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: George Cherian, netdev, linux-kernel, virtualization, edumazet,
davem
In-Reply-To: <20171206045247-mutt-send-email-mst@kernel.org>
On 2017年12月06日 10:53, Michael S. Tsirkin wrote:
> On Wed, Dec 06, 2017 at 10:31:39AM +0800, Jason Wang wrote:
>>
>> On 2017年12月06日 03:29, Michael S. Tsirkin wrote:
>>> Users of ptr_ring expect that it's safe to give the
>>> data structure a pointer and have it be available
>>> to consumers, but that actually requires an smb_wmb
>>> or a stronger barrier.
>>>
>>> In absence of such barriers and on architectures that reorder writes,
>>> consumer might read an un=initialized value from an skb pointer stored
>>> in the skb array. This was observed causing crashes.
>>>
>>> To fix, add memory barriers. The barrier we use is a wmb, the
>>> assumption being that producers do not need to read the value so we do
>>> not need to order these reads.
>>>
>>> Reported-by: George Cherian <george.cherian@cavium.com>
>>> Suggested-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>
>>> George, could you pls report whether this patch fixes
>>> the issue for you?
>>>
>>> This seems to be needed in stable as well.
>>>
>>>
>>>
>>>
>>> include/linux/ptr_ring.h | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>> index 37b4bb2..6866df4 100644
>>> --- a/include/linux/ptr_ring.h
>>> +++ b/include/linux/ptr_ring.h
>>> @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
>>> /* Note: callers invoking this in a loop must use a compiler barrier,
>>> * for example cpu_relax(). Callers must hold producer_lock.
>>> + * Callers are responsible for making sure pointer that is being queued
>>> + * points to a valid data.
>>> */
>>> static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
>>> {
>>> if (unlikely(!r->size) || r->queue[r->producer])
>>> return -ENOSPC;
>>> + /* Make sure the pointer we are storing points to a valid data. */
>>> + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
>>> + smp_wmb();
>>> +
>>> r->queue[r->producer++] = ptr;
>>> if (unlikely(r->producer >= r->size))
>>> r->producer = 0;
>>> @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
>>> if (ptr)
>>> __ptr_ring_discard_one(r);
>>> + /* Make sure anyone accessing data through the pointer is up to date. */
>>> + /* Pairs with smp_wmb in __ptr_ring_produce. */
>>> + smp_read_barrier_depends();
>>> return ptr;
>>> }
>> I was thinking whether or not it's better to move those to the callers. Then
>> we can save lots of barriers in e.g batch consuming.
>>
>> Thanks
> Batch consumers only do smp_read_barrier_depends which is free on
> non-alpha. I suggest we do the simple thing for stable and reserve
> optimizations for later.
>
Right.
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] ptr_ring: add barriers
From: George Cherian @ 2017-12-06 9:21 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: George Cherian, netdev, virtualization, edumazet, davem
In-Reply-To: <1512501990-30029-1-git-send-email-mst@redhat.com>
Hi Michael,
On 12/06/2017 12:59 AM, Michael S. Tsirkin wrote:
> Users of ptr_ring expect that it's safe to give the
> data structure a pointer and have it be available
> to consumers, but that actually requires an smb_wmb
> or a stronger barrier.
This is not the exact situation we are seeing.
Let me try to explain the situation
Affected on ARM64 platform.
1) tun_net_xmit calls skb_array_produce, which pushes the skb to the
ptr_ring, this push is protected by a producer_lock.
2)Prior to this call the tun_net_xmit calls skb_orphan which calls the
skb->destructor and sets skb->destructor and skb->sk as NULL.
2.a) These 2 writes are getting reordered
3) At the same time in the receive side (tun_ring_recv), which gets
executed in another core calls skb_array_consume which pulls the skb
from ptr ring, this pull is protected by a consumer lock.
4) eventually calling the skb->destructor (sock_wfree) with stale values.
Also note that this issue is reproducible in a long run and doesn't
happen immediately after the launch of multiple VM's (infact the
particular test cases launches 56 VM's which does iperf back and forth)
>
> In absence of such barriers and on architectures that reorder writes,
> consumer might read an un=initialized value from an skb pointer stored
> in the skb array. This was observed causing crashes.
>
> To fix, add memory barriers. The barrier we use is a wmb, the
> assumption being that producers do not need to read the value so we do
> not need to order these reads.
It is not the case that producer is reading the value, but the consumer
reading stale value. So we need to have a strict rmb in place .
>
> Reported-by: George Cherian <george.cherian@cavium.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> George, could you pls report whether this patch fixes
> the issue for you?
>
> This seems to be needed in stable as well.
>
>
>
>
> include/linux/ptr_ring.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 37b4bb2..6866df4 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
>
> /* Note: callers invoking this in a loop must use a compiler barrier,
> * for example cpu_relax(). Callers must hold producer_lock.
> + * Callers are responsible for making sure pointer that is being queued
> + * points to a valid data.
> */
> static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
> {
> if (unlikely(!r->size) || r->queue[r->producer])
> return -ENOSPC;
>
> + /* Make sure the pointer we are storing points to a valid data. */
> + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
> + smp_wmb();
> +
> r->queue[r->producer++] = ptr;
> if (unlikely(r->producer >= r->size))
> r->producer = 0;
> @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
> if (ptr)
> __ptr_ring_discard_one(r);
>
> + /* Make sure anyone accessing data through the pointer is up to date. */
> + /* Pairs with smp_wmb in __ptr_ring_produce. */
> + smp_read_barrier_depends();
> return ptr;
> }
>
>
^ permalink raw reply
* [PATCH] ptr_ring: Add barriers to fix NULL-pointer exception
From: George Cherian @ 2017-12-06 9:57 UTC (permalink / raw)
To: linux-kernel; +Cc: George Cherian, mst, netdev, virtualization, edumazet, davem
While running a multiple VM testscase with each VM running iperf
traffic between others the following kernel NULL pointer exception
was seen.
Race appears when the tun driver instance of one VM calls skb_array_produce
(from tun_net_xmit) and the the destined VM's skb_array_consume
(from tun_ring_recv), which could run concurrently on another core. Due to
which the sock_wfree gets called again from the tun_ring_recv context.
The fix is to add write/read barrier calls to be sure that we get proper
values in the tun_ring_recv context.
Crash log
[35321.580227] Unable to handle kernel NULL pointer dereference at virtual address 00000060
[35321.596720] pgd = ffff809ee552f000
[35321.603723] [00000060] *pgd=0000009f514ac003, *pud=0000009f54f7c003, *pmd=0000000000000000
[35321.620588] Internal error: Oops: 96000006 1 SMP
[35321.630461] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_4
[35321.728501] CPU: 114 PID: 5560 Comm: qemu-system-aar Not tainted 4.11.8-4k-vhe-lse+ #3
[35321.744510] Hardware name: Cavium Inc. Unknown/Unknown, BIOS 1.0 07/24/2017
[35321.758602] task: ffff80bed7fca880 task.stack: ffff80beb5128000
[35321.770600] PC is at sock_wfree+0x24/0x80
[35321.778746] LR is at skb_release_head_state+0x68/0xf8
[35321.788979] pc : [<ffff000008a772fc>] lr : [<ffff000008a79238>] pstate: 40400149
[35321.803930] sp : ffff80beb512bc30
[35321.810648] x29: ffff80beb512bc30 x28: ffff80bed7fca880
[35321.821400] x27: 000000000000004e x26: 0000000000000000
[35321.832156] x25: 000000000000000c x24: 0000000000000000
[35321.842947] x23: ffff809ece3e4900 x22: ffff80beb512be00
[35321.853729] x21: ffff000009138000 x20: 0000000000000144
[35321.864507] x19: 0000000000000000 x18: 0000000000000014
[35321.875276] x17: 0000ffff9d9689a0 x16: ffff00000828b3f8
[35321.886048] x15: 0000504d7b000000 x14: e90ab50c48680a08
[35321.896824] x13: 0101000017773f52 x12: 1080d422c00e5db6
[35321.907595] x11: 68c322bd3930cf7a x10: a8c0d07aa8c0ad16
[35321.918352] x9 : 000000001da4ed90 x8 : b50c48680a080101
[35321.929099] x7 : 000017770c521080 x6 : 000000001d6c13f2
[35321.939865] x5 : 000000001d6c13f2 x4 : 000000000000000e
[35321.950619] x3 : 000000085ff97d82 x2 : 0000000000000000
[35321.961376] x1 : ffff000008a772d8 x0 : 0000000000000500
[35321.975193] Process qemu-system-aar (pid: 5560, stack limit = 0xffff80beb5128000)
[35321.990347] Stack: (0xffff80beb512bc30 to 0xffff80beb512c000)
[35322.001982] bc20: ffff80beb512bc50 ffff000008a79238
[35322.017817] bc40: ffff809e8fd7be00 000000000000004e ffff80beb512bc70 ffff000008a79488
[35322.033651] bc60: ffff809e8fd7be00 ffff00000881307c ffff80beb512bc90 ffff000008a79678
[35322.049489] bc80: ffff809e8fd7be00 ffff80beb512be00 ffff80beb512bcb0 ffff000008812f24
[35322.065321] bca0: ffff809e8fd7be00 000000000000004e ffff80beb512bd50 ffff0000088133f0
[35322.081165] bcc0: ffff809ece3e4900 0000000000011000 ffff80beb512bdd8 ffff80beb512be00
[35322.097001] bce0: 000000001d6c13a4 0000000000000015 0000000000000124 000000000000003f
[35322.112866] bd00: ffff000008bc2000 ffff00000847b5ac 0000000000020000 ffff80be00080000
[35322.128701] bd20: 0022000000000001 ffff80bed7fc0010 ffff000008100c38 0000000000000000
[35322.144539] bd40: 0000000000000000 0000000000040b08 ffff80beb512bd80 ffff000008288f80
[35322.160395] bd60: ffff000009138000 ffff809ee7cd3500 0000000000011000 ffff80beb512beb0
[35322.176255] bd80: ffff80beb512be30 ffff00000828a224 0000000000011000 ffff809ee7cd3500
[35322.192109] bda0: 000000001d6c13a4 ffff80beb512beb0 0000000000011000 0000000000000000
[35322.207974] bdc0: 0000000000000000 000000001d6c13a4 0000000000011000 ffff809ee7cd3500
[35322.223822] bde0: 000000000000004e 0000000000000000 0000000000000000 0000000000000000
[35322.239661] be00: ffff80be00000000 000000000000004e 0000000000010fb2 ffff80beb512bdc8
[35322.255519] be20: 0000000000000001 0000000000040b08 ffff80beb512be70 ffff00000828b464
[35322.271392] be40: ffff000009138000 ffff809ee7cd3501 ffff809ee7cd3500 000000001d6c13a4
[35322.287255] be60: 0000000000011000 0000000000000015 0000000000000000 ffff0000080833f0
[35322.303090] be80: 0000000000000000 000080bef0071000 ffffffffffffffff 0000ffff9d9689cc
[35322.318951] bea0: 0000000080000000 000080bef0071000 000000000000004e 0000000000040b08
[35322.334771] bec0: 000000000000000e 000000001d6c13a4 0000000000011000 0000ffff9cc89108
[35322.350640] bee0: 0000000000000002 0000ffff9cc89000 0000ffff9cc896f0 0000000000000000
[35322.366500] bf00: 000000000000003f 000000001da4ed90 a8c0d07aa8c0ad16 68c322bd3930cf7a
[35322.382358] bf20: 1080d422c00e5db6 0101000017773f52 e90ab50c48680a08 0000504d7b000000
[35322.398209] bf40: 0000000000000000 0000ffff9d9689a0 0000000000000014 0000000000000030
[35322.414063] bf60: 000000001d6c13a4 000000001d6c0db0 000000001d6d1db0 00000000006fbbc8
[35322.429901] bf80: 0000000000011000 000000001d6ab3e8 000000001d6ab3a4 00000000007ee4c8
[35322.445751] bfa0: 0000000000000000 0000fffff363ab70 0000ffff9d9689b8 0000fffff363ab30
[35322.461588] bfc0: 0000ffff9d9689cc 0000000080000000 000000000000000e 000000000000003f
[35322.477445] bfe0: 0000000000000000 0000000000000000 ffff809ed043d758 0000000000000000
[35322.493283] Call trace:
[35322.498275] Exception stack(0xffff80beb512ba40 to 0xffff80beb512bb70)
[35322.511303] ba40: 0000000000000000 0001000000000000 ffff80beb512bc30 ffff000008a772fc
[35322.527152] ba60: 0000000040400149 0000000000000049 ffff000008bc2000 ffff80bed7fca880
[35322.542992] ba80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[35322.558863] baa0: 0000000000000000 000000001d895758 ffff80beb512bb78 0000000000000000
[35322.574702] bac0: 000000050000000b 0000000800000001 0000000a00000001 0000000b00000001
[35322.590550] bae0: 0000000e00000001 0000001800010001 ffff80beb512bbf0 0000000000040b08
[35322.606416] bb00: 0000000000000500 ffff000008a772d8 0000000000000000 000000085ff97d82
[35322.622287] bb20: 000000000000000e 000000001d6c13f2 000000001d6c13f2 000017770c521080
[35322.638144] bb40: b50c48680a080101 000000001da4ed90 a8c0d07aa8c0ad16 68c322bd3930cf7a
[35322.653992] bb60: 1080d422c00e5db6 0101000017773f52
[35322.663908] [<ffff000008a772fc>] sock_wfree+0x24/0x80
[35322.674168] [<ffff000008a79238>] skb_release_head_state+0x68/0xf8
[35322.686535] [<ffff000008a79488>] skb_release_all+0x20/0x40
[35322.697663] [<ffff000008a79678>] consume_skb+0x38/0xd8
[35322.708120] [<ffff000008812f24>] tun_do_read.part.9+0x20c/0x4f0
[35322.720149] [<ffff0000088133f0>] tun_chr_read_iter+0xc0/0xe0
[35322.731638] [<ffff000008288f80>] __vfs_read+0x100/0x160
[35322.742249] [<ffff00000828a224>] vfs_read+0x8c/0x148
[35322.752344] [<ffff00000828b464>] SyS_read+0x6c/0xd8
[35322.762263] [<ffff0000080833f0>] el0_svc_naked+0x24/0x28
[35322.773042] Code: d503201f f9400e93 b940e280 91051274 (f9403261)
Reported-by: Joseph DeVincentis <Joseph.DeVincentis@cavium.com>
Signed-off-by: George Cherian <george.cherian@cavium.com>
---
include/linux/ptr_ring.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 37b4bb2..bc3b36b 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -106,6 +106,12 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
{
if (unlikely(!r->size) || r->queue[r->producer])
return -ENOSPC;
+ /*
+ * This barrier is necessary in order to prevent race condition with
+ * with __ptr_ring_consume(). By this we make sure all the prior
+ * writes to *ptr elements are updated.
+ */
+ wmb();
r->queue[r->producer++] = ptr;
if (unlikely(r->producer >= r->size))
@@ -275,6 +281,13 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
if (ptr)
__ptr_ring_discard_one(r);
+ /*
+ * This barrier is necessary in order to prevent race condition with
+ * with __ptr_ring_produce(). Make sure all the elements of ptr is
+ * in sync with the earlier writes which was done prior to pushing
+ * it to ring
+ */
+ rmb();
return ptr;
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH RFC 0/4] crypto: engine - Permit to enqueue all async requests
From: Fabien DESSENNE @ 2017-12-06 10:59 UTC (permalink / raw)
To: Corentin Labbe, herbert@gondor.apana.org.au, Alexandre TORGUE,
arei.gonglei@huawei.com, davem@davemloft.net, jasowang@redhat.com,
mcoquelin.stm32@gmail.com, mst@redhat.com, Lionel DEBIEVE,
Benjamin GAIGNARD
Cc: virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171129084121.9385-1-clabbe.montjoie@gmail.com>
Hi Corentin,
I am fine with this proposal: it is generic enough and I have been able
to test and run the crypto engine with aead_request without changing any
single line of code.
This is what I need to be able to send the AEAD extension of the
stm32-cryp driver (successfully tested with your engine upgrade proposal).
I have also tested the stm32-hash patch.
Note that stm32-cryp (new driver applied by Herbert recently
[https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=9e054ec21ef8344345b28603fb272fe999f735db])
would also need to be converted to the new crypto engine API : this is a
trivial patch.
Thank you for your proposal, I hope that this proposal is fine for
Herbert too.
BR
Fabien
On 29/11/17 09:41, Corentin Labbe wrote:
> Hello
>
> The current crypto_engine support only ahash and ablkcipher.
> My first patch which try to add skcipher was Nacked, it will add too many functions
> and adding other algs(aead, asymetric_key) will make the situation worst.
>
> This patchset remove all algs specific stuff and now only process generic crypto_async_request.
>
> The requests handler function pointer are now moved out of struct engine and
> are now stored directly in a crypto_engine_reqctx.
>
> The original proposal of Herbert [1] cannot be done completly since the crypto_engine
> could only dequeue crypto_async_request and it is impossible to access any request_ctx
> without knowing the underlying request type.
>
> So I do something near that was requested: adding crypto_engine_reqctx in TFM context.
> Note that the current implementation expect that crypto_engine_reqctx
> is the first member of the context.
>
> The first patch convert the crypto engine with the new way,
> while the following patchs convert the 3 existing users of crypto_engine.
> Note that this split break bisection, so probably the final commit will be all merged.
>
> The 3 latest patch were compile tested only, but the first is tested successfully
> with my new sun8i-ce driver.
>
> Regards
>
> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1474434.html
>
> Corentin Labbe (4):
> crypto: engine - Permit to enqueue all async requests
> crypto: omap: convert to new crypto engine API
> crypto: virtio: convert to new crypto engine API
> crypto: stm32: convert to the new crypto engine API
>
> crypto/crypto_engine.c | 188 ++++++---------------------
> drivers/crypto/omap-aes.c | 21 ++-
> drivers/crypto/omap-aes.h | 3 +
> drivers/crypto/omap-des.c | 24 +++-
> drivers/crypto/stm32/stm32-hash.c | 22 +++-
> drivers/crypto/virtio/virtio_crypto_algs.c | 15 ++-
> drivers/crypto/virtio/virtio_crypto_common.h | 2 +-
> drivers/crypto/virtio/virtio_crypto_core.c | 3 -
> include/crypto/engine.h | 46 +++----
> 9 files changed, 122 insertions(+), 202 deletions(-)
>
^ permalink raw reply
* Re: [PATCH RFC 1/4] crypto: engine - Permit to enqueue all async requests
From: Fabien DESSENNE @ 2017-12-06 11:02 UTC (permalink / raw)
To: Corentin Labbe, herbert@gondor.apana.org.au, Alexandre TORGUE,
arei.gonglei@huawei.com, davem@davemloft.net, jasowang@redhat.com,
mcoquelin.stm32@gmail.com, mst@redhat.com
Cc: virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171129084121.9385-2-clabbe.montjoie@gmail.com>
On 29/11/17 09:41, Corentin Labbe wrote:
> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue any type of crypto_async_request.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> crypto/crypto_engine.c | 188 +++++++++++-------------------------------------
> include/crypto/engine.h | 46 +++++-------
> 2 files changed, 60 insertions(+), 174 deletions(-)
>
> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
> index 61e7c4e02fd2..f7c4c4c1f41b 100644
> --- a/crypto/crypto_engine.c
> +++ b/crypto/crypto_engine.c
> @@ -34,11 +34,10 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> bool in_kthread)
> {
> struct crypto_async_request *async_req, *backlog;
> - struct ahash_request *hreq;
> - struct ablkcipher_request *breq;
> unsigned long flags;
> bool was_busy = false;
> - int ret, rtype;
> + int ret;
> + struct crypto_engine_reqctx *enginectx;
>
> spin_lock_irqsave(&engine->queue_lock, flags);
>
> @@ -94,7 +93,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>
> spin_unlock_irqrestore(&engine->queue_lock, flags);
>
> - rtype = crypto_tfm_alg_type(engine->cur_req->tfm);
> /* Until here we get the request need to be encrypted successfully */
> if (!was_busy && engine->prepare_crypt_hardware) {
> ret = engine->prepare_crypt_hardware(engine);
> @@ -104,57 +102,31 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> }
> }
>
> - switch (rtype) {
> - case CRYPTO_ALG_TYPE_AHASH:
> - hreq = ahash_request_cast(engine->cur_req);
> - if (engine->prepare_hash_request) {
> - ret = engine->prepare_hash_request(engine, hreq);
> - if (ret) {
> - dev_err(engine->dev, "failed to prepare request: %d\n",
> - ret);
> - goto req_err;
> - }
> - engine->cur_req_prepared = true;
> - }
> - ret = engine->hash_one_request(engine, hreq);
> - if (ret) {
> - dev_err(engine->dev, "failed to hash one request from queue\n");
> - goto req_err;
> - }
> - return;
> - case CRYPTO_ALG_TYPE_ABLKCIPHER:
> - breq = ablkcipher_request_cast(engine->cur_req);
> - if (engine->prepare_cipher_request) {
> - ret = engine->prepare_cipher_request(engine, breq);
> - if (ret) {
> - dev_err(engine->dev, "failed to prepare request: %d\n",
> - ret);
> - goto req_err;
> - }
> - engine->cur_req_prepared = true;
> - }
> - ret = engine->cipher_one_request(engine, breq);
> + enginectx = crypto_tfm_ctx(async_req->tfm);
> +
> + if (enginectx->op.prepare_request) {
> + ret = enginectx->op.prepare_request(engine, async_req);
> if (ret) {
> - dev_err(engine->dev, "failed to cipher one request from queue\n");
> + dev_err(engine->dev, "failed to prepare request: %d\n",
> + ret);
> goto req_err;
> }
> - return;
> - default:
> - dev_err(engine->dev, "failed to prepare request of unknown type\n");
> - return;
> + engine->cur_req_prepared = true;
> + }
> + if (!enginectx->op.do_one_request) {
> + dev_err(engine->dev, "failed to do request\n");
> + ret = -EINVAL;
> + goto req_err;
> + }
> + ret = enginectx->op.do_one_request(engine, async_req);
> + if (ret) {
> + dev_err(engine->dev, "failed to hash one request from queue\n");
> + goto req_err;
> }
> + return;
>
> req_err:
> - switch (rtype) {
> - case CRYPTO_ALG_TYPE_AHASH:
> - hreq = ahash_request_cast(engine->cur_req);
> - crypto_finalize_hash_request(engine, hreq, ret);
> - break;
> - case CRYPTO_ALG_TYPE_ABLKCIPHER:
> - breq = ablkcipher_request_cast(engine->cur_req);
> - crypto_finalize_cipher_request(engine, breq, ret);
> - break;
> - }
> + crypto_finalize_request(engine, async_req, ret);
> return;
>
> out:
> @@ -170,59 +142,16 @@ static void crypto_pump_work(struct kthread_work *work)
> }
>
> /**
> - * crypto_transfer_cipher_request - transfer the new request into the
> - * enginequeue
> + * crypto_transfer_request - transfer the new request into the engine queue
> * @engine: the hardware engine
> * @req: the request need to be listed into the engine queue
> */
> -int crypto_transfer_cipher_request(struct crypto_engine *engine,
> - struct ablkcipher_request *req,
> - bool need_pump)
> +int crypto_transfer_request(struct crypto_engine *engine,
> + struct crypto_async_request *req, bool need_pump)
> {
> unsigned long flags;
> int ret;
>
> - spin_lock_irqsave(&engine->queue_lock, flags);
> -
> - if (!engine->running) {
> - spin_unlock_irqrestore(&engine->queue_lock, flags);
> - return -ESHUTDOWN;
> - }
> -
> - ret = ablkcipher_enqueue_request(&engine->queue, req);
> -
> - if (!engine->busy && need_pump)
> - kthread_queue_work(engine->kworker, &engine->pump_requests);
> -
> - spin_unlock_irqrestore(&engine->queue_lock, flags);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request);
> -
> -/**
> - * crypto_transfer_cipher_request_to_engine - transfer one request to list
> - * into the engine queue
> - * @engine: the hardware engine
> - * @req: the request need to be listed into the engine queue
> - */
> -int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
> - struct ablkcipher_request *req)
> -{
> - return crypto_transfer_cipher_request(engine, req, true);
> -}
> -EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request_to_engine);
> -
> -/**
> - * crypto_transfer_hash_request - transfer the new request into the
> - * enginequeue
> - * @engine: the hardware engine
> - * @req: the request need to be listed into the engine queue
> - */
> -int crypto_transfer_hash_request(struct crypto_engine *engine,
> - struct ahash_request *req, bool need_pump)
> -{
> - unsigned long flags;
> - int ret;
>
> spin_lock_irqsave(&engine->queue_lock, flags);
>
> @@ -231,7 +160,7 @@ int crypto_transfer_hash_request(struct crypto_engine *engine,
> return -ESHUTDOWN;
> }
>
> - ret = ahash_enqueue_request(&engine->queue, req);
> + ret = crypto_enqueue_request(&engine->queue, req);
>
> if (!engine->busy && need_pump)
> kthread_queue_work(engine->kworker, &engine->pump_requests);
> @@ -239,80 +168,45 @@ int crypto_transfer_hash_request(struct crypto_engine *engine,
> spin_unlock_irqrestore(&engine->queue_lock, flags);
> return ret;
> }
> -EXPORT_SYMBOL_GPL(crypto_transfer_hash_request);
> +EXPORT_SYMBOL_GPL(crypto_transfer_request);
>
> /**
> - * crypto_transfer_hash_request_to_engine - transfer one request to list
> + * crypto_transfer_request_to_engine - transfer one request to list
> * into the engine queue
> * @engine: the hardware engine
> * @req: the request need to be listed into the engine queue
> */
> -int crypto_transfer_hash_request_to_engine(struct crypto_engine *engine,
> - struct ahash_request *req)
> -{
> - return crypto_transfer_hash_request(engine, req, true);
> -}
> -EXPORT_SYMBOL_GPL(crypto_transfer_hash_request_to_engine);
> -
> -/**
> - * crypto_finalize_cipher_request - finalize one request if the request is done
> - * @engine: the hardware engine
> - * @req: the request need to be finalized
> - * @err: error number
> - */
> -void crypto_finalize_cipher_request(struct crypto_engine *engine,
> - struct ablkcipher_request *req, int err)
> +int crypto_transfer_request_to_engine(struct crypto_engine *engine,
> + struct crypto_async_request *req)
> {
> - unsigned long flags;
> - bool finalize_cur_req = false;
> - int ret;
> -
> - spin_lock_irqsave(&engine->queue_lock, flags);
> - if (engine->cur_req == &req->base)
> - finalize_cur_req = true;
> - spin_unlock_irqrestore(&engine->queue_lock, flags);
> -
> - if (finalize_cur_req) {
> - if (engine->cur_req_prepared &&
> - engine->unprepare_cipher_request) {
> - ret = engine->unprepare_cipher_request(engine, req);
> - if (ret)
> - dev_err(engine->dev, "failed to unprepare request\n");
> - }
> - spin_lock_irqsave(&engine->queue_lock, flags);
> - engine->cur_req = NULL;
> - engine->cur_req_prepared = false;
> - spin_unlock_irqrestore(&engine->queue_lock, flags);
> - }
> -
> - req->base.complete(&req->base, err);
> -
> - kthread_queue_work(engine->kworker, &engine->pump_requests);
> + return crypto_transfer_request(engine, req, true);
> }
> -EXPORT_SYMBOL_GPL(crypto_finalize_cipher_request);
> +EXPORT_SYMBOL_GPL(crypto_transfer_request_to_engine);
>
> /**
> - * crypto_finalize_hash_request - finalize one request if the request is done
> + * crypto_finalize_request - finalize one request if the request is done
> * @engine: the hardware engine
> * @req: the request need to be finalized
> * @err: error number
> */
> -void crypto_finalize_hash_request(struct crypto_engine *engine,
> - struct ahash_request *req, int err)
> +void crypto_finalize_request(struct crypto_engine *engine,
> + struct crypto_async_request *req, int err)
> {
> unsigned long flags;
> bool finalize_cur_req = false;
> int ret;
> + struct crypto_engine_reqctx *enginectx;
>
> spin_lock_irqsave(&engine->queue_lock, flags);
> - if (engine->cur_req == &req->base)
> + if (engine->cur_req == req)
> finalize_cur_req = true;
> spin_unlock_irqrestore(&engine->queue_lock, flags);
>
> if (finalize_cur_req) {
> + enginectx = crypto_tfm_ctx(req->tfm);
> if (engine->cur_req_prepared &&
> - engine->unprepare_hash_request) {
> - ret = engine->unprepare_hash_request(engine, req);
> + enginectx->op.unprepare_request) {
> + ret = enginectx->op.unprepare_request(engine, req);
> if (ret)
> dev_err(engine->dev, "failed to unprepare request\n");
> }
> @@ -322,11 +216,11 @@ void crypto_finalize_hash_request(struct crypto_engine *engine,
> spin_unlock_irqrestore(&engine->queue_lock, flags);
> }
>
> - req->base.complete(&req->base, err);
> + req->complete(req, err);
>
> kthread_queue_work(engine->kworker, &engine->pump_requests);
> }
> -EXPORT_SYMBOL_GPL(crypto_finalize_hash_request);
> +EXPORT_SYMBOL_GPL(crypto_finalize_request);
>
> /**
> * crypto_engine_start - start the hardware engine
> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
> index dd04c1699b51..2e45db45849b 100644
> --- a/include/crypto/engine.h
> +++ b/include/crypto/engine.h
> @@ -17,7 +17,6 @@
> #include <linux/kernel.h>
> #include <linux/kthread.h>
> #include <crypto/algapi.h>
> -#include <crypto/hash.h>
>
> #define ENGINE_NAME_LEN 30
> /*
> @@ -65,19 +64,6 @@ struct crypto_engine {
You also need to remove these 6 functions from the comment header of
that structure
> int (*prepare_crypt_hardware)(struct crypto_engine *engine);
> int (*unprepare_crypt_hardware)(struct crypto_engine *engine);
>
> - int (*prepare_cipher_request)(struct crypto_engine *engine,
> - struct ablkcipher_request *req);
> - int (*unprepare_cipher_request)(struct crypto_engine *engine,
> - struct ablkcipher_request *req);
> - int (*prepare_hash_request)(struct crypto_engine *engine,
> - struct ahash_request *req);
> - int (*unprepare_hash_request)(struct crypto_engine *engine,
> - struct ahash_request *req);
> - int (*cipher_one_request)(struct crypto_engine *engine,
> - struct ablkcipher_request *req);
> - int (*hash_one_request)(struct crypto_engine *engine,
> - struct ahash_request *req);
> -
> struct kthread_worker *kworker;
> struct kthread_work pump_requests;
>
> @@ -85,19 +71,25 @@ struct crypto_engine {
> struct crypto_async_request *cur_req;
> };
>
> -int crypto_transfer_cipher_request(struct crypto_engine *engine,
> - struct ablkcipher_request *req,
> - bool need_pump);
> -int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
> - struct ablkcipher_request *req);
> -int crypto_transfer_hash_request(struct crypto_engine *engine,
> - struct ahash_request *req, bool need_pump);
> -int crypto_transfer_hash_request_to_engine(struct crypto_engine *engine,
> - struct ahash_request *req);
> -void crypto_finalize_cipher_request(struct crypto_engine *engine,
> - struct ablkcipher_request *req, int err);
> -void crypto_finalize_hash_request(struct crypto_engine *engine,
> - struct ahash_request *req, int err);
> +struct crypto_engine_op {
> + int (*prepare_request)(struct crypto_engine *engine,
> + struct crypto_async_request *areq);
> + int (*unprepare_request)(struct crypto_engine *engine,
> + struct crypto_async_request *areq);
> + int (*do_one_request)(struct crypto_engine *engine,
> + struct crypto_async_request *areq);
> +};
> +
> +struct crypto_engine_reqctx {
> + struct crypto_engine_op op;
> +};
> +
> +int crypto_transfer_request(struct crypto_engine *engine,
> + struct crypto_async_request *req, bool need_pump);
> +int crypto_transfer_request_to_engine(struct crypto_engine *engine,
> + struct crypto_async_request *req);
> +void crypto_finalize_request(struct crypto_engine *engine,
> + struct crypto_async_request *req, int err);
> int crypto_engine_start(struct crypto_engine *engine);
> int crypto_engine_stop(struct crypto_engine *engine);
> struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt);
^ permalink raw reply
* Re: [PATCH RFC 4/4] crypto: stm32: convert to the new crypto engine API
From: Fabien DESSENNE @ 2017-12-06 11:03 UTC (permalink / raw)
To: Corentin Labbe, herbert@gondor.apana.org.au, Alexandre TORGUE,
arei.gonglei@huawei.com, davem@davemloft.net, jasowang@redhat.com,
mcoquelin.stm32@gmail.com, mst@redhat.com, Lionel DEBIEVE
Cc: virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171129084121.9385-5-clabbe.montjoie@gmail.com>
Hi,
On 29/11/17 09:41, Corentin Labbe wrote:
> This patch convert the driver to the new crypto engine API.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Tested-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
> drivers/crypto/stm32/stm32-hash.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
> index 4ca4a264a833..e3f9f7b04ce2 100644
> --- a/drivers/crypto/stm32/stm32-hash.c
> +++ b/drivers/crypto/stm32/stm32-hash.c
> @@ -122,6 +122,7 @@ enum stm32_hash_data_format {
> #define HASH_DMA_THRESHOLD 50
>
> struct stm32_hash_ctx {
> + struct crypto_engine_reqctx enginectx;
> struct stm32_hash_dev *hdev;
> unsigned long flags;
>
> @@ -811,7 +812,7 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err)
> rctx->flags |= HASH_FLAGS_ERRORS;
> }
>
> - crypto_finalize_hash_request(hdev->engine, req, err);
> + crypto_finalize_request(hdev->engine, &req->base, err);
> }
>
> static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
> @@ -828,15 +829,21 @@ static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
> return 0;
> }
>
> +static int stm32_hash_one_request(struct crypto_engine *engine,
> + struct crypto_async_request *areq);
> +static int stm32_hash_prepare_req(struct crypto_engine *engine,
> + struct crypto_async_request *areq);
> +
> static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
> struct ahash_request *req)
> {
> - return crypto_transfer_hash_request_to_engine(hdev->engine, req);
> + return crypto_transfer_request_to_engine(hdev->engine, &req->base);
> }
>
> static int stm32_hash_prepare_req(struct crypto_engine *engine,
> - struct ahash_request *req)
> + struct crypto_async_request *areq)
> {
> + struct ahash_request *req = ahash_request_cast(areq);
> struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
> struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
> struct stm32_hash_request_ctx *rctx;
> @@ -855,8 +862,9 @@ static int stm32_hash_prepare_req(struct crypto_engine *engine,
> }
>
> static int stm32_hash_one_request(struct crypto_engine *engine,
> - struct ahash_request *req)
> + struct crypto_async_request *areq)
> {
> + struct ahash_request *req = ahash_request_cast(areq);
> struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
> struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
> struct stm32_hash_request_ctx *rctx;
> @@ -1033,6 +1041,9 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
> if (algs_hmac_name)
> ctx->flags |= HASH_FLAGS_HMAC;
>
> + ctx->enginectx.op.do_one_request = stm32_hash_one_request;
> + ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
> + ctx->enginectx.op.unprepare_request = NULL;
> return 0;
> }
>
> @@ -1493,9 +1504,6 @@ static int stm32_hash_probe(struct platform_device *pdev)
> goto err_engine;
> }
>
> - hdev->engine->prepare_hash_request = stm32_hash_prepare_req;
> - hdev->engine->hash_one_request = stm32_hash_one_request;
> -
> ret = crypto_engine_start(hdev->engine);
> if (ret)
> goto err_engine_start;
^ permalink raw reply
* Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
From: Cornelia Huck @ 2017-12-06 11:11 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <40b3946d54c4a08b2ce3741f4d27bff99ec2f6c8.1512445595.git.zhangweiping@didichuxing.com>
On Tue, 5 Dec 2017 19:57:10 +0800
weiping zhang <zhangweiping@didichuxing.com> wrote:
> As mentioned at drivers/base/core.c:
> /*
> * NOTE: _Never_ directly free @dev after calling this function, even
> * if it returned an error! Always use put_device() to give up the
> * reference initialized in this function instead.
> */
>
> Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
> we need release @mem resource from @pdev and vm_dev->base. It make
> @pdev->vm_dev.dev.release() too complicated, so put_device just put the
> reference of register_virtio_device->device_register->device_initialize
> and release all resource in virtio_mmio_probe.
Releasing the resources when unwinding on error can work, but I think
there still are some issues (more below). This is all very tangly
code :(
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio_mmio.c | 36 ++++++++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, vm_dev);
>
> - return register_virtio_device(&vm_dev->vdev);
> + rc = register_virtio_device(&vm_dev->vdev);
> + if (rc)
> + goto put_dev;
> + return 0;
> +put_dev:
> + put_device(&vm_dev->vdev.dev);
Here you give up the extra reference from device_initialize(), which
may or may not be the last reference (since you don't know if
device_add() had already exposed the struct device to other code that
might have acquired a reference). As the device has an empty release
function, touching the device structure after that is not a real
problem, but...
> +unmap:
> + iounmap(vm_dev->base);
> +free_mem:
> + devm_release_mem_region(&pdev->dev, mem->start,
> + resource_size(mem));
> +free_vmdev:
> + devm_kfree(&pdev->dev, vm_dev);
...unconditionally freeing the device here would be a problem if other
code had acquired a reference above. (Unlikely, but we should try to
get this right.)
> + return rc;
> }
>
> static int virtio_mmio_remove(struct platform_device *pdev)
So, I think there are basically two ways of doing that:
- Move the cleanup into the currently empty release callback. Then, you
won't need to touch the remove function. The problem with that is
that you can't trigger a cleanup via put_device() if you did not call
register_virtio_device() yet.
- Move just devm_kfree() into the release function. Cleanup the
resources here, do the put_device() last thing if had you called
register_virtio_device() before and devm_kfree() if you didn't.
[Of course, I still might be missing some devm subtility, so other
comments are welcome.]
^ permalink raw reply
* Re: [PATCH] ptr_ring: add barriers
From: Michael S. Tsirkin @ 2017-12-06 12:46 UTC (permalink / raw)
To: George Cherian
Cc: George Cherian, netdev, linux-kernel, virtualization, edumazet,
davem
In-Reply-To: <7d1ce1b5-edba-b017-3131-37405f1b0c24@caviumnetworks.com>
On Wed, Dec 06, 2017 at 02:51:41PM +0530, George Cherian wrote:
> Hi Michael,
>
>
> On 12/06/2017 12:59 AM, Michael S. Tsirkin wrote:
> > Users of ptr_ring expect that it's safe to give the
> > data structure a pointer and have it be available
> > to consumers, but that actually requires an smb_wmb
> > or a stronger barrier.
> This is not the exact situation we are seeing.
Could you test the patch pls?
> Let me try to explain the situation
>
> Affected on ARM64 platform.
> 1) tun_net_xmit calls skb_array_produce, which pushes the skb to the
> ptr_ring, this push is protected by a producer_lock.
>
> 2)Prior to this call the tun_net_xmit calls skb_orphan which calls the
> skb->destructor and sets skb->destructor and skb->sk as NULL.
>
> 2.a) These 2 writes are getting reordered
>
> 3) At the same time in the receive side (tun_ring_recv), which gets executed
> in another core calls skb_array_consume which pulls the skb from ptr ring,
> this pull is protected by a consumer lock.
>
> 4) eventually calling the skb->destructor (sock_wfree) with stale values.
>
> Also note that this issue is reproducible in a long run and doesn't happen
> immediately after the launch of multiple VM's (infact the particular test
> cases launches 56 VM's which does iperf back and forth)
>
> >
> > In absence of such barriers and on architectures that reorder writes,
> > consumer might read an un=initialized value from an skb pointer stored
> > in the skb array. This was observed causing crashes.
> >
> > To fix, add memory barriers. The barrier we use is a wmb, the
> > assumption being that producers do not need to read the value so we do
> > not need to order these reads.
> It is not the case that producer is reading the value, but the consumer
> reading stale value. So we need to have a strict rmb in place .
>
> >
> > Reported-by: George Cherian <george.cherian@cavium.com>
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > George, could you pls report whether this patch fixes
> > the issue for you?
> >
> > This seems to be needed in stable as well.
> >
> >
> >
> >
> > include/linux/ptr_ring.h | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 37b4bb2..6866df4 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
> > /* Note: callers invoking this in a loop must use a compiler barrier,
> > * for example cpu_relax(). Callers must hold producer_lock.
> > + * Callers are responsible for making sure pointer that is being queued
> > + * points to a valid data.
> > */
> > static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
> > {
> > if (unlikely(!r->size) || r->queue[r->producer])
> > return -ENOSPC;
> > + /* Make sure the pointer we are storing points to a valid data. */
> > + /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
> > + smp_wmb();
> > +
> > r->queue[r->producer++] = ptr;
> > if (unlikely(r->producer >= r->size))
> > r->producer = 0;
> > @@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
> > if (ptr)
> > __ptr_ring_discard_one(r);
> > + /* Make sure anyone accessing data through the pointer is up to date. */
> > + /* Pairs with smp_wmb in __ptr_ring_produce. */
> > + smp_read_barrier_depends();
> > return ptr;
> > }
> >
^ permalink raw reply
* Re: [PATCH] ptr_ring: Add barriers to fix NULL-pointer exception
From: Michael S. Tsirkin @ 2017-12-06 13:35 UTC (permalink / raw)
To: George Cherian; +Cc: netdev, linux-kernel, virtualization, edumazet, davem
In-Reply-To: <1512554261-5030-1-git-send-email-george.cherian@cavium.com>
On Wed, Dec 06, 2017 at 09:57:41AM +0000, George Cherian wrote:
> While running a multiple VM testscase with each VM running iperf
> traffic between others the following kernel NULL pointer exception
> was seen.
>
> Race appears when the tun driver instance of one VM calls skb_array_produce
> (from tun_net_xmit) and the the destined VM's skb_array_consume
> (from tun_ring_recv), which could run concurrently on another core. Due to
> which the sock_wfree gets called again from the tun_ring_recv context.
>
> The fix is to add write/read barrier calls to be sure that we get proper
> values in the tun_ring_recv context.
>
> Crash log
> [35321.580227] Unable to handle kernel NULL pointer dereference at virtual address 00000060
> [35321.596720] pgd = ffff809ee552f000
> [35321.603723] [00000060] *pgd=0000009f514ac003, *pud=0000009f54f7c003, *pmd=0000000000000000
> [35321.620588] Internal error: Oops: 96000006 1 SMP
> [35321.630461] Modules linked in: xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_4
> [35321.728501] CPU: 114 PID: 5560 Comm: qemu-system-aar Not tainted 4.11.8-4k-vhe-lse+ #3
> [35321.744510] Hardware name: Cavium Inc. Unknown/Unknown, BIOS 1.0 07/24/2017
> [35321.758602] task: ffff80bed7fca880 task.stack: ffff80beb5128000
> [35321.770600] PC is at sock_wfree+0x24/0x80
> [35321.778746] LR is at skb_release_head_state+0x68/0xf8
> [35321.788979] pc : [<ffff000008a772fc>] lr : [<ffff000008a79238>] pstate: 40400149
> [35321.803930] sp : ffff80beb512bc30
> [35321.810648] x29: ffff80beb512bc30 x28: ffff80bed7fca880
> [35321.821400] x27: 000000000000004e x26: 0000000000000000
> [35321.832156] x25: 000000000000000c x24: 0000000000000000
> [35321.842947] x23: ffff809ece3e4900 x22: ffff80beb512be00
> [35321.853729] x21: ffff000009138000 x20: 0000000000000144
> [35321.864507] x19: 0000000000000000 x18: 0000000000000014
> [35321.875276] x17: 0000ffff9d9689a0 x16: ffff00000828b3f8
> [35321.886048] x15: 0000504d7b000000 x14: e90ab50c48680a08
> [35321.896824] x13: 0101000017773f52 x12: 1080d422c00e5db6
> [35321.907595] x11: 68c322bd3930cf7a x10: a8c0d07aa8c0ad16
> [35321.918352] x9 : 000000001da4ed90 x8 : b50c48680a080101
> [35321.929099] x7 : 000017770c521080 x6 : 000000001d6c13f2
> [35321.939865] x5 : 000000001d6c13f2 x4 : 000000000000000e
> [35321.950619] x3 : 000000085ff97d82 x2 : 0000000000000000
> [35321.961376] x1 : ffff000008a772d8 x0 : 0000000000000500
> [35321.975193] Process qemu-system-aar (pid: 5560, stack limit = 0xffff80beb5128000)
> [35321.990347] Stack: (0xffff80beb512bc30 to 0xffff80beb512c000)
> [35322.001982] bc20: ffff80beb512bc50 ffff000008a79238
> [35322.017817] bc40: ffff809e8fd7be00 000000000000004e ffff80beb512bc70 ffff000008a79488
> [35322.033651] bc60: ffff809e8fd7be00 ffff00000881307c ffff80beb512bc90 ffff000008a79678
> [35322.049489] bc80: ffff809e8fd7be00 ffff80beb512be00 ffff80beb512bcb0 ffff000008812f24
> [35322.065321] bca0: ffff809e8fd7be00 000000000000004e ffff80beb512bd50 ffff0000088133f0
> [35322.081165] bcc0: ffff809ece3e4900 0000000000011000 ffff80beb512bdd8 ffff80beb512be00
> [35322.097001] bce0: 000000001d6c13a4 0000000000000015 0000000000000124 000000000000003f
> [35322.112866] bd00: ffff000008bc2000 ffff00000847b5ac 0000000000020000 ffff80be00080000
> [35322.128701] bd20: 0022000000000001 ffff80bed7fc0010 ffff000008100c38 0000000000000000
> [35322.144539] bd40: 0000000000000000 0000000000040b08 ffff80beb512bd80 ffff000008288f80
> [35322.160395] bd60: ffff000009138000 ffff809ee7cd3500 0000000000011000 ffff80beb512beb0
> [35322.176255] bd80: ffff80beb512be30 ffff00000828a224 0000000000011000 ffff809ee7cd3500
> [35322.192109] bda0: 000000001d6c13a4 ffff80beb512beb0 0000000000011000 0000000000000000
> [35322.207974] bdc0: 0000000000000000 000000001d6c13a4 0000000000011000 ffff809ee7cd3500
> [35322.223822] bde0: 000000000000004e 0000000000000000 0000000000000000 0000000000000000
> [35322.239661] be00: ffff80be00000000 000000000000004e 0000000000010fb2 ffff80beb512bdc8
> [35322.255519] be20: 0000000000000001 0000000000040b08 ffff80beb512be70 ffff00000828b464
> [35322.271392] be40: ffff000009138000 ffff809ee7cd3501 ffff809ee7cd3500 000000001d6c13a4
> [35322.287255] be60: 0000000000011000 0000000000000015 0000000000000000 ffff0000080833f0
> [35322.303090] be80: 0000000000000000 000080bef0071000 ffffffffffffffff 0000ffff9d9689cc
> [35322.318951] bea0: 0000000080000000 000080bef0071000 000000000000004e 0000000000040b08
> [35322.334771] bec0: 000000000000000e 000000001d6c13a4 0000000000011000 0000ffff9cc89108
> [35322.350640] bee0: 0000000000000002 0000ffff9cc89000 0000ffff9cc896f0 0000000000000000
> [35322.366500] bf00: 000000000000003f 000000001da4ed90 a8c0d07aa8c0ad16 68c322bd3930cf7a
> [35322.382358] bf20: 1080d422c00e5db6 0101000017773f52 e90ab50c48680a08 0000504d7b000000
> [35322.398209] bf40: 0000000000000000 0000ffff9d9689a0 0000000000000014 0000000000000030
> [35322.414063] bf60: 000000001d6c13a4 000000001d6c0db0 000000001d6d1db0 00000000006fbbc8
> [35322.429901] bf80: 0000000000011000 000000001d6ab3e8 000000001d6ab3a4 00000000007ee4c8
> [35322.445751] bfa0: 0000000000000000 0000fffff363ab70 0000ffff9d9689b8 0000fffff363ab30
> [35322.461588] bfc0: 0000ffff9d9689cc 0000000080000000 000000000000000e 000000000000003f
> [35322.477445] bfe0: 0000000000000000 0000000000000000 ffff809ed043d758 0000000000000000
> [35322.493283] Call trace:
> [35322.498275] Exception stack(0xffff80beb512ba40 to 0xffff80beb512bb70)
> [35322.511303] ba40: 0000000000000000 0001000000000000 ffff80beb512bc30 ffff000008a772fc
> [35322.527152] ba60: 0000000040400149 0000000000000049 ffff000008bc2000 ffff80bed7fca880
> [35322.542992] ba80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [35322.558863] baa0: 0000000000000000 000000001d895758 ffff80beb512bb78 0000000000000000
> [35322.574702] bac0: 000000050000000b 0000000800000001 0000000a00000001 0000000b00000001
> [35322.590550] bae0: 0000000e00000001 0000001800010001 ffff80beb512bbf0 0000000000040b08
> [35322.606416] bb00: 0000000000000500 ffff000008a772d8 0000000000000000 000000085ff97d82
> [35322.622287] bb20: 000000000000000e 000000001d6c13f2 000000001d6c13f2 000017770c521080
> [35322.638144] bb40: b50c48680a080101 000000001da4ed90 a8c0d07aa8c0ad16 68c322bd3930cf7a
> [35322.653992] bb60: 1080d422c00e5db6 0101000017773f52
> [35322.663908] [<ffff000008a772fc>] sock_wfree+0x24/0x80
> [35322.674168] [<ffff000008a79238>] skb_release_head_state+0x68/0xf8
> [35322.686535] [<ffff000008a79488>] skb_release_all+0x20/0x40
> [35322.697663] [<ffff000008a79678>] consume_skb+0x38/0xd8
> [35322.708120] [<ffff000008812f24>] tun_do_read.part.9+0x20c/0x4f0
> [35322.720149] [<ffff0000088133f0>] tun_chr_read_iter+0xc0/0xe0
> [35322.731638] [<ffff000008288f80>] __vfs_read+0x100/0x160
> [35322.742249] [<ffff00000828a224>] vfs_read+0x8c/0x148
> [35322.752344] [<ffff00000828b464>] SyS_read+0x6c/0xd8
> [35322.762263] [<ffff0000080833f0>] el0_svc_naked+0x24/0x28
> [35322.773042] Code: d503201f f9400e93 b940e280 91051274 (f9403261)
>
> Reported-by: Joseph DeVincentis <Joseph.DeVincentis@cavium.com>
> Signed-off-by: George Cherian <george.cherian@cavium.com>
> ---
> include/linux/ptr_ring.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> index 37b4bb2..bc3b36b 100644
> --- a/include/linux/ptr_ring.h
> +++ b/include/linux/ptr_ring.h
> @@ -106,6 +106,12 @@ static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
> {
> if (unlikely(!r->size) || r->queue[r->producer])
> return -ENOSPC;
> + /*
> + * This barrier is necessary in order to prevent race condition with
> + * with __ptr_ring_consume(). By this we make sure all the prior
> + * writes to *ptr elements are updated.
> + */
> + wmb();
>
> r->queue[r->producer++] = ptr;
> if (unlikely(r->producer >= r->size))
> @@ -275,6 +281,13 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
> if (ptr)
> __ptr_ring_discard_one(r);
>
> + /*
> + * This barrier is necessary in order to prevent race condition with
> + * with __ptr_ring_produce(). Make sure all the elements of ptr is
> + * in sync with the earlier writes which was done prior to pushing
> + * it to ring
> + */
> + rmb();
> return ptr;
> }
You are trying to synchronise two CPUs so non-smp barriers make no
sense. wmb/rmb are for synchronising with MMIO.
> --
> 2.1.4
^ permalink raw reply
* Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-06 13:53 UTC (permalink / raw)
To: Cornelia Huck; +Cc: mst, weiping zhang, virtualization
In-Reply-To: <20171206121153.5f075096.cohuck@redhat.com>
2017-12-06 19:11 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Tue, 5 Dec 2017 19:57:10 +0800
> weiping zhang <zhangweiping@didichuxing.com> wrote:
>
>> As mentioned at drivers/base/core.c:
>> /*
>> * NOTE: _Never_ directly free @dev after calling this function, even
>> * if it returned an error! Always use put_device() to give up the
>> * reference initialized in this function instead.
>> */
>>
>> Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
>> we need release @mem resource from @pdev and vm_dev->base. It make
>> @pdev->vm_dev.dev.release() too complicated, so put_device just put the
>> reference of register_virtio_device->device_register->device_initialize
>> and release all resource in virtio_mmio_probe.
>
> Releasing the resources when unwinding on error can work, but I think
> there still are some issues (more below). This is all very tangly
> code :(
>
>>
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> ---
>> drivers/virtio/virtio_mmio.c | 36 ++++++++++++++++++++++++++++--------
>> 1 file changed, 28 insertions(+), 8 deletions(-)
>>
>
>> @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, vm_dev);
>>
>> - return register_virtio_device(&vm_dev->vdev);
>> + rc = register_virtio_device(&vm_dev->vdev);
>> + if (rc)
>> + goto put_dev;
>> + return 0;
>> +put_dev:
>> + put_device(&vm_dev->vdev.dev);
>
> Here you give up the extra reference from device_initialize(), which
> may or may not be the last reference (since you don't know if
> device_add() had already exposed the struct device to other code that
> might have acquired a reference). As the device has an empty release
> function, touching the device structure after that is not a real
> problem, but...
>
>> +unmap:
>> + iounmap(vm_dev->base);
>> +free_mem:
>> + devm_release_mem_region(&pdev->dev, mem->start,
>> + resource_size(mem));
>> +free_vmdev:
>> + devm_kfree(&pdev->dev, vm_dev);
>
> ...unconditionally freeing the device here would be a problem if other
> code had acquired a reference above. (Unlikely, but we should try to
> get this right.)
>
that's true, so we don't free it until it's refer count decrease to 0 and
->release called.
>> + return rc;
>> }
>>
>> static int virtio_mmio_remove(struct platform_device *pdev)
>
> So, I think there are basically two ways of doing that:
> - Move the cleanup into the currently empty release callback. Then, you
> won't need to touch the remove function. The problem with that is
> that you can't trigger a cleanup via put_device() if you did not call
> register_virtio_device() yet.
> - Move just devm_kfree() into the release function. Cleanup the
> resources here, do the put_device() last thing if had you called
> register_virtio_device() before and devm_kfree() if you didn't.
>
I prefer go second way. I'll send v3 later.
> [Of course, I still might be missing some devm subtility, so other
> comments are welcome.]
--
Thanks very much
weiping
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH v3 0/2] Add cleanup for virtio_mmio driver
From: weiping zhang @ 2017-12-06 13:58 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
this patchset try to add cleanup for virtio_mmio driver, include
virtio_mmio_probe and virtio_mmio_remove
weiping zhang (2):
virtio_mmio: add cleanup for virtio_mmio_probe
virtio_mmio: add cleanup for virtio_mmio_remove
drivers/virtio/virtio_mmio.c | 57 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 10 deletions(-)
--
2.9.4
^ permalink raw reply
* [PATCH v3 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-06 13:59 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1512568542.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
so we don't free vm_dev until vm_dev.dev.release be called.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 51 +++++++++++++++++++++++++++++++++++---------
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..ec40104 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -493,7 +493,16 @@ static const struct virtio_config_ops virtio_mmio_config_ops = {
};
-static void virtio_mmio_release_dev_empty(struct device *_d) {}
+static void virtio_mmio_release_dev(struct device *_d)
+{
+ struct virtio_device *vdev =
+ container_of(_d, struct virtio_device, dev);
+ struct virtio_mmio_device *vm_dev =
+ container_of(vdev, struct virtio_mmio_device, vdev);
+ struct platform_device *pdev = vm_dev->pdev;
+
+ devm_kfree(&pdev->dev, vm_dev);
+}
/* Platform device */
@@ -513,25 +522,30 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = &pdev->dev;
- vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
+ vm_dev->vdev.dev.release = virtio_mmio_release_dev;
vm_dev->vdev.config = &virtio_mmio_config_ops;
vm_dev->pdev = pdev;
INIT_LIST_HEAD(&vm_dev->virtqueues);
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +553,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +563,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +589,22 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(&vm_dev->vdev);
+ rc = register_virtio_device(&vm_dev->vdev);
+ if (rc) {
+ iounmap(vm_dev->base);
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+ put_device(&vm_dev->vdev.dev);
+ }
+ return rc;
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(&pdev->dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
^ permalink raw reply related
* [PATCH v3 2/2] virtio_mmio: add cleanup for virtio_mmio_remove
From: weiping zhang @ 2017-12-06 13:59 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1512568542.git.zhangweiping@didichuxing.com>
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index ec40104..a9192fe 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -610,7 +610,13 @@ static int virtio_mmio_probe(struct platform_device *pdev)
static int virtio_mmio_remove(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ iounmap(vm_dev->base);
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mem)
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
unregister_virtio_device(&vm_dev->vdev);
return 0;
--
2.9.4
^ permalink raw reply related
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