[PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

Check the FILE *f and rte_mempool *mp pointers for NULL and
return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.

Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
---
 lib/librte_mempool/rte_mempool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 332f469..efa6a6c 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
    unsigned common_count;
    unsigned cache_count;

+   if ( (f == NULL) || (mp == NULL) ) {
+#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
+       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
+#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
+       return;
+   }
    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
    fprintf(f, "  flags=%x\n", mp->flags);
    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
-- 
2.1.0

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
> 
> Check the FILE *f and rte_mempool *mp pointers for NULL and
> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> 
> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> ---
>  lib/librte_mempool/rte_mempool.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 332f469..efa6a6c 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>     unsigned common_count;
>     unsigned cache_count;
> 
> +   if ( (f == NULL) || (mp == NULL) ) {
> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
> +       return;
> +   }
>     fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>     fprintf(f, "  flags=%x\n", mp->flags);
>     fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> -- 
> 2.1.0
> 
> 
Maybe use RTE_VERIFY instead?
Neil

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

OK, let me look at RTE_VERIFY then and will send a new patch.

On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
>> 
>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
>> ---
>> lib/librte_mempool/rte_mempool.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 332f469..efa6a6c 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>>    unsigned common_count;
>>    unsigned cache_count;
>> 
>> +   if ( (f == NULL) || (mp == NULL) ) {
>> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
>> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>> +       return;
>> +   }
>>    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>>    fprintf(f, "  flags=%x\n", mp->flags);
>>    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
>> -- 
>> 2.1.0
>> 
>> 
> Maybe use RTE_VERIFY instead?
> Neil
> 

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
>> 
>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
>> ---
>> lib/librte_mempool/rte_mempool.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 332f469..efa6a6c 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>>    unsigned common_count;
>>    unsigned cache_count;
>> 
>> +   if ( (f == NULL) || (mp == NULL) ) {
>> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
>> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>> +       return;
>> +   }
>>    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>>    fprintf(f, "  flags=%x\n", mp->flags);
>>    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
>> -- 
>> 2.1.0
>> 
>> 
> Maybe use RTE_VERIFY instead?
> Neil
> 
I did not think it needs to panic as it is just a debug function and returning would be fine by me, comments?
Do we have a similar RTE_VERIFY like function that does not panic?

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Sun, Sep 28, 2014 at 01:14:05AM +0000, Wiles, Roger Keith wrote:
> 
> On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> 
> > On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
> >> 
> >> Check the FILE *f and rte_mempool *mp pointers for NULL and
> >> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> >> 
> >> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> >> ---
> >> lib/librte_mempool/rte_mempool.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >> index 332f469..efa6a6c 100644
> >> --- a/lib/librte_mempool/rte_mempool.c
> >> +++ b/lib/librte_mempool/rte_mempool.c
> >> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
> >>    unsigned common_count;
> >>    unsigned cache_count;
> >> 
> >> +   if ( (f == NULL) || (mp == NULL) ) {
> >> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> >> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
> >> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
> >> +       return;
> >> +   }
> >>    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
> >>    fprintf(f, "  flags=%x\n", mp->flags);
> >>    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> >> -- 
> >> 2.1.0
> >> 
> >> 
> > Maybe use RTE_VERIFY instead?
> > Neil
> > 
> I did not think it needs to panic as it is just a debug function and returning would be fine by me, comments?
> Do we have a similar RTE_VERIFY like function that does not panic?
> 
If we don't, it would seem useful to make one.  It beats having to do specific
condition checking/error reporting.  RTE_VERIFY_WARN or some such.
Neil

> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> 

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

On Sep 27, 2014, at 8:55 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Sun, Sep 28, 2014 at 01:14:05AM +0000, Wiles, Roger Keith wrote:
>> 
>> On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
>> 
>>> On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
>>>> 
>>>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>>>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>>>> 
>>>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
>>>> ---
>>>> lib/librte_mempool/rte_mempool.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>> 
>>>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>>>> index 332f469..efa6a6c 100644
>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>>>>   unsigned common_count;
>>>>   unsigned cache_count;
>>>> 
>>>> +   if ( (f == NULL) || (mp == NULL) ) {
>>>> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>>>> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
>>>> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>>>> +       return;
>>>> +   }
>>>>   fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>>>>   fprintf(f, "  flags=%x\n", mp->flags);
>>>>   fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
>>>> -- 
>>>> 2.1.0
>>>> 
>>>> 
>>> Maybe use RTE_VERIFY instead?
>>> Neil
>>> 
>> I did not think it needs to panic as it is just a debug function and returning would be fine by me, comments?
>> Do we have a similar RTE_VERIFY like function that does not panic?
>> 
> If we don't, it would seem useful to make one.  It beats having to do specific
> condition checking/error reporting.  RTE_VERIFY_WARN or some such.
> Neil

I decided to just use RTE_VERIFY() instead of creating a new macro for now, it seems this maybe an isolated case. I agree having RTE_VERIFY_WARN() would be nice, but as I was writing the macro I wanted to return from the function. For this routine ‘return’ would work as it returns (void), but for other routines a value may need to be returned.

Need a clean way to exit the routine without causing the macro to understand its return values. Just seem to become a bit messy at this point. Multiple macros for different return types or make the macros return a boolean value to be tested seemed to more complex then needed.
> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Sun, Sep 28, 2014 at 05:38:06AM +0000, Wiles, Roger Keith wrote:
> 
> On Sep 27, 2014, at 8:55 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> 
> > On Sun, Sep 28, 2014 at 01:14:05AM +0000, Wiles, Roger Keith wrote:
> >> 
> >> On Sep 27, 2014, at 7:37 PM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:
> >> 
> >>> On Sat, Sep 27, 2014 at 06:35:01PM +0000, Wiles, Roger Keith wrote:
> >>>> 
> >>>> Check the FILE *f and rte_mempool *mp pointers for NULL and
> >>>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> >>>> 
> >>>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> >>>> ---
> >>>> lib/librte_mempool/rte_mempool.c | 6 ++++++
> >>>> 1 file changed, 6 insertions(+)
> >>>> 
> >>>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >>>> index 332f469..efa6a6c 100644
> >>>> --- a/lib/librte_mempool/rte_mempool.c
> >>>> +++ b/lib/librte_mempool/rte_mempool.c
> >>>> @@ -765,6 +765,12 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
> >>>>   unsigned common_count;
> >>>>   unsigned cache_count;
> >>>> 
> >>>> +   if ( (f == NULL) || (mp == NULL) ) {
> >>>> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> >>>> +       fprintf(stderr, "*** Called rte_mempool_dump(%p, %p) with NULL argument\n", f, mp);
> >>>> +#endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
> >>>> +       return;
> >>>> +   }
> >>>>   fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
> >>>>   fprintf(f, "  flags=%x\n", mp->flags);
> >>>>   fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> >>>> -- 
> >>>> 2.1.0
> >>>> 
> >>>> 
> >>> Maybe use RTE_VERIFY instead?
> >>> Neil
> >>> 
> >> I did not think it needs to panic as it is just a debug function and returning would be fine by me, comments?
> >> Do we have a similar RTE_VERIFY like function that does not panic?
> >> 
> > If we don't, it would seem useful to make one.  It beats having to do specific
> > condition checking/error reporting.  RTE_VERIFY_WARN or some such.
> > Neil
> 
> I decided to just use RTE_VERIFY() instead of creating a new macro for now, it seems this maybe an isolated case. I agree having RTE_VERIFY_WARN() would be nice, but as I was writing the macro I wanted to return from the function. For this routine ‘return’ would work as it returns (void), but for other routines a value may need to be returned.
> 
Thats fine, you can do exactly what you need to do, just write the macro to
assert !!condition at the end, like this:
#define RTE_VERIFY_WARN(condition) do { \
    int ret = !!condition; \
    if (ret) \
        printf(<message>); \
    ret;\
}

