Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <df5fe3d295711666bf170d35f5196fe7b880342b.camel@linux.ibm.com>
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Paolo Abeni <pabeni@redhat.com>,
        Alexandra Winter <wintera@linux.ibm.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        "David S. Miller" <davem@davemloft.net>,
        Stefan Raspl <raspl@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>, linux-s390@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 10 Jul 2023 09:28:20 +0200
In-Reply-To: <ZKlmeDUEZf7F8+HW@corigine.com>
References: <20230707104359.3324039-1-schnelle@linux.ibm.com>
         <20230707104359.3324039-2-schnelle@linux.ibm.com>
         <ZKlmeDUEZf7F8+HW@corigine.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Sat, 2023-07-08 at 14:36 +0100, Simon Horman wrote:
> On Fri, Jul 07, 2023 at 12:43:57PM +0200, Niklas Schnelle wrote:
> > The clients array references all registered clients and is protected by
> > the clients_lock. Besides its use as general list of clients the client=
s
> > array is accessed in ism_handle_irq() to forward ISM device events to
> > clients.
> >=20
> > While the clients_lock is taken in the IRQ handler when calling
> > handle_event() it is however incorrectly not held during the
> > client->handle_irq() call and for the preceding clients[] access leavin=
g
> > it unprotected against concurrent client (un-)registration.
> >=20
> > Furthermore the accesses to ism->sba_client_arr[] in ism_register_dmb()
> > and ism_unregister_dmb() are not protected by any lock. This is
> > especially problematic as the client ID from the ism->sba_client_arr[]
> > is not checked against NO_CLIENT and neither is the client pointer
> > checked.
> >=20
> > Instead of expanding the use of the clients_lock further add a separate
> > array in struct ism_dev which references clients subscribed to the
> > device's events and IRQs. This array is protected by ism->lock which is
> > already taken in ism_handle_irq() and can be taken outside the IRQ
> > handler when adding/removing subscribers or the accessing
> > ism->sba_client_arr[]. This also means that the clients_lock is no
> > longer taken in IRQ context.
> >=20
> > Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>=20
> ...
>=20
> > @@ -71,6 +80,7 @@ int ism_register_client(struct ism_client *client)
> >  		list_for_each_entry(ism, &ism_dev_list.list, list) {
> >  			ism->priv[i] =3D NULL;
> >  			client->add(ism);
> > +			ism_setup_forwarding(client, ism);
> >  		}
> >  	}
> >  	mutex_unlock(&ism_dev_list.mutex);
>=20
> ...
>=20
> > @@ -92,6 +102,9 @@ int ism_unregister_client(struct ism_client *client)
> >  		max_client--;
> >  	spin_unlock_irqrestore(&clients_lock, flags);
> >  	list_for_each_entry(ism, &ism_dev_list.list, list) {
> > +		spin_lock_irqsave(&ism->lock, flags);
>=20
> Hi Niklas,
>=20
> The lock is taken here.
>=20
> > +		/* Stop forwarding IRQs and events */
> > +		ism->subs[client->id] =3D NULL;
> >  		for (int i =3D 0; i < ISM_NR_DMBS; ++i) {
> >  			if (ism->sba_client_arr[i] =3D=3D client->id) {
> >  				pr_err("%s: attempt to unregister client '%s'"
> > @@ -101,6 +114,7 @@ int ism_unregister_client(struct ism_client *client=
)
> >  				goto out;
>=20
> But it does not appear to be released
> (by the call to spin_unlock_irqrestore() below)
> if goto out is called here.

Good catch. Yes I screwed this up while splitting the patch up. The
spin_unlock_irqrestore() is there after patch 3 but should have been
added in patch 1. As far as I can see all 3 patches have already been
applied, otherwise I'd send a v3. Thankfully even in the in between
state this error case can really onlt happen due to driver bugs so
maybe it's okay?

>=20
> >  			}
> >  		}
> > +		spin_unlock_irqrestore(&ism->lock, flags);
> >  	}
> >  out:
> >  	mutex_unlock(&ism_dev_list.mutex);