[PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[K. Y. Srinivasan]

Cleanup the channel management code and eliminate calls to BUG_ON().
Also fix an error propagation bug in vmbus_open().

In this version of the patch-set, I have addressed comments from
Dan Carpenter.

K. Y. Srinivasan (5):
  Drivers: hv: vmbus: Cleanup vmbus_post_msg()
  Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
  Drivers: hv: vmbus: Cleanup vmbus_close_internal()
  Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()
  Drivers: hv: vmbus: Fix a bug in vmbus_open()

 drivers/hv/channel.c    |   49 +++++++++++++++++++++++++++++++---------------
 drivers/hv/connection.c |   17 ++++++++++++---
 2 files changed, 46 insertions(+), 20 deletions(-)

-- 
1.7.4.1

[PATCH V2 1/5] Drivers: hv: vmbus: Cleanup vmbus_post_msg()

[K. Y. Srinivasan]

Posting messages to the host can fail because of transient resource
related failures. Correctly deal with these failures and increase the
number of attempts to post the message before giving up.

In this version of the patch, I have normalized the error code to
Linux error code.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/connection.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index ae22e3c..e206619 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -427,10 +427,21 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 	 * insufficient resources. Retry the operation a couple of
 	 * times before giving up.
 	 */
-	while (retries < 3) {
-		ret =  hv_post_message(conn_id, 1, buffer, buflen);
-		if (ret != HV_STATUS_INSUFFICIENT_BUFFERS)
+	while (retries < 10) {
+		ret = hv_post_message(conn_id, 1, buffer, buflen);
+
+		switch (ret) {
+		case HV_STATUS_INSUFFICIENT_BUFFERS:
+			ret = -ENOMEM;
+		case -ENOMEM:
+			break;
+		case HV_STATUS_SUCCESS:
 			return ret;
+		default:
+			pr_err("hv_post_msg() failed; error code:%d\n", ret);
+			return -EINVAL;
+		}
+
 		retries++;
 		msleep(100);
 	}
-- 
1.7.4.1

[PATCH V2 2/5] Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()

[K. Y. Srinivasan]

Eliminate calls to BUG_ON() by properly handling errors. In cases where
rollback is possible, we will return the appropriate error to have the
calling code decide how to rollback state. In the case where we are
transferring ownership of the guest physical pages to the host,
we will wait for the host to respond.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 531a593..0206314 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -435,7 +435,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
 	struct vmbus_channel_gpadl_teardown *msg;
 	struct vmbus_channel_msginfo *info;
 	unsigned long flags;
-	int ret, t;
+	int ret;
 
 	info = kmalloc(sizeof(*info) +
 		       sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
@@ -457,11 +457,12 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
 	ret = vmbus_post_msg(msg,
 			       sizeof(struct vmbus_channel_gpadl_teardown));
 
-	BUG_ON(ret != 0);
-	t = wait_for_completion_timeout(&info->waitevent, 5*HZ);
-	BUG_ON(t == 0);
+	if (ret)
+		goto post_msg_err;
+
+	wait_for_completion(&info->waitevent);
 
-	/* Received a torndown response */
+post_msg_err:
 	spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
 	list_del(&info->msglistentry);
 	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
-- 
1.7.4.1

[PATCH V3 3/5] Drivers: hv: vmbus: Cleanup vmbus_close_internal()

[K. Y. Srinivasan]

Eliminate calls to BUG_ON() in vmbus_close_internal().
We have chosen to potentially leak memory, than crash the guest
in case of failures.

In this version of the patch I have addressed comments from
Dan Carpenter (dan.carpenter@oracle.com).

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |   29 +++++++++++++++++++++++------
 1 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0206314..eb3158e 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -479,7 +479,7 @@ static void reset_channel_cb(void *arg)
 	channel->onchannel_callback = NULL;
 }
 
-static void vmbus_close_internal(struct vmbus_channel *channel)
+static int vmbus_close_internal(struct vmbus_channel *channel)
 {
 	struct vmbus_channel_close_channel *msg;
 	int ret;
@@ -502,11 +502,28 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
 
 	ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_close_channel));
 
-	BUG_ON(ret != 0);
+	if (ret) {
+		pr_err("Close failed: close post msg return is %d\n", ret);
+		/*
+		 * If we failed to post the close msg,
+		 * it is perhaps better to leak memory.
+		 */
+		return ret;
+	}
+
 	/* Tear down the gpadl for the channel's ring buffer */
-	if (channel->ringbuffer_gpadlhandle)
-		vmbus_teardown_gpadl(channel,
-					  channel->ringbuffer_gpadlhandle);
+	if (channel->ringbuffer_gpadlhandle) {
+		ret = vmbus_teardown_gpadl(channel,
+					   channel->ringbuffer_gpadlhandle);
+		if (ret) {
+			pr_err("Close failed: teardown gpadl return %d\n", ret);
+			/*
+			 * If we failed to teardown gpadl,
+			 * it is perhaps better to leak memory.
+			 */
+			return ret;
+		}
+	}
 
 	/* Cleanup the ring buffers for this channel */
 	hv_ringbuffer_cleanup(&channel->outbound);
@@ -515,7 +532,7 @@ static void vmbus_close_internal(struct vmbus_channel *channel)
 	free_pages((unsigned long)channel->ringbuffer_pages,
 		get_order(channel->ringbuffer_pagecount * PAGE_SIZE));
 
-
+	return ret;
 }
 
 /*
-- 
1.7.4.1

[PATCH V2 4/5] Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()

[K. Y. Srinivasan]

Eliminate the call to BUG_ON() by waiting for the host to respond. We are
trying to reclaim the ownership of memory that was given to the host and so
we will have to wait until the host responds.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index eb3158e..9a6a2c3 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -363,7 +363,6 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 	u32 next_gpadl_handle;
 	unsigned long flags;
 	int ret = 0;
-	int t;
 
 	next_gpadl_handle = atomic_read(&vmbus_connection.next_gpadl_handle);
 	atomic_inc(&vmbus_connection.next_gpadl_handle);
@@ -410,9 +409,7 @@ int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
 
 		}
 	}
-	t = wait_for_completion_timeout(&msginfo->waitevent, 5*HZ);
-	BUG_ON(t == 0);
-
+	wait_for_completion(&msginfo->waitevent);
 
 	/* At this point, we received the gpadl created msg */
 	*gpadl_handle = gpadlmsg->gpadl;
