Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444767 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] nbd: Disallow ioctls on disconnected block device Pranay Srivastava <pranjas@gmail.com> - 2016-07-16 09:50 +0200
    Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device Alex Bligh <alex@alex.org.uk> - 2016-07-16 11:40 +0200
      Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device Pranay Srivastava <pranjas@gmail.com> - 2016-07-16 12:10 +0200
        Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block  device Wouter Verhelst <w@uter.be> - 2016-07-16 13:30 +0200
          Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device Pranay Srivastava <pranjas@gmail.com> - 2016-07-16 15:40 +0200

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

Hi,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@pengutronix.de> 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@pengutronix.de>
> ---
>  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

[toc] | [next] | [standalone]

#1444794 — Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

On 16 Jul 2016, at 08:42, Pranay Srivastava <pranjas@gmail.com> wrote:

> 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.

MAC address?! nbd clients connect over IP, and if a router reboots
between them, you could easily see two packets from the same client
come from different MAC addresses. Similarly all clients not on
the same L2 network will carry the same MAC address. So MAC address
is a very poor indicator of 'same client'.

IP address is also a poor indicator (think NAT) but is substantially
less bad.

-- 
Alex Bligh

[toc] | [prev] | [next] | [standalone]

#1444796 — Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

On Sat, Jul 16, 2016 at 3:02 PM, Alex Bligh <alex@alex.org.uk> wrote:
>
> On 16 Jul 2016, at 08:42, Pranay Srivastava <pranjas@gmail.com> wrote:
>
>> 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.
>
> MAC address?! nbd clients connect over IP, and if a router reboots
> between them, you could easily see two packets from the same client
> come from different MAC addresses. Similarly all clients not on
> the same L2 network will carry the same MAC address. So MAC address
> is a very poor indicator of 'same client'.
>
> IP address is also a poor indicator (think NAT) but is substantially
> less bad.

Okay. So how about we include some negotiated key which goes in with every
request which the server could maintain for clients that can be checked while
resetting the connection with the same server?

So am I correct that this situation can
indeed happen or the server will throw an error back to client in case
the troubled
nbd-client is trying to reconnect to the original server but requests
are going to
another server?

If yes to above query then what is the best effort we can do to avoid
such scenarios?

>
> --
> Alex Bligh
>
>
>
>



-- 
        ---P.K.S

[toc] | [prev] | [next] | [standalone]

#1444818 — Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

On Sat, Jul 16, 2016 at 03:38:40PM +0530, Pranay Srivastava wrote:
> Okay. So how about we include some negotiated key which goes in with every
> request which the server could maintain for clients that can be checked while
> resetting the connection with the same server?

Wut?

> So am I correct that this situation can
> indeed happen or the server will throw an error back to client in case
> the troubled
> nbd-client is trying to reconnect to the original server but requests
> are going to
> another server?
> 
> If yes to above query then what is the best effort we can do to avoid
> such scenarios?

Tell userspace not to do stupid things?

This isn't a problem. The kernel assumes that whatever userspace does,
once the connection is set up again everything's the way it was before.
If that's not true, then userspace is to blame, not kernel space.

Adding a "key" which we need to pass is going to make things wildly more
complicated for no benefit.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

[toc] | [prev] | [next] | [standalone]

#1444828 — Re: [Nbd] [PATCH 2/2] nbd: Disallow ioctls on disconnected block device

On Sat, Jul 16, 2016 at 4:56 PM, Wouter Verhelst <w@uter.be> wrote:
> On Sat, Jul 16, 2016 at 03:38:40PM +0530, Pranay Srivastava wrote:
>> Okay. So how about we include some negotiated key which goes in with every
>> request which the server could maintain for clients that can be checked while
>> resetting the connection with the same server?
>
> Wut?
>
>> So am I correct that this situation can
>> indeed happen or the server will throw an error back to client in case
>> the troubled
>> nbd-client is trying to reconnect to the original server but requests
>> are going to
>> another server?
>>
>> If yes to above query then what is the best effort we can do to avoid
>> such scenarios?
>
> Tell userspace not to do stupid things?
>
> This isn't a problem. The kernel assumes that whatever userspace does,
> once the connection is set up again everything's the way it was before.
> If that's not true, then userspace is to blame, not kernel space.
>
> Adding a "key" which we need to pass is going to make things wildly more
> complicated for no benefit.
Okay.
So let things roll for timeout but stop for disconnect.
>
> --
> < ron> I mean, the main *practical* problem with C++, is there's like a dozen
>        people in the world who think they really understand all of its rules,
>        and pretty much all of them are just lying to themselves too.
>  -- #debian-devel, OFTC, 2016-02-12



-- 
        ---P.K.S

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web