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

On Thu, 7 Nov 2024 08:01:11 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Wed, Nov 06, 2024 at 08:12:23PM +0100, Stefano
Brivio wrote:
  On Wed,  6 Nov 2024 17:54:17 +1100
 David Gibson <david(a)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.   
 
 Unlike some of the other things fixed recently, this one is not
 related to compile time versus runtime checks.  This one is simply
 broken compiling against an older kernel, regardless of the runtime
 version.  Without this patch, close_open_files() directly uses
 CLOSE_RANGE_UNSHARE unprotected by an #ifdef. 
Ah, right, it's a different case, indeed.

...
 
 
   * 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.   
 
 Uh... what's the connection to running as root? 
That you can't run pasta or passt altogether, and if you need some
features they provide, not covered by libslirp, you might as well need
to switch to run things as root.

...
 
  I don't
think that introducing a dependency on a >= 5.9 kernel is a
 good idea.   
 
 We already have a dependency on compiling against a >= 5.9 kernel, see
 above. 
Yes, but that would be a trivial fix.

...
 
  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.   
 
 We could do that.  We'd need to consider EINVAL as well as ENOSYS, for
 the case that the syscall exists, but the CLOSE_RANGE_UNSHARE flag
 isn't recognized. 
Right... I think we should do that.

-- 
Stefano