linux-kernel - Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device

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: <20220307142711.5af33ece.pasic@linux.ibm.com>
Date:   Mon, 7 Mar 2022 14:27:11 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>
Cc:     jjherne@...ux.ibm.com, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        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 v18 08/18] s390/vfio-ap: allow assignment of unavailable
 AP queues to mdev device

On Mon, 7 Mar 2022 07:31:21 -0500
Tony Krowiak <akrowiak@...ux.ibm.com> wrote:

> On 3/3/22 10:39, Jason J. Herne wrote:
> > On 2/14/22 19:50, Tony Krowiak wrote:  
> >>   /**
> >> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is 
> >> not configured
> >> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by 
> >> matrix mdevs
> >>    *
> >> - * @matrix_mdev: the mediated matrix device
> >> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> >> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> >>    *
> >> - * Verifies that the APQNs derived from the cross product of the AP 
> >> adapter IDs
> >> - * and AP queue indexes comprising the AP matrix are not configured 
> >> for another
> >> + * Verifies that each APQN derived from the Cartesian product of a 
> >> bitmap of
> >> + * AP adapter IDs and AP queue indexes is not configured for any matrix
> >>    * mediated device. AP queue sharing is not allowed.
> >>    *
> >> - * Return: 0 if the APQNs are not shared; otherwise returns 
> >> -EADDRINUSE.
> >> + * Return: 0 if the APQNs are not shared; otherwise return -EADDRINUSE.
> >>    */
> >> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev 
> >> *matrix_mdev)
> >> +static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
> >> +                      unsigned long *mdev_aqm)
> >>   {
> >> -    struct ap_matrix_mdev *lstdev;
> >> +    struct ap_matrix_mdev *matrix_mdev;
> >>       DECLARE_BITMAP(apm, AP_DEVICES);
> >>       DECLARE_BITMAP(aqm, AP_DOMAINS);
> >>   -    list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> >> -        if (matrix_mdev == lstdev)
> >> +    list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> >> +        /*
> >> +         * If the input apm and aqm belong to the matrix_mdev's matrix,

How about:

s/belong to the matrix_mdev's matrix/are fields of the matrix_mdev
object/


> >> +         * then move on to the next.
> >> +         */
> >> +        if (mdev_apm == matrix_mdev->matrix.apm &&
> >> +            mdev_aqm == matrix_mdev->matrix.aqm)
> >>               continue;  
> >
> > We may have a problem here. This check seems like it exists to stop 
> > you from
> > comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's 
> > newly
> > updated apm/aqm with itself would cause a false positive sharing 
> > check, right?
> > If this is the case, I think the comment should be changed to reflect 
> > that.  
> 
> You are correct, this check is performed to prevent comparing an mdev to
> itself, I'll improve the comment.
> 
> >
> > Aside from the comment, what stops this particular series of if 
> > statements from
> > allowing us to configure a second mdev with the exact same apm/aqm 
> > values as an
> > existing mdev? If we do, then this check's continue will short circuit 
> > the rest
> > of the function thereby allowing that 2nd mdev even though it should be a
> > sharing violation.  
> 
> I don't see how this is possible.

I agree with Tony and his explanation.

Furthermore IMHO is relates to the class identity vs equality problem, in
a sense that identity always implies equality.

Regards,
Halil