[PATCH v2 1/2] lib/math/rational.c: Fix divide by zero

[PATCH v2 1/2] lib/math/rational.c: Fix divide by zero

[Trent Piepho]

If the input is out of the range of the allowed values, either larger
than the largest value or closer to zero than the smallest non-zero
allowed value, then a division by zero would occur.

In the case of input too large, the division by zero will occur on the
first iteration.  The best result (largest allowed value) will be found
by always choosing the semi-convergent and excluding the denominator
based limit when finding it.

In the case of the input too small, the division by zero will occur on
the second iteration.  The numerator based semi-convergent should not be
calculated to avoid the division by zero.  But the semi-convergent vs
previous convergent test is still needed, which effectively chooses
between 0 (the previous convergent) vs the smallest allowed fraction
(best semi-convergent) as the result.

Fixes: 323dd2c3ed0 ("lib/math/rational.c: fix possible incorrect result from rational fractions helper")
Reported-by: Yiyuan Guo <yguoaz@gmail.com>
Signed-off-by: Trent Piepho <tpiepho@gmail.com>
---
 lib/math/rational.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/math/rational.c b/lib/math/rational.c
index 9781d521963d..c0ab51d8fbb9 100644
--- a/lib/math/rational.c
+++ b/lib/math/rational.c
@@ -12,6 +12,7 @@
 #include <linux/compiler.h>
 #include <linux/export.h>
 #include <linux/minmax.h>
