diff mbox series

[1/1] vhost: Protect the virtqueue from being cleared whilst still in use

Message ID 20220307191757.3177139-1-lee.jones@linaro.org (mailing list archive)
State New, archived
Headers show
Series [1/1] vhost: Protect the virtqueue from being cleared whilst still in use | expand

Commit Message

Lee Jones March 7, 2022, 7:17 p.m. UTC
vhost_vsock_handle_tx_kick() already holds the mutex during its call
to vhost_get_vq_desc().  All we have to do here is take the same lock
during virtqueue clean-up and we mitigate the reported issues.

Also WARN() as a precautionary measure.  The purpose of this is to
capture possible future race conditions which may pop up over time.

Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00

Cc: <stable@vger.kernel.org>
Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/vhost/vhost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Greg Kroah-Hartman March 7, 2022, 7:33 p.m. UTC | #1
On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> vhost_vsock_handle_tx_kick() already holds the mutex during its call
> to vhost_get_vq_desc().  All we have to do here is take the same lock
> during virtqueue clean-up and we mitigate the reported issues.
> 
> Also WARN() as a precautionary measure.  The purpose of this is to
> capture possible future race conditions which may pop up over time.
> 
> Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/vhost/vhost.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 59edb5a1ffe28..ef7e371e3e649 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>  	int i;
>  
>  	for (i = 0; i < dev->nvqs; ++i) {
> +		/* No workers should run here by design. However, races have
> +		 * previously occurred where drivers have been unable to flush
> +		 * all work properly prior to clean-up.  Without a successful
> +		 * flush the guest will malfunction, but avoiding host memory
> +		 * corruption in those cases does seem preferable.
> +		 */
> +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));

So you are trading one syzbot triggered issue for another one in the
future?  :)

If this ever can happen, handle it, but don't log it with a WARN_ON() as
that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
you want that to happen?

And what happens if the mutex is locked _RIGHT_ after you checked it?
You still have a race...

thanks,

greg k-h
Michael S. Tsirkin March 7, 2022, 10:37 p.m. UTC | #2
On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> vhost_vsock_handle_tx_kick() already holds the mutex during its call
> to vhost_get_vq_desc().  All we have to do here is take the same lock
> during virtqueue clean-up and we mitigate the reported issues.

Pls just basically copy the code comment here. this is just confuses.

> Also WARN() as a precautionary measure.  The purpose of this is to
> capture possible future race conditions which may pop up over time.
> 
> Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00

And this is a bug we already fixed, right?

> Cc: <stable@vger.kernel.org>
> Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com

not really applicable anymore ...

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/vhost/vhost.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 59edb5a1ffe28..ef7e371e3e649 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>  	int i;
>  
>  	for (i = 0; i < dev->nvqs; ++i) {
> +		/* No workers should run here by design. However, races have
> +		 * previously occurred where drivers have been unable to flush
> +		 * all work properly prior to clean-up.  Without a successful
> +		 * flush the guest will malfunction, but avoiding host memory
> +		 * corruption in those cases does seem preferable.
> +		 */
> +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> +
> +		mutex_lock(&dev->vqs[i]->mutex);
>  		if (dev->vqs[i]->error_ctx)
>  			eventfd_ctx_put(dev->vqs[i]->error_ctx);
>  		if (dev->vqs[i]->kick)
> @@ -700,6 +709,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>  		if (dev->vqs[i]->call_ctx.ctx)
>  			eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
>  		vhost_vq_reset(dev, dev->vqs[i]);
> +		mutex_unlock(&dev->vqs[i]->mutex);
>  	}
>  	vhost_dev_free_iovecs(dev);
>  	if (dev->log_ctx)
> -- 
> 2.35.1.616.g0bdcbb4464-goog
Michael S. Tsirkin March 7, 2022, 10:39 p.m. UTC | #3
On Mon, Mar 07, 2022 at 08:33:27PM +0100, Greg KH wrote:
> On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > during virtqueue clean-up and we mitigate the reported issues.
> > 
> > Also WARN() as a precautionary measure.  The purpose of this is to
> > capture possible future race conditions which may pop up over time.
> > 
> > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/vhost/vhost.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe28..ef7e371e3e649 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> >  	int i;
> >  
> >  	for (i = 0; i < dev->nvqs; ++i) {
> > +		/* No workers should run here by design. However, races have
> > +		 * previously occurred where drivers have been unable to flush
> > +		 * all work properly prior to clean-up.  Without a successful
> > +		 * flush the guest will malfunction, but avoiding host memory
> > +		 * corruption in those cases does seem preferable.
> > +		 */
> > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> 
> So you are trading one syzbot triggered issue for another one in the
> future?  :)
> 
> If this ever can happen, handle it, but don't log it with a WARN_ON() as
> that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> you want that to happen?
> 
> And what happens if the mutex is locked _RIGHT_ after you checked it?
> You still have a race...
> 
> thanks,
> 
> greg k-h

