Re: [PATCH] rumpdisk: Protect open/close/read/write with a rwlock

[Samuel Thibault]

Damien Zammit, le dim. 27 févr. 2022 00:27:02 +0000, a ecrit:
> TESTED to boot off a rump based disk

Applied, thanks!

> ---
>  rumpdisk/block-rump.c | 77 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/rumpdisk/block-rump.c b/rumpdisk/block-rump.c
> index 10fa66ad..464c5cdd 100644
> --- a/rumpdisk/block-rump.c
> +++ b/rumpdisk/block-rump.c
> @@ -52,6 +52,9 @@ static bool disabled;
> 
>  static mach_port_t master_host;
> 
> +/* rwlock to protect concurrent close while reading/writing */
> +pthread_rwlock_t rumpdisk_rwlock = PTHREAD_RWLOCK_INITIALIZER;
> +
>  /* One of these is associated with each open instance of a device.  */
>  struct block_data
>  {
> @@ -83,7 +86,8 @@ search_bd (const char *name)
> 
>    while (bd)
>      {
> -      if (!strcmp (bd->name, name))
> +      /* Check name and refcount > 0 */
> +      if (!strcmp (bd->name, name) && bd->taken)
>       return bd;
>        bd = bd->next;
>      }
> @@ -183,16 +187,26 @@ rumpdisk_device_close (void *d)
>    struct block_data *bd = d;
>    io_return_t err;
> 
> +  pthread_rwlock_wrlock (&rumpdisk_rwlock);
> +
>    bd->taken--;
>    if (bd->taken)
> -    return D_SUCCESS;
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return D_SUCCESS;
> +    }
> 
>    err = rump_errno2host (rump_sys_close (bd->rump_fd));
>    if (err != D_SUCCESS)
> -    return err;
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return err;
> +    }
> 
>    ports_port_deref (bd);
>    ports_destroy_right (bd);
> +
> +  pthread_rwlock_unlock (&rumpdisk_rwlock);
>    return D_SUCCESS;
>  }
> 
> @@ -229,6 +243,9 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
>    if (! is_disk_device (name))
>      return D_NO_SUCH_DEVICE;
> 
> +  /* Protect against concurrent open/close */
> +  pthread_rwlock_wrlock (&rumpdisk_rwlock);
> +
>    /* Find previous device or open if new */
>    bd = search_bd (name);
>    if (bd)
> @@ -236,6 +253,8 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
>        bd->taken++;
>        *devp = ports_get_right (bd);
>        *devicePoly = MACH_MSG_TYPE_MAKE_SEND;
> +
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
>        return D_SUCCESS;
>      }
> 
> @@ -243,12 +262,16 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
> 
>    fd = rump_sys_open (dev_name, dev_mode_to_rump_mode (mode));
>    if (fd < 0)
> -    return rump_errno2host (errno);
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return rump_errno2host (errno);
> +    }
> 
>    ret = rump_sys_ioctl (fd, DIOCGMEDIASIZE, &media_size);
>    if (ret < 0)
>      {
>        mach_print ("DIOCGMEDIASIZE ioctl fails\n");
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
>        return rump_errno2host (errno);
>      }
> 
> @@ -256,6 +279,7 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
>    if (ret < 0)
>      {
>        mach_print ("DIOCGSECTORSIZE ioctl fails\n");
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
>        return rump_errno2host (errno);
>      }
> 
> @@ -263,6 +287,7 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
>    if (err != 0)
>      {
>        rump_sys_close (fd);
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
>        return err;
>      }
> 
> @@ -280,6 +305,8 @@ rumpdisk_device_open (mach_port_t reply_port, 
> mach_msg_type_name_t reply_port_ty
> 
>    *devp = ports_get_right (bd);
>    *devicePoly = MACH_MSG_TYPE_MAKE_SEND;
> +
> +  pthread_rwlock_unlock (&rumpdisk_rwlock);
>    return D_SUCCESS;
>  }
> 
> @@ -296,6 +323,14 @@ rumpdisk_device_write (void *d, mach_port_t reply_port,
>    if ((bd->mode & D_WRITE) == 0)
>      return D_INVALID_OPERATION;
> 
> +  pthread_rwlock_rdlock (&rumpdisk_rwlock);
> +  /* Ensure device is still open */
> +  if (! bd->taken)
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return D_INVALID_OPERATION;
> +    }
> +
>    if ((vm_offset_t) data % pagesize)
>      {
>        /* Not aligned, have to copy to aligned buffer.  */
> @@ -307,7 +342,10 @@ rumpdisk_device_write (void *d, mach_port_t reply_port,
>        /* While at it, make it contiguous */
>        ret = vm_allocate_contiguous (master_host, mach_task_self (), &buf, 
> &pap, count, 0, 0x100000000ULL, 0);
>        if (ret != KERN_SUCCESS)
> -     return ENOMEM;
> +     {
> +       pthread_rwlock_unlock (&rumpdisk_rwlock);
> +       return ENOMEM;
> +     }
> 
>        memcpy ((void*) buf, data, count);
> 
> @@ -317,7 +355,10 @@ rumpdisk_device_write (void *d, mach_port_t reply_port,
>        vm_deallocate (mach_task_self (), (vm_address_t) buf, count);
> 
>        if (written < 0)
> -     return rump_errno2host (err);
> +     {
> +       pthread_rwlock_unlock (&rumpdisk_rwlock);
> +       return rump_errno2host (err);
> +     }
>      }
>    else
>      {
> @@ -343,7 +384,10 @@ rumpdisk_device_write (void *d, mach_port_t reply_port,
>         done = rump_sys_pwrite (bd->rump_fd, (const void *)data + written, 
> todo, (off_t)bn * bd->block_size + written);
> 
>         if (done < 0)
> -         return rump_errno2host (errno);
> +         {
> +           pthread_rwlock_unlock (&rumpdisk_rwlock);
> +           return rump_errno2host (errno);
> +         }
> 
>         written += done;
>       }
> @@ -352,6 +396,7 @@ rumpdisk_device_write (void *d, mach_port_t reply_port,
>    vm_deallocate (mach_task_self (), (vm_address_t) data, count);
> 
>    *bytes_written = (int)written;
> +  pthread_rwlock_unlock (&rumpdisk_rwlock);
>    return D_SUCCESS;
>  }
> 
> @@ -375,11 +420,22 @@ rumpdisk_device_read (void *d, mach_port_t reply_port,
>    if (count == 0)
>      return D_SUCCESS;
> 
> +  pthread_rwlock_rdlock (&rumpdisk_rwlock);
> +  /* Ensure device is still open */
> +  if (! bd->taken)
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return D_INVALID_OPERATION;
> +    }
> +
>    /* TODO: directly write at *data when it is aligned */
>    *data = 0;
>    ret = vm_allocate (mach_task_self (), &buf, npages * pagesize, TRUE);
>    if (ret != KERN_SUCCESS)
> -    return ENOMEM;
> +    {
> +      pthread_rwlock_unlock (&rumpdisk_rwlock);
> +      return ENOMEM;
> +    }
> 
>    /* Ensure physical allocation by writing a single byte of each */
>    for (i = 0; i < npages; i++)
> @@ -400,6 +456,7 @@ rumpdisk_device_read (void *d, mach_port_t reply_port,
>       {
>         err = errno;
>         vm_deallocate (mach_task_self (), buf, npages * pagesize);
> +       pthread_rwlock_unlock (&rumpdisk_rwlock);
>         return rump_errno2host (err);
>       }
> 
> @@ -408,6 +465,7 @@ rumpdisk_device_read (void *d, mach_port_t reply_port,
> 
>    *bytes_read = done;
>    *data = (void*) buf;
> +  pthread_rwlock_unlock (&rumpdisk_rwlock);
>    return D_SUCCESS;
>  }
> 
> @@ -455,9 +513,6 @@ rumpdisk_device_get_status (void *d, dev_flavor_t flavor, 
> dev_status_t status,
>   * Short term strategy:
>   *
>   * Make device_read/write multithreaded.
> - * Note: this would require an rwlock between device_open/close/read/write, 
> to
> - * protect against e.g. concurrent open, unexpected close while read/write is
> - * called, etc.
>   *
>   * Long term strategy:
>   *
> --
> 2.35.1
> 
> 
>