linux-kernel - Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests

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: <20211230043322.2ba19bbd.pasic@linux.ibm.com>
Date:   Thu, 30 Dec 2021 04:33:22 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, jjherne@...ux.ibm.com, freude@...ux.ibm.com,
        borntraeger@...ibm.com, cohuck@...hat.com, mjrosato@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        fiuczy@...ux.ibm.com, Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests

On Thu, 21 Oct 2021 11:23:25 -0400
Tony Krowiak <akrowiak@...ux.ibm.com> wrote:

> The vfio_ap device driver registers for notification when the pointer to
> the KVM object for a guest is set. Let's store the KVM pointer as well as
> the pointer to the mediated device when the KVM pointer is set.

[..]


> struct ap_matrix_dev {
>         ...
>         struct rw_semaphore guests_lock;
>         struct list_head guests;
>        ...
> }
> 
> The 'guests_lock' field is a r/w semaphore to control access to the
> 'guests' field. The 'guests' field is a list of ap_guest
> structures containing the KVM and matrix_mdev pointers for each active
> guest. An ap_guest structure will be stored into the list whenever the
> vfio_ap device driver is notified that the KVM pointer has been set and
> removed when notified that the KVM pointer has been cleared.
> 

Is this about the field or about the list including all the nodes? This
reads lie guests_lock only protects the head element, which makes no
sense to me. Because of how these lists work.

The narrowest scope that could make sense is all the list_head stuff
in the entire list. I.e. one would only need the lock to traverse or
manipulate the list, while the payload would still be subject to
the matrix_dev->lock mutex.

[..]

> +struct ap_guest {
> +	struct kvm *kvm;
> +	struct list_head node;
> +};
> +
>  /**
>   * struct ap_matrix_dev - Contains the data for the matrix device.
>   *
> @@ -39,6 +44,9 @@
>   *		single ap_matrix_mdev device. It's quite coarse but we don't
>   *		expect much contention.
>   * @vfio_ap_drv: the vfio_ap device driver
> + * @guests_lock: r/w semaphore for protecting access to @guests
> + * @guests:	list of guests (struct ap_guest) using AP devices bound to the
> + *		vfio_ap device driver.

Please compare the above. Also if it is only about the access to the
list, then you could drop the lock right after create, and not keep it
till the very end of vfio_ap_mdev_set_kvm(). Right?

In any case I'm skeptical about this whole struct ap_guest business. To
me, it looks like something that just makes things more obscure and
complicated without any real benefit.

Regards,
Halil

>   */
>  struct ap_matrix_dev {
>  	struct device device;
> @@ -47,6 +55,8 @@ struct ap_matrix_dev {
>  	struct list_head mdev_list;
>  	struct mutex lock;
>  	struct ap_driver  *vfio_ap_drv;
> +	struct rw_semaphore guests_lock;
> +	struct list_head guests;
>  };
>  
>  extern struct ap_matrix_dev *matrix_dev;