-- 
1.7.4.1

[PATCH V2 5/5] Drivers: hv: vmbus: Fix a bug in vmbus_open()

[K. Y. Srinivasan]

Fix a bug in vmbus_open() and properly propagate the error. I would
like to thank Dexuan Cui <decui@microsoft.com> for identifying the
issue.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/channel.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 9a6a2c3..19bad59 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -165,8 +165,10 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 	ret = vmbus_post_msg(open_msg,
 			       sizeof(struct vmbus_channel_open_channel));
 
-	if (ret != 0)
+	if (ret != 0) {
+		err = ret;
 		goto error1;
+	}
 
 	t = wait_for_completion_timeout(&open_info->waitevent, 5*HZ);
 	if (t == 0) {
-- 
1.7.4.1

Re: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[Sitsofe Wheeler]

On Wed, Aug 27, 2014 at 04:25:05PM -0700, K. Y. Srinivasan wrote:
> Cleanup the channel management code and eliminate calls to BUG_ON().
> Also fix an error propagation bug in vmbus_open().
> 
> In this version of the patch-set, I have addressed comments from
> Dan Carpenter.
> 
> K. Y. Srinivasan (5):
>   Drivers: hv: vmbus: Cleanup vmbus_post_msg()
>   Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
>   Drivers: hv: vmbus: Cleanup vmbus_close_internal()
>   Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()
>   Drivers: hv: vmbus: Fix a bug in vmbus_open()

This patch set resolves a number of BUG_ON crashes I was hitting on
Hyper-V while booting a kernel with verification options on. With these
I can complete a 3.17.0-rc2 boot on an SMP setup although patches from
"Drivers: hv: vmbus: Miscellaneous cleanup" are needed to resolve other
non-crash issues.

Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

-- 
Sitsofe | http://sucs.org/~sits/

RE: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[KY Srinivasan]

> -----Original Message-----
> From: Sitsofe Wheeler [mailto:sitsofe@gmail.com]
> Sent: Friday, August 29, 2014 12:29 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Dexuan Cui
> Subject: Re: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()
> 
> On Wed, Aug 27, 2014 at 04:25:05PM -0700, K. Y. Srinivasan wrote:
> > Cleanup the channel management code and eliminate calls to BUG_ON().
> > Also fix an error propagation bug in vmbus_open().
> >
> > In this version of the patch-set, I have addressed comments from Dan
> > Carpenter.
> >
> > K. Y. Srinivasan (5):
> >   Drivers: hv: vmbus: Cleanup vmbus_post_msg()
> >   Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
> >   Drivers: hv: vmbus: Cleanup vmbus_close_internal()
> >   Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()
> >   Drivers: hv: vmbus: Fix a bug in vmbus_open()
> 
> This patch set resolves a number of BUG_ON crashes I was hitting on Hyper-
> V while booting a kernel with verification options on. With these I can
> complete a 3.17.0-rc2 boot on an SMP setup although patches from
> "Drivers: hv: vmbus: Miscellaneous cleanup" are needed to resolve other
> non-crash issues.
> 
> Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

Greg,

Just checking if I should resend this patch-set. I have also sent a few patches after this.
Please let me know.

Thanks,

K. Y
> 
> --
> Sitsofe | http://sucs.org/~sits/

RE: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()

[KY Srinivasan]

> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of KY Srinivasan
> Sent: Monday, September 15, 2014 11:39 AM
> To: Sitsofe Wheeler
> Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> linux-kernel@vger.kernel.org; apw@canonical.com;
> devel@linuxdriverproject.org
> Subject: RE: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to BUG_ON()
> 
> 
> 
> > -----Original Message-----
> > From: Sitsofe Wheeler [mailto:sitsofe@gmail.com]
> > Sent: Friday, August 29, 2014 12:29 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; Dexuan Cui
> > Subject: Re: [PATCH V2 0/5] Drivers: hv: vmbus: Eliminate calls to
> > BUG_ON()
> >
> > On Wed, Aug 27, 2014 at 04:25:05PM -0700, K. Y. Srinivasan wrote:
> > > Cleanup the channel management code and eliminate calls to BUG_ON().
> > > Also fix an error propagation bug in vmbus_open().
> > >
> > > In this version of the patch-set, I have addressed comments from Dan
> > > Carpenter.
> > >
> > > K. Y. Srinivasan (5):
> > >   Drivers: hv: vmbus: Cleanup vmbus_post_msg()
> > >   Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl()
> > >   Drivers: hv: vmbus: Cleanup vmbus_close_internal()
> > >   Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl()
> > >   Drivers: hv: vmbus: Fix a bug in vmbus_open()
> >
> > This patch set resolves a number of BUG_ON crashes I was hitting on
> > Hyper- V while booting a kernel with verification options on. With
> > these I can complete a 3.17.0-rc2 boot on an SMP setup although
> > patches from
> > "Drivers: hv: vmbus: Miscellaneous cleanup" are needed to resolve
> > other non-crash issues.
> >
> > Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
> 
> Greg,
> 
> Just checking if I should resend this patch-set. I have also sent a few patches
> after this.
> Please let me know.

Ping.

K. Y
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel