Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

PATCH In review K. Y. Srinivasan 3 participants 6 messages Dec 11, 2014 – Dec 15, 2014 Hyper-V/Azure core and drivers

Share & tools

K. Y. Srinivasan

author

2014-12-11 00:02 UTC #0 lore raw

PATCH v3 1/1 Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

Correctly compute the local (gpadl) handle.
I would like to thank Michael Brown <[email protected]> for seeing this bug.

Signed-off-by K. Y. Srinivasan <[email protected]>

Reported-by Michael Brown <[email protected]>

---
Changes in V2: Added the Reported-by tag. Changes in V3: Cleaned up the commit log.

 drivers/hv/channel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/hv/channel.c +2 -2

index 433f72a..c76ffbe 100644
@@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	unsigned long flags;
 	int ret = 0;
 
-	next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
-	atomic_inc(&vmbus_connection.next_gpadl_handle);
+	next_gpadl_handle =
+		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
 
 	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
 	if (ret)

Signature · 1 line

1.7.4.1

Jeremiah Mahler

2014-12-11 02:10 UTC #1 lore raw

K. Y. Srinivasan,

Quoted 1 line · K. Y. Srinivasan

Correctly compute the local (gpadl) handle.

This description is still too sparse for me. How was it computed before and why was this incorrect? Pretend like you are trying to explain your patch to someone who has no idea what you did.

Quoted 16 lines

I would like to thank Michael Brown <[email protected]> for seeing this bug.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by Michael Brown <[email protected]>
---
Changes in V2: Added the Reported-by tag.
Changes in V3: Cleaned up the commit log.
 drivers/hv/channel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/hv/channel.c +0 -0
index 433f72a..c76ffbe 100644

drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer, · line 371

Quoted 8 lines

drivers/hv/channel.c +2 -2

@@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	unsigned long flags;
 	int ret = 0;
 
-	next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
-	atomic_inc(&vmbus_connection.next_gpadl_handle);
+	next_gpadl_handle =
+		(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);

Tell me if I understand this correctly.

Before it read the handle and incremented it.

y = x + 1

Now it reads the handle, increments it, then decrements it.

y = (x + 1) - 1 = x

Quoted 10 lines

drivers/hv/channel.c +0 -0

 	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
 	if (ret)

Signature · 7 lines

1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 1 line

- Jeremiah Mahler

KY Srinivasan

author

2014-12-15 07:00 UTC #2 lore raw

Quoted 50 lines

-----Original Message-----
From: Jeremiah Mahler [mailto:[email protected]]
Sent: Wednesday, December 10, 2014 6:10 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
K. Y. Srinivasan,
Quoted 1 line · K. Y. Srinivasan
Correctly compute the local (gpadl) handle.
This description is still too sparse for me. How was it computed before and why was this incorrect? Pretend like you are trying to explain your patch to someone who has no idea what you did.
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel
Quoted 16 lines
I would like to thank Michael Brown <[email protected]> for seeing this bug.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by Michael Brown <[email protected]>
---
Changes in V2: Added the Reported-by tag.
Changes in V3: Cleaned up the commit log.
 drivers/hv/channel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/hv/channel.c index renamed from drivers/hv/channel.c +0 -0
433f72a..c76ffbe 100644
drivers/hv/channel.c +0 -0
@@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
*channel, void *kbuffer,
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 4 lines
drivers/hv/channel.c +0 -1
 	unsigned long flags;
 	int ret = 0;
 
