Repository: osbuild/osbuild
Pull request #2006 (2025-02-07 14:21:12)

stages/kernel-cmdline: add max cmdline option
Add an option called `kernel_line_size`
to allow setting a maximum cmdline size check
value for custom kernels or other restrictions.

- This option will override the arch defaults, otherwise
- The size map is checked for current arch, otherwise
- If the current architecture is not in the map, fallback to
  4096, which is the max value allowed for
  COMMAND_LINE_SIZE.

thozza: What is the intended use case of this option?

1. To make the limit lower than the default
2. or to increase it?

If it is the first option, then I'd say the check should be done by whoever (person, script) is creating the stage options and not by the stage itself. Is the second option even technically possible (IOW is it a limitation of the platform or of how the kernel was built)?
aesteve-rh: Both. This option as it is right now in the patch, should override whatever defaults the architecture has.

I assume 2 is rare, but definitively possible, e.g., in the case of out-of-tree kernels. As an example, a quick search got me to this [patch](https://review.lineageos.org/c/LineageOS/android_kernel_xiaomi_msm8953/+/367392/1), which, as you can see, increases the size of the kernel command line. In the end the kernel will just do `strscpy(..., COMMAND_LINE_SIZE)`. And if arch defaults do not get overridden, the check will prevent the image to be built.

The case I had in mind (for automotive) is about decreasing the limit. The point you raise is valid, but having the check in `osbuild` is free once the condition is already there for arch defaults.

So if having the additional option here for case 2 is enough justification, then doing the check for case 1 is a nice addition.

thozza: @aesteve-rh Can you please fix the linter errors? Thanks!

Patch:
diff --git a/sources/org.osbuild.ostree b/sources/org.osbuild.ostree
index ed307433..8c9aaaba 100755
--- a/sources/org.osbuild.ostree
+++ b/sources/org.osbuild.ostree
@@ -16,9 +16,11 @@ supported are:
   entitlement certificate. Secret fields `consumer_cert` and
   `consumer_key` must be set.
 
-To skip TLS verification, set OSBUILD_SOURCES_OSTREE_INSECURE environment
-variable to "true". To set a HTTP(S) proxy, set OSBUILD_SOURCES_OSTREE_PROXY
-environment variable to the proxy URL.
+When one or more subpaths are specified, only those are pulled, otherwise the
+whole tree is pulled. To skip TLS verification, set
+OSBUILD_SOURCES_OSTREE_INSECURE environment variable to "true". To set a
+HTTP(S) proxy, set OSBUILD_SOURCES_OSTREE_PROXY environment variable to the
+proxy URL.
 """
 
 
@@ -57,6 +59,13 @@ SCHEMA = """
                 "type": "string",
                 "description": "content URL of the repository."
               },