Well it's a symptom of a kernel bug. I guess people with panic on
warn are not worried about DOS and more worried about integrity
and security ... am I right?
Jason Wang March 8, 2022, 6:15 a.m. UTC | #4
On Tue, Mar 8, 2022 at 3:18 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> vhost_vsock_handle_tx_kick() already holds the mutex during its call
> to vhost_get_vq_desc().  All we have to do here is take the same lock
> during virtqueue clean-up and we mitigate the reported issues.
>
> Also WARN() as a precautionary measure.  The purpose of this is to
> capture possible future race conditions which may pop up over time.
>
> Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
>
> Cc: <stable@vger.kernel.org>
> Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/vhost/vhost.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 59edb5a1ffe28..ef7e371e3e649 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>         int i;
>
>         for (i = 0; i < dev->nvqs; ++i) {
> +               /* No workers should run here by design. However, races have
> +                * previously occurred where drivers have been unable to flush
> +                * all work properly prior to clean-up.  Without a successful
> +                * flush the guest will malfunction, but avoiding host memory
> +                * corruption in those cases does seem preferable.
> +                */
> +               WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> +

I don't get how this can help, the mutex could be grabbed in the
middle of the above and below line.

> +               mutex_lock(&dev->vqs[i]->mutex);
>                 if (dev->vqs[i]->error_ctx)
>                         eventfd_ctx_put(dev->vqs[i]->error_ctx);
>                 if (dev->vqs[i]->kick)
> @@ -700,6 +709,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
>                 if (dev->vqs[i]->call_ctx.ctx)
>                         eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
>                 vhost_vq_reset(dev, dev->vqs[i]);
> +               mutex_unlock(&dev->vqs[i]->mutex);
>         }

I'm not sure it's correct to assume some behaviour of a buggy device.
For the device mutex, we use that to protect more than just err/call
and vq.

Thanks

>         vhost_dev_free_iovecs(dev);
>         if (dev->log_ctx)
> --
> 2.35.1.616.g0bdcbb4464-goog
>
Lee Jones March 8, 2022, 8:01 a.m. UTC | #5
On Mon, 07 Mar 2022, Michael S. Tsirkin wrote:

> On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > during virtqueue clean-up and we mitigate the reported issues.
> 
> Pls just basically copy the code comment here. this is just confuses.
> 
> > Also WARN() as a precautionary measure.  The purpose of this is to
> > capture possible future race conditions which may pop up over time.
> > 
> > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> 
> And this is a bug we already fixed, right?

Well, this was the bug I set out to fix.

I didn't know your patch was in flight at the time.

> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> 
> not really applicable anymore ...

I can remove these if it helps.
Lee Jones March 8, 2022, 8:08 a.m. UTC | #6
On Tue, 08 Mar 2022, Jason Wang wrote:

> On Tue, Mar 8, 2022 at 3:18 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > during virtqueue clean-up and we mitigate the reported issues.
> >
> > Also WARN() as a precautionary measure.  The purpose of this is to
> > capture possible future race conditions which may pop up over time.
> >
> > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> >
> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/vhost/vhost.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe28..ef7e371e3e649 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> >         int i;
> >
> >         for (i = 0; i < dev->nvqs; ++i) {
> > +               /* No workers should run here by design. However, races have
> > +                * previously occurred where drivers have been unable to flush
> > +                * all work properly prior to clean-up.  Without a successful
> > +                * flush the guest will malfunction, but avoiding host memory
> > +                * corruption in those cases does seem preferable.
> > +                */
> > +               WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > +
> 
> I don't get how this can help, the mutex could be grabbed in the
> middle of the above and below line.

The worst that happens in this slim scenario is we miss a warning.
The mutexes below will still function as expected and prevent possible
memory corruption.

> > +               mutex_lock(&dev->vqs[i]->mutex);
> >                 if (dev->vqs[i]->error_ctx)
> >                         eventfd_ctx_put(dev->vqs[i]->error_ctx);
> >                 if (dev->vqs[i]->kick)
> > @@ -700,6 +709,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> >                 if (dev->vqs[i]->call_ctx.ctx)
> >                         eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
> >                 vhost_vq_reset(dev, dev->vqs[i]);
> > +               mutex_unlock(&dev->vqs[i]->mutex);
> >         }
> 
> I'm not sure it's correct to assume some behaviour of a buggy device.
> For the device mutex, we use that to protect more than just err/call
> and vq.

When I authored this, I did so as *the* fix.  However, since the cause
of today's crash has now been patched, this has become a belt and
braces solution.  Michael's addition of the WARN() also has the
benefit of providing us with an early warning system for future
breakages.  Personally, I think it's kinda neat.
Lee Jones March 8, 2022, 8:10 a.m. UTC | #7
On Mon, 07 Mar 2022, Greg KH wrote:

> On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > during virtqueue clean-up and we mitigate the reported issues.
> > 
> > Also WARN() as a precautionary measure.  The purpose of this is to
> > capture possible future race conditions which may pop up over time.
> > 
> > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > 
> > Cc: <stable@vger.kernel.org>
> > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/vhost/vhost.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe28..ef7e371e3e649 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> >  	int i;
> >  
> >  	for (i = 0; i < dev->nvqs; ++i) {
> > +		/* No workers should run here by design. However, races have
> > +		 * previously occurred where drivers have been unable to flush
> > +		 * all work properly prior to clean-up.  Without a successful
> > +		 * flush the guest will malfunction, but avoiding host memory
> > +		 * corruption in those cases does seem preferable.
> > +		 */
> > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> 
> So you are trading one syzbot triggered issue for another one in the
> future?  :)
> 
> If this ever can happen, handle it, but don't log it with a WARN_ON() as
> that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> you want that to happen?

