pstore: Refactor compression initialization

PATCH Superseded Kees Cook 4 participants 7 messages Oct 17, 2018 – Oct 18, 2018 Pstore filesystem

Share & tools

Kees Cook

author

2018-10-17 21:41 UTC #0 lore raw

PATCH 1/1 pstore: Refactor compression initialization

With compression initialization now separated from pstore_register(), there is no longer a good reason to do compression method selection during fs init. Instead, merge everything together into the late init. Additionally cleans up the reporting to be more clear.

Signed-off-by Kees Cook <[email protected]>

---
This patch goes on top of Joel's. I'll have both in linux-next shortly...

---
 fs/pstore/inode.c    |  2 --
 fs/pstore/internal.h |  3 --
 fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
 3 files changed, 41 insertions(+), 29 deletions(-)

fs/pstore/inode.c +0 -2

index 5fcb845b9fec..d814723fb27d 100644
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
 {
 	int err;
 
-	pstore_choose_compression();
-
 	/* Create a convenient mount point for people to access pstore */
 	err = sysfs_create_mount_point(fs_kobj, "pstore");
 	if (err)

fs/pstore/internal.h +0 -3

index fb767e28aeb2..c029314478fa 100644
@@ -37,7 +37,4 @@ extern bool	pstore_is_mounted(void);
 extern void	pstore_record_init(struct pstore_record *record,
 				   struct pstore_info *psi);
 
-/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
 #endif

fs/pstore/platform.c +41 ~~-24~~

108 lines not on this page · raw message

Signature · 6 lines

2.17.1


-- 
Kees Cook
Pixel Security

Joel Fernandes

Reviewed-by on v1 1/1

2018-10-18 01:30 UTC #1 lore raw

Quoted 139 lines · Kees Cook

Signed-off-by Kees Cook <[email protected]>

---
This patch goes on top of Joel's. I'll have both in linux-next shortly...

---
 fs/pstore/inode.c    |  2 --
 fs/pstore/internal.h |  3 --
 fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
 3 files changed, 41 insertions(+), 29 deletions(-)

fs/pstore/inode.c +0 -2

index 5fcb845b9fec..d814723fb27d 100644
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
 {
 	int err;
 
-	pstore_choose_compression();
-
 	/* Create a convenient mount point for people to access pstore */
 	err = sysfs_create_mount_point(fs_kobj, "pstore");
 	if (err)

fs/pstore/internal.h +0 -3

index fb767e28aeb2..c029314478fa 100644
@@ -37,7 +37,4 @@ extern bool	pstore_is_mounted(void);
 extern void	pstore_record_init(struct pstore_record *record,
 				   struct pstore_info *psi);
 
-/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
 #endif

fs/pstore/platform.c +36 ~~-19~~

index f09066db2d4d..e4737a5fa68c 100644
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
 
 static void allocate_buf_for_compression(void)
 {
+	struct crypto_comp *ctx;
+	int size;
+	char *buf;
+
+	/* Skip if not built-in or compression backend not selected yet. */
 	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
 		return;
 
+	/* Skip if no pstore backend yet or compression init already done. */
+	if (!psinfo || tfm)
+		return;
+
 	if (!crypto_has_comp(zbackend->name, 0, 0)) {
-		pr_err("No %s compression\n", zbackend->name);
+		pr_err("Unknown compression: %s\n", zbackend->name);
 		return;
 	}
 
-	big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
-	if (big_oops_buf_sz <= 0)
+	size = zbackend->zbufsize(psinfo->bufsize);
+	if (size <= 0) {
+		pr_err("Invalid compression size for %s: %d\n",
+		       zbackend->name, size);
 		return;
+	}
 
-	big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-	if (!big_oops_buf) {
-		pr_err("allocate compression buffer error!\n");
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		pr_err("Failed %d byte compression buffer allocation for: %s\n",
+		       size, zbackend->name);
 		return;
 	}
 
-	tfm = crypto_alloc_comp(zbackend->name, 0, 0);
-	if (IS_ERR_OR_NULL(tfm)) {
-		kfree(big_oops_buf);
-		big_oops_buf = NULL;
-		pr_err("crypto_alloc_comp() failed!\n");
+	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+	if (IS_ERR_OR_NULL(ctx)) {
+		kfree(buf);
+		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+		       PTR_ERR(ctx));
 		return;
 	}
+
+	/* A non-NULL big_oops_buf indicates compression is available. */
+	tfm = ctx;
+	big_oops_buf_sz = size;
+	big_oops_buf = buf;
+
+	pr_info("Using compression: %s\n", zbackend->name);
 }
 
 static void free_buf_for_compression(void)
 {
-	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
 		crypto_free_comp(tfm);
 	kfree(big_oops_buf);
 	big_oops_buf = NULL;

@@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list *unused)
 			  jiffies + msecs_to_jiffies(pstore_update_ms));
 }
 
