Re: [PATCH] nbd: provide a way for userspace processes to identify device backends

[Prasanna Kalever]

On Fri, Apr 30, 2021 at 7:44 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
> On 2021/4/29 18:28, Prasanna Kumar Kalever wrote:
> > Problem:
> > On reconfigure of device, there is no way to defend if the backend
> > storage is matching with the initial backend storage.
> >
> > Say, if an initial connect request for backend "pool1/image1" got
> > mapped to /dev/nbd0 and the userspace process is terminated. A next
> > reconfigure request within NBD_ATTR_DEAD_CONN_TIMEOUT is allowed to
> > use /dev/nbd0 for a different backend "pool1/image2"
> >
> > For example, an operation like below could be dangerous:
> >
> > $ sudo rbd-nbd map --try-netlink rbd-pool/ext4-image
> > /dev/nbd0
> > $ sudo blkid /dev/nbd0
> > /dev/nbd0: UUID="bfc444b4-64b1-418f-8b36-6e0d170cfc04" TYPE="ext4"
> > $ sudo pkill -9 rbd-nbd
> > $ sudo rbd-nbd attach --try-netlink --device /dev/nbd0 rbd-pool/xfs-image
> > /dev/nbd0
> > $ sudo blkid /dev/nbd0
> > /dev/nbd0: UUID="d29bf343-6570-4069-a9ea-2fa156ced908" TYPE="xfs"
> >
> > Solution:
> > Provide a way for userspace processes to keep some metadata to identify
> > between the device and the backend, so that when a reconfigure request is
> > made, we can compare and avoid such dangerous operations.
> >
> > With this solution, as part of the initial connect request, backend
> > path can be stored in the sysfs per device config, so that on a reconfigure
> > request it's easy to check if the backend path matches with the initial
> > connect backend path.
> >
> > Please note, ioctl interface to nbd will not have these changes, as there
> > won't be any reconfigure.
> >
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
> > ---
> >   drivers/block/nbd.c              | 60 +++++++++++++++++++++++++++++++-
> >   include/uapi/linux/nbd-netlink.h |  1 +
> >   2 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 4ff71b579cfc..b5022187ad9c 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -79,6 +79,7 @@ struct link_dead_args {
> >   #define NBD_RT_HAS_CONFIG_REF               4
> >   #define NBD_RT_BOUND                        5
> >   #define NBD_RT_DISCONNECT_ON_CLOSE  6
> > +#define NBD_RT_HAS_BACKEND_FILE              7
> >
> >   #define NBD_DESTROY_ON_DISCONNECT   0
> >   #define NBD_DISCONNECT_REQUESTED    1
> > @@ -119,6 +120,8 @@ struct nbd_device {
> >
> >       struct completion *destroy_complete;
> >       unsigned long flags;
> > +
> > +     char *backend;
> >   };
> >
> >   #define NBD_CMD_REQUEUED    1
> > @@ -216,6 +219,20 @@ static const struct device_attribute pid_attr = {
> >       .show = pid_show,
> >   };
> >
> > +static ssize_t backend_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf)
> > +{
> > +     struct gendisk *disk = dev_to_disk(dev);
> > +     struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
> > +
> > +     return sprintf(buf, "%s\n", nbd->backend ?: "");
> > +}
> > +
> > +static const struct device_attribute backend_attr = {
> > +     .attr = { .name = "backend", .mode = 0444},
> > +     .show = backend_show,
> > +};
> > +
> >   static void nbd_dev_remove(struct nbd_device *nbd)
> >   {
> >       struct gendisk *disk = nbd->disk;
> > @@ -1215,6 +1232,12 @@ static void nbd_config_put(struct nbd_device *nbd)
> >                                      &config->runtime_flags))
> >                       device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
> >               nbd->task_recv = NULL;
> > +             if (test_and_clear_bit(NBD_RT_HAS_BACKEND_FILE,
> > +                                    &config->runtime_flags)) {
> > +                     device_remove_file(disk_to_dev(nbd->disk), &backend_attr);
> > +                     kfree(nbd->backend);
> > +                     nbd->backend = NULL;
> > +             }
> >               nbd_clear_sock(nbd);
> >               if (config->num_connections) {
> >                       int i;
> > @@ -1274,7 +1297,7 @@ static int nbd_start_device(struct nbd_device *nbd)
> >
> >       error = device_create_file(disk_to_dev(nbd->disk), &pid_attr);
> >       if (error) {
> > -             dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n");
> > +             dev_err(disk_to_dev(nbd->disk), "device_create_file failed for pid!\n");
> >               return error;
> >       }
> >       set_bit(NBD_RT_HAS_PID_FILE, &config->runtime_flags);
> > @@ -1681,6 +1704,7 @@ static int nbd_dev_add(int index)
> >               BLK_MQ_F_BLOCKING;
> >       nbd->tag_set.driver_data = nbd;
> >       nbd->destroy_complete = NULL;
> > +     nbd->backend = NULL;
> >
> >       err = blk_mq_alloc_tag_set(&nbd->tag_set);
> >       if (err)
> > @@ -1754,6 +1778,7 @@ static const struct nla_policy nbd_attr_policy[NBD_ATTR_MAX + 1] = {
> >       [NBD_ATTR_SOCKETS]              =       { .type = NLA_NESTED},
> >       [NBD_ATTR_DEAD_CONN_TIMEOUT]    =       { .type = NLA_U64 },
> >       [NBD_ATTR_DEVICE_LIST]          =       { .type = NLA_NESTED},
> > +     [NBD_ATTR_BACKEND_IDENTIFIER]   =       { .type = NLA_STRING},
> >   };
> >
> >   static const struct nla_policy nbd_sock_policy[NBD_SOCK_MAX + 1] = {
> > @@ -1956,6 +1981,23 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
> >               }
> >       }
> >       ret = nbd_start_device(nbd);
> > +     if (ret)
> > +             goto out;
> > +     if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) {
> > +             nbd->backend = nla_strdup(info->attrs[NBD_ATTR_BACKEND_IDENTIFIER],
> > +                                       GFP_KERNEL);
> > +             if (!nbd->backend) {
> > +                     ret = -ENOMEM;
> > +                     goto out;
> > +             }
> > +     }
> > +     ret = device_create_file(disk_to_dev(nbd->disk), &backend_attr);
> > +     if (ret) {
> > +             dev_err(disk_to_dev(nbd->disk),
> > +                     "device_create_file failed for backend!\n");
> > +             goto out;
> > +     }
> > +     set_bit(NBD_RT_HAS_BACKEND_FILE, &config->runtime_flags);
> >   out:
> >       mutex_unlock(&nbd->config_lock);
> >       if (!ret) {
> > @@ -2048,6 +2090,22 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
> >                      index);
> >               return -EINVAL;
> >       }
> > +     if (nbd->backend) {
> > +             if (info->attrs[NBD_ATTR_BACKEND_IDENTIFIER]) {
> > +                     if (nla_strcmp(info->attrs[NBD_ATTR_BACKEND_IDENTIFIER],
> > +                                    nbd->backend)) {
> > +                             mutex_unlock(&nbd_index_mutex);
> > +                             dev_err(nbd_to_dev(nbd),
> > +                                     "backend image doesn't match with %s\n",
> > +                                     nbd->backend);
> > +                             return -EINVAL;
> > +                     }
> > +             } else {
> > +                     mutex_unlock(&nbd_index_mutex);
> > +                     dev_err(nbd_to_dev(nbd), "must specify backend\n");
> > +                     return -EINVAL;
> > +             }
> > +     }
> >       if (!refcount_inc_not_zero(&nbd->refs)) {
> >               mutex_unlock(&nbd_index_mutex);
> >               printk(KERN_ERR "nbd: device at index %d is going down\n",
> > diff --git a/include/uapi/linux/nbd-netlink.h b/include/uapi/linux/nbd-netlink.h
> > index c5d0ef7aa7d5..2d0b90964227 100644
> > --- a/include/uapi/linux/nbd-netlink.h
> > +++ b/include/uapi/linux/nbd-netlink.h
> > @@ -35,6 +35,7 @@ enum {
> >       NBD_ATTR_SOCKETS,
> >       NBD_ATTR_DEAD_CONN_TIMEOUT,
> >       NBD_ATTR_DEVICE_LIST,
> > +     NBD_ATTR_BACKEND_IDENTIFIER,
> >       __NBD_ATTR_MAX,
> >   };
> >   #define NBD_ATTR_MAX (__NBD_ATTR_MAX - 1)
>
> Reviewed-by: Xiubo Li <xiubli@xxxxxxxxxx>

Thank you Xiubo Li.

Assuming this patch got lost, attempting to bring this patch onto the
top with a friendly reminder.
I sincerely request the maintainers to please take a look.

Many Thanks!
--
Prasanna