Re: [PATCH 4/8] linux_dep: Fix CLOSE_RANGE_UNSHARE availability handling

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  6 Nov 2024 17:54:17 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> If CLOSE_RANGE_UNSHARE isn't defined, we define a fallback version of
> close_range() which is a (successful) no-op.  This is broken in several
> ways:
>  * It doesn't actually fix compile if using old kernel headers, because
>    the caller of close_range() still directly uses CLOSE_RANGE_UNSHARE
>    unprotected by ifdefs

For users using distribution packages, that is, pretty much everybody
not developing passt, this is generally not a concern, because build
environments typically ship kernel headers matching the C library and
the distribution version at hand.

>  * Even if it did fix the compile, it means inconsistent behaviour between
>    a compile time failure to find the value (we silently don't close files)
>    and a runtime failure (we die with an error from close_range())

Given that this is mostly relevant for stuff built against musl (and
running on a musl-based distribution), that's not really a problem in
practice. See 6e9ecf57410b ("util: Provide own version of close_range(),
and no-op fallback").

But sure, I'm fine with these changes in general, as they're strictly
speaking more correct than the current behaviour, minus the next point.

>  * Silently not closing the files we intend to close for security reasons
>    is probably not a good idea in any case

It's arguably even worse to force users to run containers or guests as
root. That's the reason for the no-op implementation.

I don't think that introducing a dependency on a >= 5.9 kernel is a
good idea.

If this bothers you or cppcheck, I'd rather call close_range()
(possibly the weakly aliased implementation), and log a warning on
ENOSYS instead of failing.

> As bonus this fixes a cppcheck error I see with some different options I'm
> looking to apply in future.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  linux_dep.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/linux_dep.h b/linux_dep.h
> index 3a41e42..240f50a 100644
> --- a/linux_dep.h
> +++ b/linux_dep.h
> @@ -127,22 +127,18 @@ struct tcp_info_linux {
>  
>  #include <linux/close_range.h>
>  
> -#ifdef CLOSE_RANGE_UNSHARE	/* Linux kernel >= 5.9 */
>  /* glibc < 2.34 and musl as of 1.2.5 need these */
>  #ifndef SYS_close_range
>  #define SYS_close_range		436
>  #endif
> +#ifndef CLOSE_RANGE_UNSHARE	/* Linux kernel < 5.9 */
> +#define CLOSE_RANGE_UNSHARE	(1U << 1)
> +#endif
> +
>  __attribute__ ((weak))
>  /* cppcheck-suppress funcArgNamesDifferent */
>  int close_range(unsigned int first, unsigned int last, int flags) {
>  	return syscall(SYS_close_range, first, last, flags);
>  }
> -#else
> -/* No reasonable fallback option */
> -/* cppcheck-suppress funcArgNamesDifferent */
> -int close_range(unsigned int first, unsigned int last, int flags) {
> -	return 0;
> -}
> -#endif
>  
>  #endif /* LINUX_DEP_H */

-- 
Stefano