[PATCH 1/2] allow honouring libc provided SIGRTMIN/SIGRTMAX

[Rasmus Villemoes]

When an application documents that it responds such and such to
SIGRTMIN+n, that almost always means with respect to the libc-provided
SIGRTMIN. Hence I disagree with the "more correct" in commit
7b276fc17594. In any case, this is rather unfortunate:

# kill -l RTMIN+2
36
# busybox kill -l RTMIN+2
34

(the first shell is bash). We probably can't change default behaviour
after 7 years, but at least we can provide a config option.

We avoid a little code generation (repeated calls to
__libc_current_sigrtmin) by stashing SIGRTMIN/SIGRTMAX in local
variables, but it does cost ~50 bytes. The next patch serves as penance
for that.

Signed-off-by: Rasmus Villemoes 
---
 libbb/u_signal_names.c | 49 -
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/libbb/u_signal_names.c b/libbb/u_signal_names.c
index b3038e32d..5ef5c3f28 100644
--- a/libbb/u_signal_names.c
+++ b/libbb/u_signal_names.c
@@ -12,6 +12,15 @@
 //config:  help
 //config:  Support RTMIN[+n] and RTMAX[-n] signal names
 //config:  in kill, killall etc. This costs ~250 bytes.
+//config:config FEATURE_RTMINMAX_USE_LIBC_DEFINITIONS
+//config:  bool "Use the definitions of SIGRTMIN/SIGRTMAX provided by libc"
+//config:  depends on FEATURE_RTMINMAX
+//config:  help
+//config:  Some C libraries reserve a few real-time signals for internal
+//config:  use, and adjust the values of SIGRTMIN/SIGRTMAX seen by
+//config:  applications accordingly. Saying yes here means that a signal
+//config:  name RTMIN+n will be interpreted according to the libc 
definition
+//config:  of SIGRTMIN, and not the raw definition provided by the kernel.
 
 #include "libbb.h"
 
@@ -123,7 +132,7 @@ static const char signals[][7] ALIGN1 = {
 #ifdef SIGSYS
[SIGSYS   ] = "SYS",
 #endif
-#if ENABLE_FEATURE_RTMINMAX
+#if ENABLE_FEATURE_RTMINMAX && !ENABLE_FEATURE_RTMINMAX_USE_LIBC_DEFINITIONS
 # ifdef __SIGRTMIN
[__SIGRTMIN] = "RTMIN",
 # endif
@@ -140,6 +149,7 @@ static const char signals[][7] ALIGN1 = {
 int FAST_FUNC get_signum(const char *name)
 {
unsigned i;
+   unsigned sigrtmin, sigrtmax;
 
i = bb_strtou(name, NULL, 10);
if (!errno && i < NSIG) /* for shells, we allow 0 too */
@@ -168,10 +178,13 @@ int FAST_FUNC get_signum(const char *name)
 # endif
 #endif
 
-#if ENABLE_FEATURE_RTMINMAX
-# if defined(SIGRTMIN) && defined(SIGRTMAX)
-/* libc may use some rt sigs for pthreads and therefore "remap" SIGRTMIN/MAX,
- * but we want to use "raw" SIGRTMIN/MAX. Underscored names, if exist, provide
+#if ENABLE_FEATURE_RTMINMAX && defined(SIGRTMIN) && defined(SIGRTMAX)
+# if ENABLE_FEATURE_RTMINMAX_USE_LIBC_DEFINITIONS
+/* Use the libc provided values. */
+   sigrtmin = SIGRTMIN;
+   sigrtmax = SIGRTMAX;
+# else
+/* Use the "raw SIGRTMIN/MAX. Underscored names, if exist, provide
  * them. If they don't exist, fall back to non-underscored ones: */
 #  if !defined(__SIGRTMIN)
 #   define __SIGRTMIN SIGRTMIN
@@ -179,25 +192,27 @@ int FAST_FUNC get_signum(const char *name)
 #  if !defined(__SIGRTMAX)
 #   define __SIGRTMAX SIGRTMAX
 #  endif
+   sigrtmin = __SIGRTMIN;
+   sigrtmax = __SIGRTMAX;
+# endif
if (strncasecmp(name, "RTMIN", 5) == 0) {
if (!name[5])
-   return __SIGRTMIN;
+   return sigrtmin;
if (name[5] == '+') {
i = bb_strtou(name + 6, NULL, 10);
-   if (!errno && i <= __SIGRTMAX - __SIGRTMIN)
-   return __SIGRTMIN + i;
+   if (!errno && i <= sigrtmax - sigrtmin)
+   return sigrtmin + i;
}
}
else if (strncasecmp(name, "RTMAX", 5) == 0) {
if (!name[5])
-   return __SIGRTMAX;
+   return sigrtmax;
if (name[5] == '-') {
i = bb_strtou(name + 6, NULL, 10);
-   if (!errno && i <= __SIGRTMAX - __SIGRTMIN)
-   return __SIGRTMAX - i;
+   if (!errno && i <= sigrtmax - sigrtmin)
+   return sigrtmax - i;
}
}
-# endif
 #endif
 
return -1;
@@ -228,8 +243,16 @@ void FAST_FUNC print_signames(void)
printf("%2u) %s\n", signo, name);
}
 #if ENABLE_FEATURE_RTMINMAX
-# ifdef __SIGRTMAX
+# if ENABLE_FEATURE_RTMINMAX_USE_LIBC_DEFINITIONS
+#  if defined(SIGRTMIN) && defined(SIGRTMAX)
+   printf("%2u) %s\n", SIGRTMIN, "RTMIN");
+   printf("%2u) %s\n", SIGRTMAX, "RTMAX");
+#  endif
+# else
+//

[PATCH 2/2] libbb/u_signal_names.c: don't check errno after bb_strtou

[Rasmus Villemoes]

Since we're comparing the return value to a smallish integer anyway, we
might as well use that bb_strtou() returns UINT_MAX for malformed
input. Referencing errno is kinda bloaty on glibc. Seems to save ~34 or
~45 bytes (depending on RTMINMAX_USE_LIBC_DEFINITIONS).

While NSIG is not in POSIX, we do already rely on it being defined,
compile-time const and smallish, since arrays in struct globals_misc are
defined in terms of it.

Signed-off-by: Rasmus Villemoes 
---
 libbb/u_signal_names.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libbb/u_signal_names.c b/libbb/u_signal_names.c
index 5ef5c3f28..88f63192d 100644
--- a/libbb/u_signal_names.c
+++ b/libbb/u_signal_names.c
@@ -151,8 +151,12 @@ int FAST_FUNC get_signum(const char *name)
unsigned i;
unsigned sigrtmin, sigrtmax;
 
+   /* bb_strtou returns UINT_MAX on error. We should be able to
+* assume NSIG is not that big... Hence no need to check errno
+* after bb_strtou(). */
+   BUILD_BUG_ON(NSIG >= UINT_MAX);
i = bb_strtou(name, NULL, 10);
-   if (!errno && i < NSIG) /* for shells, we allow 0 too */
+   if (i < NSIG) /* for shells, we allow 0 too */
return i;
if (strncasecmp(name, "SIG", 3) == 0)
name += 3;
@@ -200,7 +204,7 @@ int FAST_FUNC get_signum(const char *name)
return sigrtmin;
if (name[5] == '+') {
i = bb_strtou(name + 6, NULL, 10);
-   if (!errno && i <= sigrtmax - sigrtmin)
+   if (i <= sigrtmax - sigrtmin)
return sigrtmin + i;
}
}
@@ -209,7 +213,7 @@ int FAST_FUNC get_signum(const char *name)
return sigrtmax;
if (name[5] == '-') {
i = bb_strtou(name + 6, NULL, 10);
-   if (!errno && i <= sigrtmax - sigrtmin)
+   if (i <= sigrtmax - sigrtmin)
return sigrtmax - i;
}
}
-- 
2.16.4

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 1/2] allow honouring libc provided SIGRTMIN/SIGRTMAX

[Rasmus Villemoes]

On 2018-09-12 16:06, Rasmus Villemoes wrote:
> When an application documents that it responds such and such to
> SIGRTMIN+n, that almost always means with respect to the libc-provided
> SIGRTMIN. Hence I disagree with the "more correct" in commit
> 7b276fc17594. In any case, this is rather unfortunate:
> 
> # kill -l RTMIN+2
> 36
> # busybox kill -l RTMIN+2
> 34
> 
> (the first shell is bash). We probably can't change default behaviour
> after 7 years, but at least we can provide a config option.

ping
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 1/2] allow honouring libc provided SIGRTMIN/SIGRTMAX

[Rasmus Villemoes]

On 2018-10-05 09:08, Rasmus Villemoes wrote:
> On 2018-09-12 16:06, Rasmus Villemoes wrote:
>> When an application documents that it responds such and such to
>> SIGRTMIN+n, that almost always means with respect to the libc-provided
>> SIGRTMIN. Hence I disagree with the "more correct" in commit
>> 7b276fc17594. In any case, this is rather unfortunate:
>>
>> # kill -l RTMIN+2
>> 36
>> # busybox kill -l RTMIN+2
>> 34
>>
>> (the first shell is bash). We probably can't change default behaviour
>> after 7 years, but at least we can provide a config option.
> 
> ping

