[PATCH v2 0/3] Drivers: hv: vmbus: protect Offer/Rescind offer processing

[PATCH v2 0/3] Drivers: hv: vmbus: protect Offer/Rescind offer processing

[Vitaly Kuznetsov]

This patch series is a successor of "[PATCH] Drivers: hv: vmbus: serialize
Offer and Rescind offer processing" and thereby have 'v2' in its name.

Changes from v1:
- Separate vmbus_device_create() return value check [K. Y. Srinivasan]
- Do not lose a rescind offer received during offer processing. Use renamed
(in [PATCH v2 2/3]) spinlock to protect simulteneous test-and-set workflow for
rescind and work fields. [K. Y. Srinivasan]

Vitaly Kuznetsov (3):
  Drivers: hv: check vmbus_device_create() return value in
    vmbus_process_offer()
  Drivers: hv: rename sc_lock to the more generic lock
  Drivers: hv: vmbus: serialize Offer and Rescind offer

 drivers/hv/channel.c      |  6 +++---
 drivers/hv/channel_mgmt.c | 45 ++++++++++++++++++++++++++++++++-------------
 include/linux/hyperv.h    |  7 ++++++-
 3 files changed, 41 insertions(+), 17 deletions(-)

-- 
1.9.3

[PATCH v2 1/3] Drivers: hv: check vmbus_device_create() return value in vmbus_process_offer()

[Vitaly Kuznetsov]

vmbus_device_create() result is not being checked in vmbus_process_offer() and
it can fail if kzalloc() fails. Add the check and do minor cleanup to avoid
additional duplication of "free_channel(); return;" block.

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 2c59f03..80590c6 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -341,11 +341,10 @@ static void vmbus_process_offer(struct work_struct *work)
 			if (channel->sc_creation_callback != NULL)
 				channel->sc_creation_callback(newchannel);
 
-			return;
+			goto out;
 		}
 
-		free_channel(newchannel);
-		return;
+		goto error;
 	}
 
 	/*
@@ -364,6 +363,8 @@ static void vmbus_process_offer(struct work_struct *work)
 		&newchannel->offermsg.offer.if_type,
 		&newchannel->offermsg.offer.if_instance,
 		newchannel);
+	if (!newchannel->device_obj)
+		goto error;
 
 	/*
 	 * Add the new device to the bus. This will kick off device-driver
@@ -379,9 +380,12 @@ static void vmbus_process_offer(struct work_struct *work)
 		list_del(&newchannel->listentry);
 		spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
 		kfree(newchannel->device_obj);
-
-		free_channel(newchannel);
+		goto error;
 	}
+out:
+	return;
+error:
+	free_channel(newchannel);
 }
 
 enum {
-- 
1.9.3

Re: [PATCH v2 1/3] Drivers: hv: check vmbus_device_create() return value in vmbus_process_offer()

[Dan Carpenter]

On Mon, Jan 19, 2015 at 05:56:11PM +0100, Vitaly Kuznetsov wrote:
> vmbus_device_create() result is not being checked in vmbus_process_offer() and
> it can fail if kzalloc() fails. Add the check and do minor cleanup to avoid
> additional duplication of "free_channel(); return;" block.
> 
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

"out" is always a bad name for a label.  It's too vague.  It implies
that the code uses "One Err" style error handling which is bug prone and
I've ranted about that in the past so I won't here.  This kind of coding
is buggier than direct returns.  But recently I've been looking at bugs
where we return zero where the code should return a negative error code
and, wow, do I hate "out" labels!

	if (function_whatever(xxx))
		goto out;

[ thousands of lines removed. ]

out:
	return ret;

Oh crap...  Did the coder mean to return success or not???

If you use a direct return then the code looks like:

	if (function_whatever(xxx))
		return 0;

In that case, you can immediately see that the coder typed "0"
deliberately.  Direct returns are best.  I guess that's not directly
related to this code.  But I didn't know that until I read to the bottom
of the patch and I already had this rant prepared in my head ready to
go...

"error" is a crap label name because it doesn't tell you what the code
does.  A better name is "err_free_chan" or something which talks about
freeing the channel.

regards,
dan carpenter

Re: [PATCH v2 1/3] Drivers: hv: check vmbus_device_create() return value in vmbus_process_offer()

[Dan Carpenter]

On Mon, Jan 19, 2015 at 08:58:58PM +0300, Dan Carpenter wrote:
> "error" is a crap label name because it doesn't tell you what the code
> does.  A better name is "err_free_chan" or something which talks about
> freeing the channel.

If you choose your label names correctly, then most of the time you can
just read the code from top to bottom and understand it without skipping
back and forth.

regards,
dan carpenter

Re: [PATCH v2 1/3] Drivers: hv: check vmbus_device_create() return value in vmbus_process_offer()

[Vitaly Kuznetsov]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Mon, Jan 19, 2015 at 05:56:11PM +0100, Vitaly Kuznetsov wrote:
>> vmbus_device_create() result is not being checked in vmbus_process_offer() and
>> it can fail if kzalloc() fails. Add the check and do minor cleanup to avoid
>> additional duplication of "free_channel(); return;" block.
>> 
>> Reported-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> "out" is always a bad name for a label.  It's too vague.  It implies
> that the code uses "One Err" style error handling which is bug prone and
> I've ranted about that in the past so I won't here.  This kind of coding
> is buggier than direct returns.  But recently I've been looking at bugs
> where we return zero where the code should return a negative error code
> and, wow, do I hate "out" labels!
>
> 	if (function_whatever(xxx))
> 		goto out;
>
> [ thousands of lines removed. ]
>
> out:
> 	return ret;
>
> Oh crap...  Did the coder mean to return success or not???
>
> If you use a direct return then the code looks like:
>
> 	if (function_whatever(xxx))
> 		return 0;
>
> In that case, you can immediately see that the coder typed "0"
> deliberately.  Direct returns are best.  I guess that's not directly
> related to this code.  But I didn't know that until I read to the bottom
> of the patch and I already had this rant prepared in my head ready to
> go...

Thank you for your rant, Dan! It contains an explanation _why_ and so is
useful. However ... :-)

1) vmbus_process_offer() returns void so we won't forget to set proper
return code.
2) this patch is a preparation for the PATCH 3/3 where the label is
being used to do some useful (non-trivial) work. "Direct returns"
approach would require us to duplicate the code or move it to a function
and call it from all return places. I consider adding "out" label being
less evil.

Anyway, I can rename it to something less provocative in PATCH 3/3,
e.g. init_rescind.

>
> "error" is a crap label name because it doesn't tell you what the code
> does.  A better name is "err_free_chan" or something which talks about
> freeing the channel.

And here I have to completely agree with you, I'll rename it in v3.

>
> regards,
> dan carpenter

-- 
  Vitaly

Re: [PATCH v2 1/3] Drivers: hv: check vmbus_device_create() return value in vmbus_process_offer()

[Dan Carpenter]

Fine.  Fine.  I saw the 3/3 patch after I wrote the email.  Anyway I
hope that my ranting has planted a seed of doubt and from now you will
begin to view "out" labels with suspicion which will eventually flourish
into the flower of hatred.

regards,
dan carpenter

[PATCH v2 2/3] Drivers: hv: rename sc_lock to the more generic lock

[Vitaly Kuznetsov]

sc_lock spinlock in struct vmbus_channel is being used to not only protect the
sc_list field, e.g. vmbus_open() function uses it to implement test-and-set
access to the state field. Rename it to the more generic 'lock' and add the
description.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel.c      |  6 +++---
 drivers/hv/channel_mgmt.c | 10 +++++-----
 include/linux/hyperv.h    |  7 ++++++-
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 433f72a..8608ed1 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -73,14 +73,14 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
 	unsigned long flags;
 	int ret, t, err = 0;
 
-	spin_lock_irqsave(&newchannel->sc_lock, flags);
+	spin_lock_irqsave(&newchannel->lock, flags);
 	if (newchannel->state == CHANNEL_OPEN_STATE) {
 		newchannel->state = CHANNEL_OPENING_STATE;
 	} else {
-		spin_unlock_irqrestore(&newchannel->sc_lock, flags);
+		spin_unlock_irqrestore(&newchannel->lock, flags);
 		return -EINVAL;
 	}
-	spin_unlock_irqrestore(&newchannel->sc_lock, flags);
+	spin_unlock_irqrestore(&newchannel->lock, flags);
 
 	newchannel->onchannel_callback = onchannelcallback;
 	newchannel->channel_callback_context = context;
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 80590c6..c526ed2 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -146,7 +146,7 @@ static struct vmbus_channel *alloc_channel(void)
 		return NULL;
 
 	spin_lock_init(&channel->inbound_lock);
-	spin_lock_init(&channel->sc_lock);
+	spin_lock_init(&channel->lock);
 
 	INIT_LIST_HEAD(&channel->sc_list);
 	INIT_LIST_HEAD(&channel->percpu_list);
@@ -246,9 +246,9 @@ static void vmbus_process_rescind_offer(struct work_struct *work)
 		spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
 	} else {
 		primary_channel = channel->primary_channel;
-		spin_lock_irqsave(&primary_channel->sc_lock, flags);
+		spin_lock_irqsave(&primary_channel->lock, flags);
 		list_del(&channel->sc_list);
-		spin_unlock_irqrestore(&primary_channel->sc_lock, flags);
+		spin_unlock_irqrestore(&primary_channel->lock, flags);
 	}
 	free_channel(channel);
 }
@@ -323,9 +323,9 @@ static void vmbus_process_offer(struct work_struct *work)
 			 * Process the sub-channel.
 			 */
 			newchannel->primary_channel = channel;
