[PATCH v2 1/2] libmultipath: prevent memory leak in alloc_path_with_pathinfo() if pp_ptr is NULL

[PATCH v2 1/2] libmultipath: prevent memory leak in alloc_path_with_pathinfo() if pp_ptr is NULL

[Mauricio Faria de Oliveira]

In alloc_path_with_pathinfo(), if the 'pp_ptr' argument is NULL
(which is acceptable and checked in the function in two places)
the 'pp' pointer is lost as it is not referenced anywhere else;
thus the memory allocated for it is leaked.

So, call free_path() in the case 'pp_ptr' is NULL too.

Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7b5b4344b2a1..3c5c808436b2 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -58,7 +58,7 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
 		err = pathinfo(pp, conf, flag | DI_BLACKLIST);
 	}
 
-	if (err)
+	if (err || !pp_ptr)
 		free_path(pp);
 	else if (pp_ptr)
 		*pp_ptr = pp;
-- 
1.8.3.1

[PATCH v2 2/2] multipathd: skip spurious event message for blacklisted paths

[Mauricio Faria de Oliveira]

Currently, multipath still prints the 'spurious uevent, path not found'
message if a path is blacklisted by something different than a devnode
in the 'change' uevent handling. (uev_trigger() calls filter_devnode()).

Thus blacklisting by vendor/product, wwid, and udev property still get
that message in the system log for paths that are explicitly marked to
be ignored (since it's verbosity level 0).

This problem happens on common scenarios such as creating filesystems
on a blacklisted device (e.g., mkfs.* /dev/sdX), which is usually run
several times on test environments, and the error message may mislead
the error checker/monitor tools with false negatives.

This patch resolves this by checking the udev property and path_info()
for PATHINFO_SKIPPED with just enough device information flags for
blacklist verification -- and it prints a debug message (verbosity
level 3) instead of an error message since this case is not an error.

Even though this introduces a bit of overhead (to get the path info),
it only happens in a corner case of an error path; so, not a problem.

Test-cases (on QEMU):
----------

Several versions of /etc/multipath.conf:

  blacklist {
      devnode "sdb"
  }

  blacklist {
      property "ID_SERIAL"
  }

  blacklist {
      wwid "0QEMU_QEMU_HARDDISK_drive-scsi0-0-0-1"
  }

  blacklist {
      device {
          vendor "QEMU"
      }
  }

Either command can be used to generate a 'change' uevent:

  # echo change > /sys/block/sdb/uevent

  # mkfs.ext3 -F /dev/sdb

The output of multipathd is monitored with:

  # multipathd -d -s
  ...

Without the patch, only the devnode blacklisting is silent;
all other cases (property, wwid, device) print the message:

  sdb: spurious uevent, path not found

With the patch applied, no message is printed by default
(that is, verbosity level 2) in any case.

With verbosity level 3 (debug), the following messages
are printed, according to the performed test-case:

  sdb: udev property ID_SERIAL blacklisted
  sdb: spurious uevent, path is blacklisted

  sdb: wwid 0QEMU_QEMU_HARDDISK_drive-scsi0-0-0-1 blacklisted
  sdb: spurious uevent, path is blacklisted

  (null): (QEMU:QEMU HARDDISK) vendor/product blacklisted
  sdb: spurious uevent, path is blacklisted

Reported-by: Yueh Chyong (Ida) Jackson <idaj@us.ibm.com>
Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
---
 multipathd/main.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d6f081f2f83a..dd7e8d69491e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1010,8 +1010,38 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 	}
 out:
 	lock_cleanup_pop(vecs->lock);
-	if (!pp)
+	if (!pp) {
+		/*
+		 *  If the path is blacklisted, print a debug/non-default verbosity message.
+		 *  - filter_devnode()	- checked by uev_trigger() (caller);
+		 *  - filter_device()	- checked by pathinfo() (DI_BLACKLIST | DI_SYSFS);
+		 *  - filter_wwid()	- checked by pathinfo() (DI_BLACKLIST | DI_WWID);
+		 *  - filter_property()	- checked here).
+		 */
+		if (uev->udev) {
+			int flag = DI_SYSFS | DI_WWID;
+			struct udev_device *udevice;
+
+			udevice = udev_device_ref(uev->udev);
+
+			if (filter_property(conf, udevice) > 0)
+				retval = PATHINFO_SKIPPED;
+			else {
+				conf = get_multipath_config();
+				retval = alloc_path_with_pathinfo(conf, udevice, flag, NULL);
+				put_multipath_config(conf);
+			}
+
+			udev_device_unref(uev->udev);
+
+			if (retval == PATHINFO_SKIPPED) {
+				condlog(3, "%s: spurious uevent, path is blacklisted", uev->kernel);
+				return 0;
+			}
+		}
+
 		condlog(0, "%s: spurious uevent, path not found", uev->kernel);
+	}
 
 	return retval;
 }
-- 
1.8.3.1

Re: [PATCH v2 2/2] multipathd: skip spurious event message for blacklisted paths

[Benjamin Marzinski]

On Tue, Dec 13, 2016 at 04:27:09PM -0200, Mauricio Faria de Oliveira wrote:

Thanks for working on this. I like this version much better, but I still
have some issues. First off, I don't see why the udev_device_ref/unref
is necessary. While servicing uevents, when we first get the udevice
from udev_monitor_receive_device() in uevent_listen() it comes with a
reference. After we finish servicing the uevent, we drop this reference
in service_uevq(). If we attach this udevice to a path (by setting
pp->udev), we need to get another reference, so that the udevice isn't
deleted when we drop the reference after servicing the event in
service_uevq(). Otherwise pp->udev would be pointing to freed memory.
We drop this reference when we free the path in free_path().

When you call alloc_path_with_pathinfo(), it already grabs the second
reference before calling pathinfo. When it exits without pp_ptr being
set, it frees that extra reference in free_path(). But this whole time,
we are still servicing the uevent, and that originl reference is always
held, so we don't ever need to worry about the udevice getting deleted
out from under us. I don't see any possible danger in removing the
udev_device_ref/unref calls from your code. They don't hurt anything,
but grabbing references when we don't need them just confuses other
people who are trying reading the code, and makes the purpose of the
references harder to understand.

My second issue is more for Hannes. Is there a reason why we don't check
filter_property in pathinfo if we call it with (DI_BLACKLIST | DI_WWID).
If we have the information to get the WWID, then we have the information
to check filter_property.  Looking at the code, I don't see how we could
be checking filter_property for paths added via uev_add_path. Am I
missing something subtle here, or should we just add a filter_property()
check to pathinfo()?

-Ben

> Reported-by: Yueh Chyong (Ida) Jackson <idaj@us.ibm.com>
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
> ---
>  multipathd/main.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index d6f081f2f83a..dd7e8d69491e 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1010,8 +1010,38 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  	}
>  out:
>  	lock_cleanup_pop(vecs->lock);
> -	if (!pp)
> +	if (!pp) {
> +		/*
> +		 *  If the path is blacklisted, print a debug/non-default verbosity message.
> +		 *  - filter_devnode()	- checked by uev_trigger() (caller);
> +		 *  - filter_device()	- checked by pathinfo() (DI_BLACKLIST | DI_SYSFS);
> +		 *  - filter_wwid()	- checked by pathinfo() (DI_BLACKLIST | DI_WWID);
> +		 *  - filter_property()	- checked here).
> +		 */
> +		if (uev->udev) {
> +			int flag = DI_SYSFS | DI_WWID;
> +			struct udev_device *udevice;
> +
> +			udevice = udev_device_ref(uev->udev);
> +
> +			if (filter_property(conf, udevice) > 0)
> +				retval = PATHINFO_SKIPPED;
> +			else {
> +				conf = get_multipath_config();
> +				retval = alloc_path_with_pathinfo(conf, udevice, flag, NULL);
> +				put_multipath_config(conf);
> +			}
> +
> +			udev_device_unref(uev->udev);
> +
> +			if (retval == PATHINFO_SKIPPED) {
> +				condlog(3, "%s: spurious uevent, path is blacklisted", uev->kernel);
> +				return 0;
> +			}
> +		}
> +
>  		condlog(0, "%s: spurious uevent, path not found", uev->kernel);
> +	}
>  
>  	return retval;
>  }
> -- 
> 1.8.3.1

Re: [PATCH v2 2/2] multipathd: skip spurious event message for blacklisted paths

[Mauricio Faria de Oliveira]

Ben,

On 12/13/2016 07:50 PM, Benjamin Marzinski wrote:
> [snip] I don't see why the udev_device_ref/unref
> is necessary. While servicing uevents, when we first get the udevice
> from udev_monitor_receive_device() in uevent_listen() it comes with a
> reference. After we finish servicing the uevent, we drop this reference
> in service_uevq().  [snip]

Oh, cool. Thanks for the detailed explanation. I hadn't realized about
that first reference. I'll remove the udev_device_ref/unref() in v3.

 > My second issue is more for Hannes. Is there a reason why we don't check
 > filter_property in pathinfo if we call it with (DI_BLACKLIST | DI_WWID).
 > If we have the information to get the WWID, then we have the information
 > to check filter_property. [snip]

If I may, I'm not sure that filter_property() has to do w/ DI_WWID.

