linux-kernel - Re: [PATCH] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET

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: <20210715133136.420c40b0.pasic@linux.ibm.com>
Date:   Thu, 15 Jul 2021 13:31:36 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Tony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, borntraeger@...ibm.com,
        cohuck@...hat.com, pasic@...ux.vnet.ibm.com, jjherne@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        frankja@...ux.ibm.com, david@...hat.com, imbrenda@...ux.ibm.com,
        hca@...ux.ibm.com
Subject: Re: [PATCH] s390/vfio-ap: do not open code locks for
 VFIO_GROUP_NOTIFY_SET_KVM notification

On Tue, 13 Jul 2021 14:05:33 -0300
Jason Gunthorpe <jgg@...dia.com> wrote:

> On Tue, Jul 13, 2021 at 06:45:17PM +0200, Halil Pasic wrote:
> 
> > Jason may give it another try to convince us that 0cc00c8d4050 only
> > silenced lockdep, but vfio_ap remained prone to deadlocks. To my best
> > knowledge using condition variable and a mutex is one of the well known
> > ways to implement an rwlock.   
> 
> The well known pattern is to use a rwsem.

I think you are missing the point. We are discussing whether
this qualifies for stable, i.e. if 0cc00c8d4050 is really broken
like the patch description says.

Using  a readers-writers lock (as a primitive) to implement a
a readers-writers lock is a fallacy, so I guess you wanted to
say that when a readers-writers lock is needed in the kernel the
obvious choices are rw_semaphore and/or rwlock_t (depending on the
spin).

What I wanted to say is using a condition variable and a mutex is
not per-see wrong, because one can even implement an readers-writers
lock with it. For reference see https://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock

> 
> This:
>         wait_event_cmd(matrix_mdev->wait_for_kvm,
>                        !matrix_mdev->kvm_busy,
>                        mutex_unlock(&matrix_dev->lock),
>                        mutex_lock(&matrix_dev->lock));
> 
> 
> Is not really a rwsem, and is invsible to lockdep.
> 

I agree. But this is not a proof of a problem that qualifies to be fixed
using the stable process as documented in 
https://github.com/torvalds/linux/blob/master/Documentation/process/stable-kernel-rules.rst

I'm in favor of rewriting this to use rw_semaphore. I'm not in favor
of proclaiming this a fix for stable, because for that you first have
to prove that you fix a real problem.

I hope we are on the same page.

Regards,
Halil