No, Syzbot doesn't report warnings, only BUGs and memory corruption.

> And what happens if the mutex is locked _RIGHT_ after you checked it?
> You still have a race...

No, we miss a warning that one time.  Memory is still protected.
Lee Jones March 8, 2022, 8:11 a.m. UTC | #8
On Tue, 08 Mar 2022, Lee Jones wrote:

> On Mon, 07 Mar 2022, Greg KH wrote:
> 
> > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > during virtqueue clean-up and we mitigate the reported issues.
> > > 
> > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > capture possible future race conditions which may pop up over time.
> > > 
> > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/vhost/vhost.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > >  	int i;
> > >  
> > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > +		/* No workers should run here by design. However, races have
> > > +		 * previously occurred where drivers have been unable to flush
> > > +		 * all work properly prior to clean-up.  Without a successful
> > > +		 * flush the guest will malfunction, but avoiding host memory
> > > +		 * corruption in those cases does seem preferable.
> > > +		 */
> > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > 
> > So you are trading one syzbot triggered issue for another one in the
> > future?  :)
> > 
> > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > you want that to happen?
> 
> No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> 
> > And what happens if the mutex is locked _RIGHT_ after you checked it?
> > You still have a race...
> 
> No, we miss a warning that one time.  Memory is still protected.

I didn't mean those "no"s to sound so negative, sorry. :)
Greg Kroah-Hartman March 8, 2022, 8:57 a.m. UTC | #9
On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> On Mon, 07 Mar 2022, Greg KH wrote:
> 
> > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > during virtqueue clean-up and we mitigate the reported issues.
> > > 
> > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > capture possible future race conditions which may pop up over time.
> > > 
> > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/vhost/vhost.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > >  	int i;
> > >  
> > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > +		/* No workers should run here by design. However, races have
> > > +		 * previously occurred where drivers have been unable to flush
> > > +		 * all work properly prior to clean-up.  Without a successful
> > > +		 * flush the guest will malfunction, but avoiding host memory
> > > +		 * corruption in those cases does seem preferable.
> > > +		 */
> > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > 
> > So you are trading one syzbot triggered issue for another one in the
> > future?  :)
> > 
> > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > you want that to happen?
> 
> No, Syzbot doesn't report warnings, only BUGs and memory corruption.

Has it changed?  Last I looked, it did trigger on WARN_* calls, which
has resulted in a huge number of kernel fixes because of that.

> > And what happens if the mutex is locked _RIGHT_ after you checked it?
> > You still have a race...
> 
> No, we miss a warning that one time.  Memory is still protected.

Then don't warn on something that doesn't matter.  This line can be
dropped as there's nothing anyone can do about it, right?

thanks,

greg k-h
Lee Jones March 8, 2022, 9:15 a.m. UTC | #10
On Tue, 08 Mar 2022, Greg KH wrote:

> On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > On Mon, 07 Mar 2022, Greg KH wrote:
> > 
> > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > 
> > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > capture possible future race conditions which may pop up over time.
> > > > 
> > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > 
> > > > Cc: <stable@vger.kernel.org>
> > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > >  	int i;
> > > >  
> > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > +		/* No workers should run here by design. However, races have
> > > > +		 * previously occurred where drivers have been unable to flush
> > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > +		 * corruption in those cases does seem preferable.
> > > > +		 */
> > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > 
> > > So you are trading one syzbot triggered issue for another one in the
> > > future?  :)
> > > 
> > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > you want that to happen?
> > 
> > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> 
> Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> has resulted in a huge number of kernel fixes because of that.

Everything is customisable in syzkaller, so maybe there are specific
builds which panic_on_warn enabled, but none that I'm involved with
do.

Here follows a topical example.  The report above in the Link: tag
comes with a crashlog [0].  In there you can see the WARN() at the
bottom of vhost_dev_cleanup() trigger many times due to a populated
(non-flushed) worker list, before finally tripping the BUG() which
triggers the report:

[0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000

> > > And what happens if the mutex is locked _RIGHT_ after you checked it?
> > > You still have a race...
> > 
> > No, we miss a warning that one time.  Memory is still protected.
> 
> Then don't warn on something that doesn't matter.  This line can be
> dropped as there's nothing anyone can do about it, right?

You'll have to take that point up with Michael.
Greg Kroah-Hartman March 8, 2022, 9:57 a.m. UTC | #11
On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> On Tue, 08 Mar 2022, Greg KH wrote:
> 
> > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > 
> > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > 
> > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > capture possible future race conditions which may pop up over time.
> > > > > 
> > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > 
> > > > > Cc: <stable@vger.kernel.org>
> > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > --- a/drivers/vhost/vhost.c
> > > > > +++ b/drivers/vhost/vhost.c
> > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > >  	int i;
> > > > >  
> > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > +		/* No workers should run here by design. However, races have
> > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > +		 * corruption in those cases does seem preferable.
> > > > > +		 */
> > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > 
> > > > So you are trading one syzbot triggered issue for another one in the
> > > > future?  :)
> > > > 
> > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > you want that to happen?
> > > 
> > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > 
> > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > has resulted in a huge number of kernel fixes because of that.
> 
> Everything is customisable in syzkaller, so maybe there are specific
> builds which panic_on_warn enabled, but none that I'm involved with
> do.

