[dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

When rte_rand_init is invoked, and the kernel
(kernel version < 3.17) running dpdk does't support
*getentropy, at the same time, the cpu does't support
rdseed, the rte_rand_init will invoke rte_get_timer_cycles
which function will invoke rte_get_hpet_cycles
(RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
allocated.

Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")

Cc: stable@dpdk.org

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_eal/linux/eal_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linux/eal_timer.c b/lib/librte_eal/linux/eal_timer.c
index a904a8297cd2..4067e46da534 100644
--- a/lib/librte_eal/linux/eal_timer.c
+++ b/lib/librte_eal/linux/eal_timer.c
@@ -26,7 +26,7 @@
 #include "eal_private.h"
 #include "eal_internal_cfg.h"
 
-enum timer_source eal_timer_source = EAL_TIMER_HPET;
+enum timer_source eal_timer_source = EAL_TIMER_TSC;
 
 #ifdef RTE_LIBEAL_USE_HPET
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Stephen Hemminger]

On Sun, 12 Apr 2020 16:27:53 +0800
xiangxia.m.yue@gmail.com wrote:

> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> When rte_rand_init is invoked, and the kernel
> (kernel version < 3.17) running dpdk does't support
> *getentropy, at the same time, the cpu does't support
> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
> which function will invoke rte_get_hpet_cycles
> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
> allocated.
> 
> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Are you sure this patch won't change current default to use HPET (which is slower)?
Before this patch users would get TSC as default.

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Tonghao Zhang]

On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Sun, 12 Apr 2020 16:27:53 +0800
> xiangxia.m.yue@gmail.com wrote:
>
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > When rte_rand_init is invoked, and the kernel
> > (kernel version < 3.17) running dpdk does't support
> > *getentropy, at the same time, the cpu does't support
> > rdseed, the rte_rand_init will invoke rte_get_timer_cycles
> > which function will invoke rte_get_hpet_cycles
> > (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
> > allocated.
> >
> > Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
> > Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> Are you sure this patch won't change current default to use HPET (which is slower)?
In rte_eal_timer_init (linux/eal_timer.c)， it will set
eal_timer_source = EAL_TIMER_TSC too.
So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
default timer source actually.
Then this patch will affect RTE_INIT function which invoke
rte_get_timer_cycles. but hpet is not available yet.
> Before this patch users would get TSC as default.



-- 
Best regards, Tonghao

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Mattias Rönnblom]

On 2020-04-14 06:43, Tonghao Zhang wrote:
> On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Sun, 12 Apr 2020 16:27:53 +0800
>> xiangxia.m.yue@gmail.com wrote:
>>
>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>
>>> When rte_rand_init is invoked, and the kernel
>>> (kernel version < 3.17) running dpdk does't support
>>> *getentropy, at the same time, the cpu does't support
>>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
>>> which function will invoke rte_get_hpet_cycles
>>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
>>> allocated.
>>>
>>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>> Are you sure this patch won't change current default to use HPET (which is slower)?
> In rte_eal_timer_init (linux/eal_timer.c)， it will set
> eal_timer_source = EAL_TIMER_TSC too.
> So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
> default timer source actually.
> Then this patch will affect RTE_INIT function which invoke
> rte_get_timer_cycles. but hpet is not available yet.


Would using rte_rdtsc() directly be an option?


>> Before this patch users would get TSC as default.
>
>

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[David Marchand]

On Tue, Apr 14, 2020 at 3:20 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-04-14 06:43, Tonghao Zhang wrote:
> > On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> >> On Sun, 12 Apr 2020 16:27:53 +0800
> >> xiangxia.m.yue@gmail.com wrote:
> >>
> >>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>
> >>> When rte_rand_init is invoked, and the kernel
> >>> (kernel version < 3.17) running dpdk does't support
> >>> *getentropy, at the same time, the cpu does't support
> >>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
> >>> which function will invoke rte_get_hpet_cycles
> >>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
> >>> allocated.
> >>>
> >>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
> >>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> >>>
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >> Are you sure this patch won't change current default to use HPET (which is slower)?
> > In rte_eal_timer_init (linux/eal_timer.c)， it will set
> > eal_timer_source = EAL_TIMER_TSC too.
> > So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
> > default timer source actually.
> > Then this patch will affect RTE_INIT function which invoke
> > rte_get_timer_cycles. but hpet is not available yet.
>
>
> Would using rte_rdtsc() directly be an option?

s/rte_rdtsc/rte_get_tsc_cycles/

This could work, but I am a bit surprised to see an initialisation in
a constructor.
The commitlog that moved rte_srand() from rte_eal_init does not
explain why it was moved.


-- 
David Marchand

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Mattias Rönnblom]

