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;
Date:   Mon, 10 Jul 2023 08:45:21 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Niklas Schnelle <schnelle@linux.ibm.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
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients
Message-ID: <ZKu3EVOANmy4KaLZ@corigine.com>
References: <20230707104359.3324039-1-schnelle@linux.ibm.com>
 <20230707104359.3324039-2-schnelle@linux.ibm.com>
 <ZKlmeDUEZf7F8+HW@corigine.com>
 <df5fe3d295711666bf170d35f5196fe7b880342b.camel@linux.ibm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <df5fe3d295711666bf170d35f5196fe7b880342b.camel@linux.ibm.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Jn8Qp73JtLSrVOM/bZvt3TYKUgG5xHtu3Zf4sLmSYKV8xGsPvw9CxPOkm905?=
 =?us-ascii?Q?keOqEw6EmC8/vcpq9zUMxvtIOfmH8Fnk5vME8f/jZxb2kSUtE+92tPrALa7V?=
 =?us-ascii?Q?RsFRT5y+ui5zsDRfvblYbeweL0/6bWJLQLtOhljVOimgY67mWafJ0yZq5FSj?=
 =?us-ascii?Q?imU0oi89VrZWs+IUDFGbkvBIjutpiyKvApPWvQtV0Vp3CfRoUGtF99wbuw/M?=
 =?us-ascii?Q?9nUF2pwR4Ntp1YO8jh7VtxABLHU/L809zZU48ewzaiH7RpsXO/PgJJcRVmfF?=
 =?us-ascii?Q?bDbfuVvEyFELWLDlF7xDhXeuye4F3pUFNWnxZL2NVokWYbnBMiL8+gHKy2gq?=
 =?us-ascii?Q?+Gt/IXXeybXgG1XrOeVhgV2MM32G18h2kNnOXaD3PGmdwMLCZwfr4I7pHqB6?=
 =?us-ascii?Q?+lF/nl+zSFDX38Kt/G9VRQsq7tBxJvc4F3I0YFmBH92VXrjIBv0rFODHQ95N?=
 =?us-ascii?Q?sVe5AYH52PLd5RRTTHs5jX9W5/7e+4x512yC80CR6lJxbvVAgBqopL37fAly?=
 =?us-ascii?Q?gs882KpUSHvWPsyncM8hbsOKUIBpK16NSfMFg2dfD8YDCDur2oohPCJSFl0B?=
 =?us-ascii?Q?2hXJq3KeAeWImj5nRcMIK90v6SpmJc6JUYjQuNn78q6mYZNWddcEQP0BnJpM?=
 =?us-ascii?Q?rFAE/vUDDLwp7wQT/Kd0QsWKD+FG75vmg3ih96s/vLd9rvOOSIYRC9tMngtH?=
 =?us-ascii?Q?eKO72Dx3Mc+eoDRyZqeWZB1uulU9G21gZzw0omvSnhDo6lP9t10ZyWcwjH9A?=
 =?us-ascii?Q?nS7MvRUtGEupgmGxCsU5YV3LuzDghjJBk2oDZKWAFChudXWjSVO24S86F5Iw?=
 =?us-ascii?Q?EfI7BFqw7EnmaIb7Bfrk4TsDB+beyJkW6ucaBZenNRLq+cMPF8m/gPpX7sJQ?=
 =?us-ascii?Q?YdtNb2zi02UDWC1k5f3SRrHeauCzoNRZGsGfd6qla7YbWlb32bBuailSL5+8?=
 =?us-ascii?Q?6b2CrWFlqCEEBRmnMIxIFEccrcVDs1Gf+5P+8S39YhzWZGaUom5NUpI8EuyE?=
 =?us-ascii?Q?1xLEBgqFVR76KvyGy/pR1YNxojYme1ufNIVwrxKoKrZlrsmvSWJy/lbHO49e?=
 =?us-ascii?Q?1tkk7qPdWyPIiOckv2N7khVRI6089hbcQbuhWaRli3pKrKsOgnOuyI2k5Wo/?=
 =?us-ascii?Q?zf2GtPYMsFJ0jnzXWOAFCn1KdRzI9NgQcHmHUjhmQVhX2RUlRbaVMyi9z36l?=
 =?us-ascii?Q?IVq/V6EMYWfqcH4Mgyt9St0g6PREtv+pZDNla9PeCqin9GgY4Yz3CqmZeBWk?=
 =?us-ascii?Q?kaVzIzA1Ze7S/Iwb5EV9D2tw+3iUPCDcOYPrwP6Gg/rMcFk/9kvSh+qJYmBN?=
 =?us-ascii?Q?Pj4Tb5NNgM8deZHRYtq69hScwW5WbkHlNjQ4Las9ZeAhyWWEQ1EaG3fKt3kj?=
 =?us-ascii?Q?2VOUouzsKAlv86P0SvUIVm0ccTd3/JzREYQC3MZVPfBuOkG5vwj0RYqUAGPk?=
 =?us-ascii?Q?ZlPIUFshK87k2ngGM3oJJ96+Ya9WCE5+nuUzBMOiXxkgFH6YlHo+IW/DMoKb?=
 =?us-ascii?Q?RkIqaG39lH21iHq5TLkWbhV3WNFGtyy0PvjMA+0gbQ9TlkZ7feyKVVCLzTWz?=
 =?us-ascii?Q?1EAwDrL8nU/t6Ek2RwErArQyK+2DAT3HXfXuuav/fhXqEblO1lhi2rQfaml3?=
 =?us-ascii?Q?gQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f19ca85-8cfa-4a1e-0869-08db8119a1bf
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jul 2023 07:45:29.5078
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UrK3gwaqjrjo2fqmad1Gl9WANUEWhYhZiUPG4jjR+YHbYWXSL8uPkG99BThPtr+xU2/P3tI0u6WQWVDWpBXY+jQBByYX5DhMmveSZ1B5+XM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR13MB5063
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Jul 10, 2023 at 09:28:20AM +0200, Niklas Schnelle wrote:
> 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 clients
> > > array is accessed in ism_handle_irq() to forward ISM device events to
> > > clients.
> > > 
> > > 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 leaving
> > > it unprotected against concurrent client (un-)registration.
> > > 
> > > 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.
> > > 
> > > 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.
> > > 
> > > Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > 
> > ...
> > 
> > > @@ -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] = NULL;
> > >  			client->add(ism);
> > > +			ism_setup_forwarding(client, ism);
> > >  		}
> > >  	}
> > >  	mutex_unlock(&ism_dev_list.mutex);
> > 
> > ...
> > 
> > > @@ -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);
> > 
> > Hi Niklas,
> > 
> > The lock is taken here.
> > 
> > > +		/* Stop forwarding IRQs and events */
> > > +		ism->subs[client->id] = NULL;
> > >  		for (int i = 0; i < ISM_NR_DMBS; ++i) {
> > >  			if (ism->sba_client_arr[i] == client->id) {
> > >  				pr_err("%s: attempt to unregister client '%s'"
> > > @@ -101,6 +114,7 @@ int ism_unregister_client(struct ism_client *client)
> > >  				goto out;
> > 
> > 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?

Hi Niklas,

I also saw the patches have been accepted after I sent my previous email.
So, given that the problem is resolved by another patch in the series,
I think the situation is as good as it is going to get.