Many systems run with panic-on-warn (i.e. the cloud), as they want to
drop a box and restart it if anything goes wrong.

That's why syzbot reports on WARN_* calls.  They should never be
reachable by userspace actions.

> Here follows a topical example.  The report above in the Link: tag
> comes with a crashlog [0].  In there you can see the WARN() at the
> bottom of vhost_dev_cleanup() trigger many times due to a populated
> (non-flushed) worker list, before finally tripping the BUG() which
> triggers the report:
> 
> [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000

Ok, so both happens here.  But don't add a warning for something that
can't happen.  Just handle it and move on.  It looks like you are
handling it in this code, so please drop the WARN_ON().

thanks,

greg k-h
Lee Jones March 8, 2022, 10:08 a.m. UTC | #12
On Tue, 08 Mar 2022, Greg KH wrote:

> On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > On Tue, 08 Mar 2022, Greg KH wrote:
> > 
> > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > 
> > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > 
> > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > capture possible future race conditions which may pop up over time.
> > > > > > 
> > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > 
> > > > > > Cc: <stable@vger.kernel.org>
> > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > --- a/drivers/vhost/vhost.c
> > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > >  	int i;
> > > > > >  
> > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > +		/* No workers should run here by design. However, races have
> > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > +		 */
> > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > 
> > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > future?  :)
> > > > > 
> > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > you want that to happen?
> > > > 
> > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > 
> > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > has resulted in a huge number of kernel fixes because of that.
> > 
> > Everything is customisable in syzkaller, so maybe there are specific
> > builds which panic_on_warn enabled, but none that I'm involved with
> > do.
> 
> Many systems run with panic-on-warn (i.e. the cloud), as they want to
> drop a box and restart it if anything goes wrong.
> 
> That's why syzbot reports on WARN_* calls.  They should never be
> reachable by userspace actions.
> 
> > Here follows a topical example.  The report above in the Link: tag
> > comes with a crashlog [0].  In there you can see the WARN() at the
> > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > (non-flushed) worker list, before finally tripping the BUG() which
> > triggers the report:
> > 
> > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> 
> Ok, so both happens here.  But don't add a warning for something that
> can't happen.  Just handle it and move on.  It looks like you are
> handling it in this code, so please drop the WARN_ON().

Happy to oblige.

Let's give Micheal a chance to speak, then I'll fix-up if he agrees.
Michael S. Tsirkin March 8, 2022, 10:55 a.m. UTC | #13
On Tue, Mar 08, 2022 at 10:57:42AM +0100, Greg KH wrote:
> On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > On Tue, 08 Mar 2022, Greg KH wrote:
> > 
> > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > 
> > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > 
> > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > capture possible future race conditions which may pop up over time.
> > > > > > 
> > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > 
> > > > > > Cc: <stable@vger.kernel.org>
> > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > ---
> > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > --- a/drivers/vhost/vhost.c
> > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > >  	int i;
> > > > > >  
> > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > +		/* No workers should run here by design. However, races have
> > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > +		 */
> > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > 
> > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > future?  :)
> > > > > 
> > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > you want that to happen?
> > > > 
> > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > 
> > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > has resulted in a huge number of kernel fixes because of that.
> > 
> > Everything is customisable in syzkaller, so maybe there are specific
> > builds which panic_on_warn enabled, but none that I'm involved with
> > do.
> 
> Many systems run with panic-on-warn (i.e. the cloud), as they want to
> drop a box and restart it if anything goes wrong.
> 
> That's why syzbot reports on WARN_* calls.  They should never be
> reachable by userspace actions.
> 
> > Here follows a topical example.  The report above in the Link: tag
> > comes with a crashlog [0].  In there you can see the WARN() at the
> > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > (non-flushed) worker list, before finally tripping the BUG() which
> > triggers the report:
> > 
> > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> 
> Ok, so both happens here.  But don't add a warning for something that
> can't happen.  Just handle it and move on.  It looks like you are
> handling it in this code, so please drop the WARN_ON().
> 
> thanks,
> 
> greg k-h

