[patch V2 0/8] rslib: Cleanup and VLA removal

[patch V2 0/8] rslib: Cleanup and VLA removal

[Thomas Gleixner]

Kees tried to get rid of the Variable Length Arrays in the Reed-Solomon
library by replacing them with fixed length arrays on stack. Though they
are rather large and Andrew did not fall in love with that solution.

This series addresses that in a different way by splitting the rs control
structure up, so that each invocation of rs_init() returns a new instance
in which the decoder buffers are allocated. The polynom tables which build
the base for the RS codecs are still shareable between the instances to
avoid large allocations and initializations of the same data over and over.

The usage sites have been audited and fixed up where necessary to
accomodate the decoder change which forbids parallel decoder invocation for
a particular rs control instance to prevent buffer corruption.

While at it the patch set tidies up the code and converts the related files
over to use SPDX license identifiers.

Changes since V1:

   Simplify error path in the diskonchip code and use the proper
   function to free the decoder.

As this spawns multiple subsystems it should either go through Andrews tree
or Kees can route it with his other hardening stuff.

Thanks,

        tglx

8<---------------
 drivers/md/dm-verity-fec.c        |    7 +
 drivers/mtd/nand/raw/cafe_nand.c  |    7 -
 drivers/mtd/nand/raw/diskonchip.c |   67 +++++-------
 include/linux/rslib.h             |   46 ++++----
 lib/reed_solomon/decode_rs.c      |   34 +++---
 lib/reed_solomon/encode_rs.c      |   15 --
 lib/reed_solomon/reed_solomon.c   |  203 ++++++++++++++++++++++----------------
 7 files changed, 205 insertions(+), 174 deletions(-)

[patch V2 1/8] rslib: Cleanup whitespace damage

[Thomas Gleixner]

