| Message ID | 20221010144123.252329-1-luomeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [resend] dm mpath: fix UAF in multipath_message() | expand |
On Mon, Oct 10 2022 at 10:41P -0400, Luo Meng <luomeng@huaweicloud.com> wrote: > From: Luo Meng <luomeng12@huawei.com> > > If dm_get_device() create dd in multipath_message(), > and then call table_deps() after dm_put_table_device(), > it will lead to concurrency UAF bugs. > > One of the concurrency UAF can be shown as below: > > (USE) | (FREE) > | target_message > | multipath_message > | dm_put_device > | dm_put_table_device # > | kfree(td) # table_device *td > ioctl # DM_TABLE_DEPS_CMD | ... > table_deps | ... > dm_get_live_or_inactive_table | ... > retrieve_dep | ... > list_for_each_entry | ... > deps->dev[count++] = | ... > huge_encode_dev | ... > (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) > | kfree(dd) # dm_dev_internal > > The root cause of UAF bugs is that find_device() failed in > dm_get_device() and will create dd and refcount set 1, kfree() > in dm_put_table() is not protected. When td, which there are > still pointers point to, is released, the concurrency UAF bug > will happen. > > This patch add a flag to determine whether to create a new dd. > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) > Signed-off-by: Luo Meng <luomeng12@huawei.com> > --- > drivers/md/dm-mpath.c | 2 +- > drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- > include/linux/device-mapper.h | 2 ++ > 3 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index 0e325469a252..1f51059520ac 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, > goto out; > } > > - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); > + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); > if (r) { > DMWARN("message: error getting device %s", > argv[1]); > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index d8034ff0cb24..ad18a41f1608 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) > } > EXPORT_SYMBOL_GPL(dm_get_dev_t); > > -/* > - * Add a device to the list, or just increment the usage count if > - * it's already present. > - */ > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > - struct dm_dev **result) > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > + struct dm_dev **result, bool create_dd) > { > int r; > dev_t dev; > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > dd = find_device(&t->devices, dev); > if (!dd) { > - dd = kmalloc(sizeof(*dd), GFP_KERNEL); > - if (!dd) > - return -ENOMEM; > - > - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > - kfree(dd); > - return r; > - } > + if (create_dd) { > + dd = kmalloc(sizeof(*dd), GFP_KERNEL); > + if (!dd) > + return -ENOMEM; > > - refcount_set(&dd->count, 1); > - list_add(&dd->list, &t->devices); > - goto out; > + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > + kfree(dd); > + return r; > + } > > + refcount_set(&dd->count, 1); > + list_add(&dd->list, &t->devices); > + goto out; > + } else > + return -ENODEV; > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > r = upgrade_mode(dd, mode, t->md); > if (r) > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > *result = dd->dm_dev; > return 0; > } > +EXPORT_SYMBOL(__dm_get_device); > + > +/* > + * Add a device to the list, or just increment the usage count if > + * it's already present. > + */ > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > + struct dm_dev **result) > +{ > + return __dm_get_device(ti, path, mode, result, true); > +} > EXPORT_SYMBOL(dm_get_device); > > static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > index 04c6acf7faaa..ef4031a844b6 100644 > --- a/include/linux/device-mapper.h > +++ b/include/linux/device-mapper.h > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); > int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > struct dm_dev **result); > void dm_put_device(struct dm_target *ti, struct dm_dev *d); > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > + struct dm_dev **result, bool create_dd); > > /* > * Information about a target type > -- > 2.31.1 > This patch is treating the one multipath case like it is only consumer of dm_get_device() that might have this issue. It feels too focused on an instance where dm_get_device()'s side-effect of creating the device is unwelcome. Feels like you're treating the symptom rather than the problem (e.g. that the "kfree() in dm_put_table() is not protected"?) Mike -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
在 2022/10/18 3:56, Mike Snitzer 写道: > On Mon, Oct 10 2022 at 10:41P -0400, > Luo Meng <luomeng@huaweicloud.com> wrote: > >> From: Luo Meng <luomeng12@huawei.com> >> >> If dm_get_device() create dd in multipath_message(), >> and then call table_deps() after dm_put_table_device(), >> it will lead to concurrency UAF bugs. >> >> One of the concurrency UAF can be shown as below: >> >> (USE) | (FREE) >> | target_message >> | multipath_message >> | dm_put_device >> | dm_put_table_device # >> | kfree(td) # table_device *td >> ioctl # DM_TABLE_DEPS_CMD | ... >> table_deps | ... >> dm_get_live_or_inactive_table | ... >> retrieve_dep | ... >> list_for_each_entry | ... >> deps->dev[count++] = | ... >> huge_encode_dev | ... >> (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) >> | kfree(dd) # dm_dev_internal >> >> The root cause of UAF bugs is that find_device() failed in >> dm_get_device() and will create dd and refcount set 1, kfree() >> in dm_put_table() is not protected. When td, which there are >> still pointers point to, is released, the concurrency UAF bug >> will happen. >> >> This patch add a flag to determine whether to create a new dd. >> >> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) >> Signed-off-by: Luo Meng <luomeng12@huawei.com> >> --- >> drivers/md/dm-mpath.c | 2 +- >> drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- >> include/linux/device-mapper.h | 2 ++ >> 3 files changed, 29 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c >> index 0e325469a252..1f51059520ac 100644 >> --- a/drivers/md/dm-mpath.c >> +++ b/drivers/md/dm-mpath.c >> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, >> goto out; >> } >> >> - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); >> + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); >> if (r) { >> DMWARN("message: error getting device %s", >> argv[1]); >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >> index d8034ff0cb24..ad18a41f1608 100644 >> --- a/drivers/md/dm-table.c >> +++ b/drivers/md/dm-table.c >> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) >> } >> EXPORT_SYMBOL_GPL(dm_get_dev_t); >> >> -/* >> - * Add a device to the list, or just increment the usage count if >> - * it's already present. >> - */ >> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> - struct dm_dev **result) >> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result, bool create_dd) >> { >> int r; >> dev_t dev; >> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> >> dd = find_device(&t->devices, dev); >> if (!dd) { >> - dd = kmalloc(sizeof(*dd), GFP_KERNEL); >> - if (!dd) >> - return -ENOMEM; >> - >> - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >> - kfree(dd); >> - return r; >> - } >> + if (create_dd) { >> + dd = kmalloc(sizeof(*dd), GFP_KERNEL); >> + if (!dd) >> + return -ENOMEM; >> >> - refcount_set(&dd->count, 1); >> - list_add(&dd->list, &t->devices); >> - goto out; >> + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >> + kfree(dd); >> + return r; >> + } >> >> + refcount_set(&dd->count, 1); >> + list_add(&dd->list, &t->devices); >> + goto out; >> + } else >> + return -ENODEV; >> } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { >> r = upgrade_mode(dd, mode, t->md); >> if (r) >> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> *result = dd->dm_dev; >> return 0; >> } >> +EXPORT_SYMBOL(__dm_get_device); >> + >> +/* >> + * Add a device to the list, or just increment the usage count if >> + * it's already present. >> + */ >> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result) >> +{ >> + return __dm_get_device(ti, path, mode, result, true); >> +} >> EXPORT_SYMBOL(dm_get_device); >> >> static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, >> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h >> index 04c6acf7faaa..ef4031a844b6 100644 >> --- a/include/linux/device-mapper.h >> +++ b/include/linux/device-mapper.h >> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); >> int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> struct dm_dev **result); >> void dm_put_device(struct dm_target *ti, struct dm_dev *d); >> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result, bool create_dd); >> >> /* >> * Information about a target type >> -- >> 2.31.1 >> > > This patch is treating the one multipath case like it is only consumer > of dm_get_device() that might have this issue. It feels too focused on > an instance where dm_get_device()'s side-effect of creating the device > is unwelcome. Feels like you're treating the symptom rather than the > problem (e.g. that the "kfree() in dm_put_table() is not protected"?) > > Mike > Thanks for your reply. I know it is not the best way to slove the problem, however I have no idea about it. Do you have a better way to fix it? Luo Meng -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
在 2022/10/18 3:56, Mike Snitzer 写道: > On Mon, Oct 10 2022 at 10:41P -0400, > Luo Meng <luomeng@huaweicloud.com> wrote: > >> From: Luo Meng <luomeng12@huawei.com> >> >> If dm_get_device() create dd in multipath_message(), >> and then call table_deps() after dm_put_table_device(), >> it will lead to concurrency UAF bugs. >> >> One of the concurrency UAF can be shown as below: >> >> (USE) | (FREE) >> | target_message >> | multipath_message >> | dm_put_device >> | dm_put_table_device # >> | kfree(td) # table_device *td >> ioctl # DM_TABLE_DEPS_CMD | ... >> table_deps | ... >> dm_get_live_or_inactive_table | ... >> retrieve_dep | ... >> list_for_each_entry | ... >> deps->dev[count++] = | ... >> huge_encode_dev | ... >> (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) >> | kfree(dd) # dm_dev_internal >> >> The root cause of UAF bugs is that find_device() failed in >> dm_get_device() and will create dd and refcount set 1, kfree() >> in dm_put_table() is not protected. When td, which there are >> still pointers point to, is released, the concurrency UAF bug >> will happen. >> >> This patch add a flag to determine whether to create a new dd. >> >> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) >> Signed-off-by: Luo Meng <luomeng12@huawei.com> >> --- >> drivers/md/dm-mpath.c | 2 +- >> drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- >> include/linux/device-mapper.h | 2 ++ >> 3 files changed, 29 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c >> index 0e325469a252..1f51059520ac 100644 >> --- a/drivers/md/dm-mpath.c >> +++ b/drivers/md/dm-mpath.c >> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, >> goto out; >> } >> >> - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); >> + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); >> if (r) { >> DMWARN("message: error getting device %s", >> argv[1]); >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >> index d8034ff0cb24..ad18a41f1608 100644 >> --- a/drivers/md/dm-table.c >> +++ b/drivers/md/dm-table.c >> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) >> } >> EXPORT_SYMBOL_GPL(dm_get_dev_t); >> >> -/* >> - * Add a device to the list, or just increment the usage count if >> - * it's already present. >> - */ >> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> - struct dm_dev **result) >> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result, bool create_dd) >> { >> int r; >> dev_t dev; >> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> >> dd = find_device(&t->devices, dev); >> if (!dd) { >> - dd = kmalloc(sizeof(*dd), GFP_KERNEL); >> - if (!dd) >> - return -ENOMEM; >> - >> - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >> - kfree(dd); >> - return r; >> - } >> + if (create_dd) { >> + dd = kmalloc(sizeof(*dd), GFP_KERNEL); >> + if (!dd) >> + return -ENOMEM; >> >> - refcount_set(&dd->count, 1); >> - list_add(&dd->list, &t->devices); >> - goto out; >> + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >> + kfree(dd); >> + return r; >> + } >> >> + refcount_set(&dd->count, 1); >> + list_add(&dd->list, &t->devices); >> + goto out; >> + } else >> + return -ENODEV; >> } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { >> r = upgrade_mode(dd, mode, t->md); >> if (r) >> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> *result = dd->dm_dev; >> return 0; >> } >> +EXPORT_SYMBOL(__dm_get_device); >> + >> +/* >> + * Add a device to the list, or just increment the usage count if >> + * it's already present. >> + */ >> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result) >> +{ >> + return __dm_get_device(ti, path, mode, result, true); >> +} >> EXPORT_SYMBOL(dm_get_device); >> >> static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, >> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h >> index 04c6acf7faaa..ef4031a844b6 100644 >> --- a/include/linux/device-mapper.h >> +++ b/include/linux/device-mapper.h >> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); >> int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> struct dm_dev **result); >> void dm_put_device(struct dm_target *ti, struct dm_dev *d); >> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >> + struct dm_dev **result, bool create_dd); >> >> /* >> * Information about a target type >> -- >> 2.31.1 >> > This patch is treating the one multipath case like it is only consumer > of dm_get_device() that might have this issue. It feels too focused on > an instance where dm_get_device()'s side-effect of creating the device > is unwelcome. Feels like you're treating the symptom rather than the > problem (e.g. that the "kfree() in dm_put_table() is not protected"?) > > Mike In other cases, kfree() in dm_put_table() is protected by srcu. For example: USE FREE table_deps dev_remove dm_get_live_or_inactive_table dm_sync_table // lock srcu_read_lock(&md->io_barrier) // wait for unlock synchronize_srcu(&md->io_barrier) retrieve_deps dm_put_live_table // unlock srcu_read_unlock(&md->io_barrier...) dm_table_destroy linear_dtr dm_put_device dm_put_table_device However, in multipath_message(), the dm_dev is created and destroyed under srcu_read_lock, which means destroying dm_dev in this process and using dm_dev in other process will happen at same time since both of them hold srcu_read_lock. target_message dm_get_live_table // lock multipath_message dm_get_device // created // may be got by other processes dm_put_device // destroyed // may be used by other processes dm_put_live_table // unlock We figured out some other solutions: 1) Judge refcount of dd under some lock before using dm_dev; 2) Get dd from list and use dm_dev under rcu; 3) Implement dm_get_device_xxx() with reference to dm_get_device() for dm-mpath to avoid creating dd when find_device() failed. Compared to solutions above, Luo Meng's patch may be more appropriate. Any suggestions will be appreciated. Li > -- > dm-devel mailing list > dm-devel@redhat.com > https://listman.redhat.com/mailman/listinfo/dm-devel > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Friendly ping ... Thanks, Li 在 2023/5/18 20:11, lilingfeng (A) 写道: > > 在 2022/10/18 3:56, Mike Snitzer 写道: >> On Mon, Oct 10 2022 at 10:41P -0400, >> Luo Meng <luomeng@huaweicloud.com> wrote: >> >>> From: Luo Meng <luomeng12@huawei.com> >>> >>> If dm_get_device() create dd in multipath_message(), >>> and then call table_deps() after dm_put_table_device(), >>> it will lead to concurrency UAF bugs. >>> >>> One of the concurrency UAF can be shown as below: >>> >>> (USE) | (FREE) >>> | target_message >>> | multipath_message >>> | dm_put_device >>> | dm_put_table_device # >>> | kfree(td) # >>> table_device *td >>> ioctl # DM_TABLE_DEPS_CMD | ... >>> table_deps | ... >>> dm_get_live_or_inactive_table | ... >>> retrieve_dep | ... >>> list_for_each_entry | ... >>> deps->dev[count++] = | ... >>> huge_encode_dev | ... >>> (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) >>> | kfree(dd) # >>> dm_dev_internal >>> >>> The root cause of UAF bugs is that find_device() failed in >>> dm_get_device() and will create dd and refcount set 1, kfree() >>> in dm_put_table() is not protected. When td, which there are >>> still pointers point to, is released, the concurrency UAF bug >>> will happen. >>> >>> This patch add a flag to determine whether to create a new dd. >>> >>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range >>> parameters) >>> Signed-off-by: Luo Meng <luomeng12@huawei.com> >>> --- >>> drivers/md/dm-mpath.c | 2 +- >>> drivers/md/dm-table.c | 43 >>> +++++++++++++++++++++-------------- >>> include/linux/device-mapper.h | 2 ++ >>> 3 files changed, 29 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c >>> index 0e325469a252..1f51059520ac 100644 >>> --- a/drivers/md/dm-mpath.c >>> +++ b/drivers/md/dm-mpath.c >>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target >>> *ti, unsigned argc, char **argv, >>> goto out; >>> } >>> - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), >>> &dev); >>> + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), >>> &dev, false); >>> if (r) { >>> DMWARN("message: error getting device %s", >>> argv[1]); >>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >>> index d8034ff0cb24..ad18a41f1608 100644 >>> --- a/drivers/md/dm-table.c >>> +++ b/drivers/md/dm-table.c >>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) >>> } >>> EXPORT_SYMBOL_GPL(dm_get_dev_t); >>> -/* >>> - * Add a device to the list, or just increment the usage count if >>> - * it's already present. >>> - */ >>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t >>> mode, >>> - struct dm_dev **result) >>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t >>> mode, >>> + struct dm_dev **result, bool create_dd) >>> { >>> int r; >>> dev_t dev; >>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const >>> char *path, fmode_t mode, >>> dd = find_device(&t->devices, dev); >>> if (!dd) { >>> - dd = kmalloc(sizeof(*dd), GFP_KERNEL); >>> - if (!dd) >>> - return -ENOMEM; >>> - >>> - if ((r = dm_get_table_device(t->md, dev, mode, >>> &dd->dm_dev))) { >>> - kfree(dd); >>> - return r; >>> - } >>> + if (create_dd) { >>> + dd = kmalloc(sizeof(*dd), GFP_KERNEL); >>> + if (!dd) >>> + return -ENOMEM; >>> - refcount_set(&dd->count, 1); >>> - list_add(&dd->list, &t->devices); >>> - goto out; >>> + if ((r = dm_get_table_device(t->md, dev, mode, >>> &dd->dm_dev))) { >>> + kfree(dd); >>> + return r; >>> + } >>> + refcount_set(&dd->count, 1); >>> + list_add(&dd->list, &t->devices); >>> + goto out; >>> + } else >>> + return -ENODEV; >>> } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { >>> r = upgrade_mode(dd, mode, t->md); >>> if (r) >>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const >>> char *path, fmode_t mode, >>> *result = dd->dm_dev; >>> return 0; >>> } >>> +EXPORT_SYMBOL(__dm_get_device); >>> + >>> +/* >>> + * Add a device to the list, or just increment the usage count if >>> + * it's already present. >>> + */ >>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t >>> mode, >>> + struct dm_dev **result) >>> +{ >>> + return __dm_get_device(ti, path, mode, result, true); >>> +} >>> EXPORT_SYMBOL(dm_get_device); >>> static int dm_set_device_limits(struct dm_target *ti, struct >>> dm_dev *dev, >>> diff --git a/include/linux/device-mapper.h >>> b/include/linux/device-mapper.h >>> index 04c6acf7faaa..ef4031a844b6 100644 >>> --- a/include/linux/device-mapper.h >>> +++ b/include/linux/device-mapper.h >>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); >>> int dm_get_device(struct dm_target *ti, const char *path, fmode_t >>> mode, >>> struct dm_dev **result); >>> void dm_put_device(struct dm_target *ti, struct dm_dev *d); >>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t >>> mode, >>> + struct dm_dev **result, bool create_dd); >>> /* >>> * Information about a target type >>> -- >>> 2.31.1 >>> >> This patch is treating the one multipath case like it is only consumer >> of dm_get_device() that might have this issue. It feels too focused on >> an instance where dm_get_device()'s side-effect of creating the device >> is unwelcome. Feels like you're treating the symptom rather than the >> problem (e.g. that the "kfree() in dm_put_table() is not protected"?) >> >> Mike > > In other cases, kfree() in dm_put_table() is protected by srcu. > For example: > USE FREE > table_deps dev_remove > dm_get_live_or_inactive_table dm_sync_table > // lock > srcu_read_lock(&md->io_barrier) > // wait for unlock > synchronize_srcu(&md->io_barrier) > retrieve_deps > dm_put_live_table > // unlock > srcu_read_unlock(&md->io_barrier...) > dm_table_destroy > linear_dtr > dm_put_device > dm_put_table_device > > However, in multipath_message(), the dm_dev is created and destroyed > under srcu_read_lock, which means destroying dm_dev in this process > and using dm_dev in other process will happen at same time since both > of them hold srcu_read_lock. > > target_message > dm_get_live_table // lock > multipath_message > dm_get_device // created > // may be got by other processes > dm_put_device // destroyed > // may be used by other processes > dm_put_live_table // unlock > > We figured out some other solutions: > 1) Judge refcount of dd under some lock before using dm_dev; > 2) Get dd from list and use dm_dev under rcu; > 3) Implement dm_get_device_xxx() with reference to dm_get_device() > for dm-mpath to avoid creating dd when find_device() failed. > > Compared to solutions above, Luo Meng's patch may be more appropriate. > Any suggestions will be appreciated. > > Li > >> -- >> dm-devel mailing list >> dm-devel@redhat.com >> https://listman.redhat.com/mailman/listinfo/dm-devel >> >> -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, May 18 2023 at 8:11P -0400, lilingfeng (A) <lilingfeng3@huawei.com> wrote: > > 在 2022/10/18 3:56, Mike Snitzer 写道: > > On Mon, Oct 10 2022 at 10:41P -0400, > > Luo Meng <luomeng@huaweicloud.com> wrote: > > > > > From: Luo Meng <luomeng12@huawei.com> > > > > > > If dm_get_device() create dd in multipath_message(), > > > and then call table_deps() after dm_put_table_device(), > > > it will lead to concurrency UAF bugs. > > > > > > One of the concurrency UAF can be shown as below: > > > > > > (USE) | (FREE) > > > | target_message > > > | multipath_message > > > | dm_put_device > > > | dm_put_table_device # > > > | kfree(td) # table_device *td > > > ioctl # DM_TABLE_DEPS_CMD | ... > > > table_deps | ... > > > dm_get_live_or_inactive_table | ... > > > retrieve_dep | ... > > > list_for_each_entry | ... > > > deps->dev[count++] = | ... > > > huge_encode_dev | ... > > > (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) > > > | kfree(dd) # dm_dev_internal > > > > > > The root cause of UAF bugs is that find_device() failed in > > > dm_get_device() and will create dd and refcount set 1, kfree() > > > in dm_put_table() is not protected. When td, which there are > > > still pointers point to, is released, the concurrency UAF bug > > > will happen. > > > > > > This patch add a flag to determine whether to create a new dd. > > > > > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) > > > Signed-off-by: Luo Meng <luomeng12@huawei.com> > > > --- > > > drivers/md/dm-mpath.c | 2 +- > > > drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- > > > include/linux/device-mapper.h | 2 ++ > > > 3 files changed, 29 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > > > index 0e325469a252..1f51059520ac 100644 > > > --- a/drivers/md/dm-mpath.c > > > +++ b/drivers/md/dm-mpath.c > > > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, > > > goto out; > > > } > > > - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); > > > + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); > > > if (r) { > > > DMWARN("message: error getting device %s", > > > argv[1]); > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > > > index d8034ff0cb24..ad18a41f1608 100644 > > > --- a/drivers/md/dm-table.c > > > +++ b/drivers/md/dm-table.c > > > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) > > > } > > > EXPORT_SYMBOL_GPL(dm_get_dev_t); > > > -/* > > > - * Add a device to the list, or just increment the usage count if > > > - * it's already present. > > > - */ > > > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > - struct dm_dev **result) > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > + struct dm_dev **result, bool create_dd) > > > { > > > int r; > > > dev_t dev; > > > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > dd = find_device(&t->devices, dev); > > > if (!dd) { > > > - dd = kmalloc(sizeof(*dd), GFP_KERNEL); > > > - if (!dd) > > > - return -ENOMEM; > > > - > > > - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > > > - kfree(dd); > > > - return r; > > > - } > > > + if (create_dd) { > > > + dd = kmalloc(sizeof(*dd), GFP_KERNEL); > > > + if (!dd) > > > + return -ENOMEM; > > > - refcount_set(&dd->count, 1); > > > - list_add(&dd->list, &t->devices); > > > - goto out; > > > + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > > > + kfree(dd); > > > + return r; > > > + } > > > + refcount_set(&dd->count, 1); > > > + list_add(&dd->list, &t->devices); > > > + goto out; > > > + } else > > > + return -ENODEV; > > > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > > > r = upgrade_mode(dd, mode, t->md); > > > if (r) > > > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > *result = dd->dm_dev; > > > return 0; > > > } > > > +EXPORT_SYMBOL(__dm_get_device); > > > + > > > +/* > > > + * Add a device to the list, or just increment the usage count if > > > + * it's already present. > > > + */ > > > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > + struct dm_dev **result) > > > +{ > > > + return __dm_get_device(ti, path, mode, result, true); > > > +} > > > EXPORT_SYMBOL(dm_get_device); > > > static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, > > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > > > index 04c6acf7faaa..ef4031a844b6 100644 > > > --- a/include/linux/device-mapper.h > > > +++ b/include/linux/device-mapper.h > > > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); > > > int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > struct dm_dev **result); > > > void dm_put_device(struct dm_target *ti, struct dm_dev *d); > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > + struct dm_dev **result, bool create_dd); > > > /* > > > * Information about a target type > > > -- > > > 2.31.1 > > > > > This patch is treating the one multipath case like it is only consumer > > of dm_get_device() that might have this issue. It feels too focused on > > an instance where dm_get_device()'s side-effect of creating the device > > is unwelcome. Feels like you're treating the symptom rather than the > > problem (e.g. that the "kfree() in dm_put_table() is not protected"?) > > > > Mike > > In other cases, kfree() in dm_put_table() is protected by srcu. > For example: > USE FREE > table_deps dev_remove > dm_get_live_or_inactive_table dm_sync_table > // lock > srcu_read_lock(&md->io_barrier) > // wait for unlock > synchronize_srcu(&md->io_barrier) > retrieve_deps > dm_put_live_table > // unlock > srcu_read_unlock(&md->io_barrier...) > dm_table_destroy > linear_dtr > dm_put_device > dm_put_table_device > > However, in multipath_message(), the dm_dev is created and destroyed > under srcu_read_lock, which means destroying dm_dev in this process > and using dm_dev in other process will happen at same time since both > of them hold srcu_read_lock. > > target_message > dm_get_live_table // lock > multipath_message > dm_get_device // created > // may be got by other processes > dm_put_device // destroyed > // may be used by other processes > dm_put_live_table // unlock > > We figured out some other solutions: > 1) Judge refcount of dd under some lock before using dm_dev; > 2) Get dd from list and use dm_dev under rcu; > 3) Implement dm_get_device_xxx() with reference to dm_get_device() > for dm-mpath to avoid creating dd when find_device() failed. > > Compared to solutions above, Luo Meng's patch may be more appropriate. > Any suggestions will be appreciated. > > Li [Cc'ing Mikulas so he can take a look at this too.] The proposed patch papers over the issue, leaving a landmine for some future DM target. In addition, the patch header is very muddled about relation between table_device and dm_dev_internal. And also needs clarification like: "kfree(td) in dm_put_table_device() is not interlocked with dm_dev_internal list (table->devices) management" Also, the commit referenced with the "Fixes:" is bogus. Wouldn't this change address the UAF (albeit, list_for_each_entry_safe likely needed in methods like retrieve_deps())? diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 7d208b2b1a19..39e4c9ee8f16 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -434,9 +434,9 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d) return; } if (refcount_dec_and_test(&dd->count)) { - dm_put_table_device(ti->table->md, d); list_del(&dd->list); kfree(dd); + dm_put_table_device(ti->table->md, d); } } EXPORT_SYMBOL(dm_put_device); -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
在 2023/8/3 0:33, Mike Snitzer 写道: > On Thu, May 18 2023 at 8:11P -0400, > lilingfeng (A) <lilingfeng3@huawei.com> wrote: > >> 在 2022/10/18 3:56, Mike Snitzer 写道: >>> On Mon, Oct 10 2022 at 10:41P -0400, >>> Luo Meng <luomeng@huaweicloud.com> wrote: >>> >>>> From: Luo Meng <luomeng12@huawei.com> >>>> >>>> If dm_get_device() create dd in multipath_message(), >>>> and then call table_deps() after dm_put_table_device(), >>>> it will lead to concurrency UAF bugs. >>>> >>>> One of the concurrency UAF can be shown as below: >>>> >>>> (USE) | (FREE) >>>> | target_message >>>> | multipath_message >>>> | dm_put_device >>>> | dm_put_table_device # >>>> | kfree(td) # table_device *td >>>> ioctl # DM_TABLE_DEPS_CMD | ... >>>> table_deps | ... >>>> dm_get_live_or_inactive_table | ... >>>> retrieve_dep | ... >>>> list_for_each_entry | ... >>>> deps->dev[count++] = | ... >>>> huge_encode_dev | ... >>>> (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) >>>> | kfree(dd) # dm_dev_internal >>>> >>>> The root cause of UAF bugs is that find_device() failed in >>>> dm_get_device() and will create dd and refcount set 1, kfree() >>>> in dm_put_table() is not protected. When td, which there are >>>> still pointers point to, is released, the concurrency UAF bug >>>> will happen. >>>> >>>> This patch add a flag to determine whether to create a new dd. >>>> >>>> Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) >>>> Signed-off-by: Luo Meng <luomeng12@huawei.com> >>>> --- >>>> drivers/md/dm-mpath.c | 2 +- >>>> drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- >>>> include/linux/device-mapper.h | 2 ++ >>>> 3 files changed, 29 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c >>>> index 0e325469a252..1f51059520ac 100644 >>>> --- a/drivers/md/dm-mpath.c >>>> +++ b/drivers/md/dm-mpath.c >>>> @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, >>>> goto out; >>>> } >>>> - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); >>>> + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); >>>> if (r) { >>>> DMWARN("message: error getting device %s", >>>> argv[1]); >>>> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >>>> index d8034ff0cb24..ad18a41f1608 100644 >>>> --- a/drivers/md/dm-table.c >>>> +++ b/drivers/md/dm-table.c >>>> @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) >>>> } >>>> EXPORT_SYMBOL_GPL(dm_get_dev_t); >>>> -/* >>>> - * Add a device to the list, or just increment the usage count if >>>> - * it's already present. >>>> - */ >>>> -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> - struct dm_dev **result) >>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> + struct dm_dev **result, bool create_dd) >>>> { >>>> int r; >>>> dev_t dev; >>>> @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> dd = find_device(&t->devices, dev); >>>> if (!dd) { >>>> - dd = kmalloc(sizeof(*dd), GFP_KERNEL); >>>> - if (!dd) >>>> - return -ENOMEM; >>>> - >>>> - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >>>> - kfree(dd); >>>> - return r; >>>> - } >>>> + if (create_dd) { >>>> + dd = kmalloc(sizeof(*dd), GFP_KERNEL); >>>> + if (!dd) >>>> + return -ENOMEM; >>>> - refcount_set(&dd->count, 1); >>>> - list_add(&dd->list, &t->devices); >>>> - goto out; >>>> + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { >>>> + kfree(dd); >>>> + return r; >>>> + } >>>> + refcount_set(&dd->count, 1); >>>> + list_add(&dd->list, &t->devices); >>>> + goto out; >>>> + } else >>>> + return -ENODEV; >>>> } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { >>>> r = upgrade_mode(dd, mode, t->md); >>>> if (r) >>>> @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> *result = dd->dm_dev; >>>> return 0; >>>> } >>>> +EXPORT_SYMBOL(__dm_get_device); >>>> + >>>> +/* >>>> + * Add a device to the list, or just increment the usage count if >>>> + * it's already present. >>>> + */ >>>> +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> + struct dm_dev **result) >>>> +{ >>>> + return __dm_get_device(ti, path, mode, result, true); >>>> +} >>>> EXPORT_SYMBOL(dm_get_device); >>>> static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, >>>> diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h >>>> index 04c6acf7faaa..ef4031a844b6 100644 >>>> --- a/include/linux/device-mapper.h >>>> +++ b/include/linux/device-mapper.h >>>> @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); >>>> int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> struct dm_dev **result); >>>> void dm_put_device(struct dm_target *ti, struct dm_dev *d); >>>> +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, >>>> + struct dm_dev **result, bool create_dd); >>>> /* >>>> * Information about a target type >>>> -- >>>> 2.31.1 >>>> >>> This patch is treating the one multipath case like it is only consumer >>> of dm_get_device() that might have this issue. It feels too focused on >>> an instance where dm_get_device()'s side-effect of creating the device >>> is unwelcome. Feels like you're treating the symptom rather than the >>> problem (e.g. that the "kfree() in dm_put_table() is not protected"?) >>> >>> Mike >> In other cases, kfree() in dm_put_table() is protected by srcu. >> For example: >> USE FREE >> table_deps dev_remove >> dm_get_live_or_inactive_table dm_sync_table >> // lock >> srcu_read_lock(&md->io_barrier) >> // wait for unlock >> synchronize_srcu(&md->io_barrier) >> retrieve_deps >> dm_put_live_table >> // unlock >> srcu_read_unlock(&md->io_barrier...) >> dm_table_destroy >> linear_dtr >> dm_put_device >> dm_put_table_device >> >> However, in multipath_message(), the dm_dev is created and destroyed >> under srcu_read_lock, which means destroying dm_dev in this process >> and using dm_dev in other process will happen at same time since both >> of them hold srcu_read_lock. >> >> target_message >> dm_get_live_table // lock >> multipath_message >> dm_get_device // created >> // may be got by other processes >> dm_put_device // destroyed >> // may be used by other processes >> dm_put_live_table // unlock >> >> We figured out some other solutions: >> 1) Judge refcount of dd under some lock before using dm_dev; >> 2) Get dd from list and use dm_dev under rcu; >> 3) Implement dm_get_device_xxx() with reference to dm_get_device() >> for dm-mpath to avoid creating dd when find_device() failed. >> >> Compared to solutions above, Luo Meng's patch may be more appropriate. >> Any suggestions will be appreciated. >> >> Li > [Cc'ing Mikulas so he can take a look at this too.] > > The proposed patch papers over the issue, leaving a landmine for some > future DM target. > > In addition, the patch header is very muddled about relation between > table_device and dm_dev_internal. And also needs clarification like: > "kfree(td) in dm_put_table_device() is not interlocked with > dm_dev_internal list (table->devices) management" > > Also, the commit referenced with the "Fixes:" is bogus. > > Wouldn't this change address the UAF (albeit, list_for_each_entry_safe > likely needed in methods like retrieve_deps())? > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 7d208b2b1a19..39e4c9ee8f16 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -434,9 +434,9 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d) > return; > } > if (refcount_dec_and_test(&dd->count)) { > - dm_put_table_device(ti->table->md, d); > list_del(&dd->list); > kfree(dd); > + dm_put_table_device(ti->table->md, d); > } > } > EXPORT_SYMBOL(dm_put_device); Thanks for your reply. Deleting dm_dev_internal from the list before freeing table_device may not fix the problem since the process of "USE" may have got dm_dev_internal before deleting and try to use it after freeing. (USE) | (FREE) | target_message | multipath_message | dm_put_device ioctl # DM_TABLE_DEPS_CMD | ... table_deps | ... dm_get_live_or_inactive_table | ... retrieve_dep | ... list_for_each_entry ---- got dd | ... deps->dev[count++] = | ... | list_del(&dd->list) ---- delete | kfree(dd) # dm_dev_internal | dm_put_table_device # | kfree(td) ---- free huge_encode_dev | ... (dd->dm_dev->bdev->bd_dev) | ----> UAF Li -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2 Aug 2023, Mike Snitzer wrote: > On Thu, May 18 2023 at 8:11P -0400, > lilingfeng (A) <lilingfeng3@huawei.com> wrote: > > > > > 在 2022/10/18 3:56, Mike Snitzer 写道: > > > On Mon, Oct 10 2022 at 10:41P -0400, > > > Luo Meng <luomeng@huaweicloud.com> wrote: > > > > > > > From: Luo Meng <luomeng12@huawei.com> > > > > > > > > If dm_get_device() create dd in multipath_message(), > > > > and then call table_deps() after dm_put_table_device(), > > > > it will lead to concurrency UAF bugs. > > > > > > > > One of the concurrency UAF can be shown as below: > > > > > > > > (USE) | (FREE) > > > > | target_message > > > > | multipath_message > > > > | dm_put_device > > > > | dm_put_table_device # > > > > | kfree(td) # table_device *td > > > > ioctl # DM_TABLE_DEPS_CMD | ... > > > > table_deps | ... > > > > dm_get_live_or_inactive_table | ... > > > > retrieve_dep | ... > > > > list_for_each_entry | ... > > > > deps->dev[count++] = | ... > > > > huge_encode_dev | ... > > > > (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list) > > > > | kfree(dd) # dm_dev_internal > > > > > > > > The root cause of UAF bugs is that find_device() failed in > > > > dm_get_device() and will create dd and refcount set 1, kfree() > > > > in dm_put_table() is not protected. When td, which there are > > > > still pointers point to, is released, the concurrency UAF bug > > > > will happen. > > > > > > > > This patch add a flag to determine whether to create a new dd. > > > > > > > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters) > > > > Signed-off-by: Luo Meng <luomeng12@huawei.com> > > > > --- > > > > drivers/md/dm-mpath.c | 2 +- > > > > drivers/md/dm-table.c | 43 +++++++++++++++++++++-------------- > > > > include/linux/device-mapper.h | 2 ++ > > > > 3 files changed, 29 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > > > > index 0e325469a252..1f51059520ac 100644 > > > > --- a/drivers/md/dm-mpath.c > > > > +++ b/drivers/md/dm-mpath.c > > > > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, > > > > goto out; > > > > } > > > > - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); > > > > + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); > > > > if (r) { > > > > DMWARN("message: error getting device %s", > > > > argv[1]); > > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > > > > index d8034ff0cb24..ad18a41f1608 100644 > > > > --- a/drivers/md/dm-table.c > > > > +++ b/drivers/md/dm-table.c > > > > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) > > > > } > > > > EXPORT_SYMBOL_GPL(dm_get_dev_t); > > > > -/* > > > > - * Add a device to the list, or just increment the usage count if > > > > - * it's already present. > > > > - */ > > > > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > - struct dm_dev **result) > > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > + struct dm_dev **result, bool create_dd) > > > > { > > > > int r; > > > > dev_t dev; > > > > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > dd = find_device(&t->devices, dev); > > > > if (!dd) { > > > > - dd = kmalloc(sizeof(*dd), GFP_KERNEL); > > > > - if (!dd) > > > > - return -ENOMEM; > > > > - > > > > - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > > > > - kfree(dd); > > > > - return r; > > > > - } > > > > + if (create_dd) { > > > > + dd = kmalloc(sizeof(*dd), GFP_KERNEL); > > > > + if (!dd) > > > > + return -ENOMEM; > > > > - refcount_set(&dd->count, 1); > > > > - list_add(&dd->list, &t->devices); > > > > - goto out; > > > > + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { > > > > + kfree(dd); > > > > + return r; > > > > + } > > > > + refcount_set(&dd->count, 1); > > > > + list_add(&dd->list, &t->devices); > > > > + goto out; > > > > + } else > > > > + return -ENODEV; > > > > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > > > > r = upgrade_mode(dd, mode, t->md); > > > > if (r) > > > > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > *result = dd->dm_dev; > > > > return 0; > > > > } > > > > +EXPORT_SYMBOL(__dm_get_device); > > > > + > > > > +/* > > > > + * Add a device to the list, or just increment the usage count if > > > > + * it's already present. > > > > + */ > > > > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > + struct dm_dev **result) > > > > +{ > > > > + return __dm_get_device(ti, path, mode, result, true); > > > > +} > > > > EXPORT_SYMBOL(dm_get_device); > > > > static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, > > > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > > > > index 04c6acf7faaa..ef4031a844b6 100644 > > > > --- a/include/linux/device-mapper.h > > > > +++ b/include/linux/device-mapper.h > > > > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); > > > > int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > struct dm_dev **result); > > > > void dm_put_device(struct dm_target *ti, struct dm_dev *d); > > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, > > > > + struct dm_dev **result, bool create_dd); > > > > /* > > > > * Information about a target type > > > > -- > > > > 2.31.1 > > > > > > > This patch is treating the one multipath case like it is only consumer > > > of dm_get_device() that might have this issue. It feels too focused on > > > an instance where dm_get_device()'s side-effect of creating the device > > > is unwelcome. Feels like you're treating the symptom rather than the > > > problem (e.g. that the "kfree() in dm_put_table() is not protected"?) > > > > > > Mike > > > > In other cases, kfree() in dm_put_table() is protected by srcu. > > For example: > > USE FREE > > table_deps dev_remove > > dm_get_live_or_inactive_table dm_sync_table > > // lock > > srcu_read_lock(&md->io_barrier) > > // wait for unlock > > synchronize_srcu(&md->io_barrier) > > retrieve_deps > > dm_put_live_table > > // unlock > > srcu_read_unlock(&md->io_barrier...) > > dm_table_destroy > > linear_dtr > > dm_put_device > > dm_put_table_device > > > > However, in multipath_message(), the dm_dev is created and destroyed > > under srcu_read_lock, which means destroying dm_dev in this process > > and using dm_dev in other process will happen at same time since both > > of them hold srcu_read_lock. > > > > target_message > > dm_get_live_table // lock > > multipath_message > > dm_get_device // created > > // may be got by other processes > > dm_put_device // destroyed > > // may be used by other processes > > dm_put_live_table // unlock > > > > We figured out some other solutions: > > 1) Judge refcount of dd under some lock before using dm_dev; > > 2) Get dd from list and use dm_dev under rcu; > > 3) Implement dm_get_device_xxx() with reference to dm_get_device() > > for dm-mpath to avoid creating dd when find_device() failed. > > > > Compared to solutions above, Luo Meng's patch may be more appropriate. > > Any suggestions will be appreciated. > > > > Li > > [Cc'ing Mikulas so he can take a look at this too.] Hi I suggest this patch (but it's only compile-tested, so please run some testsuite on it). Mikulas From: Mikulas Patocka <mpatocka@redhat.com> Subject: [PATCH] dm: fix a race condition in retrieve_deps There's a race condition in the multipath target when retrieve_deps races with multipath_message. multipath_message opens and closes a device using dm_get_device and dm_put_device; retrieve_deps walks the list of open devices without holding any lock. The end result may be memory corruption or use-after-free memory access. multipath_message already holds a mutex that prevents two multipath_messages from running concurrently - in order to fix this race condition, we modify retrieve_deps, so that it grabs and frees this mutex too. We add an entry "devices_mutex" to "struct dm_table" and we introduce a function "dm_table_set_devices_mutex" that sets it. The mutex is set in the multipath target contructor. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/md/dm-core.h | 5 +++++ drivers/md/dm-ioctl.c | 9 ++++++++- drivers/md/dm-mpath.c | 2 ++ drivers/md/dm-table.c | 6 ++++++ include/linux/device-mapper.h | 5 +++++ 5 files changed, 26 insertions(+), 1 deletion(-) Index: linux-2.6/drivers/md/dm-core.h =================================================================== --- linux-2.6.orig/drivers/md/dm-core.h +++ linux-2.6/drivers/md/dm-core.h @@ -214,6 +214,11 @@ struct dm_table { /* a list of devices used by this table */ struct list_head devices; + /* + * The mutex should be set if the target modifies the "devices" list + * outside of the constructor and destructor. + */ + struct mutex *devices_mutex; /* events get handed up using this callback */ void (*event_fn)(void *data); Index: linux-2.6/drivers/md/dm-table.c =================================================================== --- linux-2.6.orig/drivers/md/dm-table.c +++ linux-2.6/drivers/md/dm-table.c @@ -2034,6 +2034,12 @@ struct list_head *dm_table_get_devices(s return &t->devices; } +void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m) +{ + t->devices_mutex = m; +} +EXPORT_SYMBOL(dm_table_set_devices_mutex); + blk_mode_t dm_table_get_mode(struct dm_table *t) { return t->mode; Index: linux-2.6/drivers/md/dm-ioctl.c =================================================================== --- linux-2.6.orig/drivers/md/dm-ioctl.c +++ linux-2.6/drivers/md/dm-ioctl.c @@ -1630,6 +1630,9 @@ static void retrieve_deps(struct dm_tabl struct dm_dev_internal *dd; struct dm_target_deps *deps; + if (table->devices_mutex) + mutex_lock(table->devices_mutex); + deps = get_result_buffer(param, param_size, &len); /* @@ -1644,7 +1647,7 @@ static void retrieve_deps(struct dm_tabl needed = struct_size(deps, dev, count); if (len < needed) { param->flags |= DM_BUFFER_FULL_FLAG; - return; + goto ret; } /* @@ -1656,6 +1659,10 @@ static void retrieve_deps(struct dm_tabl deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev); param->data_size = param->data_start + needed; + +ret: + if (table->devices_mutex) + mutex_unlock(table->devices_mutex); } static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size) Index: linux-2.6/drivers/md/dm-mpath.c =================================================================== --- linux-2.6.orig/drivers/md/dm-mpath.c +++ linux-2.6/drivers/md/dm-mpath.c @@ -1268,6 +1268,8 @@ static int multipath_ctr(struct dm_targe else ti->per_io_data_size = sizeof(struct dm_mpath_io); + dm_table_set_devices_mutex(ti->table, &m->work_mutex); + return 0; bad: Index: linux-2.6/include/linux/device-mapper.h =================================================================== --- linux-2.6.orig/include/linux/device-mapper.h +++ linux-2.6/include/linux/device-mapper.h @@ -177,6 +177,11 @@ struct dm_dev { int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode, struct dm_dev **result); void dm_put_device(struct dm_target *ti, struct dm_dev *d); +/* + * The mutex must be set if we use dm_get_device or dm_put_device outside + * of the constructor and destructor. + */ +void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m); /* * Information about a target type -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 8 Aug 2023, Mikulas Patocka wrote: > On Wed, 2 Aug 2023, Mike Snitzer wrote: > > > [Cc'ing Mikulas so he can take a look at this too.] > > Hi > > I suggest this patch (but it's only compile-tested, so please run some > testsuite on it). > > Mikulas I self-nack that patch - it doesn't work if there are multiple targets calling dm_table_set_devices_mutex in the same table. This is not an issue for multipath, but it will be a problem if other targets use this functionality. Here I'm sending a better patch that doesn't need any modification to the targets at all. Mikulas From: Mikulas Patocka <mpatocka at redhat.com> Subject: [PATCH] dm: fix a race condition in retrieve_deps There's a race condition in the multipath target when retrieve_deps races with multipath_message calling dm_get_device and dm_put_device. retrieve_deps walks the list of open devices without holding any lock but multipath may add or remove devices to the list while it is running. The end result may be memory corruption or use-after-free memory access. Fix this bug by introducing a new rw semaphore "devices_lock". We grab devices_lock for read in retrieve_deps and we grab it for write in dm_get_device and dm_put_device. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/md/dm-core.h | 1 + drivers/md/dm-ioctl.c | 7 ++++++- drivers/md/dm-table.c | 32 ++++++++++++++++++++++++-------- 3 files changed, 31 insertions(+), 9 deletions(-) Index: linux-2.6/drivers/md/dm-core.h =================================================================== --- linux-2.6.orig/drivers/md/dm-core.h +++ linux-2.6/drivers/md/dm-core.h @@ -214,6 +214,7 @@ struct dm_table { /* a list of devices used by this table */ struct list_head devices; + struct rw_semaphore devices_lock; /* events get handed up using this callback */ void (*event_fn)(void *data); Index: linux-2.6/drivers/md/dm-ioctl.c =================================================================== --- linux-2.6.orig/drivers/md/dm-ioctl.c +++ linux-2.6/drivers/md/dm-ioctl.c @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl struct dm_dev_internal *dd; struct dm_target_deps *deps; + down_read(&table->devices_lock); + deps = get_result_buffer(param, param_size, &len); /* @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl needed = struct_size(deps, dev, count); if (len < needed) { param->flags |= DM_BUFFER_FULL_FLAG; - return; + goto out; } /* @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev); param->data_size = param->data_start + needed; + +out: + up_read(&table->devices_lock); } static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size) Index: linux-2.6/drivers/md/dm-table.c =================================================================== --- linux-2.6.orig/drivers/md/dm-table.c +++ linux-2.6/drivers/md/dm-table.c @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re return -ENOMEM; INIT_LIST_HEAD(&t->devices); + init_rwsem(&t->devices_lock); if (!num_targets) num_targets = KEYS_PER_NODE; @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target if (dev == disk_devt(t->md->disk)) return -EINVAL; + down_write(&t->devices_lock); + dd = find_device(&t->devices, dev); if (!dd) { dd = kmalloc(sizeof(*dd), GFP_KERNEL); - if (!dd) - return -ENOMEM; + if (!dd) { + r = -ENOMEM; + goto unlock_ret_r; + } r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev); if (r) { kfree(dd); - return r; + goto unlock_ret_r; } refcount_set(&dd->count, 1); @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { r = upgrade_mode(dd, mode, t->md); if (r) - return r; + goto unlock_ret_r; } refcount_inc(&dd->count); out: + up_write(&t->devices_lock); *result = dd->dm_dev; return 0; + +unlock_ret_r: + up_write(&t->devices_lock); + return r; } EXPORT_SYMBOL(dm_get_device); @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d void dm_put_device(struct dm_target *ti, struct dm_dev *d) { int found = 0; - struct list_head *devices = &ti->table->devices; + struct dm_table *t = ti->table; + struct list_head *devices = &t->devices; struct dm_dev_internal *dd; + down_write(&t->devices_lock); + list_for_each_entry(dd, devices, list) { if (dd->dm_dev == d) { found = 1; @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti, } if (!found) { DMERR("%s: device %s not in table devices list", - dm_device_name(ti->table->md), d->name); - return; + dm_device_name(t->md), d->name); + goto unlock_ret; } if (refcount_dec_and_test(&dd->count)) { - dm_put_table_device(ti->table->md, d); + dm_put_table_device(t->md, d); list_del(&dd->list); kfree(dd); } + +unlock_ret: + up_write(&t->devices_lock); } EXPORT_SYMBOL(dm_put_device); -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Thanks to Mikulas for the patch. I have test the patch and it can fix the problem. Can this patch be applied to mainline? Thanks. 在 2023/8/9 18:44, Mikulas Patocka 写道: > > On Tue, 8 Aug 2023, Mikulas Patocka wrote: > >> On Wed, 2 Aug 2023, Mike Snitzer wrote: >> >>> [Cc'ing Mikulas so he can take a look at this too.] >> Hi >> >> I suggest this patch (but it's only compile-tested, so please run some >> testsuite on it). >> >> Mikulas > I self-nack that patch - it doesn't work if there are multiple targets > calling dm_table_set_devices_mutex in the same table. This is not an issue > for multipath, but it will be a problem if other targets use this > functionality. > > Here I'm sending a better patch that doesn't need any modification to the > targets at all. > > Mikulas > > > > From: Mikulas Patocka <mpatocka at redhat.com> > Subject: [PATCH] dm: fix a race condition in retrieve_deps > > There's a race condition in the multipath target when retrieve_deps races > with multipath_message calling dm_get_device and dm_put_device. > retrieve_deps walks the list of open devices without holding any lock but > multipath may add or remove devices to the list while it is running. The > end result may be memory corruption or use-after-free memory access. > > Fix this bug by introducing a new rw semaphore "devices_lock". We grab > devices_lock for read in retrieve_deps and we grab it for write in > dm_get_device and dm_put_device. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > --- > drivers/md/dm-core.h | 1 + > drivers/md/dm-ioctl.c | 7 ++++++- > drivers/md/dm-table.c | 32 ++++++++++++++++++++++++-------- > 3 files changed, 31 insertions(+), 9 deletions(-) > > Index: linux-2.6/drivers/md/dm-core.h > =================================================================== > --- linux-2.6.orig/drivers/md/dm-core.h > +++ linux-2.6/drivers/md/dm-core.h > @@ -214,6 +214,7 @@ struct dm_table { > > /* a list of devices used by this table */ > struct list_head devices; > + struct rw_semaphore devices_lock; > > /* events get handed up using this callback */ > void (*event_fn)(void *data); > Index: linux-2.6/drivers/md/dm-ioctl.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-ioctl.c > +++ linux-2.6/drivers/md/dm-ioctl.c > @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl > struct dm_dev_internal *dd; > struct dm_target_deps *deps; > > + down_read(&table->devices_lock); > + > deps = get_result_buffer(param, param_size, &len); > > /* > @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl > needed = struct_size(deps, dev, count); > if (len < needed) { > param->flags |= DM_BUFFER_FULL_FLAG; > - return; > + goto out; > } > > /* > @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl > deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev); > > param->data_size = param->data_start + needed; > + > +out: > + up_read(&table->devices_lock); > } > > static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size) > Index: linux-2.6/drivers/md/dm-table.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-table.c > +++ linux-2.6/drivers/md/dm-table.c > @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re > return -ENOMEM; > > INIT_LIST_HEAD(&t->devices); > + init_rwsem(&t->devices_lock); > > if (!num_targets) > num_targets = KEYS_PER_NODE; > @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target > if (dev == disk_devt(t->md->disk)) > return -EINVAL; > > + down_write(&t->devices_lock); > + > dd = find_device(&t->devices, dev); > if (!dd) { > dd = kmalloc(sizeof(*dd), GFP_KERNEL); > - if (!dd) > - return -ENOMEM; > + if (!dd) { > + r = -ENOMEM; > + goto unlock_ret_r; > + } > > r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev); > if (r) { > kfree(dd); > - return r; > + goto unlock_ret_r; > } > > refcount_set(&dd->count, 1); > @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > r = upgrade_mode(dd, mode, t->md); > if (r) > - return r; > + goto unlock_ret_r; > } > refcount_inc(&dd->count); > out: > + up_write(&t->devices_lock); > *result = dd->dm_dev; > return 0; > + > +unlock_ret_r: > + up_write(&t->devices_lock); > + return r; > } > EXPORT_SYMBOL(dm_get_device); > > @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d > void dm_put_device(struct dm_target *ti, struct dm_dev *d) > { > int found = 0; > - struct list_head *devices = &ti->table->devices; > + struct dm_table *t = ti->table; > + struct list_head *devices = &t->devices; > struct dm_dev_internal *dd; > > + down_write(&t->devices_lock); > + > list_for_each_entry(dd, devices, list) { > if (dd->dm_dev == d) { > found = 1; > @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti, > } > if (!found) { > DMERR("%s: device %s not in table devices list", > - dm_device_name(ti->table->md), d->name); > - return; > + dm_device_name(t->md), d->name); > + goto unlock_ret; > } > if (refcount_dec_and_test(&dd->count)) { > - dm_put_table_device(ti->table->md, d); > + dm_put_table_device(t->md, d); > list_del(&dd->list); > kfree(dd); > } > + > +unlock_ret: > + up_write(&t->devices_lock); > } > EXPORT_SYMBOL(dm_put_device); > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Please reply to Mikulas's updated patch with your Reviewed-by: and possibly Tested-by: Thanks, Mike On Tue, Sep 5, 2023, 10:16 PM Li Lingfeng <lilingfeng3@huawei.com> wrote: > Hi > > Thanks to Mikulas for the patch. I have test the patch and it can fix > the problem. > Can this patch be applied to mainline? > > Thanks. > > 在 2023/8/9 18:44, Mikulas Patocka 写道: > > > > On Tue, 8 Aug 2023, Mikulas Patocka wrote: > > > >> On Wed, 2 Aug 2023, Mike Snitzer wrote: > >> > >>> [Cc'ing Mikulas so he can take a look at this too.] > >> Hi > >> > >> I suggest this patch (but it's only compile-tested, so please run some > >> testsuite on it). > >> > >> Mikulas > > I self-nack that patch - it doesn't work if there are multiple targets > > calling dm_table_set_devices_mutex in the same table. This is not an > issue > > for multipath, but it will be a problem if other targets use this > > functionality. > > > > Here I'm sending a better patch that doesn't need any modification to the > > targets at all. > > > > Mikulas > > > > > > > > From: Mikulas Patocka <mpatocka at redhat.com> > > Subject: [PATCH] dm: fix a race condition in retrieve_deps > > > > There's a race condition in the multipath target when retrieve_deps races > > with multipath_message calling dm_get_device and dm_put_device. > > retrieve_deps walks the list of open devices without holding any lock but > > multipath may add or remove devices to the list while it is running. The > > end result may be memory corruption or use-after-free memory access. > > > > Fix this bug by introducing a new rw semaphore "devices_lock". We grab > > devices_lock for read in retrieve_deps and we grab it for write in > > dm_get_device and dm_put_device. > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/md/dm-core.h | 1 + > > drivers/md/dm-ioctl.c | 7 ++++++- > > drivers/md/dm-table.c | 32 ++++++++++++++++++++++++-------- > > 3 files changed, 31 insertions(+), 9 deletions(-) > > > > Index: linux-2.6/drivers/md/dm-core.h > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-core.h > > +++ linux-2.6/drivers/md/dm-core.h > > @@ -214,6 +214,7 @@ struct dm_table { > > > > /* a list of devices used by this table */ > > struct list_head devices; > > + struct rw_semaphore devices_lock; > > > > /* events get handed up using this callback */ > > void (*event_fn)(void *data); > > Index: linux-2.6/drivers/md/dm-ioctl.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-ioctl.c > > +++ linux-2.6/drivers/md/dm-ioctl.c > > @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl > > struct dm_dev_internal *dd; > > struct dm_target_deps *deps; > > > > + down_read(&table->devices_lock); > > + > > deps = get_result_buffer(param, param_size, &len); > > > > /* > > @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl > > needed = struct_size(deps, dev, count); > > if (len < needed) { > > param->flags |= DM_BUFFER_FULL_FLAG; > > - return; > > + goto out; > > } > > > > /* > > @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl > > deps->dev[count++] = > huge_encode_dev(dd->dm_dev->bdev->bd_dev); > > > > param->data_size = param->data_start + needed; > > + > > +out: > > + up_read(&table->devices_lock); > > } > > > > static int table_deps(struct file *filp, struct dm_ioctl *param, > size_t param_size) > > Index: linux-2.6/drivers/md/dm-table.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-table.c > > +++ linux-2.6/drivers/md/dm-table.c > > @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re > > return -ENOMEM; > > > > INIT_LIST_HEAD(&t->devices); > > + init_rwsem(&t->devices_lock); > > > > if (!num_targets) > > num_targets = KEYS_PER_NODE; > > @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target > > if (dev == disk_devt(t->md->disk)) > > return -EINVAL; > > > > + down_write(&t->devices_lock); > > + > > dd = find_device(&t->devices, dev); > > if (!dd) { > > dd = kmalloc(sizeof(*dd), GFP_KERNEL); > > - if (!dd) > > - return -ENOMEM; > > + if (!dd) { > > + r = -ENOMEM; > > + goto unlock_ret_r; > > + } > > > > r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev); > > if (r) { > > kfree(dd); > > - return r; > > + goto unlock_ret_r; > > } > > > > refcount_set(&dd->count, 1); > > @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target > > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > > r = upgrade_mode(dd, mode, t->md); > > if (r) > > - return r; > > + goto unlock_ret_r; > > } > > refcount_inc(&dd->count); > > out: > > + up_write(&t->devices_lock); > > *result = dd->dm_dev; > > return 0; > > + > > +unlock_ret_r: > > + up_write(&t->devices_lock); > > + return r; > > } > > EXPORT_SYMBOL(dm_get_device); > > > > @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d > > void dm_put_device(struct dm_target *ti, struct dm_dev *d) > > { > > int found = 0; > > - struct list_head *devices = &ti->table->devices; > > + struct dm_table *t = ti->table; > > + struct list_head *devices = &t->devices; > > struct dm_dev_internal *dd; > > > > + down_write(&t->devices_lock); > > + > > list_for_each_entry(dd, devices, list) { > > if (dd->dm_dev == d) { > > found = 1; > > @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti, > > } > > if (!found) { > > DMERR("%s: device %s not in table devices list", > > - dm_device_name(ti->table->md), d->name); > > - return; > > + dm_device_name(t->md), d->name); > > + goto unlock_ret; > > } > > if (refcount_dec_and_test(&dd->count)) { > > - dm_put_table_device(ti->table->md, d); > > + dm_put_table_device(t->md, d); > > list_del(&dd->list); > > kfree(dd); > > } > > + > > +unlock_ret: > > + up_write(&t->devices_lock); > > } > > EXPORT_SYMBOL(dm_put_device); > > > > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://listman.redhat.com/mailman/listinfo/dm-devel > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
在 2023/8/9 18:44, Mikulas Patocka 写道: > > On Tue, 8 Aug 2023, Mikulas Patocka wrote: > >> On Wed, 2 Aug 2023, Mike Snitzer wrote: >> >>> [Cc'ing Mikulas so he can take a look at this too.] >> Hi >> >> I suggest this patch (but it's only compile-tested, so please run some >> testsuite on it). >> >> Mikulas > I self-nack that patch - it doesn't work if there are multiple targets > calling dm_table_set_devices_mutex in the same table. This is not an issue > for multipath, but it will be a problem if other targets use this > functionality. > > Here I'm sending a better patch that doesn't need any modification to the > targets at all. > > Mikulas > > > > From: Mikulas Patocka <mpatocka at redhat.com> > Subject: [PATCH] dm: fix a race condition in retrieve_deps > > There's a race condition in the multipath target when retrieve_deps races > with multipath_message calling dm_get_device and dm_put_device. > retrieve_deps walks the list of open devices without holding any lock but > multipath may add or remove devices to the list while it is running. The > end result may be memory corruption or use-after-free memory access. > > Fix this bug by introducing a new rw semaphore "devices_lock". We grab > devices_lock for read in retrieve_deps and we grab it for write in > dm_get_device and dm_put_device. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > --- > drivers/md/dm-core.h | 1 + > drivers/md/dm-ioctl.c | 7 ++++++- > drivers/md/dm-table.c | 32 ++++++++++++++++++++++++-------- > 3 files changed, 31 insertions(+), 9 deletions(-) > > Index: linux-2.6/drivers/md/dm-core.h > =================================================================== > --- linux-2.6.orig/drivers/md/dm-core.h > +++ linux-2.6/drivers/md/dm-core.h > @@ -214,6 +214,7 @@ struct dm_table { > > /* a list of devices used by this table */ > struct list_head devices; > + struct rw_semaphore devices_lock; > > /* events get handed up using this callback */ > void (*event_fn)(void *data); > Index: linux-2.6/drivers/md/dm-ioctl.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-ioctl.c > +++ linux-2.6/drivers/md/dm-ioctl.c > @@ -1630,6 +1630,8 @@ static void retrieve_deps(struct dm_tabl > struct dm_dev_internal *dd; > struct dm_target_deps *deps; > > + down_read(&table->devices_lock); > + > deps = get_result_buffer(param, param_size, &len); > > /* > @@ -1644,7 +1646,7 @@ static void retrieve_deps(struct dm_tabl > needed = struct_size(deps, dev, count); > if (len < needed) { > param->flags |= DM_BUFFER_FULL_FLAG; > - return; > + goto out; > } > > /* > @@ -1656,6 +1658,9 @@ static void retrieve_deps(struct dm_tabl > deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev); > > param->data_size = param->data_start + needed; > + > +out: > + up_read(&table->devices_lock); > } > > static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size) > Index: linux-2.6/drivers/md/dm-table.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-table.c > +++ linux-2.6/drivers/md/dm-table.c > @@ -135,6 +135,7 @@ int dm_table_create(struct dm_table **re > return -ENOMEM; > > INIT_LIST_HEAD(&t->devices); > + init_rwsem(&t->devices_lock); > > if (!num_targets) > num_targets = KEYS_PER_NODE; > @@ -359,16 +360,20 @@ int __ref dm_get_device(struct dm_target > if (dev == disk_devt(t->md->disk)) > return -EINVAL; > > + down_write(&t->devices_lock); > + > dd = find_device(&t->devices, dev); > if (!dd) { > dd = kmalloc(sizeof(*dd), GFP_KERNEL); > - if (!dd) > - return -ENOMEM; > + if (!dd) { > + r = -ENOMEM; > + goto unlock_ret_r; > + } > > r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev); > if (r) { > kfree(dd); > - return r; > + goto unlock_ret_r; > } > > refcount_set(&dd->count, 1); > @@ -378,12 +383,17 @@ int __ref dm_get_device(struct dm_target > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { > r = upgrade_mode(dd, mode, t->md); > if (r) > - return r; > + goto unlock_ret_r; > } > refcount_inc(&dd->count); > out: > + up_write(&t->devices_lock); > *result = dd->dm_dev; > return 0; > + > +unlock_ret_r: > + up_write(&t->devices_lock); > + return r; > } > EXPORT_SYMBOL(dm_get_device); > > @@ -419,9 +429,12 @@ static int dm_set_device_limits(struct d > void dm_put_device(struct dm_target *ti, struct dm_dev *d) > { > int found = 0; > - struct list_head *devices = &ti->table->devices; > + struct dm_table *t = ti->table; > + struct list_head *devices = &t->devices; > struct dm_dev_internal *dd; > > + down_write(&t->devices_lock); > + > list_for_each_entry(dd, devices, list) { > if (dd->dm_dev == d) { > found = 1; > @@ -430,14 +443,17 @@ void dm_put_device(struct dm_target *ti, > } > if (!found) { > DMERR("%s: device %s not in table devices list", > - dm_device_name(ti->table->md), d->name); > - return; > + dm_device_name(t->md), d->name); > + goto unlock_ret; > } > if (refcount_dec_and_test(&dd->count)) { > - dm_put_table_device(ti->table->md, d); > + dm_put_table_device(t->md, d); > list_del(&dd->list); > kfree(dd); > } > + > +unlock_ret: > + up_write(&t->devices_lock); > } > EXPORT_SYMBOL(dm_put_device); > Tested-by: Li Lingfeng <lilingfeng3@huawei.com> > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 0e325469a252..1f51059520ac 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv, goto out; } - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev); + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false); if (r) { DMWARN("message: error getting device %s", argv[1]); diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index d8034ff0cb24..ad18a41f1608 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path) } EXPORT_SYMBOL_GPL(dm_get_dev_t); -/* - * Add a device to the list, or just increment the usage count if - * it's already present. - */ -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, - struct dm_dev **result) +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, + struct dm_dev **result, bool create_dd) { int r; dev_t dev; @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, dd = find_device(&t->devices, dev); if (!dd) { - dd = kmalloc(sizeof(*dd), GFP_KERNEL); - if (!dd) - return -ENOMEM; - - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { - kfree(dd); - return r; - } + if (create_dd) { + dd = kmalloc(sizeof(*dd), GFP_KERNEL); + if (!dd) + return -ENOMEM; - refcount_set(&dd->count, 1); - list_add(&dd->list, &t->devices); - goto out; + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) { + kfree(dd); + return r; + } + refcount_set(&dd->count, 1); + list_add(&dd->list, &t->devices); + goto out; + } else + return -ENODEV; } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { r = upgrade_mode(dd, mode, t->md); if (r) @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, *result = dd->dm_dev; return 0; } +EXPORT_SYMBOL(__dm_get_device); + +/* + * Add a device to the list, or just increment the usage count if + * it's already present. + */ +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, + struct dm_dev **result) +{ + return __dm_get_device(ti, path, mode, result, true); +} EXPORT_SYMBOL(dm_get_device); static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h index 04c6acf7faaa..ef4031a844b6 100644 --- a/include/linux/device-mapper.h +++ b/include/linux/device-mapper.h @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path); int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, struct dm_dev **result); void dm_put_device(struct dm_target *ti, struct dm_dev *d); +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode, + struct dm_dev **result, bool create_dd); /* * Information about a target type