Re: [PATCH v2] isolation: Add --chroot-fallback option

[Mateusz Andrzejewski <mandrzejewski06@gmail.com>]

Hi Stefano

Thanks for the detailed review.

On 7/4/2026 5:27 PM, Stefano Brivio wrote:
 > Mateusz, thanks for following up.
 >
 > I see a few very minor issues, reported inline below, that I can fix up
 > on merge if you agree (but you can re-post, as you wish).
 >
 > On Thu,  2 Jul 2026 09:13:31 +0200
 > Mateusz Andrzejewski <mandrzejewski06@gmail.com> wrote:
 >
 >> For integrations, which use rootfs on tmpfs or initramfs, it is not
 >> allowed to use pivot_root(). It results with invalid argument (EINVAL)
 >> error. Introduce --chroot-fallback option as a workaround with
 >> MS_MOVE + chroot().
 >>
 >> Due to weaker isolation of chroot() method (we don't unmount old root),
 >> user has tu explicitly enable fallback with the new option. First,
 >> always try to sandbox with pivot_root(). In both cases the new root is
 >> placed into an empty tmpfs.
 >>
 >> For the solution to work we keep CAP_SYS_CHROOT capability, which is
 >> dropped at the end of the isolate_prefork() function.
 >>
 >> Link: https://bugs.passt.top/show_bug.cgi?id=104
 >>
 >> Signed-off-by: Mateusz Andrzejewski 
<mateusz.andrzejewski@mikronika.com.pl>
 >> ---
 >> Changes since v1:
 >> - added a description of the new option to the man page
 >> - renamed option to --chroot-fallback
 >> - removed auxiliary variable in conf()
 >> - passed execution context to isolate_user()
 >> - fixed indentation and other coding style issues
 >> - fixed cppcheck issue with variableScope in isolate_prefork()
 >>
 >> Direct write to &c->chroot_fallback is not valid, because it's a bool
 >> variable and compilation results with an incompatible-pointer-types
 >> warning (int* expected).
 > By the way, right, my previous suggestion implied that you would also
 > turn that to an int, but I forgot to say that. On the other hand:
 >
 >> To fix this, the assignment was moved to the '32' label in the switch
 >> statement and the temporary helper variable could be removed. This
 >> follows the same pattern of other boolean options.
 > ...this is also true, and I think it's more important to keep it
 > consistent rather than saving three lines there, so let's leave it like
 > you did now.

Yeah, I was a little bit confused by that suggestion, because in the 
topic of bug 104 you commented:
"if you need to keep 'int no_pivot_root' in struct ctx for whatever 
reason, (...) it could be a bool (we started passt as C90 but it became 
C11 meanwhile)"
So I thought that every new binary variable should be a bool now.

 >>  conf.c      |  8 +++++-
 >>  isolation.c | 71 ++++++++++++++++++++++++++++++++++++++++++-----------
 >>  isolation.h |  4 +--
 >>  passt.1     |  9 +++++++
 >>  passt.h     |  2 ++
 >>  5 files changed, 77 insertions(+), 17 deletions(-)
 >>
 >> diff --git a/conf.c b/conf.c
 >> index 4755a9f..6755bf6 100644
 >> --- a/conf.c
 >> +++ b/conf.c
 >> @@ -654,6 +654,8 @@ static void usage(const char *name, FILE *f, int 
status)
 >>          "  --no-ra        Disable router advertisements\n"
 >>          "  --freebind        Bind to any address for forwarding\n"
 >>          "  --no-map-gw        Don't map gateway address to host\n"
 >> +        "  --chroot-fallback    Use chroot() if pivot_root() fails\n"
 >> +        "    can be useful for rootfs on tmpfs or initramfs\n"
 > I was suggesting that you would explain this in the documentation, but I
 > think having it in the man page is sufficient, while this usage message
 > isn't supposed to have all the details, and it's already long enough.
 >
 > So I'd rather drop the "can be useful ..." line from here. Not a strong
 > preference from my side, though (it's just one extra line after all).

Ahh, sorry, I misunderstood. You can remove "can be useful.." line if 
you want to.

 >>          "  -4, --ipv4-only    Enable IPv4 operation only\n"
 >>          "  -6, --ipv6-only    Enable IPv6 operation only\n"
 >>          "  -t, --tcp-ports SPEC    TCP port forwarding to %s\n"
 >> @@ -1233,6 +1235,7 @@ void conf(struct ctx *c, int argc, char **argv)
 >>          {"migrate-no-linger", no_argument,    NULL, 30 },
 >>          {"stats", required_argument,        NULL, 31 },
 >>          {"conf-path",    required_argument,    NULL,   'c' },
 >> +        {"chroot-fallback", no_argument,    NULL,  32 },
 >>          { 0 },
 >>      };
 >>      const char *optstring = 
"+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
 >> @@ -1467,6 +1470,9 @@ void conf(struct ctx *c, int argc, char **argv)
 >>                  die("Can't display statistics if not running in 
foreground");
 >>              c->stats = strtol(optarg, NULL, 0);
 >>              break;
 >> +        case 32:
 >> +            c->chroot_fallback = true;
 >> +            break;
 >>          case 'd':
 >>              c->debug = 1;
 >>              c->quiet = 0;
 >> @@ -1879,7 +1885,7 @@ void conf(struct ctx *c, int argc, char **argv)
 >>
 >>      conf_open_files(c);    /* Before any possible setuid() / 
setgid() */
 >>
 >> -    isolate_user(uid, gid, !netns_only, userns, c->mode);
 >> +    isolate_user(c, uid, gid, !netns_only, userns);
 >>
 >>      if (c->no_icmp)
 >>          c->no_ndp = 1;
 >> diff --git a/isolation.c b/isolation.c
 >> index 7e6225d..08e4008 100644
 >> --- a/isolation.c
 >> +++ b/isolation.c
 >> @@ -166,6 +166,31 @@ static void clamp_caps(void)
 >>          die_perror("Couldn't drop inheritable capabilities");
 >>  }
 >>
 >> +/**
 >> + * move_root() - Use chroot() instead of pivot_root() for sandboxing
 >> + *
 >> + * Return: negative error code on failure, zero on success
 >> + */
 >> +static int move_root(void)
 >> +{
 >> +    if (mount(TMPDIR, "/", "", MS_MOVE, "")) {
 >> +        err_perror("Failed to move root into empty tmpfs");
 >> +        return -errno;
 >> +    }
 >> +
 >> +    if (chroot(".")) {
 >> +        err_perror("Failed to chroot() into empty tmpfs");
 >> +        return -errno;
 >> +    }
 >> +
 >> +    if (chdir("/")) {
 >> +        err_perror("Failed to change directory into new root");
 >> +        return -errno;
 >> +    }
 >> +
 >> +    return 0;
 >> +}
 >> +
 >>  /**
 >>   * isolate_initial() - Early, mostly config independent self isolation
 >>   * @argc:    Argument count
 >> @@ -195,14 +220,18 @@ void isolate_initial(int argc, char **argv)
 >>       *  - CAP_SYS_ADMIN, so that we can setns() to the netns.
 >>       *  - Keep CAP_NET_ADMIN, so that we can configure interfaces
 >>       *
 >> +     * We have to keep CAP_SYS_CHROOT in case of --chroot-fallback 
option
 >> +     * being enabled, so we can fallback from pivot_root() to 
chroot() in
 > Very minor but... the noun is "fallback" and the verb is "fall back".
 >
 >> +     * isolate_prefork().
 >> +     *
 >>       * It's debatable whether it's useful to drop caps when we
 >>       * retain SETUID and SYS_ADMIN, but we might as well. We drop
 >>       * further capabilities in isolate_user() and
 >>       * isolate_prefork().
 >>       */
 >>      keep = BIT(CAP_NET_BIND_SERVICE) | BIT(CAP_SETUID) | 
BIT(CAP_SETGID) |
 >> -           BIT(CAP_SYS_ADMIN) | BIT(CAP_NET_ADMIN) | 
BIT(CAP_DAC_OVERRIDE);
 >> -
 >> +           BIT(CAP_SYS_ADMIN) | BIT(CAP_NET_ADMIN) | 
BIT(CAP_DAC_OVERRIDE) |
 >> +           BIT(CAP_SYS_CHROOT);
 >>      /* Since Linux 5.12, if we want to update /proc/self/uid_map to 
create
 >>       * a mapping from UID 0, which only happens with pasta spawning 
a child
 >>       * from a non-init user namespace (pasta can't run as root), we 
need to
 >> @@ -220,11 +249,11 @@ void isolate_initial(int argc, char **argv)
 >>
 >>  /**
 >>   * isolate_user() - Switch to final UID/GID and move into userns
 >> + * @c:        Execution context
 >>   * @uid:    User ID to run as (in original userns)
 >>   * @gid:    Group ID to run as (in original userns)
 >>   * @use_userns:    Whether to join or create a userns
 >>   * @userns:    userns path to enter, may be empty
 >> - * @mode:    Mode (passt or pasta)
 >>   *
 >>   * Should:
 >>   *  - set our final UID and GID
 >> @@ -232,8 +261,8 @@ void isolate_initial(int argc, char **argv)
 >>   * Mustn't:
 >>   *  - remove filesystem access (we need that for further setup)
 >>   */
 >> -void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char 
*userns,
 >> -          enum passt_modes mode)
 >> +void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool 