[-- Attachment #1: rslib--Cleanup_whitespace_damage.patch --]
[-- Type: text/plain, Size: 4847 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

Instead of mixing the whitespace cleanup into functional changes, mop it up
first.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 include/linux/rslib.h           |   14 +++++++-------
 lib/reed_solomon/reed_solomon.c |   20 ++++++++++----------
 2 files changed, 17 insertions(+), 17 deletions(-)

--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -39,15 +39,15 @@
  * @list:	List entry for the rs control list
 */
 struct rs_control {
-	int 		mm;
-	int 		nn;
+	int		mm;
+	int		nn;
 	uint16_t	*alpha_to;
 	uint16_t	*index_of;
 	uint16_t	*genpoly;
-	int 		nroots;
-	int 		fcr;
-	int 		prim;
-	int 		iprim;
+	int		nroots;
+	int		fcr;
+	int		prim;
+	int		iprim;
 	int		gfpoly;
 	int		(*gffunc)(int);
 	int		users;
@@ -80,7 +80,7 @@ int decode_rs16(struct rs_control *rs, u
 struct rs_control *init_rs(int symsize, int gfpoly, int fcr, int prim,
 			   int nroots);
 struct rs_control *init_rs_non_canonical(int symsize, int (*func)(int),
-                                         int fcr, int prim, int nroots);
+					 int fcr, int prim, int nroots);
 
 /* Release a rs control structure */
 void free_rs(struct rs_control *rs);
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -64,7 +64,7 @@ static DEFINE_MUTEX(rslistlock);
  * en/decoding. Fill the arrays according to the given parameters.
  */
 static struct rs_control *rs_init(int symsize, int gfpoly, int (*gffunc)(int),
-                                  int fcr, int prim, int nroots)
+				  int fcr, int prim, int nroots)
 {
 	struct rs_control *rs;
 	int i, j, sr, root, iprim;
@@ -191,14 +191,14 @@ void free_rs(struct rs_control *rs)
  *  @gffunc:	pointer to function to generate the next field element,
  *		or the multiplicative identity element if given 0.  Used
  *		instead of gfpoly if gfpoly is 0
- *  @fcr:  	the first consecutive root of the rs code generator polynomial
+ *  @fcr:	the first consecutive root of the rs code generator polynomial
  *		in index form
  *  @prim:	primitive element to generate polynomial roots
  *  @nroots:	RS code generator polynomial degree (number of roots)
  */
 static struct rs_control *init_rs_internal(int symsize, int gfpoly,
-                                           int (*gffunc)(int), int fcr,
-                                           int prim, int nroots)
+					   int (*gffunc)(int), int fcr,
+					   int prim, int nroots)
 {
 	struct list_head	*tmp;
 	struct rs_control	*rs;
@@ -207,9 +207,9 @@ static struct rs_control *init_rs_intern
 	if (symsize < 1)
 		return NULL;
 	if (fcr < 0 || fcr >= (1<<symsize))
-    		return NULL;
+		return NULL;
 	if (prim <= 0 || prim >= (1<<symsize))
-    		return NULL;
+		return NULL;
 	if (nroots < 0 || nroots >= (1<<symsize))
 		return NULL;
 
@@ -252,13 +252,13 @@ static struct rs_control *init_rs_intern
  *  @gfpoly:	the extended Galois field generator polynomial coefficients,
  *		with the 0th coefficient in the low order bit. The polynomial
  *		must be primitive;
- *  @fcr:  	the first consecutive root of the rs code generator polynomial
+ *  @fcr:	the first consecutive root of the rs code generator polynomial
  *		in index form
  *  @prim:	primitive element to generate polynomial roots
  *  @nroots:	RS code generator polynomial degree (number of roots)
  */
 struct rs_control *init_rs(int symsize, int gfpoly, int fcr, int prim,
-                           int nroots)
+			   int nroots)
 {
 	return init_rs_internal(symsize, gfpoly, NULL, fcr, prim, nroots);
 }
@@ -271,13 +271,13 @@ struct rs_control *init_rs(int symsize,
  *  @gffunc:	pointer to function to generate the next field element,
  *		or the multiplicative identity element if given 0.  Used
  *		instead of gfpoly if gfpoly is 0
- *  @fcr:  	the first consecutive root of the rs code generator polynomial
+ *  @fcr:	the first consecutive root of the rs code generator polynomial
  *		in index form
  *  @prim:	primitive element to generate polynomial roots
  *  @nroots:	RS code generator polynomial degree (number of roots)
  */
 struct rs_control *init_rs_non_canonical(int symsize, int (*gffunc)(int),
-                                         int fcr, int prim, int nroots)
+					 int fcr, int prim, int nroots)
 {
 	return init_rs_internal(symsize, 0, gffunc, fcr, prim, nroots);
 }

[patch V2 2/8] rslib: Cleanup top level comments

[Thomas Gleixner]

[-- Attachment #1: rslib--Cleanup_top_level_comments.patch --]
[-- Type: text/plain, Size: 4053 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

File references and stale CVS ids are really not useful.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 include/linux/rslib.h           |    7 +------
 lib/reed_solomon/decode_rs.c    |   12 ++----------
 lib/reed_solomon/encode_rs.c    |   13 ++-----------
 lib/reed_solomon/reed_solomon.c |    9 +--------
 4 files changed, 6 insertions(+), 35 deletions(-)

--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -1,16 +1,11 @@
 /*
- * include/linux/rslib.h
- *
- * Overview:
- *   Generic Reed Solomon encoder / decoder library
+ * Generic Reed Solomon encoder / decoder library
  *
  * Copyright (C) 2004 Thomas Gleixner (tglx@linutronix.de)
  *
  * RS code lifted from reed solomon library written by Phil Karn
  * Copyright 2002 Phil Karn, KA9Q
  *
- * $Id: rslib.h,v 1.4 2005/11/07 11:14:52 gleixner Exp $
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -1,20 +1,12 @@
 /*
- * lib/reed_solomon/decode_rs.c
- *
- * Overview:
- *   Generic Reed Solomon encoder / decoder library
+ * Generic Reed Solomon encoder / decoder library
  *
  * Copyright 2002, Phil Karn, KA9Q
  * May be used under the terms of the GNU General Public License (GPL)
  *
  * Adaption to the kernel by Thomas Gleixner (tglx@linutronix.de)
  *
- * $Id: decode_rs.c,v 1.7 2005/11/07 11:14:59 gleixner Exp $
- *
- */
-
-/* Generic data width independent code which is included by the
- * wrappers.
+ * Generic data width independent code which is included by the wrappers.
  */
 {
 	int deg_lambda, el, deg_omega;
--- a/lib/reed_solomon/encode_rs.c
+++ b/lib/reed_solomon/encode_rs.c
@@ -1,21 +1,12 @@
 /*
- * lib/reed_solomon/encode_rs.c
- *
- * Overview:
- *   Generic Reed Solomon encoder / decoder library
+ * Generic Reed Solomon encoder / decoder library
  *
  * Copyright 2002, Phil Karn, KA9Q
  * May be used under the terms of the GNU General Public License (GPL)
  *
  * Adaption to the kernel by Thomas Gleixner (tglx@linutronix.de)
  *
- * $Id: encode_rs.c,v 1.5 2005/11/07 11:14:59 gleixner Exp $
- *
- */
-
-/* Generic data width independent code which is included by the
- * wrappers.
- * int encode_rsX (struct rs_control *rs, uintX_t *data, int len, uintY_t *par)
+ * Generic data width independent code which is included by the wrappers.
  */
 {
 	int i, j, pad;
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -1,16 +1,11 @@
 /*
- * lib/reed_solomon/reed_solomon.c
- *
- * Overview:
- *   Generic Reed Solomon encoder / decoder library
+ * Generic Reed Solomon encoder / decoder library
  *
  * Copyright (C) 2004 Thomas Gleixner (tglx@linutronix.de)
  *
  * Reed Solomon code lifted from reed solomon library written by Phil Karn
  * Copyright 2002 Phil Karn, KA9Q
  *
- * $Id: rslib.c,v 1.7 2005/11/07 11:14:59 gleixner Exp $
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -35,9 +30,7 @@
  * second stage, which does the decoding / error correction itself.
  * Many hw encoders provide a syndrome calculation over the received
  * data + syndrome and can call the second stage directly.
- *
  */
-
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/init.h>

[patch V2 3/8] rslib: Add SPDX identifiers

[Thomas Gleixner]

[-- Attachment #1: rslib--Add_SPDX_identifiers.patch --]
[-- Type: text/plain, Size: 2403 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

The Reed-Solomon library is based on code from Phil Karn who granted
permission to import it into the kernel under the GPL V2.

See commit 15b5423757a7 ("Shared Reed-Solomon ECC library") in the history
git tree at: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git

  ...
  The encoder/decoder code is lifted from the GPL'd userspace RS-library
  written by Phil Karn. I modified/wrapped it to provide the different
  functions which we need in the MTD/NAND code.
  ...    
  Signed-Off-By: Thomas Gleixner <tglx@linutronix.de>
  Signed-Off-By: David Woodhouse <dwmw2@infradead.org>
  "No objections at all. Just keep the authorship notices." -- Phil Karn

Add the proper SPDX identifiers according to
Documentation/process/license-rules.rst.


Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 include/linux/rslib.h           |    1 +
 lib/reed_solomon/decode_rs.c    |    1 +
 lib/reed_solomon/encode_rs.c    |    1 +
 lib/reed_solomon/reed_solomon.c |    1 +
 4 files changed, 4 insertions(+)

--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Generic Reed Solomon encoder / decoder library
  *
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Generic Reed Solomon encoder / decoder library
  *
--- a/lib/reed_solomon/encode_rs.c
+++ b/lib/reed_solomon/encode_rs.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Generic Reed Solomon encoder / decoder library
  *
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Generic Reed Solomon encoder / decoder library
  *

Re: [patch V2 3/8] rslib: Add SPDX identifiers

[Greg Kroah-Hartman]

On Thu, Apr 19, 2018 at 12:04:44PM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The Reed-Solomon library is based on code from Phil Karn who granted
> permission to import it into the kernel under the GPL V2.
> 
> See commit 15b5423757a7 ("Shared Reed-Solomon ECC library") in the history
> git tree at: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
>   ...
>   The encoder/decoder code is lifted from the GPL'd userspace RS-library
>   written by Phil Karn. I modified/wrapped it to provide the different
>   functions which we need in the MTD/NAND code.
>   ...    
>   Signed-Off-By: Thomas Gleixner <tglx@linutronix.de>
>   Signed-Off-By: David Woodhouse <dwmw2@infradead.org>
>   "No objections at all. Just keep the authorship notices." -- Phil Karn
> 
> Add the proper SPDX identifiers according to
> Documentation/process/license-rules.rst.
> 
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kate Stewart <kstewart@linuxfoundation.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [patch V2 3/8] rslib: Add SPDX identifiers

[Kate Stewart]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Thu, Apr 19, 2018 at 5:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
>
> The Reed-Solomon library is based on code from Phil Karn who granted
> permission to import it into the kernel under the GPL V2.
>
> See commit 15b5423757a7 ("Shared Reed-Solomon ECC library") in the history
> git tree at: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.
> git
>
>   ...
>   The encoder/decoder code is lifted from the GPL'd userspace RS-library
>   written by Phil Karn. I modified/wrapped it to provide the different
>   functions which we need in the MTD/NAND code.
>   ...
>   Signed-Off-By: Thomas Gleixner <tglx@linutronix.de>
>   Signed-Off-By: David Woodhouse <dwmw2@infradead.org>
>   "No objections at all. Just keep the authorship notices." -- Phil Karn
>
> Add the proper SPDX identifiers according to
> Documentation/process/license-rules.rst.
>
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>
>
>  Reviewed-by: Kate Stewart <kstewart@linuxfoundation.org>

[-- Attachment #2: Type: text/html, Size: 5055 bytes --]

[patch V2 4/8] rslib: Remove GPL boilerplate

[Thomas Gleixner]

[-- Attachment #1: rslib--Remove_GPL_boilerplate.patch --]
[-- Type: text/plain, Size: 1993 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

Now that SPDX identifiers are in place, remove the GPL boiler plate
text. Leave the notices which document that Phil Karn granted permission in
place (encode/decode source code). The modified files are code written for
the kernel by me.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 include/linux/rslib.h           |    5 -----
 lib/reed_solomon/reed_solomon.c |    4 ----
 2 files changed, 9 deletions(-)

--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -6,12 +6,7 @@
  *
  * RS code lifted from reed solomon library written by Phil Karn
  * Copyright 2002 Phil Karn, KA9Q
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
  */
-
 #ifndef _RSLIB_H_
 #define _RSLIB_H_
 
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -7,10 +7,6 @@
  * Reed Solomon code lifted from reed solomon library written by Phil Karn
  * Copyright 2002 Phil Karn, KA9Q
  *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
  * Description:
  *
  * The generic Reed Solomon library provides runtime configurable

Re: [patch V2 4/8] rslib: Remove GPL boilerplate

[Greg Kroah-Hartman]

On Thu, Apr 19, 2018 at 12:04:45PM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Now that SPDX identifiers are in place, remove the GPL boiler plate
> text. Leave the notices which document that Phil Karn granted permission in
> place (encode/decode source code). The modified files are code written for
> the kernel by me.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kate Stewart <kstewart@linuxfoundation.org>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [patch V2 4/8] rslib: Remove GPL boilerplate

[Kate Stewart]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On Thu, Apr 19, 2018 at 5:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
>
> Now that SPDX identifiers are in place, remove the GPL boiler plate
> text. Leave the notices which document that Phil Karn granted permission in
> place (encode/decode source code). The modified files are code written for
> the kernel by me.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>
>

 Reviewed-by: Kate Stewart <kstewart@linuxfoundation.org>

[-- Attachment #2: Type: text/html, Size: 3801 bytes --]

[patch V2 5/8] rslib: Split rs control struct

[Thomas Gleixner]

[-- Attachment #1: rslib--Split_rs_control_struct.patch --]
[-- Type: text/plain, Size: 16311 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

The decoder library uses variable length arrays on stack. To get rid of
them it would be simple to allocate fixed length arrays on stack, but those
might become rather large. The other solution is to allocate the buffers in
the rs control structure, but this cannot be done as long as the structure
can be shared by several users. Sharing is desired because the RS polynom
tables are large and initialization is time consuming.

To solve this split the codec information out of the control structure and
have a pointer to a shared codec in it. Instantiate the control structure
for each user, create a new codec if no shareable is avaiable yet.  Adjust
all affected usage sites to the new scheme.

This allows to add per instance decoder buffers to the control structure
later on.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 drivers/mtd/nand/raw/cafe_nand.c    |    7 +
 drivers/mtd/nand/raw/diskonchip.c   |    7 +
 include/linux/rslib.h           |   18 +++--
 lib/reed_solomon/decode_rs.c    |    1 
 lib/reed_solomon/encode_rs.c    |    1 
 lib/reed_solomon/reed_solomon.c |  144 ++++++++++++++++++++++------------------
 6 files changed, 104 insertions(+), 74 deletions(-)

--- a/drivers/mtd/nand/raw/cafe_nand.c
+++ b/drivers/mtd/nand/raw/cafe_nand.c
@@ -394,12 +394,13 @@ static int cafe_nand_read_page(struct mt
 
 		for (i=0; i<8; i+=2) {
 			uint32_t tmp = cafe_readl(cafe, NAND_ECC_SYN01 + (i*2));
-			syn[i] = cafe->rs->index_of[tmp & 0xfff];
-			syn[i+1] = cafe->rs->index_of[(tmp >> 16) & 0xfff];
+
+			syn[i] = cafe->rs->codec->index_of[tmp & 0xfff];
+			syn[i+1] = cafe->rs->codec->index_of[(tmp >> 16) & 0xfff];
 		}
 
 		n = decode_rs16(cafe->rs, NULL, NULL, 1367, syn, 0, pos, 0,
-		                pat);
+				pat);
 
 		for (i = 0; i < n; i++) {
 			int p = pos[i];
--- a/drivers/mtd/nand/raw/diskonchip.c
+++ b/drivers/mtd/nand/raw/diskonchip.c
@@ -142,6 +142,7 @@ static int doc_ecc_decode(struct rs_cont
 	int i, j, nerr, errpos[8];
 	uint8_t parity;
 	uint16_t ds[4], s[5], tmp, errval[8], syn[4];
+	struct rs_codec *cd = rs->codec;
 
 	memset(syn, 0, sizeof(syn));
 	/* Convert the ecc bytes into words */
@@ -162,15 +163,15 @@ static int doc_ecc_decode(struct rs_cont
 	for (j = 1; j < NROOTS; j++) {
 		if (ds[j] == 0)
 			continue;
-		tmp = rs->index_of[ds[j]];
+		tmp = cd->index_of[ds[j]];
 		for (i = 0; i < NROOTS; i++)
-			s[i] ^= rs->alpha_to[rs_modnn(rs, tmp + (FCR + i) * j)];
+			s[i] ^= cd->alpha_to[rs_modnn(cd, tmp + (FCR + i) * j)];
 	}
 
 	/* Calc syn[i] = s[i] / alpha^(v + i) */
 	for (i = 0; i < NROOTS; i++) {
 		if (s[i])
-			syn[i] = rs_modnn(rs, rs->index_of[s[i]] + (NN - FCR - i));
+			syn[i] = rs_modnn(cd, cd->index_of[s[i]] + (NN - FCR - i));
 	}
 	/* Call the decoder library */
 	nerr = decode_rs16(rs, NULL, NULL, 1019, syn, 0, errpos, 0, errval);
--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -13,7 +13,7 @@
 #include <linux/list.h>
 
 /**
- * struct rs_control - rs control structure
+ * struct rs_codec - rs codec data
  *
  * @mm:		Bits per symbol
  * @nn:		Symbols per block (= (1<<mm)-1)
@@ -27,9 +27,9 @@
  * @gfpoly:	The primitive generator polynominal
  * @gffunc:	Function to generate the field, if non-canonical representation
  * @users:	Users of this structure
- * @list:	List entry for the rs control list
+ * @list:	List entry for the rs codec list
 */
-struct rs_control {
+struct rs_codec {
 	int		mm;
 	int		nn;
 	uint16_t	*alpha_to;
@@ -45,6 +45,14 @@ struct rs_control {
 	struct list_head list;
 };
 
+/**
+ * struct rs_control - rs control structure per instance
+ * @codec:	The codec used for this instance
+ */
+struct rs_control {
+	struct rs_codec	*codec;
+};
+
 /* General purpose RS codec, 8-bit data width, symbol width 1-15 bit  */
 #ifdef CONFIG_REED_SOLOMON_ENC8
 int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
@@ -78,7 +86,7 @@ void free_rs(struct rs_control *rs);
 
 /** modulo replacement for galois field arithmetics
  *
- *  @rs:	the rs control structure
+ *  @rs:	Pointer to the RS codec
  *  @x:		the value to reduce
  *
  *  where
@@ -88,7 +96,7 @@ void free_rs(struct rs_control *rs);
  *  Simple arithmetic modulo would return a wrong result for values
  *  >= 3 * rs->nn
 */
-static inline int rs_modnn(struct rs_control *rs, int x)
+static inline int rs_modnn(struct rs_codec *rs, int x)
 {
 	while (x >= rs->nn) {
 		x -= rs->nn;
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -10,6 +10,7 @@
  * Generic data width independent code which is included by the wrappers.
  */
 {
+	struct rs_codec *rs = rsc->codec;
 	int deg_lambda, el, deg_omega;
 	int i, j, r, k, pad;
 	int nn = rs->nn;
--- a/lib/reed_solomon/encode_rs.c
+++ b/lib/reed_solomon/encode_rs.c
@@ -10,6 +10,7 @@
  * Generic data width independent code which is included by the wrappers.
  */
 {
+	struct rs_codec *rs = rsc->codec;
 	int i, j, pad;
 	int nn = rs->nn;
 	int nroots = rs->nroots;
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -11,22 +11,23 @@
  *
  * The generic Reed Solomon library provides runtime configurable
  * encoding / decoding of RS codes.
- * Each user must call init_rs to get a pointer to a rs_control
- * structure for the given rs parameters. This structure is either
- * generated or a already available matching control structure is used.
- * If a structure is generated then the polynomial arrays for
- * fast encoding / decoding are built. This can take some time so
- * make sure not to call this function from a time critical path.
- * Usually a module / driver should initialize the necessary
- * rs_control structure on module / driver init and release it
- * on exit.
- * The encoding puts the calculated syndrome into a given syndrome
- * buffer.
- * The decoding is a two step process. The first step calculates
- * the syndrome over the received (data + syndrome) and calls the
- * second stage, which does the decoding / error correction itself.
- * Many hw encoders provide a syndrome calculation over the received
- * data + syndrome and can call the second stage directly.
+ *
+ * Each user must call init_rs to get a pointer to a rs_control structure
+ * for the given rs parameters. The control struct is unique per instance.
+ * It points to a codec which can be shared by multiple control structures.
+ * If a codec is newly allocated then the polynomial arrays for fast
+ * encoding / decoding are built. This can take some time so make sure not
+ * to call this function from a time critical path.  Usually a module /
+ * driver should initialize the necessary rs_control structure on module /
+ * driver init and release it on exit.
+ *
+ * The encoding puts the calculated syndrome into a given syndrome buffer.
+ *
+ * The decoding is a two step process. The first step calculates the
+ * syndrome over the received (data + syndrome) and calls the second stage,
+ * which does the decoding / error correction itself.  Many hw encoders
+ * provide a syndrome calculation over the received data + syndrome and can
+ * call the second stage directly.
  */
 #include <linux/errno.h>
 #include <linux/kernel.h>
@@ -36,13 +37,13 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 
-/* This list holds all currently allocated rs control structures */
-static LIST_HEAD (rslist);
+/* This list holds all currently allocated rs codec structures */
+static LIST_HEAD(codec_list);
 /* Protection for the list */
 static DEFINE_MUTEX(rslistlock);
 
 /**
- * rs_init - Initialize a Reed-Solomon codec
+ * codec_init - Initialize a Reed-Solomon codec
  * @symsize:	symbol size, bits (1-8)
  * @gfpoly:	Field generator polynomial coefficients
  * @gffunc:	Field generator function
@@ -50,18 +51,17 @@ static DEFINE_MUTEX(rslistlock);
  * @prim:	primitive element to generate polynomial roots
  * @nroots:	RS code generator polynomial degree (number of roots)
  *
- * Allocate a control structure and the polynom arrays for faster
+ * Allocate a codec structure and the polynom arrays for faster
  * en/decoding. Fill the arrays according to the given parameters.
  */
-static struct rs_control *rs_init(int symsize, int gfpoly, int (*gffunc)(int),
-				  int fcr, int prim, int nroots)
+static struct rs_codec *codec_init(int symsize, int gfpoly, int (*gffunc)(int),
+				   int fcr, int prim, int nroots)
 {
-	struct rs_control *rs;
 	int i, j, sr, root, iprim;
+	struct rs_codec *rs;
 
-	/* Allocate the control structure */
-	rs = kmalloc(sizeof (struct rs_control), GFP_KERNEL);
-	if (rs == NULL)
+	rs = kmalloc(sizeof(*rs), GFP_KERNEL);
+	if (!rs)
 		return NULL;
 
 	INIT_LIST_HEAD(&rs->list);
@@ -138,6 +138,9 @@ static struct rs_control *rs_init(int sy
 	/* convert rs->genpoly[] to index form for quicker encoding */
 	for (i = 0; i <= nroots; i++)
 		rs->genpoly[i] = rs->index_of[rs->genpoly[i]];
+
+	rs->users = 1;
+	list_add(&rs->list, &codec_list);
 	return rs;
 
 	/* Error exit */
@@ -154,26 +157,36 @@ static struct rs_control *rs_init(int sy
 
 
 /**
- *  free_rs - Free the rs control structure, if it is no longer used
- *  @rs:	the control structure which is not longer used by the
+ *  free_rs - Free the rs control structure
+ *  @rs:	The control structure which is not longer used by the
  *		caller
+ *
+ * Free the control structure. If @rs is the last user of the associated
+ * codec, free the codec as well.
  */
 void free_rs(struct rs_control *rs)
 {
+	struct rs_codec *cd;
+
+	if (!rs)
+		return;
+
+	cd = rs->codec;
 	mutex_lock(&rslistlock);
-	rs->users--;
-	if(!rs->users) {
-		list_del(&rs->list);
-		kfree(rs->alpha_to);
-		kfree(rs->index_of);
-		kfree(rs->genpoly);
-		kfree(rs);
+	cd->users--;
+	if(!cd->users) {
+		list_del(&cd->list);
+		kfree(cd->alpha_to);
+		kfree(cd->index_of);
+		kfree(cd->genpoly);
+		kfree(cd);
 	}
 	mutex_unlock(&rslistlock);
+	kfree(rs);
 }
 
 /**
- * init_rs_internal - Find a matching or allocate a new rs control structure
+ * init_rs_internal - Allocate rs control, find a matching codec or allocate a new one
  *  @symsize:	the symbol size (number of bits)
  *  @gfpoly:	the extended Galois field generator polynomial coefficients,
  *		with the 0th coefficient in the low order bit. The polynomial
@@ -190,8 +203,8 @@ static struct rs_control *init_rs_intern
 					   int (*gffunc)(int), int fcr,
 					   int prim, int nroots)
 {
-	struct list_head	*tmp;
-	struct rs_control	*rs;
+	struct list_head *tmp;
+	struct rs_control *rs;
 
 	/* Sanity checks */
 	if (symsize < 1)
@@ -203,33 +216,39 @@ static struct rs_control *init_rs_intern
 	if (nroots < 0 || nroots >= (1<<symsize))
 		return NULL;
 
+	rs = kzalloc(sizeof(*rs), GFP_KERNEL);
+	if (!rs)
+		return NULL;
+
 	mutex_lock(&rslistlock);
 
 	/* Walk through the list and look for a matching entry */
-	list_for_each(tmp, &rslist) {
-		rs = list_entry(tmp, struct rs_control, list);
-		if (symsize != rs->mm)
+	list_for_each(tmp, &codec_list) {
+		struct rs_codec *cd = list_entry(tmp, struct rs_codec, list);
+
+		if (symsize != cd->mm)
 			continue;
-		if (gfpoly != rs->gfpoly)
+		if (gfpoly != cd->gfpoly)
 			continue;
-		if (gffunc != rs->gffunc)
+		if (gffunc != cd->gffunc)
 			continue;
-		if (fcr != rs->fcr)
+		if (fcr != cd->fcr)
 			continue;
-		if (prim != rs->prim)
+		if (prim != cd->prim)
 			continue;
-		if (nroots != rs->nroots)
+		if (nroots != cd->nroots)
 			continue;
 		/* We have a matching one already */
-		rs->users++;
+		cd->users++;
+		rs->codec = cd;
 		goto out;
 	}
 
 	/* Create a new one */
-	rs = rs_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
-	if (rs) {
-		rs->users = 1;
-		list_add(&rs->list, &rslist);
+	rs->codec = codec_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
+	if (!rs->codec) {
+		kfree(rs);
+		rs = NULL;
 	}
 out:
 	mutex_unlock(&rslistlock);
@@ -237,7 +256,7 @@ static struct rs_control *init_rs_intern
 }
 
 /**
- * init_rs - Find a matching or allocate a new rs control structure
+ * init_rs - Create a RS control struct and initialize it
  *  @symsize:	the symbol size (number of bits)
  *  @gfpoly:	the extended Galois field generator polynomial coefficients,
  *		with the 0th coefficient in the low order bit. The polynomial
@@ -254,9 +273,8 @@ struct rs_control *init_rs(int symsize,
 }
 
 /**
- * init_rs_non_canonical - Find a matching or allocate a new rs control
- *                         structure, for fields with non-canonical
- *                         representation
+ * init_rs_non_canonical - Allocate rs control struct for fields with
+ *                         non-canonical representation
  *  @symsize:	the symbol size (number of bits)
  *  @gffunc:	pointer to function to generate the next field element,
  *		or the multiplicative identity element if given 0.  Used
@@ -275,7 +293,7 @@ struct rs_control *init_rs_non_canonical
 #ifdef CONFIG_REED_SOLOMON_ENC8
 /**
  *  encode_rs8 - Calculate the parity for data values (8bit data width)
- *  @rs:	the rs control structure
+ *  @rsc:	the rs control structure
  *  @data:	data field of a given type
  *  @len:	data length
  *  @par:	parity data, must be initialized by caller (usually all 0)
@@ -285,7 +303,7 @@ struct rs_control *init_rs_non_canonical
  *  symbol size > 8. The calling code must take care of encoding of the
  *  syndrome result for storage itself.
  */
-int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
+int encode_rs8(struct rs_control *rsc, uint8_t *data, int len, uint16_t *par,
 	       uint16_t invmsk)
 {
 #include "encode_rs.c"
@@ -296,7 +314,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
 #ifdef CONFIG_REED_SOLOMON_DEC8
 /**
  *  decode_rs8 - Decode codeword (8bit data width)
- *  @rs:	the rs control structure
+ *  @rsc:	the rs control structure
  *  @data:	data field of a given type
  *  @par:	received parity data field
  *  @len:	data length
@@ -311,7 +329,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
  *  syndrome result and the received parity before calling this code.
  *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
-int decode_rs8(struct rs_control *rs, uint8_t *data, uint16_t *par, int len,
+int decode_rs8(struct rs_control *rsc, uint8_t *data, uint16_t *par, int len,
 	       uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
 	       uint16_t *corr)
 {
@@ -323,7 +341,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
 #ifdef CONFIG_REED_SOLOMON_ENC16
 /**
  *  encode_rs16 - Calculate the parity for data values (16bit data width)
- *  @rs:	the rs control structure
+ *  @rsc:	the rs control structure
  *  @data:	data field of a given type
  *  @len:	data length
  *  @par:	parity data, must be initialized by caller (usually all 0)
@@ -331,7 +349,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
  *
  *  Each field in the data array contains up to symbol size bits of valid data.
  */
-int encode_rs16(struct rs_control *rs, uint16_t *data, int len, uint16_t *par,
+int encode_rs16(struct rs_control *rsc, uint16_t *data, int len, uint16_t *par,
 	uint16_t invmsk)
 {
 #include "encode_rs.c"
@@ -342,7 +360,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
 #ifdef CONFIG_REED_SOLOMON_DEC16
 /**
  *  decode_rs16 - Decode codeword (16bit data width)
- *  @rs:	the rs control structure
+ *  @rsc:	the rs control structure
  *  @data:	data field of a given type
  *  @par:	received parity data field
  *  @len:	data length
@@ -355,7 +373,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
  *  Each field in the data array contains up to symbol size bits of valid data.
  *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
-int decode_rs16(struct rs_control *rs, uint16_t *data, uint16_t *par, int len,
+int decode_rs16(struct rs_control *rsc, uint16_t *data, uint16_t *par, int len,
 		uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
 		uint16_t *corr)
 {

Re: [patch V2 5/8] rslib: Split rs control struct

[Boris Brezillon]

On Thu, 19 Apr 2018 12:04:46 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The decoder library uses variable length arrays on stack. To get rid of
> them it would be simple to allocate fixed length arrays on stack, but those
> might become rather large. The other solution is to allocate the buffers in
> the rs control structure, but this cannot be done as long as the structure
> can be shared by several users. Sharing is desired because the RS polynom
> tables are large and initialization is time consuming.
> 
> To solve this split the codec information out of the control structure and
> have a pointer to a shared codec in it. Instantiate the control structure
> for each user, create a new codec if no shareable is avaiable yet.  Adjust
> all affected usage sites to the new scheme.
> 
> This allows to add per instance decoder buffers to the control structure
> later on.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>
> 
> ---
>  drivers/mtd/nand/raw/cafe_nand.c    |    7 +
>  drivers/mtd/nand/raw/diskonchip.c   |    7 +

For NAND relate stuff:

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

Don't know which release this patch series is targeting, but if it's
4.18 I'll need an immutable branch just in case other patches touch the
same files.

>  include/linux/rslib.h           |   18 +++--
>  lib/reed_solomon/decode_rs.c    |    1 
>  lib/reed_solomon/encode_rs.c    |    1 
>  lib/reed_solomon/reed_solomon.c |  144 ++++++++++++++++++++++------------------
>  6 files changed, 104 insertions(+), 74 deletions(-)
> 
> --- a/drivers/mtd/nand/raw/cafe_nand.c
> +++ b/drivers/mtd/nand/raw/cafe_nand.c
> @@ -394,12 +394,13 @@ static int cafe_nand_read_page(struct mt
>  
>  		for (i=0; i<8; i+=2) {
>  			uint32_t tmp = cafe_readl(cafe, NAND_ECC_SYN01 + (i*2));
> -			syn[i] = cafe->rs->index_of[tmp & 0xfff];
> -			syn[i+1] = cafe->rs->index_of[(tmp >> 16) & 0xfff];
> +
> +			syn[i] = cafe->rs->codec->index_of[tmp & 0xfff];
> +			syn[i+1] = cafe->rs->codec->index_of[(tmp >> 16) & 0xfff];
>  		}
>  
>  		n = decode_rs16(cafe->rs, NULL, NULL, 1367, syn, 0, pos, 0,
> -		                pat);
> +				pat);
>  
>  		for (i = 0; i < n; i++) {
>  			int p = pos[i];
> --- a/drivers/mtd/nand/raw/diskonchip.c
> +++ b/drivers/mtd/nand/raw/diskonchip.c
> @@ -142,6 +142,7 @@ static int doc_ecc_decode(struct rs_cont
>  	int i, j, nerr, errpos[8];
>  	uint8_t parity;
>  	uint16_t ds[4], s[5], tmp, errval[8], syn[4];
> +	struct rs_codec *cd = rs->codec;
>  
>  	memset(syn, 0, sizeof(syn));
>  	/* Convert the ecc bytes into words */
> @@ -162,15 +163,15 @@ static int doc_ecc_decode(struct rs_cont
>  	for (j = 1; j < NROOTS; j++) {
>  		if (ds[j] == 0)
>  			continue;
> -		tmp = rs->index_of[ds[j]];
> +		tmp = cd->index_of[ds[j]];
>  		for (i = 0; i < NROOTS; i++)
> -			s[i] ^= rs->alpha_to[rs_modnn(rs, tmp + (FCR + i) * j)];
> +			s[i] ^= cd->alpha_to[rs_modnn(cd, tmp + (FCR + i) * j)];
>  	}
>  
>  	/* Calc syn[i] = s[i] / alpha^(v + i) */
>  	for (i = 0; i < NROOTS; i++) {
>  		if (s[i])
> -			syn[i] = rs_modnn(rs, rs->index_of[s[i]] + (NN - FCR - i));
> +			syn[i] = rs_modnn(cd, cd->index_of[s[i]] + (NN - FCR - i));
>  	}
>  	/* Call the decoder library */
>  	nerr = decode_rs16(rs, NULL, NULL, 1019, syn, 0, errpos, 0, errval);
> --- a/include/linux/rslib.h
> +++ b/include/linux/rslib.h
> @@ -13,7 +13,7 @@
>  #include <linux/list.h>
>  
>  /**
> - * struct rs_control - rs control structure
> + * struct rs_codec - rs codec data
>   *
>   * @mm:		Bits per symbol
>   * @nn:		Symbols per block (= (1<<mm)-1)
> @@ -27,9 +27,9 @@
>   * @gfpoly:	The primitive generator polynominal
>   * @gffunc:	Function to generate the field, if non-canonical representation
>   * @users:	Users of this structure
> - * @list:	List entry for the rs control list
> + * @list:	List entry for the rs codec list
>  */
> -struct rs_control {
> +struct rs_codec {
>  	int		mm;
>  	int		nn;
>  	uint16_t	*alpha_to;
> @@ -45,6 +45,14 @@ struct rs_control {
>  	struct list_head list;
>  };
>  
> +/**
> + * struct rs_control - rs control structure per instance
> + * @codec:	The codec used for this instance
> + */
> +struct rs_control {
> +	struct rs_codec	*codec;
> +};
> +
>  /* General purpose RS codec, 8-bit data width, symbol width 1-15 bit  */
>  #ifdef CONFIG_REED_SOLOMON_ENC8
>  int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
> @@ -78,7 +86,7 @@ void free_rs(struct rs_control *rs);
>  
>  /** modulo replacement for galois field arithmetics
>   *
> - *  @rs:	the rs control structure
> + *  @rs:	Pointer to the RS codec
>   *  @x:		the value to reduce
>   *
>   *  where
> @@ -88,7 +96,7 @@ void free_rs(struct rs_control *rs);
>   *  Simple arithmetic modulo would return a wrong result for values
>   *  >= 3 * rs->nn
>  */
> -static inline int rs_modnn(struct rs_control *rs, int x)
> +static inline int rs_modnn(struct rs_codec *rs, int x)
>  {
>  	while (x >= rs->nn) {
>  		x -= rs->nn;
> --- a/lib/reed_solomon/decode_rs.c
> +++ b/lib/reed_solomon/decode_rs.c
> @@ -10,6 +10,7 @@
>   * Generic data width independent code which is included by the wrappers.
>   */
>  {
> +	struct rs_codec *rs = rsc->codec;
>  	int deg_lambda, el, deg_omega;
>  	int i, j, r, k, pad;
>  	int nn = rs->nn;
> --- a/lib/reed_solomon/encode_rs.c
> +++ b/lib/reed_solomon/encode_rs.c
> @@ -10,6 +10,7 @@
>   * Generic data width independent code which is included by the wrappers.
>   */
>  {
> +	struct rs_codec *rs = rsc->codec;
>  	int i, j, pad;
>  	int nn = rs->nn;
>  	int nroots = rs->nroots;
> --- a/lib/reed_solomon/reed_solomon.c
> +++ b/lib/reed_solomon/reed_solomon.c
> @@ -11,22 +11,23 @@
>   *
>   * The generic Reed Solomon library provides runtime configurable
>   * encoding / decoding of RS codes.
> - * Each user must call init_rs to get a pointer to a rs_control
> - * structure for the given rs parameters. This structure is either
> - * generated or a already available matching control structure is used.
> - * If a structure is generated then the polynomial arrays for
> - * fast encoding / decoding are built. This can take some time so
> - * make sure not to call this function from a time critical path.
> - * Usually a module / driver should initialize the necessary
> - * rs_control structure on module / driver init and release it
> - * on exit.
> - * The encoding puts the calculated syndrome into a given syndrome
> - * buffer.
> - * The decoding is a two step process. The first step calculates
> - * the syndrome over the received (data + syndrome) and calls the
> - * second stage, which does the decoding / error correction itself.
> - * Many hw encoders provide a syndrome calculation over the received
> - * data + syndrome and can call the second stage directly.
> + *
> + * Each user must call init_rs to get a pointer to a rs_control structure
> + * for the given rs parameters. The control struct is unique per instance.
> + * It points to a codec which can be shared by multiple control structures.
> + * If a codec is newly allocated then the polynomial arrays for fast
> + * encoding / decoding are built. This can take some time so make sure not
> + * to call this function from a time critical path.  Usually a module /
> + * driver should initialize the necessary rs_control structure on module /
> + * driver init and release it on exit.
> + *
> + * The encoding puts the calculated syndrome into a given syndrome buffer.
> + *
> + * The decoding is a two step process. The first step calculates the
> + * syndrome over the received (data + syndrome) and calls the second stage,
> + * which does the decoding / error correction itself.  Many hw encoders
> + * provide a syndrome calculation over the received data + syndrome and can
> + * call the second stage directly.
>   */
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
> @@ -36,13 +37,13 @@
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
>  
> -/* This list holds all currently allocated rs control structures */
> -static LIST_HEAD (rslist);
> +/* This list holds all currently allocated rs codec structures */
> +static LIST_HEAD(codec_list);
>  /* Protection for the list */
>  static DEFINE_MUTEX(rslistlock);
>  
>  /**
> - * rs_init - Initialize a Reed-Solomon codec
> + * codec_init - Initialize a Reed-Solomon codec
>   * @symsize:	symbol size, bits (1-8)
>   * @gfpoly:	Field generator polynomial coefficients
>   * @gffunc:	Field generator function
> @@ -50,18 +51,17 @@ static DEFINE_MUTEX(rslistlock);
>   * @prim:	primitive element to generate polynomial roots
>   * @nroots:	RS code generator polynomial degree (number of roots)
>   *
> - * Allocate a control structure and the polynom arrays for faster
> + * Allocate a codec structure and the polynom arrays for faster
>   * en/decoding. Fill the arrays according to the given parameters.
>   */
> -static struct rs_control *rs_init(int symsize, int gfpoly, int (*gffunc)(int),
> -				  int fcr, int prim, int nroots)
> +static struct rs_codec *codec_init(int symsize, int gfpoly, int (*gffunc)(int),
> +				   int fcr, int prim, int nroots)
>  {
> -	struct rs_control *rs;
>  	int i, j, sr, root, iprim;
> +	struct rs_codec *rs;
>  
> -	/* Allocate the control structure */
> -	rs = kmalloc(sizeof (struct rs_control), GFP_KERNEL);
> -	if (rs == NULL)
> +	rs = kmalloc(sizeof(*rs), GFP_KERNEL);
> +	if (!rs)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&rs->list);
> @@ -138,6 +138,9 @@ static struct rs_control *rs_init(int sy
>  	/* convert rs->genpoly[] to index form for quicker encoding */
>  	for (i = 0; i <= nroots; i++)
>  		rs->genpoly[i] = rs->index_of[rs->genpoly[i]];
> +
> +	rs->users = 1;
> +	list_add(&rs->list, &codec_list);
>  	return rs;
>  
>  	/* Error exit */
> @@ -154,26 +157,36 @@ static struct rs_control *rs_init(int sy
>  
>  
>  /**
> - *  free_rs - Free the rs control structure, if it is no longer used
> - *  @rs:	the control structure which is not longer used by the
> + *  free_rs - Free the rs control structure
> + *  @rs:	The control structure which is not longer used by the
>   *		caller
> + *
> + * Free the control structure. If @rs is the last user of the associated
> + * codec, free the codec as well.
>   */
>  void free_rs(struct rs_control *rs)
>  {
> +	struct rs_codec *cd;
> +
> +	if (!rs)
> +		return;
> +
> +	cd = rs->codec;
>  	mutex_lock(&rslistlock);
> -	rs->users--;
> -	if(!rs->users) {
> -		list_del(&rs->list);
> -		kfree(rs->alpha_to);
> -		kfree(rs->index_of);
> -		kfree(rs->genpoly);
> -		kfree(rs);
> +	cd->users--;
> +	if(!cd->users) {
> +		list_del(&cd->list);
> +		kfree(cd->alpha_to);
> +		kfree(cd->index_of);
> +		kfree(cd->genpoly);
> +		kfree(cd);
>  	}
>  	mutex_unlock(&rslistlock);
> +	kfree(rs);
>  }
>  
>  /**
> - * init_rs_internal - Find a matching or allocate a new rs control structure
> + * init_rs_internal - Allocate rs control, find a matching codec or allocate a new one
>   *  @symsize:	the symbol size (number of bits)
>   *  @gfpoly:	the extended Galois field generator polynomial coefficients,
>   *		with the 0th coefficient in the low order bit. The polynomial
> @@ -190,8 +203,8 @@ static struct rs_control *init_rs_intern
>  					   int (*gffunc)(int), int fcr,
>  					   int prim, int nroots)
>  {
> -	struct list_head	*tmp;
> -	struct rs_control	*rs;
> +	struct list_head *tmp;
> +	struct rs_control *rs;
>  
>  	/* Sanity checks */
>  	if (symsize < 1)
> @@ -203,33 +216,39 @@ static struct rs_control *init_rs_intern
>  	if (nroots < 0 || nroots >= (1<<symsize))
>  		return NULL;
>  
> +	rs = kzalloc(sizeof(*rs), GFP_KERNEL);
> +	if (!rs)
> +		return NULL;
> +
>  	mutex_lock(&rslistlock);
>  
>  	/* Walk through the list and look for a matching entry */
> -	list_for_each(tmp, &rslist) {
> -		rs = list_entry(tmp, struct rs_control, list);
> -		if (symsize != rs->mm)
> +	list_for_each(tmp, &codec_list) {
> +		struct rs_codec *cd = list_entry(tmp, struct rs_codec, list);
> +
> +		if (symsize != cd->mm)
>  			continue;
> -		if (gfpoly != rs->gfpoly)
> +		if (gfpoly != cd->gfpoly)
>  			continue;
> -		if (gffunc != rs->gffunc)
> +		if (gffunc != cd->gffunc)
>  			continue;
> -		if (fcr != rs->fcr)
> +		if (fcr != cd->fcr)
>  			continue;
> -		if (prim != rs->prim)
> +		if (prim != cd->prim)
>  			continue;
> -		if (nroots != rs->nroots)
> +		if (nroots != cd->nroots)
>  			continue;
>  		/* We have a matching one already */
> -		rs->users++;
> +		cd->users++;
> +		rs->codec = cd;
>  		goto out;
>  	}
>  
>  	/* Create a new one */
> -	rs = rs_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
> -	if (rs) {
> -		rs->users = 1;
> -		list_add(&rs->list, &rslist);
> +	rs->codec = codec_init(symsize, gfpoly, gffunc, fcr, prim, nroots);
> +	if (!rs->codec) {
> +		kfree(rs);
> +		rs = NULL;
>  	}
>  out:
>  	mutex_unlock(&rslistlock);
> @@ -237,7 +256,7 @@ static struct rs_control *init_rs_intern
>  }
>  
>  /**
> - * init_rs - Find a matching or allocate a new rs control structure
> + * init_rs - Create a RS control struct and initialize it
>   *  @symsize:	the symbol size (number of bits)
>   *  @gfpoly:	the extended Galois field generator polynomial coefficients,
>   *		with the 0th coefficient in the low order bit. The polynomial
> @@ -254,9 +273,8 @@ struct rs_control *init_rs(int symsize,
>  }
>  
>  /**
> - * init_rs_non_canonical - Find a matching or allocate a new rs control
> - *                         structure, for fields with non-canonical
> - *                         representation
> + * init_rs_non_canonical - Allocate rs control struct for fields with
> + *                         non-canonical representation
>   *  @symsize:	the symbol size (number of bits)
>   *  @gffunc:	pointer to function to generate the next field element,
>   *		or the multiplicative identity element if given 0.  Used
> @@ -275,7 +293,7 @@ struct rs_control *init_rs_non_canonical
>  #ifdef CONFIG_REED_SOLOMON_ENC8
>  /**
>   *  encode_rs8 - Calculate the parity for data values (8bit data width)
> - *  @rs:	the rs control structure
> + *  @rsc:	the rs control structure
>   *  @data:	data field of a given type
>   *  @len:	data length
>   *  @par:	parity data, must be initialized by caller (usually all 0)
> @@ -285,7 +303,7 @@ struct rs_control *init_rs_non_canonical
>   *  symbol size > 8. The calling code must take care of encoding of the
>   *  syndrome result for storage itself.
>   */
> -int encode_rs8(struct rs_control *rs, uint8_t *data, int len, uint16_t *par,
> +int encode_rs8(struct rs_control *rsc, uint8_t *data, int len, uint16_t *par,
>  	       uint16_t invmsk)
>  {
>  #include "encode_rs.c"
> @@ -296,7 +314,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
>  #ifdef CONFIG_REED_SOLOMON_DEC8
>  /**
>   *  decode_rs8 - Decode codeword (8bit data width)
> - *  @rs:	the rs control structure
> + *  @rsc:	the rs control structure
>   *  @data:	data field of a given type
>   *  @par:	received parity data field
>   *  @len:	data length
> @@ -311,7 +329,7 @@ EXPORT_SYMBOL_GPL(encode_rs8);
>   *  syndrome result and the received parity before calling this code.
>   *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
>   */
> -int decode_rs8(struct rs_control *rs, uint8_t *data, uint16_t *par, int len,
> +int decode_rs8(struct rs_control *rsc, uint8_t *data, uint16_t *par, int len,
>  	       uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
>  	       uint16_t *corr)
>  {
> @@ -323,7 +341,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
>  #ifdef CONFIG_REED_SOLOMON_ENC16
>  /**
>   *  encode_rs16 - Calculate the parity for data values (16bit data width)
> - *  @rs:	the rs control structure
> + *  @rsc:	the rs control structure
>   *  @data:	data field of a given type
>   *  @len:	data length
>   *  @par:	parity data, must be initialized by caller (usually all 0)
> @@ -331,7 +349,7 @@ EXPORT_SYMBOL_GPL(decode_rs8);
>   *
>   *  Each field in the data array contains up to symbol size bits of valid data.
>   */
> -int encode_rs16(struct rs_control *rs, uint16_t *data, int len, uint16_t *par,
> +int encode_rs16(struct rs_control *rsc, uint16_t *data, int len, uint16_t *par,
>  	uint16_t invmsk)
>  {
>  #include "encode_rs.c"
> @@ -342,7 +360,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
>  #ifdef CONFIG_REED_SOLOMON_DEC16
>  /**
>   *  decode_rs16 - Decode codeword (16bit data width)
> - *  @rs:	the rs control structure
> + *  @rsc:	the rs control structure
>   *  @data:	data field of a given type
>   *  @par:	received parity data field
>   *  @len:	data length
> @@ -355,7 +373,7 @@ EXPORT_SYMBOL_GPL(encode_rs16);
>   *  Each field in the data array contains up to symbol size bits of valid data.
>   *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
>   */
> -int decode_rs16(struct rs_control *rs, uint16_t *data, uint16_t *par, int len,
> +int decode_rs16(struct rs_control *rsc, uint16_t *data, uint16_t *par, int len,
>  		uint16_t *s, int no_eras, int *eras_pos, uint16_t invmsk,
>  		uint16_t *corr)
>  {
> 
> 
> 
> 
> 

[patch V2 6/8] mtd/diskonchip: Allocate rs control per instance

[Thomas Gleixner]

[-- Attachment #1: mtd-diskonchip--Allocate_rs_control_per_instance.patch --]
[-- Type: text/plain, Size: 5233 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

The reed solomon library is moving the on stack decoder buffers into the rs
control structure. That would break the DoC driver because multiple
instances share the same control structure and can operate in parallel. At
least in theory....

Instantiate a rs control instance per DoC device to avoid that. The per
instance buffer is fine as the operation on a single DoC instance is
serialized by the MTD/NAND core.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 drivers/mtd/nand/raw/diskonchip.c |   60 +++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

--- a/drivers/mtd/nand/raw/diskonchip.c
+++ b/drivers/mtd/nand/raw/diskonchip.c
@@ -66,6 +66,7 @@ struct doc_priv {
 	int curchip;
 	int mh0_page;
 	int mh1_page;
+	struct rs_control *rs_decoder;
 	struct mtd_info *nextdoc;
 
 	/* Handle the last stage of initialization (BBT scan, partitioning) */
@@ -123,9 +124,6 @@ MODULE_PARM_DESC(doc_config_location, "P
 /* Number of symbols */
 #define NN 1023
 
-/* the Reed Solomon control structure */
-static struct rs_control *rs_decoder;
-
 /*
  * The HW decoder in the DoC ASIC's provides us a error syndrome,
  * which we must convert to a standard syndrome usable by the generic
@@ -931,7 +929,7 @@ static int doc200x_correct_data(struct m
 				calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i);
 		}
 
-		ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
+		ret = doc_ecc_decode(doc->rs_decoder, dat, calc_ecc);
 		if (ret > 0)
 			pr_err("doc200x_correct_data corrected %d errors\n",
 			       ret);
@@ -1422,10 +1420,10 @@ static inline int __init doc2001plus_ini
 
 static int __init doc_probe(unsigned long physadr)
 {
+	struct nand_chip *nand = NULL;
+	struct doc_priv *doc = NULL;
 	unsigned char ChipID;
 	struct mtd_info *mtd;
-	struct nand_chip *nand;
-	struct doc_priv *doc;
 	void __iomem *virtadr;
 	unsigned char save_control;
 	unsigned char tmp, tmpb, tmpc;
@@ -1562,8 +1560,25 @@ static int __init doc_probe(unsigned lon
 		goto fail;
 	}
 
+
+	/*
+	 * Allocate a RS codec instance
+	 *
+	 * Symbolsize is 10 (bits)
+	 * Primitve polynomial is x^10+x^3+1
+	 * First consecutive root is 510
+	 * Primitve element to generate roots = 1
+	 * Generator polinomial degree = 4
+	 */
+	doc = (struct doc_priv *) (nand + 1);
+	doc->rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
+	if (!doc->rs_decoder) {
+		pr_err("DiskOnChip: Could not create a RS codec\n");
+		ret = -ENOMEM;
+		goto fail;
+	}
+
 	mtd			= nand_to_mtd(nand);
-	doc			= (struct doc_priv *) (nand + 1);
 	nand->bbt_td		= (struct nand_bbt_descr *) (doc + 1);
 	nand->bbt_md		= nand->bbt_td + 1;
 
@@ -1613,7 +1628,6 @@ static int __init doc_probe(unsigned lon
 		   haven't yet added it.  This is handled without incident by
 		   mtd_device_unregister, as far as I can tell. */
 		nand_release(mtd);
-		kfree(nand);
 		goto fail;
 	}
 
@@ -1626,6 +1640,9 @@ static int __init doc_probe(unsigned lon
 	   actually a DiskOnChip.  */
 	WriteDOC(save_control, virtadr, DOCControl);
  fail:
+	if (doc)
+		free_rs(doc->rs_decoder);
+	kfree(nand);
 	iounmap(virtadr);
 
 error_ioremap:
@@ -1648,6 +1665,7 @@ static void release_nanddoc(void)
 		nand_release(mtd);
 		iounmap(doc->virtadr);
 		release_mem_region(doc->physadr, DOC_IOREMAP_LEN);
+		free_rs(doc->rs_decoder);
 		kfree(nand);
 	}
 }
@@ -1656,27 +1674,12 @@ static int __init init_nanddoc(void)
 {
 	int i, ret = 0;
 
-	/* We could create the decoder on demand, if memory is a concern.
-	 * This way we have it handy, if an error happens
-	 *
-	 * Symbolsize is 10 (bits)
-	 * Primitve polynomial is x^10+x^3+1
-	 * first consecutive root is 510
-	 * primitve element to generate roots = 1
-	 * generator polinomial degree = 4
-	 */
-	rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
-	if (!rs_decoder) {
-		pr_err("DiskOnChip: Could not create a RS decoder\n");
-		return -ENOMEM;
-	}
-
 	if (doc_config_location) {
 		pr_info("Using configured DiskOnChip probe address 0x%lx\n",
 			doc_config_location);
 		ret = doc_probe(doc_config_location);
 		if (ret < 0)
-			goto outerr;
+			return ret;
 	} else {
 		for (i = 0; (doc_locations[i] != 0xffffffff); i++) {
 			doc_probe(doc_locations[i]);
@@ -1687,11 +1690,7 @@ static int __init init_nanddoc(void)
 	if (!doclist) {
 		pr_info("No valid DiskOnChip devices found\n");
 		ret = -ENODEV;
-		goto outerr;
 	}
-	return 0;
- outerr:
-	free_rs(rs_decoder);
 	return ret;
 }
 
@@ -1699,11 +1698,6 @@ static void __exit cleanup_nanddoc(void)
 {
 	/* Cleanup the nand/DoC resources */
 	release_nanddoc();
-
-	/* Free the reed solomon resources */
-	if (rs_decoder) {
-		free_rs(rs_decoder);
-	}
 }
 
 module_init(init_nanddoc);

Re: [patch V2 6/8] mtd/diskonchip: Allocate rs control per instance

[Boris Brezillon]

On Thu, 19 Apr 2018 12:04:47 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The reed solomon library is moving the on stack decoder buffers into the rs
> control structure. That would break the DoC driver because multiple
> instances share the same control structure and can operate in parallel. At
> least in theory....
> 
> Instantiate a rs control instance per DoC device to avoid that. The per
> instance buffer is fine as the operation on a single DoC instance is
> serialized by the MTD/NAND core.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

If you happen to send a new version, can you change the subject prefix
to "mtd: rawnand: diskonchip: " (instead of "mtd/diskonchip: ")?

> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>
> 
> ---
>  drivers/mtd/nand/raw/diskonchip.c |   60 +++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 33 deletions(-)
> 
> --- a/drivers/mtd/nand/raw/diskonchip.c
> +++ b/drivers/mtd/nand/raw/diskonchip.c
> @@ -66,6 +66,7 @@ struct doc_priv {
>  	int curchip;
>  	int mh0_page;
>  	int mh1_page;
> +	struct rs_control *rs_decoder;
>  	struct mtd_info *nextdoc;
>  
>  	/* Handle the last stage of initialization (BBT scan, partitioning) */
> @@ -123,9 +124,6 @@ MODULE_PARM_DESC(doc_config_location, "P
>  /* Number of symbols */
>  #define NN 1023
>  
> -/* the Reed Solomon control structure */
> -static struct rs_control *rs_decoder;
> -
>  /*
>   * The HW decoder in the DoC ASIC's provides us a error syndrome,
>   * which we must convert to a standard syndrome usable by the generic
> @@ -931,7 +929,7 @@ static int doc200x_correct_data(struct m
>  				calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i);
>  		}
>  
> -		ret = doc_ecc_decode(rs_decoder, dat, calc_ecc);
> +		ret = doc_ecc_decode(doc->rs_decoder, dat, calc_ecc);
>  		if (ret > 0)
>  			pr_err("doc200x_correct_data corrected %d errors\n",
>  			       ret);
> @@ -1422,10 +1420,10 @@ static inline int __init doc2001plus_ini
>  
>  static int __init doc_probe(unsigned long physadr)
>  {
> +	struct nand_chip *nand = NULL;
> +	struct doc_priv *doc = NULL;
>  	unsigned char ChipID;
>  	struct mtd_info *mtd;
> -	struct nand_chip *nand;
> -	struct doc_priv *doc;
>  	void __iomem *virtadr;
>  	unsigned char save_control;
>  	unsigned char tmp, tmpb, tmpc;
> @@ -1562,8 +1560,25 @@ static int __init doc_probe(unsigned lon
>  		goto fail;
>  	}
>  
> +
> +	/*
> +	 * Allocate a RS codec instance
> +	 *
> +	 * Symbolsize is 10 (bits)
> +	 * Primitve polynomial is x^10+x^3+1
> +	 * First consecutive root is 510
> +	 * Primitve element to generate roots = 1
> +	 * Generator polinomial degree = 4
> +	 */
> +	doc = (struct doc_priv *) (nand + 1);
> +	doc->rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
> +	if (!doc->rs_decoder) {
> +		pr_err("DiskOnChip: Could not create a RS codec\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
>  	mtd			= nand_to_mtd(nand);
> -	doc			= (struct doc_priv *) (nand + 1);
>  	nand->bbt_td		= (struct nand_bbt_descr *) (doc + 1);
>  	nand->bbt_md		= nand->bbt_td + 1;
>  
> @@ -1613,7 +1628,6 @@ static int __init doc_probe(unsigned lon
>  		   haven't yet added it.  This is handled without incident by
>  		   mtd_device_unregister, as far as I can tell. */
>  		nand_release(mtd);
> -		kfree(nand);
>  		goto fail;
>  	}
>  
> @@ -1626,6 +1640,9 @@ static int __init doc_probe(unsigned lon
>  	   actually a DiskOnChip.  */
>  	WriteDOC(save_control, virtadr, DOCControl);
>   fail:
> +	if (doc)
> +		free_rs(doc->rs_decoder);
> +	kfree(nand);
>  	iounmap(virtadr);
>  
>  error_ioremap:
> @@ -1648,6 +1665,7 @@ static void release_nanddoc(void)
>  		nand_release(mtd);
>  		iounmap(doc->virtadr);
>  		release_mem_region(doc->physadr, DOC_IOREMAP_LEN);
> +		free_rs(doc->rs_decoder);
>  		kfree(nand);
>  	}
>  }
> @@ -1656,27 +1674,12 @@ static int __init init_nanddoc(void)
>  {
>  	int i, ret = 0;
>  
> -	/* We could create the decoder on demand, if memory is a concern.
> -	 * This way we have it handy, if an error happens
> -	 *
> -	 * Symbolsize is 10 (bits)
> -	 * Primitve polynomial is x^10+x^3+1
> -	 * first consecutive root is 510
> -	 * primitve element to generate roots = 1
> -	 * generator polinomial degree = 4
> -	 */
> -	rs_decoder = init_rs(10, 0x409, FCR, 1, NROOTS);
> -	if (!rs_decoder) {
> -		pr_err("DiskOnChip: Could not create a RS decoder\n");
> -		return -ENOMEM;
> -	}
> -
>  	if (doc_config_location) {
>  		pr_info("Using configured DiskOnChip probe address 0x%lx\n",
>  			doc_config_location);
>  		ret = doc_probe(doc_config_location);
>  		if (ret < 0)
> -			goto outerr;
> +			return ret;
>  	} else {
>  		for (i = 0; (doc_locations[i] != 0xffffffff); i++) {
>  			doc_probe(doc_locations[i]);
> @@ -1687,11 +1690,7 @@ static int __init init_nanddoc(void)
>  	if (!doclist) {
>  		pr_info("No valid DiskOnChip devices found\n");
>  		ret = -ENODEV;
> -		goto outerr;
>  	}
> -	return 0;
> - outerr:
> -	free_rs(rs_decoder);
>  	return ret;
>  }
>  
> @@ -1699,11 +1698,6 @@ static void __exit cleanup_nanddoc(void)
>  {
>  	/* Cleanup the nand/DoC resources */
>  	release_nanddoc();
> -
> -	/* Free the reed solomon resources */
> -	if (rs_decoder) {
> -		free_rs(rs_decoder);
> -	}
>  }
>  
>  module_init(init_nanddoc);
> 
> 

[patch V2 7/8] dm verity fec: Check result of init_rs()

[Thomas Gleixner]

[-- Attachment #1: dm_verity_fec--Check_result_of_init_rs--.patch --]
[-- Type: text/plain, Size: 1345 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

The allocation of the reed solomon control structure can fail, but
fec_alloc_bufs() ignores that and subsequent operations in dm verity use
the potential NULL pointer unconditionally.

Add a proper check and abort if init_rs() fails.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 drivers/md/dm-verity-fec.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -308,8 +308,13 @@ static int fec_alloc_bufs(struct dm_veri
 {
 	unsigned n;
 
-	if (!fio->rs)
+	if (!fio->rs) {
 		fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO);
+		if (!fio->rs) {
+			DMERR("failed to allocate RS control structure");
+			return -ENOMEM;
+		}
+	}
 
 	fec_for_each_prealloc_buffer(n) {
 		if (fio->bufs[n])

Re: [patch V2 7/8] dm verity fec: Check result of init_rs()

[Mike Snitzer]

On Thu, Apr 19 2018 at  6:04am -0400,
Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The allocation of the reed solomon control structure can fail, but
> fec_alloc_bufs() ignores that and subsequent operations in dm verity use
> the potential NULL pointer unconditionally.
> 
> Add a proper check and abort if init_rs() fails.

This changelog makes little sense: init_rs() isn't in play relative to
this patch.

And it runs counter to this commit's changelog:

commit 34c96507e8f6be497c15497be05f489fb34c5880
Author: NeilBrown <neilb@suse.com>
Date:   Mon Apr 10 12:13:00 2017 +1000

    dm verity fec: fix GFP flags used with mempool_alloc()

    mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no
    point testing for failure.

    One place the code tested for failure was passing "0" as the GFP
    flags.  This is most unusual and is probably meant to be GFP_NOIO,
    so that is changed.

    Also, allocation from ->extra_pool and ->prealloc_pool are repeated
    before releasing the previous allocation.  This can deadlock if the code
    is servicing a write under high memory pressure.  To avoid deadlocks,
    change these to use GFP_NOWAIT and leave the error handling in place.

    Signed-off-by: NeilBrown <neilb@suse.com>
    Signed-off-by: Mike Snitzer <snitzer@redhat.com>

Seems there is no real need for this patch.  Neil, what do you think?

Thanks,
Mike


> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Mike Snitzer <snitzer@redhat.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Andrew Morton <akpm@linuxfoundation.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Alasdair Kergon <agk@redhat.com>
> 
> ---
>  drivers/md/dm-verity-fec.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> --- a/drivers/md/dm-verity-fec.c
> +++ b/drivers/md/dm-verity-fec.c
> @@ -308,8 +308,13 @@ static int fec_alloc_bufs(struct dm_veri
>  {
>  	unsigned n;
>  
> -	if (!fio->rs)
> +	if (!fio->rs) {
>  		fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO);
> +		if (!fio->rs) {
> +			DMERR("failed to allocate RS control structure");
> +			return -ENOMEM;
> +		}
> +	}
>  
>  	fec_for_each_prealloc_buffer(n) {
>  		if (fio->bufs[n])
> 
> 
> 
> 
> 

Re: [patch V2 7/8] dm verity fec: Check result of init_rs()

[Thomas Gleixner]

On Thu, 19 Apr 2018, Mike Snitzer wrote:

> On Thu, Apr 19 2018 at  6:04am -0400,
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > The allocation of the reed solomon control structure can fail, but
> > fec_alloc_bufs() ignores that and subsequent operations in dm verity use
> > the potential NULL pointer unconditionally.
> > 
> > Add a proper check and abort if init_rs() fails.
> 
> This changelog makes little sense: init_rs() isn't in play relative to
> this patch.

	fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO);

        f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc,
                                    fec_rs_free, (void *) v);

static void *fec_rs_alloc(gfp_t gfp_mask, void *pool_data)
{
	struct dm_verity *v = (struct dm_verity *)pool_data;

        return init_rs(8, 0x11d, 0, 1, v->fec->roots);
}

So init_rs() is part of the chain, right?

Yes. I missed the NOIO part. But....

> And it runs counter to this commit's changelog:
> 
> commit 34c96507e8f6be497c15497be05f489fb34c5880
> Author: NeilBrown <neilb@suse.com>
> Date:   Mon Apr 10 12:13:00 2017 +1000
> 
>     dm verity fec: fix GFP flags used with mempool_alloc()
> 
>     mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no
>     point testing for failure.
> 
>     One place the code tested for failure was passing "0" as the GFP
>     flags.  This is most unusual and is probably meant to be GFP_NOIO,
>     so that is changed.
> 
>     Also, allocation from ->extra_pool and ->prealloc_pool are repeated
>     before releasing the previous allocation.  This can deadlock if the code
>     is servicing a write under high memory pressure.  To avoid deadlocks,
>     change these to use GFP_NOWAIT and leave the error handling in place.
> 
>     Signed-off-by: NeilBrown <neilb@suse.com>
>     Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> 
> Seems there is no real need for this patch.  Neil, what do you think?

The analysis above forgot to look at the mempool->alloc() callback. So yes,
while the NOIO is good at the mempool level, but init_rs() uses GPF_KERNEL
so there might be a different can of wurms lurking.

Thanks,

	tglx

Re: [patch V2 7/8] dm verity fec: Check result of init_rs()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]

On Thu, Apr 19 2018, Thomas Gleixner wrote:

> On Thu, 19 Apr 2018, Mike Snitzer wrote:
>
>> On Thu, Apr 19 2018 at  6:04am -0400,
>> Thomas Gleixner <tglx@linutronix.de> wrote:
>> 
>> > From: Thomas Gleixner <tglx@linutronix.de>
>> > 
>> > The allocation of the reed solomon control structure can fail, but
>> > fec_alloc_bufs() ignores that and subsequent operations in dm verity use
>> > the potential NULL pointer unconditionally.
>> > 
>> > Add a proper check and abort if init_rs() fails.
>> 
>> This changelog makes little sense: init_rs() isn't in play relative to
>> this patch.
>
> 	fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO);
>
>         f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc,
>                                     fec_rs_free, (void *) v);
>
> static void *fec_rs_alloc(gfp_t gfp_mask, void *pool_data)
> {
> 	struct dm_verity *v = (struct dm_verity *)pool_data;
>
>         return init_rs(8, 0x11d, 0, 1, v->fec->roots);
> }
>
> So init_rs() is part of the chain, right?
>
> Yes. I missed the NOIO part. But....
>
>> And it runs counter to this commit's changelog:
>> 
>> commit 34c96507e8f6be497c15497be05f489fb34c5880
>> Author: NeilBrown <neilb@suse.com>
>> Date:   Mon Apr 10 12:13:00 2017 +1000
>> 
>>     dm verity fec: fix GFP flags used with mempool_alloc()
>> 
>>     mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no
>>     point testing for failure.
>> 
>>     One place the code tested for failure was passing "0" as the GFP
>>     flags.  This is most unusual and is probably meant to be GFP_NOIO,
>>     so that is changed.
>> 
>>     Also, allocation from ->extra_pool and ->prealloc_pool are repeated
>>     before releasing the previous allocation.  This can deadlock if the code
>>     is servicing a write under high memory pressure.  To avoid deadlocks,
>>     change these to use GFP_NOWAIT and leave the error handling in place.
>> 
>>     Signed-off-by: NeilBrown <neilb@suse.com>
>>     Signed-off-by: Mike Snitzer <snitzer@redhat.com>
>> 
>> Seems there is no real need for this patch.  Neil, what do you think?

I think the code is correct as-is.

>
> The analysis above forgot to look at the mempool->alloc() callback. So yes,
> while the NOIO is good at the mempool level, but init_rs() uses GPF_KERNEL
> so there might be a different can of wurms lurking.

The ->alloc call back is not relevant to the question of when
mempool_alloc() can return NULL.
If the ->alloc() callback returns a non-NULL value, it will be returned
by mempool_alloc().
If it returns NULL, that will not be returned.

mempool_alloc() *only* returns NULL in one place:

	if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
		spin_unlock_irqrestore(&pool->lock, flags);
		return NULL;
	}

so a NULL return is purely dependent on the GFP flags passed.
GFP_NOIO contains __GFP_DIRECT_RECLAIM, so NULL cannot be returned.

It seems quite broken that init_rs() uses GFP_KERNEL. It should take a
gfp_t arg for the allocation.
If the mempool_alloc() above really needs GFP_NOIO, then it could
theoretically deadlock as it performs a GFP_KERNEL allocation inside
rs_init().  So in that sense, the code is not correct as-is.
It could possibly be fixed by calling memalloc_noio_save() /
memalloc_noio_restore() around the call to init_rs() in fec_rs_alloc().


Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [patch V2 7/8] dm verity fec: Check result of init_rs()

[Thomas Gleixner]

On Fri, 20 Apr 2018, NeilBrown wrote:
> On Thu, Apr 19 2018, Thomas Gleixner wrote:
> > The analysis above forgot to look at the mempool->alloc() callback. So yes,
> > while the NOIO is good at the mempool level, but init_rs() uses GPF_KERNEL
> > so there might be a different can of wurms lurking.
> 
> The ->alloc call back is not relevant to the question of when
> mempool_alloc() can return NULL.
> If the ->alloc() callback returns a non-NULL value, it will be returned
> by mempool_alloc().
> If it returns NULL, that will not be returned.

Yes, as I said before, I missed the NOIO flag.

> 
> mempool_alloc() *only* returns NULL in one place:
> 
> 	if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> 		spin_unlock_irqrestore(&pool->lock, flags);
> 		return NULL;
> 	}
> 
> so a NULL return is purely dependent on the GFP flags passed.
> GFP_NOIO contains __GFP_DIRECT_RECLAIM, so NULL cannot be returned.
> 
> It seems quite broken that init_rs() uses GFP_KERNEL. It should take a
> gfp_t arg for the allocation.

Well, init_rs() was that way before somebody used it in a mempool_alloc()
callback. And all other users are fine with GFP_KERNEL AFAICT.

> If the mempool_alloc() above really needs GFP_NOIO, then it could
> theoretically deadlock as it performs a GFP_KERNEL allocation inside
> rs_init().  So in that sense, the code is not correct as-is.
> It could possibly be fixed by calling memalloc_noio_save() /
> memalloc_noio_restore() around the call to init_rs() in fec_rs_alloc().

No, we surely can add a gfp aware version of init_rs(). It's trivial enough
to do.

Thanks,

	tglx

[patch V2 8/8] rslib: Allocate decoder buffers to avoid VLAs

[Thomas Gleixner]

[-- Attachment #1: rslib--Allocate_decoder_buffers_to_avoid_VLAs.patch --]
[-- Type: text/plain, Size: 5399 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

To get rid of the variable length arrays on stack in the RS decoder it's
necessary to allocate the decoder buffers per control structure instance.

All usage sites have been checked for potential parallel decoder usage and
fixed where necessary. Kees confirmed that the pstore decoding is strictly
single threaded so there should be no surprises.

Allocate them in the rs control structure sized depending on the number of
roots for the chosen codec and adapt the decoder code to make use of them.

Document the fact that decode operations based on a particular rs control
instance cannot run in parallel and the caller has to ensure that as it's
not possible to provide a proper locking construct which fits all use
cases.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Andrew Morton <akpm@linuxfoundation.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Alasdair Kergon <agk@redhat.com>

---
 include/linux/rslib.h           |    1 +
 lib/reed_solomon/decode_rs.c    |   20 +++++++++++++-------
 lib/reed_solomon/reed_solomon.c |   31 ++++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 8 deletions(-)

--- a/include/linux/rslib.h
+++ b/include/linux/rslib.h
@@ -51,6 +51,7 @@ struct rs_codec {
  */
 struct rs_control {
 	struct rs_codec	*codec;
+	uint16_t	buffers[0];
 };
 
 /* General purpose RS codec, 8-bit data width, symbol width 1-15 bit  */
--- a/lib/reed_solomon/decode_rs.c
+++ b/lib/reed_solomon/decode_rs.c
@@ -21,16 +21,22 @@
 	uint16_t *alpha_to = rs->alpha_to;
 	uint16_t *index_of = rs->index_of;
 	uint16_t u, q, tmp, num1, num2, den, discr_r, syn_error;
-	/* Err+Eras Locator poly and syndrome poly The maximum value
-	 * of nroots is 8. So the necessary stack size will be about
-	 * 220 bytes max.
-	 */
-	uint16_t lambda[nroots + 1], syn[nroots];
-	uint16_t b[nroots + 1], t[nroots + 1], omega[nroots + 1];
-	uint16_t root[nroots], reg[nroots + 1], loc[nroots];
 	int count = 0;
 	uint16_t msk = (uint16_t) rs->nn;
 
+	/*
+	 * The decoder buffers are in the rs control struct. They are
+	 * arrays sized [nroots + 1]
+	 */
+	uint16_t *lambda = rsc->buffers + RS_DECODE_LAMBDA * (nroots + 1);
+	uint16_t *syn = rsc->buffers + RS_DECODE_SYN * (nroots + 1);
+	uint16_t *b = rsc->buffers + RS_DECODE_B * (nroots + 1);
+	uint16_t *t = rsc->buffers + RS_DECODE_T * (nroots + 1);
+	uint16_t *omega = rsc->buffers + RS_DECODE_OMEGA * (nroots + 1);
+	uint16_t *root = rsc->buffers + RS_DECODE_ROOT * (nroots + 1);
+	uint16_t *reg = rsc->buffers + RS_DECODE_REG * (nroots + 1);
+	uint16_t *loc = rsc->buffers + RS_DECODE_LOC * (nroots + 1);
+
 	/* Check length parameter for validity */
 	pad = nn - nroots - len;
 	BUG_ON(pad < 0 || pad >= nn);
--- a/lib/reed_solomon/reed_solomon.c
+++ b/lib/reed_solomon/reed_solomon.c
@@ -37,6 +37,18 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 
+enum {
+	RS_DECODE_LAMBDA,
+	RS_DECODE_SYN,
+	RS_DECODE_B,
+	RS_DECODE_T,
+	RS_DECODE_OMEGA,
+	RS_DECODE_ROOT,
+	RS_DECODE_REG,
+	RS_DECODE_LOC,
+	RS_DECODE_NUM_BUFFERS
+};
+
 /* This list holds all currently allocated rs codec structures */
 static LIST_HEAD(codec_list);
 /* Protection for the list */
@@ -205,6 +217,7 @@ static struct rs_control *init_rs_intern
 {
 	struct list_head *tmp;
 	struct rs_control *rs;
+	unsigned int bsize;
 
 	/* Sanity checks */
 	if (symsize < 1)
@@ -216,7 +229,13 @@ static struct rs_control *init_rs_intern
 	if (nroots < 0 || nroots >= (1<<symsize))
 		return NULL;
 
-	rs = kzalloc(sizeof(*rs), GFP_KERNEL);
+	/*
+	 * The decoder needs buffers in each control struct instance to
+	 * avoid variable size or large fixed size allocations on
+	 * stack. Size the buffers to arrays of [nroots + 1].
+	 */
+	bsize = sizeof(uint16_t) * RS_DECODE_NUM_BUFFERS * (nroots + 1);
+	rs = kzalloc(sizeof(*rs) + bsize, GFP_KERNEL);
 	if (!rs)
 		return NULL;
 
@@ -327,6 +346,11 @@ EXPORT_SYMBOL_GPL(encode_rs8);
  *  The syndrome and parity uses a uint16_t data type to enable
  *  symbol size > 8. The calling code must take care of decoding of the
  *  syndrome result and the received parity before calling this code.
+ *
+ *  Note: The rs_control struct @rsc contains buffers which are used for
+ *  decoding, so the caller has to ensure that decoder invocations are
+ *  serialized.
+ *
  *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
 int decode_rs8(struct rs_control *rsc, uint8_t *data, uint16_t *par, int len,
@@ -371,6 +395,11 @@ EXPORT_SYMBOL_GPL(encode_rs16);
  *  @corr:	buffer to store correction bitmask on eras_pos
  *
  *  Each field in the data array contains up to symbol size bits of valid data.
+ *
+ *  Note: The rc_control struct @rsc contains buffers which are used for
+ *  decoding, so the caller has to ensure that decoder invocations are
+ *  serialized.
+ *
  *  Returns the number of corrected bits or -EBADMSG for uncorrectable errors.
  */
 int decode_rs16(struct rs_control *rsc, uint16_t *data, uint16_t *par, int len,

Re: [patch V2 0/8] rslib: Cleanup and VLA removal

[Kees Cook]

On Thu, Apr 19, 2018 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Kees tried to get rid of the Variable Length Arrays in the Reed-Solomon
> library by replacing them with fixed length arrays on stack. Though they
> are rather large and Andrew did not fall in love with that solution.
>
> This series addresses that in a different way by splitting the rs control
> structure up, so that each invocation of rs_init() returns a new instance
> in which the decoder buffers are allocated. The polynom tables which build
> the base for the RS codecs are still shareable between the instances to
> avoid large allocations and initializations of the same data over and over.
>
> The usage sites have been audited and fixed up where necessary to
> accomodate the decoder change which forbids parallel decoder invocation for
> a particular rs control instance to prevent buffer corruption.
>
> While at it the patch set tidies up the code and converts the related files
> over to use SPDX license identifiers.
>
> Changes since V1:
>
>    Simplify error path in the diskonchip code and use the proper
>    function to free the decoder.
>
> As this spawns multiple subsystems it should either go through Andrews tree
> or Kees can route it with his other hardening stuff.

I can start collecting ignored VLA patches. I haven't done that yet as
most maintainers have been taking them. There are only a handful
unapplied. Should I start the tree with rslib? :)

Either way:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security