[dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Jie Zhou]

From: Jie Zhou <jizh@microsoft.com>

lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
on Windows do not check parameters to fail fast for invalid
parameters, which captured by DPDK UT alarm_autotest.

Enforce Windows lib/eal alarm APIs parameters check and log
invalid parameter info.

Signed-off-by: Jie Zhou <jizh@microsoft.com>
Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>

---
 lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index f5bf88715a..7bb79ae869 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -4,6 +4,7 @@
 
 #include <stdatomic.h>
 #include <stdbool.h>
+#include <inttypes.h>
 
 #include <rte_alarm.h>
 #include <rte_spinlock.h>
@@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	LARGE_INTEGER deadline;
 	int ret;
 
+	/* Check if us is valid */
+	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
+		RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
+			"Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
+			us);
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	/* Check if callback is not NULL */
+	if (!cb_fn) {
+		RTE_LOG(ERR, EAL, "NULL callback\n");
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	/* Calculate deadline ASAP, unit of measure = 100ns. */
 	GetSystemTimePreciseAsFileTime(&ft);
 	deadline.LowPart = ft.dwLowDateTime;
@@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 	bool executing;
 
 	removed = 0;
+
+	if (!cb_fn) {
+		RTE_LOG(ERR, EAL, "NULL callback\n");
+		return -EINVAL;
+	}
+
 	do {
 		executing = false;
 
-- 
2.31.0.vfs.0.1

Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Dmitry Kozlyuk]

Hi Jie,

2021-06-23 17:36 (UTC-0700), Jie Zhou:
> From: Jie Zhou <jizh@microsoft.com>
> 
> lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> on Windows do not check parameters to fail fast for invalid
> parameters, which captured by DPDK UT alarm_autotest.

Please use past tense to describe situation before the patch.
A nit, but browsing the log, I see that errors are usually "caught"
rather then "captured"; consistency would be nice.

> 
> Enforce Windows lib/eal alarm APIs parameters check and log
> invalid parameter info.

Fixes tag needed.

> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> 
> ---
>  lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> index f5bf88715a..7bb79ae869 100644
> --- a/lib/eal/windows/eal_alarm.c
> +++ b/lib/eal/windows/eal_alarm.c
> @@ -4,6 +4,7 @@
>  
>  #include <stdatomic.h>
>  #include <stdbool.h>
> +#include <inttypes.h>
>  
>  #include <rte_alarm.h>
>  #include <rte_spinlock.h>
> @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	LARGE_INTEGER deadline;
>  	int ret;
>  
> +	/* Check if us is valid */
> +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {

This condition is specific to Linux EAL. In fact, it's not very useful even
there, because actual upper bound for `us` depends on current time.
No bounds are specified in API description at all.
Windows check would be different, but these considerations remain valid.

Maybe it's alarm_autotest or API description that needs adjustments,
but not the implementation. I understand that you're enabling UT for Windows
and not correcting tests themselves, but I'm against inserting checks known
to be incorrect.

> +		RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> +			"Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> +			us);

Why does Windows need these messages, while Linux and FreeBSD don't?
How will printing API contract here help the user who gets the message?

> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	/* Check if callback is not NULL */
> +	if (!cb_fn) {

Pointers (`cb_fn`) must be checked for `NULL` explicitly.
You won't need an obvious comment after that.

> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>  	/* Calculate deadline ASAP, unit of measure = 100ns. */
>  	GetSystemTimePreciseAsFileTime(&ft);
>  	deadline.LowPart = ft.dwLowDateTime;
> @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	bool executing;
>  
>  	removed = 0;
> +
> +	if (!cb_fn) {
> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		return -EINVAL;
> +	}
> +
>  	do {
>  		executing = false;
>  

Please also fix other style issues:
http://mails.dpdk.org/archives/test-report/2021-June/200580.html

Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Tyler Retzlaff]

On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> Hi Jie,
> 
> 2021-06-23 17:36 (UTC-0700), Jie Zhou:
> > From: Jie Zhou <jizh@microsoft.com>
> > 
> > lib/eal alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> > on Windows do not check parameters to fail fast for invalid
> > parameters, which captured by DPDK UT alarm_autotest.
> 
> Please use past tense to describe situation before the patch.
> A nit, but browsing the log, I see that errors are usually "caught"
> rather then "captured"; consistency would be nice.
> 
> > 
> > Enforce Windows lib/eal alarm APIs parameters check and log
> > invalid parameter info.
> 
> Fixes tag needed.
> 
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > 
> > ---
> >  lib/eal/windows/eal_alarm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> > index f5bf88715a..7bb79ae869 100644
> > --- a/lib/eal/windows/eal_alarm.c
> > +++ b/lib/eal/windows/eal_alarm.c
> > @@ -4,6 +4,7 @@
> >  
> >  #include <stdatomic.h>
> >  #include <stdbool.h>
> > +#include <inttypes.h>
> >  
> >  #include <rte_alarm.h>
> >  #include <rte_spinlock.h>
> > @@ -91,6 +92,22 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	LARGE_INTEGER deadline;
> >  	int ret;
> >  
> > +	/* Check if us is valid */
> > +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {
> 
> This condition is specific to Linux EAL. In fact, it's not very useful even
> there, because actual upper bound for `us` depends on current time.
> No bounds are specified in API description at all.
> Windows check would be different, but these considerations remain valid.
> 
> Maybe it's alarm_autotest or API description that needs adjustments,
> but not the implementation.

i agree with your assessment. it's a bit silly to test a range
constraint where the range is just some arbitrary range and not the real
range. changing the implementation to calculate the "real" valid range
isn't practical.  i'm sure linux implementors would argue that catching
some extremely out of range values is better than none?

i guess a correct test would would calculate the valid range and test
against that but as per above the implementation won't pass the test.

so we are left with

* matching the range imposed in the linux implementation so we pass the
  test as is.

* don't run the test with the input data that exercises this bogus range
  constraint.

i guess based on your comments you prefer the latter? so is our action
here to submit a patch for the test that doesn't test this range
conditionally on execenv windows?

> I understand that you're enabling UT for Windows
> and not correcting tests themselves, but I'm against inserting checks known
> to be incorrect.
> 
> > +		RTE_LOG(ERR, EAL, "Invalid us: %" PRIu64 "\n"
> > +			"Valid us range is 1 to (UINT64_MAX - US_PER_S)\n",
> > +			us);
> 
> Why does Windows need these messages, while Linux and FreeBSD don't?
> How will printing API contract here help the user who gets the message?

i'll discuss this subject to whether or not we remove the above range
check. for now i'll defer discussion.

> 
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	/* Check if callback is not NULL */
> > +	if (!cb_fn) {
> 
> Pointers (`cb_fn`) must be checked for `NULL` explicitly.
> You won't need an obvious comment after that.
> 
> > +		RTE_LOG(ERR, EAL, "NULL callback\n");
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> >  	/* Calculate deadline ASAP, unit of measure = 100ns. */
> >  	GetSystemTimePreciseAsFileTime(&ft);
> >  	deadline.LowPart = ft.dwLowDateTime;
> > @@ -180,6 +197,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
> >  	bool executing;
> >  
> >  	removed = 0;
> > +
> > +	if (!cb_fn) {
> > +		RTE_LOG(ERR, EAL, "NULL callback\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	do {
> >  		executing = false;
> >  
> 
> Please also fix other style issues:
> http://mails.dpdk.org/archives/test-report/2021-June/200580.html

Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Dmitry Kozlyuk]

2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > Hi Jie,
> > 
> > 2021-06-23 17:36 (UTC-0700), Jie Zhou:  
> > > From: Jie Zhou <jizh@microsoft.com>
> [...]
> > > +	/* Check if us is valid */
> > > +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {  
> > 
> > This condition is specific to Linux EAL. In fact, it's not very useful even
> > there, because actual upper bound for `us` depends on current time.
> > No bounds are specified in API description at all.
> > Windows check would be different, but these considerations remain valid.
> > 
> > Maybe it's alarm_autotest or API description that needs adjustments,
> > but not the implementation.  
> 
> i agree with your assessment. it's a bit silly to test a range
> constraint where the range is just some arbitrary range and not the real
> range. changing the implementation to calculate the "real" valid range
> isn't practical.  i'm sure linux implementors would argue that catching
> some extremely out of range values is better than none?

Why we care about overflow?
1. It likely indicates a bug in the app.
2. Alarm is can to be scheduled to some time in the past and fire
immediately, which means API did not do what it promises.
I see the only way to tell the interval is incorrect if it gives
(would give) time in the past when added to the current time.
But this is an implementation detail and does not need testing.
A separate patch can be submitted to change behavior for all OS.

> 
> i guess a correct test would would calculate the valid range and test
> against that but as per above the implementation won't pass the test.
> 
> so we are left with
> 
> * matching the range imposed in the linux implementation so we pass the
>   test as is.

It would be the worst thing one could do, defying the purpose of unit tests.

> * don't run the test with the input data that exercises this bogus range
>   constraint.
> 
> i guess based on your comments you prefer the latter? so is our action
> here to submit a patch for the test that doesn't test this range
> conditionally on execenv windows?

Yes, please remove this check from the test as it verifies no contract.

Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Tyler Retzlaff]

On Fri, Jul 02, 2021 at 12:36:45AM +0300, Dmitry Kozlyuk wrote:
> 2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> > On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > > Hi Jie,
> > > 
> > > 2021-06-23 17:36 (UTC-0700), Jie Zhou:  
> > > > From: Jie Zhou <jizh@microsoft.com>
> > [...]
> > > > +	/* Check if us is valid */
> > > > +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {  
> > > 
> > > This condition is specific to Linux EAL. In fact, it's not very useful even
> > > there, because actual upper bound for `us` depends on current time.
> > > No bounds are specified in API description at all.
> > > Windows check would be different, but these considerations remain valid.
> > > 
> > > Maybe it's alarm_autotest or API description that needs adjustments,
> > > but not the implementation.  
> > 
> > i agree with your assessment. it's a bit silly to test a range
> > constraint where the range is just some arbitrary range and not the real
> > range. changing the implementation to calculate the "real" valid range
> > isn't practical.  i'm sure linux implementors would argue that catching
> > some extremely out of range values is better than none?
> 
> Why we care about overflow?
> 1. It likely indicates a bug in the app.
> 2. Alarm is can to be scheduled to some time in the past and fire
> immediately, which means API did not do what it promises.
> I see the only way to tell the interval is incorrect if it gives
> (would give) time in the past when added to the current time.
> But this is an implementation detail and does not need testing.
> A separate patch can be submitted to change behavior for all OS.
> 
> > 
> > i guess a correct test would would calculate the valid range and test
> > against that but as per above the implementation won't pass the test.
> > 
> > so we are left with
> > 
> > * matching the range imposed in the linux implementation so we pass the
> >   test as is.
> 
> It would be the worst thing one could do, defying the purpose of unit tests.

agreed.

> 
> > * don't run the test with the input data that exercises this bogus range
> >   constraint.
> > 
> > i guess based on your comments you prefer the latter? so is our action
> > here to submit a patch for the test that doesn't test this range
> > conditionally on execenv windows?
> 
> Yes, please remove this check from the test as it verifies no contract.

we'll go with this.

thanks

Re: [dpdk-dev] [PATCH v1] lib/eal: enforce alarm APIs parameters check

[Jie Zhou]

On Thu, Jul 01, 2021 at 02:45:24PM -0700, Tyler Retzlaff wrote:
> On Fri, Jul 02, 2021 at 12:36:45AM +0300, Dmitry Kozlyuk wrote:
> > 2021-07-01 09:21 (UTC-0700), Tyler Retzlaff:
> > > On Thu, Jul 01, 2021 at 02:31:29AM +0300, Dmitry Kozlyuk wrote:
> > > > Hi Jie,
> > > > 
> > > > 2021-06-23 17:36 (UTC-0700), Jie Zhou:  
> > > > > From: Jie Zhou <jizh@microsoft.com>
> > > [...]
> > > > > +	/* Check if us is valid */
> > > > > +	if (us < 1 || us >(UINT64_MAX - US_PER_S)) {  
> > > > 
> > > > This condition is specific to Linux EAL. In fact, it's not very useful even
> > > > there, because actual upper bound for `us` depends on current time.
> > > > No bounds are specified in API description at all.
> > > > Windows check would be different, but these considerations remain valid.
> > > > 
> > > > Maybe it's alarm_autotest or API description that needs adjustments,
> > > > but not the implementation.  
> > > 
> > > i agree with your assessment. it's a bit silly to test a range
> > > constraint where the range is just some arbitrary range and not the real
> > > range. changing the implementation to calculate the "real" valid range
> > > isn't practical.  i'm sure linux implementors would argue that catching
> > > some extremely out of range values is better than none?
> > 
> > Why we care about overflow?
> > 1. It likely indicates a bug in the app.
> > 2. Alarm is can to be scheduled to some time in the past and fire
> > immediately, which means API did not do what it promises.
> > I see the only way to tell the interval is incorrect if it gives
> > (would give) time in the past when added to the current time.
> > But this is an implementation detail and does not need testing.
> > A separate patch can be submitted to change behavior for all OS.
> > 
> > > 
> > > i guess a correct test would would calculate the valid range and test
> > > against that but as per above the implementation won't pass the test.
> > > 
> > > so we are left with
> > > 
> > > * matching the range imposed in the linux implementation so we pass the
> > >   test as is.
> > 
> > It would be the worst thing one could do, defying the purpose of unit tests.
> 
> agreed.
> 
> > 
> > > * don't run the test with the input data that exercises this bogus range
> > >   constraint.
> > > 
> > > i guess based on your comments you prefer the latter? so is our action
> > > here to submit a patch for the test that doesn't test this range
> > > conditionally on execenv windows?
> > 
> > Yes, please remove this check from the test as it verifies no contract.
> 
> we'll go with this.
> 
> thanks

Thanks Dmitry and Tyler. Will address these in V2.

[dpdk-dev] [PATCH v2] eal/windows: enforce alarm APIs parameter check

[Jie Zhou]

eal/windows alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
did not check parameters to fail fast for invalid parameters, which
caught by DPDK UT alarm_autotest.

Enforce eal/windows alarm APIs parameter check to fail fast for
invalid parameters.

Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
Cc: stable@dpdk.org

Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>

---
V2 changes:
    - Remove API parameter check on arbitrary 'us' range
    - Do explicit NULL cb_fn check

---
 lib/eal/windows/eal_alarm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index f5bf88715a..e5dc54efb8 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -91,6 +91,12 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
 	LARGE_INTEGER deadline;
 	int ret;
 
+	if (cb_fn == NULL) {
+		RTE_LOG(ERR, EAL, "NULL callback\n");
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	/* Calculate deadline ASAP, unit of measure = 100ns. */
 	GetSystemTimePreciseAsFileTime(&ft);
 	deadline.LowPart = ft.dwLowDateTime;
@@ -180,6 +186,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 	bool executing;
 
 	removed = 0;
+
+	if (cb_fn == NULL) {
+		RTE_LOG(ERR, EAL, "NULL callback\n");
+		return -EINVAL;
+	}
+
 	do {
 		executing = false;
 
-- 
2.31.0.vfs.0.1

Re: [dpdk-dev] [PATCH v2] eal/windows: enforce alarm APIs parameter check

[Dmitry Kozlyuk]

2021-07-07 13:25 (UTC-0700), Jie Zhou:
> eal/windows alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> did not check parameters to fail fast for invalid parameters, which
> caught by DPDK UT alarm_autotest.
> 
> Enforce eal/windows alarm APIs parameter check to fail fast for
> invalid parameters.
> 
> Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> 
> ---
> V2 changes:
>     - Remove API parameter check on arbitrary 'us' range
>     - Do explicit NULL cb_fn check
> 
> ---
>  lib/eal/windows/eal_alarm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
> index f5bf88715a..e5dc54efb8 100644
> --- a/lib/eal/windows/eal_alarm.c
> +++ b/lib/eal/windows/eal_alarm.c
> @@ -91,6 +91,12 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	LARGE_INTEGER deadline;
>  	int ret;
>  
> +	if (cb_fn == NULL) {
> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
>  	/* Calculate deadline ASAP, unit of measure = 100ns. */
>  	GetSystemTimePreciseAsFileTime(&ft);
>  	deadline.LowPart = ft.dwLowDateTime;
> @@ -180,6 +186,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
>  	bool executing;
>  
>  	removed = 0;
> +
> +	if (cb_fn == NULL) {
> +		RTE_LOG(ERR, EAL, "NULL callback\n");
> +		return -EINVAL;
> +	}
> +
>  	do {
>  		executing = false;
>  

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v2] eal/windows: enforce alarm APIs parameter check

[Thomas Monjalon]

21/07/2021 17:28, Dmitry Kozlyuk:
> 2021-07-07 13:25 (UTC-0700), Jie Zhou:
> > eal/windows alarm APIs rte_eal_alarm_set and rte_eal_alarm_cancel
> > did not check parameters to fail fast for invalid parameters, which
> > caught by DPDK UT alarm_autotest.
> > 
> > Enforce eal/windows alarm APIs parameter check to fail fast for
> > invalid parameters.
> > 
> > Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> 
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Applied with title "eal/windows: check callback parameter of alarm functions"