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

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

[Vitaly Kuznetsov]

This patch series is a renamed successor of "[PATCH] Drivers: hv: vmbus:
serialize Offer and Rescind offer processing".

Changes from v2:
- Rename labels in vmbus_process_offer() ("out" -> "done_init_rescind" in
  PATCH 3/3, "error" -> "err_free_chan" in PATCH 1/3) [Dan Carpenter]
- Invert condition, update comment, and remove "out" label in
  vmbus_onoffer_rescind()

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 | 50 ++++++++++++++++++++++++++++++++---------------
 include/linux/hyperv.h    |  7 ++++++-
 3 files changed, 43 insertions(+), 20 deletions(-)

-- 
1.9.3

[PATCH v3 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..01f2c2b 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 err_free_chan;
 	}
 
 	/*
@@ -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 err_free_chan;
 
 	/*
 	 * 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 err_free_chan;
 	}
+out:
+	return;
+err_free_chan:
+	free_channel(newchannel);
 }
 
 enum {
-- 
1.9.3

[PATCH v3 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 01f2c2b..c6fdd74 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 v3 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 | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c6fdd74..877a944 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);
 
@@ -341,7 +338,7 @@ static void vmbus_process_offer(struct work_struct *work)
 			if (channel->sc_creation_callback != NULL)
 				channel->sc_creation_callback(newchannel);
 
-			goto out;
+			goto done_init_rescind;
 		}
 
 		goto err_free_chan;
@@ -382,7 +379,14 @@ static void vmbus_process_offer(struct work_struct *work)
 		kfree(newchannel->device_obj);
 		goto err_free_chan;
 	}
-out:
+done_init_rescind:
+	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;
 err_free_chan:
 	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,20 @@ 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;
+	/*
+	 * channel->work.func != vmbus_process_rescind_offer means we are still
+	 * processing offer request and the rescind offer processing should be
+	 * postponed. It will be done at the very end of vmbus_process_offer()
+	 * as rescind flag is being checked there.
+	 */
+	if (channel->work.func == vmbus_process_rescind_offer)
+		/* work is initialized for vmbus_process_rescind_offer() from
+		 * vmbus_process_offer() where the channel got created */
+		queue_work(channel->controlwq, &channel->work);
 
-	/* work is initialized for vmbus_process_rescind_offer() from
-	 * vmbus_process_offer() where the channel got created */
-	queue_work(channel->controlwq, &channel->work);
+	spin_unlock_irqrestore(&channel->lock, flags);
 }
 
 /*
-- 
1.9.3

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 20, 2015 7:45 AM
> To: KY Srinivasan; devel@linuxdriverproject.org
> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
> Radim Krčmář; Dan Carpenter
> Subject: [PATCH v3 1/3] Drivers: hv: check vmbus_device_create() return
> value in vmbus_process_offer()
> 
> 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>

Thanks Vitaly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.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..01f2c2b 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 err_free_chan;
>  	}
> 
>  	/*
> @@ -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 err_free_chan;
> 
>  	/*
>  	 * 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 err_free_chan;
>  	}
> +out:
> +	return;
> +err_free_chan:
> +	free_channel(newchannel);
>  }
> 
>  enum {
> --
> 1.9.3

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 20, 2015 7:45 AM
> To: KY Srinivasan; devel@linuxdriverproject.org
> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
> Radim Krčmář; Dan Carpenter
> Subject: [PATCH v3 2/3] Drivers: hv: rename sc_lock to the more generic lock
> 
> 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>
Thanks Vitaly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.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
> 01f2c2b..c6fdd74 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

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

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4915 bytes --]



> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 20, 2015 7:45 AM
> To: KY Srinivasan; devel@linuxdriverproject.org
> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
> Radim Krčmář; Dan Carpenter
> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
> 
> 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>

Thanks Vitaly.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/channel_mgmt.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index
> c6fdd74..877a944 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);
> 
> @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct work_struct
> *work)
>  			if (channel->sc_creation_callback != NULL)
>  				channel->sc_creation_callback(newchannel);
> 
> -			goto out;
> +			goto done_init_rescind;
>  		}
> 
>  		goto err_free_chan;
> @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct work_struct
> *work)
>  		kfree(newchannel->device_obj);
>  		goto err_free_chan;
>  	}
> -out:
> +done_init_rescind:
> +	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;
>  err_free_chan:
>  	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,20 @@ 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;
> +	/*
> +	 * channel->work.func != vmbus_process_rescind_offer means we
> are still
> +	 * processing offer request and the rescind offer processing should
> be
> +	 * postponed. It will be done at the very end of
> vmbus_process_offer()
> +	 * as rescind flag is being checked there.
> +	 */
> +	if (channel->work.func == vmbus_process_rescind_offer)
> +		/* work is initialized for vmbus_process_rescind_offer() from
> +		 * vmbus_process_offer() where the channel got created */
> +		queue_work(channel->controlwq, &channel->work);
> 
> -	/* work is initialized for vmbus_process_rescind_offer() from
> -	 * vmbus_process_offer() where the channel got created */
> -	queue_work(channel->controlwq, &channel->work);
> +	spin_unlock_irqrestore(&channel->lock, flags);
>  }
> 
>  /*
> --
> 1.9.3

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Jason Wang]

On Tue, Jan 20, 2015 at 11:45 PM, Vitaly Kuznetsov 
<vkuznets@redhat.com> 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>
> ---
>  drivers/hv/channel_mgmt.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Acked-by: Jason Wang <jasowang@redhat.com>
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 2c59f03..01f2c2b 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 err_free_chan;
>  	}
>  
>  	/*
> @@ -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 err_free_chan;
>  
>  	/*
>  	 * 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 err_free_chan;
>  	}
> +out:
> +	return;
> +err_free_chan:
> +	free_channel(newchannel);
>  }
>  
>  enum {
> -- 
> 1.9.3
> 

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

[Jason Wang]

On Tue, Jan 20, 2015 at 11:45 PM, Vitaly Kuznetsov 
<vkuznets@redhat.com> wrote:
> 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(-)

Acked-by: Jason Wang <jasowang@redhat.com>
> 
> 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 01f2c2b..c6fdd74 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
> 

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

[Jason Wang]

On Tue, Jan 20, 2015 at 11:45 PM, Vitaly Kuznetsov 
<vkuznets@redhat.com> wrote:
> 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>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>
> 
>  drivers/hv/channel_mgmt.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index c6fdd74..877a944 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);
>  
> @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct 
> work_struct *work)
>  			if (channel->sc_creation_callback != NULL)
>  				channel->sc_creation_callback(newchannel);
>  
> -			goto out;
> +			goto done_init_rescind;
>  		}
>  
>  		goto err_free_chan;
> @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct 
> work_struct *work)
>  		kfree(newchannel->device_obj);
>  		goto err_free_chan;
>  	}
> -out:
> +done_init_rescind:
> +	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;
>  err_free_chan:
>  	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,20 @@ 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;
> +	/*
> +	 * channel->work.func != vmbus_process_rescind_offer means we are 
> still
> +	 * processing offer request and the rescind offer processing should 
> be
> +	 * postponed. It will be done at the very end of 
> vmbus_process_offer()
> +	 * as rescind flag is being checked there.
> +	 */
> +	if (channel->work.func == vmbus_process_rescind_offer)
> +		/* work is initialized for vmbus_process_rescind_offer() from
> +		 * vmbus_process_offer() where the channel got created */
> +		queue_work(channel->controlwq, &channel->work);
>  
> -	/* work is initialized for vmbus_process_rescind_offer() from
> -	 * vmbus_process_offer() where the channel got created */
> -	queue_work(channel->controlwq, &channel->work);
> +	spin_unlock_irqrestore(&channel->lock, flags);
>  }
>  
>  /*
> -- 
> 1.9.3
> 

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

[Dexuan Cui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5550 bytes --]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, January 20, 2015 23:45 PM
> To: KY Srinivasan; devel@linuxdriverproject.org
> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
> Radim Krčmář; Dan Carpenter
> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
> 
> 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 | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index c6fdd74..877a944 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);
> 
> @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct work_struct
> *work)
>  			if (channel->sc_creation_callback != NULL)
>  				channel->sc_creation_callback(newchannel);
> 
> -			goto out;
> +			goto done_init_rescind;
>  		}
> 
>  		goto err_free_chan;
> @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct work_struct
> *work)
>  		kfree(newchannel->device_obj);
>  		goto err_free_chan;
>  	}
> -out:
> +done_init_rescind:
> +	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;
>  err_free_chan:
>  	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,20 @@ 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;
> +	/*
> +	 * channel->work.func != vmbus_process_rescind_offer means we
> are still
> +	 * processing offer request and the rescind offer processing should
> be
> +	 * postponed. It will be done at the very end of
> vmbus_process_offer()
> +	 * as rescind flag is being checked there.
> +	 */
> +	if (channel->work.func == vmbus_process_rescind_offer)
> +		/* work is initialized for vmbus_process_rescind_offer() from
> +		 * vmbus_process_offer() where the channel got created */
> +		queue_work(channel->controlwq, &channel->work);
> 
> -	/* work is initialized for vmbus_process_rescind_offer() from
> -	 * vmbus_process_offer() where the channel got created */
> -	queue_work(channel->controlwq, &channel->work);
> +	spin_unlock_irqrestore(&channel->lock, flags);
>  }
> 
>  /*
> --

Hi Vitaly and all,
I have 2 questions:
In vmbus_process_offer(),  in the cases of "goto err_free_chan",
should we consider the possibility a rescind message could be pending for 
the new channel?
In the cases,  because we don't run 
"INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
vmbus_onoffer_rescind() will do nothing and as a result, 
vmbus_process_rescind_offer() won't be invoked.

Question 2: in vmbus_process_offer(), in the case
vmbus_device_register() fails, we'll run
"list_del(&newchannel->listentry);" -- just after this line,
 what will happen at this time if relid2channel() returns NULL
in vmbus_onoffer_rescind()?

I think we'll lose the rescind message. 

Thanks,
-- Dexuan
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Tuesday, January 20, 2015 23:45 PM
>> To: KY Srinivasan; devel@linuxdriverproject.org
>> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
>> Radim Krčmář; Dan Carpenter
>> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
>> 
>> 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 | 30 ++++++++++++++++++++++--------
>>  1 file changed, 22 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index c6fdd74..877a944 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);
>> 
>> @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct work_struct
>> *work)
>>  			if (channel->sc_creation_callback != NULL)
>>  				channel->sc_creation_callback(newchannel);
>> 
>> -			goto out;
>> +			goto done_init_rescind;
>>  		}
>> 
>>  		goto err_free_chan;
>> @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct work_struct
>> *work)
>>  		kfree(newchannel->device_obj);
>>  		goto err_free_chan;
>>  	}
>> -out:
>> +done_init_rescind:
>> +	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;
>>  err_free_chan:
>>  	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,20 @@ 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;
>> +	/*
>> +	 * channel->work.func != vmbus_process_rescind_offer means we
>> are still
>> +	 * processing offer request and the rescind offer processing should
>> be
>> +	 * postponed. It will be done at the very end of
>> vmbus_process_offer()
>> +	 * as rescind flag is being checked there.
>> +	 */
>> +	if (channel->work.func == vmbus_process_rescind_offer)
>> +		/* work is initialized for vmbus_process_rescind_offer() from
>> +		 * vmbus_process_offer() where the channel got created */
>> +		queue_work(channel->controlwq, &channel->work);
>> 
>> -	/* work is initialized for vmbus_process_rescind_offer() from
>> -	 * vmbus_process_offer() where the channel got created */
>> -	queue_work(channel->controlwq, &channel->work);
>> +	spin_unlock_irqrestore(&channel->lock, flags);
>>  }
>> 
>>  /*
>> --
>
> Hi Vitaly and all,
> I have 2 questions:
> In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> should we consider the possibility a rescind message could be pending for 
> the new channel?
> In the cases,  because we don't run 
> "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> vmbus_onoffer_rescind() will do nothing and as a result, 
> vmbus_process_rescind_offer() won't be invoked.

Yes, but processing the rescind offer results in freeing the channel
(and this processing supposes the channel wasn't freed before) so
there is no difference... or is it?

>
> Question 2: in vmbus_process_offer(), in the case
> vmbus_device_register() fails, we'll run
> "list_del(&newchannel->listentry);" -- just after this line,
>  what will happen at this time if relid2channel() returns NULL
> in vmbus_onoffer_rescind()?
>
> I think we'll lose the rescind message. 
>

Yes, but same logic applies - we already freed the channes so no rescind
proccessing required. If we still need to do something we need to
add support for already freed channel to the rescind offer processing path.

> Thanks,
> -- Dexuan

-- 
  Vitaly

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

[Dexuan Cui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2587 bytes --]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, January 28, 2015 20:09 PM
> To: Dexuan Cui
> Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang; linux-
> kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
> Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
> offer
> 
> Dexuan Cui <decui@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Tuesday, January 20, 2015 23:45 PM
> >> To: KY Srinivasan; devel@linuxdriverproject.org
> >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
> >> Radim Krčmář; Dan Carpenter
> >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
> offer
> ...
> >
> > Hi Vitaly and all,
> > I have 2 questions:
> > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> > should we consider the possibility a rescind message could be pending for
> > the new channel?
> > In the cases,  because we don't run
> > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> > vmbus_onoffer_rescind() will do nothing and as a result,
> > vmbus_process_rescind_offer() won't be invoked.
> 
> Yes, but processing the rescind offer results in freeing the channel
> (and this processing supposes the channel wasn't freed before) so
> there is no difference... or is it?
> 
> >
> > Question 2: in vmbus_process_offer(), in the case
> > vmbus_device_register() fails, we'll run
> > "list_del(&newchannel->listentry);" -- just after this line,
> >  what will happen at this time if relid2channel() returns NULL
> > in vmbus_onoffer_rescind()?
> >
> > I think we'll lose the rescind message.
> >
> 
> Yes, but same logic applies - we already freed the channes so no rescind
> proccessing required.
free_channel() and vmbus_process_rescind_offer() are different, because
 the latter does more work, e.g., sending the host a message 
CHANNELMSG_RELID_RELEASED.

In the cases of "goto err_free_chan" + "a pending rescind message",
the host may expect the message CHANNELMSG_RELID_RELEASED and
could reoffer the channel once the message is received.

It would be better if the VM doesn't lose the rescind message here. :-)

>  If we still need to do something we need to
> add support for already freed channel to the rescind offer processing path.
> 

Thanks,
-- Dexuan
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Vitaly Kuznetsov]

Dexuan Cui <decui@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Wednesday, January 28, 2015 20:09 PM
>> To: Dexuan Cui
>> Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang; linux-
>> kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
>> Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
>> offer
>> 
>> Dexuan Cui <decui@microsoft.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> >> Sent: Tuesday, January 20, 2015 23:45 PM
>> >> To: KY Srinivasan; devel@linuxdriverproject.org
>> >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;
>> >> Radim Krčmář; Dan Carpenter
>> >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
>> offer
>> ...
>> >
>> > Hi Vitaly and all,
>> > I have 2 questions:
>> > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
>> > should we consider the possibility a rescind message could be pending for
>> > the new channel?
>> > In the cases,  because we don't run
>> > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
>> > vmbus_onoffer_rescind() will do nothing and as a result,
>> > vmbus_process_rescind_offer() won't be invoked.
>> 
>> Yes, but processing the rescind offer results in freeing the channel
>> (and this processing supposes the channel wasn't freed before) so
>> there is no difference... or is it?
>> 
>> >
>> > Question 2: in vmbus_process_offer(), in the case
>> > vmbus_device_register() fails, we'll run
>> > "list_del(&newchannel->listentry);" -- just after this line,
>> >  what will happen at this time if relid2channel() returns NULL
>> > in vmbus_onoffer_rescind()?
>> >
>> > I think we'll lose the rescind message.
>> >
>> 
>> Yes, but same logic applies - we already freed the channes so no rescind
>> proccessing required.
> free_channel() and vmbus_process_rescind_offer() are different, because
>  the latter does more work, e.g., sending the host a message 
> CHANNELMSG_RELID_RELEASED.
>
> In the cases of "goto err_free_chan" + "a pending rescind message",
> the host may expect the message CHANNELMSG_RELID_RELEASED and
> could reoffer the channel once the message is received.
>
> It would be better if the VM doesn't lose the rescind message
> here. :-)

Ah, I see, CHANNELMSG_RELID_RELEASED is expected from us in any
case. I'll doing that in a separate patch is noone objects.

Thanks for the review,

>
>>  If we still need to do something we need to
>> add support for already freed channel to the rescind offer processing path.
>> 
>
> Thanks,
> -- Dexuan

-- 
  Vitaly

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

[Jason Wang]

On Wed, Jan 28, 2015 at 7:57 PM, Dexuan Cui <decui@microsoft.com> wrote:
>>  -----Original Message-----
>>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>  Sent: Tuesday, January 20, 2015 23:45 PM
>>  To: KY Srinivasan; devel@linuxdriverproject.org
>>  Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason 
>> Wang;
>>  Radim Krčmář; Dan Carpenter
>>  Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and 
>> Rescind offer
>>  
>>  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 | 30 ++++++++++++++++++++++--------
>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>  
>>  diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>>  index c6fdd74..877a944 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);
>>  
>>  @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct 
>> work_struct
>>  *work)
>>   			if (channel->sc_creation_callback != NULL)
>>   				channel->sc_creation_callback(newchannel);
>>  
>>  -			goto out;
>>  +			goto done_init_rescind;
>>   		}
>>  
>>   		goto err_free_chan;
>>  @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct 
>> work_struct
>>  *work)
>>   		kfree(newchannel->device_obj);
>>   		goto err_free_chan;
>>   	}
>>  -out:
>>  +done_init_rescind:
>>  +	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;
>>   err_free_chan:
>>   	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,20 @@ 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;
>>  +	/*
>>  +	 * channel->work.func != vmbus_process_rescind_offer means we
>>  are still
>>  +	 * processing offer request and the rescind offer processing 
>> should
>>  be
>>  +	 * postponed. It will be done at the very end of
>>  vmbus_process_offer()
>>  +	 * as rescind flag is being checked there.
>>  +	 */
>>  +	if (channel->work.func == vmbus_process_rescind_offer)
>>  +		/* work is initialized for vmbus_process_rescind_offer() from
>>  +		 * vmbus_process_offer() where the channel got created */
>>  +		queue_work(channel->controlwq, &channel->work);
>>  
>>  -	/* work is initialized for vmbus_process_rescind_offer() from
>>  -	 * vmbus_process_offer() where the channel got created */
>>  -	queue_work(channel->controlwq, &channel->work);
>>  +	spin_unlock_irqrestore(&channel->lock, flags);
>>   }
>>  
>>   /*
>>  --
> 
> Hi Vitaly and all,
> I have 2 questions:
> In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> should we consider the possibility a rescind message could be pending 
> for 
> the new channel?
> In the cases,  because we don't run 
> "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> vmbus_onoffer_rescind() will do nothing and as a result, 
> vmbus_process_rescind_offer() won't be invoked.
> 
> Question 2: in vmbus_process_offer(), in the case
> vmbus_device_register() fails, we'll run
> "list_del(&newchannel->listentry);" -- just after this line,
>  what will happen at this time if relid2channel() returns NULL
> in vmbus_onoffer_rescind()?
> 
> I think we'll lose the rescind message. 

If CHANNELMSG_RELID_RELEASED is mandatory, need INIT_WORK during 
onoffer_rescind() unconditionally and and we need post this message 
without the help of relid2channel() since:

- relid2channel() only valid when the channel was added to list, so 
either in the case of question 2 or before list_add_tail() in 
vmbus_process_offer()
- the channel rescind offer message has a relid

> 
> 
> Thanks,
> -- Dexuan

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

[Jason Wang]

On Wed, Jan 28, 2015 at 8:51 PM, Dexuan Cui <decui@microsoft.com> wrote:
>>  -----Original Message-----
>>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>  Sent: Wednesday, January 28, 2015 20:09 PM
>>  To: Dexuan Cui
>>  Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang; 
>> linux-
>>  kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
>>  Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and 
>> Rescind
>>  offer
>>  
>>  Dexuan Cui <decui@microsoft.com> writes:
>>  
>>  >> -----Original Message-----
>>  >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>  >> Sent: Tuesday, January 20, 2015 23:45 PM
>>  >> To: KY Srinivasan; devel@linuxdriverproject.org
>>  >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; 
>> Jason Wang;
>>  >> Radim Krčmář; Dan Carpenter
>>  >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and 
>> Rescind
>>  offer
>>  ...
>>  >
>>  > Hi Vitaly and all,
>>  > I have 2 questions:
>>  > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
>>  > should we consider the possibility a rescind message could be 
>> pending for
>>  > the new channel?
>>  > In the cases,  because we don't run
>>  > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
>>  > vmbus_onoffer_rescind() will do nothing and as a result,
>>  > vmbus_process_rescind_offer() won't be invoked.
>>  
>>  Yes, but processing the rescind offer results in freeing the channel
>>  (and this processing supposes the channel wasn't freed before) so
>>  there is no difference... or is it?
>>  
>>  >
>>  > Question 2: in vmbus_process_offer(), in the case
>>  > vmbus_device_register() fails, we'll run
>>  > "list_del(&newchannel->listentry);" -- just after this line,
>>  >  what will happen at this time if relid2channel() returns NULL
>>  > in vmbus_onoffer_rescind()?
>>  >
>>  > I think we'll lose the rescind message.
>>  >
>>  
>>  Yes, but same logic applies - we already freed the channes so no 
>> rescind
>>  proccessing required.
> free_channel() and vmbus_process_rescind_offer() are different, 
> because
>  the latter does more work, e.g., sending the host a message 
> CHANNELMSG_RELID_RELEASED.
> 
> In the cases of "goto err_free_chan" + "a pending rescind message",
> the host may expect the message CHANNELMSG_RELID_RELEASED and
> could reoffer the channel once the message is received.
> 
> It would be better if the VM doesn't lose the rescind message here. 
> :-)

It's interesting that rescind needs a ack from guest. But looks like 
the offer does not need it? Is there a spec for this for us for 
reference?

Thanks
> 
> 
>>   If we still need to do something we need to
>>  add support for already freed channel to the rescind offer 
>> processing path.
>>  
> 
> Thanks,
> -- Dexuan

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

[Jason Wang]

On Wed, Jan 28, 2015 at 9:09 PM, Vitaly Kuznetsov <vkuznets@redhat.com> 
wrote:
> Dexuan Cui <decui@microsoft.com> writes:
> 
>>>  -----Original Message-----
>>>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>>  Sent: Wednesday, January 28, 2015 20:09 PM
>>>  To: Dexuan Cui
>>>  Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang; 
>>> linux-
>>>  kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
>>>  Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer 
>>> and Rescind
>>>  offer
>>>  
>>>  Dexuan Cui <decui@microsoft.com> writes:
>>>  
>>>  >> -----Original Message-----
>>>  >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>>>  >> Sent: Tuesday, January 20, 2015 23:45 PM
>>>  >> To: KY Srinivasan; devel@linuxdriverproject.org
>>>  >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; 
>>> Jason Wang;
>>>  >> Radim Krčmář; Dan Carpenter
>>>  >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and 
>>> Rescind
>>>  offer
>>>  ...
>>>  >
>>>  > Hi Vitaly and all,
>>>  > I have 2 questions:
>>>  > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
>>>  > should we consider the possibility a rescind message could be 
>>> pending for
>>>  > the new channel?
>>>  > In the cases,  because we don't run
>>>  > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
>>>  > vmbus_onoffer_rescind() will do nothing and as a result,
>>>  > vmbus_process_rescind_offer() won't be invoked.
>>>  
>>>  Yes, but processing the rescind offer results in freeing the 
>>> channel
>>>  (and this processing supposes the channel wasn't freed before) so
>>>  there is no difference... or is it?
>>>  
>>>  >
>>>  > Question 2: in vmbus_process_offer(), in the case
>>>  > vmbus_device_register() fails, we'll run
>>>  > "list_del(&newchannel->listentry);" -- just after this line,
>>>  >  what will happen at this time if relid2channel() returns NULL
>>>  > in vmbus_onoffer_rescind()?
>>>  >
>>>  > I think we'll lose the rescind message.
>>>  >
>>>  
>>>  Yes, but same logic applies - we already freed the channes so no 
>>> rescind
>>>  proccessing required.
>>  free_channel() and vmbus_process_rescind_offer() are different, 
>> because
>>   the latter does more work, e.g., sending the host a message 
>>  CHANNELMSG_RELID_RELEASED.
>> 
>>  In the cases of "goto err_free_chan" + "a pending rescind message",
>>  the host may expect the message CHANNELMSG_RELID_RELEASED and
>>  could reoffer the channel once the message is received.
>> 
>>  It would be better if the VM doesn't lose the rescind message
>>  here. :-)
> 
> Ah, I see, CHANNELMSG_RELID_RELEASED is expected from us in any
> case. I'll doing that in a separate patch is noone objects.

All the evil come from the un-serialized processing of message. I 
wonder whether we can do all the processing in workqueue and then those 
were automatically serialized.

> 
> Thanks for the review,
> 
>> 
>>>   If we still need to do something we need to
>>>  add support for already freed channel to the rescind offer 
>>> processing path.
>>>  
>> 
>>  Thanks,
>>  -- Dexuan
> 
> -- 
>   Vitaly
> --
> 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/

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

[Dexuan Cui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3683 bytes --]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 29, 2015 18:09 PM
> To: Dexuan Cui
> Cc: Vitaly Kuznetsov; KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang;
> linux-kernel@vger.kernel.org; Radim Krčmář; Dan Carpenter
> Subject: RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind offer
> 
> 
> 
> On Wed, Jan 28, 2015 at 8:51 PM, Dexuan Cui <decui@microsoft.com> wrote:
> >>  -----Original Message-----
> >>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >>  Sent: Wednesday, January 28, 2015 20:09 PM
> >>  To: Dexuan Cui
> >>  Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang;
> >> linux-
> >>  kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
> >>  Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and
> >> Rescind
> >>  offer
> >>
> >>  Dexuan Cui <decui@microsoft.com> writes:
> >>
> >>  >> -----Original Message-----
> >>  >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >>  >> Sent: Tuesday, January 20, 2015 23:45 PM
> >>  >> To: KY Srinivasan; devel@linuxdriverproject.org
> >>  >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui;
> >> Jason Wang;
> >>  >> Radim Krčmář; Dan Carpenter
> >>  >> Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and
> >> Rescind
> >>  offer
> >>  ...
> >>  >
> >>  > Hi Vitaly and all,
> >>  > I have 2 questions:
> >>  > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> >>  > should we consider the possibility a rescind message could be
> >> pending for
> >>  > the new channel?
> >>  > In the cases,  because we don't run
> >>  > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> >>  > vmbus_onoffer_rescind() will do nothing and as a result,
> >>  > vmbus_process_rescind_offer() won't be invoked.
> >>
> >>  Yes, but processing the rescind offer results in freeing the channel
> >>  (and this processing supposes the channel wasn't freed before) so
> >>  there is no difference... or is it?
> >>
> >>  >
> >>  > Question 2: in vmbus_process_offer(), in the case
> >>  > vmbus_device_register() fails, we'll run
> >>  > "list_del(&newchannel->listentry);" -- just after this line,
> >>  >  what will happen at this time if relid2channel() returns NULL
> >>  > in vmbus_onoffer_rescind()?
> >>  >
> >>  > I think we'll lose the rescind message.
> >>  >
> >>
> >>  Yes, but same logic applies - we already freed the channes so no
> >> rescind
> >>  proccessing required.
> > free_channel() and vmbus_process_rescind_offer() are different,
> > because
> >  the latter does more work, e.g., sending the host a message
> > CHANNELMSG_RELID_RELEASED.
> >
> > In the cases of "goto err_free_chan" + "a pending rescind message",
> > the host may expect the message CHANNELMSG_RELID_RELEASED and
> > could reoffer the channel once the message is received.
> >
> > It would be better if the VM doesn't lose the rescind message here.
> > :-)
> 
> It's interesting that rescind needs a ack from guest. But looks like
> the offer does not need it? Is there a spec for this for us for
> reference?

My understanding is:
The host may reuse the same relid after the VM acks the rescind message.

I don't have a VMBus spec either.

> >
> >
> >>   If we still need to do something we need to
> >>  add support for already freed channel to the rescind offer
> >> processing path.
> >>
This sounds reasonable to me.
Error handling is always full of  various corner cases...

Thanks,
-- Dexuan
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7172 bytes --]



> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 29, 2015 2:07 AM
> To: Dexuan Cui
> Cc: Vitaly Kuznetsov; KY Srinivasan; devel@linuxdriverproject.org; Haiyang
> Zhang; linux-kernel@vger.kernel.org; Radim Krčmář; Dan Carpenter
> Subject: RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
> offer
> 
> 
> 
> On Wed, Jan 28, 2015 at 7:57 PM, Dexuan Cui <decui@microsoft.com> wrote:
> >>  -----Original Message-----
> >>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >>  Sent: Tuesday, January 20, 2015 23:45 PM
> >>  To: KY Srinivasan; devel@linuxdriverproject.org
> >>  Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Jason
> >> Wang;  Radim Krčmář; Dan Carpenter
> >>  Subject: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and
> >> Rescind offer
> >>
> >>  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 | 30 ++++++++++++++++++++++--------
> >>   1 file changed, 22 insertions(+), 8 deletions(-)
> >>
> >>  diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> >> index c6fdd74..877a944 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);
> >>
> >>  @@ -341,7 +338,7 @@ static void vmbus_process_offer(struct
> >> work_struct
> >>  *work)
> >>   			if (channel->sc_creation_callback != NULL)
> >>   				channel->sc_creation_callback(newchannel);
> >>
> >>  -			goto out;
> >>  +			goto done_init_rescind;
> >>   		}
> >>
> >>   		goto err_free_chan;
> >>  @@ -382,7 +379,14 @@ static void vmbus_process_offer(struct
> >> work_struct
> >>  *work)
> >>   		kfree(newchannel->device_obj);
> >>   		goto err_free_chan;
> >>   	}
> >>  -out:
> >>  +done_init_rescind:
> >>  +	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;
> >>   err_free_chan:
> >>   	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,20 @@ 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;
> >>  +	/*
> >>  +	 * channel->work.func != vmbus_process_rescind_offer means we
> >>  are still
> >>  +	 * processing offer request and the rescind offer processing
> >> should
> >>  be
> >>  +	 * postponed. It will be done at the very end of
> >>  vmbus_process_offer()
> >>  +	 * as rescind flag is being checked there.
> >>  +	 */
> >>  +	if (channel->work.func == vmbus_process_rescind_offer)
> >>  +		/* work is initialized for vmbus_process_rescind_offer() from
> >>  +		 * vmbus_process_offer() where the channel got created */
> >>  +		queue_work(channel->controlwq, &channel->work);
> >>
> >>  -	/* work is initialized for vmbus_process_rescind_offer() from
> >>  -	 * vmbus_process_offer() where the channel got created */
> >>  -	queue_work(channel->controlwq, &channel->work);
> >>  +	spin_unlock_irqrestore(&channel->lock, flags);
> >>   }
> >>
> >>   /*
> >>  --
> >
> > Hi Vitaly and all,
> > I have 2 questions:
> > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> > should we consider the possibility a rescind message could be pending
> > for the new channel?
> > In the cases,  because we don't run
> > "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> > vmbus_onoffer_rescind() will do nothing and as a result,
> > vmbus_process_rescind_offer() won't be invoked.
> >
> > Question 2: in vmbus_process_offer(), in the case
> > vmbus_device_register() fails, we'll run
> > "list_del(&newchannel->listentry);" -- just after this line,  what
> > will happen at this time if relid2channel() returns NULL in
> > vmbus_onoffer_rescind()?
> >
> > I think we'll lose the rescind message.
> 
> If CHANNELMSG_RELID_RELEASED is mandatory, need INIT_WORK during
> onoffer_rescind() unconditionally and and we need post this message
> without the help of relid2channel() since:
> 
> - relid2channel() only valid when the channel was added to list, so either in
> the case of question 2 or before list_add_tail() in
> vmbus_process_offer()
> - the channel rescind offer message has a relid
> 
Once the channel is rescinded from the host, we need to send the RELID_RELEASED message to the host
for the host to potentially resend the offer.

K. Y 
> >
> >
> > Thanks,
> > -- Dexuan

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4280 bytes --]



> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, January 29, 2015 8:21 PM
> To: Jason Wang
> Cc: Vitaly Kuznetsov; KY Srinivasan; devel@linuxdriverproject.org; Haiyang
> Zhang; linux-kernel@vger.kernel.org; Radim Krčmář; Dan Carpenter
> Subject: RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and Rescind
> offer
> 
> > -----Original Message-----
> > From: Jason Wang [mailto:jasowang@redhat.com]
> > Sent: Thursday, January 29, 2015 18:09 PM
> > To: Dexuan Cui
> > Cc: Vitaly Kuznetsov; KY Srinivasan; devel@linuxdriverproject.org;
> > Haiyang Zhang; linux-kernel@vger.kernel.org; Radim Krčmář; Dan
> > Carpenter
> > Subject: RE: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer and
> > Rescind offer
> >
> >
> >
> > On Wed, Jan 28, 2015 at 8:51 PM, Dexuan Cui <decui@microsoft.com>
> wrote:
> > >>  -----Original Message-----
> > >>  From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > >>  Sent: Wednesday, January 28, 2015 20:09 PM
> > >>  To: Dexuan Cui
> > >>  Cc: KY Srinivasan; devel@linuxdriverproject.org; Haiyang Zhang;
> > >> linux-
> > >>  kernel@vger.kernel.org; Jason Wang; Radim Krčmář; Dan Carpenter
> > >>  Subject: Re: [PATCH v3 3/3] Drivers: hv: vmbus: serialize Offer
> > >> and Rescind  offer
> > >>
> > >>  Dexuan Cui <decui@microsoft.com> writes:
> > >>
> > >>  >> -----Original Message-----
> > >>  >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]  >> Sent:
> > >> Tuesday, January 20, 2015 23:45 PM  >> To: KY Srinivasan;
> > >> devel@linuxdriverproject.org  >> Cc: Haiyang Zhang;
> > >> linux-kernel@vger.kernel.org; Dexuan Cui; Jason Wang;  >> Radim
> > >> Krčmář; Dan Carpenter  >> Subject: [PATCH v3 3/3] Drivers: hv:
> > >> vmbus: serialize Offer and Rescind  offer  ...
> > >>  >
> > >>  > Hi Vitaly and all,
> > >>  > I have 2 questions:
> > >>  > In vmbus_process_offer(),  in the cases of "goto err_free_chan",
> > >> > should we consider the possibility a rescind message could be
> > >> pending for  > the new channel?
> > >>  > In the cases,  because we don't run  >
> > >> "INIT_WORK(&newchannel->work, vmbus_process_rescind_offer); ",
> >
> > >> vmbus_onoffer_rescind() will do nothing and as a result,  >
> > >> vmbus_process_rescind_offer() won't be invoked.
> > >>
> > >>  Yes, but processing the rescind offer results in freeing the
> > >> channel  (and this processing supposes the channel wasn't freed
> > >> before) so  there is no difference... or is it?
> > >>
> > >>  >
> > >>  > Question 2: in vmbus_process_offer(), in the case  >
> > >> vmbus_device_register() fails, we'll run  >
> > >> "list_del(&newchannel->listentry);" -- just after this line,  >
> > >> what will happen at this time if relid2channel() returns NULL  > in
> > >> vmbus_onoffer_rescind()?
> > >>  >
> > >>  > I think we'll lose the rescind message.
> > >>  >
> > >>
> > >>  Yes, but same logic applies - we already freed the channes so no
> > >> rescind  proccessing required.
> > > free_channel() and vmbus_process_rescind_offer() are different,
> > > because  the latter does more work, e.g., sending the host a message
> > > CHANNELMSG_RELID_RELEASED.
> > >
> > > In the cases of "goto err_free_chan" + "a pending rescind message",
> > > the host may expect the message CHANNELMSG_RELID_RELEASED and
> could
> > > reoffer the channel once the message is received.
> > >
> > > It would be better if the VM doesn't lose the rescind message here.
> > > :-)
> >
> > It's interesting that rescind needs a ack from guest. But looks like
> > the offer does not need it? Is there a  spec for this for us for
> > reference?
> 
> My understanding is:
> The host may reuse the same relid after the VM acks the rescind message.
> 
> I don't have a VMBus spec either.

That is correct. This change was I think introduced recently (requiring the guest to release the RelID).

K. Y
> 
> > >
> > >
> > >>   If we still need to do something we need to  add support for
> > >> already freed channel to the rescind offer processing path.
> > >>
> This sounds reasonable to me.
> Error handling is always full of  various corner cases...
> 
> Thanks,
> -- Dexuan

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