-void __init pstore_choose_compression(void)
+static int __init pstore_compression_late_init(void)
 {
 	const struct pstore_zbackend *step;
 
 	if (!compress)
-		return;
+		return 0;
 
+	/* Locate desired compression method. */
 	for (step = zbackends; step->name; step++) {
 		if (!strcmp(compress, step->name)) {
 			zbackend = step;
-			pr_info("using %s compression\n", zbackend->name);
-			return;
+			break;
 		}
 	}
-}
 
-static int __init pstore_compression_late_init(void)
-{
 	/*

fs/pstore/platform.c static void pstore_timefunc(struct timer_list *unused) · line 807

Quoted 10 lines

fs/pstore/platform.c +4 -5

-	 * Check if any pstore backends registered earlier but did not allocate
-	 * for compression because crypto was not ready, if so then initialize
-	 * compression.
+	 * Check if any pstore backends registered earlier but did not
+	 * initialize compression because crypto was not ready. If so,
+	 * then initialize compression now.
 	 */
-	if (psinfo && !tfm)
-		allocate_buf_for_compression();
+	allocate_buf_for_compression();

We can also get rid of the 'zbackend' global variable since choosing the compression backend and allocating the buffers are done at the same time?

Otherwise looks good to me,

Reviewed-by Joel Fernandes (Google) <[email protected]>

thanks,

- Joel

Sai Prakash Ranjan

Tested-by

2018-10-18 04:42 UTC #2 lore raw

On 10/18/2018 7:00 AM, Joel Fernandes wrote:

Quoted 153 lines · Kees Cook

Quoted 139 lines

Signed-off-by Kees Cook <[email protected]>

---
This patch goes on top of Joel's. I'll have both in linux-next shortly...

---
  fs/pstore/inode.c    |  2 --
  fs/pstore/internal.h |  3 --
  fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
  3 files changed, 41 insertions(+), 29 deletions(-)

fs/pstore/inode.c +0 -2

index 5fcb845b9fec..d814723fb27d 100644
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
  {
  	int err;
 
-	pstore_choose_compression();
-
  	/* Create a convenient mount point for people to access pstore */
  	err = sysfs_create_mount_point(fs_kobj, "pstore");
  	if (err)

fs/pstore/internal.h +0 -3

index fb767e28aeb2..c029314478fa 100644
@@ -37,7 +37,4 @@ extern bool	pstore_is_mounted(void);
  extern void	pstore_record_init(struct pstore_record *record,
  				   struct pstore_info *psi);
 
-/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
  #endif

fs/pstore/platform.c +36 ~~-19~~

index f09066db2d4d..e4737a5fa68c 100644
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
 
  static void allocate_buf_for_compression(void)
  {
+	struct crypto_comp *ctx;
+	int size;
+	char *buf;
+
+	/* Skip if not built-in or compression backend not selected yet. */
  	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
  		return;
 
+	/* Skip if no pstore backend yet or compression init already done. */
+	if (!psinfo || tfm)
+		return;
+
  	if (!crypto_has_comp(zbackend->name, 0, 0)) {
-		pr_err("No %s compression\n", zbackend->name);
+		pr_err("Unknown compression: %s\n", zbackend->name);
  		return;
  	}
 
-	big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
-	if (big_oops_buf_sz <= 0)
+	size = zbackend->zbufsize(psinfo->bufsize);
+	if (size <= 0) {
+		pr_err("Invalid compression size for %s: %d\n",
+		       zbackend->name, size);
  		return;
+	}
 
-	big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-	if (!big_oops_buf) {
-		pr_err("allocate compression buffer error!\n");
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		pr_err("Failed %d byte compression buffer allocation for: %s\n",
+		       size, zbackend->name);
  		return;
  	}
 
-	tfm = crypto_alloc_comp(zbackend->name, 0, 0);
-	if (IS_ERR_OR_NULL(tfm)) {
-		kfree(big_oops_buf);
-		big_oops_buf = NULL;
-		pr_err("crypto_alloc_comp() failed!\n");
+	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+	if (IS_ERR_OR_NULL(ctx)) {
+		kfree(buf);
+		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+		       PTR_ERR(ctx));
  		return;
  	}
+
+	/* A non-NULL big_oops_buf indicates compression is available. */
+	tfm = ctx;
+	big_oops_buf_sz = size;
+	big_oops_buf = buf;
+
+	pr_info("Using compression: %s\n", zbackend->name);
  }
 
  static void free_buf_for_compression(void)
  {
-	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
+	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
  		crypto_free_comp(tfm);
  	kfree(big_oops_buf);
  	big_oops_buf = NULL;

@@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list *unused)
  			  jiffies + msecs_to_jiffies(pstore_update_ms));
  }
 
