From 69c6f139a7202713e01b04024ddefe8019d50efd Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Thu, 7 Sep 2017 16:05:20 +0200 Subject: [PATCH] krbd: rewrite "already mapped" code The "already mapped" code, introduced in commit d6a66fc8f49b ("rbd: before rbd map, warn if the image is already mapped") is broken: because of a use-after-free on attribute strings, the warning isn't even printed half the time. Rewrite making use of udev enumeration filters and fix the interface while at it. Signed-off-by: Ilya Dryomov --- src/include/krbd.h | 6 +- src/krbd.cc | 102 +++++++++++++++++---------------- src/tools/rbd/action/Kernel.cc | 12 ++-- 3 files changed, 61 insertions(+), 59 deletions(-) diff --git a/src/include/krbd.h b/src/include/krbd.h index 4f6da4fbfaf..eb753f5b684 100644 --- a/src/include/krbd.h +++ b/src/include/krbd.h @@ -26,6 +26,8 @@ void krbd_destroy(struct krbd_ctx *ctx); int krbd_map(struct krbd_ctx *ctx, const char *pool, const char *image, const char *snap, const char *options, char **pdevnode); +int krbd_is_mapped(struct krbd_ctx *ctx, const char *pool, const char *image, + const char *snap, char **pdevnode); int krbd_unmap(struct krbd_ctx *ctx, const char *devnode, const char *options); @@ -45,10 +47,6 @@ namespace ceph { int krbd_showmapped(struct krbd_ctx *ctx, ceph::Formatter *f); -int krbd_is_image_mapped(struct krbd_ctx *ctx, const char *poolname, - const char *imgname, const char *snapname, - std::ostringstream &mapped_info, bool &is_mapped); - #endif /* __cplusplus */ #endif /* CEPH_KRBD_H */ diff --git a/src/krbd.cc b/src/krbd.cc index 50292c18ff2..523c3db1f5f 100644 --- a/src/krbd.cc +++ b/src/krbd.cc @@ -713,6 +713,46 @@ int dump_images(struct krbd_ctx *ctx, Formatter *f) return r; } +static int is_mapped_image(struct udev *udev, const char *pool, + const char *image, const char *snap, string *pname) +{ + struct udev_enumerate *enm; + struct udev_list_entry *l; + int r; + + if (strcmp(snap, "") == 0) + snap = "-"; + + enm = udev_enumerate_new(udev); + if (!enm) + return -ENOMEM; + + r = enumerate_devices(enm, pool, image, snap); + if (r < 0) + goto out_enm; + + l = udev_enumerate_get_list_entry(enm); + if (l) { + struct udev_device *dev; + + dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); + if (!dev) { + r = -ENOMEM; + goto out_enm; + } + + r = 1; + *pname = get_kernel_rbd_name(udev_device_get_sysname(dev)); + udev_device_unref(dev); + } else { + r = 0; /* not mapped */ + } + +out_enm: + udev_enumerate_unref(enm); + return r; +} + extern "C" int krbd_create_from_context(rados_config_t cct, struct krbd_ctx **pctx) { @@ -777,58 +817,22 @@ int krbd_showmapped(struct krbd_ctx *ctx, Formatter *f) return dump_images(ctx, f); } -int krbd_is_image_mapped(struct krbd_ctx *ctx, const char *poolname, - const char *imgname, const char *snapname, - std::ostringstream &mapped_info, bool &is_mapped) { - struct udev_enumerate *enm; - struct udev_list_entry *l; - struct udev *udev = ctx->udev; - const char *mapped_id, *mapped_pool, *mapped_image, *mapped_snap; +extern "C" int krbd_is_mapped(struct krbd_ctx *ctx, const char *pool, + const char *image, const char *snap, + char **pdevnode) +{ + string name; + char *devnode; int r; - is_mapped = false; - - enm = udev_enumerate_new(udev); - if(!enm) - return -ENOMEM; - - r = udev_enumerate_add_match_subsystem(enm, "rbd"); - if (r < 0) - goto out_enm; - - r = udev_enumerate_scan_devices(enm); - if (r < 0) - goto out_enm; - udev_list_entry_foreach(l, udev_enumerate_get_list_entry(enm)) { - struct udev_device *dev; + r = is_mapped_image(ctx->udev, pool, image, snap, &name); + if (r <= 0) /* error or not mapped */ + return r; - dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); - if (dev) { - mapped_id = udev_device_get_sysname(dev); - mapped_pool = udev_device_get_sysattr_value(dev, "pool"); - mapped_image = udev_device_get_sysattr_value(dev, "name"); - mapped_snap = udev_device_get_sysattr_value(dev, "current_snap"); - string kname = get_kernel_rbd_name(mapped_id); + devnode = strdup(name.c_str()); + if (!devnode) + return -ENOMEM; - udev_device_unref(dev); - if (mapped_pool && poolname && strcmp(mapped_pool, poolname) == 0 && - mapped_image && imgname && strcmp(mapped_image, imgname) == 0) { - if (!snapname || snapname[0] == '\0') { - mapped_info << "image " << *poolname << "/" << *imgname - << " already mapped as " << kname; - is_mapped = true; - goto out_enm; - } else if (snapname && mapped_snap && - strcmp(snapname, mapped_snap) == 0) { - mapped_info << "image " << *poolname << "/" << *imgname << "@" - << *snapname << " already mapped as " << kname; - is_mapped = true; - goto out_enm; - } - } - } - } -out_enm: - udev_enumerate_unref(enm); + *pdevnode = devnode; return r; } diff --git a/src/tools/rbd/action/Kernel.cc b/src/tools/rbd/action/Kernel.cc index 7c848070378..a0ffca60f41 100644 --- a/src/tools/rbd/action/Kernel.cc +++ b/src/tools/rbd/action/Kernel.cc @@ -289,10 +289,9 @@ static int do_kernel_map(const char *poolname, const char *imgname, { #if defined(WITH_KRBD) struct krbd_ctx *krbd; - std::ostringstream oss, mapped_info; + std::ostringstream oss; char *devnode; int r; - bool img_mapped; r = krbd_create_from_context(g_ceph_context, &krbd); if (r < 0) @@ -313,13 +312,14 @@ static int do_kernel_map(const char *poolname, const char *imgname, } } - r = krbd_is_image_mapped(krbd, poolname, imgname, snapname, - mapped_info, img_mapped); + r = krbd_is_mapped(krbd, poolname, imgname, snapname, &devnode); if (r < 0) { std::cerr << "rbd: warning: can't get image map infomation: " << cpp_strerror(r) << std::endl; - } else if (img_mapped) { - std::cerr << "rbd: warning: " << mapped_info.str() << std::endl; + } else if (r > 0) { + std::cerr << "rbd: warning: image already mapped as " << devnode + << std::endl; + free(devnode); } r = krbd_map(krbd, poolname, imgname, snapname, oss.str().c_str(), &devnode); -- 2.39.5