+#include <linux/limits.h>
 
 /*
  * calculate best rational approximation for a given fraction
@@ -78,13 +79,18 @@ void rational_best_approximation(
 		 * found below as 't'.
 		 */
 		if ((n2 > max_numerator) || (d2 > max_denominator)) {
-			unsigned long t = min((max_numerator - n0) / n1,
-					      (max_denominator - d0) / d1);
+			unsigned long t = ULONG_MAX;
 
-			/* This tests if the semi-convergent is closer
-			 * than the previous convergent.
+			if (d1)
+				t = (max_denominator - d0) / d1;
+			if (n1)
+				t = min(t, (max_numerator - n0) / n1);
+
+			/* This tests if the semi-convergent is closer than the previous
+			 * convergent.  If d1 is zero there is no previous convergent as this
+			 * is the 1st iteration, so always choose the semi-convergent.
 			 */
-			if (2u * t > a || (2u * t == a && d0 * dp > d1 * d)) {
+			if (!d1 || 2u * t > a || (2u * t == a && d0 * dp > d1 * d)) {
 				n1 = n0 + t * n1;
 				d1 = d0 + t * d1;
 			}
-- 
2.26.2

[PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Trent Piepho]

Adds a number of test cases that cover a range of possible code paths.

Signed-off-by: Trent Piepho <tpiepho@gmail.com>
---
 lib/Kconfig.debug        | 12 +++++++++
 lib/math/Makefile        |  1 +
 lib/math/rational-test.c | 56 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 lib/math/rational-test.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580..6c0e66a7d416 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2429,6 +2429,18 @@ config BITS_TEST
 
 	  If unsure, say N.
 
+config RATIONAL_KUNIT_TEST
+	tristate "KUnit test for rational.c" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	select RATIONAL
+	default KUNIT_ALL_TESTS
+	help
+	  This builds the rational math unit test.
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
diff --git a/lib/math/Makefile b/lib/math/Makefile
index 7456edb864fc..bfac26ddfc22 100644
--- a/lib/math/Makefile
+++ b/lib/math/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_PRIME_NUMBERS)	+= prime_numbers.o
 obj-$(CONFIG_RATIONAL)		+= rational.o
 
 obj-$(CONFIG_TEST_DIV64)	+= test_div64.o
+obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
diff --git a/lib/math/rational-test.c b/lib/math/rational-test.c
new file mode 100644
index 000000000000..f64166dbe9ea
--- /dev/null
+++ b/lib/math/rational-test.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/test.h>
+
+#include <linux/rational.h>
+
+struct rational_test_param {
+	unsigned long num, den;
+	unsigned long max_num, max_den;
+	unsigned long exp_num, exp_den;
+
+	const char *name;
+};
+
+static const struct rational_test_param test_parameters[] = {
+	{ 1230,	10,	100, 20,	100, 1,    "Exceeds bounds, semi-convergent term > ½ last term" },
+	{ 34567,100, 	120, 20,	120, 1,    "Exceeds bounds, semi-convergent term < ½ last term" },
+	{ 1, 30,	100, 10,	0, 1,	   "Closest to zero" },
+	{ 1, 19,	100, 10,	1, 10,     "Closest to smallest non-zero" },
+	{ 27,32,	16, 16,		11, 13,    "Use convergent" },
+	{ 1155, 7735,	255, 255,	33, 221,   "Exact answer" },
+	{ 87, 32,	70, 32,		68, 25,    "Semiconvergent, numerator limit" },
+	{ 14533, 4626,	15000, 2400,	7433, 2366, "Semiconvergent, demominator limit" },
+};
+
+static void get_desc(const struct rational_test_param *param, char *desc)
+{
+	strscpy(desc, param->name, KUNIT_PARAM_DESC_SIZE);
+}
+
+/* Creates function rational_gen_params */
+KUNIT_ARRAY_PARAM(rational, test_parameters, get_desc);
+
+static void rational_test(struct kunit *test)
+{
+	const struct rational_test_param *param = (const struct rational_test_param *)test->param_value;
+	unsigned long n = 0, d = 0;
+
+	rational_best_approximation(param->num, param->den, param->max_num, param->max_den, &n, &d);
+	KUNIT_EXPECT_EQ(test, n, param->exp_num);
+	KUNIT_EXPECT_EQ(test, d, param->exp_den);
+}
+
+static struct kunit_case rational_test_cases[] = {
+       KUNIT_CASE_PARAM(rational_test, rational_gen_params),
+       {}
+};
+
+static struct kunit_suite rational_test_suite = {
+       .name = "rational",
+       .test_cases = rational_test_cases,
+};
+
+kunit_test_suites(&rational_test_suite);
+
+MODULE_LICENSE("GPL v2");
-- 
2.26.2

Re: [PATCH v2 1/2] lib/math/rational.c: Fix divide by zero

[Andy Shevchenko]

On Tue, May 25, 2021 at 07:42:49AM -0700, Trent Piepho wrote:
> If the input is out of the range of the allowed values, either larger
> than the largest value or closer to zero than the smallest non-zero
> allowed value, then a division by zero would occur.
> 
> In the case of input too large, the division by zero will occur on the
> first iteration.  The best result (largest allowed value) will be found
> by always choosing the semi-convergent and excluding the denominator
> based limit when finding it.
> 
> In the case of the input too small, the division by zero will occur on
> the second iteration.  The numerator based semi-convergent should not be
> calculated to avoid the division by zero.  But the semi-convergent vs
> previous convergent test is still needed, which effectively chooses
> between 0 (the previous convergent) vs the smallest allowed fraction
> (best semi-convergent) as the result.

LGTM, thanks!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 323dd2c3ed0 ("lib/math/rational.c: fix possible incorrect result from rational fractions helper")
> Reported-by: Yiyuan Guo <yguoaz@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> ---
>  lib/math/rational.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/math/rational.c b/lib/math/rational.c
> index 9781d521963d..c0ab51d8fbb9 100644
> --- a/lib/math/rational.c
> +++ b/lib/math/rational.c
> @@ -12,6 +12,7 @@
>  #include <linux/compiler.h>
>  #include <linux/export.h>
>  #include <linux/minmax.h>
> +#include <linux/limits.h>
>  
>  /*
>   * calculate best rational approximation for a given fraction
> @@ -78,13 +79,18 @@ void rational_best_approximation(
>  		 * found below as 't'.
>  		 */
>  		if ((n2 > max_numerator) || (d2 > max_denominator)) {
> -			unsigned long t = min((max_numerator - n0) / n1,
> -					      (max_denominator - d0) / d1);
> +			unsigned long t = ULONG_MAX;
>  
> -			/* This tests if the semi-convergent is closer
> -			 * than the previous convergent.
> +			if (d1)
> +				t = (max_denominator - d0) / d1;
> +			if (n1)
> +				t = min(t, (max_numerator - n0) / n1);
> +
> +			/* This tests if the semi-convergent is closer than the previous
> +			 * convergent.  If d1 is zero there is no previous convergent as this
> +			 * is the 1st iteration, so always choose the semi-convergent.
>  			 */
> -			if (2u * t > a || (2u * t == a && d0 * dp > d1 * d)) {
> +			if (!d1 || 2u * t > a || (2u * t == a && d0 * dp > d1 * d)) {
>  				n1 = n0 + t * n1;
>  				d1 = d0 + t * d1;
>  			}
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Andy Shevchenko]

On Tue, May 25, 2021 at 07:42:50AM -0700, Trent Piepho wrote:
> Adds a number of test cases that cover a range of possible code paths.

Good for the starter!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> ---
>  lib/Kconfig.debug        | 12 +++++++++
>  lib/math/Makefile        |  1 +
>  lib/math/rational-test.c | 56 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+)
>  create mode 100644 lib/math/rational-test.c
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 678c13967580..6c0e66a7d416 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2429,6 +2429,18 @@ config BITS_TEST
>  
>  	  If unsure, say N.
>  
> +config RATIONAL_KUNIT_TEST
> +	tristate "KUnit test for rational.c" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	select RATIONAL
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This builds the rational math unit test.
> +	  For more information on KUnit and unit tests in general please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> +
>  config TEST_UDELAY
>  	tristate "udelay test driver"
>  	help
> diff --git a/lib/math/Makefile b/lib/math/Makefile
> index 7456edb864fc..bfac26ddfc22 100644
> --- a/lib/math/Makefile
> +++ b/lib/math/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_PRIME_NUMBERS)	+= prime_numbers.o
>  obj-$(CONFIG_RATIONAL)		+= rational.o
>  
>  obj-$(CONFIG_TEST_DIV64)	+= test_div64.o
> +obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
> diff --git a/lib/math/rational-test.c b/lib/math/rational-test.c
> new file mode 100644
> index 000000000000..f64166dbe9ea
> --- /dev/null
> +++ b/lib/math/rational-test.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/test.h>
> +
> +#include <linux/rational.h>
> +
> +struct rational_test_param {
> +	unsigned long num, den;
> +	unsigned long max_num, max_den;
> +	unsigned long exp_num, exp_den;
> +
> +	const char *name;
> +};
> +
> +static const struct rational_test_param test_parameters[] = {
> +	{ 1230,	10,	100, 20,	100, 1,    "Exceeds bounds, semi-convergent term > ½ last term" },
> +	{ 34567,100, 	120, 20,	120, 1,    "Exceeds bounds, semi-convergent term < ½ last term" },
> +	{ 1, 30,	100, 10,	0, 1,	   "Closest to zero" },
> +	{ 1, 19,	100, 10,	1, 10,     "Closest to smallest non-zero" },
> +	{ 27,32,	16, 16,		11, 13,    "Use convergent" },
> +	{ 1155, 7735,	255, 255,	33, 221,   "Exact answer" },
> +	{ 87, 32,	70, 32,		68, 25,    "Semiconvergent, numerator limit" },
> +	{ 14533, 4626,	15000, 2400,	7433, 2366, "Semiconvergent, demominator limit" },
> +};
> +
> +static void get_desc(const struct rational_test_param *param, char *desc)
> +{
> +	strscpy(desc, param->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +/* Creates function rational_gen_params */
> +KUNIT_ARRAY_PARAM(rational, test_parameters, get_desc);
> +
> +static void rational_test(struct kunit *test)
> +{
> +	const struct rational_test_param *param = (const struct rational_test_param *)test->param_value;
> +	unsigned long n = 0, d = 0;
> +
> +	rational_best_approximation(param->num, param->den, param->max_num, param->max_den, &n, &d);
> +	KUNIT_EXPECT_EQ(test, n, param->exp_num);
> +	KUNIT_EXPECT_EQ(test, d, param->exp_den);
> +}
> +
> +static struct kunit_case rational_test_cases[] = {
> +       KUNIT_CASE_PARAM(rational_test, rational_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite rational_test_suite = {
> +       .name = "rational",
> +       .test_cases = rational_test_cases,
> +};
> +
> +kunit_test_suites(&rational_test_suite);
> +
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Daniel Latypov]

On Tue, May 25, 2021 at 7:43 AM Trent Piepho <tpiepho@gmail.com> wrote:
>
> Adds a number of test cases that cover a range of possible code paths.
>
> Signed-off-by: Trent Piepho <tpiepho@gmail.com>

Reviewed-by: Daniel Latypov <dlatypov@google.com>

Looks really good to me, just two nits.

Tangent:
I didn't check to see that this covers all the interesting cases, but
it seems like it does.
If you want, you can try generating a code coverage report to double check.
Instructions for doing so can be found in
https://lore.kernel.org/linux-kselftest/20210414222256.1280532-1-dlatypov@google.com/
I would have done that and included the #s in this email, but my
workplace decided to subtly break my workstation in some way and I
haven't gotten around to root causing...

> ---
>  lib/Kconfig.debug        | 12 +++++++++
>  lib/math/Makefile        |  1 +
>  lib/math/rational-test.c | 56 ++++++++++++++++++++++++++++++++++++++++

Ah, sorry, I forgot to mention this in the previous email.
If you look at kunit/style.rst docs, you'll see the documentation now
states a preference for the name of this file to be one of
{rational_test.c, rational_kunit.c}

Unfortunately, we have a number of tests throughout the tree that
don't reflect this, including lib/kunit/kunit-example-test.c :(

>  3 files changed, 69 insertions(+)
>  create mode 100644 lib/math/rational-test.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 678c13967580..6c0e66a7d416 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2429,6 +2429,18 @@ config BITS_TEST
>
>           If unsure, say N.
>
> +config RATIONAL_KUNIT_TEST
> +       tristate "KUnit test for rational.c" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       select RATIONAL
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds the rational math unit test.
> +         For more information on KUnit and unit tests in general please refer
> +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +         If unsure, say N.
> +
>  config TEST_UDELAY
>         tristate "udelay test driver"
>         help
> diff --git a/lib/math/Makefile b/lib/math/Makefile
> index 7456edb864fc..bfac26ddfc22 100644
> --- a/lib/math/Makefile
> +++ b/lib/math/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_PRIME_NUMBERS)     += prime_numbers.o
>  obj-$(CONFIG_RATIONAL)         += rational.o
>
>  obj-$(CONFIG_TEST_DIV64)       += test_div64.o
> +obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
> diff --git a/lib/math/rational-test.c b/lib/math/rational-test.c
> new file mode 100644
> index 000000000000..f64166dbe9ea
> --- /dev/null
> +++ b/lib/math/rational-test.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/test.h>
> +
> +#include <linux/rational.h>
> +
> +struct rational_test_param {
> +       unsigned long num, den;
> +       unsigned long max_num, max_den;
> +       unsigned long exp_num, exp_den;
> +
> +       const char *name;
> +};
> +
> +static const struct rational_test_param test_parameters[] = {
> +       { 1230, 10,     100, 20,        100, 1,    "Exceeds bounds, semi-convergent term > ½ last term" },
> +       { 34567,100,    120, 20,        120, 1,    "Exceeds bounds, semi-convergent term < ½ last term" },

very minor nit: there's an extraneous space character after the "100,"

> +       { 1, 30,        100, 10,        0, 1,      "Closest to zero" },
> +       { 1, 19,        100, 10,        1, 10,     "Closest to smallest non-zero" },
> +       { 27,32,        16, 16,         11, 13,    "Use convergent" },
> +       { 1155, 7735,   255, 255,       33, 221,   "Exact answer" },
> +       { 87, 32,       70, 32,         68, 25,    "Semiconvergent, numerator limit" },
> +       { 14533, 4626,  15000, 2400,    7433, 2366, "Semiconvergent, demominator limit" },
> +};
> +
> +static void get_desc(const struct rational_test_param *param, char *desc)
> +{
> +       strscpy(desc, param->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +/* Creates function rational_gen_params */
> +KUNIT_ARRAY_PARAM(rational, test_parameters, get_desc);
> +
> +static void rational_test(struct kunit *test)
> +{
> +       const struct rational_test_param *param = (const struct rational_test_param *)test->param_value;
> +       unsigned long n = 0, d = 0;
> +
> +       rational_best_approximation(param->num, param->den, param->max_num, param->max_den, &n, &d);
> +       KUNIT_EXPECT_EQ(test, n, param->exp_num);
> +       KUNIT_EXPECT_EQ(test, d, param->exp_den);
> +}
> +
> +static struct kunit_case rational_test_cases[] = {
> +       KUNIT_CASE_PARAM(rational_test, rational_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite rational_test_suite = {
> +       .name = "rational",
> +       .test_cases = rational_test_cases,
> +};
> +
> +kunit_test_suites(&rational_test_suite);
> +
> +MODULE_LICENSE("GPL v2");
> --
> 2.26.2
>

Re: [PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Trent Piepho]

On Tue, May 25, 2021 at 10:34 AM Daniel Latypov <dlatypov@google.com> wrote:
> On Tue, May 25, 2021 at 7:43 AM Trent Piepho <tpiepho@gmail.com> wrote:
> > Adds a number of test cases that cover a range of possible code paths.
> >
> > Signed-off-by: Trent Piepho <tpiepho@gmail.com>
>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
>
> Looks really good to me, just two nits.
>
> Tangent:
> I didn't check to see that this covers all the interesting cases, but
> it seems like it does.
> If you want, you can try generating a code coverage report to double check.
> Instructions for doing so can be found in
> https://lore.kernel.org/linux-kselftest/20210414222256.1280532-1-dlatypov@google.com/
> I would have done that and included the #s in this email, but my
> workplace decided to subtly break my workstation in some way and I
> haven't gotten around to root causing...

I installed a gcc 6.4 toolchain and changed the uml_abort() call to
exit(), coverage was generated, but still truncated and incorrectly
near 0%.  So what I did was crash after running all the test cases I
cared about by dividing by zero and then coverage data was produced
correctly.  It's 100% by lines.  But I think both possibilities when
the largest semiconvergent is exactly half the previous convergent
aren't tested.

> >  lib/math/rational-test.c | 56 ++++++++++++++++++++++++++++++++++++++++
>
> Ah, sorry, I forgot to mention this in the previous email.
> If you look at kunit/style.rst docs, you'll see the documentation now
> states a preference for the name of this file to be one of
> {rational_test.c, rational_kunit.c}

Before I chose a name, I checked every file with kunit tests cases,
and *-test was the most common naming pattern, including the sample
case.  I would be nice if changing the docs to say something is a
standard also updated the code to make that reality.

Re: [PATCH v2 1/2] lib/math/rational.c: Fix divide by zero

[Andrew Morton]

On Tue, 25 May 2021 07:42:49 -0700 Trent Piepho <tpiepho@gmail.com> wrote:

> If the input is out of the range of the allowed values, either larger
> than the largest value or closer to zero than the smallest non-zero
> allowed value, then a division by zero would occur.
> 
> In the case of input too large, the division by zero will occur on the
> first iteration.  The best result (largest allowed value) will be found
> by always choosing the semi-convergent and excluding the denominator
> based limit when finding it.
> 
> In the case of the input too small, the division by zero will occur on
> the second iteration.  The numerator based semi-convergent should not be
> calculated to avoid the division by zero.  But the semi-convergent vs
> previous convergent test is still needed, which effectively chooses
> between 0 (the previous convergent) vs the smallest allowed fraction
> (best semi-convergent) as the result.

Is there any known userspace workload which can trigger this?

IOW, should it be backported into -stable and fast-tracked into 5.13 or
will a 5.14 merge suffice?

Re: [PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Andrew Morton]

On Tue, 25 May 2021 07:42:50 -0700 Trent Piepho <tpiepho@gmail.com> wrote:

> Adds a number of test cases that cover a range of possible code paths.
> 
> --- /dev/null
> +++ b/lib/math/rational-test.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/test.h>
> +
> +#include <linux/rational.h>
> +
> +struct rational_test_param {
> +	unsigned long num, den;
> +	unsigned long max_num, max_den;
> +	unsigned long exp_num, exp_den;
> +
> +	const char *name;
> +};
> +
> +static const struct rational_test_param test_parameters[] = {
> +	{ 1230,	10,	100, 20,	100, 1,    "Exceeds bounds, semi-convergent term > ½ last term" },
> +	{ 34567,100, 	120, 20,	120, 1,    "Exceeds bounds, semi-convergent term < ½ last term" },

It seems to be asking for trouble to use these characters in kernel
output - heaven knows what sort of weird output devices people
are using.  So I think I'll switch to "1/2", OK?

> +	{ 1, 30,	100, 10,	0, 1,	   "Closest to zero" },
> +	{ 1, 19,	100, 10,	1, 10,     "Closest to smallest non-zero" },
> +	{ 27,32,	16, 16,		11, 13,    "Use convergent" },
> +	{ 1155, 7735,	255, 255,	33, 221,   "Exact answer" },
> +	{ 87, 32,	70, 32,		68, 25,    "Semiconvergent, numerator limit" },
> +	{ 14533, 4626,	15000, 2400,	7433, 2366, "Semiconvergent, demominator limit" },
> +};
> ...
> +static struct kunit_case rational_test_cases[] = {
> +       KUNIT_CASE_PARAM(rational_test, rational_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite rational_test_suite = {
> +       .name = "rational",
> +       .test_cases = rational_test_cases,
> +};

And let's use tabs to indent here.  checkpatch detects this.

Re: [PATCH v2 2/2] lib/math/rational: Add Kunit test cases

[Daniel Latypov]

On Tue, May 25, 2021 at 2:09 PM Trent Piepho <tpiepho@gmail.com> wrote:
>
> On Tue, May 25, 2021 at 10:34 AM Daniel Latypov <dlatypov@google.com> wrote:
> > On Tue, May 25, 2021 at 7:43 AM Trent Piepho <tpiepho@gmail.com> wrote:
> > > Adds a number of test cases that cover a range of possible code paths.
> > >
> > > Signed-off-by: Trent Piepho <tpiepho@gmail.com>
> >
> > Reviewed-by: Daniel Latypov <dlatypov@google.com>
> >
> > Looks really good to me, just two nits.
> >
> > Tangent:
> > I didn't check to see that this covers all the interesting cases, but
> > it seems like it does.
> > If you want, you can try generating a code coverage report to double check.
> > Instructions for doing so can be found in
> > https://lore.kernel.org/linux-kselftest/20210414222256.1280532-1-dlatypov@google.com/
> > I would have done that and included the #s in this email, but my
> > workplace decided to subtly break my workstation in some way and I
> > haven't gotten around to root causing...
>
> I installed a gcc 6.4 toolchain and changed the uml_abort() call to
> exit(), coverage was generated, but still truncated and incorrectly
> near 0%.  So what I did was crash after running all the test cases I
> cared about by dividing by zero and then coverage data was produced
> correctly.  It's 100% by lines.  But I think both possibilities when
> the largest semiconvergent is exactly half the previous convergent
> aren't tested.

Yeah, it's finicky.
Hopefully Brendan's change to cleanly shutdown the kernel at the end
of the tests might make it a bit more robust (still probably requires
gcc-6).

Thanks for running it!
It's nice to know we have 100% (line) coverage.

>
> > >  lib/math/rational-test.c | 56 ++++++++++++++++++++++++++++++++++++++++
> >
> > Ah, sorry, I forgot to mention this in the previous email.
> > If you look at kunit/style.rst docs, you'll see the documentation now
> > states a preference for the name of this file to be one of
> > {rational_test.c, rational_kunit.c}
>
> Before I chose a name, I checked every file with kunit tests cases,
> and *-test was the most common naming pattern, including the sample
> case.  I would be nice if changing the docs to say something is a
> standard also updated the code to make that reality.

Yeah, it's frustrating...
Andy sent out some changes renaming a lot of the files, but we've not
managed to get everything up to date.

Re: [PATCH v2 1/2] lib/math/rational.c: Fix divide by zero

[Andy Shevchenko]

On Tue, May 25, 2021 at 02:46:54PM -0700, Andrew Morton wrote:
> On Tue, 25 May 2021 07:42:49 -0700 Trent Piepho <tpiepho@gmail.com> wrote:
> 
> > If the input is out of the range of the allowed values, either larger
> > than the largest value or closer to zero than the smallest non-zero
> > allowed value, then a division by zero would occur.
> > 
> > In the case of input too large, the division by zero will occur on the
> > first iteration.  The best result (largest allowed value) will be found
> > by always choosing the semi-convergent and excluding the denominator
> > based limit when finding it.
> > 
> > In the case of the input too small, the division by zero will occur on
> > the second iteration.  The numerator based semi-convergent should not be
> > calculated to avoid the division by zero.  But the semi-convergent vs
> > previous convergent test is still needed, which effectively chooses
> > between 0 (the previous convergent) vs the smallest allowed fraction
> > (best semi-convergent) as the result.
> 
> Is there any known userspace workload which can trigger this?

Doubtfully.

What I can imagine (which also can be impossible) is to try on some UART
drivers that use this algorithm to ask crazy baud rates (much higher than the
clock). Sounds like a theoretical possibility is present, but practically
no one except bad hackers would do that (requires a special code to be written
in user space to select BOTHER with an arbitrary input in termios).

> IOW, should it be backported into -stable and fast-tracked into 5.13 or
> will a 5.14 merge suffice?

-- 
With Best Regards,
Andy Shevchenko