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: [email protected]
Cc: Jason Wang <[email protected]>
Signed-off-by: Stefan Hajnoczi <[email protected]>
---
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
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: [email protected]
> Cc: Jason Wang <[email protected]>
> Signed-off-by: Stefan Hajnoczi <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]>
> ---
> 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
On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <[email protected]> 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
From: Stefan Hajnoczi <[email protected]>
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: [email protected]
Cc: Jason Wang <[email protected]>
Signed-off-by: Stefan Hajnoczi <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]>
---
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
On Mon, Apr 09, 2018 at 09:52:13AM -0700, Linus Torvalds wrote:
> On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <[email protected]> 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
On Tue, Apr 10, 2018 at 3:40 AM, Michael S. Tsirkin <[email protected]> wrote:
> From: Stefan Hajnoczi <[email protected]>
>
> 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: [email protected]
> Cc: Jason Wang <[email protected]>
> Signed-off-by: Stefan Hajnoczi <[email protected]>
> Acked-by: Michael S. Tsirkin <[email protected]>
>
> ---
> 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