-void __init pstore_choose_compression(void)
+static int __init pstore_compression_late_init(void)
  {
  	const struct pstore_zbackend *step;
 
  	if (!compress)
-		return;
+		return 0;
 
+	/* Locate desired compression method. */
  	for (step = zbackends; step->name; step++) {
  		if (!strcmp(compress, step->name)) {
  			zbackend = step;
-			pr_info("using %s compression\n", zbackend->name);
-			return;
+			break;
  		}
  	}
-}
 
-static int __init pstore_compression_late_init(void)
-{
  	/*

fs/pstore/platform.c static void pstore_timefunc(struct timer_list *unused) · line 807

Quoted 10 lines

fs/pstore/platform.c +4 -5

-	 * Check if any pstore backends registered earlier but did not allocate
-	 * for compression because crypto was not ready, if so then initialize
-	 * compression.
+	 * Check if any pstore backends registered earlier but did not
+	 * initialize compression because crypto was not ready. If so,
+	 * then initialize compression now.
  	 */
-	if (psinfo && !tfm)
-		allocate_buf_for_compression();
+	allocate_buf_for_compression();

We can also get rid of the 'zbackend' global variable since choosing the compression backend and allocating the buffers are done at the same time?

Quoted 4 lines

Otherwise looks good to me,
Reviewed-by Joel Fernandes (Google) <[email protected]>

Tested this on top of Joel's patch, works fine on getting early crash pstore dmesg logs, thanks.

Tested-by Sai Prakash Ranjan <[email protected]>

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Kees Cook

author

2018-10-18 04:44 UTC #3 lore raw

Quoted 155 lines · Sai Prakash Ranjan

On 10/18/2018 7:00 AM, Joel Fernandes wrote:

Quoted 153 lines · Kees Cook

Quoted 138 lines

Signed-off-by Kees Cook <[email protected]>

---
This patch goes on top of Joel's. I'll have both in linux-next shortly...

---
  fs/pstore/inode.c    |  2 --
  fs/pstore/internal.h |  3 --
  fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
  3 files changed, 41 insertions(+), 29 deletions(-)

fs/pstore/inode.c +0 -1

index 5fcb845b9fec..d814723fb27d 100644
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
  {
        int err;
  -     pstore_choose_compression();
-
        /* Create a convenient mount point for people to access pstore */
        err = sysfs_create_mount_point(fs_kobj, "pstore");
        if (err)

fs/pstore/internal.h +0 -2

index fb767e28aeb2..c029314478fa 100644
@@ -37,7 +37,4 @@ extern bool   pstore_is_mounted(void);
  extern void   pstore_record_init(struct pstore_record *record,
                                   struct pstore_info *psi);
  -/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
  #endif

fs/pstore/platform.c +5 -0

index f09066db2d4d..e4737a5fa68c 100644
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
    static void allocate_buf_for_compression(void)
  {
+       struct crypto_comp *ctx;
+       int size;
+       char *buf;
+
+       /* Skip if not built-in or compression backend not selected yet.
 */
        if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
                return;
  +     /* Skip if no pstore backend yet or compression init already

done. */

fs/pstore/platform.c +12 -4

+       if (!psinfo || tfm)
+               return;
+
        if (!crypto_has_comp(zbackend->name, 0, 0)) {
-               pr_err("No %s compression\n", zbackend->name);
+               pr_err("Unknown compression: %s\n", zbackend->name);
                return;
        }
  -     big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
-       if (big_oops_buf_sz <= 0)
+       size = zbackend->zbufsize(psinfo->bufsize);
+       if (size <= 0) {
+               pr_err("Invalid compression size for %s: %d\n",
+                      zbackend->name, size);
                return;
+       }
  -     big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-       if (!big_oops_buf) {
-               pr_err("allocate compression buffer error!\n");
+       buf = kmalloc(size, GFP_KERNEL);
+       if (!buf) {
+               pr_err("Failed %d byte compression buffer allocation for:

%s\n",

fs/pstore/platform.c +5 -4

+                      size, zbackend->name);
                return;
        }
  -     tfm = crypto_alloc_comp(zbackend->name, 0, 0);
-       if (IS_ERR_OR_NULL(tfm)) {
-               kfree(big_oops_buf);
-               big_oops_buf = NULL;
-               pr_err("crypto_alloc_comp() failed!\n");
+       ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+       if (IS_ERR_OR_NULL(ctx)) {
+               kfree(buf);
+               pr_err("crypto_alloc_comp('%s') failed: %ld\n",

zbackend->name,

fs/pstore/platform.c +12 -7

+                      PTR_ERR(ctx));
                return;
        }
+
+       /* A non-NULL big_oops_buf indicates compression is available. */
+       tfm = ctx;
+       big_oops_buf_sz = size;
+       big_oops_buf = buf;
+
+       pr_info("Using compression: %s\n", zbackend->name);
  }
    static void free_buf_for_compression(void)
  {
-       if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
+       if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
                crypto_free_comp(tfm);
        kfree(big_oops_buf);
        big_oops_buf = NULL;

@@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list
 *unused)
                          jiffies + msecs_to_jiffies(pstore_update_ms));
  }
  -void __init pstore_choose_compression(void)
+static int __init pstore_compression_late_init(void)
  {
        const struct pstore_zbackend *step;
        if (!compress)
-               return;
+               return 0;
  +     /* Locate desired compression method. */
        for (step = zbackends; step->name; step++) {
                if (!strcmp(compress, step->name)) {
                        zbackend = step;
-                       pr_info("using %s compression\n",
 zbackend->name);
-                       return;
+                       break;
                }
        }
-}
  -static int __init pstore_compression_late_init(void)
-{
        /*
-        * Check if any pstore backends registered earlier but did not

allocate
- * for compression because crypto was not ready, if so then
initialize

fs/pstore/platform.c static void pstore_timefunc(struct timer_list *unused) · line 807

Quoted 8 lines

fs/pstore/platform.c +4 -3

-        * compression.
+        * Check if any pstore backends registered earlier but did not
+        * initialize compression because crypto was not ready. If so,
+        * then initialize compression now.
         */
-       if (psinfo && !tfm)
-               allocate_buf_for_compression();
+       allocate_buf_for_compression();

We can also get rid of the 'zbackend' global variable since choosing the compression backend and allocating the buffers are done at the same time?

Otherwise looks good to me,

Quoted 7 lines

Quoted 2 lines
Reviewed-by Joel Fernandes (Google) <[email protected]>
Tested this on top of Joel's patch, works fine on getting early crash pstore dmesg logs, thanks.
Tested-by Sai Prakash Ranjan <[email protected]>

Awesome! Thanks for testing it. :)

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

author

2018-10-18 04:44 UTC #4 lore raw

In reply to Joel Fernandes

Quoted 137 lines · Joel Fernandes

Quoted 136 lines · Kees Cook

Signed-off-by Kees Cook <[email protected]>

---
This patch goes on top of Joel's. I'll have both in linux-next shortly...

---
 fs/pstore/inode.c    |  2 --
 fs/pstore/internal.h |  3 --
 fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
 3 files changed, 41 insertions(+), 29 deletions(-)

fs/pstore/inode.c +0 -2

index 5fcb845b9fec..d814723fb27d 100644
@@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
 {
      int err;
 
-     pstore_choose_compression();
-
      /* Create a convenient mount point for people to access pstore */
      err = sysfs_create_mount_point(fs_kobj, "pstore");
      if (err)

fs/pstore/internal.h +0 -3

index fb767e28aeb2..c029314478fa 100644
@@ -37,7 +37,4 @@ extern bool pstore_is_mounted(void);
 extern void  pstore_record_init(struct pstore_record *record,
                                 struct pstore_info *psi);
 
-/* Called during module_init() */
-extern void __init pstore_choose_compression(void);
-
 #endif

fs/pstore/platform.c +36 ~~-17~~

index f09066db2d4d..e4737a5fa68c 100644
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
 
 static void allocate_buf_for_compression(void)
 {
+     struct crypto_comp *ctx;
+     int size;
+     char *buf;
+
+     /* Skip if not built-in or compression backend not selected yet. */
      if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
              return;
 
+     /* Skip if no pstore backend yet or compression init already done. */
+     if (!psinfo || tfm)
+             return;
+
      if (!crypto_has_comp(zbackend->name, 0, 0)) {
-             pr_err("No %s compression\n", zbackend->name);
+             pr_err("Unknown compression: %s\n", zbackend->name);
              return;
      }
 
-     big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
-     if (big_oops_buf_sz <= 0)
+     size = zbackend->zbufsize(psinfo->bufsize);
+     if (size <= 0) {
+             pr_err("Invalid compression size for %s: %d\n",
+                    zbackend->name, size);
              return;
+     }
 
-     big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-     if (!big_oops_buf) {
-             pr_err("allocate compression buffer error!\n");
+     buf = kmalloc(size, GFP_KERNEL);
+     if (!buf) {
+             pr_err("Failed %d byte compression buffer allocation for: %s\n",
+                    size, zbackend->name);
              return;
      }
 
-     tfm = crypto_alloc_comp(zbackend->name, 0, 0);
-     if (IS_ERR_OR_NULL(tfm)) {
-             kfree(big_oops_buf);
-             big_oops_buf = NULL;
-             pr_err("crypto_alloc_comp() failed!\n");
+     ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+     if (IS_ERR_OR_NULL(ctx)) {
+             kfree(buf);
+             pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+                    PTR_ERR(ctx));
              return;
      }
+
+     /* A non-NULL big_oops_buf indicates compression is available. */
+     tfm = ctx;
+     big_oops_buf_sz = size;
+     big_oops_buf = buf;
+
+     pr_info("Using compression: %s\n", zbackend->name);
 }
 
 static void free_buf_for_compression(void)
 {
-     if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
+     if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
              crypto_free_comp(tfm);
      kfree(big_oops_buf);
      big_oops_buf = NULL;

@@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list *unused)
                        jiffies + msecs_to_jiffies(pstore_update_ms));
 }
 
-void __init pstore_choose_compression(void)
+static int __init pstore_compression_late_init(void)
 {
      const struct pstore_zbackend *step;
 
      if (!compress)
-             return;
+             return 0;
 
+     /* Locate desired compression method. */
      for (step = zbackends; step->name; step++) {
              if (!strcmp(compress, step->name)) {
                      zbackend = step;
-                     pr_info("using %s compression\n", zbackend->name);
-                     return;
+                     break;
              }
      }
-}

Quoted 16 lines

fs/pstore/platform.c static void pstore_timefunc(struct timer_list *unused) · line 807

Quoted 13 lines

fs/pstore/platform.c +4 -7

-static int __init pstore_compression_late_init(void)
-{
      /*
-      * Check if any pstore backends registered earlier but did not allocate
-      * for compression because crypto was not ready, if so then initialize
-      * compression.
+      * Check if any pstore backends registered earlier but did not
+      * initialize compression because crypto was not ready. If so,
+      * then initialize compression now.
       */
-     if (psinfo && !tfm)
-             allocate_buf_for_compression();
+     allocate_buf_for_compression();

We can also get rid of the 'zbackend' global variable since choosing the compression backend and allocating the buffers are done at the same time?

That's my intention once I push the zbufsize functions up into the crypto subsystem (there's a separate series for this that I need to refresh).

Quoted 3 lines

Otherwise looks good to me,
Reviewed-by Joel Fernandes (Google) <[email protected]>

Thanks!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Joe Perches

2018-10-18 17:14 UTC #5 lore raw

In reply to Kees Cook

Quoted 4 lines · Kees Cook

With compression initialization now separated from pstore_register(), there is no longer a good reason to do compression method selection during fs init. Instead, merge everything together into the late init. Additionally cleans up the reporting to be more clear.

[]

fs/pstore/platform.c

Quoted 1 line

fs/pstore/platform.c +0 -0

[]

fs/pstore/platform.c static int pstore_decompress(void *in, void *out,

Quoted 1 line

fs/pstore/platform.c +0 -0

@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,

[]

fs/pstore/platform.c static int pstore_decompress(void *in, void *out, · line 304

Quoted 7 lines

fs/pstore/platform.c +4 -3

-	big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-	if (!big_oops_buf) {
-		pr_err("allocate compression buffer error!\n");
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		pr_err("Failed %d byte compression buffer allocation for: %s\n",
+		       size, zbackend->name);

Given that there is a generic OOM message emitted on kmalloc failures, rather than expanding the unnecessary alloc failure message, how about just deleting it instead?

Kees Cook

author

2018-10-18 18:01 UTC #6 lore raw

Quoted 18 lines · Joe Perches

Quoted 4 lines · Kees Cook
With compression initialization now separated from pstore_register(), there is no longer a good reason to do compression method selection during fs init. Instead, merge everything together into the late init. Additionally cleans up the reporting to be more clear.
[]
fs/pstore/platform.c
Quoted 1 line
fs/pstore/platform.c +0 -0
[]
fs/pstore/platform.c static int pstore_decompress(void *in, void *out,
Quoted 1 line
fs/pstore/platform.c +0 -0
@@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
[]
fs/pstore/platform.c static int pstore_decompress(void *in, void *out, · line 304
Quoted 7 lines
fs/pstore/platform.c +4 -3
-     big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
-     if (!big_oops_buf) {
-             pr_err("allocate compression buffer error!\n");
+     buf = kmalloc(size, GFP_KERNEL);
+     if (!buf) {
+             pr_err("Failed %d byte compression buffer allocation for: %s\n",
+                    size, zbackend->name);

Quoted 3 lines

Given that there is a generic OOM message emitted on kmalloc failures, rather than expanding the unnecessary alloc failure message, how about just deleting it instead?

I prefer including more context (i.e. the desired compression method).

-Kees

Signature · 2 lines

Kees Cook
Pixel Security