Re: [PATCH] Fix null pointer error after vmbus loading

Re: [PATCH] Fix null pointer error after vmbus loading

[Greg KH]

On Wed, Oct 21, 2009 at 07:24:23PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Fix null pointer error after vmbus loading.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> ---
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 582318f..11431cc 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -538,9 +538,10 @@ static int vmbus_child_device_register(struct hv_device *root_device_obj,
>                    child_device_ctx);
> 
>         /* Make sure we are not registered already */
> -       if (strlen(dev_name(&child_device_ctx->device)) != 0) {
> +       if (dev_name(&child_device_ctx->device) != NULL &&
> +           strlen(dev_name(&child_device_ctx->device)) != 0) {

Why would the device name ever be NULL?  Why would you need to check it
here?

confused,

greg k-h

Re: [PATCH] Fix null pointer error after vmbus loading

[Greg KH]

On Wed, Oct 21, 2009 at 07:24:23PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Fix null pointer error after vmbus loading.
> 
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> ---
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index 582318f..11431cc 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -538,9 +538,10 @@ static int vmbus_child_device_register(struct hv_device *root_device_obj,
>                    child_device_ctx);
> 
>         /* Make sure we are not registered already */
> -       if (strlen(dev_name(&child_device_ctx->device)) != 0) {
> +       if (dev_name(&child_device_ctx->device) != NULL &&
> +           strlen(dev_name(&child_device_ctx->device)) != 0) {

Also, this patch was sent in html format, and all of the tabs were
converted to spaces, making it impossible to apply.

Please read the file, Documentation/email-clients.txt and follow the
suggestions there in order to be able to send patches that will both
appear on the linux-kernel mailing list, and be able to be applied
properly.

thanks,

greg k-h

RE: [PATCH] Fix null pointer error after vmbus loading

[Haiyang Zhang]

> >         /* Make sure we are not registered already */
> > -       if (strlen(dev_name(&child_device_ctx->device)) != 0) {
> > +       if (dev_name(&child_device_ctx->device) != NULL &&
> > +           strlen(dev_name(&child_device_ctx->device)) != 0) {
> 
> Why would the device name ever be NULL?  Why would you need to check it
> here?

In recently kernel versions, char bus_id[BUS_ID_SIZE] in struct device has been replaced by char* type kobj.name, which is NULL if name is not yet set.
The function vmbus_child_device_create() alloc the memory for struct device_context, which includes struct device as a member. All data are initialized to zero, so the device name ptr, aka kobj.name, is NULL at the beginning:
	child_device_ctx = kzalloc(sizeof(struct device_context), GFP_KERNEL);
So, a NULL pointer checking is added to avoid null pointer dereference error if strlen() is called on the NULL name pointer. After we confirm the device isn't registered yet, we will set the name and register the device.

BTW, I forgot to switch to TXT mode in my last email, I will resend the patch in TXT only mode.

Thanks,

- Haiyang

Re: [PATCH] Fix null pointer error after vmbus loading

[Greg KH]

On Thu, Oct 22, 2009 at 03:39:05PM +0000, Haiyang Zhang wrote:
> > >         /* Make sure we are not registered already */
> > > -       if (strlen(dev_name(&child_device_ctx->device)) != 0) {
> > > +       if (dev_name(&child_device_ctx->device) != NULL &&
> > > +           strlen(dev_name(&child_device_ctx->device)) != 0) {
> > 
> > Why would the device name ever be NULL?  Why would you need to check it
> > here?
> 
> In recently kernel versions, char bus_id[BUS_ID_SIZE] in struct device
> has been replaced by char* type kobj.name, which is NULL if name is
> not yet set.

Yes, but you should never care about this being NULL.

> The function vmbus_child_device_create() alloc the memory for struct
> device_context, which includes struct device as a member. All data are
> initialized to zero, so the device name ptr, aka kobj.name, is NULL at
> the beginning:
> 	child_device_ctx = kzalloc(sizeof(struct device_context), GFP_KERNEL);
> So, a NULL pointer checking is added to avoid null pointer dereference
> error if strlen() is called on the NULL name pointer. After we confirm
> the device isn't registered yet, we will set the name and register the
> device.

But this is not how you check to see if your device is registered at
all, you can not rely on the driver core to have the name set or not to
reflect the status of the device.  Devices have many stages in their
lifecycle, and the name reliably shows none of them.

Actually, why do you need to know this?  Shouldn't you always know the
state of your device, how can you get passed an unregistered device?

That is the real fix that needs to be done here.

thanks,

greg k-h

RE: [PATCH] Fix null pointer error after vmbus loading

[Haiyang Zhang]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> But this is not how you check to see if your device is registered at
> all, you can not rely on the driver core to have the name set or not to
> reflect the status of the device.  Devices have many stages in their
> lifecycle, and the name reliably shows none of them.
> 
> Actually, why do you need to know this?  Shouldn't you always know the
> state of your device, how can you get passed an unregistered device?

I have traced down the callers to the function, vmbus_child_device_register(),
and verified that the struct device was kzalloc-ed by the current thread,
and not visible to other threads before we register the device. So the 
dev name is always NULL here, we can skip the NULL & strlen checking. I 
will send you the new patch soon.

Thanks,

- Haiyang