iiuic, it just checks the /name/ of each udev property of udev_device
against the blacklist/blacklist-exceptions entries for property names
in the conf struct (i.e., it doesn't need the path wwid).

And that information about the udev_device is available upfront,
even before we call pathinfo() (BTW, I guess this is the reason
why it's not called from within it, but rather at a point in
path_discover() that only the udev_device is available, like
filter_devnode() too.)

 > [snip]  Looking at the code, I don't see how we could
 > be checking filter_property for paths added via uev_add_path. Am I
 > missing something subtle here, or should we just add a filter_property()
 > check to pathinfo()?

You're correct. If a path is added via uev_add_path() it ignores
the property blacklisting, and end up creating/joining a devmap.
It seems like an unhandled case.

So, I'll submit a v3 w/ your suggestion for this too.

Thanks!


Simple test case w/ change/add uevents:

# cat /etc/multipath.conf
blacklist {
         property "ID_SERIAL"
}

# multipathd -d -s -v3 &
...

# echo change > /sys/block/sdb/uevent
uevent 'change' from '/devices/vio/2000/host0/target0:0:0/0:0:0:1/block/sdb'
Forwarding 1 uevents
sdb: udev property ID_SERIAL blacklisted
sdb: spurious uevent, path is blacklisted

# echo add > /sys/block/sdb/uevent
uevent 'add' from '/devices/vio/2000/host0/target0:0:0/0:0:0:1/block/sdb'
Forwarding 1 uevents
sdb: add path (uevent)
...
0QEMU_QEMU_HARDDISK_drive-scsi0-0-0-1: load table [0 4194304 multipath 1 
retain_attached_hw_handler 0 1 1 service-time 0 1 1 8:16 1]
...
sdb [8:16]: path added to devmap 0QEMU_QEMU_HARDDISK_drive-scsi0-0-0-1





-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [PATCH v2 2/2] multipathd: skip spurious event message for blacklisted paths

[Martin Wilck]

On Tue, 2016-12-13 at 15:50 -0600, Benjamin Marzinski wrote:

> On Tue, Dec 13, 2016 at 04:27:09PM -0200, Mauricio Faria de Oliveira
> wrote:
> When you call alloc_path_with_pathinfo(), it already grabs the second
> reference before calling pathinfo. When it exits without pp_ptr being
> set, it frees that extra reference in free_path(). But this whole
> time,
> we are still servicing the uevent, and that originl reference is
> always
> held, so we don't ever need to worry about the udevice getting
> deleted
> out from under us. I don't see any possible danger in removing the
> udev_device_ref/unref calls from your code. They don't hurt anything,
> but grabbing references when we don't need them just confuses other
> people who are trying reading the code, and makes the purpose of the
> references harder to understand.

As someone who has just recently started reading the multipath-tools
code in depth, I tend to disagree. Someone reading the function
including the udev_device_ref()/unref() calls would see that the device
references are handled correctly in the function itself, without having
to know or having to track down how the device references are created
and dropped in other parts of the code. Moreover, if the global ref
handling was ever to be changed in the future, this function would
still be "safe".

If the udev_device_ref/unref calls are removed, at least a comment
explaining why they aren't needed would help new readers of the code.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH v2 2/2] multipathd: skip spurious event message for blacklisted paths

[Benjamin Marzinski]

On Wed, Dec 14, 2016 at 01:53:09PM +0100, Martin Wilck wrote:
> On Tue, 2016-12-13 at 15:50 -0600, Benjamin Marzinski wrote:
> 
> > On Tue, Dec 13, 2016 at 04:27:09PM -0200, Mauricio Faria de Oliveira
> > wrote:
> > When you call alloc_path_with_pathinfo(), it already grabs the second
> > reference before calling pathinfo. When it exits without pp_ptr being
> > set, it frees that extra reference in free_path(). But this whole
> > time,
> > we are still servicing the uevent, and that originl reference is
> > always
> > held, so we don't ever need to worry about the udevice getting
> > deleted
> > out from under us. I don't see any possible danger in removing the
> > udev_device_ref/unref calls from your code. They don't hurt anything,
> > but grabbing references when we don't need them just confuses other
> > people who are trying reading the code, and makes the purpose of the
> > references harder to understand.
> 
> As someone who has just recently started reading the multipath-tools
> code in depth, I tend to disagree. Someone reading the function
> including the udev_device_ref()/unref() calls would see that the device
> references are handled correctly in the function itself, without having
> to know or having to track down how the device references are created
> and dropped in other parts of the code. Moreover, if the global ref
> handling was ever to be changed in the future, this function would
> still be "safe".

I would argue that you don't need to track down these reference. Since
uev_update_path is accessing the device, you can assume that it has a
reference (otherwise it would be accessing memory that could be freed at
any moment, and may already be freed). If the code you add doesn't drop
that reference, then it is clearly able to acces the udev device,
without worrying about the memory going away. If the code you add DOES
drop a reference, then you really DO need to track down the ref/unref
calls, because otherwise, you could be breaking things. For instance, if
this patch dropped a reference between those ref/unref calls, that would
break multipathd. After the unref call, the memory could now be freed,
where before, uev_update_path returned with the reference still held.
The only time you ever need to grab a another reference is if your code
stores the udev device, so that it can exist after multipathd finishes
processing the uevent. When your code finally is done with the udev
device, it needs to drop the reference, so the the memory can get freed.

No matter how global ref handling is changed in the future,
uev_update_path will still need to be called with a reference to the
udev device, because it accesses it. So as long as it doesn't store the
device, it doesn't need to grab another reference.

 
> If the udev_device_ref/unref calls are removed, at least a comment
> explaining why they aren't needed would help new readers of the code.

But then why not comment all the functions that access the udev device,
since most of them don't grab a reference (nor do they need to).  If
we're going to comment the code, I'd rather put comments in the small
number of places where we do need to grab or drop extra references,
explaining why it is necessary there.

-Ben

> Regards
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel