md/dm-mpath: check whether all pgpaths have same uuid in multipath_ctr()

Commit Message

Zhiqiang Liu March 20, 2021, 7:19 a.m. UTC

From: Zhiqiang Liu <[e-mail]>

When we make IO stress test on multipath device, there will
be a metadata err because of wrong path. In the test, we
concurrent execute 'iscsi device login|logout' and
'multipath -r' command with IO stress on multipath device.
In some case, systemd-udevd may have not time to process
uevents of iscsi device logout|login, and then 'multipath -r'
command triggers multipathd daemon calls ioctl to load table
with incorrect old device info from systemd-udevd.
Then, one iscsi path may be incorrectly attached to another
multipath which has different uuid. Finally, the metadata err
occurs when umounting filesystem to down write metadata on
the iscsi device which is actually not owned by the multipath
device.

So we need to check whether all pgpaths of one multipath have
the same uuid, if not, we should throw a error.

Signed-off-by: Zhiqiang Liu <[e-mail]>
Signed-off-by: lixiaokeng <[e-mail]>
Signed-off-by: linfeilong <[e-mail]>
Signed-off-by: Wubo <[e-mail]>
---
 drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_lib.c |  1 +
 2 files changed, 53 insertions(+)

Comments

kernel test robot March 20, 2021, 2:34 p.m. UTC | #1

Hi Zhiqiang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on dm/for-next]
[also build test ERROR on mkp-scsi/for-next scsi/for-next v5.12-rc3 next-20210319]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zhiqiang-Liu/md-dm-mpath-check-whether-all-pgpaths-have-same-uuid-in-multipath_ctr/20210320-195717
base:   https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git for-next
config: i386-randconfig-c001-20210320 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/f8f908f78541c7da502df6f31f772ca6e8c71732
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhiqiang-Liu/md-dm-mpath-check-whether-all-pgpaths-have-same-uuid-in-multipath_ctr/20210320-195717
        git checkout f8f908f78541c7da502df6f31f772ca6e8c71732
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "scsi_device_from_queue" [drivers/md/dm-multipath.ko] undefined!
>> ERROR: modpost: "scsi_vpd_lun_id" [drivers/md/dm-multipath.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Christoph Hellwig March 22, 2021, 8:11 a.m. UTC | #2

On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> From: Zhiqiang Liu <[e-mail]>
> 
> When we make IO stress test on multipath device, there will
> be a metadata err because of wrong path. In the test, we
> concurrent execute 'iscsi device login|logout' and
> 'multipath -r' command with IO stress on multipath device.
> In some case, systemd-udevd may have not time to process
> uevents of iscsi device logout|login, and then 'multipath -r'
> command triggers multipathd daemon calls ioctl to load table
> with incorrect old device info from systemd-udevd.
> Then, one iscsi path may be incorrectly attached to another
> multipath which has different uuid. Finally, the metadata err
> occurs when umounting filesystem to down write metadata on
> the iscsi device which is actually not owned by the multipath
> device.
> 
> So we need to check whether all pgpaths of one multipath have
> the same uuid, if not, we should throw a error.
> 
> Signed-off-by: Zhiqiang Liu <[e-mail]>
> Signed-off-by: lixiaokeng <[e-mail]>
> Signed-off-by: linfeilong <[e-mail]>
> Signed-off-by: Wubo <[e-mail]>
> ---
>  drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/scsi_lib.c |  1 +
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index bced42f082b0..f0b995784b53 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -24,6 +24,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/delay.h>
>  #include <scsi/scsi_dh.h>
> +#include <linux/dm-ioctl.h>
>  #include <linux/atomic.h>
>  #include <linux/blk-mq.h>
> 
> @@ -1169,6 +1170,45 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
>  	return r;
>  }
> 
> +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> +#define MPATH_UUID_PREFIX_LEN 7
> +static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
> +{
> +	char pgpath_uuid[DM_UUID_LEN] = {0};
> +	struct request_queue *q;
> +	struct pgpath *pgpath;
> +	struct scsi_device *sdev;
> +	ssize_t count;
> +	int r = 0;
> +
> +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
> +		q = bdev_get_queue(pgpath->path.dev->bdev);
> +		sdev = scsi_device_from_queue(q);

Common dm-multipath code should never poke into scsi internals.  This
is something for the device handler to check.  It probably also won't
work for all older devices.

Mike Snitzer March 22, 2021, 2:22 p.m. UTC | #3

On Mon, Mar 22 2021 at  4:11am -0400,
Christoph Hellwig <[e-mail]> wrote:

> On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> > From: Zhiqiang Liu <[e-mail]>
> > 
> > When we make IO stress test on multipath device, there will
> > be a metadata err because of wrong path. In the test, we
> > concurrent execute 'iscsi device login|logout' and
> > 'multipath -r' command with IO stress on multipath device.
> > In some case, systemd-udevd may have not time to process
> > uevents of iscsi device logout|login, and then 'multipath -r'
> > command triggers multipathd daemon calls ioctl to load table
> > with incorrect old device info from systemd-udevd.
> > Then, one iscsi path may be incorrectly attached to another
> > multipath which has different uuid. Finally, the metadata err
> > occurs when umounting filesystem to down write metadata on
> > the iscsi device which is actually not owned by the multipath
> > device.
> > 
> > So we need to check whether all pgpaths of one multipath have
> > the same uuid, if not, we should throw a error.
> > 
> > Signed-off-by: Zhiqiang Liu <[e-mail]>
> > Signed-off-by: lixiaokeng <[e-mail]>
> > Signed-off-by: linfeilong <[e-mail]>
> > Signed-off-by: Wubo <[e-mail]>
> > ---
> >  drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/scsi/scsi_lib.c |  1 +
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > index bced42f082b0..f0b995784b53 100644
> > --- a/drivers/md/dm-mpath.c
> > +++ b/drivers/md/dm-mpath.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/workqueue.h>
> >  #include <linux/delay.h>
> >  #include <scsi/scsi_dh.h>
> > +#include <linux/dm-ioctl.h>
> >  #include <linux/atomic.h>
> >  #include <linux/blk-mq.h>
> > 
> > @@ -1169,6 +1170,45 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
> >  	return r;
> >  }
> > 
> > +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> > +#define MPATH_UUID_PREFIX_LEN 7
> > +static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
> > +{
> > +	char pgpath_uuid[DM_UUID_LEN] = {0};
> > +	struct request_queue *q;
> > +	struct pgpath *pgpath;
> > +	struct scsi_device *sdev;
> > +	ssize_t count;
> > +	int r = 0;
> > +
> > +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
> > +		q = bdev_get_queue(pgpath->path.dev->bdev);
> > +		sdev = scsi_device_from_queue(q);
> 
> Common dm-multipath code should never poke into scsi internals.  This
> is something for the device handler to check.  It probably also won't
> work for all older devices.

Definitely.

But that aside, userspace (multipathd) _should_ be able to do extra
validation, _before_ pushing down a new table to the kernel, rather than
forcing the kernel to do it.

Li Xiao Keng March 23, 2021, 7:47 a.m. UTC | #4

On 2021/3/22 22:22, Mike Snitzer wrote:
> On Mon, Mar 22 2021 at  4:11am -0400,
> Christoph Hellwig <[e-mail]> wrote:
> 
>> On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
>>> From: Zhiqiang Liu <[e-mail]>
>>>
>>> When we make IO stress test on multipath device, there will
>>> be a metadata err because of wrong path. In the test, we
>>> concurrent execute 'iscsi device login|logout' and
>>> 'multipath -r' command with IO stress on multipath device.
>>> In some case, systemd-udevd may have not time to process
>>> uevents of iscsi device logout|login, and then 'multipath -r'
>>> command triggers multipathd daemon calls ioctl to load table
>>> with incorrect old device info from systemd-udevd.
>>> Then, one iscsi path may be incorrectly attached to another
>>> multipath which has different uuid. Finally, the metadata err
>>> occurs when umounting filesystem to down write metadata on
>>> the iscsi device which is actually not owned by the multipath
>>> device.
>>>
>>> So we need to check whether all pgpaths of one multipath have
>>> the same uuid, if not, we should throw a error.
>>>
>>> Signed-off-by: Zhiqiang Liu <[e-mail]>
>>> Signed-off-by: lixiaokeng <[e-mail]>
>>> Signed-off-by: linfeilong <[e-mail]>
>>> Signed-off-by: Wubo <[e-mail]>
>>> ---
>>>  drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/scsi/scsi_lib.c |  1 +
>>>  2 files changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>> index bced42f082b0..f0b995784b53 100644
>>> --- a/drivers/md/dm-mpath.c
>>> +++ b/drivers/md/dm-mpath.c
>>> @@ -24,6 +24,7 @@
>>>  #include <linux/workqueue.h>
>>>  #include <linux/delay.h>
>>>  #include <scsi/scsi_dh.h>
>>> +#include <linux/dm-ioctl.h>
>>>  #include <linux/atomic.h>
>>>  #include <linux/blk-mq.h>
>>>
>>> @@ -1169,6 +1170,45 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
>>>  	return r;
>>>  }
>>>
>>> +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
>>> +#define MPATH_UUID_PREFIX_LEN 7
>>> +static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
>>> +{
>>> +	char pgpath_uuid[DM_UUID_LEN] = {0};
>>> +	struct request_queue *q;
>>> +	struct pgpath *pgpath;
>>> +	struct scsi_device *sdev;
>>> +	ssize_t count;
>>> +	int r = 0;
>>> +
>>> +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
>>> +		q = bdev_get_queue(pgpath->path.dev->bdev);
>>> +		sdev = scsi_device_from_queue(q);
>>
>> Common dm-multipath code should never poke into scsi internals.  This
>> is something for the device handler to check.  It probably also won't
>> work for all older devices.
> 
> Definitely.
> 
> But that aside, userspace (multipathd) _should_ be able to do extra
> validation, _before_ pushing down a new table to the kernel, rather than
> forcing the kernel to do it.
> 

Martin (committer of multipath-tools) said that:
"Don't get me wrong, I don't argue against tough testing. But we should
be aware that there are always time intervals during which multipathd's
picture of the present devices is different from what the kernel sees."

It is difficult to solve this in multipathd.

Regards,
Lixiaokeng

Ewan Milne March 23, 2021, 5:11 p.m. UTC | #5

On Tue, 2021-03-23 at 15:47 +0800, lixiaokeng wrote:
> 
> On 2021/3/22 22:22, Mike Snitzer wrote:
> > On Mon, Mar 22 2021 at  4:11am -0400,
> > Christoph Hellwig <[e-mail]> wrote:
> > 
> > > On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> > > > From: Zhiqiang Liu <[e-mail]>
> > > > 
> > > > When we make IO stress test on multipath device, there will
> > > > be a metadata err because of wrong path. In the test, we
> > > > concurrent execute 'iscsi device login|logout' and
> > > > 'multipath -r' command with IO stress on multipath device.
> > > > In some case, systemd-udevd may have not time to process
> > > > uevents of iscsi device logout|login, and then 'multipath -r'
> > > > command triggers multipathd daemon calls ioctl to load table
> > > > with incorrect old device info from systemd-udevd.
> > > > Then, one iscsi path may be incorrectly attached to another
> > > > multipath which has different uuid. Finally, the metadata err
> > > > occurs when umounting filesystem to down write metadata on
> > > > the iscsi device which is actually not owned by the multipath
> > > > device.
> > > > 
> > > > So we need to check whether all pgpaths of one multipath have
> > > > the same uuid, if not, we should throw a error.
> > > > 
> > > > Signed-off-by: Zhiqiang Liu <[e-mail]>
> > > > Signed-off-by: lixiaokeng <[e-mail]>
> > > > Signed-off-by: linfeilong <[e-mail]>
> > > > Signed-off-by: Wubo <[e-mail]>
> > > > ---
> > > >  drivers/md/dm-mpath.c   | 52
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/scsi/scsi_lib.c |  1 +
> > > >  2 files changed, 53 insertions(+)
> > > > 
> > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > > index bced42f082b0..f0b995784b53 100644
> > > > --- a/drivers/md/dm-mpath.c
> > > > +++ b/drivers/md/dm-mpath.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <linux/workqueue.h>
> > > >  #include <linux/delay.h>
> > > >  #include <scsi/scsi_dh.h>
> > > > +#include <linux/dm-ioctl.h>
> > > >  #include <linux/atomic.h>
> > > >  #include <linux/blk-mq.h>
> > > > 
> > > > @@ -1169,6 +1170,45 @@ static int parse_features(struct
> > > > dm_arg_set *as, struct multipath *m)
> > > >  	return r;
> > > >  }
> > > > 
> > > > +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> > > > +#define MPATH_UUID_PREFIX_LEN 7
> > > > +static int check_pg_uuid(struct priority_group *pg, char
> > > > *md_uuid)
> > > > +{
> > > > +	char pgpath_uuid[DM_UUID_LEN] = {0};
> > > > +	struct request_queue *q;
> > > > +	struct pgpath *pgpath;
> > > > +	struct scsi_device *sdev;
> > > > +	ssize_t count;
> > > > +	int r = 0;
> > > > +
> > > > +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
> > > > +		q = bdev_get_queue(pgpath->path.dev->bdev);
> > > > +		sdev = scsi_device_from_queue(q);
> > > 
> > > Common dm-multipath code should never poke into scsi
> > > internals.  This
> > > is something for the device handler to check.  It probably also
> > > won't
> > > work for all older devices.
> > 
> > Definitely.
> > 
> > But that aside, userspace (multipathd) _should_ be able to do extra
> > validation, _before_ pushing down a new table to the kernel, rather
> > than
> > forcing the kernel to do it.
> > 
> 
> Martin (committer of multipath-tools) said that:
> "Don't get me wrong, I don't argue against tough testing. But we
> should
> be aware that there are always time intervals during which
> multipathd's
> picture of the present devices is different from what the kernel
> sees."
> 
> It is difficult to solve this in multipathd.
> 
> Regards,
> Lixiaokeng
> 

I think the patch is no good.  There are plenty of devices that don't
support VPD page 83h:

int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
{
        u8 cur_id_type = 0xff;
        u8 cur_id_size = 0;
        unsigned char *d, *cur_id_str;
        unsigned char __rcu *vpd_pg83;
        int id_size = -EINVAL;

        rcu_read_lock();
        vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
        if (!vpd_pg83) {
                rcu_read_unlock();
                return -ENXIO;
        }

and the DM layer should not be looking at the properties of the
underlying devices in this way anyway.  It should be pushed down
to the table.

-Ewan

Zhiqiang Liu March 25, 2021, 1:12 a.m. UTC | #6

On 2021/3/24 1:11, Ewan D. Milne wrote:
> On Tue, 2021-03-23 at 15:47 +0800, lixiaokeng wrote:
>>
>> On 2021/3/22 22:22, Mike Snitzer wrote:
>>> On Mon, Mar 22 2021 at  4:11am -0400,
>>> Christoph Hellwig <[e-mail]> wrote:
>>>
>>>> On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
>>>>> From: Zhiqiang Liu <[e-mail]>
>>>>>
>>>>> When we make IO stress test on multipath device, there will
>>>>> be a metadata err because of wrong path. In the test, we
>>>>> concurrent execute 'iscsi device login|logout' and
>>>>> 'multipath -r' command with IO stress on multipath device.
>>>>> In some case, systemd-udevd may have not time to process
>>>>> uevents of iscsi device logout|login, and then 'multipath -r'
>>>>> command triggers multipathd daemon calls ioctl to load table
>>>>> with incorrect old device info from systemd-udevd.
>>>>> Then, one iscsi path may be incorrectly attached to another
>>>>> multipath which has different uuid. Finally, the metadata err
>>>>> occurs when umounting filesystem to down write metadata on
>>>>> the iscsi device which is actually not owned by the multipath
>>>>> device.
>>>>>
>>>>> So we need to check whether all pgpaths of one multipath have
>>>>> the same uuid, if not, we should throw a error.
>>>>>
>>>>> Signed-off-by: Zhiqiang Liu <[e-mail]>
>>>>> Signed-off-by: lixiaokeng <[e-mail]>
>>>>> Signed-off-by: linfeilong <[e-mail]>
>>>>> Signed-off-by: Wubo <[e-mail]>
>>>>> ---
>>>>>  drivers/md/dm-mpath.c   | 52
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/scsi/scsi_lib.c |  1 +
>>>>>  2 files changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>>>> index bced42f082b0..f0b995784b53 100644
>>>>> --- a/drivers/md/dm-mpath.c
>>>>> +++ b/drivers/md/dm-mpath.c
>>>>> @@ -24,6 +24,7 @@
>>>>>  #include <linux/workqueue.h>
>>>>>  #include <linux/delay.h>
>>>>>  #include <scsi/scsi_dh.h>
>>>>> +#include <linux/dm-ioctl.h>
>>>>>  #include <linux/atomic.h>
>>>>>  #include <linux/blk-mq.h>
>>>>>
>>>>> @@ -1169,6 +1170,45 @@ static int parse_features(struct
>>>>> dm_arg_set *as, struct multipath *m)
>>>>>  	return r;
>>>>>  }
>>>>>
>>>>> +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
>>>>> +#define MPATH_UUID_PREFIX_LEN 7
>>>>> +static int check_pg_uuid(struct priority_group *pg, char
>>>>> *md_uuid)
>>>>> +{
>>>>> +	char pgpath_uuid[DM_UUID_LEN] = {0};
>>>>> +	struct request_queue *q;
>>>>> +	struct pgpath *pgpath;
>>>>> +	struct scsi_device *sdev;
>>>>> +	ssize_t count;
>>>>> +	int r = 0;
>>>>> +
>>>>> +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
>>>>> +		q = bdev_get_queue(pgpath->path.dev->bdev);
>>>>> +		sdev = scsi_device_from_queue(q);
>>>>
>>>> Common dm-multipath code should never poke into scsi
>>>> internals.  This
>>>> is something for the device handler to check.  It probably also
>>>> won't
>>>> work for all older devices.
>>>
>>> Definitely.
>>>
>>> But that aside, userspace (multipathd) _should_ be able to do extra
>>> validation, _before_ pushing down a new table to the kernel, rather
>>> than
>>> forcing the kernel to do it.
>>>
>>
>> Martin (committer of multipath-tools) said that:
>> "Don't get me wrong, I don't argue against tough testing. But we
>> should
>> be aware that there are always time intervals during which
>> multipathd's
>> picture of the present devices is different from what the kernel
>> sees."
>>
>> It is difficult to solve this in multipathd.
>>
>> Regards,
>> Lixiaokeng
>>
> 
> I think the patch is no good.  There are plenty of devices that don't
> support VPD page 83h:
> 
> int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
> {
>         u8 cur_id_type = 0xff;
>         u8 cur_id_size = 0;
>         unsigned char *d, *cur_id_str;
>         unsigned char __rcu *vpd_pg83;
>         int id_size = -EINVAL;
> 
>         rcu_read_lock();
>         vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
>         if (!vpd_pg83) {
>                 rcu_read_unlock();
>                 return -ENXIO;
>         }
> 
> and the DM layer should not be looking at the properties of the
> underlying devices in this way anyway.  It should be pushed down
> to the table.
> 
Thanks for your suggestion.
I will have a try to modify the patch as your advice.


Regards
Zhiqiang Liu.
> -Ewan
> 
> 
> 
> 
> .
>

Zhiqiang Liu March 25, 2021, 1:21 a.m. UTC | #7

On 2021/3/22 22:22, Mike Snitzer wrote:
> On Mon, Mar 22 2021 at  4:11am -0400,
> Christoph Hellwig <[e-mail]> wrote:
> 
>> On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
>>> From: Zhiqiang Liu <[e-mail]>
>>>
>>> When we make IO stress test on multipath device, there will
>>> be a metadata err because of wrong path. In the test, we
>>> concurrent execute 'iscsi device login|logout' and
>>> 'multipath -r' command with IO stress on multipath device.
>>> In some case, systemd-udevd may have not time to process
>>> uevents of iscsi device logout|login, and then 'multipath -r'
>>> command triggers multipathd daemon calls ioctl to load table
>>> with incorrect old device info from systemd-udevd.
>>> Then, one iscsi path may be incorrectly attached to another
>>> multipath which has different uuid. Finally, the metadata err
>>> occurs when umounting filesystem to down write metadata on
>>> the iscsi device which is actually not owned by the multipath
>>> device.
>>>
>>> So we need to check whether all pgpaths of one multipath have
>>> the same uuid, if not, we should throw a error.
>>>
>>> Signed-off-by: Zhiqiang Liu <[e-mail]>
>>> Signed-off-by: lixiaokeng <[e-mail]>
>>> Signed-off-by: linfeilong <[e-mail]>
>>> Signed-off-by: Wubo <[e-mail]>
>>> ---
>>>  drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
>>>  drivers/scsi/scsi_lib.c |  1 +
>>>  2 files changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>>> index bced42f082b0..f0b995784b53 100644
>>> --- a/drivers/md/dm-mpath.c
>>> +++ b/drivers/md/dm-mpath.c
>>> @@ -24,6 +24,7 @@
>>>  #include <linux/workqueue.h>
>>>  #include <linux/delay.h>
>>>  #include <scsi/scsi_dh.h>
>>> +#include <linux/dm-ioctl.h>
>>>  #include <linux/atomic.h>
>>>  #include <linux/blk-mq.h>
>>>
>>> @@ -1169,6 +1170,45 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
>>>  	return r;
>>>  }
>>>
>>> +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
>>> +#define MPATH_UUID_PREFIX_LEN 7
>>> +static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
>>> +{
>>> +	char pgpath_uuid[DM_UUID_LEN] = {0};
>>> +	struct request_queue *q;
>>> +	struct pgpath *pgpath;
>>> +	struct scsi_device *sdev;
>>> +	ssize_t count;
>>> +	int r = 0;
>>> +
>>> +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
>>> +		q = bdev_get_queue(pgpath->path.dev->bdev);
>>> +		sdev = scsi_device_from_queue(q);
>>
>> Common dm-multipath code should never poke into scsi internals.  This
>> is something for the device handler to check.  It probably also won't
>> work for all older devices.
> 
> Definitely.
> 
> But that aside, userspace (multipathd) _should_ be able to do extra
> validation, _before_ pushing down a new table to the kernel, rather than
> forcing the kernel to do it.

As your said, it is better to do extra validation in userspace (multipathd).
However, in some cases, the userspace cannot see the real-time present devices
info as Martin (committer of multipath-tools) said.
In addition, the kernel can see right device info in the table at any time,
so the uuid check in kernel can ensure one multipath is composed with paths mapped to
the same device.

Considering the severity of the wrong path in multipath, I think it worths more
checking.

Regards
Zhiqiang Liu.


> 
> .
>

Mike Snitzer March 25, 2021, 3:14 p.m. UTC | #8

On Wed, Mar 24 2021 at  9:21pm -0400,
Zhiqiang Liu <[e-mail]> wrote:

> 
> 
> On 2021/3/22 22:22, Mike Snitzer wrote:
> > On Mon, Mar 22 2021 at  4:11am -0400,
> > Christoph Hellwig <[e-mail]> wrote:
> > 
> >> On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> >>> From: Zhiqiang Liu <[e-mail]>
> >>>
> >>> When we make IO stress test on multipath device, there will
> >>> be a metadata err because of wrong path. In the test, we
> >>> concurrent execute 'iscsi device login|logout' and
> >>> 'multipath -r' command with IO stress on multipath device.
> >>> In some case, systemd-udevd may have not time to process
> >>> uevents of iscsi device logout|login, and then 'multipath -r'
> >>> command triggers multipathd daemon calls ioctl to load table
> >>> with incorrect old device info from systemd-udevd.
> >>> Then, one iscsi path may be incorrectly attached to another
> >>> multipath which has different uuid. Finally, the metadata err
> >>> occurs when umounting filesystem to down write metadata on
> >>> the iscsi device which is actually not owned by the multipath
> >>> device.
> >>>
> >>> So we need to check whether all pgpaths of one multipath have
> >>> the same uuid, if not, we should throw a error.
> >>>
> >>> Signed-off-by: Zhiqiang Liu <[e-mail]>
> >>> Signed-off-by: lixiaokeng <[e-mail]>
> >>> Signed-off-by: linfeilong <[e-mail]>
> >>> Signed-off-by: Wubo <[e-mail]>
> >>> ---
> >>>  drivers/md/dm-mpath.c   | 52 +++++++++++++++++++++++++++++++++++++++++
> >>>  drivers/scsi/scsi_lib.c |  1 +
> >>>  2 files changed, 53 insertions(+)
> >>>
> >>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> >>> index bced42f082b0..f0b995784b53 100644
> >>> --- a/drivers/md/dm-mpath.c
> >>> +++ b/drivers/md/dm-mpath.c
> >>> @@ -24,6 +24,7 @@
> >>>  #include <linux/workqueue.h>
> >>>  #include <linux/delay.h>
> >>>  #include <scsi/scsi_dh.h>
> >>> +#include <linux/dm-ioctl.h>
> >>>  #include <linux/atomic.h>
> >>>  #include <linux/blk-mq.h>
> >>>
> >>> @@ -1169,6 +1170,45 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
> >>>  	return r;
> >>>  }
> >>>
> >>> +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> >>> +#define MPATH_UUID_PREFIX_LEN 7
> >>> +static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
> >>> +{
> >>> +	char pgpath_uuid[DM_UUID_LEN] = {0};
> >>> +	struct request_queue *q;
> >>> +	struct pgpath *pgpath;
> >>> +	struct scsi_device *sdev;
> >>> +	ssize_t count;
> >>> +	int r = 0;
> >>> +
> >>> +	list_for_each_entry(pgpath, &pg->pgpaths, list) {
> >>> +		q = bdev_get_queue(pgpath->path.dev->bdev);
> >>> +		sdev = scsi_device_from_queue(q);
> >>
> >> Common dm-multipath code should never poke into scsi internals.  This
> >> is something for the device handler to check.  It probably also won't
> >> work for all older devices.
> > 
> > Definitely.
> > 
> > But that aside, userspace (multipathd) _should_ be able to do extra
> > validation, _before_ pushing down a new table to the kernel, rather than
> > forcing the kernel to do it.
> 
> As your said, it is better to do extra validation in userspace (multipathd).
> However, in some cases, the userspace cannot see the real-time present devices
> info as Martin (committer of multipath-tools) said.
> In addition, the kernel can see right device info in the table at any time,
> so the uuid check in kernel can ensure one multipath is composed with paths mapped to
> the same device.
> 
> Considering the severity of the wrong path in multipath, I think it worths more
> checking.

As already said: this should be fixable in userspace.  Please work with
multipath-tools developers to address this.

Mike

Martin Wilck March 26, 2021, 5:12 p.m. UTC | #9

On Thu, 2021-03-25 at 11:14 -0400, Mike Snitzer wrote:
> On Wed, Mar 24 2021 at  9:21pm -0400,
> Zhiqiang Liu <[e-mail]> wrote:
> 
> > 
> > 
> > On 2021/3/22 22:22, Mike Snitzer wrote:
> > > On Mon, Mar 22 2021 at  4:11am -0400,
> > > Christoph Hellwig <[e-mail]> wrote:
> > > 
> > > > On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> > > > > From: Zhiqiang Liu <[e-mail]>
> > > > > 
> > > > > When we make IO stress test on multipath device, there will
> > > > > be a metadata err because of wrong path. In the test, we
> > > > > concurrent execute 'iscsi device login|logout' and
> > > > > 'multipath -r' command with IO stress on multipath device.
> > > > > In some case, systemd-udevd may have not time to process
> > > > > uevents of iscsi device logout|login, and then 'multipath -r'
> > > > > command triggers multipathd daemon calls ioctl to load table
> > > > > with incorrect old device info from systemd-udevd.
> > > > > Then, one iscsi path may be incorrectly attached to another
> > > > > multipath which has different uuid. Finally, the metadata err
> > > > > occurs when umounting filesystem to down write metadata on
> > > > > the iscsi device which is actually not owned by the multipath
> > > > > device.
> > > > > 
> > > > > So we need to check whether all pgpaths of one multipath have
> > > > > the same uuid, if not, we should throw a error.
> > > > > 
> > > > > Signed-off-by: Zhiqiang Liu <[e-mail]>
> > > > > Signed-off-by: lixiaokeng <[e-mail]>
> > > > > Signed-off-by: linfeilong <[e-mail]>
> > > > > Signed-off-by: Wubo <[e-mail]>
> > > > > ---
> > > > >  drivers/md/dm-mpath.c   | 52
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/scsi/scsi_lib.c |  1 +
> > > > >  2 files changed, 53 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > > > index bced42f082b0..f0b995784b53 100644
> > > > > --- a/drivers/md/dm-mpath.c
> > > > > +++ b/drivers/md/dm-mpath.c
> > > > > @@ -24,6 +24,7 @@
> > > > >  #include <linux/workqueue.h>
> > > > >  #include <linux/delay.h>
> > > > >  #include <scsi/scsi_dh.h>
> > > > > +#include <linux/dm-ioctl.h>
> > > > >  #include <linux/atomic.h>
> > > > >  #include <linux/blk-mq.h>
> > > > > 
> > > > > @@ -1169,6 +1170,45 @@ static int parse_features(struct
> > > > > dm_arg_set *as, struct multipath *m)
> > > > >         return r;
> > > > >  }
> > > > > 
> > > > > +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> > > > > +#define MPATH_UUID_PREFIX_LEN 7
> > > > > +static int check_pg_uuid(struct priority_group *pg, char
> > > > > *md_uuid)
> > > > > +{
> > > > > +       char pgpath_uuid[DM_UUID_LEN] = {0};
> > > > > +       struct request_queue *q;
> > > > > +       struct pgpath *pgpath;
> > > > > +       struct scsi_device *sdev;
> > > > > +       ssize_t count;
> > > > > +       int r = 0;
> > > > > +
> > > > > +       list_for_each_entry(pgpath, &pg->pgpaths, list) {
> > > > > +               q = bdev_get_queue(pgpath->path.dev->bdev);
> > > > > +               sdev = scsi_device_from_queue(q);
> > > > 
> > > > Common dm-multipath code should never poke into scsi
> > > > internals.  This
> > > > is something for the device handler to check.  It probably also
> > > > won't
> > > > work for all older devices.
> > > 
> > > Definitely.
> > > 
> > > But that aside, userspace (multipathd) _should_ be able to do
> > > extra
> > > validation, _before_ pushing down a new table to the kernel,
> > > rather than
> > > forcing the kernel to do it.
> > 
> > As your said, it is better to do extra validation in userspace
> > (multipathd).
> > However, in some cases, the userspace cannot see the real-time
> > present devices
> > info as Martin (committer of multipath-tools) said.
> > In addition, the kernel can see right device info in the table at
> > any time,
> > so the uuid check in kernel can ensure one multipath is composed
> > with paths mapped to
> > the same device.
> > 
> > Considering the severity of the wrong path in multipath, I think it
> > worths more
> > checking.
> 
> As already said: this should be fixable in userspace.  Please work
> with
> multipath-tools developers to address this.

I agree this patch won't help, because the kernel doesn't (re)attach
devices to multipath maps by itself. If multipathd actively adds a
device to a map, it must check the WWID beforehand, and so it does (and
has been doing so for years).

But in general, it's hard to avoid WWID mismatches entirely in user
space. We have no problem if a device is removed an re-added. But if it
looks like a device just having been offline or unreachable for some
time and then reappear, it gets tricky. We might even miss the fact
that the device was temporarily away. multipathd can't constantly poll
devices just to detect changes - and what if the sysfs vpd attributes
stay the same because the kernel didn't even notice?

It would be great if userspace could rely on the kernel to deliver
events in such cases. I want look into monitoring SCSI UNIT ATTENTION
events, which multipathd currently doesn't. That might cover many
situations. But I've been told that in some situations really no event
arrived in user space, and I'm not sure if that was a fault of the
storage involved (no UNIT ATTENTION sent) or something else.

Another possibility would be that the kernel used sysfs_notify() for
the inquiry or vpd_pgXY attributes for SCSI (and similar attirbutes for
other device types).

Regards
Martin

Patch

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index bced42f082b0..f0b995784b53 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -24,6 +24,7 @@ 
 #include <linux/workqueue.h>
 #include <linux/delay.h>
 #include <scsi/scsi_dh.h>
+#include <linux/dm-ioctl.h>
 #include <linux/atomic.h>
 #include <linux/blk-mq.h>

@@ -1169,6 +1170,45 @@  static int parse_features(struct dm_arg_set *as, struct multipath *m)
 	return r;
 }

+#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
+#define MPATH_UUID_PREFIX_LEN 7
+static int check_pg_uuid(struct priority_group *pg, char *md_uuid)
+{
+	char pgpath_uuid[DM_UUID_LEN] = {0};
+	struct request_queue *q;
+	struct pgpath *pgpath;
+	struct scsi_device *sdev;
+	ssize_t count;
+	int r = 0;
+
+	list_for_each_entry(pgpath, &pg->pgpaths, list) {
+		q = bdev_get_queue(pgpath->path.dev->bdev);
+		sdev = scsi_device_from_queue(q);
+		if (!sdev) {
+			r = -EINVAL;
+			goto out;
+		}
+
+		count = scsi_vpd_lun_id(sdev, pgpath_uuid, DM_UUID_LEN);
+		if (count <= SCSI_VPD_LUN_ID_PREFIX_LEN) {
+			r = -EINVAL;
+			put_device(&sdev->sdev_gendev);
+			goto out;
+		}
+
+		if (strcmp(md_uuid + MPATH_UUID_PREFIX_LEN,
+			   pgpath_uuid + SCSI_VPD_LUN_ID_PREFIX_LEN)) {
+			r = -EINVAL;
+			put_device(&sdev->sdev_gendev);
+			goto out;
+		}
+		put_device(&sdev->sdev_gendev);
+	}
+
+out:
+	return r;
+}
+
 static int multipath_ctr(struct dm_target *ti, unsigned argc, char **argv)
 {
 	/* target arguments */
@@ -1183,6 +1223,7 @@  static int multipath_ctr(struct dm_target *ti, unsigned argc, char **argv)
 	unsigned pg_count = 0;
 	unsigned next_pg_num;
 	unsigned long flags;
+	char md_uuid[DM_UUID_LEN] = {0};

 	as.argc = argc;
 	as.argv = argv;
@@ -1220,6 +1261,11 @@  static int multipath_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad;
 	}

+	if (dm_copy_name_and_uuid(dm_table_get_md(ti->table), NULL, md_uuid)) {
+		r = -ENXIO;
+		goto bad;
+	}
+
 	/* parse the priority groups */
 	while (as.argc) {
 		struct priority_group *pg;
@@ -1231,6 +1277,12 @@  static int multipath_ctr(struct dm_target *ti, unsigned argc, char **argv)
 			goto bad;
 		}

+		if (check_pg_uuid(pg, md_uuid)) {
+			ti->error = "uuid of pgpaths mismatch";
+			r = -EINVAL;
+			goto bad;
+		}
+
 		nr_valid_paths += pg->nr_pgpaths;
 		atomic_set(&m->nr_valid_paths, nr_valid_paths);

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7d52a11e1b61..fee82262a227 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1953,6 +1953,7 @@  struct scsi_device *scsi_device_from_queue(struct request_queue *q)

 	return sdev;
 }
+EXPORT_SYMBOL(scsi_device_from_queue);

 /**
  * scsi_block_requests - Utility function used by low-level drivers to prevent