Since we only have one udev_monitor reference throughout libvirt, we
should either clear the pointer we copy around to prevent invalid memory
access once we unref this single reference, or start using reference
counting provided by libudev. This patch follows the former and only
clears the pointer.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index ea10dc3ce..cd19e79c1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
         if (udev_monitor != NULL) {
             udev = udev_monitor_get_udev(udev_monitor);
             udev_monitor_unref(udev_monitor);
+            priv->udev_monitor = NULL;
         }
     }
 
-- 
2.13.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/26/2017 02:44 AM, Erik Skultety wrote:
> Since we only have one udev_monitor reference throughout libvirt, we
> should either clear the pointer we copy around to prevent invalid memory
> access once we unref this single reference, or start using reference
> counting provided by libudev. This patch follows the former and only
> clears the pointer.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 1 +
>  1 file changed, 1 insertion(+)
> 

What's here, works, but expanding things out a bit for this one...

Since we have the nodeDeviceLock, why not within:

     if (priv) {
     }

do a VIR_FREE(driver->privateData); inside the if, that should set
driver->privateData = NULL too.  Avoids something finding driver->priv
later in the instant that the nodeDeviceUnlock is run and something else
gets the lock and looks @priv... IOW: See, patch 5 comments.

I think if driver->priv is set to NULL, then there'd be no need to set
udev_monitor to NULL, but it cannot hurt especially if this is some sort
of race with udevEventHandleCallback, so...

Reviewed-by: John Ferlan <jferlan@redhat.com>

But I do think the VIR_FREE(priv) should move and be a
VIR_FREE(driver->privateData)... Note, not @priv because that won't set
@privateData = NULl which is what we'd want.  You may even want to leave
a comment about that indicating trying to close a race w/ event handling.

John

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index ea10dc3ce..cd19e79c1 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
>          if (udev_monitor != NULL) {
>              udev = udev_monitor_get_udev(udev_monitor);
>              udev_monitor_unref(udev_monitor);
> +            priv->udev_monitor = NULL;
>          }
>      }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Aug 01, 2017 at 03:25:37PM -0400, John Ferlan wrote:
>
>
> On 07/26/2017 02:44 AM, Erik Skultety wrote:
> > Since we only have one udev_monitor reference throughout libvirt, we
> > should either clear the pointer we copy around to prevent invalid memory
> > access once we unref this single reference, or start using reference
> > counting provided by libudev. This patch follows the former and only
> > clears the pointer.
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
>
> What's here, works, but expanding things out a bit for this one...
>
> Since we have the nodeDeviceLock, why not within:
>
>      if (priv) {
>      }
>
> do a VIR_FREE(driver->privateData); inside the if, that should set
> driver->privateData = NULL too.  Avoids something finding driver->priv
> later in the instant that the nodeDeviceUnlock is run and something else

Good point.

> gets the lock and looks @priv... IOW: See, patch 5 comments.
>
> I think if driver->priv is set to NULL, then there'd be no need to set
> udev_monitor to NULL, but it cannot hurt especially if this is some sort
> of race with udevEventHandleCallback, so...
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> But I do think the VIR_FREE(priv) should move and be a
> VIR_FREE(driver->privateData)... Note, not @priv because that won't set
> @privateData = NULl which is what we'd want.  You may even want to leave
> a comment about that indicating trying to close a race w/ event handling.

I don't know, to me commenting such a change would appear as a bogus comment,
IOW, you always have to protect access to shared data (unless implementing a
lockless algorithm) so I think this change is self-explanatory, but anyways
thanks for pointing it out, I'll definitely adjust the code.

Erik

>
> John
>
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index ea10dc3ce..cd19e79c1 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1581,6 +1581,7 @@ nodeStateCleanup(void)
> >          if (udev_monitor != NULL) {
> >              udev = udev_monitor_get_udev(udev_monitor);
> >              udev_monitor_unref(udev_monitor);
> > +            priv->udev_monitor = NULL;
> >          }
> >      }
> >
> >

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list