[LEDE-DEV,ubox] getrandom: Define SYS_getrandom if no definition exists

Le 17/09/2016 à 16:49, Etienne Champetier a écrit :
> Hi Florian,
> 
> What is your use case?

Pre-built toolchain which ships with kernel headers 3.8 at the moment.

> If you run it on kernel less than 3.17 this will then fail at run time ...

I run a kernel newer than 3.17, but that's a good point.

> I would prefer to not compile it instead of having an unusable binary

Ok, so something more like that:


        $(LN) ../../sbin/kmodloader $(1)/usr/sbin/rmmod


> 
> Regards
> Etienne
> 
> 
> Le 17 sept. 2016 10:17 PM, "Florian Fainelli" <f.fainelli@gmail.com
> <mailto:f.fainelli@gmail.com>> a écrit :
> 
>     Building ubox against kernel headers earlier than 3.17 would fail since
>     SYS_getrandom was not defined, so provide a definition for it.
> 
>     Signed-off-by: Florian Fainelli <f.fainelli@gmail.com
>     <mailto:f.fainelli@gmail.com>>
>     ---
>      getrandom.c | 4 ++++
>      1 file changed, 4 insertions(+)
> 
>     diff --git a/getrandom.c b/getrandom.c
>     index 96712028589b..e324072fcf0e 100644
>     --- a/getrandom.c
>     +++ b/getrandom.c
>     @@ -32,6 +32,10 @@ static int usage(char *name)
>          return EXIT_FAILURE;
>      }
> 
>     +#ifndef SYS_getrandom
>     +#define SYS_getrandom  278
>     +#endif
>     +
>      int main(int argc, char *argv[])
>      {
>          if (argc != 2)
>     --
>     2.7.4
>

On 18/09/2016 02:13, Florian Fainelli wrote:
> Le 17/09/2016 à 16:49, Etienne Champetier a écrit :
>> Hi Florian,
>>
>> What is your use case?
> 
> Pre-built toolchain which ships with kernel headers 3.8 at the moment.
> 
>> If you run it on kernel less than 3.17 this will then fail at run time ...
> 
> I run a kernel newer than 3.17, but that's a good point.
> 
>> I would prefer to not compile it instead of having an unusable binary
> 
> Ok, so something more like that:
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 6cf0c934aac6..1d04c1d77662 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -16,10 +16,15 @@ IF(DEBUG)
>    ADD_DEFINITIONS(-DDEBUG -g3)
>  ENDIF()
> 
> -ADD_EXECUTABLE(getrandom getrandom.c)
> -INSTALL(TARGETS getrandom
> +INCLUDE (CheckSymbolExists)
> +CHECK_SYMBOL_EXISTS(SYS_getrandom sycall.h getrandom)
> +
> +IF(getrandom)
> +  ADD_EXECUTABLE(getrandom getrandom.c)
> +  INSTALL(TARGETS getrandom
>         RUNTIME DESTINATION bin
> -)
> +  )
> +ENDIF()
> 
>  ADD_EXECUTABLE(kmodloader kmodloader.c)
>  TARGET_LINK_LIBRARIES(kmodloader ubox)
> 
> diff --git a/package/system/ubox/Makefile b/package/system/ubox/Makefile
> index 9f0c4dc10250..48371e168fd3 100644
> --- a/package/system/ubox/Makefile
> +++ b/package/system/ubox/Makefile
> @@ -32,7 +32,7 @@ define Package/ubox/install
>         $(INSTALL_DIR) $(1)/sbin $(1)/usr/sbin $(1)/lib $(1)/usr/bin
> $(1)/etc/init.d
> 
>         $(INSTALL_BIN)
> $(PKG_INSTALL_DIR)/usr/sbin/{kmodloader,validate_data} $(1)/sbin/
> -       $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/getrandom $(1)/usr/bin/
> +       -$(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/getrandom $(1)/usr/bin/
>         $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/libvalidate.so $(1)/lib
> 
>         $(LN) ../../sbin/kmodloader $(1)/usr/sbin/rmmod
> 
> 

looks better. i think that the code invoking getrandom would also need
to be changed to fallback to urandom if the helper does not exist.

	John

Le 18/09/2016 à 12:06, Etienne Champetier a écrit :
> (Answering from my phone with gmail so this email is in HTML and will
> get bounced by the ML)
> 
> Le 18 sept. 2016 14:03, "John Crispin" <john@phrozen.org
> <mailto:john@phrozen.org>> a écrit :
>>
>>
>>
>> On 18/09/2016 02:13, Florian Fainelli wrote:
>> > Le 17/09/2016 à 16:49, Etienne Champetier a écrit :
>> >> Hi Florian,
>> >>
>> >> What is your use case?
>> >
>> > Pre-built toolchain which ships with kernel headers 3.8 at the moment.
>> >
>> >> If you run it on kernel less than 3.17 this will then fail at run
> time ...
>> >
>> > I run a kernel newer than 3.17, but that's a good point.
>> >
>> >> I would prefer to not compile it instead of having an unusable binary
>> >
>> > Ok, so something more like that:
> 
> I still doesn't understand why getrandom isn't built with target kernel
> headers
> All LEDE targets are using linux 4.4 and OpenWRT targets are 3.18+, so
> this feels like the wrong fix for me

I am using an external toolchain which ships with kernel headers 3.8,
and anybody building an external toolchain in OpenWrt may run into a
similar problem.

Irrespective of the kernel version, since the system call is not present
in all kernel versions, testing for its presence does not hurt.

Besides that, people may end up backporting a newer user-space to run
with an older kernel that is pre 3.17, making things robust and testing
for what you use in your application is a good practice that has years
of precedence (autotols would not exist otherwise).

Hi Florian,

2016-09-18 21:36 GMT+02:00 Florian Fainelli <f.fainelli@gmail.com>:
>
> Le 18/09/2016 à 12:06, Etienne Champetier a écrit :
> > (Answering from my phone with gmail so this email is in HTML and will
> > get bounced by the ML)
> >
> > Le 18 sept. 2016 14:03, "John Crispin" <john@phrozen.org
> > <mailto:john@phrozen.org>> a écrit :
> >>
> >>
> >>
> >> On 18/09/2016 02:13, Florian Fainelli wrote:
> >> > Le 17/09/2016 à 16:49, Etienne Champetier a écrit :
> >> >> Hi Florian,
> >> >>
> >> >> What is your use case?
> >> >
> >> > Pre-built toolchain which ships with kernel headers 3.8 at the moment.
> >> >
> >> >> If you run it on kernel less than 3.17 this will then fail at run
> > time ...
> >> >
> >> > I run a kernel newer than 3.17, but that's a good point.
> >> >
> >> >> I would prefer to not compile it instead of having an unusable binary
> >> >
> >> > Ok, so something more like that:
> >
> > I still doesn't understand why getrandom isn't built with target kernel
> > headers
> > All LEDE targets are using linux 4.4 and OpenWRT targets are 3.18+, so
> > this feels like the wrong fix for me
>
> I am using an external toolchain which ships with kernel headers 3.8,
> and anybody building an external toolchain in OpenWrt may run into a
> similar problem.
>
> Irrespective of the kernel version, since the system call is not present
> in all kernel versions, testing for its presence does not hurt.

Checking for it's presence doesn't hurt, but silently failing (not
compiling) does.
Without a fallback in /sbin/urandom_seed, you now have a run-time failure ...
https://git.lede-project.org/?p=source.git;a=blob;f=package/base-files/files/sbin/urandom_seed;hb=HEAD
There is no good fallback, ie you can't emulate getrandom() (that is
why i introduced this helper in the first place)

If you are using a kernel newer than 3.17, but your toolchain is
shipping with  kernel header 3.8, may I suggest just patching this
toolchain? (Or using your V1 in your local fork)

>
> Besides that, people may end up backporting a newer user-space to run
> with an older kernel that is pre 3.17, making things robust and testing
> for what you use in your application is a good practice that has years
> of precedence (autotols would not exist otherwise).

When there is no fallback, a compilation failure is a good start :)

To sum up nack for your V1 patch as it's dangerous (runtime failure of
getrandom)
your V2 patch helps to backport other binaries from ubox (ubox now
build on pre 3.17),
but it still break urandom_seed script with old external toolchain

Regards
Etienne

> --
> Florian

On 09/21/2016 01:34 PM, Etienne Champetier wrote:
> Hi Florian,
> 
> 2016-09-18 21:36 GMT+02:00 Florian Fainelli <f.fainelli@gmail.com>:
>>
>> Le 18/09/2016 à 12:06, Etienne Champetier a écrit :
>>> (Answering from my phone with gmail so this email is in HTML and will
>>> get bounced by the ML)
>>>
>>> Le 18 sept. 2016 14:03, "John Crispin" <john@phrozen.org
>>> <mailto:john@phrozen.org>> a écrit :
>>>>
>>>>
>>>>
>>>> On 18/09/2016 02:13, Florian Fainelli wrote:
>>>>> Le 17/09/2016 à 16:49, Etienne Champetier a écrit :
>>>>>> Hi Florian,
>>>>>>
>>>>>> What is your use case?
>>>>>
>>>>> Pre-built toolchain which ships with kernel headers 3.8 at the moment.
>>>>>
>>>>>> If you run it on kernel less than 3.17 this will then fail at run
>>> time ...
>>>>>
>>>>> I run a kernel newer than 3.17, but that's a good point.
>>>>>
>>>>>> I would prefer to not compile it instead of having an unusable binary
>>>>>
>>>>> Ok, so something more like that:
>>>
>>> I still doesn't understand why getrandom isn't built with target kernel
>>> headers
>>> All LEDE targets are using linux 4.4 and OpenWRT targets are 3.18+, so
>>> this feels like the wrong fix for me
>>
>> I am using an external toolchain which ships with kernel headers 3.8,
>> and anybody building an external toolchain in OpenWrt may run into a
>> similar problem.
>>
>> Irrespective of the kernel version, since the system call is not present
>> in all kernel versions, testing for its presence does not hurt.
> 
> Checking for it's presence doesn't hurt, but silently failing (not
> compiling) does.
> Without a fallback in /sbin/urandom_seed, you now have a run-time failure ...
> https://git.lede-project.org/?p=source.git;a=blob;f=package/base-files/files/sbin/urandom_seed;hb=HEAD
> There is no good fallback, ie you can't emulate getrandom() (that is
> why i introduced this helper in the first place)
> 
> If you are using a kernel newer than 3.17, but your toolchain is
> shipping with  kernel header 3.8, may I suggest just patching this
> toolchain? (Or using your V1 in your local fork)

Not an option at the moment, however, checking at runtime if the kernel
is greater than 3.17 and providing the definition of the system call
number (278) might be an option in that case, that seems most likely
like a local patch though, not suitable for upstream ubox.

> 
>>
>> Besides that, people may end up backporting a newer user-space to run
>> with an older kernel that is pre 3.17, making things robust and testing
>> for what you use in your application is a good practice that has years
>> of precedence (autotols would not exist otherwise).
> 
> When there is no fallback, a compilation failure is a good start :)
> 
> To sum up nack for your V1 patch as it's dangerous (runtime failure of
> getrandom)
> your V2 patch helps to backport other binaries from ubox (ubox now
> build on pre 3.17),
> but it still break urandom_seed script with old external toolchain

Yep, it's starting to look a lot like systemd, loving it already!