Hmm. Well this will mean if we ever reintroduce the bug then
syzkaller will not catch it for us :( And the bug is there,
it just results in a hard to reproduce error for userspace.

Not sure what to do here. Export panic_on_warn flag to modules
and check it here?
Michael S. Tsirkin March 8, 2022, 11:05 a.m. UTC | #14
On Tue, Mar 08, 2022 at 09:57:57AM +0100, Greg KH wrote:
> > > And what happens if the mutex is locked _RIGHT_ after you checked it?
> > > You still have a race...
> > 
> > No, we miss a warning that one time.  Memory is still protected.
> 
> Then don't warn on something that doesn't matter.  This line can be
> dropped as there's nothing anyone can do about it, right?

I mean, the reason I wanted the warning is because there's a kernel
bug, and it will break userspace. warning is just telling us this.
is the bug reacheable from userspace? if we knew that we won't
need the lock ...
Michael S. Tsirkin March 8, 2022, 11:06 a.m. UTC | #15
On Tue, Mar 08, 2022 at 08:08:25AM +0000, Lee Jones wrote:
> On Tue, 08 Mar 2022, Jason Wang wrote:
> 
> > On Tue, Mar 8, 2022 at 3:18 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > during virtqueue clean-up and we mitigate the reported issues.
> > >
> > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > capture possible future race conditions which may pop up over time.
> > >
> > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > >
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/vhost/vhost.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > >         int i;
> > >
> > >         for (i = 0; i < dev->nvqs; ++i) {
> > > +               /* No workers should run here by design. However, races have
> > > +                * previously occurred where drivers have been unable to flush
> > > +                * all work properly prior to clean-up.  Without a successful
> > > +                * flush the guest will malfunction, but avoiding host memory
> > > +                * corruption in those cases does seem preferable.
> > > +                */
> > > +               WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > +
> > 
> > I don't get how this can help, the mutex could be grabbed in the
> > middle of the above and below line.
> 
> The worst that happens in this slim scenario is we miss a warning.
> The mutexes below will still function as expected and prevent possible
> memory corruption.

maybe. or maybe corruption already happened and this is the
fallout.

> > > +               mutex_lock(&dev->vqs[i]->mutex);
> > >                 if (dev->vqs[i]->error_ctx)
> > >                         eventfd_ctx_put(dev->vqs[i]->error_ctx);
> > >                 if (dev->vqs[i]->kick)
> > > @@ -700,6 +709,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > >                 if (dev->vqs[i]->call_ctx.ctx)
> > >                         eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
> > >                 vhost_vq_reset(dev, dev->vqs[i]);
> > > +               mutex_unlock(&dev->vqs[i]->mutex);
> > >         }
> > 
> > I'm not sure it's correct to assume some behaviour of a buggy device.
> > For the device mutex, we use that to protect more than just err/call
> > and vq.
> 
> When I authored this, I did so as *the* fix.  However, since the cause
> of today's crash has now been patched, this has become a belt and
> braces solution.  Michael's addition of the WARN() also has the
> benefit of providing us with an early warning system for future
> breakages.  Personally, I think it's kinda neat.
> 
> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Michael S. Tsirkin March 8, 2022, 11:07 a.m. UTC | #16
On Tue, Mar 08, 2022 at 08:01:32AM +0000, Lee Jones wrote:
> On Mon, 07 Mar 2022, Michael S. Tsirkin wrote:
> 
> > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > during virtqueue clean-up and we mitigate the reported issues.
> > 
> > Pls just basically copy the code comment here. this is just confuses.
> > 
> > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > capture possible future race conditions which may pop up over time.
> > > 
> > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > 
> > And this is a bug we already fixed, right?
> 
> Well, this was the bug I set out to fix.
> 
> I didn't know your patch was in flight at the time.
> 
> > > Cc: <stable@vger.kernel.org>
> > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > 
> > not really applicable anymore ...
> 
> I can remove these if it helps.

yes let's do that pls.

> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Greg Kroah-Hartman March 8, 2022, 11:45 a.m. UTC | #17
On Tue, Mar 08, 2022 at 05:55:58AM -0500, Michael S. Tsirkin wrote:
> On Tue, Mar 08, 2022 at 10:57:42AM +0100, Greg KH wrote:
> > On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > > On Tue, 08 Mar 2022, Greg KH wrote:
> > > 
> > > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > > 
> > > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > > 
> > > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > > capture possible future race conditions which may pop up over time.
> > > > > > > 
> > > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > > 
> > > > > > > Cc: <stable@vger.kernel.org>
> > > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > > --- a/drivers/vhost/vhost.c
> > > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > > >  	int i;
> > > > > > >  
> > > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > > +		/* No workers should run here by design. However, races have
> > > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > > +		 */
> > > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > > 
> > > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > > future?  :)
> > > > > > 
> > > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > > you want that to happen?
> > > > > 
> > > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > > 
> > > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > > has resulted in a huge number of kernel fixes because of that.
> > > 
> > > Everything is customisable in syzkaller, so maybe there are specific
> > > builds which panic_on_warn enabled, but none that I'm involved with
> > > do.
> > 
> > Many systems run with panic-on-warn (i.e. the cloud), as they want to
> > drop a box and restart it if anything goes wrong.
> > 
> > That's why syzbot reports on WARN_* calls.  They should never be
> > reachable by userspace actions.
> > 
> > > Here follows a topical example.  The report above in the Link: tag
> > > comes with a crashlog [0].  In there you can see the WARN() at the
> > > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > > (non-flushed) worker list, before finally tripping the BUG() which
> > > triggers the report:
> > > 
> > > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> > 
> > Ok, so both happens here.  But don't add a warning for something that
> > can't happen.  Just handle it and move on.  It looks like you are
> > handling it in this code, so please drop the WARN_ON().
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hmm. Well this will mean if we ever reintroduce the bug then
> syzkaller will not catch it for us :( And the bug is there,
> it just results in a hard to reproduce error for userspace.

Is this an error you can recover from in the kernel?  What is userspace
supposed to know with this information when it sees it?

> Not sure what to do here. Export panic_on_warn flag to modules
> and check it here?

Hah, no, never do that :)

thanks,

