Message ID | a8bfee14-55f8-3534-c48d-d74286118939@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
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!
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