ping2
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 1/2] allow honouring libc provided SIGRTMIN/SIGRTMAX

[Rasmus Villemoes]

On 2018-10-31 11:13, Denys Vlasenko wrote:
> Applied, thanks!

Thanks, but the commit message was mangled somehow, so now it just reads


In any case, this is rather unfortunate:

36
34



I.e. the lines beginning with # were stripped, making it rather
confusing. You may want to look into setting commit.cleanup=scissors or
whitespace to avoid that kind of thing to happen again.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: Patch: use BB_GLOBAL_CONST where applicable

[Rasmus Villemoes]

On 06/06/2019 20.19, Denys Vlasenko wrote:
> On Wed, Jun 5, 2019 at 5:51 PM Luís Marques  wrote:
>> Hello,
>>
>> This patch mainly intends to add Clang/LLVM support, which currently is 
>> broken.
>>
>> Problem: the const pointer trick (used by the struct globals, etc.) is
>> technically undefined behavior. In practice, it causes problems with
>> an LLVM-based toolchain, since LLVM optimizes away the store to the
>> const pointer (despite the memory barrier), and therefore the applets
>> crash when they dereference the null pointers.
> 
> Can you experiment with LLVM and find a definition of SET_PTR_TO_GLOBALS()
> which works for it?

I think something like this should work without invoking UB - from the C
_compiler_'s perspective, we never cast a const qualifier away, so all
accesses to the objects go through proper lvalues. It's just that the
assembler and linker have colluded to make the two objects occupy the
same memory location (in .data, or perhaps .bss).

$ grep . g.c h.c
g.c:static int __some_global;
g.c:extern const int some_global
__attribute__((__alias__("__some_global")));
g.c:void init(void)
g.c:{
g.c:__some_global = 123;
g.c:}
h.c:#include 
h.c:extern void init(void);
h.c:extern const int some_global;
h.c:int main(int argc, char *argv[])
h.c:{
h.c:init();
h.c:printf("%d\n", some_global);
h.c:printf("%d\n", some_global);
h.c:return 0;
h.c:}

$ gcc -O2 -U_FORTIFY_SOURCE -o g.o -c g.c
$ gcc -O2 -U_FORTIFY_SOURCE -o h.o -c h.c
$ nm g.o
 T init
 b __some_global
 B some_global

and objdump on h.o shows that some_global gets loaded just once to a
callee-saved register:
 :
   0:   53  push   %rbx
   1:   e8 00 00 00 00  callq  6  2:
R_X86_64_PLT32   init-0x4
   6:   8b 1d 00 00 00 00   mov0x0(%rip),%ebx# c
 8: R_X86_64_PC32some_global-0x4
   c:   48 8d 3d 00 00 00 00lea0x0(%rip),%rdi# 13
   f: R_X86_64_PC32.LC0-0x4
  13:   31 c0   xor%eax,%eax
  15:   89 de   mov%ebx,%esi
  17:   e8 00 00 00 00  callq  1c18:
R_X86_64_PLT32  printf-0x4
  1c:   48 8d 3d 00 00 00 00lea0x0(%rip),%rdi# 23
   1f: R_X86_64_PC32   .LC0-0x4
  23:   89 de   mov%ebx,%esi
  25:   31 c0   xor%eax,%eax
  27:   e8 00 00 00 00  callq  2c28:
R_X86_64_PLT32  printf-0x4
  2c:   31 c0   xor%eax,%eax
  2e:   5b  pop%rbx
  2f:   c3  retq

So this of course requires that the code that initializes __some_global
is guaranteed to run before anything can look at some_global - in fact,
above, the compiler _could_ have decided to hoist the load of
some_global before the call of init(), but that seems to be true for the
current situation as well.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH] bb_make_directory(): simplify and robustify

[Rasmus Villemoes]

For something like 'mkdir -m 0700 foo', if the caller happens to have
a permissive umask (say allowing group write via 0007 or 0002), the
created directory will temporarily have more permissions than
requested. That's a mild security issue.

This reworks bb_make_directory() to always create both intermediate
and the final component with 0 permissions, then chmods to the final
value.

Aside from the robustification, this also simplifies the code
somewhat (net -31LOC), since we get rid of the complicated "what umask
do we have now and be sure to set it back" - we only need to query the
current umask in case mode==-1, and can set it back immediately. Then
mode contains the mode of the final component, and intermediate
components should then be chmod'ed to pmode=mode|0300.

x86_64 defconfig:

function old new   delta
bb_make_directory452     396     -56

Signed-off-by: Rasmus Villemoes 
---
Lightly tested and strace'd to see that it behaves as expected.

 libbb/make_directory.c | 83 +-
 1 file changed, 26 insertions(+), 57 deletions(-)

diff --git a/libbb/make_directory.c b/libbb/make_directory.c
index 9b03bb8d0..9d95c2383 100644
--- a/libbb/make_directory.c
+++ b/libbb/make_directory.c
@@ -26,9 +26,8 @@
 
 int FAST_FUNC bb_make_directory(char *path, long mode, int flags)
 {
-   mode_t cur_mask;
-   mode_t org_mask;
const char *fail_msg;
+   mode_t pmode;
char *s;
char c;
struct stat st;
@@ -48,7 +47,13 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int 
flags)
 // return 0; /* ".." */
}
 
-   org_mask = cur_mask = (mode_t)-1L;
+   if (mode == -1) {
+   mode_t org_mask = umask(0);
+   mode = 0777 & ~org_mask;
+   umask(org_mask);
+   }
+   pmode = mode | 0300;
+
s = path;
while (1) {
c = '\0';
@@ -68,32 +73,8 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int 
flags)
}
}
 
