vhost: fix vhost_vq_access_ok() log check

PATCH In review Stefan Hajnoczi 4 participants 10 messages Apr 9, 2018 – Apr 10, 2018

Subsystems (5)

Networking Network drivers KVM virtio core +1 more subsystems

lore

Share & tools

lore

Stefan Hajnoczi

author

2018-04-09 13:10 UTC #0 lore raw

PATCH 1/1 vhost: fix vhost_vq_access_ok() log check

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(-)

drivers/vhost/vhost.c +1 -1

index 5320039671b7..f6af4210679a 100644
@@ -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);

Signature · 1 line

2.14.3

Michael S. Tsirkin

maintainer Acked-by on v1 1/1

2018-04-09 13:58 UTC #1 lore raw

Quoted 16 lines · Stefan Hajnoczi

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:

Quoted 7 lines

  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]>

Quoted 19 lines

---
 drivers/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/vhost/vhost.c +1 -1

index 5320039671b7..f6af4210679a 100644
@@ -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);

Signature · 1 line

2.14.3

Linus Torvalds

2018-04-09 16:52 UTC #2 lore raw

In reply to Stefan Hajnoczi

drivers/vhost/vhost.c int vhost_vq_access_ok(struct vhost_virtqueue *vq) · line 1250

Quoted 7 lines · Stefan Hajnoczi

drivers/vhost/vhost.c +1 -1

@@ -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

Michael S. Tsirkin

maintainer

2018-04-09 19:54 UTC #3 lore raw

Quoted 31 lines · Linus Torvalds

drivers/vhost/vhost.c int vhost_vq_access_ok(struct vhost_virtqueue *vq) · line 1250
Quoted 7 lines · Stefan Hajnoczi
drivers/vhost/vhost.c +1 -1
@@ -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.

Quoted 6 lines

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.

Signature · 1 line

MST

Michael S. Tsirkin

maintainer

2018-04-09 19:54 UTC #4 lore raw

In reply to Linus Torvalds

Quoted 31 lines · Linus Torvalds

drivers/vhost/vhost.c int vhost_vq_access_ok(struct vhost_virtqueue *vq) · line 1250
Quoted 7 lines · Stefan Hajnoczi
drivers/vhost/vhost.c +1 -1
@@ -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.

Quoted 6 lines

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.

Signature · 1 line

MST

Linus Torvalds

2018-04-09 16:52 UTC #5 lore raw

In reply to Stefan Hajnoczi

drivers/vhost/vhost.c int vhost_vq_access_ok(struct vhost_virtqueue *vq) · line 1250

Quoted 7 lines · Stefan Hajnoczi

drivers/vhost/vhost.c +1 -1

@@ -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

Michael S. Tsirkin

maintainer

2018-04-09 19:40 UTC #6 lore raw

PATCH RESEND 1/1 vhost: fix vhost_vq_access_ok() log check

In reply to Stefan Hajnoczi

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(-)

drivers/vhost/vhost.c +1 -1

index 5320039671b7..f6af4210679a 100644
@@ -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);

Signature · 1 line

2.14.3

Stefan Hajnoczi

NAK

2018-04-10 01:05 UTC #7 lore raw

Quoted 22 lines · Michael S. Tsirkin

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;

Quoted 8 lines

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

Stefan Hajnoczi

NAK

2018-04-10 01:05 UTC #8 lore raw

In reply to Michael S. Tsirkin

Quoted 22 lines · Michael S. Tsirkin

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;

Quoted 8 lines

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

Michael S. Tsirkin

maintainer

2018-04-09 19:40 UTC #9 lore raw

PATCH RESEND 1/1 vhost: fix vhost_vq_access_ok() log check

In reply to Stefan Hajnoczi

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(-)

drivers/vhost/vhost.c +1 -1

index 5320039671b7..f6af4210679a 100644
@@ -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);

Signature · 1 line

2.14.3