Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 22:07:43 -0400
From:   Daniel Jordan <daniel.m.jordan@oracle.com>
To:     Steffen Klassert <steffen.klassert@secunet.com>
Cc:     eadavis@sina.com, linux-crypto@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        syzbot+bc05445bc14148d51915@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] padata: fix lockdep warning in padata serialization
Message-ID: <20220923020743.qnoe62uih5k4v72j@oracle.com>
References: <20220919151248.smfo7nq6yoqzy2vo@oracle.com>
 <20220920003908.391835-1-eadavis@sina.com>
 <20220920014711.bvreurf4ex44w6oj@oracle.com>
 <20220920055443.GI2950045@gauss3.secunet.de>
 <20220920141057.cy54d5ukflrgay3a@oracle.com>
 <20220921073616.GZ2950045@gauss3.secunet.de>
 <20220921185138.c6chlv77ugfrsukj@oracle.com>
 <20220922105537.GI2950045@gauss3.secunet.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220922105537.GI2950045@gauss3.secunet.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yIv38eL8QAnBTCgXze84aJKxZq6Td2llc3wVWXlvjVRHpwO+drKTas6IVdeK?=
 =?us-ascii?Q?rkvnZYoE7nr8FP/NaNzfyI025S5UhqTMt9ztvhOK5yVgD44jzkxg7N182xiM?=
 =?us-ascii?Q?yAFcbQPGIbtZPd1YwaNuW2pR41L5HjAutrxotdE5/lboeuFQ5agLJKhrB6rT?=
 =?us-ascii?Q?Io9uxPxzxwz068UIZmbDflS2a7k52k7YeCbTV5qadetuM8D4/wuO0k7G0bGV?=
 =?us-ascii?Q?hGg1ozW++cnW2YGC+Y0PZ/bujtWr300e1Hjza3pjtNcR12u6GBE5O3c+MLfe?=
 =?us-ascii?Q?T9ox9eTba1U9J7MI6say1JJUTsM9NG6jzL3wMeKC50pD80eqy6Gh1S+7sbYh?=
 =?us-ascii?Q?w2nSUSJf+oFs+sE30eN3IuIL8evzqVlY+gWDuSfDh8Yk38jHluMnUx9Jm9hP?=
 =?us-ascii?Q?P7yz6VEr5SlbgVsCXv1Gqm/LALCiFTcUV0VKs3sElzFVxKj8obWse1KFkueA?=
 =?us-ascii?Q?sTVFrcSMAAm3YswmGMSdRP/sbSqiRHUpBuh8jcexi4RHi1+NzsNW8cMUB6Sk?=
 =?us-ascii?Q?dZyc8rN8s/nBY5pGGBZ5fKZNqM6+2xNZkWiP9sdGaOA7pCzfnq2QsqWfpn/E?=
 =?us-ascii?Q?AKvH21ad2t/j8KgTQ9EQdrWLWpg2XFS2SxIbWq0m5mRiJDwwspduW4VnzGv4?=
 =?us-ascii?Q?vDCfzz1J9ldta8xLfPcpXvIlG1ig6fsgC27z79/+FFiYMcvRYj07QKqsnPaL?=
 =?us-ascii?Q?RomRocrQPIgNhENQ9zok8t7UvmLoAlWs8opgloBSBIicfLr4MU54fIZFngzU?=
 =?us-ascii?Q?j5Bd3SEzX+Bky6N46APv8/5K6AUvJehL/Liq8kWXL5rmDmJtT9W0mP/cbU8a?=
 =?us-ascii?Q?xmk7Jl748D1fGVv1BwQJ4Ogmnurx28YnpKnQ5h4mji0urJByPXvNIz+NmrTU?=
 =?us-ascii?Q?PwiHR4cbTKSFFFbRuYrMN3JkId863SEj2tCftufLSrqM4q2aweV/REoJYIbb?=
 =?us-ascii?Q?AuZm3Osj9TJMMsRepWQeB+wvvtrReNQYS4CJ7yDgoaYKKYbqQB85KVxObs3t?=
 =?us-ascii?Q?wYIbfsQ6s/qcNyed3oLCum9cJRSAdTd6XSPXY1bbTffVJF10l2J0L0uLPvGC?=
 =?us-ascii?Q?mDFepK/f0fSgZv0AjPY9CYiPfUqsYXuxtb8ATkEz9sbvao+sIH6mPmdOJyfq?=
 =?us-ascii?Q?9sj9czvrmoMMD0Eq9BcQYMT56RuV3Q+q7C9cn5jhx5M13VxILe2uEO5cL80C?=
 =?us-ascii?Q?VKVTJ5/roo1UXChPO+t3BE2Db5ScJcqI5WpfaaH3VyutWCIIPIyynfPdkHm6?=
 =?us-ascii?Q?0pKXy/Jxh6A+jRT50Oq3kQtVMqvCZoWjb7TnPANZY0lrpj6UCXI2pywa8w0K?=
 =?us-ascii?Q?4G9hNdJu4KCJyu3ZadIQENqyIIIJwXSKWsrufmj7lFGL8KRmn1xjHyj2Zla3?=
 =?us-ascii?Q?teQ+oAYf3HtMD9Yk//nHSAnDZf6kx6k7wc1q5zwdERLF9lRYABT4Fuh01ziW?=
 =?us-ascii?Q?7hkfG8zyf1hGl8UDM22Kv0vFUsgk4ktG8bcI7u/pRV0QnHVBMkB0g8tpb3lB?=
 =?us-ascii?Q?oq0gRlb3Ig7oQZoghhY853vUSOvV614HXucYgGzWT5bT7dx8Q2u5yd+EuLgO?=
 =?us-ascii?Q?hXayeasS6W3YKYSKZfQhEMw1W1v6NRdv7yeub0EDQNG9wYTVaK+MVOMKyP47?=
 =?us-ascii?Q?yg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f2c28e6-3b25-4d38-058b-08da9d0868a5
X-MS-Exchange-CrossTenant-AuthSource: SA1PR10MB5711.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 02:07:46.9478
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yeoOqPZ/FEhFpx7bgsOGJCH5KS+8sNlDqz5GQx0RXsGoTtPbA5VISPQU24k9sELZx9gKdLur0doTF1ngAkhv+OVMGCx9aJQSnxF8rqytSwU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR10MB5623
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-22_16,2022-09-22_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=848
 spamscore=0 adultscore=0 mlxscore=0 malwarescore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209230011
X-Proofpoint-GUID: Di2od25YiBwU4ezPglMHqaH77IKRZBXP
X-Proofpoint-ORIG-GUID: Di2od25YiBwU4ezPglMHqaH77IKRZBXP
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On Thu, Sep 22, 2022 at 12:55:37PM +0200, Steffen Klassert wrote:
> On Wed, Sep 21, 2022 at 02:51:38PM -0400, Daniel Jordan wrote:
> > On Wed, Sep 21, 2022 at 09:36:16AM +0200, Steffen Klassert wrote:
> > > On Tue, Sep 20, 2022 at 10:10:57AM -0400, Daniel Jordan wrote:
> > > > Yeah, padata_do_serial can be called with BHs off, like in the tipc
> > > > stack, but there are also cases where BHs can be on, like lockdep said
> > > > here:
> > > 
> > > padata_do_serial was designed to run with BHs off, it is a bug if it
> > > runs with BHs on. But I don't see a case where this can happen. The
> > > only user of padata_do_serial is pcrypt in its serialization callbacks
> > > (pcrypt_aead_enc, pcrypt_aead_dec) and the async crypto callback
> > > pcrypt_aead_done. pcrypt_aead_enc and pcrypt_aead_dec are issued via
> > > the padata_serial_worker with the padata->serial call. BHs are
> > > off here. The crypto callback also runs with BHs off.
> > > 
> > > What do I miss here?
> > 
> > Ugh.. this newer, buggy part of padata_do_parallel:
> > 
> >   /* Maximum works limit exceeded, run in the current task. */
> >   padata->parallel(padata);
> 
> Oh well...
> 
> > This skips the usual path in padata_parallel_worker, which disables BHs.
> > They should be left off in the above case too.
> > 
> > What about this?
> > 
> > ---8<---
> > 
> > Subject: [PATCH] padata: always leave BHs disabled when running ->parallel()
> > 
> > A deadlock can happen when an overloaded system runs ->parallel() in the
> > context of the current task:
> > 
> >     padata_do_parallel
> >       ->parallel()
> >         pcrypt_aead_enc/dec
> >           padata_do_serial
> >             spin_lock(&reorder->lock) // BHs still enabled
> >               <interrupt>
> >                 ...
> >                   __do_softirq
> >                     ...
> >                       padata_do_serial
> >                         spin_lock(&reorder->lock)
> > 
> > It's a bug for BHs to be on in _do_serial as Steffen points out, so
> > ensure they're off in the "current task" case like they are in
> > padata_parallel_worker to avoid this situation.
> > 
> > Reported-by: syzbot+bc05445bc14148d51915@syzkaller.appspotmail.com
> > Fixes: 4611ce224688 ("padata: allocate work structures for parallel jobs from a pool")
> > Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> 
> Yes, that makes sense.
> 
> Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Thanks.

> But we also should look at the call to padata_find_next where BHs are
> on. padata_find_next takes the same lock as padata_do_serial, so this
> might be a candidate for a deadlock too.

Yeah, that seems broken, it's now on my list of things to fix.  Probably
worth staring at the rest of the locking for a bit too.