[PATCH] pstore: Refactor compression initialization

[PATCH] pstore: Refactor compression initialization

[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.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(-)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 5fcb845b9fec..d814723fb27d 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -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)
diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
index fb767e28aeb2..c029314478fa 100644
--- a/fs/pstore/internal.h
+++ b/fs/pstore/internal.h
@@ -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
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index f09066db2d4d..e4737a5fa68c 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -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)
-{
 	/*
-	 * 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();
+
 	return 0;
 }
 late_initcall(pstore_compression_late_init);
-- 
2.17.1


-- 
Kees Cook
Pixel Security

Re: [PATCH] pstore: Refactor compression initialization

[Joel Fernandes]

On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
> 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 <keescook@chromium.org>
> ---
> 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(-)
> 
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 5fcb845b9fec..d814723fb27d 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -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)
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index fb767e28aeb2..c029314478fa 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -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
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index f09066db2d4d..e4737a5fa68c 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -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)
> -{
>  	/*
> -	 * 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) <joel@joelfernandes.org>

thanks,

 - Joel

Re: [PATCH] pstore: Refactor compression initialization

[Sai Prakash Ranjan]

On 10/18/2018 7:00 AM, Joel Fernandes wrote:
On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
>> 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 <keescook@chromium.org>
>> ---
>> 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(-)
>>
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index 5fcb845b9fec..d814723fb27d 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -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)
>> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
>> index fb767e28aeb2..c029314478fa 100644
>> --- a/fs/pstore/internal.h
>> +++ b/fs/pstore/internal.h
>> @@ -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
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index f09066db2d4d..e4737a5fa68c 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -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)
>> -{
>>   	/*
>> -	 * 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) <joel@joelfernandes.org>
> 

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

Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

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

Re: [PATCH] pstore: Refactor compression initialization

[Kees Cook]

On Wed, Oct 17, 2018 at 6:30 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
>> 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 <keescook@chromium.org>
>> ---
>> 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(-)
>>
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index 5fcb845b9fec..d814723fb27d 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -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)
>> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
>> index fb767e28aeb2..c029314478fa 100644
>> --- a/fs/pstore/internal.h
>> +++ b/fs/pstore/internal.h
>> @@ -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
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index f09066db2d4d..e4737a5fa68c 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -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)
>> -{
>>       /*
>> -      * 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).

> Otherwise looks good to me,
>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] pstore: Refactor compression initialization

[Kees Cook]

On Wed, Oct 17, 2018 at 9:42 PM, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
> On 10/18/2018 7:00 AM, Joel Fernandes wrote:
> On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
>>>
>>> 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 <keescook@chromium.org>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>>> index 5fcb845b9fec..d814723fb27d 100644
>>> --- a/fs/pstore/inode.c
>>> +++ b/fs/pstore/inode.c
>>> @@ -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)
>>> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
>>> index fb767e28aeb2..c029314478fa 100644
>>> --- a/fs/pstore/internal.h
>>> +++ b/fs/pstore/internal.h
>>> @@ -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
>>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>>> index f09066db2d4d..e4737a5fa68c 100644
>>> --- a/fs/pstore/platform.c
>>> +++ b/fs/pstore/platform.c
>>> @@ -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)
>>> -{
>>>         /*
>>> -        * 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) <joel@joelfernandes.org>
>>
>
> Tested this on top of Joel's patch, works fine on getting early crash pstore
> dmesg logs, thanks.
>
> Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

Awesome! Thanks for testing it. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] pstore: Refactor compression initialization

[Joe Perches]

On Wed, 2018-10-17 at 14:41 -0700, Kees Cook wrote:
> 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.
[]
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
[]
> @@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
[]
> -	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?

Re: [PATCH] pstore: Refactor compression initialization

[Kees Cook]

On Thu, Oct 18, 2018 at 10:14 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2018-10-17 at 14:41 -0700, Kees Cook wrote:
>> 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.
> []
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> []
>> @@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
> []
>> -     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?

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

-Kees

-- 
Kees Cook
Pixel Security