greg k-h
Michael S. Tsirkin March 8, 2022, 12:27 p.m. UTC | #18
On Tue, Mar 08, 2022 at 12:45:19PM +0100, Greg KH wrote:
> On Tue, Mar 08, 2022 at 05:55:58AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Mar 08, 2022 at 10:57:42AM +0100, Greg KH wrote:
> > > On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > > > On Tue, 08 Mar 2022, Greg KH wrote:
> > > > 
> > > > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > > > 
> > > > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > > > 
> > > > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > > > capture possible future race conditions which may pop up over time.
> > > > > > > > 
> > > > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > > > 
> > > > > > > > Cc: <stable@vger.kernel.org>
> > > > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > ---
> > > > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > > > --- a/drivers/vhost/vhost.c
> > > > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > > > >  	int i;
> > > > > > > >  
> > > > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > > > +		/* No workers should run here by design. However, races have
> > > > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > > > +		 */
> > > > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > > > 
> > > > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > > > future?  :)
> > > > > > > 
> > > > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > > > you want that to happen?
> > > > > > 
> > > > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > > > 
> > > > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > > > has resulted in a huge number of kernel fixes because of that.
> > > > 
> > > > Everything is customisable in syzkaller, so maybe there are specific
> > > > builds which panic_on_warn enabled, but none that I'm involved with
> > > > do.
> > > 
> > > Many systems run with panic-on-warn (i.e. the cloud), as they want to
> > > drop a box and restart it if anything goes wrong.
> > > 
> > > That's why syzbot reports on WARN_* calls.  They should never be
> > > reachable by userspace actions.
> > > 
> > > > Here follows a topical example.  The report above in the Link: tag
> > > > comes with a crashlog [0].  In there you can see the WARN() at the
> > > > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > > > (non-flushed) worker list, before finally tripping the BUG() which
> > > > triggers the report:
> > > > 
> > > > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> > > 
> > > Ok, so both happens here.  But don't add a warning for something that
> > > can't happen.  Just handle it and move on.  It looks like you are
> > > handling it in this code, so please drop the WARN_ON().
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Hmm. Well this will mean if we ever reintroduce the bug then
> > syzkaller will not catch it for us :( And the bug is there,
> > it just results in a hard to reproduce error for userspace.
> 
> Is this an error you can recover from in the kernel?
>  What is userspace
> supposed to know with this information when it sees it?

IIUC we are talking about a use after free here since we somehow
managed to have a pointer to the device in a worker while
device is being destroyed.

That's the point of the warning as use after free is hard to debug. You
ask can we recover from a use after free? 

As regards to the added lock, IIUC it kind of shifts the use after free
window to later and since we zero out some of the memory just before we
free it, it's a bit more likely to recover.  I would still like to see
some more analysis on why the situation is always better than it was
before though.

> > Not sure what to do here. Export panic_on_warn flag to modules
> > and check it here?
> 
> Hah, no, never do that :)
> 
> thanks,
> 
> greg k-h
Lee Jones March 8, 2022, 1:17 p.m. UTC | #19
On Tue, 08 Mar 2022, Michael S. Tsirkin wrote:

