rte_random: fix crash when random init
Checks
Commit Message
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(-)
Comments
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.
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.
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.
>
>
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.
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.
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.
>
>
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.
>>
>>
>
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?
@@ -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