On 2020-04-14 15:35, David Marchand wrote:
> On Tue, Apr 14, 2020 at 3:20 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-04-14 06:43, Tonghao Zhang wrote:
>>> On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
>>> <stephen@networkplumber.org> wrote:
>>>> On Sun, 12 Apr 2020 16:27:53 +0800
>>>> xiangxia.m.yue@gmail.com wrote:
>>>>
>>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>
>>>>> When rte_rand_init is invoked, and the kernel
>>>>> (kernel version < 3.17) running dpdk does't support
>>>>> *getentropy, at the same time, the cpu does't support
>>>>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
>>>>> which function will invoke rte_get_hpet_cycles
>>>>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
>>>>> allocated.
>>>>>
>>>>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
>>>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
>>>>>
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>> Are you sure this patch won't change current default to use HPET (which is slower)?
>>> In rte_eal_timer_init (linux/eal_timer.c)， it will set
>>> eal_timer_source = EAL_TIMER_TSC too.
>>> So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
>>> default timer source actually.
>>> Then this patch will affect RTE_INIT function which invoke
>>> rte_get_timer_cycles. but hpet is not available yet.
>>
>> Would using rte_rdtsc() directly be an option?
> s/rte_rdtsc/rte_get_tsc_cycles/
>
> This could work, but I am a bit surprised to see an initialisation in
> a constructor.
> The commitlog that moved rte_srand() from rte_eal_init does not
> explain why it was moved.
>
>
The initialization (i.e. automatic seeding) grew in complexity somewhat, 
and with the new rte_random.c file, it felt like it would have a good, 
new home.


That said, maybe it would have been better to add an initialization 
function to the rte_random.h API, and have it called from 
rte_eal_init(), to avoid the ordering issues with constructors. Yet 
another alternative would be to just move the seeding logic.

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Tonghao Zhang]

On Tue, Apr 14, 2020 at 11:37 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-04-14 15:35, David Marchand wrote:
> > On Tue, Apr 14, 2020 at 3:20 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> >> On 2020-04-14 06:43, Tonghao Zhang wrote:
> >>> On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
> >>> <stephen@networkplumber.org> wrote:
> >>>> On Sun, 12 Apr 2020 16:27:53 +0800
> >>>> xiangxia.m.yue@gmail.com wrote:
> >>>>
> >>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>>>
> >>>>> When rte_rand_init is invoked, and the kernel
> >>>>> (kernel version < 3.17) running dpdk does't support
> >>>>> *getentropy, at the same time, the cpu does't support
> >>>>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
> >>>>> which function will invoke rte_get_hpet_cycles
> >>>>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
> >>>>> allocated.
> >>>>>
> >>>>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
> >>>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> >>>>>
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>> Are you sure this patch won't change current default to use HPET (which is slower)?
> >>> In rte_eal_timer_init (linux/eal_timer.c)， it will set
> >>> eal_timer_source = EAL_TIMER_TSC too.
> >>> So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
> >>> default timer source actually.
> >>> Then this patch will affect RTE_INIT function which invoke
> >>> rte_get_timer_cycles. but hpet is not available yet.
> >>
> >> Would using rte_rdtsc() directly be an option?
> > s/rte_rdtsc/rte_get_tsc_cycles/
> >
> > This could work, but I am a bit surprised to see an initialisation in
> > a constructor.
> > The commitlog that moved rte_srand() from rte_eal_init does not
> > explain why it was moved.
> >
> >
> The initialization (i.e. automatic seeding) grew in complexity somewhat,
> and with the new rte_random.c file, it felt like it would have a good,
> new home.
>
>
> That said, maybe it would have been better to add an initialization
> function to the rte_random.h API, and have it called from
> rte_eal_init(), to avoid the ordering issues with constructors.
If we use that solution  we can register a callback which invoked when
almost resources are  available:
http://patches.dpdk.org/patch/68313/
> Yet
> another alternative would be to just move the seeding logic.
>
>


-- 
Best regards, Tonghao

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[Mattias Rönnblom]

On 2020-04-15 01:42, Tonghao Zhang wrote:
> On Tue, Apr 14, 2020 at 11:37 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> On 2020-04-14 15:35, David Marchand wrote:
>>> On Tue, Apr 14, 2020 at 3:20 PM Mattias Rönnblom
>>> <mattias.ronnblom@ericsson.com> wrote:
>>>> On 2020-04-14 06:43, Tonghao Zhang wrote:
>>>>> On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
>>>>> <stephen@networkplumber.org> wrote:
>>>>>> On Sun, 12 Apr 2020 16:27:53 +0800
>>>>>> xiangxia.m.yue@gmail.com wrote:
>>>>>>
>>>>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>>>
>>>>>>> When rte_rand_init is invoked, and the kernel
>>>>>>> (kernel version < 3.17) running dpdk does't support
>>>>>>> *getentropy, at the same time, the cpu does't support
>>>>>>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
>>>>>>> which function will invoke rte_get_hpet_cycles
>>>>>>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
>>>>>>> allocated.
>>>>>>>
>>>>>>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
>>>>>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
>>>>>>>
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>>>> Are you sure this patch won't change current default to use HPET (which is slower)?
>>>>> In rte_eal_timer_init (linux/eal_timer.c)， it will set
>>>>> eal_timer_source = EAL_TIMER_TSC too.
>>>>> So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
>>>>> default timer source actually.
>>>>> Then this patch will affect RTE_INIT function which invoke
>>>>> rte_get_timer_cycles. but hpet is not available yet.
>>>> Would using rte_rdtsc() directly be an option?
>>> s/rte_rdtsc/rte_get_tsc_cycles/
>>>
>>> This could work, but I am a bit surprised to see an initialisation in
>>> a constructor.
>>> The commitlog that moved rte_srand() from rte_eal_init does not
>>> explain why it was moved.
>>>
>>>
>> The initialization (i.e. automatic seeding) grew in complexity somewhat,
>> and with the new rte_random.c file, it felt like it would have a good,
>> new home.
>>
>>
>> That said, maybe it would have been better to add an initialization
>> function to the rte_random.h API, and have it called from
>> rte_eal_init(), to avoid the ordering issues with constructors.
> If we use that solution  we can register a callback which invoked when
> almost resources are  available:
> http://patches.dpdk.org/patch/68313/