-			spin_lock_irqsave(&channel->sc_lock, flags);
+			spin_lock_irqsave(&channel->lock, flags);
 			list_add_tail(&newchannel->sc_list, &channel->sc_list);
-			spin_unlock_irqrestore(&channel->sc_lock, flags);
+			spin_unlock_irqrestore(&channel->lock, flags);
 
 			if (newchannel->target_cpu != get_cpu()) {
 				put_cpu();
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 476c685..02dd978 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -722,7 +722,12 @@ struct vmbus_channel {
 	 */
 	void (*sc_creation_callback)(struct vmbus_channel *new_sc);
 
-	spinlock_t sc_lock;
+	/*
+	 * The spinlock to protect the structure. It is being used to protect
+	 * test-and-set access to various attributes of the structure as well
+	 * as all sc_list operations.
+	 */
+	spinlock_t lock;
 	/*
 	 * All Sub-channels of a primary channel are linked here.
 	 */
-- 
1.9.3

[PATCH v2 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer

[Vitaly Kuznetsov]

Commit 4b2f9abea52a ("staging: hv: convert channel_mgmt.c to not call
osd_schedule_callback")' was written under an assumption that we never receive
Rescind offer while we're still processing the initial Offer request. However,
the issue we fixed in 04a258c162a8 could be caused by this assumption not
always being true.

In particular, we need to protect against the following:
1) Receiving a Rescind offer after we do queue_work() for processing an Offer
   request and before we actually enter vmbus_process_offer(). work.func points
   to vmbus_process_offer() at this moment and in vmbus_onoffer_rescind() we do
   another queue_work() without a check so we'll enter vmbus_process_offer()
   twice.
2) Receiving a Rescind offer after we enter vmbus_process_offer() and
   especially after we set >state = CHANNEL_OPEN_STATE. Many things can go
   wrong in that case, e.g. we can call free_channel() while we're still using
   it.

Implement the required protection by changing work->func at the very end of
vmbus_process_offer() and checking work->func in vmbus_onoffer_rescind(). In
case we receive rescind offer during or before vmbus_process_offer() is done
we set rescind flag to true and we check it at the end of vmbus_process_offer()
so such offer will not get lost.

Suggested-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c526ed2..512b74a 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -279,9 +279,6 @@ static void vmbus_process_offer(struct work_struct *work)
 	int ret;
 	unsigned long flags;
 
-	/* The next possible work is rescind handling */
-	INIT_WORK(&newchannel->work, vmbus_process_rescind_offer);
-
 	/* Make sure this is a new offer */
 	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
 
@@ -383,6 +380,13 @@ static void vmbus_process_offer(struct work_struct *work)
 		goto error;
 	}
 out:
+	spin_lock_irqsave(&newchannel->lock, flags);
+	/* The next possible work is rescind handling */
+	INIT_WORK(&newchannel->work, vmbus_process_rescind_offer);
+	/* Check if rescind offer was already received */
+	if (newchannel->rescind)
+		queue_work(newchannel->controlwq, &newchannel->work);
+	spin_unlock_irqrestore(&newchannel->lock, flags);
 	return;
 error:
 	free_channel(newchannel);
@@ -520,6 +524,7 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
 {
 	struct vmbus_channel_rescind_offer *rescind;
 	struct vmbus_channel *channel;
+	unsigned long flags;
 
 	rescind = (struct vmbus_channel_rescind_offer *)hdr;
 	channel = relid2channel(rescind->child_relid);
@@ -528,11 +533,21 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
 		/* Just return here, no channel found */
 		return;
 
+	spin_lock_irqsave(&channel->lock, flags);
 	channel->rescind = true;
+	if (channel->work.func != vmbus_process_rescind_offer)
+		/*
+		 * We're still processing offer request, postponing rescind
+		 * offer processing. It will be done when vmbus_process_offer()
+		 * is done as it checks rescind flag.
+		 */
+		goto out;
 
 	/* work is initialized for vmbus_process_rescind_offer() from
 	 * vmbus_process_offer() where the channel got created */
 	queue_work(channel->controlwq, &channel->work);
+out:
+	spin_unlock_irqrestore(&channel->lock, flags);
 }
 
 /*
-- 
1.9.3