linux-kernel - Re: [RFC][PATCH -next] pstore: replace spin_lock with spin_trylock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110927173413.GJ5795@redhat.com>
Date:	Tue, 27 Sep 2011 13:34:13 -0400
From:	Don Zickus <dzickus@...hat.com>
To:	Seiji Aguchi <seiji.aguchi@....com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Vivek Goyal <vgoyal@...hat.com>,
	Matthew Garrett <mjg@...hat.com>,
	"tony.luck@...el.com" <tony.luck@...el.com>,
	"gong.chen@...el.com" <gong.chen@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Satoru Moriya <satoru.moriya@....com>
Subject: Re: [RFC][PATCH -next] pstore: replace spin_lock with
 spin_trylock_irqsave in panic path

On Tue, Sep 27, 2011 at 01:14:59PM -0400, Seiji Aguchi wrote:
> Hi,
> 
> [Problem]
> Currently, pstore takes spin_trylock(&psinfo->buf_lock) in NMI context.
> And it takes spin_lock(&psinfo->buf_lock) in other cases.
> 
> If there are some bugs in pstore and kernel panics, spin_lock(&psinfo->buf_lock) causes deadlock 
> and panic_notifier_chain will not work.

Ok, so I missed your 'return' first time through and originally had a
bunch of comments.  So I would suggest adding a comment explaining why we
are returning in that failure.

Personally, I am not sure we want to abort here at the pstore layer, it
should probably be aborted lower.  There isn't any reason why we can't
continue from a pstore perspective (we can just bust the spinlock).

>From an ERST perspective, the state machine might be screwed up, hence
aborting in that layer could make sense.  But I don't think I agree with
the 'return' statement.

So I am opposed to it for now.

Cheers,
Don


> 
> [Patch Description]
> For solving this problem, this patch replaces spin_lock with spin_trylock_irqsave in panic path.
> 
> Dead lock in panic path will not happen by applying this patch.
> 
>  Signed-off-by: Seiji Aguchi <seiji.aguchi@....com>
> 
> ---
>  fs/pstore/platform.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 0472924..9882892 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -97,12 +97,15 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>  	else
>  		why = "Unknown";
>  
> -	if (in_nmi()) {
> -		is_locked = spin_trylock(&psinfo->buf_lock);
> -		if (!is_locked)
> -			pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
> +	if (reason == KMSG_DUMP_PANIC) {
> +		is_locked = spin_trylock_irqsave(&psinfo->buf_lock, flags);
> +		if (!is_locked) {
> +			pr_err("pstore dump routine skipped in panic path\n");
> +			return;
> +		}
>  	} else
>  		spin_lock_irqsave(&psinfo->buf_lock, flags);
> +
>  	oopscount++;
>  	while (total < kmsg_bytes) {
>  		dst = psinfo->buf;
> @@ -131,11 +134,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>  		total += l1_cpy + l2_cpy;
>  		part++;
>  	}
> -	if (in_nmi()) {
> -		if (is_locked)
> -			spin_unlock(&psinfo->buf_lock);
> -	} else
> -		spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> +	spin_unlock_irqrestore(&psinfo->buf_lock, flags);
>  }
>  
>  static struct kmsg_dumper pstore_dumper = {
> --
> 1.7.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/