Then, you can use the macro as a conditional itself anywhere you want:

int function(void *arguments)
{
    if (RTE_VERIFY(arguments == NULL))
        return 1
....
}

> Need a clean way to exit the routine without causing the macro to understand its return values. Just seem to become a bit messy at this point. Multiple macros for different return types or make the macros return a boolean value to be tested seemed to more complex then needed.
See above, thats how all the Linux WARN_ON macros work.

Neil

> > 
> >> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> 

[PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

Check the FILE *f and rte_mempool *mp pointers for NULL and
return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.

Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
---
 lib/librte_mempool/rte_mempool.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 332f469..0f71f10 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -765,6 +765,9 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
    unsigned common_count;
    unsigned cache_count;

+   RTE_VERIFY(f != NULL);
+   RTE_VERIFY(mp != NULL);
+
    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
    fprintf(f, "  flags=%x\n", mp->flags);
    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
-- 
2.1.0

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> 
> Check the FILE *f and rte_mempool *mp pointers for NULL and
> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> 
> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> ---
>  lib/librte_mempool/rte_mempool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 332f469..0f71f10 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -765,6 +765,9 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>     unsigned common_count;
>     unsigned cache_count;
> 
> +   RTE_VERIFY(f != NULL);
> +   RTE_VERIFY(mp != NULL);
> +
>     fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>     fprintf(f, "  flags=%x\n", mp->flags);
>     fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> -- 
> 2.1.0
> 
> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
> 
> 

I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
macro here instead using what I suggested in my other note
Neil

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

On Sep 28, 2014, at 7:27 AM, Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
>> 
>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
>> ---
>> lib/librte_mempool/rte_mempool.c | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 332f469..0f71f10 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -765,6 +765,9 @@ rte_mempool_dump(FILE *f, const struct rte_mempool *mp)
>>    unsigned common_count;
>>    unsigned cache_count;
>> 
>> +   RTE_VERIFY(f != NULL);
>> +   RTE_VERIFY(mp != NULL);
>> +
>>    fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>>    fprintf(f, "  flags=%x\n", mp->flags);
>>    fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
>> -- 
>> 2.1.0
>> 
>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>> 
>> 
> 
> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> macro here instead using what I suggested in my other note
> Neil

Maybe I can add RTE_VERIFY_WARN() later or someone else can.
> 

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Thomas Monjalon]

2014-09-28 08:27, Neil Horman:
> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > 
> > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> 
> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> macro here instead using what I suggested in my other note

Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
If you look elsewhere in the DPDK code, you'll see that it's not common to do
such check on input parameters.
A similar discussion already happened few months ago:
	http://dpdk.org/ml/archives/dev/2014-June/003900.html

-- 
Thomas

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> 2014-09-28 08:27, Neil Horman:
> > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > 
> > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > 
> > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > macro here instead using what I suggested in my other note
> 
> Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> If you look elsewhere in the DPDK code, you'll see that it's not common to do
> such check on input parameters.
> A similar discussion already happened few months ago:
> 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> 
Not sure what your point is here Thomas.  I think we're all in agreement that
NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
with a NULL check at all and just accept the crash as it is?

Neil

> -- 
> Thomas
> 

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Bruce Richardson]

On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > 2014-09-28 08:27, Neil Horman:
> > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > 
> > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > 
> > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > macro here instead using what I suggested in my other note
> > 
> > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > such check on input parameters.
> > A similar discussion already happened few months ago:
> > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > 
> Not sure what your point is here Thomas.  I think we're all in agreement that
> NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> with a NULL check at all and just accept the crash as it is?
>

In the general case:
* Code in the datapath should not have things like NULL checks
* However, datapath code should generally have a debug option which turns 
  these checks on to help debugging if needed. 
* Code not in the datapath probably should have these checks.

My 2c here

/Bruce

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Wiles, Roger Keith]

On Oct 1, 2014, at 10:43 AM, Bruce Richardson <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
>> On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
>>> 2014-09-28 08:27, Neil Horman:
>>>> On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
>>>>> Check the FILE *f and rte_mempool *mp pointers for NULL and
>>>>> return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
>>>>> 
>>>>> Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
>>>> 
>>>> I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
>>>> thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
>>>> macro here instead using what I suggested in my other note
>>> 
>>> Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
>>> If you look elsewhere in the DPDK code, you'll see that it's not common to do
>>> such check on input parameters.
>>> A similar discussion already happened few months ago:
>>> 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
>>> 
>> Not sure what your point is here Thomas.  I think we're all in agreement that
>> NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
>> with a NULL check at all and just accept the crash as it is?
>> 
> 
> In the general case:
> * Code in the datapath should not have things like NULL checks
> * However, datapath code should generally have a debug option which turns 
>  these checks on to help debugging if needed. 
> * Code not in the datapath probably should have these checks.

IMO the last point is the reason for the rte_mempool_dump() check. We could always add a new macro (if not already defined) that could be compiled out if not enabled as a debug. I would not want to put ‘ifdef’ around that code but include the ifdef in the macro/header file. Removing ifdefs from the code .c files should be the long term goal as a side note.
> 
> My 2c here
> 
> /Bruce

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > 2014-09-28 08:27, Neil Horman:
> > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > 
> > > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > > 
> > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > macro here instead using what I suggested in my other note
> > > 
> > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > such check on input parameters.
> > > A similar discussion already happened few months ago:
> > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > 
> > Not sure what your point is here Thomas.  I think we're all in agreement that
> > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > with a NULL check at all and just accept the crash as it is?
> >
> 
> In the general case:
> * Code in the datapath should not have things like NULL checks
> * However, datapath code should generally have a debug option which turns 
>   these checks on to help debugging if needed. 
> * Code not in the datapath probably should have these checks.
> 
Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
datapath, its rather by definition a debug function, isn't it?
Neil

> My 2c here
> 
> /Bruce
> 

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Bruce Richardson]

On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > 2014-09-28 08:27, Neil Horman:
> > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > 
> > > > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > > > 
> > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > macro here instead using what I suggested in my other note
> > > > 
> > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > such check on input parameters.
> > > > A similar discussion already happened few months ago:
> > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > 
> > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > with a NULL check at all and just accept the crash as it is?
> > >
> > 
> > In the general case:
> > * Code in the datapath should not have things like NULL checks
> > * However, datapath code should generally have a debug option which turns 
> >   these checks on to help debugging if needed. 
> > * Code not in the datapath probably should have these checks.
> > 
> Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> datapath, its rather by definition a debug function, isn't it?
> Neil

Yes, agreed.  [So it probably should have the NULL check].
/Bruce

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Thomas Monjalon]

2014-10-01 17:05, Bruce Richardson:
> On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > 
> > > > > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > > > > 
> > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > macro here instead using what I suggested in my other note
> > > > > 
> > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > such check on input parameters.
> > > > > A similar discussion already happened few months ago:
> > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > 
> > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > with a NULL check at all and just accept the crash as it is?
> > > >
> > > 
> > > In the general case:
> > > * Code in the datapath should not have things like NULL checks
> > > * However, datapath code should generally have a debug option which turns 
> > >   these checks on to help debugging if needed. 
> > > * Code not in the datapath probably should have these checks.
> > > 
> > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > datapath, its rather by definition a debug function, isn't it?
> > Neil
> 
> Yes, agreed.  [So it probably should have the NULL check].

I have many arguments to not do this check:
1) If it was a coding rule to do this kind of check, it should be done in
almost every functions.
2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
3) Why check only NULL value? 1 and 2 are also some invalid values...

-- 
Thomas

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Neil Horman]

On Thu, Oct 02, 2014 at 09:47:19AM +0200, Thomas Monjalon wrote:
> 2014-10-01 17:05, Bruce Richardson:
> > On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > > 
> > > > > > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > > > > > 
> > > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > > macro here instead using what I suggested in my other note
> > > > > > 
> > > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > > such check on input parameters.
> > > > > > A similar discussion already happened few months ago:
> > > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > > 
> > > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > > with a NULL check at all and just accept the crash as it is?
> > > > >
> > > > 
> > > > In the general case:
> > > > * Code in the datapath should not have things like NULL checks
> > > > * However, datapath code should generally have a debug option which turns 
> > > >   these checks on to help debugging if needed. 
> > > > * Code not in the datapath probably should have these checks.
> > > > 
> > > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > > datapath, its rather by definition a debug function, isn't it?
> > > Neil
> > 
> > Yes, agreed.  [So it probably should have the NULL check].
> 
> I have many arguments to not do this check:
> 1) If it was a coding rule to do this kind of check, it should be done in
> almost every functions.
Only if NULL is an invalid value, and we spot check for NULL all the time (see
eal_parse_coremask as an example from a quick search).

> 2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
Its also quite common to do the check.  I think this is more about if it makes
sense to do it here (i.e. is it a common error to pass a NULL pointer into
mempool_dump?).  If so, an extra check with its own specific panic might be
nice.

> 3) Why check only NULL value? 1 and 2 are also some invalid values...
> 
Because NULL is the common case.
Neil

> -- 
> Thomas
> 

Re: [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer.

[Thomas Monjalon]

2014-10-02 07:37, Neil Horman:
> On Thu, Oct 02, 2014 at 09:47:19AM +0200, Thomas Monjalon wrote:
> > 2014-10-01 17:05, Bruce Richardson:
> > > On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote:
> > > > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote:
> > > > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote:
> > > > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote:
> > > > > > > 2014-09-28 08:27, Neil Horman:
> > > > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote:
> > > > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and
> > > > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Keith Wiles <keith.wiles-CWA4WttNNZF54TAoqtyWWQ@public.gmane.org>
> > > > > > > > 
> > > > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here,
> > > > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN
> > > > > > > > macro here instead using what I suggested in my other note
> > > > > > > 
> > > > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application.
> > > > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do
> > > > > > > such check on input parameters.
> > > > > > > A similar discussion already happened few months ago:
> > > > > > > 	http://dpdk.org/ml/archives/dev/2014-June/003900.html
> > > > > > > 
> > > > > > Not sure what your point is here Thomas.  I think we're all in agreement that
> > > > > > NULL is a bad value to pass in here.  Are you asserting that we shouldn't bother
> > > > > > with a NULL check at all and just accept the crash as it is?
> > > > > >
> > > > > 
> > > > > In the general case:
> > > > > * Code in the datapath should not have things like NULL checks
> > > > > * However, datapath code should generally have a debug option which turns 
> > > > >   these checks on to help debugging if needed. 
> > > > > * Code not in the datapath probably should have these checks.
> > > > > 
> > > > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the
> > > > datapath, its rather by definition a debug function, isn't it?
> > > > Neil
> > > 
> > > Yes, agreed.  [So it probably should have the NULL check].
> > 
> > I have many arguments to not do this check:
> > 1) If it was a coding rule to do this kind of check, it should be done in
> > almost every functions.
> Only if NULL is an invalid value, and we spot check for NULL all the time (see
> eal_parse_coremask as an example from a quick search).
> 
> > 2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)?
> Its also quite common to do the check.  I think this is more about if it makes
> sense to do it here (i.e. is it a common error to pass a NULL pointer into
> mempool_dump?).  If so, an extra check with its own specific panic might be
> nice.
> 
> > 3) Why check only NULL value? 1 and 2 are also some invalid values...
> > 
> Because NULL is the common case.
> Neil

Argumentation accepted and patch applied.

-- 
Thomas