netdev - Re: [PATCH V6 vfio 05/10] vfio: Introduce the DMA logging feature support

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: <20220906164154.756e26aa.alex.williamson@redhat.com>
Date:   Tue, 6 Sep 2022 16:41:54 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Yishai Hadas <yishaih@...dia.com>
Cc:     <jgg@...dia.com>, <saeedm@...dia.com>, <kvm@...r.kernel.org>,
        <netdev@...r.kernel.org>, <kuba@...nel.org>,
        <kevin.tian@...el.com>, <joao.m.martins@...cle.com>,
        <leonro@...dia.com>, <maorg@...dia.com>, <cohuck@...hat.com>
Subject: Re: [PATCH V6 vfio 05/10] vfio: Introduce the DMA logging feature
 support

On Mon, 5 Sep 2022 13:58:47 +0300
Yishai Hadas <yishaih@...dia.com> wrote:
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e05ddc6fe6a5..b17f2f454389 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -108,6 +110,21 @@ struct vfio_migration_ops {
>  				   enum vfio_device_mig_state *curr_state);
>  };
>  
> +/**
> + * @log_start: Optional callback to ask the device start DMA logging.
> + * @log_stop: Optional callback to ask the device stop DMA logging.
> + * @log_read_and_clear: Optional callback to ask the device read
> + *         and clear the dirty DMAs in some given range.

I don't see anywhere in the core that we track the device state
relative to the DEVICE_FEATURE_DMA_LOGGING features, nor do we
explicitly put the responsibility on the driver implementation to
handle invalid user requests.  The mlx5 driver implementation appears
to do this, but maybe we should at least include a requirement here, ex.

   The vfio core implementation of the DEVICE_FEATURE_DMA_LOGGING_ set
   of features does not track logging state relative to the device,
   therefore the device implementation of vfio_log_ops must handle
   arbitrary user requests.  This includes rejecting subsequent calls
   to log_start without an intervening log_stop, as well as graceful
   handling of log_stop and log_read_and_clear from invalid states.

With something like that.

Acked-by: Alex Williamson <alex.williamson@...hat.com>

You can also add my Ack on 3, 4, and (fwiw) 6-10 as I assume this would
be a PR from Leon.  Thanks,

Alex

> + */
> +struct vfio_log_ops {
> +	int (*log_start)(struct vfio_device *device,
> +		struct rb_root_cached *ranges, u32 nnodes, u64 *page_size);
> +	int (*log_stop)(struct vfio_device *device);
> +	int (*log_read_and_clear)(struct vfio_device *device,
> +		unsigned long iova, unsigned long length,
> +		struct iova_bitmap *dirty);
> +};
> +
>  /**
>   * vfio_check_feature - Validate user input for the VFIO_DEVICE_FEATURE ioctl
>   * @flags: Arg from the device_feature op