[PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[K. Y. Srinivasan]

Correctly compute the local (gpadl) handle. 
I would like to thank Michael Brown <mcb30@ipxe.org> for seeing this bug.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reported-by: Michael Brown <mcb30@ipxe.org>
---
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(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 433f72a..c76ffbe 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -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)
-- 
1.7.4.1

Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[Jeremiah Mahler]

K. Y. Srinivasan,

On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote:
> 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.

> I would like to thank Michael Brown <mcb30@ipxe.org> for seeing this bug.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reported-by: Michael Brown <mcb30@ipxe.org>
> ---
> 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(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 433f72a..c76ffbe 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -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

>  	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
- Jeremiah Mahler

RE: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[KY Srinivasan]

> -----Original Message-----
> From: Jeremiah Mahler [mailto:jmmahler@gmail.com]
> Sent: Wednesday, December 10, 2014 6:10 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; mcb30@ipxe.org
> Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in
> vmbus_establish_gpadl()
> 
> K. Y. Srinivasan,
> 
> On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote:
> > 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.
> 
> > I would like to thank Michael Brown <mcb30@ipxe.org> for seeing this bug.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reported-by: Michael Brown <mcb30@ipxe.org>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> > 433f72a..c76ffbe 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -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

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
> 
> >  	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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> - Jeremiah Mahler

Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[Jeremiah Mahler]

KY Srinivasan,

On Mon, Dec 15, 2014 at 07:00:45AM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Jeremiah Mahler [mailto:jmmahler@gmail.com]
> > Sent: Wednesday, December 10, 2014 6:10 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; mcb30@ipxe.org
> > Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in
> > vmbus_establish_gpadl()
> > 
> > K. Y. Srinivasan,
> > 
> > On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote:
> > > 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.
> > 
> > > I would like to thank Michael Brown <mcb30@ipxe.org> for seeing this bug.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Reported-by: Michael Brown <mcb30@ipxe.org>
> > > ---
> > > 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(-)
> > >
> > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> > > 433f72a..c76ffbe 100644
> > > --- a/drivers/hv/channel.c
> > > +++ b/drivers/hv/channel.c
> > > @@ -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
> 
> 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
> > 
[...]

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.

-- 
- Jeremiah Mahler

Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[Andy Whitcroft]

On Sun, Dec 14, 2014 at 11:59:19PM -0800, Jeremiah Mahler wrote:
> KY Srinivasan,
> 
> On Mon, Dec 15, 2014 at 07:00:45AM +0000, KY Srinivasan wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Jeremiah Mahler [mailto:jmmahler@gmail.com]
> > > Sent: Wednesday, December 10, 2014 6:10 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > > jasowang@redhat.com; mcb30@ipxe.org
> > > Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in
> > > vmbus_establish_gpadl()
> > > 
> > > K. Y. Srinivasan,
> > > 
> > > On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote:
> > > > 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.
> > > 
> > > > I would like to thank Michael Brown <mcb30@ipxe.org> for seeing this bug.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Reported-by: Michael Brown <mcb30@ipxe.org>
> > > > ---
> > > > 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(-)
> > > >
> > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> > > > 433f72a..c76ffbe 100644
> > > > --- a/drivers/hv/channel.c
> > > > +++ b/drivers/hv/channel.c
> > > > @@ -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
> > 
> > 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
> > > 
> [...]
> 
> 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

Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[Jeremiah Mahler]

Andy,

On Mon, Dec 15, 2014 at 03:47:04PM +0000, Andy Whitcroft wrote:
> On Sun, Dec 14, 2014 at 11:59:19PM -0800, Jeremiah Mahler wrote:
[...]
> 
> 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

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

Thanks for clearing that up for me :-)

-- 
- Jeremiah Mahler