linux-kernel - Re: [PATCH 1/2] pstore: Enable compression on normal path (again)

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: <CAGXu5j+1dcKDGzohndvWvqV3kooru+JDH+UjkEO2Pi9kBUUjSA@mail.gmail.com>
Date:	Fri, 20 May 2016 16:03:01 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Anton Vorontsov <anton@...msg.org>,
	Colin Cross <ccross@...roid.com>,
	Tony Luck <tony.luck@...el.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] pstore: Enable compression on normal path (again)

On Wed, May 18, 2016 at 5:00 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> The commit f0e2efcfd2717 ("pstore: do not use message compression
> without lock") added a check to 'is_locked' to avoid breakage in
> concurrent accesses.  But it has a side-effect of disabling compression
> on normal path since 'is_locked' variable is not set.  As normal path
> always takes the lock, it should be initialized to 1.
>
> This also makes the unlock code a bit simpler.
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  fs/pstore/platform.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 588461bb2dd4..04a0164a2066 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -284,7 +284,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>         u64             id;
>         unsigned int    part = 1;
>         unsigned long   flags = 0;
> -       int             is_locked = 0;
> +       int             is_locked = 1;
>         int             ret;
>
>         why = get_reason_str(reason);
> @@ -350,10 +350,7 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>                 total += total_len;
>                 part++;
>         }
> -       if (pstore_cannot_block_path(reason)) {
> -               if (is_locked)
> -                       spin_unlock_irqrestore(&psinfo->buf_lock, flags);
> -       } else
> +       if (is_locked)
>                 spin_unlock_irqrestore(&psinfo->buf_lock, flags);
>  }

This makes sense, but I think I'd prefer that is_locked get set in the
"else", since that makes it a little easier to read, IMO. I'll adjust
it and add it to the tree.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security