We might then instead have a new ordering issue, if we wait too long 
with rte_random initialization, since other initialization code might 
well use rte_rand(). It seems like the memory heap expansion code does 
currently. rte_rand() is a pretty basic function, that should be safe to 
call early during initialization, I think.


I would suggest first switching to rte_get_tsc_cycles() and later 
potentially change to explicit (i.e. non-constructor), early, rte_random 
initialization.


>> Yet
>> another alternative would be to just move the seeding logic.
>>
>>
>

Re: [dpdk-dev] [PATCH dpdk-dev] rte_random: fix crash when random init

[David Marchand]

On Wed, Apr 15, 2020 at 8:01 AM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> On 2020-04-15 01:42, Tonghao Zhang wrote:
> > On Tue, Apr 14, 2020 at 11:37 PM Mattias Rönnblom
> > <mattias.ronnblom@ericsson.com> wrote:
> >> On 2020-04-14 15:35, David Marchand wrote:
> >>> On Tue, Apr 14, 2020 at 3:20 PM Mattias Rönnblom
> >>> <mattias.ronnblom@ericsson.com> wrote:
> >>>> On 2020-04-14 06:43, Tonghao Zhang wrote:
> >>>>> On Tue, Apr 14, 2020 at 12:07 PM Stephen Hemminger
> >>>>> <stephen@networkplumber.org> wrote:
> >>>>>> On Sun, 12 Apr 2020 16:27:53 +0800
> >>>>>> xiangxia.m.yue@gmail.com wrote:
> >>>>>>
> >>>>>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>>>>>
> >>>>>>> When rte_rand_init is invoked, and the kernel
> >>>>>>> (kernel version < 3.17) running dpdk does't support
> >>>>>>> *getentropy, at the same time, the cpu does't support
> >>>>>>> rdseed, the rte_rand_init will invoke rte_get_timer_cycles
> >>>>>>> which function will invoke rte_get_hpet_cycles
> >>>>>>> (RTE_LIBEAL_USE_HPET was enabled) while *eal_hpet is not
> >>>>>>> allocated.
> >>>>>>>
> >>>>>>> Fixes: faf8fd252785 ("eal: improve entropy for initial PRNG seed")
> >>>>>>> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> >>>>>>>
> >>>>>>> Cc: stable@dpdk.org
> >>>>>>>
> >>>>>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>>>> Are you sure this patch won't change current default to use HPET (which is slower)?
> >>>>> In rte_eal_timer_init (linux/eal_timer.c)， it will set
> >>>>> eal_timer_source = EAL_TIMER_TSC too.
> >>>>> So after rte_eal_init, eal_timer_source == EAL_TIMER_TSC which is the
> >>>>> default timer source actually.
> >>>>> Then this patch will affect RTE_INIT function which invoke
> >>>>> rte_get_timer_cycles. but hpet is not available yet.
> >>>> Would using rte_rdtsc() directly be an option?
> >>> s/rte_rdtsc/rte_get_tsc_cycles/
> >>>
> >>> This could work, but I am a bit surprised to see an initialisation in
> >>> a constructor.
> >>> The commitlog that moved rte_srand() from rte_eal_init does not
> >>> explain why it was moved.
> >>>
> >>>
> >> The initialization (i.e. automatic seeding) grew in complexity somewhat,
> >> and with the new rte_random.c file, it felt like it would have a good,
> >> new home.
> >>
> >>
> >> That said, maybe it would have been better to add an initialization
> >> function to the rte_random.h API, and have it called from
> >> rte_eal_init(), to avoid the ordering issues with constructors.
> > If we use that solution  we can register a callback which invoked when
> > almost resources are  available:
> > http://patches.dpdk.org/patch/68313/
>
>
> We might then instead have a new ordering issue, if we wait too long
> with rte_random initialization, since other initialization code might
> well use rte_rand(). It seems like the memory heap expansion code does
> currently. rte_rand() is a pretty basic function, that should be safe to
> call early during initialization, I think.
>
>
> I would suggest first switching to rte_get_tsc_cycles() and later
> potentially change to explicit (i.e. non-constructor), early, rte_random
> initialization.

Switching to rte_get_tsc_cycles seems fine to me.
Who will send the fix?


-- 
David Marchand