Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

[Pranay Srivastava]

Hi,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:
> After NBD_DO_IT exited the block device may still be used. Make sure
> that we handle intended disconnects differently and do not allow any
> changed of the nbd device.
>
> This patch should avoid that the nbd-client connects to a different server
> and the users of the block device are suddenly reading/writing from a
> different backend device.
>
> For timeouts it is still possible to setup a new socket so that the
> connection may be refreshed without creating problems for all users.

But Shouldn't time out be checked for last end point?

For example, consider the following steps

1) Timeout occurs but server[nbd-s1] comes up again albeit with a different
    network address.

2) The previous network address of server [nbd-s1] has now been assigned to
    another new nbd server [nbd-s2]

3) A new nbd-client tries to setup the socket again, Negotiation would
be done again
    [correct?]. If correct then wouldn't we be sending data to wrong
device this time?

So instead can't we put a mechanism in place for network address + mac
to be same
for allowing clients to reconnect? Do let me know if this is not of concern.

4) If 3) doesn't apply then let's disallow all ioctls until nbd device
is reset.

>
> Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> ---
>  drivers/block/nbd.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 620660f3ff0f..39358efac73e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -708,6 +708,18 @@ 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)
>  {
> +       /*
> +        * After a disconnect was instructed, do not allow any further actions
> +        * on the block device that would lead to a new connected endpoint.
> +        * This condition stays until nbd_reset was called either because all
> +        * users closed the device or because of CLEAR_SOCK.
> +        */
> +       if (nbd->disconnect &&
> +           cmd != NBD_CLEAR_SOCK && cmd != NBD_PRINT_DEBUG) {
> +               dev_info(disk_to_dev(nbd->disk), "Device is still busy after instructing a disconnect\n");
> +               return -EBUSY;
> +       }
> +
>         switch (cmd) {
>         case NBD_DISCONNECT: {
>                 struct request sreq;
> @@ -733,11 +745,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>         }
>
>         case NBD_CLEAR_SOCK:
> -               sock_shutdown(nbd);
> -               nbd_clear_que(nbd);
> -               BUG_ON(!list_empty(&nbd->queue_head));
> -               BUG_ON(!list_empty(&nbd->waiting_queue));
> -               kill_bdev(bdev);
> +               if (nbd->disconnect) {
> +                       nbd_reset(nbd);
> +               } else {
> +                       sock_shutdown(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: {
> @@ -812,8 +828,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 mutex_lock(&nbd->tx_lock);
>                 nbd->task_recv = NULL;
>
> -               if (nbd->disconnect) /* user requested, ignore socket errors */
> +               if (nbd->disconnect) { /* user requested, ignore socket errors */
> +                       sock_shutdown(nbd);
>                         error = 0;
> +               }
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>
> --
> 2.1.4
>



-- 
        ---P.K.S