From fba39ef32d03c3bd82f59835386502b193d97072 Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 21:38:57 -0700 Subject: [PATCH 1/5] 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 (cherry picked from commit c3015927a3d3a823c15b268939a8ff80fca982db) Signed-off-by: Greg Kurz --- 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 81801888a29f9f67006a89d6b288c9132eb93448 Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 21:56:15 -0700 Subject: [PATCH 2/5] 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 (cherry picked from commit 5cafe217703b44edfa5de964a1ec1af73ee2dd41) Signed-off-by: Greg Kurz --- 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 43b0e95800f60bfd5b39d43e8a5d67c3de4f224e Mon Sep 17 00:00:00 2001 From: Feng Wang Date: Mon, 12 Sep 2022 22:09:35 -0700 Subject: [PATCH 3/5] 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 (cherry picked from commit f914319874f6bc094081cac8c5997e866e1ca64b) Signed-off-by: Greg Kurz --- 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") } From d44e39e0592c38d6707a4ae0907cf7495526e613 Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Mon, 19 Sep 2022 16:03:06 +0800 Subject: [PATCH 4/5] runtime-rs: fix incorrect comments Some comments for types are incorrect in file src/libs/kata-types/src/config/hypervisor/mod.rs Fixes: #5187 Signed-off-by: Bin Liu (cherry picked from commit 3f65ff2d07409cdcc54d08dfa996ed34293f27c8) Signed-off-by: Greg Kurz --- src/libs/kata-types/src/config/hypervisor/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libs/kata-types/src/config/hypervisor/mod.rs b/src/libs/kata-types/src/config/hypervisor/mod.rs index 0ab024c6c9..0df6693226 100644 --- a/src/libs/kata-types/src/config/hypervisor/mod.rs +++ b/src/libs/kata-types/src/config/hypervisor/mod.rs @@ -320,7 +320,7 @@ impl CpuInfo { } } -/// Configuration information for shared filesystem, such virtio-9p and virtio-fs. +/// Configuration information for debug #[derive(Clone, Debug, Default, Deserialize, Serialize)] pub struct DebugInfo { /// This option changes the default hypervisor and kernel parameters to enable debug output @@ -596,7 +596,7 @@ impl MemoryInfo { } } -/// Configuration information for virtual machine. +/// Configuration information for network. #[derive(Clone, Debug, Default, Deserialize, Serialize)] pub struct NetworkInfo { /// If vhost-net backend for virtio-net is not desired, set to true. @@ -638,7 +638,7 @@ impl NetworkInfo { } } -/// Configuration information for virtual machine. +/// Configuration information for security. #[derive(Clone, Debug, Default, Deserialize, Serialize)] pub struct SecurityInfo { /// Enable running QEMU VMM as a non-root user. From b0c5f040f02f9444d30e1d969cc527b9146e03d1 Mon Sep 17 00:00:00 2001 From: Bin Liu Date: Tue, 27 Sep 2022 11:47:37 +0800 Subject: [PATCH 5/5] runtime-rs: set agent timeout to 0 for stream RPCs For stream RPCs: - write_stdin - read_stdout - read_stderr there should be no timeout (by setting it to 0). Fixes: #5249 Signed-off-by: Bin Liu (cherry picked from commit 20bcaf0e363cf581cd45b3b7973bb674dc1ccd4f) Signed-off-by: Greg Kurz --- src/runtime-rs/crates/agent/src/kata/agent.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/runtime-rs/crates/agent/src/kata/agent.rs b/src/runtime-rs/crates/agent/src/kata/agent.rs index 52afbee9de..31b3d6635d 100644 --- a/src/runtime-rs/crates/agent/src/kata/agent.rs +++ b/src/runtime-rs/crates/agent/src/kata/agent.rs @@ -96,9 +96,9 @@ impl_agent!( stats_container | crate::ContainerID | crate::StatsContainerResponse | None, pause_container | crate::ContainerID | crate::Empty | None, resume_container | crate::ContainerID | crate::Empty | None, - write_stdin | crate::WriteStreamRequest | crate::WriteStreamResponse | None, - read_stdout | crate::ReadStreamRequest | crate::ReadStreamResponse | None, - read_stderr | crate::ReadStreamRequest | crate::ReadStreamResponse | None, + write_stdin | crate::WriteStreamRequest | crate::WriteStreamResponse | Some(0), + read_stdout | crate::ReadStreamRequest | crate::ReadStreamResponse | Some(0), + read_stderr | crate::ReadStreamRequest | crate::ReadStreamResponse | Some(0), close_stdin | crate::CloseStdinRequest | crate::Empty | None, tty_win_resize | crate::TtyWinResizeRequest | crate::Empty | None, update_interface | crate::UpdateInterfaceRequest | crate::Interface | None,