Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754258AbdDDMyK (ORCPT ); Tue, 4 Apr 2017 08:54:10 -0400 Received: from mail-pg0-f54.google.com ([74.125.83.54]:33803 "EHLO mail-pg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753304AbdDDMyH (ORCPT ); Tue, 4 Apr 2017 08:54:07 -0400 MIME-Version: 1.0 X-Originating-IP: [195.54.192.103] In-Reply-To: <877f30v19q.fsf@concordia.ellerman.id.au> References: <1490882779-16066-1-git-send-email-kda@linux-powerpc.org> <877f30v19q.fsf@concordia.ellerman.id.au> From: Denis Kirjanov Date: Tue, 4 Apr 2017 15:54:01 +0300 Message-ID: Subject: Re: [PATCH] tty/hvc_console: fix console lock ordering with spinlock To: Michael Ellerman Cc: gregkh@linuxfoundation.org, jslaby@suse.com, linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org, "benh@kernel.crashing.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1728 Lines: 58 On 4/4/17, Michael Ellerman wrote: > Denis Kirjanov writes: > >> hvc_remove() takes a spin lock first then acquires the console >> semaphore. This situation can easily lead to a deadlock scenario >> where we call scheduler with spin lock held. > > Have you actually hit the deadlock? Because that code's been like that > for years and I've never received a bug report. Nope, I didn't. I've found the bug in the code while looking at the lockdep output > >> diff --git a/drivers/tty/hvc/hvc_console.c >> b/drivers/tty/hvc/hvc_console.c >> index b19ae36..a8d3991 100644 >> --- a/drivers/tty/hvc/hvc_console.c >> +++ b/drivers/tty/hvc/hvc_console.c >> @@ -920,17 +920,17 @@ int hvc_remove(struct hvc_struct *hp) >> >> tty = tty_port_tty_get(&hp->port); >> >> + console_lock(); >> spin_lock_irqsave(&hp->lock, flags); >> if (hp->index < MAX_NR_HVC_CONSOLES) { >> - console_lock(); >> vtermnos[hp->index] = -1; >> cons_ops[hp->index] = NULL; >> - console_unlock(); >> } >> >> /* Don't whack hp->irq because tty_hangup() will need to free the irq. >> */ >> >> spin_unlock_irqrestore(&hp->lock, flags); >> + console_unlock(); > > I get that you're trying to do the minimal change, but I don't think the > result is ideal. If this isn't a console hvc then we take both locks but > do nothing. > > So what about: > > if (hp->index < MAX_NR_HVC_CONSOLES) { > console_lock(); > spin_lock_irqsave(&hp->lock, flags); > vtermnos[hp->index] = -1; > cons_ops[hp->index] = NULL; > spin_unlock_irqrestore(&hp->lock, flags); > console_unlock(); > } Are you sure that we don't corrupt the hp->index between hvc_poll in interrupt context and hvc_remoev? > > cheers >