From c3015927a3d3a823c15b268939a8ff80fca982db Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 21:38:57 -0700 Subject: [PATCH 1/3] runtime: add more debug logs for non-root user operation Previously the logging was insufficient and made debugging difficult Fixes: #5155 Signed-off-by: Feng Wang --- src/runtime/pkg/containerd-shim-v2/create.go | 15 +++++++++++++-- src/runtime/virtcontainers/qemu.go | 17 +++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/runtime/pkg/containerd-shim-v2/create.go b/src/runtime/pkg/containerd-shim-v2/create.go index 65113ac1bd..70738326ad 100644 --- a/src/runtime/pkg/containerd-shim-v2/create.go +++ b/src/runtime/pkg/containerd-shim-v2/create.go @@ -26,6 +26,7 @@ import ( "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/rootless" "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" + "github.com/sirupsen/logrus" // only register the proto type crioption "github.com/containerd/containerd/pkg/runtimeoptions/v1" @@ -136,7 +137,7 @@ func create(ctx context.Context, s *service, r *taskAPI.CreateTaskRequest) (*con katautils.HandleFactory(ctx, vci, s.config) rootless.SetRootless(s.config.HypervisorConfig.Rootless) if rootless.IsRootless() { - if err := configureNonRootHypervisor(s.config); err != nil { + if err := configureNonRootHypervisor(s.config, r.ID); err != nil { return nil, err } } @@ -303,13 +304,17 @@ func doMount(mounts []*containerd_types.Mount, rootfs string) error { return nil } -func configureNonRootHypervisor(runtimeConfig *oci.RuntimeConfig) error { +func configureNonRootHypervisor(runtimeConfig *oci.RuntimeConfig, sandboxId string) error { userName, err := utils.CreateVmmUser() if err != nil { return err } defer func() { if err != nil { + shimLog.WithFields(logrus.Fields{ + "user_name": userName, + "sandbox_id": sandboxId, + }).WithError(err).Warn("configure non root hypervisor failed, delete the user") if err2 := utils.RemoveVmmUser(userName); err2 != nil { shimLog.WithField("userName", userName).WithError(err).Warn("failed to remove user") } @@ -331,6 +336,12 @@ func configureNonRootHypervisor(runtimeConfig *oci.RuntimeConfig) error { } runtimeConfig.HypervisorConfig.Uid = uint32(uid) runtimeConfig.HypervisorConfig.Gid = uint32(gid) + shimLog.WithFields(logrus.Fields{ + "user_name": userName, + "uid": uid, + "gid": gid, + "sandbox_id": sandboxId, + }).Debug("successfully created a non root user for the hypervisor") userTmpDir := path.Join("/run/user/", fmt.Sprint(uid)) _, err = os.Stat(userTmpDir) diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 6ef2310f41..439cd47f72 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -1061,13 +1061,26 @@ func (q *qemu) cleanupVM() error { if rootless.IsRootless() { u, err := user.LookupId(strconv.Itoa(int(q.config.Uid))) if err != nil { - q.Logger().WithError(err).WithField("uid", q.config.Uid).Warn("failed to find the user") + q.Logger().WithError(err).WithFields( + logrus.Fields{ + "user": u.Username, + "uid": q.config.Uid, + }).Warn("failed to find the user") return nil } if err := pkgUtils.RemoveVmmUser(u.Username); err != nil { - q.Logger().WithError(err).WithField("user", u.Username).Warn("failed to delete the user") + q.Logger().WithError(err).WithFields( + logrus.Fields{ + "user": u.Username, + "uid": q.config.Uid, + }).Warn("failed to delete the user") } + q.Logger().WithFields( + logrus.Fields{ + "user": u.Username, + "uid": q.config.Uid, + }).Debug("successfully removed the non root user") } return nil From 5cafe217703b44edfa5de964a1ec1af73ee2dd41 Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 21:56:15 -0700 Subject: [PATCH 2/3] runtime: make StopVM thread-safe StopVM can be invoked by multiple threads and needs to be thread-safe Fixes: #5155 Signed-off-by: Feng Wang --- src/runtime/virtcontainers/qemu.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 439cd47f72..051d405ffc 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -110,6 +110,8 @@ type qemu struct { nvdimmCount int stopped bool + + mu sync.Mutex } const ( @@ -968,6 +970,8 @@ func (q *qemu) waitVM(ctx context.Context, timeout int) error { // StopVM will stop the Sandbox's VM. func (q *qemu) StopVM(ctx context.Context, waitOnly bool) error { + q.mu.Lock() + defer q.mu.Unlock() span, _ := katatrace.Trace(ctx, q.Logger(), "StopVM", qemuTracingTags, map[string]string{"sandbox_id": q.id}) defer span.End() From f914319874f6bc094081cac8c5997e866e1ca64b Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 22:09:35 -0700 Subject: [PATCH 3/3] runtime: store the user name in hypervisor config The user name will be used to delete the user instead of relying on uid lookup because uid can be reused. Fixes: #5155 Signed-off-by: Feng Wang --- src/runtime/pkg/containerd-shim-v2/create.go | 1 + src/runtime/virtcontainers/hypervisor.go | 3 +++ src/runtime/virtcontainers/qemu.go | 15 +++++++-------- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/runtime/pkg/containerd-shim-v2/create.go b/src/runtime/pkg/containerd-shim-v2/create.go index 70738326ad..474925b122 100644 --- a/src/runtime/pkg/containerd-shim-v2/create.go +++ b/src/runtime/pkg/containerd-shim-v2/create.go @@ -335,6 +335,7 @@ func configureNonRootHypervisor(runtimeConfig *oci.RuntimeConfig, sandboxId stri return err } runtimeConfig.HypervisorConfig.Uid = uint32(uid) + runtimeConfig.HypervisorConfig.User = userName runtimeConfig.HypervisorConfig.Gid = uint32(gid) shimLog.WithFields(logrus.Fields{ "user_name": userName, diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 0e7b4785bb..01604fe1bf 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -380,6 +380,9 @@ type HypervisorConfig struct { // BlockiDeviceAIO specifies the I/O API to be used. BlockDeviceAIO string + // The user maps to the uid. + User string + // KernelParams are additional guest kernel parameters. KernelParams []Param diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index 051d405ffc..785aa0182c 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -680,7 +680,7 @@ func (q *qemu) checkBpfEnabled() { q.Logger().WithError(err).Warningf("failed to get bpf_jit_enable status") return } - enabled, err := strconv.Atoi(string(out)) + enabled, err := strconv.Atoi(strings.TrimSpace(string(out))) if err != nil { q.Logger().WithError(err).Warningf("failed to convert bpf_jit_enable status to integer") return @@ -1063,26 +1063,25 @@ func (q *qemu) cleanupVM() error { } if rootless.IsRootless() { - u, err := user.LookupId(strconv.Itoa(int(q.config.Uid))) - if err != nil { + if _, err := user.Lookup(q.config.User); err != nil { q.Logger().WithError(err).WithFields( logrus.Fields{ - "user": u.Username, + "user": q.config.User, "uid": q.config.Uid, - }).Warn("failed to find the user") + }).Warn("failed to find the user, it might have been removed") return nil } - if err := pkgUtils.RemoveVmmUser(u.Username); err != nil { + if err := pkgUtils.RemoveVmmUser(q.config.User); err != nil { q.Logger().WithError(err).WithFields( logrus.Fields{ - "user": u.Username, + "user": q.config.User, "uid": q.config.Uid, }).Warn("failed to delete the user") } q.Logger().WithFields( logrus.Fields{ - "user": u.Username, + "user": q.config.User, "uid": q.config.Uid, }).Debug("successfully removed the non root user") }