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] [day] [month] [year] [list]

Message-ID: <YxhoKWfnwiLqxyQX@unreal>
Date:   Wed, 7 Sep 2022 12:45:13 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Yishai Hadas <yishaih@...dia.com>, 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,
        maorg@...dia.com, cohuck@...hat.com
Subject: Re: [PATCH V6 vfio 05/10] vfio: Introduce the DMA logging feature
 support

On Tue, Sep 06, 2022 at 04:41:54PM -0600, Alex Williamson wrote:
> 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,

Yes, it is.
I sent PR based on clean 6.0-rc4.

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
>