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

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

[Namhyung Kim]

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@kernel.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);
 }
 
-- 
2.8.0

[PATCH 2/2] pstore: Cleanup pstore_dump()

[Namhyung Kim]

The code is duplicate between compression is enabled or not.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 fs/pstore/platform.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 04a0164a2066..277730f6c462 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -304,19 +304,25 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 		int hsize;
 		int zipped_len = -1;
 		size_t len;
-		bool compressed;
+		bool compressed = false;
 		size_t total_len;
 
 		if (big_oops_buf && is_locked) {
 			dst = big_oops_buf;
-			hsize = sprintf(dst, "%s#%d Part%u\n", why,
-							oopscount, part);
-			size = big_oops_buf_sz - hsize;
+			size = big_oops_buf_sz;
+		} else {
+			dst = psinfo->buf;
+			size = psinfo->bufsize;
+		}
 
-			if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
-								size, &len))
-				break;
+		hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount, part);
+		size -= hsize;
+
+		if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
+					  size, &len))
+			break;
 
+		if (big_oops_buf && is_locked) {
 			zipped_len = pstore_compress(dst, psinfo->buf,
 						hsize + len, psinfo->bufsize);
 
@@ -324,21 +330,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
 				compressed = true;
 				total_len = zipped_len;
 			} else {
-				compressed = false;
 				total_len = copy_kmsg_to_buffer(hsize, len);
 			}
 		} else {
-			dst = psinfo->buf;
-			hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount,
-									part);
-			size = psinfo->bufsize - hsize;
-			dst += hsize;
-
-			if (!kmsg_dump_get_buffer(dumper, true, dst,
-								size, &len))
-				break;
-
-			compressed = false;
 			total_len = hsize + len;
 		}
 
-- 
2.8.0

Re: [PATCH 2/2] pstore: Cleanup pstore_dump()

[Kees Cook]

On Wed, May 18, 2016 at 5:00 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> The code is duplicate between compression is enabled or not.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  fs/pstore/platform.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 04a0164a2066..277730f6c462 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -304,19 +304,25 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>                 int hsize;
>                 int zipped_len = -1;
>                 size_t len;
> -               bool compressed;
> +               bool compressed = false;
>                 size_t total_len;
>
>                 if (big_oops_buf && is_locked) {
>                         dst = big_oops_buf;
> -                       hsize = sprintf(dst, "%s#%d Part%u\n", why,
> -                                                       oopscount, part);
> -                       size = big_oops_buf_sz - hsize;
> +                       size = big_oops_buf_sz;
> +               } else {
> +                       dst = psinfo->buf;
> +                       size = psinfo->bufsize;
> +               }
>
> -                       if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
> -                                                               size, &len))
> -                               break;
> +               hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount, part);
> +               size -= hsize;
> +
> +               if (!kmsg_dump_get_buffer(dumper, true, dst + hsize,
> +                                         size, &len))
> +                       break;
>
> +               if (big_oops_buf && is_locked) {
>                         zipped_len = pstore_compress(dst, psinfo->buf,
>                                                 hsize + len, psinfo->bufsize);
>
> @@ -324,21 +330,9 @@ static void pstore_dump(struct kmsg_dumper *dumper,
>                                 compressed = true;
>                                 total_len = zipped_len;
>                         } else {
> -                               compressed = false;
>                                 total_len = copy_kmsg_to_buffer(hsize, len);
>                         }
>                 } else {
> -                       dst = psinfo->buf;
> -                       hsize = sprintf(dst, "%s#%d Part%u\n", why, oopscount,
> -                                                                       part);
> -                       size = psinfo->bufsize - hsize;
> -                       dst += hsize;
> -
> -                       if (!kmsg_dump_get_buffer(dumper, true, dst,
> -                                                               size, &len))
> -                               break;
> -
> -                       compressed = false;
>                         total_len = hsize + len;
>                 }
>
> --
> 2.8.0
>

Nice cleanup, thanks! Applied.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

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

[Kees Cook]

On Wed, May 18, 2016 at 5:00 AM, Namhyung Kim <namhyung@kernel.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@kernel.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