use_userns,
 >> +          const char *userns)
 >>  {
 >>      uint64_t ns_caps = 0;
 >>
 >> @@ -277,7 +306,14 @@ void isolate_user(uid_t uid, gid_t gid, bool 
use_userns, const char *userns,
 >>       * netns
 >>       */
 >>      ns_caps |= BIT(CAP_SYS_ADMIN);
 >> -    if (mode == MODE_PASTA) {
 >> +
 >> +    /* Only keep CAP_SYS_CHROOT for the --chroot-fallback case. 
Otherwise
 >> +     * it can be dropped
 >> +     */
 >> +    if (c->chroot_fallback)
 >> +        ns_caps |= BIT(CAP_SYS_CHROOT);
 >> +
 >> +    if (c->mode == MODE_PASTA) {
 >>          /* Keep CAP_NET_ADMIN, so we can configure the if */
 >>          ns_caps |= BIT(CAP_NET_ADMIN);
 >>          /* Keep CAP_NET_BIND_SERVICE, so we can splice
 >> @@ -331,7 +367,7 @@ int isolate_prefork(const struct ctx *c)
 >>      if (mount("", TMPDIR, "tmpfs",
 >>            MS_NODEV | MS_NOEXEC | MS_NOSUID | MS_RDONLY,
 >>            "nr_inodes=2,nr_blocks=0")) {
 >> -        err_perror("Failed to mount empty tmpfs for pivot_root()");
 >> +        err_perror("Failed to mount empty tmpfs for sandboxing");
 >>          return -errno;
 >>      }
 >>
 >> @@ -341,13 +377,20 @@ int isolate_prefork(const struct ctx *c)
 >>      }
 >>
 >>      if (syscall(SYS_pivot_root, ".", ".")) {
 >> -        err_perror("Failed to pivot_root() into empty tmpfs");
 >> -        return -errno;
 >> -    }
 >> -
 >> -    if (umount2(".", MNT_DETACH | UMOUNT_NOFOLLOW)) {
 >> -        err_perror("Failed to unmount original root filesystem");
 >> -        return -errno;
 >> +        if (c->chroot_fallback) {
 >> +            int rc;
 >> +            info("Failed to pivot_root(), fallback to chroot()...");
 >> +            if ((rc = move_root()))
 >> +                return rc;
 >> +        } else {
 >> +            err_perror("Failed to pivot_root() into empty tmpfs");
 >> +            return -errno;
 >> +        }
 >> +    } else {
 >> +        if (umount2(".", MNT_DETACH | UMOUNT_NOFOLLOW)) {
 >> +            err_perror("Failed to unmount original root filesystem");
 >> +            return -errno;
 >> +        }
 >>      }
 >>
 >>      /* Now that initialization is more-or-less complete, we can
 >> diff --git a/isolation.h b/isolation.h
 >> index 0576168..66b6968 100644
 >> --- a/isolation.h
 >> +++ b/isolation.h
 >> @@ -11,8 +11,8 @@
 >>  #include <unistd.h>
 >>
 >>  void isolate_initial(int argc, char **argv);
 >> -void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char 
*userns,
 >> -          enum passt_modes mode);
 >> +void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool 
use_userns,
 >> +          const char *userns);
 >>  int isolate_prefork(const struct ctx *c);
 >>  void isolate_postfork(const struct ctx *c);
 >>
 >> diff --git a/passt.1 b/passt.1
 >> index 908fd4a..8a33c2e 100644
 >> --- a/passt.1
 >> +++ b/passt.1
 >> @@ -393,6 +393,15 @@ as destination, to the host. Implied if there 
is no gateway on the selected
 >>  default route, or if there is no default route, for any of the 
enabled address
 >>  families.
 >>
 >> +.TP
 >> +.BR \-\-chroot-fallback
 >> +Enable a fallback to chroot() in case pivot_root() returns an 
error. Useful for
 > Maybe useful to specify: "in case pivot_root(), used to switch to an
 > empty filesystem for stricter isolation, returns an error".
 >
 > Otherwise most users will have no idea what this is about.
 >
 >> +integrations, which use rootfs on tmpfs or initramfs, where it is 
not allowed to
 > Also rather minor, but I think this sentence is a bit difficult to
 > follow:
 >
 > - "integrations which use ..." shouldn't have a comma, as the
 >   following clause is essential for the meaning of the sentence
 >   (restrictive / essential vs. nonrestrictive / nonessential)
 >
 > - "allowed" can't be used in an impersonal way. It needs an actual
 >   subject, that is: "pivot_root() usage isn't allowed", but you can't
 >   say that "_it_ is not allowed to use pivot_root()" (I happily do this
 >   mistake all the time as a native Italian speaker).
 >
 > I would turn this to:
 >
 >   Useful for integrations which use tmpfs or initramfs as root
 >   filesystem, where pivot_root() can't be used, as it results in an
 >   invalid argument error (EINVAL).

Yes, I've been struggling with this for a long time -  I tend to apply 
Polish syntax to English sentences...
Feel free to correct any other similar issues as well.

 >> +use pivot_root(), because it results with an invalid argument error 
(EINVAL).
 >> +
 >> +By default, fallback is disabled. If pivot_root() fails, then the 
entire
 > No need for brevity here: "the fallback is disabled".
 >
 >> +sandboxing process fails.
 >> +
 >>  .TP
 >>  .BR \-\-map-guest-addr " " \fIaddr
 >>  Translate \fIaddr\fR in the guest to be equal to the guest's assigned
 >> diff --git a/passt.h b/passt.h
 >> index 16506dc..a20148e 100644
 >> --- a/passt.h
 >> +++ b/passt.h
 >> @@ -214,6 +214,7 @@ struct ip6_ctx {
 >>   * @splice_only:    Only enable loopback forwarding
 >>   * @host_lo_to_ns_lo:    Map host loopback addresses to ns loopback 
addresses
 >>   * @freebind:        Allow binding of non-local addresses for 
forwarding
 >> + * @chroot_fallback:    Use chroot() in case pivot_root() fails
 >>   * @low_wmem:        Low probed net.core.wmem_max
 >>   * @low_rmem:        Low probed net.core.rmem_max
 >>   * @no_bindtodevice:    Unprivileged SO_BINDTODEVICE not available
 >> @@ -299,6 +300,7 @@ struct ctx {
 >>      int splice_only;
 >>      int host_lo_to_ns_lo;
 >>      int freebind;
 >> +    bool chroot_fallback;
 >>
 >>      int low_wmem;
 >>      int low_rmem;
 > The rest looks good to me, let me know if you agree with these changes,
 > and if you prefer to re-post or that I just apply them.

I agree with the changes. You can aplly these fixes yourself, I don't 
mind. Thank you.

Regards,
Mateusz