-   if (c != '\0') {
-   /* Intermediate dirs: must have wx for user */
-   if (cur_mask == (mode_t)-1L) { /* wasn't done yet? */
-   mode_t new_mask;
-   org_mask = umask(0);
-   cur_mask = 0;
-   /* Clear u=wx in umask - this ensures
-* they won't be cleared on mkdir */
-   new_mask = (org_mask & ~(mode_t)0300);
-   //bb_error_msg("org_mask:%o cur_mask:%o", 
org_mask, new_mask);
-   if (new_mask != cur_mask) {
-   cur_mask = new_mask;
-   umask(new_mask);
-   }
-   }
-   } else {
-   /* Last component: uses original umask */
-   //bb_error_msg("1 org_mask:%o", org_mask);
-   if (org_mask != cur_mask) {
-   cur_mask = org_mask;
-   umask(org_mask);
-   }
-   }
-
//bb_error_msg("mkdir '%s'", path);
-   if (mkdir(path, 0777) < 0) {
+   if (mkdir(path, ) < 0) {
/* If we failed for any other reason than the directory
 * already exists, output a diagnostic and return -1 */
if ((errno != EEXIST && errno != EISDIR)
@@ -103,36 +84,31 @@ int FAST_FUNC bb_make_directory(char *path, long mode, int 
flags)
fail_msg = "create";
break;
}
-   /* Since the directory exists, don't attempt to change
-* permissions if it was the full target.  Note that
-* this is not an error condition. */
-   if (!c) {
-   goto ret0;
-   }
+   /* Since the directory exists, don't attempt
+* to change permissions.  Note that this is
+* not an error condition. */
} else {
if (flags & FILEUTILS_VERBOSE) {
printf("created directory: '%s'\n", path);
}
-   }
-
-   if (!c) {
-   /* Done.  If necessary, update perms on the newly
-* created directory.  Failure

Re: [PATCH] bb_make_directory(): simplify and robustify

[Rasmus Villemoes]

On 11/03/2020 13.22, Rasmus Villemoes wrote:
> For something like 'mkdir -m 0700 foo', if the caller happens to have
> a permissive umask (say allowing group write via 0007 or 0002), the
> created directory will temporarily have more permissions than
> requested. That's a mild security issue.
> 
> This reworks bb_make_directory() to always create both intermediate
> and the final component with 0 permissions, then chmods to the final
> value.

Urgh, please ignore this patch. While it works as advertised, it may
break the case of two processes doing "mkdir -p a/b/c" and "mkdir -p
a/b/d" in parallel - if b is created by the first process, but not yet
chmod'ed, the second process will fail. So newly created intermediate
directories must be born with at least u+wx, which means there's no way
around umask fiddling :(

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

When the nowayout option is used with a watchdog device, busybox'
current behaviour of always doing a open/write magic char/close
sequence before the "real" open causes warning messages in the kernel
log:

[   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
[   16.212196] watchdog: watchdog0: watchdog did not stop!

The latter may also appear by itself in case the watchdog is of the
type that cannot be stopped once started.

These warnings can be somewhat ominous and distracting, so allow
configuring whether to use this open-write-close-open sequence rather
than just open. Also saves a bit of .text when disabled:

function old new   delta
shutdown_on_signal31  58 +27
watchdog_main339 306 -33
shutdown_watchdog 34   - -34
--
(add/remove: 0/1 grow/shrink: 1/1 up/down: 27/-67)Total: -40 bytes

Signed-off-by: Rasmus Villemoes 
---
 miscutils/watchdog.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/miscutils/watchdog.c b/miscutils/watchdog.c
index 0ed10bcf1..2a8ff2de6 100644
--- a/miscutils/watchdog.c
+++ b/miscutils/watchdog.c
@@ -18,6 +18,17 @@
 //config:  watchdog applet ever fails to write the magic character within a
 //config:  certain amount of time, the watchdog device assumes the system 
has
 //config:  hung, and will cause the hardware to reboot.
+//config:
+//config:config FEATURE_WATCHDOG_OPEN_TWICE
+//config:  bool "Open watchdog device twice, closing it gracefully in 
between"
+//config:  depends on WATCHDOG
+//config:  default y
+//config:  help
+//config:  When enabled, the watchdog device is opened and then immediately
+//config:  magic-closed, before being opened a second time. This may be 
necessary
+//config:  for some watchdog devices, but can cause spurious warnings in 
the
+//config:  kernel log if the nowayout feature is enabled. Say n if you know
+//config:  you don't need this and want to avoid those spurious warnings.
 
 //applet:IF_WATCHDOG(APPLET(watchdog, BB_DIR_SBIN, BB_SUID_DROP))
 
@@ -73,6 +84,7 @@ static void watchdog_open(const char* device)
/* Use known fd # - avoid needing global 'int fd' */
xmove_fd(xopen(device, O_WRONLY), 3);
 
+#if ENABLE_FEATURE_WATCHDOG_OPEN_TWICE
/* If the watchdog driver can do something other than cause a reboot
 * on a timeout, then it's possible this program may be starting from
 * a state when the watchdog hadn't been previously stopped with
@@ -82,6 +94,7 @@ static void watchdog_open(const char* device)
shutdown_watchdog();
 
xmove_fd(xopen(device, O_WRONLY), 3);
+#endif
 }
 
 int watchdog_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
-- 
2.29.2

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 1/2] libbb: add xopen_fd() helper

[Rasmus Villemoes]

xmove_fd(xopen(...), ...) is a common pattern. Add a helper for
that. On x86-64, this compiles to 18 bytes, so just a few call sites
need to be converted for a net win.

Signed-off-by: Rasmus Villemoes 
---
 include/libbb.h   | 1 +
 libbb/xfuncs_printf.c | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/include/libbb.h b/include/libbb.h
index cb6336474..ce5eaa5fb 100644
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -608,6 +608,7 @@ int open3_or_warn(const char *pathname, int flags, int 
mode) FAST_FUNC;
 int open_or_warn(const char *pathname, int flags) FAST_FUNC;
 int xopen3(const char *pathname, int flags, int mode) FAST_FUNC;
 int xopen(const char *pathname, int flags) FAST_FUNC;
+void xopen_fd(const char *pathname, int flags, int fd) FAST_FUNC;
 int xopen_nonblocking(const char *pathname) FAST_FUNC;
 int xopen_as_uid_gid(const char *pathname, int flags, uid_t u, gid_t g) 
FAST_FUNC;
 int open_or_warn_stdin(const char *pathname) FAST_FUNC;
diff --git a/libbb/xfuncs_printf.c b/libbb/xfuncs_printf.c
index 99596b9d0..f7c439c2f 100644
--- a/libbb/xfuncs_printf.c
+++ b/libbb/xfuncs_printf.c
@@ -160,6 +160,12 @@ int FAST_FUNC xopen(const char *pathname, int flags)
return xopen3(pathname, flags, 0666);
 }
 
+// Die if we can't open a file as a given fd.
+void FAST_FUNC xopen_fd(const char *pathname, int flags, int fd)
+{
+   return xmove_fd(xopen(pathname, flags), fd);
+}
+
 // Warn if we can't open a file and return a fd.
 int FAST_FUNC open3_or_warn(const char *pathname, int flags, int mode)
 {
-- 
2.29.2

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 2/2] tree-wide: use xopen_fd

[Rasmus Villemoes]

Typical saving per callsite (on x86-64) is 7 bytes, so net win at 3
conversions.

function old new   delta
xopen_fd   -  18 +18
xargs_main   871 864  -7
vlock_main   409 402  -7
uniq_main463 456  -7
ubi_tools_main  12981291  -7
split_main   581 574  -7
sort_main   10651058  -7
shuf_main550 543  -7
reinitialize 206 199  -7
nc_main 12251218  -7
mkfs_minix_main 28652858  -7
mkfs_ext2_main  24052398  -7
fsck_minix_main 31453138  -7
acpid_main  13261319  -7
cpio_main654 645  -9
close_dev_fd  28  19  -9
watchdog_main335 321 -14
dd_main 16421628 -14
--
(add/remove: 1/0 grow/shrink: 0/17 up/down: 18/-137) Total: -119 bytes

Signed-off-by: Rasmus Villemoes 
---
 archival/cpio.c | 6 +++---
 coreutils/dd.c  | 4 ++--
 coreutils/shuf.c| 2 +-
 coreutils/sort.c| 2 +-
 coreutils/split.c   | 2 +-
 coreutils/uniq.c| 2 +-
 findutils/xargs.c   | 2 +-
 loginutils/vlock.c  | 2 +-
 miscutils/less.c| 2 +-
 miscutils/ubi_tools.c   | 2 +-
 miscutils/watchdog.c| 4 ++--
 networking/nc_bloaty.c  | 2 +-
 util-linux/acpid.c  | 2 +-
 util-linux/fdisk.c  | 2 +-
 util-linux/fsck_minix.c | 2 +-
 util-linux/mkfs_ext2.c  | 2 +-
 util-linux/mkfs_minix.c | 2 +-
 17 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/archival/cpio.c b/archival/cpio.c
index 94303389e..ab918f61a 100644
--- a/archival/cpio.c
+++ b/archival/cpio.c
@@ -405,11 +405,11 @@ int cpio_main(int argc UNUSED_PARAM, char **argv)
}
 #if !ENABLE_FEATURE_CPIO_O
if (opt & OPT_FILE) { /* -F */
-   xmove_fd(xopen(cpio_filename, O_RDONLY), STDIN_FILENO);
+   xopen_fd(cpio_filename, O_RDONLY, STDIN_FILENO);
}
 #else
if ((opt & (OPT_FILE|OPT_CREATE)) == OPT_FILE) { /* -F without -o */
-   xmove_fd(xopen(cpio_filename, O_RDONLY), STDIN_FILENO);
+   xopen_fd(cpio_filename, O_RDONLY, STDIN_FILENO);
}
if (opt & OPT_PASSTHROUGH) {
pid_t pid;
@@ -459,7 +459,7 @@ int cpio_main(int argc UNUSED_PARAM, char **argv)
if (cpio_fmt[0] != 'n') /* we _require_ "-H newc" */
bb_show_usage();
if (opt & OPT_FILE) {
-   xmove_fd(xopen(cpio_filename, O_WRONLY | O_CREAT | 
O_TRUNC), STDOUT_FILENO);
+   xopen_fd(cpio_filename, O_WRONLY | O_CREAT | O_TRUNC, 
STDOUT_FILENO);
}
  dump:
return cpio_o();
diff --git a/coreutils/dd.c b/coreutils/dd.c
index 24d7f0b84..215c61de9 100644
--- a/coreutils/dd.c
+++ b/coreutils/dd.c
@@ -500,7 +500,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
if (G.flags & FLAG_IDIRECT)
iflag |= O_DIRECT;
 #endif
-   xmove_fd(xopen(infile, iflag), ifd);
+   xopen_fd(infile, iflag, ifd);
} else {
infile = bb_msg_standard_input;
}
@@ -515,7 +515,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
if (G.flags & FLAG_ODIRECT)
oflag |= O_DIRECT;
 #endif
-   xmove_fd(xopen(outfile, oflag), ofd);
+   xopen_fd(outfile, oflag, ofd);
 
if (seek && !(G.flags & FLAG_NOTRUNC)) {
size_t blocksz = (G.flags & FLAG_SEEK_BYTES) ? 1 : obs;
diff --git a/coreutils/shuf.c b/coreutils/shuf.c
index fdbd3e9b2..a347eb645 100644
--- a/coreutils/shuf.c
+++ b/coreutils/shuf.c
@@ -132,7 +132,7 @@ int shuf_main(int argc, char **argv)
shuffle_lines(lines, numlines);
 
if (opts & OPT_o)
-   xmove_fd(xopen(opt_o_str, O_WRONLY|O_CREAT|O_TRUNC), 
STDOUT_FILENO);
+   xopen_fd(opt_o_str, O_WRONLY | O_CREAT | O_TRUNC, 
STDOUT_FILENO);
 
if (opts & OPT_n) {
unsigned maxlines;
diff --git a/coreutils/sort.c b/coreutils/sort.c
index b194847d1..36c160c16 100644
--- a/coreutils/sort

Re: AW: [PATCH 1/2] libbb: add xopen_fd() helper

[Rasmus Villemoes]

On 01/03/2021 18.14, Walter Harms wrote:
> nice observation,
> 
> but IMHO the name is a bit misleading, i6 should be more generic like:
> xopen_as()

he, I actually considered that name; I'm fine with that one as well.
I'll wait a day or two, then resend with a global search-replace done.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v2 2/2] tree-wide: use xopen_as

[Rasmus Villemoes]

Typical saving per callsite (on x86-64) is 7 bytes, so net win at 3
conversions.

function old new   delta
xopen_as   -  18 +18
xargs_main   871 864  -7
vlock_main   409 402  -7
uniq_main463 456  -7
ubi_tools_main  12981291  -7
split_main   581 574  -7
sort_main   10651058  -7
shuf_main550 543  -7
reinitialize 206 199  -7
nc_main 12251218  -7
mkfs_minix_main 28652858  -7
mkfs_ext2_main  24052398  -7
fsck_minix_main 31453138  -7
acpid_main  13261319  -7
cpio_main654 645  -9
close_dev_fd  28  19  -9
watchdog_main335 321 -14
dd_main 16421628 -14
--
(add/remove: 1/0 grow/shrink: 0/17 up/down: 18/-137) Total: -119 bytes

Signed-off-by: Rasmus Villemoes 
---
 archival/cpio.c | 6 +++---
 coreutils/dd.c  | 4 ++--
 coreutils/shuf.c| 2 +-
 coreutils/sort.c| 2 +-
 coreutils/split.c   | 2 +-
 coreutils/uniq.c| 2 +-
 findutils/xargs.c   | 2 +-
 loginutils/vlock.c  | 2 +-
 miscutils/less.c| 2 +-
 miscutils/ubi_tools.c   | 2 +-
 miscutils/watchdog.c| 4 ++--
 networking/nc_bloaty.c  | 2 +-
 util-linux/acpid.c  | 2 +-
 util-linux/fdisk.c  | 2 +-
 util-linux/fsck_minix.c | 2 +-
 util-linux/mkfs_ext2.c  | 2 +-
 util-linux/mkfs_minix.c | 2 +-
 17 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/archival/cpio.c b/archival/cpio.c
index 94303389e..0d25ead91 100644
--- a/archival/cpio.c
+++ b/archival/cpio.c
@@ -405,11 +405,11 @@ int cpio_main(int argc UNUSED_PARAM, char **argv)
}
 #if !ENABLE_FEATURE_CPIO_O
if (opt & OPT_FILE) { /* -F */
-   xmove_fd(xopen(cpio_filename, O_RDONLY), STDIN_FILENO);
+   xopen_as(cpio_filename, O_RDONLY, STDIN_FILENO);
}
 #else
if ((opt & (OPT_FILE|OPT_CREATE)) == OPT_FILE) { /* -F without -o */
-   xmove_fd(xopen(cpio_filename, O_RDONLY), STDIN_FILENO);
+   xopen_as(cpio_filename, O_RDONLY, STDIN_FILENO);
}
if (opt & OPT_PASSTHROUGH) {
pid_t pid;
@@ -459,7 +459,7 @@ int cpio_main(int argc UNUSED_PARAM, char **argv)
if (cpio_fmt[0] != 'n') /* we _require_ "-H newc" */
bb_show_usage();
if (opt & OPT_FILE) {
-   xmove_fd(xopen(cpio_filename, O_WRONLY | O_CREAT | 
O_TRUNC), STDOUT_FILENO);
+   xopen_as(cpio_filename, O_WRONLY | O_CREAT | O_TRUNC, 
STDOUT_FILENO);
}
  dump:
return cpio_o();
diff --git a/coreutils/dd.c b/coreutils/dd.c
index 24d7f0b84..6fd8b1daf 100644
--- a/coreutils/dd.c
+++ b/coreutils/dd.c
@@ -500,7 +500,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
if (G.flags & FLAG_IDIRECT)
iflag |= O_DIRECT;
 #endif
-   xmove_fd(xopen(infile, iflag), ifd);
+   xopen_as(infile, iflag, ifd);
} else {
infile = bb_msg_standard_input;
}
@@ -515,7 +515,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
if (G.flags & FLAG_ODIRECT)
oflag |= O_DIRECT;
 #endif
-   xmove_fd(xopen(outfile, oflag), ofd);
+   xopen_as(outfile, oflag, ofd);
 
if (seek && !(G.flags & FLAG_NOTRUNC)) {
size_t blocksz = (G.flags & FLAG_SEEK_BYTES) ? 1 : obs;
diff --git a/coreutils/shuf.c b/coreutils/shuf.c
index fdbd3e9b2..b9daaaf38 100644
--- a/coreutils/shuf.c
+++ b/coreutils/shuf.c
@@ -132,7 +132,7 @@ int shuf_main(int argc, char **argv)
shuffle_lines(lines, numlines);
 
if (opts & OPT_o)
-   xmove_fd(xopen(opt_o_str, O_WRONLY|O_CREAT|O_TRUNC), 
STDOUT_FILENO);
+   xopen_as(opt_o_str, O_WRONLY | O_CREAT | O_TRUNC, 
STDOUT_FILENO);
 
if (opts & OPT_n) {
unsigned maxlines;
diff --git a/coreutils/sort.c b/coreutils/sort.c
index b194847d1..899f2cc51 100644
--- a/coreutils/sort

[PATCH v2 1/2] libbb: add xopen_as() helper

[Rasmus Villemoes]

xmove_fd(xopen(...), ...) is a common pattern. Add a helper for
that. On x86-64, this compiles to 18 bytes, so just a few call sites
need to be converted for a net win.

Signed-off-by: Rasmus Villemoes 
---
 include/libbb.h   | 1 +
 libbb/xfuncs_printf.c | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/include/libbb.h b/include/libbb.h
index ece03e7d8..bdd99403d 100644
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -608,6 +608,7 @@ int open3_or_warn(const char *pathname, int flags, int 
mode) FAST_FUNC;
 int open_or_warn(const char *pathname, int flags) FAST_FUNC;
 int xopen3(const char *pathname, int flags, int mode) FAST_FUNC;
 int xopen(const char *pathname, int flags) FAST_FUNC;
+void xopen_as(const char *pathname, int flags, int fd) FAST_FUNC;
 int xopen_nonblocking(const char *pathname) FAST_FUNC;
 int xopen_as_uid_gid(const char *pathname, int flags, uid_t u, gid_t g) 
FAST_FUNC;
 int open_or_warn_stdin(const char *pathname) FAST_FUNC;
diff --git a/libbb/xfuncs_printf.c b/libbb/xfuncs_printf.c
index f0399ca45..255e80190 100644
--- a/libbb/xfuncs_printf.c
+++ b/libbb/xfuncs_printf.c
@@ -160,6 +160,12 @@ int FAST_FUNC xopen(const char *pathname, int flags)
return xopen3(pathname, flags, 0666);
 }
 
+// Die if we can't open a file as a given fd.
+void FAST_FUNC xopen_as(const char *pathname, int flags, int fd)
+{
+   return xmove_fd(xopen(pathname, flags), fd);
+}
+
 // Warn if we can't open a file and return a fd.
 int FAST_FUNC open3_or_warn(const char *pathname, int flags, int mode)
 {
-- 
2.29.2

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

On 01/03/2021 16.11, Rasmus Villemoes wrote:
> When the nowayout option is used with a watchdog device, busybox'
> current behaviour of always doing a open/write magic char/close
> sequence before the "real" open causes warning messages in the kernel
> log:
> 
> [   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
> [   16.212196] watchdog: watchdog0: watchdog did not stop!
> 
> The latter may also appear by itself in case the watchdog is of the
> type that cannot be stopped once started.
> 
> These warnings can be somewhat ominous and distracting, so allow
> configuring whether to use this open-write-close-open sequence rather
> than just open.

ping
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

On 09/03/2021 13.33, Rasmus Villemoes wrote:
> On 01/03/2021 16.11, Rasmus Villemoes wrote:
>> When the nowayout option is used with a watchdog device, busybox'
>> current behaviour of always doing a open/write magic char/close
>> sequence before the "real" open causes warning messages in the kernel
>> log:
>>
>> [   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
>> [   16.212196] watchdog: watchdog0: watchdog did not stop!
>>
>> The latter may also appear by itself in case the watchdog is of the
>> type that cannot be stopped once started.
>>
>> These warnings can be somewhat ominous and distracting, so allow
>> configuring whether to use this open-write-close-open sequence rather
>> than just open.
> 
> ping

ping^2

Come to think of it, I actually think the open_twice feature should be
default n. It seems one might as well start the watchdog daemon with a
wrapper that does "printf 'V' > /dev/watchdog0; exec watchdog ...". But
either way, please consider this patch.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

On 15/03/2021 12.16, dewelo...@wp.pl wrote:
> Dnia 2021-03-15, o godz. 09:03:07

> 
> It seems that the open-write-close-open sequence has been introduced in 
> busybox 1.28.0 by commit 
> https://git.busybox.net/busybox/commit/?h=31c765081dc41f158786545fbea9294be4685bd2

Yeah, I could probably have spelled that out; I did cc the author of
that commit.

> AFAIU the commit message says that it is a kind of a workaround for some 
> possible implementations of hardware watchdogs, which could enter wrong state 
> when their supervisor in userspace is restarted (confused by close/open? I 
> don't know). But it doesn't name any single example of an existing watchdog 
> like that.
> 
> In my case (softdog nowayout=1) that "fix" only adds:
> - bloat to busybox, and
> - annoying critical error message from kernel (yes, "watchdog: watchdog0: 
> watchdog did not stop!" is at KERN_CRIT level)
> 
> So I'm definitely for making the behaviour introduced by commit 
> 31c765081dc41f158786545fbea9294be4685bd2 optional.

Thanks.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

On 15/03/2021 23.41, dewelo...@wp.pl wrote:
> Dnia 2021-03-15, o godz. 08:26:38
> Matt Spinler  napisał(a):
> 
 for not responding sooner.
>> In my case, the code is on a BMC (OpenBMC), and the output of the 
>> watchdog is wired to a fan watchdog hardware circuit (involving a latch 
>> and a standalone fan control chip), which the watchdog app is pinging.  
>> So if the watchdog app dies the fans get set to full speed, however on 
>> the next main power cycle of the server (the BMC is on a different power 
>> domain) we want things back to normal, so the watchdog has to work again 
>> when it is restarted, which it wasn't doing without my change.

Thanks for the explanation. Can you point me at the watchdog driver code
which does this "get things back to normal" on a (graceful) shutdown/stop?

> I wonder what is the "architecturally correct" way of fixing this kind of 
> problem, since doing "stop" as a way to "start" seems counter-intuitive.

Indeed.

Denys, can you consider applying the up-thread patch. Whether you'll
make it default n or y is up to you. Even if it's n, I think one can
achieve the same thing with a wrapper

#!/bin/sh
( shift $(($# - 1)) && [ -c "$1" ] && printf 'V' > "$1" )
exec busybox watchdog "$@"

(or in any other number of ways, depending on how one starts the
daemon). But there is no way of avoiding the annoying "nowayout"
messages from the kernel if busybox is built with this
open-write-close-open.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] watchdog: make open-close-open functionality a config knob

[Rasmus Villemoes]

On 17/03/2021 22.35, Matt Spinler wrote:
> 
> 
> On 3/17/2021 3:42 PM, Rasmus Villemoes wrote:
>> On 15/03/2021 23.41, dewelo...@wp.pl wrote:
>>> Dnia 2021-03-15, o godz. 08:26:38
>>> Matt Spinler  napisał(a):
>>>
>>   for not responding sooner.
>>>> In my case, the code is on a BMC (OpenBMC), and the output of the
>>>> watchdog is wired to a fan watchdog hardware circuit (involving a latch
>>>> and a standalone fan control chip), which the watchdog app is pinging.
>>>> So if the watchdog app dies the fans get set to full speed, however on
>>>> the next main power cycle of the server (the BMC is on a different
>>>> power
>>>> domain) we want things back to normal, so the watchdog has to work
>>>> again
>>>> when it is restarted, which it wasn't doing without my change.
>> Thanks for the explanation. Can you point me at the watchdog driver code
>> which does this "get things back to normal" on a (graceful)
>> shutdown/stop?
> 
> Our hardware is an ASPEED BMC chip, and here is the driver:
> https://github.com/torvalds/linux/blob/master/drivers/watchdog/aspeed_wdt.c

[That reminds me, I reported a bug in that driver long ago:

https://lore.kernel.org/lkml/20190812131356.23039-1-li...@rasmusvillemoes.dk/

Probably not related, but given that you have the hardware, you might be
interested in getting it fixed.]

But I don't understand how the userspace work-around can have any effect
(note: I'm not doubting you, this is all just curiosity on my part). The
only thing that's done in _stop() is to clear the WDT_CTRL_ENABLE bit.

But the first time you open the device (or possibly during _probe() if
the kernel finds the WDT_CTRL_ENABLE bit set), aspeed_wdt_enable() is
called, which as the very first thing does writel(0, wdt->base +
WDT_CTRL);, which certainly clears the WDT_CTRL_ENABLE bit. The driver
also has a comment

/*
 * The watchdog is running, but invoke aspeed_wdt_start() to
 * write wdt->ctrl to WDT_CTRL to ensure the watchdog's
 * configuration conforms to the driver's expectations.
 * Primarily, ensure we're using the 1MHz clock source.
 */

but if that doesn't work as intended, clearly it's the driver that needs
patching.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v2] watchdog: make open-write-close-open functionality a config knob

[Rasmus Villemoes]

The behaviour introduced by commit 31c765081dc4 ("watchdog: stop
watchdog first on startup") causes warnings in the kernel log when the
nowayout feature is enabled:

[   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
[   16.212196] watchdog: watchdog0: watchdog did not stop!

The latter may also appear by itself in case the watchdog is of the
type that cannot be stopped once started (e.g. the common
always-running gpio_wdt kind).

These warnings can be somewhat ominous and distracting, so allow
configuring whether to use this open-write-close-open sequence rather
than just open. Also saves a bit of .text when disabled:

function old new   delta
shutdown_on_signal31  58 +27
watchdog_main339 306 -33
shutdown_watchdog 34   - -34
--
(add/remove: 0/1 grow/shrink: 1/1 up/down: 27/-67)Total: -40 bytes

Make it default n:

- It's a workaround for one specific type of watchdog (and
  that seems to be a defect in the kernel driver)
- Even when not enabled in busybox config, it can easily be
  implemented outside busybox
- Code size
- Commit 31c765081dc4 should be considered a regression for all the
  boards that now end up with KERN_CRIT warnings in dmesg.
- The author of that commit said "This use case is evidently rare, so
  if it is indeed causing problems for other people I'd OK then I
  understand whatever needs to be done." in the v1 thread.

Cc: Matt Spinler 
Cc: dewelo...@wp.pl
Signed-off-by: Rasmus Villemoes 
---
 miscutils/watchdog.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/miscutils/watchdog.c b/miscutils/watchdog.c
index 0ed10bcf1..c2ee06dcf 100644
--- a/miscutils/watchdog.c
+++ b/miscutils/watchdog.c
@@ -18,6 +18,21 @@
 //config:  watchdog applet ever fails to write the magic character within a
 //config:  certain amount of time, the watchdog device assumes the system 
has
 //config:  hung, and will cause the hardware to reboot.
+//config:
+//config:config FEATURE_WATCHDOG_OPEN_TWICE
+//config:  bool "Open watchdog device twice, closing it gracefully in 
between"
+//config:  depends on WATCHDOG
+//config:  default n
+//config:  help
+//config:  When enabled, the watchdog device is opened and then immediately
+//config:  magic-closed, before being opened a second time. This may be 
necessary
+//config:  for some watchdog devices, but can cause spurious warnings in 
the
+//config:  kernel log if the nowayout feature is enabled. Also, if this 
workaround
+//config:  is really needed for you machine to work properly, consider 
whether
+//config:  it should be fixed in the kernel driver instead. Even when 
disabled,
+//config:  the behaviour is easily emulated with a "printf 'V' > 
/dev/watchdog"
+//config:  immediately before starting the busybox watchdog daemnn. Say n 
unless
+//config:  you know that you absolutely need this.
 
 //applet:IF_WATCHDOG(APPLET(watchdog, BB_DIR_SBIN, BB_SUID_DROP))
 
@@ -73,6 +88,7 @@ static void watchdog_open(const char* device)
/* Use known fd # - avoid needing global 'int fd' */
xmove_fd(xopen(device, O_WRONLY), 3);
 
+#if ENABLE_FEATURE_WATCHDOG_OPEN_TWICE
/* If the watchdog driver can do something other than cause a reboot
 * on a timeout, then it's possible this program may be starting from
 * a state when the watchdog hadn't been previously stopped with
@@ -82,6 +98,7 @@ static void watchdog_open(const char* device)
shutdown_watchdog();
 
xmove_fd(xopen(device, O_WRONLY), 3);
+#endif
 }
 
 int watchdog_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
-- 
2.29.2

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3] watchdog: make open-write-close-open functionality a config knob

[Rasmus Villemoes]

The behaviour introduced by commit 31c765081dc4 ("watchdog: stop
watchdog first on startup") causes warnings in the kernel log when the
nowayout feature is enabled:

[   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
[   16.212196] watchdog: watchdog0: watchdog did not stop!

The latter may also appear by itself in case the watchdog is of the
type that cannot be stopped once started (e.g. the common
always-running gpio_wdt kind).

These warnings can be somewhat ominous and distracting, so allow
configuring whether to use this open-write-close-open sequence rather
than just open. Also saves a bit of .text when disabled:

function old new   delta
shutdown_on_signal31  58 +27
watchdog_main339 306 -33
shutdown_watchdog 34   - -34
--
(add/remove: 0/1 grow/shrink: 1/1 up/down: 27/-67)Total: -40 bytes

Make it default n:

- It's a workaround for one specific type of watchdog (and
  that seems to be a defect in the kernel driver)
- Even when not enabled in busybox config, it can easily be
  implemented outside busybox
- Code size
- Commit 31c765081dc4 should be considered a regression for all the
  boards that now end up with KERN_CRIT warnings in dmesg.
- The author of that commit said "This use case is evidently rare, so
  if it is indeed causing problems for other people I'd OK then I
  understand whatever needs to be done." in the v1 thread.

Cc: Matt Spinler 
Cc: dewelo...@wp.pl
Cc: tito 
Signed-off-by: Rasmus Villemoes 
---
v3: typo fix
v2: change default to n, reword help text and commit log accordingly.

 miscutils/watchdog.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/miscutils/watchdog.c b/miscutils/watchdog.c
index 0ed10bcf1..959e4995d 100644
--- a/miscutils/watchdog.c
+++ b/miscutils/watchdog.c
@@ -18,6 +18,21 @@
 //config:  watchdog applet ever fails to write the magic character within a
 //config:  certain amount of time, the watchdog device assumes the system 
has
 //config:  hung, and will cause the hardware to reboot.
+//config:
+//config:config FEATURE_WATCHDOG_OPEN_TWICE
+//config:  bool "Open watchdog device twice, closing it gracefully in 
between"
+//config:  depends on WATCHDOG
+//config:  default n
+//config:  help
+//config:  When enabled, the watchdog device is opened and then immediately
+//config:  magic-closed, before being opened a second time. This may be 
necessary
+//config:  for some watchdog devices, but can cause spurious warnings in 
the
+//config:  kernel log if the nowayout feature is enabled. Also, if this 
workaround
+//config:  is really needed for you machine to work properly, consider 
whether
+//config:  it should be fixed in the kernel driver instead. Even when 
disabled,
+//config:  the behaviour is easily emulated with a "printf 'V' > 
/dev/watchdog"
+//config:  immediately before starting the busybox watchdog daemon. Say n 
unless
+//config:  you know that you absolutely need this.
 
 //applet:IF_WATCHDOG(APPLET(watchdog, BB_DIR_SBIN, BB_SUID_DROP))
 
@@ -73,6 +88,7 @@ static void watchdog_open(const char* device)
/* Use known fd # - avoid needing global 'int fd' */
xmove_fd(xopen(device, O_WRONLY), 3);
 
+#if ENABLE_FEATURE_WATCHDOG_OPEN_TWICE
/* If the watchdog driver can do something other than cause a reboot
 * on a timeout, then it's possible this program may be starting from
 * a state when the watchdog hadn't been previously stopped with
@@ -82,6 +98,7 @@ static void watchdog_open(const char* device)
shutdown_watchdog();
 
xmove_fd(xopen(device, O_WRONLY), 3);
+#endif
 }
 
 int watchdog_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
-- 
2.29.2

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] touch: add config option to set microsecond timestamps

[Rasmus Villemoes]

On 08/04/2021 06.07, Peter D wrote:
> This patch adds a config option which makes touch set timestamps to the
> current microsecond, instead of the current second.

Why not go all the way and start using utimensat() and do nanoseconds?
That's the resolution presented in struct stat, and what most file
systems store - without that, it's not possible to accurately replicate
one file's timestamps on another. Also, POSIX nowadays specifies touch
in terms of utimensat()
.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH v2] touch: switch to using utimensat() and futimens()

[Rasmus Villemoes]

On 11/04/2021 07.41, urmum-69 wrote:
> This patch changes the functions used to update timestamps in touch.
> 
> Before, utimes() and lutimes() were used, which had certain
> disadvantages.
> They are unable to handle nanosecond timestamps, and implementations of
> certain features like -a and -m require running stat() in a loop.
> 
> Almost all implementations of utimes() and lutimes() are wrappers for
> utimensat(), this is the case for glibc, ulibc and musl libc.
> -
> - result = (ENABLE_FEATURE_TOUCH_NODEREF && (opts & OPT_h) ? 
> lutimes : utimes)(*argv, newtime);
> -
> + int result =
> +#if ENABLE_FEATURE_TOUCH_NODEREF
> + (opts & OPT_h) ? utimensat(AT_FDCWD, *argv, newtime, 
> AT_SYMLINK_NOFOLLOW) :
> +#endif
> + utimensat(AT_FDCWD, *argv, newtime, 0);

OPT_h is 0 if ENABLE_FEATURE_TOUCH_NODEREF is not set. So all of this
can be simplified by just using

  (opts & OPT_h) ? AT_SYMLINK_NOFOLLOW : 0

as the flags argument. Maybe, for line length and readability, assign
that to a "const unsigned flags" local variable and pass that. But no
need to split a ternary operator across ifdeffery.

Other than that, from a quick read-through, it looks good.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH v3] watchdog: make open-write-close-open functionality a config knob

[Rasmus Villemoes]

Ping.

Denys, do you see any problem with default-disabling this, given the
rationale in the commit log and the feedback from others?

On 06/04/2021 10.14, Rasmus Villemoes wrote:
> The behaviour introduced by commit 31c765081dc4 ("watchdog: stop
> watchdog first on startup") causes warnings in the kernel log when the
> nowayout feature is enabled:
> 
> [   16.212184] watchdog: watchdog0: nowayout prevents watchdog being stopped!
> [   16.212196] watchdog: watchdog0: watchdog did not stop!
> 
> The latter may also appear by itself in case the watchdog is of the
> type that cannot be stopped once started (e.g. the common
> always-running gpio_wdt kind).
> 
> These warnings can be somewhat ominous and distracting, so allow
> configuring whether to use this open-write-close-open sequence rather
> than just open. Also saves a bit of .text when disabled:
> 
> function old new   delta
> shutdown_on_signal31  58 +27
> watchdog_main339 306 -33
> shutdown_watchdog 34   - -34
> --
> (add/remove: 0/1 grow/shrink: 1/1 up/down: 27/-67)Total: -40 bytes
> 
> Make it default n:
> 
> - It's a workaround for one specific type of watchdog (and
>   that seems to be a defect in the kernel driver)
> - Even when not enabled in busybox config, it can easily be
>   implemented outside busybox
> - Code size
> - Commit 31c765081dc4 should be considered a regression for all the
>   boards that now end up with KERN_CRIT warnings in dmesg.
> - The author of that commit said "This use case is evidently rare, so
>   if it is indeed causing problems for other people I'd OK then I
>   understand whatever needs to be done." in the v1 thread.
> 
> Cc: Matt Spinler 
> Cc: dewelo...@wp.pl
> Cc: tito 
> Signed-off-by: Rasmus Villemoes 
> ---
> v3: typo fix
> v2: change default to n, reword help text and commit log accordingly.
> 
>  miscutils/watchdog.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/miscutils/watchdog.c b/miscutils/watchdog.c
> index 0ed10bcf1..959e4995d 100644
> --- a/miscutils/watchdog.c
> +++ b/miscutils/watchdog.c
> @@ -18,6 +18,21 @@
>  //config:watchdog applet ever fails to write the magic character within a
>  //config:certain amount of time, the watchdog device assumes the system 
> has
>  //config:hung, and will cause the hardware to reboot.
> +//config:
> +//config:config FEATURE_WATCHDOG_OPEN_TWICE
> +//config:bool "Open watchdog device twice, closing it gracefully in 
> between"
> +//config:depends on WATCHDOG
> +//config:default n
> +//config:help
> +//config:When enabled, the watchdog device is opened and then immediately
> +//config:magic-closed, before being opened a second time. This may be 
> necessary
> +//config:for some watchdog devices, but can cause spurious warnings in 
> the
> +//config:kernel log if the nowayout feature is enabled. Also, if this 
> workaround
> +//config:is really needed for you machine to work properly, consider 
> whether
> +//config:it should be fixed in the kernel driver instead. Even when 
> disabled,
> +//config:the behaviour is easily emulated with a "printf 'V' > 
> /dev/watchdog"
> +//config:immediately before starting the busybox watchdog daemon. Say n 
> unless
> +//config:you know that you absolutely need this.
>  
>  //applet:IF_WATCHDOG(APPLET(watchdog, BB_DIR_SBIN, BB_SUID_DROP))
>  
> @@ -73,6 +88,7 @@ static void watchdog_open(const char* device)
>   /* Use known fd # - avoid needing global 'int fd' */
>   xmove_fd(xopen(device, O_WRONLY), 3);
>  
> +#if ENABLE_FEATURE_WATCHDOG_OPEN_TWICE
>   /* If the watchdog driver can do something other than cause a reboot
>* on a timeout, then it's possible this program may be starting from
>* a state when the watchdog hadn't been previously stopped with
> @@ -82,6 +98,7 @@ static void watchdog_open(const char* device)
>   shutdown_watchdog();
>  
>   xmove_fd(xopen(device, O_WRONLY), 3);
> +#endif
>  }
>  
>  int watchdog_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
> 


-- 
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villem...@prevas.dk
www.prevas.dk
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH v3] watchdog: make open-write-close-open functionality a config knob

[Rasmus Villemoes]

On 13/04/2021 16.05, Denys Vlasenko wrote:
> Applied, thank you

Thanks, but why does the result 50a37459 have completely unrelated hunks
like

-#define OPT_FOREGROUND  (1 << 0)
-#define OPT_STIMER  (1 << 1)
-#define OPT_HTIMER  (1 << 2)
-

+#define OPT_FOREGROUND  (1 << 0)
+#define OPT_STIMER  (1 << 1)
+#define OPT_HTIMER  (1 << 2)

-   stimer_duration, htimer_duration * 1000);
+   stimer_duration, htimer_duration);

?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 1/3] touch: implement nanosecond precision times

[Rasmus Villemoes]

On 14/04/2021 00.33, Denys Vlasenko wrote:
> On Tue, Apr 13, 2021 at 11:03 PM Xabier Oneca -- xOneca
>  wrote:
>> Hi Peter,
>>
 Sorry if I trampled down your patch, but I had this patch prepared a
 while ago and didn't feel like rebasing my other patches. Indeed,
 yours gave me +10 bytes, so I decided to send my version too. They are
 very similar, tho.
>>>
>>> This is no problem - seeing as your patch is smaller and more well
>>> documented, I don't feel trampled on at all.
>>> That was my first time submitting a patch anywhere - I had fun writing
>>> it anyway.
>>
>> Well... Your patch was very nice, and I first doubted if I should send
>> my version.
>>
>> In the end Denys accepted your version... sort of :)
>>
>> He over-shrunk the code, and now 'touch -am file' does not work. :S
> 
> Looks like a kernel-side problem to me: with [OMIT, OMIT],
> utimensat does not even check existence of the file!

First, "touch -am" is supposed to be equivalent to not passing either
flag, i.e. update both. (-a means "update atime", not "don't update
mtime", after all). So busybox touch should never get to pass OMIT,OMIT
in the first place. Second, that behaviour is documented in the man page

   If both tv_nsec fields are specified as UTIME_OMIT, then the
Linux implementation of utimensat() succeeds even  if  the
   file referred to by dirfd and pathname does not exist.

But that's irrelevant, it's simply the current touch.c code that is buggy.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

busybox modprobe v modules.builtin

[Rasmus Villemoes]

When I do

  busybox modprobe squashfs

with squashfs builtin to the kernel, I get

  modprobe: module squashfs not found in modules.dep

and an exit value of 1. But

commit 803c85a20710b8d9026775f5668237fff496dc1e
Author: Ben Hutchings 
Date:   Thu Apr 6 11:54:04 2017 +0200

modprobe: read modules.builtin

suggests that this should work as expected (silent success), as for kmod
modprobe. I'm pretty sure the problem is the (older!)

commit 78854520ebecfd24d5c80a266d6779bd1e069016
Author: Denys Vlasenko 
Date:   Thu Jan 1 19:02:40 2015 +0100

modprobe: revert checking for /, stop doing basename() on modprobe args

because when reading in modules.builtin, we very much need to do that
basenamization. And a very quick test switching back to
bb_get_last_path_component_nostrip() in filename2modname() makes
modprobe work as expected for the above case.

I see that there are several commits already dealing with 788545, e.g.

commit a88db5c1a99ebc0ae23b5d108113d9b8af7afc3c
Author: Denys Vlasenko 
Date:   Sat Feb 21 17:08:35 2015 +0100

modinfo: fix fallout from basename removal

commit 9de69c024c7c47f3f8733dbc7c9522966fcd73a9
Author: Natanael Copa 
Date:   Fri Jan 16 13:53:05 2015 +0100

modprobe: fix modprobe -r and parsing of /etc/modprobe.d

commit e998b08f118a0e485fffaa513bac133df2e3843b
Author: Denys Vlasenko 
Date:   Thu Jan 15 02:48:36 2015 +0100

modprobe: fix recent breakage: modules.dep reading code needs to
strip dirname

I have no idea what the right fix is. There seems to be a big confusion
about when a string is a pathname, filename, module name or module alias.

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 0/5] simplify and improve crypt_make_salt()

[Rasmus Villemoes]

Currently, crypt_make_salt() can only return 2^28 different salt
strings. There are some low-hanging fruits allowing us to reduce the
code size, and even with patch 5 which obviously by itself increases
the footprint, the combined result of these patches is

function old new   delta
crypt_make_salt  104  95  -9
i64c  41  24 -17
--
(add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-26) Total: -26 bytes

while making sure (in the normal case where /dev/urandom is available)
that the salt string is completely random, and that we are not in any
case doing worse than what the current code does.


Rasmus Villemoes (5):
  pw_encrypt.c: make i64c() slightly smaller
  crypt_make_salt: cleanup leftover comments
  crypt_make_salt(): simplify and improve
  crypt_make_salt(): fixup odd calling convention
  crypt_make_salt(): try to use actual random bytes for salt generation

 include/libbb.h| 11 ++-
 libbb/pw_encrypt.c | 27 +++
 networking/httpd.c |  2 +-
 3 files changed, 14 insertions(+), 26 deletions(-)

-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 1/5] pw_encrypt.c: make i64c() slightly smaller

[Rasmus Villemoes]

In ASCII at least, '.' is 0x2e, '/' is 0x2f and '0' is 0x30. So the
existing "if (i < 12)" case does the right thing also for i <= 1.

I don't know if busybox supports anything but ASCII environments, but
since we can do it build-time with preprocessor conditionals, we might
as well leave the two cases in the code, which then (along with the
terse comment) also serves as a bit of explanation for what the i<12
case then ends up producing.

function old new   delta
i64c  41  24 -17
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-17) Total: -17 bytes

Signed-off-by: Rasmus Villemoes 
---
 libbb/pw_encrypt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c
index 3463fd95b..adbdc1d1e 100644
--- a/libbb/pw_encrypt.c
+++ b/libbb/pw_encrypt.c
@@ -20,10 +20,13 @@
 static int i64c(int i)
 {
i &= 0x3f;
+   /* In ascii, '.', '/' and '0' are consecutive. */
+#if '0' - 2 != '.' || '0' - 1 != '/'
if (i == 0)
return '.';
if (i == 1)
return '/';
+#endif
if (i < 12)
return ('0' - 2 + i);
if (i < 38)
-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 2/5] crypt_make_salt: cleanup leftover comments

[Rasmus Villemoes]

Way back in commit 12a432715f06, crypt_make_salt() was changed to use
monotonic_us() instead of time(NULL) as a poor man's random source,
and, since that then at least made it much less likely that
consecutive calls would return the same result, at the same time lost
the "rnd" parameter. Remnants of that were left in comments, but 13
years later that really serves no purpose.

Signed-off-by: Rasmus Villemoes 
---
 include/libbb.h| 11 ++-
 libbb/pw_encrypt.c |  3 +--
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/include/libbb.h b/include/libbb.h
index ef5d04713..db61e62ab 100644
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -1783,15 +1783,8 @@ int ask_and_check_password(const struct passwd *pw) 
FAST_FUNC;
 #endif
 extern char *pw_encrypt(const char *clear, const char *salt, int cleanup) 
FAST_FUNC;
 extern int obscure(const char *old, const char *newval, const struct passwd 
*pwdp) FAST_FUNC;
-/*
- * rnd is additional random input. New one is returned.
- * Useful if you call crypt_make_salt many times in a row:
- * rnd = crypt_make_salt(buf1, 4, 0);
- * rnd = crypt_make_salt(buf2, 4, rnd);
- * rnd = crypt_make_salt(buf3, 4, rnd);
- * (otherwise we risk having same salt generated)
- */
-extern int crypt_make_salt(char *p, int cnt /*, int rnd*/) FAST_FUNC;
+
+extern int crypt_make_salt(char *p, int cnt) FAST_FUNC;
 /* "$N$" + sha_salt_16_bytes + NUL */
 #define MAX_PW_SALT_LEN (3 + 16 + 1)
 extern char* crypt_make_pw_salt(char p[MAX_PW_SALT_LEN], const char *algo) 
FAST_FUNC;
diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c
index adbdc1d1e..458792faa 100644
--- a/libbb/pw_encrypt.c
+++ b/libbb/pw_encrypt.c
@@ -34,9 +34,8 @@ static int i64c(int i)
return ('a' - 38 + i);
 }
 
-int FAST_FUNC crypt_make_salt(char *p, int cnt /*, int x */)
+int FAST_FUNC crypt_make_salt(char *p, int cnt)
 {
-   /* was: x += ... */
unsigned x = getpid() + monotonic_us();
do {
/* x = (x*1664525 + 1013904223) % 2^32 generator is lame
-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 3/5] crypt_make_salt(): simplify and improve

[Rasmus Villemoes]

The use of a PRNG here is useless. No matter what, the generated salt
string is completely determined by the initial value of x.

In fact, since we only look at bits 16-27 and it's a LCG with a
power-of-2 modulus (so the high bits never affect lower bits), only
2^28 different salts can be produced.

So instead of pretending to generate a random string, just
base64-encode the bits we actually have, and make sure all of them are
used. That way we can produce 2^32 instead of 2^28 different salts,
and it also becomes more transparent (due to the trailing '.'
characters from when x hits 0) that the salt generation is not
particularly thorough.

Signed-off-by: Rasmus Villemoes 
---
 libbb/pw_encrypt.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c
index 458792faa..9ac75b281 100644
--- a/libbb/pw_encrypt.c
+++ b/libbb/pw_encrypt.c
@@ -37,18 +37,11 @@ static int i64c(int i)
 int FAST_FUNC crypt_make_salt(char *p, int cnt)
 {
unsigned x = getpid() + monotonic_us();
+   /* Odd calling convention... */
+   cnt *= 2;
do {
-   /* x = (x*1664525 + 1013904223) % 2^32 generator is lame
-* (low-order bit is not "random", etc...),
-* but for our purposes it is good enough */
-   x = x*1664525 + 1013904223;
-   /* BTW, Park and Miller's "minimal standard generator" is
-* x = x*16807 % ((2^31)-1)
-* It has no problem with visibly alternating lowest bit
-* but is also weak in cryptographic sense + needs div,
-* which needs more code (and slower) on many CPUs */
-   *p++ = i64c(x >> 16);
-   *p++ = i64c(x >> 22);
+   *p++ = i64c(x);
+   x >>= 6;
} while (--cnt);
*p = '\0';
return x;
-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 4/5] crypt_make_salt(): fixup odd calling convention

[Rasmus Villemoes]

Due to the implementation of crypt_make_salt(), it used to make sense
that the length parameter was only half of the expected length of the
salt, but that's not so any more, so clean it up and make the callers
pass the desired length directly.

Signed-off-by: Rasmus Villemoes 
---
 libbb/pw_encrypt.c | 8 +++-
 networking/httpd.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c
index 9ac75b281..3de31f711 100644
--- a/libbb/pw_encrypt.c
+++ b/libbb/pw_encrypt.c
@@ -37,8 +37,6 @@ static int i64c(int i)
 int FAST_FUNC crypt_make_salt(char *p, int cnt)
 {
unsigned x = getpid() + monotonic_us();
-   /* Odd calling convention... */
-   cnt *= 2;
do {
*p++ = i64c(x);
x >>= 6;
@@ -49,21 +47,21 @@ int FAST_FUNC crypt_make_salt(char *p, int cnt)
 
 char* FAST_FUNC crypt_make_pw_salt(char salt[MAX_PW_SALT_LEN], const char 
*algo)
 {
-   int len = 2/2;
+   int len = 2;
char *salt_ptr = salt;
 
/* Standard chpasswd uses uppercase algos ("MD5", not "md5").
 * Need to be case-insensitive in the code below.
 */
if ((algo[0]|0x20) != 'd') { /* not des */
-   len = 8/2; /* so far assuming md5 */
+   len = 8; /* so far assuming md5 */
*salt_ptr++ = '$';
*salt_ptr++ = '1';
*salt_ptr++ = '$';
 #if !ENABLE_USE_BB_CRYPT || ENABLE_USE_BB_CRYPT_SHA
if ((algo[0]|0x20) == 's') { /* sha */
salt[1] = '5' + (strcasecmp(algo, "sha512") == 0);
-   len = 16/2;
+   len = 16;
}
 #endif
}
diff --git a/networking/httpd.c b/networking/httpd.c
index ddcb03bca..825a87d81 100644
--- a/networking/httpd.c
+++ b/networking/httpd.c
@@ -2826,7 +2826,7 @@ int httpd_main(int argc UNUSED_PARAM, char **argv)
salt[0] = '$';
salt[1] = '1';
salt[2] = '$';
-   crypt_make_salt(salt + 3, 4);
+   crypt_make_salt(salt + 3, 8);
puts(pw_encrypt(pass, salt, /*cleanup:*/ 0));
return 0;
}
-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 5/5] crypt_make_salt(): try to use actual random bytes for salt generation

[Rasmus Villemoes]

Instead of limiting the possible generated salts to 2^32 different
ones, try to get some actual random bits and mix those in. Keep the
old pseudo-random generation in place, so that even if
open_read_close() fails or only returns a partial result, we're not
doing any worse than previously.

In fact, even with /dev/urandom being unavailable, this might still
improve things a bit since whatever sort-of random content might be in
the p buffer on entry is then mixed in.

Signed-off-by: Rasmus Villemoes 
---
 libbb/pw_encrypt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c
index 3de31f711..cbb1f36f0 100644
--- a/libbb/pw_encrypt.c
+++ b/libbb/pw_encrypt.c
@@ -37,8 +37,10 @@ static int i64c(int i)
 int FAST_FUNC crypt_make_salt(char *p, int cnt)
 {
unsigned x = getpid() + monotonic_us();
+   open_read_close("/dev/urandom", p, cnt);
do {
-   *p++ = i64c(x);
+   *p = i64c(x + *p);
+   p++;
x >>= 6;
} while (--cnt);
*p = '\0';
-- 
2.40.1.1.g1c60b9335d

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 5/5] crypt_make_salt(): try to use actual random bytes for salt generation

[Rasmus Villemoes]

On 15/04/2024 21.05, Steffen Nurpmeso wrote:
> Rasmus Villemoes wrote in
>  <20240415125628.780178-6-rasmus.villem...@prevas.dk>:
>  |Instead of limiting the possible generated salts to 2^32 different
>  |ones, try to get some actual random bits and mix those in. Keep the
>  |old pseudo-random generation in place, so that even if
>  |open_read_close() fails or only returns a partial result, we're not
>  |doing any worse than previously.
>  |
>  |In fact, even with /dev/urandom being unavailable, this might still
>  |improve things a bit since whatever sort-of random content might be in
>  |the p buffer on entry is then mixed in.
> 
> By that time i thought (completely unrelated with your work) that
> the code from miscutils/seedrng.c which has a more broad view of
> where to get random data from should possibly be generalized.
> There are more than just one match for /dev/urandom, and they all
> want some random bits.

Yeah, I know, I did go grepping for 'random' to see if bb already had
some "gimme some random bytes" helper, and I've also sort-of followed
the seedrng saga. But since there was no such helper, and just
best-effort here is good enough for a significant improvement in the
normal case, I just went with the oneliner open_read_close(), which is
also used in generate_uuid() without checking the return value. If some
common infrastructure for randomness should materialize, this can
trivially be switched over. But I really didn't want to tie this series
to creation of and bikeshedding over such infrastructure.

Rasmus

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 0/5] simplify and improve crypt_make_salt()

[Rasmus Villemoes]

On 15/04/2024 14.56, Rasmus Villemoes wrote:
> Currently, crypt_make_salt() can only return 2^28 different salt
> strings. There are some low-hanging fruits allowing us to reduce the
> code size, and even with patch 5 which obviously by itself increases
> the footprint, the combined result of these patches is
> 
> function old new   delta
> crypt_make_salt  104  95  -9
> i64c  41  24 -17
> --
> (add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-26) Total: -26 bytes
> 
> while making sure (in the normal case where /dev/urandom is available)
> that the salt string is completely random, and that we are not in any
> case doing worse than what the current code does.

If there are no other comments, could these be considered for applying?

Thanks,
Rasmus


> 
> Rasmus Villemoes (5):
>   pw_encrypt.c: make i64c() slightly smaller
>   crypt_make_salt: cleanup leftover comments
>   crypt_make_salt(): simplify and improve
>   crypt_make_salt(): fixup odd calling convention
>   crypt_make_salt(): try to use actual random bytes for salt generation
> 
>  include/libbb.h| 11 ++-
>  libbb/pw_encrypt.c | 27 +++
>  networking/httpd.c |  2 +-
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

test -w on r/o filesystems

[Rasmus Villemoes]

It seems that busybox' "test" returns the wrong result when applied to a
R/O filesystem.

### Permissions "allow" write for root
# ls -ld /
drwxr-xr-x 15 root root 221 Apr  5  2011 /
### But the rootfs is (and can only be) mounted ro
# mount | grep ' / '
/dev/mmcblk0p5 on / type squashfs (ro,noatime,errors=continue)
### With coreutils test
# test -w / && echo Writable || echo Not writable
Not writable
### With busybox test
# busybox test -w / && echo Writable || echo Not writable
Writable

Rasmus
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 0/5] simplify and improve crypt_make_salt()

[Rasmus Villemoes]

On 15/04/2024 14.56, Rasmus Villemoes wrote:
> Currently, crypt_make_salt() can only return 2^28 different salt
> strings. There are some low-hanging fruits allowing us to reduce the
> code size, and even with patch 5 which obviously by itself increases
> the footprint, the combined result of these patches is
> 
> function old new   delta
> crypt_make_salt  104  95  -9
> i64c  41  24 -17
> --
> (add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-26) Total: -26 bytes
> 
> while making sure (in the normal case where /dev/urandom is available)
> that the salt string is completely random, and that we are not in any
> case doing worse than what the current code does.
> 

Ping.

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 0/5] simplify and improve crypt_make_salt()

[Rasmus Villemoes]

On 15/04/2024 14.56, Rasmus Villemoes wrote:
> Currently, crypt_make_salt() can only return 2^28 different salt
> strings. There are some low-hanging fruits allowing us to reduce the
> code size, and even with patch 5 which obviously by itself increases
> the footprint, the combined result of these patches is
> 
> function old new   delta
> crypt_make_salt  104  95  -9
> i64c  41  24 -17
> --
> (add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-26) Total: -26 bytes
> 
> while making sure (in the normal case where /dev/urandom is available)
> that the salt string is completely random, and that we are not in any
> case doing worse than what the current code does.

ping ping

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH] switch_root: remove /init check

[Rasmus Villemoes]

On 31/05/2024 00.08, Linus Heckemann wrote:

> I was having a look at this again, and I'm not sure reimplementing the
> kernel command line parsing logic[1][2] is really desirable. 

FWIW, I'm with Linus. It's also odd for busybox to have a check that the
util-linux version of this doesn't.

For the original patch:

Acked-by: Rasmus Villemoes 

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 0/5] simplify and improve crypt_make_salt()

[Rasmus Villemoes]

On 15/04/2024 14.56, Rasmus Villemoes wrote:
> Currently, crypt_make_salt() can only return 2^28 different salt
> strings. There are some low-hanging fruits allowing us to reduce the
> code size, and even with patch 5 which obviously by itself increases
> the footprint, the combined result of these patches is
> 
> function old new   delta
> crypt_make_salt  104  95  -9
> i64c  41  24 -17
> --
> (add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-26) Total: -26 bytes
> 
> while making sure (in the normal case where /dev/urandom is available)
> that the salt string is completely random, and that we are not in any
> case doing worse than what the current code does.

ping^2
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox