| Message ID | 20240313091412.20865-4-francois.perrad@gadz.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [01/31] package/busybox: move the hush config in a fragment | expand |
Hi Francois, On 13/03/2024 10:13, Francois Perrad wrote: > Signed-off-by: Francois Perrad <francois.perrad@gadz.org> > --- > package/diffutils/diffutils.mk | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/package/diffutils/diffutils.mk b/package/diffutils/diffutils.mk > index 111926686..fe20a0e67 100644 > --- a/package/diffutils/diffutils.mk > +++ b/package/diffutils/diffutils.mk > @@ -18,4 +18,8 @@ ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y) > DIFFUTILS_CONF_ENV += gl_cv_func_getopt_gnu=yes > endif > > +define DIFFUTILS_BUSYBOX_CONFIG_FIXUPS > + $(call KCONFIG_DISABLE_OPT,CONFIG_DIFF) I don't understand the point of this series. In general, there is no reason to disable applets from busybox just because there's a "full" alternative installed on the system. Usually the busybox size difference is completely negligible compared to the the "full" installation size. And the busybox one can still be useful in some cases (when called as 'busybox diff' instead of 'diff'). If you really care about removing the redundant busybox options, you can easily supply a custom busybox config. Is there a really good reason for all this? Regards, Arnout > +endef > + > $(eval $(autotools-package))
Le ven. 15 mars 2024 à 20:57, Arnout Vandecappelle via buildroot < buildroot@buildroot.org> a écrit : > Hi Francois, > > On 13/03/2024 10:13, Francois Perrad wrote: > > Signed-off-by: Francois Perrad <francois.perrad@gadz.org> > > --- > > package/diffutils/diffutils.mk | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/package/diffutils/diffutils.mk b/package/diffutils/ > diffutils.mk > > index 111926686..fe20a0e67 100644 > > --- a/package/diffutils/diffutils.mk > > +++ b/package/diffutils/diffutils.mk > > @@ -18,4 +18,8 @@ ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y) > > DIFFUTILS_CONF_ENV += gl_cv_func_getopt_gnu=yes > > endif > > > > +define DIFFUTILS_BUSYBOX_CONFIG_FIXUPS > > + $(call KCONFIG_DISABLE_OPT,CONFIG_DIFF) > > I don't understand the point of this series. In general, there is no > reason to > disable applets from busybox just because there's a "full" alternative > installed > on the system. Usually the busybox size difference is completely > negligible > compared to the the "full" installation size. And the busybox one can > still be > useful in some cases (when called as 'busybox diff' instead of 'diff'). > > If you really care about removing the redundant busybox options, you can > easily supply a custom busybox config. > > Is there a really good reason for all this? > > In fact, in this serie, the only important patches are the 2 first. I wrote the nexts, like this one, in order to show an use case of the 2nd patch "package/busybox: handle LIBFOO_BUSYBOX_CONFIG_FIXUPS". But it was a bad idea. Note that my goal is not to reducing the image size, but to reducing the attack surface (from a cyber security point of view). Busybox uses the Kconfig infrastructure. Currently, it could be configured via a full custom defconfig or via fragments. But, I think that altering the configuration by fixups could be also useful. Francois > Regards, > Arnout > > > +endef > > + > > $(eval $(autotools-package)) > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot >
François, All, On 2024-03-16 11:59 +0100, François Perrad spake thusly: > Le ven. 15 mars 2024 à 20:57, Arnout Vandecappelle via buildroot <[1] > buildroot@buildroot.org> a écrit : > On 13/03/2024 10:13, Francois Perrad wrote: > > Signed-off-by: Francois Perrad <[2]francois.perrad@gadz.org> > > --- > > package/diffutils/[3]diffutils.mk | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/package/diffutils/[4]diffutils.mk b/package/diffutils/[5] > diffutils.mk > > index 111926686..fe20a0e67 100644 > > --- a/package/diffutils/[6]diffutils.mk > > +++ b/package/diffutils/[7]diffutils.mk > > @@ -18,4 +18,8 @@ ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y) > > DIFFUTILS_CONF_ENV += gl_cv_func_getopt_gnu=yes > > endif > > > > +define DIFFUTILS_BUSYBOX_CONFIG_FIXUPS > > + $(call KCONFIG_DISABLE_OPT,CONFIG_DIFF) > > I don't understand the point of this series. In general, there is no > reason to > disable applets from busybox just because there's a "full" alternative > installed > on the system. Usually the busybox size difference is completely negligible > compared to the the "full" installation size. And the busybox one can still > be > useful in some cases (when called as 'busybox diff' instead of 'diff'). > > If you really care about removing the redundant busybox options, you can > easily supply a custom busybox config. > > Is there a really good reason for all this? > > In fact, in this serie, the only important patches are the 2 first. I didn't understand what was so important in the first patch, so you'd have to extend the commit log with a bit more explanations, should you respin it later. Also, it seems totally unrelated to the rest of the series, so it should probably be sent separately. > I wrote the nexts, like this one, in order to show an use case of the 2nd patch > "package/busybox: handle LIBFOO_BUSYBOX_CONFIG_FIXUPS". As for patch 2, I see that it is modelled after the similar feature we have for the linux kernel. It would seem reasonable to have in the first place. However, I still fail to see what the point really is. Indeed, for the kernel, the feature is there because packages need a feature from the kerenl, so we want to allo them to enable it, very seldom to disable them [0]. But for busybox, there is not need to explicitly disable options: the install order guarantees that the full-blown variant win over the busybox applets. Also, as Arnout pointed out, it is still possible and interesting to keep the applets in busybox even when the ful-blown variant is enabled. Indeed, it is possible to build an initramfs that only contains busybox, which applets are used until the final root is mounted and swithroot-ed into. So, we do not ned a feature to _disable_ options in Busybox. Now, on the other side of the coin, do we need a feature that allows packages to _enable_ options in Busybox? Unlike the kernel, it is possible to have a generic Busybox configuration that works "almost everywhere" (well, two: one for MMU, one for noMMU). So that's the path we have chosen in Buildroot: we cary those two configs, and there is no reason to automatically tweak those; a concerned user will have to provide their own, tailored to their particular use-case. > Note that my goal is not to reducing the image size, but to reducing the attack > surface (from a cyber security point of view). Although I do laud the effort, this goes against the points mentioned above. A user who wants to address security will have to review their Busybox config and decide whether to drop or keep options, based on their use-case (esp. the initramfs case). > Busybox uses the Kconfig infrastructure. > Currently, it could be configured via a full custom defconfig or via fragments. > But, I think that altering the configuration by fixups could be also useful. The fact that it uses the Kconfig infra is no reason in itself to make it behave like the kernel (otherwise we'd have to do the same for all the other kconfig-based packages) [1] So, given all the above, I would say that we do not need this feature at all for busybox. [0] we have only three packages that disable kernel options: - systemd disables legacy sysfs (which were entirely dropped in v6.4) - kernel-module-imx-gpu-viv (a driver) disables an incompatible driver specific to the IMX kernel fork - zfs which needs stuff unavailable when those options are set (like unused ksyms) Those are really special cases; usually packages need to enable options. [1] and if we went that route, then there would be no reason why we would not have to have a mechanism for packages to be able to force --{enable,disable,with,without}-option of other, autoconf-based packages, or -D{ENABLE,DISABLE}_OPTION for other, cmake-based packages, and similarly for all infras. Nope; no, no, no... Regards, Yann E. MORIN. > > Francois > > > Regards, > Arnout > > > +endef > > + > > $(eval $(autotools-package)) > _______________________________________________ > buildroot mailing list > [8]buildroot@buildroot.org > [9]https://lists.buildroot.org/mailman/listinfo/buildroot > > > References: > > [1] mailto:buildroot@buildroot.org > [2] mailto:francois.perrad@gadz.org > [3] http://diffutils.mk/ > [4] http://diffutils.mk/ > [5] http://diffutils.mk/ > [6] http://diffutils.mk/ > [7] http://diffutils.mk/ > [8] mailto:buildroot@buildroot.org > [9] https://lists.buildroot.org/mailman/listinfo/buildroot > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
diff --git a/package/diffutils/diffutils.mk b/package/diffutils/diffutils.mk index 111926686..fe20a0e67 100644 --- a/package/diffutils/diffutils.mk +++ b/package/diffutils/diffutils.mk @@ -18,4 +18,8 @@ ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y) DIFFUTILS_CONF_ENV += gl_cv_func_getopt_gnu=yes endif +define DIFFUTILS_BUSYBOX_CONFIG_FIXUPS + $(call KCONFIG_DISABLE_OPT,CONFIG_DIFF) +endef + $(eval $(autotools-package))
Signed-off-by: Francois Perrad <francois.perrad@gadz.org> --- package/diffutils/diffutils.mk | 4 ++++ 1 file changed, 4 insertions(+)