-	next_gpadl_handle =
atomic_read(&vmbus_connection.next_gpadl_handle);
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 3 lines
drivers/hv/channel.c +2 -1
-	atomic_inc(&vmbus_connection.next_gpadl_handle);
+	next_gpadl_handle =
+
	(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
Quoted 1 line
Tell me if I understand this correctly.

Quoted 7 lines

Before it read the handle and incremented it.
y = x + 1
Now it reads the handle, increments it, then decrements it.
y = (x + 1) - 1 = x

This code can be executed concurrently on multiple CPUs. We want to ensure that each call to establish gpadl gets a unique local handle. The earlier code was buggy in that we would read the handle and then atomically increment it. Thus, multiple CPUs could read the identical current value which would be their local handle. What we want is the ability to atomically read and increment the value - this would ensure that each caller got a unique value even if they executed the code concurrently on multiple CPUs. The API atomic_inc_return(), atomically increments and returns the incremented value. We locally decrement this value to emulate the logic of "read the current value and atomically increment the value.

Hope this helps,

K. Y

Quoted 14 lines

Quoted 11 lines
drivers/hv/channel.c +0 -1
 	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
 	if (ret)
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
- Jeremiah Mahler

Jeremiah Mahler

2014-12-15 07:59 UTC #3 lore raw

KY Srinivasan,

Quoted 67 lines · KY Srinivasan

Quoted 50 lines
-----Original Message-----
From: Jeremiah Mahler [mailto:[email protected]]
Sent: Wednesday, December 10, 2014 6:10 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
K. Y. Srinivasan,
Quoted 1 line · K. Y. Srinivasan
Correctly compute the local (gpadl) handle.
This description is still too sparse for me. How was it computed before and why was this incorrect? Pretend like you are trying to explain your patch to someone who has no idea what you did.
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel
Quoted 16 lines
I would like to thank Michael Brown <[email protected]> for seeing this bug.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by Michael Brown <[email protected]>
---
Changes in V2: Added the Reported-by tag.
Changes in V3: Cleaned up the commit log.
 drivers/hv/channel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/hv/channel.c index renamed from drivers/hv/channel.c +0 -0
433f72a..c76ffbe 100644
drivers/hv/channel.c +0 -0
@@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
*channel, void *kbuffer,
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 4 lines
drivers/hv/channel.c +0 -1
 	unsigned long flags;
 	int ret = 0;
 
-	next_gpadl_handle =
atomic_read(&vmbus_connection.next_gpadl_handle);
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 3 lines
drivers/hv/channel.c +2 -1
-	atomic_inc(&vmbus_connection.next_gpadl_handle);
+	next_gpadl_handle =
+
	(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
Quoted 1 line
Tell me if I understand this correctly.
Quoted 7 lines
Before it read the handle and incremented it.
y = x + 1
Now it reads the handle, increments it, then decrements it.
y = (x + 1) - 1 = x
This code can be executed concurrently on multiple CPUs. We want to ensure that each call to establish gpadl gets a unique local handle. The earlier code was buggy in that we would read the handle and then atomically increment it. Thus, multiple CPUs could read the identical current value which would be their local handle. What we want is the ability to atomically read and increment the value - this would ensure that each caller got a unique value even if they executed the code concurrently on multiple CPUs. The API atomic_inc_return(), atomically increments and returns the incremented value. We locally decrement this value to emulate the logic of "read the current value and atomically increment the value.

Quoted 4 lines

Hope this helps,
K. Y
Quoted 1 line

[...]

So to avoid concurrency issues you used a single atomic operation instead of two separate operations. That make sense. But it still doesn't explain why you changed the calculation by subtracting 1.

Signature · 1 line

- Jeremiah Mahler

Andy Whitcroft

2014-12-15 15:47 UTC #4 lore raw

Quoted 72 lines · Jeremiah Mahler

KY Srinivasan,
Quoted 69 lines · KY Srinivasan
Quoted 50 lines
-----Original Message-----
From: Jeremiah Mahler [mailto:[email protected]]
Sent: Wednesday, December 10, 2014 6:10 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()
K. Y. Srinivasan,
Quoted 1 line · K. Y. Srinivasan
Correctly compute the local (gpadl) handle.
This description is still too sparse for me. How was it computed before and why was this incorrect? Pretend like you are trying to explain your patch to someone who has no idea what you did.
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel
Quoted 16 lines
I would like to thank Michael Brown <[email protected]> for seeing this bug.
Signed-off-by K. Y. Srinivasan <[email protected]>
Reported-by Michael Brown <[email protected]>
---
Changes in V2: Added the Reported-by tag.
Changes in V3: Cleaned up the commit log.
 drivers/hv/channel.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/hv/channel.c index renamed from drivers/hv/channel.c +0 -0
433f72a..c76ffbe 100644
drivers/hv/channel.c +0 -0
@@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
*channel, void *kbuffer,
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 4 lines
drivers/hv/channel.c +0 -1
 	unsigned long flags;
 	int ret = 0;
 
-	next_gpadl_handle =
atomic_read(&vmbus_connection.next_gpadl_handle);
drivers/hv/channel.c int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
Quoted 3 lines
drivers/hv/channel.c +2 -1
-	atomic_inc(&vmbus_connection.next_gpadl_handle);
+	next_gpadl_handle =
+
	(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
Quoted 1 line
Tell me if I understand this correctly.
Quoted 7 lines
Before it read the handle and incremented it.
y = x + 1
Now it reads the handle, increments it, then decrements it.
y = (x + 1) - 1 = x
This code can be executed concurrently on multiple CPUs. We want to ensure that each call to establish gpadl gets a unique local handle. The earlier code was buggy in that we would read the handle and then atomically increment it. Thus, multiple CPUs could read the identical current value which would be their local handle. What we want is the ability to atomically read and increment the value - this would ensure that each caller got a unique value even if they executed the code concurrently on multiple CPUs. The API atomic_inc_return(), atomically increments and returns the incremented value. We locally decrement this value to emulate the logic of "read the current value and atomically increment the value.
Hope this helps,

Quoted 7 lines

Quoted 2 lines
K. Y
Quoted 1 line
[...]
So to avoid concurrency issues you used a single atomic operation instead of two separate operations. That make sense. But it still doesn't explain why you changed the calculation by subtracting 1.

The calculation appears identical to my reading, the original form was:

  next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
  atomic_inc(&vmbus_connection.next_gpadl_handle);

or:

  y = x;
  x++;

so y == x' (x before incrementing)

the new code is:

  next_gpadl_handle = (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);

or:

  y = ++x - 1;

Also making y = x' (x before incrementing)

-apw

Jeremiah Mahler

2014-12-15 18:51 UTC #5 lore raw

Andy,

Quoted 1 line · Andy Whitcroft

On Sun, Dec 14, 2014 at 11:59:19PM -0800, Jeremiah Mahler wrote:

[...]

Quoted 17 lines

The calculation appears identical to my reading, the original form was:
  next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
  atomic_inc(&vmbus_connection.next_gpadl_handle);
or:
  y = x;
  x++;
so y == x' (x before incrementing)
the new code is:
  next_gpadl_handle = (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);

Quoted 7 lines

or:
  y = ++x - 1;
Also making y = x' (x before incrementing)
-apw

Ah, you are right. The increment before/after messed me up.

Thanks for clearing that up for me :-)

Signature · 1 line

- Jeremiah Mahler