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:   Tue, 20 Sep 2022 07:54:43 +0200
From:   Steffen Klassert <steffen.klassert@secunet.com>
To:     Daniel Jordan <daniel.m.jordan@oracle.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: <20220920055443.GI2950045@gauss3.secunet.de>
References: <20220919151248.smfo7nq6yoqzy2vo@oracle.com>
 <20220920003908.391835-1-eadavis@sina.com>
 <20220920014711.bvreurf4ex44w6oj@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220920014711.bvreurf4ex44w6oj@oracle.com>
Precedence: bulk

On Mon, Sep 19, 2022 at 09:47:11PM -0400, Daniel Jordan wrote:
> On Tue, Sep 20, 2022 at 08:39:08AM +0800, eadavis@sina.com wrote:
> > From: Edward Adam Davis <eadavis@sina.com>
> > 
> > On Mon, 19 Sep 2022 11:12:48 -0400, Daniel Jordan wrote:
> > > Hi Edward,
> > > 
> > > On Mon, Sep 19, 2022 at 09:05:55AM +0800, eadavis@sina.com wrote:
> > > > From: Edward Adam Davis <eadavis@sina.com>
> > > > 
> > > > Parallelized object serialization uses spin_unlock for unlocking a spin lock
> > > > that was previously locked with spin_lock.
> > > 
> > > There's nothing unusual about that, though?
> > > 
> > > > This caused the following lockdep warning about an inconsistent lock
> > > > state:
> > > > 
> > > >         inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
> > > 
> > > Neither HARDIRQ-ON-W nor IN-HARDIRQ-W appear in the syzbot report, did
> > > you mean SOFTIRQ-ON-W and IN-SOFTIRQ-W?
> > Yes, I want say: inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> > > 
> > > > We must use spin_lock_irqsave, because it is possible to trigger tipc 
> > > > from an irq handler.
> > > 
> > > A softirq handler, not a hardirq handler.  I'd suggest using
> > > spin_lock_bh() instead of _irqsave in your patch.
> > I think _irqsave better than _bh, it can save the irq context, but _bh not, 
> > and in tipc call trace contain SOFTIRQ-ON-W and IN-SOFTIRQ-W.
> 
> _irqsave saving the context is about handling nested hardirq disables.
> It's not needed here since we don't need to care about disabling
> hardirq.
> 
> _bh is for disabling softirq, a different context from hardirq.  We want
> _bh here since the deadlock happens when a CPU takes the lock in both
> task and softirq context.  padata uses _bh lock variants because it can
> be called in softirq context but not hardirq.  Let's be consistent and
> do it in this case too.

padata_do_serial is called with BHs off, so using spin_lock_bh should not
fix anything here. I guess the problem is that we call padata_find_next
after we enabled the BHs in padata_reorder.