Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions contextual-classifier/ContextualClassifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -471,11 +471,43 @@ void ContextualClassifier::loadStaticProcData() {
LOGI(CLASSIFIER_TAG, "Successfully loaded static proc data");
}

int8_t ContextualClassifier::isContainerProcess(pid_t pid) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current name-based filtering is more of a Docker-specific workaround. Since systemd-run, systemd-nspawn, and other cgroup-managed workloads may run into the same issue, is there a more generic solution based on cgroup hierarchy rather than process/container names?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is intentionally specific to Docker.
Consider this a hotfix for the issue reported in https://orbit/CR/4689720. A more generic solution (e.g. detecting any external cgroup manager, not just Docker) that we can track through #333 urm issue

// Check if the process lives inside a container cgroup scope by reading
// /proc/<pid>/cgroup. A Docker container init process has a path like:
// 0::/system.slice/docker-<CID>.scope
// Moving it out of this scope causes systemd to remove the now-empty scope,
// which breaks subsequent docker exec calls with:
// "can't open cgroup: .../docker-<CID>.scope: No such file or directory"

// TODO: extend to Kubernetes container runtimes when needed:
// - containerd (k8s default runtime): cgroup path contains "containerd-<CID>.scope"
// - CRI-O (OpenShift / k8s alternative): cgroup path contains "crio-<CID>.scope"

std::ifstream cgroupFile("/proc/" + std::to_string(pid) + "/cgroup");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we add this to a exclusion list that classifier already has, why whitelisting code needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rajulup

  1. docker run
    → we Container(repo) created using this command (docker run -dit --name repro python:3.12-slim)
    → New container PID 2223 placed in system.slice/docker-bd04adaedc13.scope ✅

  2. URM kicks in
    → Sees PID 2223 (python3 one)
    → Moves it to urm.slice/focused.apps ⚠️

  3. docker-bd04adaedc13.scope is now EMPTY
    → systemd deletes the scope 🗑️

  4. docker exec repro /bin/true
    → runc looks for PID 2223 at docker-bd04adaedc13.scope
    → SCOPE IS GONE 💥
    → "can't open cgroup: no such file or directory" ❌

    so all docker images type we have add in blocker list. that's why come with this solution.

if(cgroupFile.is_open()) {
std::string line;
while(std::getline(cgroupFile, line)) {
if(line.find("docker-") != std::string::npos &&
line.find(".scope") != std::string::npos) {
return true;
}
}
}
Comment on lines +485 to +495

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functionality] Fragile substring matching for container-scope detection - Medium

In isContainerProcess, the check line.find("docker-") != std::string::npos && line.find(".scope") != std::string::npos only verifies that both substrings exist somewhere in the line, without validating their relative order or that they form a single contiguous token (e.g., docker-<CID>.scope). This could theoretically produce false positives on unusual cgroup path strings that happen to contain both substrings unrelated to a real docker scope, or false negatives if .scope appears before docker- in some other combined cgroup line format. A regex-based match anchored to the expected pattern (e.g., docker-[A-Za-z0-9]+\.scope) would be more robust.

Fixed Code Snippet
#include <regex>
...
static const std::regex kDockerScopeRegex(R"(docker-[0-9a-fA-F]+\.scope)");
...
while(std::getline(cgroupFile, line)) {
    if(std::regex_search(line, kDockerScopeRegex)) {
        return true;
    }
}

return false;
}

int8_t ContextualClassifier::shouldProcBeIgnored(int32_t evType, pid_t pid) {
if(evType == CC_APP_CLOSE) {
return false;
Comment on lines +484 to 501

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functionality] Substring-based docker scope detection may produce false positives

The detection logic checks whether a single cgroup line contains both "docker-" and ".scope" substrings anywhere in the line, without validating structure (e.g., that they are contiguous as docker-<CID>.scope). A cgroup line with unrelated content that happens to include both substrings (e.g., a path segment named similarly) could be misidentified as a container process, causing legitimate processes to be skipped by URM.

Consider using a more precise match, e.g., checking that .scope immediately follows the docker-<CID> pattern, or using a regex to validate the expected format.

Fixed Code Snippet
// Example of tighter matching using find + suffix check
size_t dockerPos = line.find("docker-");
if(dockerPos != std::string::npos &&
   line.compare(line.size() - 6, 6, ".scope") == 0) {
    return true;
}

}

// Skip Docker container init processes to avoid breaking docker exec.
if(this->isContainerProcess(pid)) {
LOGI(CLASSIFIER_TAG,
"Ignoring Docker container process PID: " + std::to_string(pid));
return true;
}

std::string procName = "";
if(AuxRoutines::fetchComm(pid, procName) != 0) {
return true;
Expand Down
4 changes: 4 additions & 0 deletions contextual-classifier/Include/ContextualClassifier.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ class ContextualClassifier {
int8_t checkClientPriority(const std::string& procName);
int8_t shouldProcBeIgnored(int32_t evType, pid_t pid);

// Returns true if the process lives inside a container cgroup scope
// (e.g. docker-<CID>.scope), indicating it should not be moved by URM.
int8_t isContainerProcess(pid_t pid);

// Transparently get the classification object, without expecting the client
// to be aware of the underlying model / implementation.
Inference* GetInferenceObject();
Expand Down
Loading