+              "subpaths": {
+                "type": "array",
+                "items": {
+                  "type": "string",
+                  "description": "only include path(s) when fetching (fetch all when empty)"
+                }
+              },
               "gpgkeys": {
                 "type": "array",
                 "items": {
@@ -114,6 +123,9 @@ class OSTreeSource(sources.SourceService):
     def fetch_one(self, checksum, desc):
         commit = checksum
         remote = desc["remote"]
+        subpaths = remote.get("subpaths", [])
+        subpaths = [f'--subpath={p}' for p in subpaths]
+
         # This is a temporary remote so we'll just use a random name
         name = str(uuid.uuid4())
 
@@ -121,7 +133,7 @@ class OSTreeSource(sources.SourceService):
 
         # Transfer the commit: remote → cache
         print(f"pulling {commit}", file=sys.stderr)
-        ostree.cli("pull", name, commit, repo=self.repo)
+        ostree.cli("pull", name, commit, *subpaths, repo=self.repo)
 
         # Remove the temporary remote again
         ostree.cli("remote", "delete", name, repo=self.repo)
diff --git a/stages/org.osbuild.kernel-cmdline b/stages/org.osbuild.kernel-cmdline
index 159468ce..df72094f 100755
--- a/stages/org.osbuild.kernel-cmdline
+++ b/stages/org.osbuild.kernel-cmdline
@@ -11,7 +11,7 @@ MAX_SIZE_MAP = {
     "x86_64": 2048,
     # https://github.com/torvalds/linux/blob/master/arch/x86/include/asm/setup.h
     "i386": 2048,
-    # https://github.com/torvalds/linux/blame/master/arch/arm/include/uapi/asm/setup.h
+    # https://github.com/torvalds/linux/blob/master/arch/arm/include/uapi/asm/setup.h
     "arm": 1024,
     # https://github.com/torvalds/linux/blob/master/arch/arm64/include/uapi/asm/setup.h
     "aarch64": 2048,
@@ -29,7 +29,8 @@ MAX_SIZE_MAP = {
 def main(tree, options):
     root_fs_uuid = options.get("root_fs_uuid", "")
     additional = options.get("kernel_opts", "")
-    max_cmdline_size = MAX_SIZE_MAP.get(platform.machine().lower(), 4096)
+    max_cmdline_size = options.get("kernel_cmdline_size",
+                                   MAX_SIZE_MAP.get(platform.machine().lower(), 4096))
 
     params = []
 
diff --git a/stages/org.osbuild.kernel-cmdline.meta.json b/stages/org.osbuild.kernel-cmdline.meta.json
index 4ba73d4e..e511078b 100644
--- a/stages/org.osbuild.kernel-cmdline.meta.json
+++ b/stages/org.osbuild.kernel-cmdline.meta.json
@@ -30,6 +30,12 @@
         "description": "Additional kernel boot options",
         "type": "string",
         "default": ""
+      },
+      "kernel_cmdline_size": {
+        "description": "Sets a custom maximum kernel cmdline size",
+        "type": "integer",
+        "minimum": 256,
+        "maximum": 4096
       }
     }
   }


Test patch:
diff --git a/osbuild/testutil/net.py b/osbuild/testutil/net.py
index bb8e2885..183fdb58 100644
--- a/osbuild/testutil/net.py
+++ b/osbuild/testutil/net.py
@@ -4,8 +4,10 @@ network related utilities
 """
 import contextlib
 import http.server
+import os
 import socket
 import ssl
+import sys
 import threading
 
 try:
@@ -25,6 +27,12 @@ except ImportError:
 from .atomic import AtomicCounter
 
 
+def print_dir(directory):
+    for root, _, files in os.walk(directory):
+        for fn in files:
+            print(os.path.join(root, fn))
+
+
 def _get_free_port():
     s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     s.bind(("localhost", 0))
@@ -47,6 +55,8 @@ class SilentHTTPRequestHandler(http.server.SimpleHTTPRequestHandler):
 class DirHTTPServer(ThreadingHTTPServer):
     def __init__(self, *args, directory=None, simulate_failures=0, **kwargs):
         super().__init__(*args, **kwargs)
+        print("Serving:", file=sys.stderr)
+        print_dir(directory)
         self.directory = directory
         self.simulate_failures = AtomicCounter(simulate_failures)
         self.reqs = AtomicCounter()
diff --git a/sources/test/test_ostree_source.py b/sources/test/test_ostree_source.py
index 8d1ab750..48df06f7 100644
--- a/sources/test/test_ostree_source.py
+++ b/sources/test/test_ostree_source.py
@@ -1,5 +1,6 @@
 #!/usr/bin/python3
 
+import os
 import pathlib
 import tempfile
 
@@ -27,11 +28,12 @@ def test_ostree_source_exists(tmp_path, sources_service):
     assert sources_service.exists("sha256:" + commit, None)
 
 
-def make_test_sources(proto, port, fake_commit, **secrets):
+def make_test_sources(proto, port, fake_commit, subpaths, **secrets):
     sources = {
         fake_commit: {
             "remote": {
                 "url": f"{proto}://localhost:{port}",
+                "subpaths": subpaths,
             }
         }
     }
@@ -42,7 +44,9 @@ def make_test_sources(proto, port, fake_commit, **secrets):
 
 def make_repo(root):
     with tempfile.TemporaryDirectory() as empty_tmpdir:
-        ostree.cli("init", f"--repo={root}")
+        ostree.cli("init", "--mode=archive", f"--repo={root}")
+        os.mknod(os.path.join(empty_tmpdir, "a.txt"))
+        os.mknod(os.path.join(empty_tmpdir, "b.txt"))
         return ostree.cli("commit", f"--repo={root}", "--orphan", empty_tmpdir).stdout.rstrip()
 
 
@@ -53,12 +57,27 @@ def test_ostree_pull_plain(tmp_path, sources_service):
     fake_commit = make_repo(fake_httpd_root)
 
     with http_serve_directory(fake_httpd_root) as httpd:
-        test_sources = make_test_sources("http", httpd.server_port, fake_commit)
+        test_sources = make_test_sources("http", httpd.server_port, fake_commit, [])
         sources_service.setup({"cache": tmp_path, "options": {}})
         sources_service.fetch_all(test_sources)
         assert sources_service.exists("sha256:" + fake_commit, None)
 
 
+@pytest.mark.skipif(not has_executable("ostree"), reason="need ostree")
+def test_ostree_pull_subtree(tmp_path, sources_service, capsys):
+    fake_httpd_root = tmp_path / "fake-httpd-root"
+    fake_httpd_root.mkdir(exist_ok=True)
+    fake_commit = make_repo(fake_httpd_root)
+
+    with http_serve_directory(fake_httpd_root) as httpd:
+        test_sources = make_test_sources("http", httpd.server_port, fake_commit, ["/b.txt"])
+        sources_service.setup({"cache": tmp_path, "options": {}})
+        sources_service.fetch_all(test_sources)
+        assert sources_service.exists("sha256:" + fake_commit, None)
+
+    assert "--subpath=/b.txt" in capsys.readouterr().err
+
+
 @pytest.mark.skipif(not has_executable("ostree"), reason="need ostree")
 def test_ostree_pull_plain_mtls(tmp_path, sources_service, monkeypatch):
     fake_httpd_root = tmp_path / "fake-httpd-root"
@@ -73,7 +92,7 @@ def test_ostree_pull_plain_mtls(tmp_path, sources_service, monkeypatch):
 
     with https_serve_directory(fake_httpd_root, cert1, key1) as httpd:
         monkeypatch.setenv("OSBUILD_SOURCES_OSTREE_INSECURE", "1")
-        test_sources = make_test_sources("https", httpd.server_port, fake_commit, name="org.osbuild.mtls")
+        test_sources = make_test_sources("https", httpd.server_port, fake_commit, [], name="org.osbuild.mtls")
         sources_service.setup({"cache": tmp_path, "options": {}})
         sources_service.fetch_all(test_sources)
         assert sources_service.exists("sha256:" + fake_commit, None)
diff --git a/stages/test/test_kernel_cmdline.py b/stages/test/test_kernel_cmdline.py
new file mode 100644
index 00000000..9b0c87fd
--- /dev/null
+++ b/stages/test/test_kernel_cmdline.py
@@ -0,0 +1,63 @@
+#!/usr/bin/python3
+
+from unittest import mock
+
+import pytest  # type: ignore
+
+from osbuild import testutil
+
+STAGE_NAME = "org.osbuild.kernel-cmdline"
+
+
+@pytest.mark.parametrize("options, expected", [
+    ({"kernel_opts": "--custom-opt=some"}, "--custom-opt=some"),
+    ({"root_fs_uuid": "some-uuid"}, "root=UUID=some-uuid"),
+    ({"root_fs_uuid": "other-uuid", "kernel_opts": "--custom-opt=other"},
+     "root=UUID=other-uuid --custom-opt=other"),
+])
+def test_kernel_cmdline(tmp_path, stage_module, options, expected):
+    tree = tmp_path / "tree"
+    tree.mkdir()
+
+    stage_module.main(tree, options)
+    cmdline_file = tree / "etc" / "kernel" / "cmdline"
+    assert cmdline_file.exists()
+    assert cmdline_file.read_text() == expected
+
+
+@pytest.mark.parametrize("options, arch", [
+    ({"kernel_opts": "a" * 2049}, "x86_64"),
+    ({"kernel_opts": "a" * 1025}, "arm"),
+    ({"kernel_opts": "a" * 2049}, "aarch64"),
+    ({"kernel_opts": "a" * 257, "kernel_cmdline_size": 256}, "any"),
+])
+@mock.patch("platform.machine")
+def test_kernel_opts_size_check(mock_machine, tmp_path, stage_module, options, arch):
+    tree = tmp_path / "tree"
+    tree.mkdir()
+    mock_machine.return_value = arch
+
+    with pytest.raises(ValueError) as e:
+        stage_module.main(tree, options)
+    assert str(e.value).startswith("The size of the kernel cmdline options cannot be larger than")
+
+
+@pytest.mark.parametrize("options, expected_error", [
+    ({"kernel_cmdline_size": 256}, ""),
+    ({"kernel_cmdline_size": 4096}, ""),
+    ({"kernel_cmdline_size": "not integer"}, "is not of type 'integer'"),
+    ({"kernel_cmdline_size": 0}, "is less than the minimum of 256"),
+    ({"kernel_cmdline_size": 4097}, "is greater than the maximum of 4096"),
+])
+def test_schema_validation_kernel_cmdline(stage_schema, options, expected_error):
+    test_input = {
+        "type": STAGE_NAME,
+        "options": options
+    }
+    res = stage_schema.validate(test_input)
+
+    if not expected_error:
+        assert res.valid
+    else:
+        assert not res.valid
+        testutil.assert_jsonschema_error_contains(res, expected_error, expected_num_errs=1)