linux-kernel - Re: [PATCH v5 3/4] make nbd device wait for its users

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+aCy1FmWxVWN22-DibTw+tgs=9fOip8A645Ui2e-JG9UdJjfA@mail.gmail.com>
Date:	Sat, 16 Jul 2016 16:12:24 +0530
From:	Pranay Srivastava <pranjas@...il.com>
To:	Markus Pargmann <mpa@...gutronix.de>,
	nbd-general@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Cc:	Pranay Kr Srivastava <pranjas@...il.com>
Subject: Re: [PATCH v5 3/4] make nbd device wait for its users

Hi Markus


On Sat, Jul 16, 2016 at 4:06 PM, Pranay Kr Srivastava <pranjas@...il.com> wrote:
>         When a timeout occurs or a recv fails, then
>         instead of abruptly killing nbd block device
>         wait for its users to finish.
>
>         This is more required when filesystem(s) like
>         ext2 or ext3 don't expect their buffer heads to
>         disappear while the filesystem is mounted.
>
>         Each open is now refcounted with the device being released
>         for re-use only when there are no "other users".
>
>         A timedout or a disconnected device, if in use, can't
>         be used until it has been resetted. The reset happens
>         when all tasks having this bdev open closes this bdev.
>
> Behavioral Change:
>
> 1)      NBD_DO_IT will not wait for the device to be reset. Hence
>         the nbd-client "may exit" while some other process is using
>         this device without actually doing the reset on this device
>         , hence thus making it unusable until all such user space
>         processes have stopped using this device.
>
> 2)      There's a window where the nbd-client will not be able to
>         change / issue ioctls to the nbd device. This is when there's
>         been a disconnect issued or a timeout has occured, however
>         there are "other" user processes which currently have this
>         nbd device opened.
>
> Signed-off-by: Pranay Kr Srivastava <pranjas@...il.com>
> ---
>  drivers/block/nbd.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 4919760..fe36280 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -74,6 +74,7 @@ struct nbd_device {
>          *This is specifically for calling sock_shutdown, for now.
>          */
>         struct work_struct ws_shutdown;
> +       atomic_t users; /* Users that opened the block device */
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -699,6 +700,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd);
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                        unsigned int cmd, unsigned long arg)
>  {
> +       if (nbd->disconnect || nbd->timedout)
> +               return -EBUSY;
> +
>         switch (cmd) {
>         case NBD_DISCONNECT: {
>                 struct request sreq;
> @@ -728,7 +732,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 nbd_clear_que(nbd);
>                 BUG_ON(!list_empty(&nbd->queue_head));
>                 BUG_ON(!list_empty(&nbd->waiting_queue));
> -               kill_bdev(bdev);
>                 return 0;
>
>         case NBD_SET_SOCK: {
> @@ -804,16 +807,12 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 mutex_lock(&nbd->tx_lock);
>                 nbd->task_recv = NULL;
>                 nbd_clear_que(nbd);
> -               kill_bdev(bdev);
> -               nbd_bdev_reset(bdev);
>
>                 if (nbd->disconnect) /* user requested, ignore socket errors */
>                         error = 0;
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>
> -               nbd_reset(nbd);
> -
>                 return error;
>         }
>
> @@ -852,10 +851,38 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>         return error;
>  }
>
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +       struct nbd_device *nbd = bdev->bd_disk->private_data;
> +
> +       atomic_inc(&nbd->users);
> +
> +       return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +       struct nbd_device *nbd = disk->private_data;
> +       struct block_device *bdev = bdget (part_devt(
> +                                               dev_to_part(nbd_to_dev(nbd))));
> +
> +       WARN_ON(!bdev);
> +       if (atomic_dec_and_test(&nbd->users)) {
> +               if (bdev) {
> +                       nbd_bdev_reset(bdev);
> +                       kill_bdev(bdev);
> +                       bdput(bdev);
> +               }
> +               nbd_reset(nbd);
> +       }
> +}
> +
>  static const struct block_device_operations nbd_fops = {
>         .owner =        THIS_MODULE,
>         .ioctl =        nbd_ioctl,
>         .compat_ioctl = nbd_ioctl,
> +       .open =         nbd_open,
> +       .release =      nbd_release,
>  };
>
>  static void nbd_ws_func_shutdown(struct work_struct *ws_nbd)
> --
> 2.6.2
>
I've taken the patch you had send earlier , with atomics, and modified that.

I've added the ioctl handling part as part of this patch instead of keeping
it separate.

Let me know of any changes that should be done.
I'll send the whole series again later after taking the reviews of everyone.

-- 
        ---P.K.S