* Re: [PATCH] vhost: fix vhost_vq_access_ok() log check
[not found] <20180409131021.5132-1-stefanha@redhat.com>
@ 2018-04-09 13:58 ` Michael S. Tsirkin
2018-04-09 16:52 ` Linus Torvalds
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-09 13:58 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: syzkaller-bugs, linux-kernel, kvm, virtualization
On Mon, Apr 09, 2018 at 09:10:21PM +0800, Stefan Hajnoczi wrote:
> Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
> when IOTLB is enabled") introduced a regression. The logic was
> originally:
>
> if (vq->iotlb)
> return 1;
> return A && B;
>
> After the patch the short-circuit logic for A was inverted:
>
> if (A || vq->iotlb)
> return A;
> return B;
>
> The correct logic is:
>
> if (!A || vq->iotlb)
> return A;
> return B;
>
> Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5320039671b7..f6af4210679a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> int ret = vq_log_access_ok(vq, vq->log_base);
>
> - if (ret || vq->iotlb)
> + if (!ret || vq->iotlb)
> return ret;
>
> return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
> --
> 2.14.3
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] vhost: fix vhost_vq_access_ok() log check
[not found] <20180409131021.5132-1-stefanha@redhat.com>
2018-04-09 13:58 ` [PATCH] vhost: fix vhost_vq_access_ok() log check Michael S. Tsirkin
@ 2018-04-09 16:52 ` Linus Torvalds
2018-04-09 19:40 ` [PATCH RESEND net] " Michael S. Tsirkin
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Linus Torvalds @ 2018-04-09 16:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: KVM list, Michael S. Tsirkin, syzkaller-bugs,
Linux Kernel Mailing List, virtualization
On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> @@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> int ret = vq_log_access_ok(vq, vq->log_base);
>
> - if (ret || vq->iotlb)
> + if (!ret || vq->iotlb)
> return ret;
That logic is still very non-obvious.
This code already had one bug because of an odd illegible test
sequence. Let's not keep the crazy code.
Why not just do the *obvious* thing, and get rid of "ret" entirely,
and make the damn thing return a boolean, and then just write it all
as
/* Caller should have vq mutex and device mutex */
bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
if (!vq_log_access_ok(vq, vq->log_base))
return false;
if (vq->iotlb || vq_access_ok(vq, vq->num, vq->desc,
vq->avail, vq->used);
}
which makes the logic obvious: if vq_log_access_ok() fails, then then
vhost_vq_access_ok() fails unconditionally.
Otherwise, we need to have an iotlb, or a successful vq_access_ok() check.
Doesn't that all make more sense, and avoid the insane "ret" value use
that is really quite subtle?
Linus
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH RESEND net] vhost: fix vhost_vq_access_ok() log check
[not found] <20180409131021.5132-1-stefanha@redhat.com>
2018-04-09 13:58 ` [PATCH] vhost: fix vhost_vq_access_ok() log check Michael S. Tsirkin
2018-04-09 16:52 ` Linus Torvalds
@ 2018-04-09 19:40 ` Michael S. Tsirkin
[not found] ` <CA+55aFzJ_JPtU5SjAHHv--p3ZcAw4gVidxvrLLZF7w+kbHCNdg@mail.gmail.com>
[not found] ` <20180409174731-mutt-send-email-mst@kernel.org>
4 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-09 19:40 UTC (permalink / raw)
To: David Miller, netdev, virtualization
Cc: kvm, mst, syzkaller-bugs, linux-kernel, Stefan Hajnoczi
From: Stefan Hajnoczi <stefanha@redhat.com>
Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
when IOTLB is enabled") introduced a regression. The logic was
originally:
if (vq->iotlb)
return 1;
return A && B;
After the patch the short-circuit logic for A was inverted:
if (A || vq->iotlb)
return A;
return B;
The correct logic is:
if (!A || vq->iotlb)
return A;
return B;
Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5320039671b7..f6af4210679a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
int ret = vq_log_access_ok(vq, vq->log_base);
- if (ret || vq->iotlb)
+ if (!ret || vq->iotlb)
return ret;
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread[parent not found: <CA+55aFzJ_JPtU5SjAHHv--p3ZcAw4gVidxvrLLZF7w+kbHCNdg@mail.gmail.com>]
* Re: [PATCH] vhost: fix vhost_vq_access_ok() log check
[not found] ` <CA+55aFzJ_JPtU5SjAHHv--p3ZcAw4gVidxvrLLZF7w+kbHCNdg@mail.gmail.com>
@ 2018-04-09 19:54 ` Michael S. Tsirkin
0 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-09 19:54 UTC (permalink / raw)
To: Linus Torvalds
Cc: KVM list, syzkaller-bugs, Linux Kernel Mailing List,
virtualization, Stefan Hajnoczi
On Mon, Apr 09, 2018 at 09:52:13AM -0700, Linus Torvalds wrote:
> On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > @@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq)
> > {
> > int ret = vq_log_access_ok(vq, vq->log_base);
> >
> > - if (ret || vq->iotlb)
> > + if (!ret || vq->iotlb)
> > return ret;
>
> That logic is still very non-obvious.
>
> This code already had one bug because of an odd illegible test
> sequence. Let's not keep the crazy code.
>
> Why not just do the *obvious* thing, and get rid of "ret" entirely,
> and make the damn thing return a boolean, and then just write it all
> as
>
> /* Caller should have vq mutex and device mutex */
> bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
> {
> if (!vq_log_access_ok(vq, vq->log_base))
> return false;
>
> if (vq->iotlb || vq_access_ok(vq, vq->num, vq->desc,
> vq->avail, vq->used);
> }
>
> which makes the logic obvious: if vq_log_access_ok() fails, then then
> vhost_vq_access_ok() fails unconditionally.
>
> Otherwise, we need to have an iotlb, or a successful vq_access_ok() check.
>
> Doesn't that all make more sense, and avoid the insane "ret" value use
> that is really quite subtle?
>
> Linus
I agree it's cleaner.
Stefan, I reposted your patch on netdev (since the breakage got applied
there too).
Would you like to self-nak it and post v2? Pls remember to CC netdev.
--
MST
^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20180409174731-mutt-send-email-mst@kernel.org>]
* Re: [PATCH RESEND net] vhost: fix vhost_vq_access_ok() log check
[not found] ` <20180409174731-mutt-send-email-mst@kernel.org>
@ 2018-04-10 1:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2018-04-10 1:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, syzkaller-bugs, linux-kernel, Linux Virtualization,
Stefan Hajnoczi, David Miller
On Tue, Apr 10, 2018 at 3:40 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Commit d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log
> when IOTLB is enabled") introduced a regression. The logic was
> originally:
>
> if (vq->iotlb)
> return 1;
> return A && B;
>
> After the patch the short-circuit logic for A was inverted:
>
> if (A || vq->iotlb)
> return A;
> return B;
>
> The correct logic is:
>
> if (!A || vq->iotlb)
> return A;
> return B;
>
> Reported-by: syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
NACK
I will send a v2 with cleaner logic as suggested by Linus.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread