[PATCH 0/2] Drivers: vmbus: Fix rescind handling in uio_hv_generic

[PATCH 0/2] Drivers: vmbus: Fix rescind handling in uio_hv_generic

[Naman Jain]

Fix a few issues in rescind handling in uio_hv_generic driver.
Patches are based on linux-next-rc4 tip.

Steps to reproduce issue:
* Probe uio_hv_generic driver and create channels to use fcopy
* Disable the guest service on host and then Enable it.
or
* repeatedly do cat "/dev/uioX" on the device created for fcopy.

Naman Jain (1):
  Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

Saurabh Sengar (1):
  uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

 drivers/hv/vmbus_drv.c       |  1 +
 drivers/uio/uio_hv_generic.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)


base-commit: 469f1bad3c1c6e268059f78c0eec7e9552b3894c
-- 
2.34.1

[PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

[Naman Jain]

From: Saurabh Sengar <ssengar@linux.microsoft.com>

For primary VMBus channels primary_channel pointer is always NULL. This
pointer is valid only for the secondry channels.

Fix NULL pointer dereference by retrieving the device_obj from the parent
in the absence of a valid primary_channel pointer.

Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
---
 drivers/uio/uio_hv_generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index b45653752301..c99890c16d29 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -109,7 +109,8 @@ static void hv_uio_channel_cb(void *context)
  */
 static void hv_uio_rescind(struct vmbus_channel *channel)
 {
-	struct hv_device *hv_dev = channel->primary_channel->device_obj;
+	struct hv_device *hv_dev = channel->primary_channel ?
+				   channel->primary_channel->device_obj : channel->device_obj;
 	struct hv_uio_private_data *pdata = hv_get_drvdata(hv_dev);
 
 	/*
-- 
2.34.1

Re: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

[Greg Kroah-Hartman]

On Thu, Aug 22, 2024 at 04:39:11PM +0530, Naman Jain wrote:
> From: Saurabh Sengar <ssengar@linux.microsoft.com>
> 
> For primary VMBus channels primary_channel pointer is always NULL. This
> pointer is valid only for the secondry channels.
> 
> Fix NULL pointer dereference by retrieving the device_obj from the parent
> in the absence of a valid primary_channel pointer.
> 
> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
>  drivers/uio/uio_hv_generic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

[Michael Kelley]

From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> 
> From: Saurabh Sengar <ssengar@linux.microsoft.com>
> 
> For primary VMBus channels primary_channel pointer is always NULL. This
> pointer is valid only for the secondry channels.
> 
> Fix NULL pointer dereference by retrieving the device_obj from the parent
> in the absence of a valid primary_channel pointer.
> 
> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
>  drivers/uio/uio_hv_generic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index b45653752301..c99890c16d29 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -109,7 +109,8 @@ static void hv_uio_channel_cb(void *context)
>   */
>  static void hv_uio_rescind(struct vmbus_channel *channel)
>  {
> -	struct hv_device *hv_dev = channel->primary_channel->device_obj;
> +	struct hv_device *hv_dev = channel->primary_channel ?
> +				   channel->primary_channel->device_obj : channel->device_obj;

It looks to me like hv_uio_rescind() is called only for the primary
channel. That makes sense, because waking up the reader should
presumably be done once for the device, not once for each channel.

Rather than generalizing the function so it works for both primary
and secondary channels, I'd suggest checking if the channel is a
secondary channel. If so, output a warning message or do WARN(),
and then return immediately, as some there's some kind of
programming error.

Looking at the history of the code, it appears that rescinding a UIO
device could never have worked. Is that your conclusion as well,
or am I missing something?

Michael

Re: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

[Naman Jain]

On 8/25/2024 8:21 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
>>
>> From: Saurabh Sengar <ssengar@linux.microsoft.com>
>>
>> For primary VMBus channels primary_channel pointer is always NULL. This
>> pointer is valid only for the secondry channels.
>>
>> Fix NULL pointer dereference by retrieving the device_obj from the parent
>> in the absence of a valid primary_channel pointer.
>>
>> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
>> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   drivers/uio/uio_hv_generic.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
>> index b45653752301..c99890c16d29 100644
>> --- a/drivers/uio/uio_hv_generic.c
>> +++ b/drivers/uio/uio_hv_generic.c
>> @@ -109,7 +109,8 @@ static void hv_uio_channel_cb(void *context)
>>    */
>>   static void hv_uio_rescind(struct vmbus_channel *channel)
>>   {
>> -	struct hv_device *hv_dev = channel->primary_channel->device_obj;
>> +	struct hv_device *hv_dev = channel->primary_channel ?
>> +				   channel->primary_channel->device_obj : channel->device_obj;
> 
> It looks to me like hv_uio_rescind() is called only for the primary
> channel. That makes sense, because waking up the reader should
> presumably be done once for the device, not once for each channel.
> 
> Rather than generalizing the function so it works for both primary
> and secondary channels, I'd suggest checking if the channel is a
> secondary channel. If so, output a warning message or do WARN(),
> and then return immediately, as some there's some kind of
> programming error.

Thanks for reviewing Michael. By design, there is different handling for 
secondary channel rescind in channel_mgmt.c [1] and this callback is not 
expected to be called. I will make the change in my next patch to make 
it a warning.


[1]
         } else if (channel->primary_channel != NULL) {
                 /*
                  * Sub-channel is being rescinded. Following is the channel
                  * close sequence when initiated from the driveri (refer to
                  * vmbus_close() for details):
                  * 1. Close all sub-channels first
                  * 2. Then close the primary channel.
                  */

> 
> Looking at the history of the code, it appears that rescinding a UIO
> device could never have worked. Is that your conclusion as well,
> or am I missing something?
> 
> Michael

Most likely, yes.


Regards,
Naman

[PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Naman Jain]

Rescind offer handling relies on rescind callbacks for some of the
resources cleanup, if they are registered. It does not unregister
vmbus device for the primary channel closure, when callback is
registered.
Add logic to unregister vmbus for the primary channel in rescind callback
to ensure channel removal and relid release, and to ensure rescind flag
is false when driver probe happens again.

Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
---
 drivers/hv/vmbus_drv.c       | 1 +
 drivers/uio/uio_hv_generic.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index c857dc3975be..4bae382a3eb4 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
 	 */
 	device_unregister(&device_obj->device);
 }
+EXPORT_SYMBOL_GPL(vmbus_device_unregister);
 
 #ifdef CONFIG_ACPI
 /*
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index c99890c16d29..ea26c0b460d6 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
 
 	/* Wake up reader */
 	uio_event_notify(&pdata->info);
+
+	/*
+	 * With rescind callback registered, rescind path will not unregister the device
+	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
+	 */
+	if (!channel->primary_channel)
+		vmbus_device_unregister(channel->device_obj);
 }
 
 /* Sysfs API to allow mmap of the ring buffers
-- 
2.34.1

Re: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Greg Kroah-Hartman]

On Thu, Aug 22, 2024 at 04:39:12PM +0530, Naman Jain wrote:
> Rescind offer handling relies on rescind callbacks for some of the
> resources cleanup, if they are registered. It does not unregister
> vmbus device for the primary channel closure, when callback is
> registered.
> Add logic to unregister vmbus for the primary channel in rescind callback
> to ensure channel removal and relid release, and to ensure rescind flag
> is false when driver probe happens again.
> 
> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c       | 1 +
>  drivers/uio/uio_hv_generic.c | 7 +++++++
>  2 files changed, 8 insertions(+)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Michael Kelley]

From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> 
> Rescind offer handling relies on rescind callbacks for some of the
> resources cleanup, if they are registered. It does not unregister
> vmbus device for the primary channel closure, when callback is
> registered.
> Add logic to unregister vmbus for the primary channel in rescind callback
> to ensure channel removal and relid release, and to ensure rescind flag
> is false when driver probe happens again.
> 
> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
>  drivers/hv/vmbus_drv.c       | 1 +
>  drivers/uio/uio_hv_generic.c | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index c857dc3975be..4bae382a3eb4 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device
> *device_obj)
>  	 */
>  	device_unregister(&device_obj->device);
>  }
> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
> 
>  #ifdef CONFIG_ACPI
>  /*
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index c99890c16d29..ea26c0b460d6 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
> 
>  	/* Wake up reader */
>  	uio_event_notify(&pdata->info);
> +
> +	/*
> +	 * With rescind callback registered, rescind path will not unregister the device
> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
> +	 */
> +	if (!channel->primary_channel)
> +		vmbus_device_unregister(channel->device_obj);

When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
call to vmbus_device_unregister(). But it does so bracketed with get_device()/
put_device(). Your code here does not do the bracketing. Is there a reason for
the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
bracketing, and I can't definitively say if it is really needed. So I guess I'm
just asking if you know. :-)

Michael

>  }
> 
>  /* ysfs API to allow mmap of the ring buffers
> --
> 2.34.1
> 

Re: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Naman Jain]

On 8/25/2024 8:27 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
>>
>> Rescind offer handling relies on rescind callbacks for some of the
>> resources cleanup, if they are registered. It does not unregister
>> vmbus device for the primary channel closure, when callback is
>> registered.
>> Add logic to unregister vmbus for the primary channel in rescind callback
>> to ensure channel removal and relid release, and to ensure rescind flag
>> is false when driver probe happens again.
>>
>> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   drivers/hv/vmbus_drv.c       | 1 +
>>   drivers/uio/uio_hv_generic.c | 7 +++++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index c857dc3975be..4bae382a3eb4 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device
>> *device_obj)
>>   	 */
>>   	device_unregister(&device_obj->device);
>>   }
>> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
>>
>>   #ifdef CONFIG_ACPI
>>   /*
>> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
>> index c99890c16d29..ea26c0b460d6 100644
>> --- a/drivers/uio/uio_hv_generic.c
>> +++ b/drivers/uio/uio_hv_generic.c
>> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
>>
>>   	/* Wake up reader */
>>   	uio_event_notify(&pdata->info);
>> +
>> +	/*
>> +	 * With rescind callback registered, rescind path will not unregister the device
>> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
>> +	 */
>> +	if (!channel->primary_channel)
>> +		vmbus_device_unregister(channel->device_obj);
> 
> When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
> call to vmbus_device_unregister(). But it does so bracketed with get_device()/
> put_device(). Your code here does not do the bracketing. Is there a reason for
> the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
> bracketing, and I can't definitively say if it is really needed. So I guess I'm
> just asking if you know. :-)
> 
> Michael
> 
>>   }
>>
>>   /* ysfs API to allow mmap of the ring buffers
>> --
>> 2.34.1
>>

IMHO, we have already NULL checked channel->device_obj and other couple 
of things to make sure we are safe to clean this up. At other places as 
well, I don't see the use of put and get device. So I think its not 
required. I am open to suggestions.

Regards,
Naman

RE: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Michael Kelley]

From: Naman Jain <namjain@linux.microsoft.com> Sent: Sunday, August 25, 2024 10:32 PM
> 
> On 8/25/2024 8:27 AM, Michael Kelley wrote:
> > From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> >>
> >> Rescind offer handling relies on rescind callbacks for some of the
> >> resources cleanup, if they are registered. It does not unregister
> >> vmbus device for the primary channel closure, when callback is
> >> registered.
> >> Add logic to unregister vmbus for the primary channel in rescind callback
> >> to ensure channel removal and relid release, and to ensure rescind flag
> >> is false when driver probe happens again.
> >>
> >> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> >> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> >> ---
> >>   drivers/hv/vmbus_drv.c       | 1 +
> >>   drivers/uio/uio_hv_generic.c | 7 +++++++
> >>   2 files changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> >> index c857dc3975be..4bae382a3eb4 100644
> >> --- a/drivers/hv/vmbus_drv.c
> >> +++ b/drivers/hv/vmbus_drv.c
> >> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> >>   	 */
> >>   	device_unregister(&device_obj->device);
> >>   }
> >> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
> >>
> >>   #ifdef CONFIG_ACPI
> >>   /*
> >> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> >> index c99890c16d29..ea26c0b460d6 100644
> >> --- a/drivers/uio/uio_hv_generic.c
> >> +++ b/drivers/uio/uio_hv_generic.c
> >> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
> >>
> >>   	/* Wake up reader */
> >>   	uio_event_notify(&pdata->info);
> >> +
> >> +	/*
> >> +	 * With rescind callback registered, rescind path will not unregister the device
> >> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
> >> +	 */
> >> +	if (!channel->primary_channel)
> >> +		vmbus_device_unregister(channel->device_obj);
> >
> > When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
> > call to vmbus_device_unregister(). But it does so bracketed with get_device()/
> > put_device(). Your code here does not do the bracketing. Is there a reason for
> > the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
> > bracketing, and I can't definitively say if it is really needed. So I guess I'm
> > just asking if you know. :-)
> >
> > Michael
> 
> IMHO, we have already NULL checked channel->device_obj and other couple
> of things to make sure we are safe to clean this up. At other places as
> well, I don't see the use of put and get device. So I think its not
> required. I am open to suggestions.
> 
> Regards,
> Naman

OK. I'm good with what you've said, and don't have any further suggestions.
Go with what your patch already has. :-)

Michael

Re: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Naman Jain]

On 8/26/2024 11:10 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Sunday, August 25, 2024 10:32 PM
>>
>> On 8/25/2024 8:27 AM, Michael Kelley wrote:
>>> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
>>>>
>>>> Rescind offer handling relies on rescind callbacks for some of the
>>>> resources cleanup, if they are registered. It does not unregister
>>>> vmbus device for the primary channel closure, when callback is
>>>> registered.
>>>> Add logic to unregister vmbus for the primary channel in rescind callback
>>>> to ensure channel removal and relid release, and to ensure rescind flag
>>>> is false when driver probe happens again.
>>>>
>>>> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
>>>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>>>> ---
>>>>    drivers/hv/vmbus_drv.c       | 1 +
>>>>    drivers/uio/uio_hv_generic.c | 7 +++++++
>>>>    2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>>>> index c857dc3975be..4bae382a3eb4 100644
>>>> --- a/drivers/hv/vmbus_drv.c
>>>> +++ b/drivers/hv/vmbus_drv.c
>>>> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
>>>>    	 */
>>>>    	device_unregister(&device_obj->device);
>>>>    }
>>>> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
>>>>
>>>>    #ifdef CONFIG_ACPI
>>>>    /*
>>>> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
>>>> index c99890c16d29..ea26c0b460d6 100644
>>>> --- a/drivers/uio/uio_hv_generic.c
>>>> +++ b/drivers/uio/uio_hv_generic.c
>>>> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
>>>>
>>>>    	/* Wake up reader */
>>>>    	uio_event_notify(&pdata->info);
>>>> +
>>>> +	/*
>>>> +	 * With rescind callback registered, rescind path will not unregister the device
>>>> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
>>>> +	 */
>>>> +	if (!channel->primary_channel)
>>>> +		vmbus_device_unregister(channel->device_obj);
>>>
>>> When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
>>> call to vmbus_device_unregister(). But it does so bracketed with get_device()/
>>> put_device(). Your code here does not do the bracketing. Is there a reason for
>>> the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
>>> bracketing, and I can't definitively say if it is really needed. So I guess I'm
>>> just asking if you know. :-)
>>>
>>> Michael
>>
>> IMHO, we have already NULL checked channel->device_obj and other couple
>> of things to make sure we are safe to clean this up. At other places as
>> well, I don't see the use of put and get device. So I think its not
>> required. I am open to suggestions.
>>
>> Regards,
>> Naman
> 
> OK. I'm good with what you've said, and don't have any further suggestions.
> Go with what your patch already has. :-)
> 
> Michael

Thank you Michael. I'll wait for some time before posting v2, if there 
are any more review comments.

Regards,
Naman

Re: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Saurabh Singh Sengar]

On Mon, Aug 26, 2024 at 05:40:37AM +0000, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Sunday, August 25, 2024 10:32 PM
> > 
> > On 8/25/2024 8:27 AM, Michael Kelley wrote:
> > > From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> > >>
> > >> Rescind offer handling relies on rescind callbacks for some of the
> > >> resources cleanup, if they are registered. It does not unregister
> > >> vmbus device for the primary channel closure, when callback is
> > >> registered.
> > >> Add logic to unregister vmbus for the primary channel in rescind callback
> > >> to ensure channel removal and relid release, and to ensure rescind flag
> > >> is false when driver probe happens again.
> > >>
> > >> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> > >> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> > >> ---
> > >>   drivers/hv/vmbus_drv.c       | 1 +
> > >>   drivers/uio/uio_hv_generic.c | 7 +++++++
> > >>   2 files changed, 8 insertions(+)
> > >>
> > >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > >> index c857dc3975be..4bae382a3eb4 100644
> > >> --- a/drivers/hv/vmbus_drv.c
> > >> +++ b/drivers/hv/vmbus_drv.c
> > >> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> > >>   	 */
> > >>   	device_unregister(&device_obj->device);
> > >>   }
> > >> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
> > >>
> > >>   #ifdef CONFIG_ACPI
> > >>   /*
> > >> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> > >> index c99890c16d29..ea26c0b460d6 100644
> > >> --- a/drivers/uio/uio_hv_generic.c
> > >> +++ b/drivers/uio/uio_hv_generic.c
> > >> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
> > >>
> > >>   	/* Wake up reader */
> > >>   	uio_event_notify(&pdata->info);
> > >> +
> > >> +	/*
> > >> +	 * With rescind callback registered, rescind path will not unregister the device
> > >> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
> > >> +	 */
> > >> +	if (!channel->primary_channel)
> > >> +		vmbus_device_unregister(channel->device_obj);
> > >
> > > When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
> > > call to vmbus_device_unregister(). But it does so bracketed with get_device()/
> > > put_device(). Your code here does not do the bracketing. Is there a reason for
> > > the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
> > > bracketing, and I can't definitively say if it is really needed. So I guess I'm
> > > just asking if you know. :-)
> > >
> > > Michael
> > 
> > IMHO, we have already NULL checked channel->device_obj and other couple
> > of things to make sure we are safe to clean this up. At other places as
> > well, I don't see the use of put and get device. So I think its not
> > required. I am open to suggestions.
> > 
> > Regards,
> > Naman
> 
> OK. I'm good with what you've said, and don't have any further suggestions.
> Go with what your patch already has. :-)
> 
> Michael


Michael,

If we look at vmbus_onoffer_rescind function, hv_uio_rescind can only be called
if channel->device_obj is not NULL. By this if we conclude that hv_uio_rescind can
never be called for secondary channel I think we can simplify hv_uio_rescind
only for primary channel.

In the first patch of this series, instead of this:
+	struct hv_device *hv_dev = channel->primary_channel ?
+				   channel->primary_channel->device_obj : channel->device_obj;

We can only have

+	struct hv_device *hv_dev = channel->device_obj;


In second patch, instead of this:
+        if (!channel->primary_channel)
+                vmbus_device_unregister(channel->device_obj);

We can only have:
+                vmbus_device_unregister(channel->device_obj);
	

Possibly WARN for secondary channel is also not required as that will never happen ?


- Saurabh

RE: [PATCH 2/2] Drivers: hv: vmbus: Fix rescind handling in uio_hv_generic

[Michael Kelley]

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Tuesday, August 27, 2024 11:24 AM
> 
> On Mon, Aug 26, 2024 at 05:40:37AM +0000, Michael Kelley wrote:
> > From: Naman Jain <namjain@linux.microsoft.com> Sent: Sunday, August 25, 2024 10:32 PM
> > >
> > > On 8/25/2024 8:27 AM, Michael Kelley wrote:
> > > > From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> > > >>
> > > >> Rescind offer handling relies on rescind callbacks for some of the
> > > >> resources cleanup, if they are registered. It does not unregister
> > > >> vmbus device for the primary channel closure, when callback is
> > > >> registered.
> > > >> Add logic to unregister vmbus for the primary channel in rescind callback
> > > >> to ensure channel removal and relid release, and to ensure rescind flag
> > > >> is false when driver probe happens again.
> > > >>
> > > >> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> > > >> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> > > >> ---
> > > >>   drivers/hv/vmbus_drv.c       | 1 +
> > > >>   drivers/uio/uio_hv_generic.c | 7 +++++++
> > > >>   2 files changed, 8 insertions(+)
> > > >>
> > > >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > >> index c857dc3975be..4bae382a3eb4 100644
> > > >> --- a/drivers/hv/vmbus_drv.c
> > > >> +++ b/drivers/hv/vmbus_drv.c
> > > >> @@ -1952,6 +1952,7 @@ void vmbus_device_unregister(struct hv_device *device_obj)
> > > >>   	 */
> > > >>   	device_unregister(&device_obj->device);
> > > >>   }
> > > >> +EXPORT_SYMBOL_GPL(vmbus_device_unregister);
> > > >>
> > > >>   #ifdef CONFIG_ACPI
> > > >>   /*
> > > >> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> > > >> index c99890c16d29..ea26c0b460d6 100644
> > > >> --- a/drivers/uio/uio_hv_generic.c
> > > >> +++ b/drivers/uio/uio_hv_generic.c
> > > >> @@ -121,6 +121,13 @@ static void hv_uio_rescind(struct vmbus_channel *channel)
> > > >>
> > > >>   	/* Wake up reader */
> > > >>   	uio_event_notify(&pdata->info);
> > > >> +
> > > >> +	/*
> > > >> +	 * With rescind callback registered, rescind path will not unregister the device
> > > >> +	 * when the primary channel is rescinded. Without it, next onoffer msg does not come.
> > > >> +	 */
> > > >> +	if (!channel->primary_channel)
> > > >> +		vmbus_device_unregister(channel->device_obj);
> > > >
> > > > When the rescind callback is *not* set, vmbus_onoffer_rescind() makes the
> > > > call to vmbus_device_unregister(). But it does so bracketed with get_device()/
> > > > put_device(). Your code here does not do the bracketing. Is there a reason for
> > > > the difference? Frankly, I'm not sure why vmbus_onoffer_rescind() does the
> > > > bracketing, and I can't definitively say if it is really needed. So I guess I'm
> > > > just asking if you know. :-)
> > > >
> > > > Michael
> > >
> > > IMHO, we have already NULL checked channel->device_obj and other couple
> > > of things to make sure we are safe to clean this up. At other places as
> > > well, I don't see the use of put and get device. So I think its not
> > > required. I am open to suggestions.
> > >
> > > Regards,
> > > Naman
> >
> > OK. I'm good with what you've said, and don't have any further suggestions.
> > Go with what your patch already has. :-)
> >
> > Michael
> 
> 
> Michael,
> 
> If we look at vmbus_onoffer_rescind function, hv_uio_rescind can only be called
> if channel->device_obj is not NULL. By this if we conclude that hv_uio_rescind can
> never be called for secondary channel I think we can simplify hv_uio_rescind
> only for primary channel.
> 
> In the first patch of this series, instead of this:
> +	struct hv_device *hv_dev = channel->primary_channel ?
> +				   channel->primary_channel->device_obj : channel->device_obj;
> 
> We can only have
> 
> +	struct hv_device *hv_dev = channel->device_obj;
> 

Agreed. That was the intent of my previous comments on the first patch.

> 
> In second patch, instead of this:
> +        if (!channel->primary_channel)
> +                vmbus_device_unregister(channel->device_obj);
> 
> We can only have:
> +                vmbus_device_unregister(channel->device_obj);
> 

Agreed.

> 
> Possibly WARN for secondary channel is also not required as that will never happen ?
> 

Agreed -- the WARN is optional. I'm OK with leaving it out. But please
leave a comment in both places that the function is only called for
the primary channel, so it's not necessary to do any checking of the
primary_channel field. Future readers of the code will thank you. :-)

Michael


> 
> - Saurabh