[dm-devel] [PATCH v2 5/6] libmultipath: limit reading 0xc9 vpd page

[Martin Wilck]

On Wed, 2020-11-04 at 00:54 -0600, Benjamin Marzinski wrote:
> Only rdac arrays support 0xC9 vpd page inquiries. All other arrays
> will
> return a failure. Only do the rdac inquiry when detecting array
> capabilities if the array's path checker is explicitly set to rdac,
> or
> the path checker is not set, and the array reports that it supports
> vpd
> page 0xC9 in the Supported VPD Pages (0x00) vpd page.
> 
> Multipath was doing the check if either the path checker was set to
> rdac, or no path checker was set.  This means that for almost all
> non-rdac arrays, multipath was issuing a bad inquiry. This was
> annoying
> users.
> 
> Cc: Steve Schremmer <sschremm at netapp.com>
> Cc: NetApp RDAC team <ng-eseries-upstream-maintainers at netapp.com>
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  libmultipath/discovery.c | 25 +++++++++++++++++++++++++
>  libmultipath/discovery.h |  1 +
>  libmultipath/propsel.c   | 10 ++++++----
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 95ddbbbd..5669690d 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1346,6 +1346,31 @@ fetch_vpd_page(int fd, int pg, unsigned char
> *buff)
>  	return buff_len;
>  }
>  
> +/* heavily based on sg_inq.c from sg3_utils */
> +bool
> +is_vpd_page_supported(int fd, int pg)
> +{
> +	int i, len, buff_len;
> +	unsigned char buff[4096];
> +
> +	buff_len = fetch_vpd_page(fd, 0x00, buff);
> +	if (buff_len < 0)
> +		return false;
> +	if (buff_len < 4) {
> +		condlog(3, "VPD page 00h too short");
> +		return false;
> +	}
> +
> +	len = buff[3] + 4;

SPC-4 says that the page length is a 16-bit value.
You may also want to check buff[1] == 0.

> +	if (len > buff_len)
> +		condlog(3, "vpd page 00h trucated, expected %d, have
> %d",
> +			len, buff_len);
> +	for (i = 4; i < len; ++i)
> +		if (buff[i] == pg)
> +			return true;
> +	return false;
> +}
> +
>  int
>  get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
>  {
> diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
> index 6444887d..d3193daf 100644
> --- a/libmultipath/discovery.h
> +++ b/libmultipath/discovery.h
> @@ -56,6 +56,7 @@ int sysfs_get_asymmetric_access_state(struct path
> *pp,
>  				      char *buff, int buflen);
>  int get_uid(struct path * pp, int path_state, struct udev_device
> *udev,
>  	    int allow_fallback);
> +bool is_vpd_page_supported(int fd, int pg);
>  
>  /*
>   * discovery bitmask
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index fa4ac5d9..f771a830 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -496,13 +496,15 @@ check_rdac(struct path * pp)
>  {
>  	int len;
>  	char buff[44];
> -	const char *checker_name;
> +	const char *checker_name = NULL;
>  
>  	if (pp->bus != SYSFS_BUS_SCSI)
>  		return 0;
> -	/* Avoid ioctl if this is likely not an RDAC array */
> -	if (__do_set_from_hwe(checker_name, pp, checker_name) &&
> -	    strcmp(checker_name, RDAC))
> +	/* Avoid checking 0xc9 if this is likely not an RDAC array */
> +	if (!__do_set_from_hwe(checker_name, pp, checker_name) &&
> +	    !is_vpd_page_supported(pp->fd, 0xC9))
> +		return 0;
> +	if (checker_name && strcmp(checker_name, RDAC))

Do we still need the name check after testing whether 0xc9 is
supported? Well I guess it doesn't harm.

Regards
Martin

>  		return 0;
>  	len = get_vpd_sgio(pp->fd, 0xC9, 0, buff, 44);
>  	if (len <= 0)

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer