Re: [RFC][PATCH -next] pstore: replace spin_lock withspin_trylock_irqsave in panic path

[Don Zickus]

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@xxxxxxx>
> 
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/