> On Tue, Mar 08, 2022 at 12:45:19PM +0100, Greg KH wrote:
> > On Tue, Mar 08, 2022 at 05:55:58AM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Mar 08, 2022 at 10:57:42AM +0100, Greg KH wrote:
> > > > On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > > > > On Tue, 08 Mar 2022, Greg KH wrote:
> > > > > 
> > > > > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > > > > 
> > > > > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > > > > 
> > > > > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > > > > capture possible future race conditions which may pop up over time.
> > > > > > > > > 
> > > > > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > > > > 
> > > > > > > > > Cc: <stable@vger.kernel.org>
> > > > > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > > > > --- a/drivers/vhost/vhost.c
> > > > > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > > > > >  	int i;
> > > > > > > > >  
> > > > > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > > > > +		/* No workers should run here by design. However, races have
> > > > > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > > > > +		 */
> > > > > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > > > > 
> > > > > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > > > > future?  :)
> > > > > > > > 
> > > > > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > > > > you want that to happen?
> > > > > > > 
> > > > > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > > > > 
> > > > > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > > > > has resulted in a huge number of kernel fixes because of that.
> > > > > 
> > > > > Everything is customisable in syzkaller, so maybe there are specific
> > > > > builds which panic_on_warn enabled, but none that I'm involved with
> > > > > do.
> > > > 
> > > > Many systems run with panic-on-warn (i.e. the cloud), as they want to
> > > > drop a box and restart it if anything goes wrong.
> > > > 
> > > > That's why syzbot reports on WARN_* calls.  They should never be
> > > > reachable by userspace actions.
> > > > 
> > > > > Here follows a topical example.  The report above in the Link: tag
> > > > > comes with a crashlog [0].  In there you can see the WARN() at the
> > > > > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > > > > (non-flushed) worker list, before finally tripping the BUG() which
> > > > > triggers the report:
> > > > > 
> > > > > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> > > > 
> > > > Ok, so both happens here.  But don't add a warning for something that
> > > > can't happen.  Just handle it and move on.  It looks like you are
> > > > handling it in this code, so please drop the WARN_ON().
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Hmm. Well this will mean if we ever reintroduce the bug then
> > > syzkaller will not catch it for us :( And the bug is there,
> > > it just results in a hard to reproduce error for userspace.
> > 
> > Is this an error you can recover from in the kernel?
> >  What is userspace
> > supposed to know with this information when it sees it?
> 
> IIUC we are talking about a use after free here since we somehow
> managed to have a pointer to the device in a worker while
> device is being destroyed.
> 
> That's the point of the warning as use after free is hard to debug. You
> ask can we recover from a use after free? 
> 
> As regards to the added lock, IIUC it kind of shifts the use after free
> window to later and since we zero out some of the memory just before we
> free it, it's a bit more likely to recover.  I would still like to see
> some more analysis on why the situation is always better than it was
> before though.

With the locks in place, the UAF should not occur.

The issue here is that you have 2 different tasks processing the
same area of memory (via pointers to structs).  In these scenarios you
should always provide locking and/or reference counting to prevent
memory corruption or UAF.
Michael S. Tsirkin March 8, 2022, 5:17 p.m. UTC | #20
On Tue, Mar 08, 2022 at 01:17:03PM +0000, Lee Jones wrote:
> On Tue, 08 Mar 2022, Michael S. Tsirkin wrote:
> 
> > On Tue, Mar 08, 2022 at 12:45:19PM +0100, Greg KH wrote:
> > > On Tue, Mar 08, 2022 at 05:55:58AM -0500, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 08, 2022 at 10:57:42AM +0100, Greg KH wrote:
> > > > > On Tue, Mar 08, 2022 at 09:15:27AM +0000, Lee Jones wrote:
> > > > > > On Tue, 08 Mar 2022, Greg KH wrote:
> > > > > > 
> > > > > > > On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > > > > > > > On Mon, 07 Mar 2022, Greg KH wrote:
> > > > > > > > 
> > > > > > > > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > > > > > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > > > > > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > > > > > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > > > > > > > 
> > > > > > > > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > > > > > > > capture possible future race conditions which may pop up over time.
> > > > > > > > > > 
> > > > > > > > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > > > > > > > 
> > > > > > > > > > Cc: <stable@vger.kernel.org>
> > > > > > > > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > > > > > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > > > > > > > --- a/drivers/vhost/vhost.c
> > > > > > > > > > +++ b/drivers/vhost/vhost.c
> > > > > > > > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > > > > > > > >  	int i;
> > > > > > > > > >  
> > > > > > > > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > > > > > > > +		/* No workers should run here by design. However, races have
> > > > > > > > > > +		 * previously occurred where drivers have been unable to flush
> > > > > > > > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > > > > > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > > > > > > > +		 * corruption in those cases does seem preferable.
> > > > > > > > > > +		 */
> > > > > > > > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > > > > > > > 
> > > > > > > > > So you are trading one syzbot triggered issue for another one in the
> > > > > > > > > future?  :)
> > > > > > > > > 
> > > > > > > > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > > > > > > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > > > > > > > you want that to happen?
> > > > > > > > 
> > > > > > > > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> > > > > > > 
> > > > > > > Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> > > > > > > has resulted in a huge number of kernel fixes because of that.
> > > > > > 
> > > > > > Everything is customisable in syzkaller, so maybe there are specific
> > > > > > builds which panic_on_warn enabled, but none that I'm involved with
> > > > > > do.
> > > > > 
> > > > > Many systems run with panic-on-warn (i.e. the cloud), as they want to
> > > > > drop a box and restart it if anything goes wrong.
> > > > > 
> > > > > That's why syzbot reports on WARN_* calls.  They should never be
> > > > > reachable by userspace actions.
> > > > > 
> > > > > > Here follows a topical example.  The report above in the Link: tag
> > > > > > comes with a crashlog [0].  In there you can see the WARN() at the
> > > > > > bottom of vhost_dev_cleanup() trigger many times due to a populated
> > > > > > (non-flushed) worker list, before finally tripping the BUG() which
> > > > > > triggers the report:
> > > > > > 
> > > > > > [0] https://syzkaller.appspot.com/text?tag=CrashLog&x=16a61fce700000
> > > > > 
> > > > > Ok, so both happens here.  But don't add a warning for something that
> > > > > can't happen.  Just handle it and move on.  It looks like you are
> > > > > handling it in this code, so please drop the WARN_ON().
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Hmm. Well this will mean if we ever reintroduce the bug then
> > > > syzkaller will not catch it for us :( And the bug is there,
> > > > it just results in a hard to reproduce error for userspace.
> > > 
> > > Is this an error you can recover from in the kernel?
> > >  What is userspace
> > > supposed to know with this information when it sees it?
> > 
> > IIUC we are talking about a use after free here since we somehow
> > managed to have a pointer to the device in a worker while
> > device is being destroyed.
> > 
> > That's the point of the warning as use after free is hard to debug. You
> > ask can we recover from a use after free? 
> > 
> > As regards to the added lock, IIUC it kind of shifts the use after free
> > window to later and since we zero out some of the memory just before we
> > free it, it's a bit more likely to recover.  I would still like to see
> > some more analysis on why the situation is always better than it was
> > before though.
> 
> With the locks in place, the UAF should not occur.

This really depends which UAF. Yes use of vq->private_data is protected
by a lock inside the VQ.

However, we are talking about vhost_net_release, which ends up doing

        kfree(n->dev.vqs);
...
        kvfree(n);

if someone is holding a pointer to a vq or the device itself at this
point, no locks that are part of one of said structures will be
effective in preventing a use after free, and using a lock to delay such
accesses to this point just might make it more likely there's a use
after free.


All of the above is why we didn't rush to apply the locking patch in the
first place, for all that it seemed to fix the sysboz crash.



> The issue here is that you have 2 different tasks processing the
> same area of memory (via pointers to structs).  In these scenarios you
> should always provide locking and/or reference counting to prevent
> memory corruption or UAF.

But we should not have 2 tasks doing that, and if we do then lock
just might be ineffective since the lock itself is released.

Again maybe in this case it makes sense but it needs a more detailed
analysis to show it's a net win than just "we have two tasks ergo we
need locking".

> -- 
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Leon Romanovsky March 9, 2022, 6:52 p.m. UTC | #21
On Tue, Mar 08, 2022 at 09:57:57AM +0100, Greg KH wrote:
> On Tue, Mar 08, 2022 at 08:10:06AM +0000, Lee Jones wrote:
> > On Mon, 07 Mar 2022, Greg KH wrote:
> > 
> > > On Mon, Mar 07, 2022 at 07:17:57PM +0000, Lee Jones wrote:
> > > > vhost_vsock_handle_tx_kick() already holds the mutex during its call
> > > > to vhost_get_vq_desc().  All we have to do here is take the same lock
> > > > during virtqueue clean-up and we mitigate the reported issues.
> > > > 
> > > > Also WARN() as a precautionary measure.  The purpose of this is to
> > > > capture possible future race conditions which may pop up over time.
> > > > 
> > > > Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00
> > > > 
> > > > Cc: <stable@vger.kernel.org>
> > > > Reported-by: syzbot+adc3cb32385586bec859@syzkaller.appspotmail.com
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/vhost/vhost.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > > index 59edb5a1ffe28..ef7e371e3e649 100644
> > > > --- a/drivers/vhost/vhost.c
> > > > +++ b/drivers/vhost/vhost.c
> > > > @@ -693,6 +693,15 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > >  	int i;
> > > >  
> > > >  	for (i = 0; i < dev->nvqs; ++i) {
> > > > +		/* No workers should run here by design. However, races have
> > > > +		 * previously occurred where drivers have been unable to flush
> > > > +		 * all work properly prior to clean-up.  Without a successful
> > > > +		 * flush the guest will malfunction, but avoiding host memory
> > > > +		 * corruption in those cases does seem preferable.
> > > > +		 */
> > > > +		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
> > > 
> > > So you are trading one syzbot triggered issue for another one in the
> > > future?  :)
> > > 
> > > If this ever can happen, handle it, but don't log it with a WARN_ON() as
> > > that will trigger the panic-on-warn boxes, as well as syzbot.  Unless
> > > you want that to happen?
> > 
> > No, Syzbot doesn't report warnings, only BUGs and memory corruption.
> 
> Has it changed?  Last I looked, it did trigger on WARN_* calls, which
> has resulted in a huge number of kernel fixes because of that.
> 
> > > And what happens if the mutex is locked _RIGHT_ after you checked it?
> > > You still have a race...
> > 
> > No, we miss a warning that one time.  Memory is still protected.
> 
> Then don't warn on something that doesn't matter.  This line can be
> dropped as there's nothing anyone can do about it, right?

Greg, at least two other reviewers said that this line shouldn't be at
all.

https://lore.kernel.org/all/CACGkMEsjmCNQPjxPjXL0WUfbMg8ARnumEp4yjUxqznMKR1nKSQ@mail.gmail.com/
https://lore.kernel.org/all/YiG61RqXFvq%2Ft0fB@unreal/
https://lore.kernel.org/all/YiETnIcfZCLb63oB@unreal/

Thanks

> 
> thanks,
> 
> greg k-h
diff mbox series

Patch

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 59edb5a1ffe28..ef7e371e3e649 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -693,6 +693,15 @@  void vhost_dev_cleanup(struct vhost_dev *dev)
 	int i;
 
 	for (i = 0; i < dev->nvqs; ++i) {
+		/* No workers should run here by design. However, races have
+		 * previously occurred where drivers have been unable to flush
+		 * all work properly prior to clean-up.  Without a successful
+		 * flush the guest will malfunction, but avoiding host memory
+		 * corruption in those cases does seem preferable.
+		 */
+		WARN_ON(mutex_is_locked(&dev->vqs[i]->mutex));
+
+		mutex_lock(&dev->vqs[i]->mutex);
 		if (dev->vqs[i]->error_ctx)
 			eventfd_ctx_put(dev->vqs[i]->error_ctx);
 		if (dev->vqs[i]->kick)
@@ -700,6 +709,7 @@  void vhost_dev_cleanup(struct vhost_dev *dev)
 		if (dev->vqs[i]->call_ctx.ctx)
 			eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
 		vhost_vq_reset(dev, dev->vqs[i]);
+		mutex_unlock(&dev->vqs[i]->mutex);
 	}
 	vhost_dev_free_iovecs(dev);
 	if (dev->log_ctx)