From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from atuin.qyliss.net (localhost [IPv6:::1]) by atuin.qyliss.net (Postfix) with ESMTP id D54BD77D4; Thu, 23 Jul 2026 23:07:59 +0000 (UTC) Received: by atuin.qyliss.net (Postfix, from userid 993) id 45CBD772F; Thu, 23 Jul 2026 23:07:57 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on atuin.qyliss.net X-Spam-Level: X-Spam-Status: No, score=-0.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DMARC_PASS,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=4.0.1 Received: from mail-yw1-x112c.google.com (mail-yw1-x112c.google.com [IPv6:2607:f8b0:4864:20::112c]) by atuin.qyliss.net (Postfix) with ESMTPS id 09750772C for ; Thu, 23 Jul 2026 23:07:54 +0000 (UTC) Received: by mail-yw1-x112c.google.com with SMTP id 00721157ae682-80e24970f1dso7558767b3.0 for ; Thu, 23 Jul 2026 16:07:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1784848067; x=1785452867; darn=spectrum-os.org; h=content-type:autocrypt:in-reply-to:content-language:references:cc :to:subject:from:user-agent:mime-version:date:message-id:from:to:cc :subject:date:message-id:reply-to:content-type; bh=WyPj4kH2aNbkJbkJ0hJTFQ3QM8+rvqm3eCsjKEnSRQc=; b=RUBtEjpLvIBSLwNOzMwj+mdxV/aoOOVEWdObPkMQLQ5zcXLdDyCdv14MRvQG0xKhrN 1ETz3EFaaqNV280wQP42m6tYQELhVz+h77hfSJE7vPm/x/OQD+C3ibGZNHliEB8+qMOz ecNeP9KnrY/AxEvMTg0Zq03J+4ktHpapdlPnac2PQHFWEyv8oVqMWkfhFxYOQombDkYn /r2pEh2S9ybYAPEhhCGMSr6Rl0mXwluEkyOiP/yAc+AYq4QRixYQow7eMc1dNGS+5eRS GzQ+cGaJa1FprvGvdlrHpafMR+7XScetSgwRaSek9zTO4JZpEE8RU6Pgmp32BxLY5Ylz fWLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784848067; x=1785452867; h=content-type:autocrypt:in-reply-to:content-language:references:cc :to:subject:from:user-agent:mime-version:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=WyPj4kH2aNbkJbkJ0hJTFQ3QM8+rvqm3eCsjKEnSRQc=; b=NRG7nxckUAuN6ulVhnBvecrly356VSim//WFp0zfpet84gm0wttkNXOFWS5apzgM7n QzZCTC0S1YuZGxVNQOS0f/2UdYUX/R4LY+MEQ2bZHar8u+Y+hSZcegH8jh8UG66PmK94 kpZ/zuq+PPcFfwsQmhXCmwSdyUvP5mEHstA+cPeZDNn6u4+9cbZ3c7HX7qHMRZmfe75Z YVXppscWGV4BBjP87/Oxkoak4A9JKBK8/3zjMQ7uLRgAkP0qr4xwFgm3au9+Z4LPuIzK YXi3jiddOJeB9IBc6FXFUdTTtadDb3QgeNNgCHSQcfjjICC6Y7jxxjUZopKEOdauSXAh foVQ== X-Gm-Message-State: AOJu0YwRH8tODKQqLjJnmK2bzTPUUiFdO0XmoUYq0LX8Krfu7rRT/oNr gNxlhoMnne6kJEeXNgvsjLlaFLvE6kEjMislod1BMp79vUJGqYaWLLvyToDJLA== X-Gm-Gg: AR+sD10VSrArplEaw5TxKJkRbcmzyyiLshak3xT4ADy5hccfq1TC2xugmIu5wNNWZK8 LvEaGv955emKpRvrxhOC3mDwpWPDbyEUp7Ok+/exITotl7aR++RdEMlXPPFatRX6s/J3PozIM8/ qhRvlCWR1KBrdNnFKIgloi/KJEBhvPOSaT4aNIPBSemCQPpgBTw+/I8Zbw25rjdsTgcfPhZeWPt jbnu9x93C7L/uJg/a1K3j4YriiD13rxV7ypso2x7pf3tGykcrn0DDNnC0jA7+GXPPU0owjALK36 EkZJAOX9vAUd4/vT5dXGVpuFaIXFgYk6IiXlhNWVJcJQrqDIsuakMOAEroAt3VQB5ysehlUJm8G ImF/doD7bP3XAl6WyGD8lRFVeUx715+iI1j2q3rdWENfhDVCIbgKm02Kg2q+jCdvwY0qL1oczrI 6Z/rLB X-Received: by 2002:a05:690c:6f05:b0:81d:2254:8504 with SMTP id 00721157ae682-81f4c302746mr17749657b3.56.1784848066614; Thu, 23 Jul 2026 16:07:46 -0700 (PDT) Received: from [10.138.10.6] ([185.98.168.14]) by smtp.gmail.com with ESMTPSA id 00721157ae682-81f33c6432csm35470477b3.20.2026.07.23.16.07.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Jul 2026 16:07:45 -0700 (PDT) Message-ID: Date: Thu, 23 Jul 2026 19:07:41 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Demi Marie Obenour Subject: Re: [PATCH v4 02/20] tools: Add control group manager To: Alyssa Ross References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> <87a4rjrp2t.fsf@alyssa.is> Content-Language: en-US In-Reply-To: <87a4rjrp2t.fsf@alyssa.is> Autocrypt: addr=demiobenour@gmail.com; keydata= xsFNBFp+A0oBEADffj6anl9/BHhUSxGTICeVl2tob7hPDdhHNgPR4C8xlYt5q49yB+l2nipd aq+4Gk6FZfqC825TKl7eRpUjMriwle4r3R0ydSIGcy4M6eb0IcxmuPYfbWpr/si88QKgyGSV Z7GeNW1UnzTdhYHuFlk8dBSmB1fzhEYEk0RcJqg4AKoq6/3/UorR+FaSuVwT7rqzGrTlscnT DlPWgRzrQ3jssesI7sZLm82E3pJSgaUoCdCOlL7MMPCJwI8JpPlBedRpe9tfVyfu3euTPLPx wcV3L/cfWPGSL4PofBtB8NUU6QwYiQ9Hzx4xOyn67zW73/G0Q2vPPRst8LBDqlxLjbtx/WLR 6h3nBc3eyuZ+q62HS1pJ5EvUT1vjyJ1ySrqtUXWQ4XlZyoEFUfpJxJoN0A9HCxmHGVckzTRl 5FMWo8TCniHynNXsBtDQbabt7aNEOaAJdE7to0AH3T/Bvwzcp0ZJtBk0EM6YeMLtotUut7h2 Bkg1b//r6bTBswMBXVJ5H44Qf0+eKeUg7whSC9qpYOzzrm7+0r9F5u3qF8ZTx55TJc2g656C 9a1P1MYVysLvkLvS4H+crmxA/i08Tc1h+x9RRvqba4lSzZ6/Tmt60DPM5Sc4R0nSm9BBff0N m0bSNRS8InXdO1Aq3362QKX2NOwcL5YaStwODNyZUqF7izjK4QARAQABzTxEZW1pIE1hcmll IE9iZW5vdXIgKGxvdmVyIG9mIGNvZGluZykgPGRlbWlvYmVub3VyQGdtYWlsLmNvbT7CwXgE EwECACIFAlp+A0oCGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJELKItV//nCLBhr8Q AK/xrb4wyi71xII2hkFBpT59ObLN+32FQT7R3lbZRjVFjc6yMUjOb1H/hJVxx+yo5gsSj5LS 9AwggioUSrcUKldfA/PKKai2mzTlUDxTcF3vKx6iMXKA6AqwAw4B57ZEJoMM6egm57TV19kz PMc879NV2nc6+elaKl+/kbVeD3qvBuEwsTe2Do3HAAdrfUG/j9erwIk6gha/Hp9yZlCnPTX+ VK+xifQqt8RtMqS5R/S8z0msJMI/ajNU03kFjOpqrYziv6OZLJ5cuKb3bZU5aoaRQRDzkFIR 6aqtFLTohTo20QywXwRa39uFaOT/0YMpNyel0kdOszFOykTEGI2u+kja35g9TkH90kkBTG+a EWttIht0Hy6YFmwjcAxisSakBuHnHuMSOiyRQLu43ej2+mDWgItLZ48Mu0C3IG1seeQDjEYP tqvyZ6bGkf2Vj+L6wLoLLIhRZxQOedqArIk/Sb2SzQYuxN44IDRt+3ZcDqsPppoKcxSyd1Ny 2tpvjYJXlfKmOYLhTWs8nwlAlSHX/c/jz/ywwf7eSvGknToo1Y0VpRtoxMaKW1nvH0OeCSVJ itfRP7YbiRVc2aNqWPCSgtqHAuVraBRbAFLKh9d2rKFB3BmynTUpc1BQLJP8+D5oNyb8Ts4x Xd3iV/uD8JLGJfYZIR7oGWFLP4uZ3tkneDfYzsFNBFp+A0oBEAC9ynZI9LU+uJkMeEJeJyQ/ 8VFkCJQPQZEsIGzOTlPnwvVna0AS86n2Z+rK7R/usYs5iJCZ55/JISWd8xD57ue0eB47bcJv VqGlObI2DEG8TwaW0O0duRhDgzMEL4t1KdRAepIESBEA/iPpI4gfUbVEIEQuqdqQyO4GAe+M kD0Hy5JH/0qgFmbaSegNTdQg5iqYjRZ3ttiswalql1/iSyv1WYeC1OAs+2BLOAT2NEggSiVO txEfgewsQtCWi8H1SoirakIfo45Hz0tk/Ad9ZWh2PvOGt97Ka85o4TLJxgJJqGEnqcFUZnJJ riwoaRIS8N2C8/nEM53jb1sH0gYddMU3QxY7dYNLIUrRKQeNkF30dK7V6JRH7pleRlf+wQcN fRAIUrNlatj9TxwivQrKnC9aIFFHEy/0mAgtrQShcMRmMgVlRoOA5B8RTulRLCmkafvwuhs6 dCxN0GNAORIVVFxjx9Vn7OqYPgwiofZ6SbEl0hgPyWBQvE85klFLZLoj7p+joDY1XNQztmfA rnJ9x+YV4igjWImINAZSlmEcYtd+xy3Li/8oeYDAqrsnrOjb+WvGhCykJk4urBog2LNtcyCj kTs7F+WeXGUo0NDhbd3Z6AyFfqeF7uJ3D5hlpX2nI9no/ugPrrTVoVZAgrrnNz0iZG2DVx46 x913pVKHl5mlYQARAQABwsFfBBgBAgAJBQJafgNKAhsMAAoJELKItV//nCLBwNIP/AiIHE8b oIqReFQyaMzxq6lE4YZCZNj65B/nkDOvodSiwfwjjVVE2V3iEzxMHbgyTCGA67+Bo/d5aQGj gn0TPtsGzelyQHipaUzEyrsceUGWYoKXYyVWKEfyh0cDfnd9diAm3VeNqchtcMpoehETH8fr RHnJdBcjf112PzQSdKC6kqU0Q196c4Vp5HDOQfNiDnTf7gZSj0BraHOByy9LEDCLhQiCmr+2 E0rW4tBtDAn2HkT9uf32ZGqJCn1O+2uVfFhGu6vPE5qkqrbSE8TG+03H8ecU2q50zgHWPdHM OBvy3EhzfAh2VmOSTcRK+tSUe/u3wdLRDPwv/DTzGI36Kgky9MsDC5gpIwNbOJP2G/q1wT1o Gkw4IXfWv2ufWiXqJ+k7HEi2N1sree7Dy9KBCqb+ca1vFhYPDJfhP75I/VnzHVssZ/rYZ9+5 1yDoUABoNdJNSGUYl+Yh9Pw9pE3Kt4EFzUlFZWbE4xKL/NPno+z4J9aWemLLszcYz/u3XnbO vUSQHSrmfOzX3cV4yfmjM5lewgSstoxGyTx2M8enslgdXhPthZlDnTnOT+C+OTsh8+m5tos8 HQjaPM01MKBiAqdPgksm1wu2DrrwUi6ChRVTUBcj6+/9IJ81H2P2gJk3Ls3AVIxIffLoY34E +MYSfkEjBz0E8CLOcAw7JIwAaeBT Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------4yMvcemXzzYetUZJvh0GzikH" Message-ID-Hash: MC5NMK24GQGS3ED4XVI4YP2YEYJUB26A X-Message-ID-Hash: MC5NMK24GQGS3ED4XVI4YP2YEYJUB26A X-MailFrom: demiobenour@gmail.com X-Mailman-Rule-Hits: member-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; header-match-devel.spectrum-os.org-0; header-match-devel.spectrum-os.org-1; header-match-devel.spectrum-os.org-2; header-match-devel.spectrum-os.org-3; header-match-devel.spectrum-os.org-4; emergency CC: Spectrum OS Development X-Mailman-Version: 3.3.10 Precedence: list List-Id: Patches and low-level development discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --------------4yMvcemXzzYetUZJvh0GzikH Content-Type: multipart/mixed; boundary="------------Oah20Kd60CV0uZydWctY8V08"; protected-headers="v1"; hp="clear" Message-ID: Date: Thu, 23 Jul 2026 19:07:41 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Demi Marie Obenour Subject: Re: [PATCH v4 02/20] tools: Add control group manager To: Alyssa Ross Cc: Spectrum OS Development References: <20260721-cgroups-v4-0-46b2e5fff7b6@gmail.com> <20260721-cgroups-v4-2-46b2e5fff7b6@gmail.com> <87a4rjrp2t.fsf@alyssa.is> Content-Language: en-US In-Reply-To: <87a4rjrp2t.fsf@alyssa.is> Autocrypt: addr=demiobenour@gmail.com; keydata= xsFNBFp+A0oBEADffj6anl9/BHhUSxGTICeVl2tob7hPDdhHNgPR4C8xlYt5q49yB+l2nipd aq+4Gk6FZfqC825TKl7eRpUjMriwle4r3R0ydSIGcy4M6eb0IcxmuPYfbWpr/si88QKgyGSV Z7GeNW1UnzTdhYHuFlk8dBSmB1fzhEYEk0RcJqg4AKoq6/3/UorR+FaSuVwT7rqzGrTlscnT DlPWgRzrQ3jssesI7sZLm82E3pJSgaUoCdCOlL7MMPCJwI8JpPlBedRpe9tfVyfu3euTPLPx wcV3L/cfWPGSL4PofBtB8NUU6QwYiQ9Hzx4xOyn67zW73/G0Q2vPPRst8LBDqlxLjbtx/WLR 6h3nBc3eyuZ+q62HS1pJ5EvUT1vjyJ1ySrqtUXWQ4XlZyoEFUfpJxJoN0A9HCxmHGVckzTRl 5FMWo8TCniHynNXsBtDQbabt7aNEOaAJdE7to0AH3T/Bvwzcp0ZJtBk0EM6YeMLtotUut7h2 Bkg1b//r6bTBswMBXVJ5H44Qf0+eKeUg7whSC9qpYOzzrm7+0r9F5u3qF8ZTx55TJc2g656C 9a1P1MYVysLvkLvS4H+crmxA/i08Tc1h+x9RRvqba4lSzZ6/Tmt60DPM5Sc4R0nSm9BBff0N m0bSNRS8InXdO1Aq3362QKX2NOwcL5YaStwODNyZUqF7izjK4QARAQABzTxEZW1pIE1hcmll IE9iZW5vdXIgKGxvdmVyIG9mIGNvZGluZykgPGRlbWlvYmVub3VyQGdtYWlsLmNvbT7CwXgE EwECACIFAlp+A0oCGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJELKItV//nCLBhr8Q AK/xrb4wyi71xII2hkFBpT59ObLN+32FQT7R3lbZRjVFjc6yMUjOb1H/hJVxx+yo5gsSj5LS 9AwggioUSrcUKldfA/PKKai2mzTlUDxTcF3vKx6iMXKA6AqwAw4B57ZEJoMM6egm57TV19kz PMc879NV2nc6+elaKl+/kbVeD3qvBuEwsTe2Do3HAAdrfUG/j9erwIk6gha/Hp9yZlCnPTX+ VK+xifQqt8RtMqS5R/S8z0msJMI/ajNU03kFjOpqrYziv6OZLJ5cuKb3bZU5aoaRQRDzkFIR 6aqtFLTohTo20QywXwRa39uFaOT/0YMpNyel0kdOszFOykTEGI2u+kja35g9TkH90kkBTG+a EWttIht0Hy6YFmwjcAxisSakBuHnHuMSOiyRQLu43ej2+mDWgItLZ48Mu0C3IG1seeQDjEYP tqvyZ6bGkf2Vj+L6wLoLLIhRZxQOedqArIk/Sb2SzQYuxN44IDRt+3ZcDqsPppoKcxSyd1Ny 2tpvjYJXlfKmOYLhTWs8nwlAlSHX/c/jz/ywwf7eSvGknToo1Y0VpRtoxMaKW1nvH0OeCSVJ itfRP7YbiRVc2aNqWPCSgtqHAuVraBRbAFLKh9d2rKFB3BmynTUpc1BQLJP8+D5oNyb8Ts4x Xd3iV/uD8JLGJfYZIR7oGWFLP4uZ3tkneDfYzsFNBFp+A0oBEAC9ynZI9LU+uJkMeEJeJyQ/ 8VFkCJQPQZEsIGzOTlPnwvVna0AS86n2Z+rK7R/usYs5iJCZ55/JISWd8xD57ue0eB47bcJv VqGlObI2DEG8TwaW0O0duRhDgzMEL4t1KdRAepIESBEA/iPpI4gfUbVEIEQuqdqQyO4GAe+M kD0Hy5JH/0qgFmbaSegNTdQg5iqYjRZ3ttiswalql1/iSyv1WYeC1OAs+2BLOAT2NEggSiVO txEfgewsQtCWi8H1SoirakIfo45Hz0tk/Ad9ZWh2PvOGt97Ka85o4TLJxgJJqGEnqcFUZnJJ riwoaRIS8N2C8/nEM53jb1sH0gYddMU3QxY7dYNLIUrRKQeNkF30dK7V6JRH7pleRlf+wQcN fRAIUrNlatj9TxwivQrKnC9aIFFHEy/0mAgtrQShcMRmMgVlRoOA5B8RTulRLCmkafvwuhs6 dCxN0GNAORIVVFxjx9Vn7OqYPgwiofZ6SbEl0hgPyWBQvE85klFLZLoj7p+joDY1XNQztmfA rnJ9x+YV4igjWImINAZSlmEcYtd+xy3Li/8oeYDAqrsnrOjb+WvGhCykJk4urBog2LNtcyCj kTs7F+WeXGUo0NDhbd3Z6AyFfqeF7uJ3D5hlpX2nI9no/ugPrrTVoVZAgrrnNz0iZG2DVx46 x913pVKHl5mlYQARAQABwsFfBBgBAgAJBQJafgNKAhsMAAoJELKItV//nCLBwNIP/AiIHE8b oIqReFQyaMzxq6lE4YZCZNj65B/nkDOvodSiwfwjjVVE2V3iEzxMHbgyTCGA67+Bo/d5aQGj gn0TPtsGzelyQHipaUzEyrsceUGWYoKXYyVWKEfyh0cDfnd9diAm3VeNqchtcMpoehETH8fr RHnJdBcjf112PzQSdKC6kqU0Q196c4Vp5HDOQfNiDnTf7gZSj0BraHOByy9LEDCLhQiCmr+2 E0rW4tBtDAn2HkT9uf32ZGqJCn1O+2uVfFhGu6vPE5qkqrbSE8TG+03H8ecU2q50zgHWPdHM OBvy3EhzfAh2VmOSTcRK+tSUe/u3wdLRDPwv/DTzGI36Kgky9MsDC5gpIwNbOJP2G/q1wT1o Gkw4IXfWv2ufWiXqJ+k7HEi2N1sree7Dy9KBCqb+ca1vFhYPDJfhP75I/VnzHVssZ/rYZ9+5 1yDoUABoNdJNSGUYl+Yh9Pw9pE3Kt4EFzUlFZWbE4xKL/NPno+z4J9aWemLLszcYz/u3XnbO vUSQHSrmfOzX3cV4yfmjM5lewgSstoxGyTx2M8enslgdXhPthZlDnTnOT+C+OTsh8+m5tos8 HQjaPM01MKBiAqdPgksm1wu2DrrwUi6ChRVTUBcj6+/9IJ81H2P2gJk3Ls3AVIxIffLoY34E +MYSfkEjBz0E8CLOcAw7JIwAaeBT --------------Oah20Kd60CV0uZydWctY8V08 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 7/22/26 12:01, Alyssa Ross wrote: > Demi Marie Obenour writes: >=20 >> The cgroup-setup Rust program can create and purge cgroups. It can al= so >> wait for one to become empty, spawn a program in a cgroup, and more. = In >> the future, it will also support cgroup-based resource control. Locki= ng >> is used to ensure that concurrent invocations are safe. >> >> This program can also be used in an s6 finish script. When passed the= >> args of such a script, it automatically purges the correct cgroup. It= >> also tells s6 to not restart the service if it dumped core. Core dump= s >> are often due to memory corruption, and automatically restarting a >> service that dumped core makes memory corruption attacks easier. >> >> Signed-off-by: Demi Marie Obenour >> --- >> .codespellrc | 2 +- >> host/rootfs/default.nix | 6 +- >> host/rootfs/file-list.mk | 2 + >> host/rootfs/image/usr/bin/cgroup-purge | 1 + >> host/rootfs/image/usr/bin/cgroup-s6-finish | 1 + >> pkgs/default.nix | 1 + >> tools/cgroup-setup/Cargo.lock | 67 ++++++ >> tools/cgroup-setup/Cargo.lock.license | 2 + >> tools/cgroup-setup/Cargo.toml | 11 + >> tools/cgroup-setup/default.nix | 18 ++ >> tools/cgroup-setup/src/cgroup.rs | 349 ++++++++++++++++++++= +++++++++ >> tools/cgroup-setup/src/main.rs | 347 ++++++++++++++++++++= ++++++++ >> 12 files changed, 803 insertions(+), 4 deletions(-) >=20 >> diff --git a/host/rootfs/file-list.mk b/host/rootfs/file-list.mk >> index 3899d620717fc97f42e669e5313c4100dcf5b1cd..e1280ab56d8797e40b9b1c= 584ab0daef3cda41d7 100644 >> --- a/host/rootfs/file-list.mk >> +++ b/host/rootfs/file-list.mk >> @@ -79,6 +79,8 @@ LINKS =3D \ >> image/etc/s6-linux-init/run-image/service/vmm/template/run \ >> image/lib \ >> image/sbin \ >> + image/usr/bin/cgroup-purge \ >> + image/usr/bin/cgroup-s6-finish \ >> image/usr/bin/systemd-udevd >> =20 >> S6_RC_FILES =3D \ >> diff --git a/host/rootfs/image/usr/bin/cgroup-purge b/host/rootfs/imag= e/usr/bin/cgroup-purge >> new file mode 120000 >> index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb= 0613e39acc9df979df >> --- /dev/null >> +++ b/host/rootfs/image/usr/bin/cgroup-purge >> @@ -0,0 +1 @@ >> +cgroup-setup >> \ No newline at end of file >> diff --git a/host/rootfs/image/usr/bin/cgroup-s6-finish b/host/rootfs/= image/usr/bin/cgroup-s6-finish >> new file mode 120000 >> index 0000000000000000000000000000000000000000..a0c8d8e144d72b69c613eb= 0613e39acc9df979df >> --- /dev/null >> +++ b/host/rootfs/image/usr/bin/cgroup-s6-finish >> @@ -0,0 +1 @@ >> +cgroup-setup >> \ No newline at end of file >=20 > Usually packages that expect to be invoked via symlinks like this > (coreutils, busybox, execline) install their own symlinks, rather than > expecting systems to create them. I think these would be more > appropriate in a postInstall in tools/cgroup-setup/default.nix. >=20 >> diff --git a/pkgs/default.nix b/pkgs/default.nix >> index 44f7b5ff78cb6b9e755292a6a417d0b627ed3fb0..0a13393164ad5d7f752e63= 0763f3f97166479af5 100644 >> --- a/pkgs/default.nix >> +++ b/pkgs/default.nix >> @@ -51,6 +51,7 @@ let >> driverSupport =3D true; >> }; >> spectrum-router =3D self.callSpectrumPackage ../tools/router {}; >> + spectrum-cgroup-setup =3D self.callSpectrumPackage ../tools/cgrou= p-setup {}; >> xdg-desktop-portal-spectrum-host =3D >> self.callSpectrumPackage ../tools/xdg-desktop-portal-spectrum-h= ost {}; >> =20 >> diff --git a/tools/cgroup-setup/Cargo.lock b/tools/cgroup-setup/Cargo.= lock >> new file mode 100644 >> index 0000000000000000000000000000000000000000..fe967b3aa02c296c87b6b3= 6ac59253dbe0a32de9 >> --- /dev/null >> +++ b/tools/cgroup-setup/Cargo.lock >> @@ -0,0 +1,67 @@ >> +# This file is automatically @generated by Cargo. >> +# It is not intended for manual editing. >> +version =3D 4 >> + >> +[[package]] >> +name =3D "bitflags" >> +version =3D "2.11.1" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "c4512299f36f043ab09a583e57bceb5a5aab7a73db1805848e8fef3= c9e8c78b3" >> + >> +[[package]] >> +name =3D "cgroup-setup" >> +version =3D "0.0.0" >> +dependencies =3D [ >> + "libc", >> + "rustix", >> +] >> + >> +[[package]] >> +name =3D "errno" >> +version =3D "0.3.14" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d0= 42b63efeb" >> +dependencies =3D [ >> + "libc", >> + "windows-sys", >> +] >> + >> +[[package]] >> +name =3D "libc" >> +version =3D "0.2.186" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61e= ce587de66" >> + >> +[[package]] >> +name =3D "linux-raw-sys" >> +version =3D "0.12.1" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769= c25d58a53" >> + >> +[[package]] >> +name =3D "rustix" >> +version =3D "1.1.4" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e= 9930ba190" >> +dependencies =3D [ >> + "bitflags", >> + "errno", >> + "libc", >> + "linux-raw-sys", >> + "windows-sys", >> +] >> + >> +[[package]] >> +name =3D "windows-link" >> +version =3D "0.2.1" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e= 70d0a4fe5" >> + >> +[[package]] >> +name =3D "windows-sys" >> +version =3D "0.61.2" >> +source =3D "registry+https://github.com/rust-lang/crates.io-index" >> +checksum =3D "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb= 3921ff3fc" >> +dependencies =3D [ >> + "windows-link", >> +] >> diff --git a/tools/cgroup-setup/Cargo.lock.license b/tools/cgroup-setu= p/Cargo.lock.license >> new file mode 100644 >> index 0000000000000000000000000000000000000000..aa108acd23886d8302eaf7= babff90d1b08ae19fb >> --- /dev/null >> +++ b/tools/cgroup-setup/Cargo.lock.license >> @@ -0,0 +1,2 @@ >> +SPDX-License-Identifier: EUPL-1.2+ >> +SPDX-FileCopyrightText: 2026 Demi Marie Obenour >=20 > I think this should be CC0-1.0 like every other Cargo.lock.license. > There's nothing copyrightable about it. Will fix in v5. >> diff --git a/tools/cgroup-setup/Cargo.toml b/tools/cgroup-setup/Cargo.= toml >> new file mode 100644 >> index 0000000000000000000000000000000000000000..7ed6d6a0ea3bbfc4064b9f= 39383d0788c4bd84e5 >> --- /dev/null >> +++ b/tools/cgroup-setup/Cargo.toml >> @@ -0,0 +1,11 @@ >> +# SPDX-License-Identifier: CC0-1.0 >> +# SPDX-FileCopyrightText: 2025 Alyssa Ross >=20 > I surely did not contribute anything copyrightable to this. Okay, I'll remove this in v5. >> +# SPDX-FileCopyrightText: 2026 Demi Marie Obenour >> + >> +[package] >> +name =3D "cgroup-setup" >> +edition =3D "2024" >> + >> +[dependencies] >> +libc =3D "0.2.177" >> +rustix =3D { version =3D "1.1.2", features =3D ["fs"] } >=20 >> diff --git a/tools/cgroup-setup/src/cgroup.rs b/tools/cgroup-setup/src= /cgroup.rs >> new file mode 100644 >> index 0000000000000000000000000000000000000000..c953d26badfdac0a1e3d70= 57a867aec3b3247e18 >> --- /dev/null >> +++ b/tools/cgroup-setup/src/cgroup.rs >> @@ -0,0 +1,349 @@ >> +// SPDX-License-Identifier: EUPL-1.2+ >> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour >> + >> +use std::ffi::OsStr; >> +use std::fmt::Display; >> +use std::fs::File; >> +use std::io::{Read as _, Seek as _, Write as _}; >> +use std::os::unix::prelude::*; >> + >> +use std::path::{Component, Path, PathBuf}; >> + >> +use rustix::fs::{AtFlags, FlockOperation, XattrFlags}; >> +use rustix::{ >> + fs::{Mode, OFlags, ResolveFlags}, >> + io::Errno, >> +}; >> + >> +#[derive(Debug)] >> +pub(crate) struct Cgroup { >> + path: PathBuf, >> + fd: Vec<(OwnedFd, bool)>, >=20 > There's no point storing all these exclusivity bools, is there? I thin= k > only the last one is ever checked, so we could make things tighter and > clearer like this, where we only track the exclusivity of the last fd: Cgroup::enable_subtree_control() checks the exclusivity of the caller-provided depth. Line 232 of main.rs calls enable_subtree_control(2). These bools are only used in assertions, so they could be removed. I will leave that up to you. The advantage of keeping them is that a panic is vastly easier to debug than a race condition due to improper locking. > fd: Vec, > exclusive: bool, >=20 >> +} >> + >> +impl AsFd for Cgroup { >> + fn as_fd(&self) -> BorrowedFd<'_> { >> + self.fd.last().unwrap().0.as_fd() >> + } >> +} >> + >> +fn assert_single_component(component: &[u8]) { >=20 > Why not &Path, which is already guaranteed not to have a NUL byte? > Perhaps this whole thing could be simplified to > Some(component).as_os_str() =3D=3D component.file_name()? Maybe that's= too > clever, though=E2=80=A6 Path isn't actually guaranteed to not have a NUL byte. File::open(Path::new("\0")) fails with InvalidInput rather than panicking. I'm very used to writing this kind of code in C, so I went with a C-like style instead of using Rust stdlib APIs. I don't like having extra abstractions in this kind of code, as it obscures what is going on under the hood. That is less important here, but it's very important in programs like mount-flatpak. For instance, RESOLVE_BENEATH or RESOLVE_IN_ROOT can result in spurious EAGAIN errors, which libpathrs resolves using a retry loop that has a failure rate of about 0.1% when the system is being hammered by repeatedly calling rename(). This is needed for container runtimes that need to follow symlinks in their target filesystems, but mount-flatpak doesn't need to do that when traversing OSTree repositories. For mount-flatpak, an explicit check that the path doesn't have "." or ".." components, and using RESOLVE_NO_SYMLINKS | RESOLVE_NO_MAGICLINKS is just as secure and lacks this problem. >> + match component { >> + b"" | b"." | b".." =3D> panic!("bad component"), >> + _ if component.contains(&b'\0') =3D> panic!("NUL in component= "), >> + _ if component.contains(&b'/') =3D> panic!("/ in component"),= >> + _ =3D> {} >> + } >> +} >> + >> +impl Cgroup { >> + pub fn new(exclusive: bool) -> Result { >> + let cgroup_root =3D rustix::fs::openat2( >> + rustix::fs::CWD, >> + Path::new("/sys/fs/cgroup"), >> + OFlags::DIRECTORY | OFlags::RDONLY | OFlags::CLOEXEC | OF= lags::NOFOLLOW, >> + Mode::empty(), >> + ResolveFlags::NO_MAGICLINKS | ResolveFlags::NO_SYMLINKS, >> + ) >=20 > I pointed out in my review of v2 that OFlags::NOFOLLOW is redundant wit= h > ResolveFlags::NO_SYMLINKS, but now it seesm to have come back across th= e > board. Whoops, sorry about that. Will fix in v5. >> + .map_err(|e| format!("Cannot open /sys/fs/cgroup: {e}"))?; >> + >> + let lock_operation =3D if exclusive { >> + FlockOperation::LockExclusive >> + } else { >> + FlockOperation::LockShared >> + }; >> + rustix::fs::flock(cgroup_root.as_fd(), lock_operation) >> + .map_err(|e| format!("Cannot lock /sys/fs/cgroup: {e}"))?= ; >> + Ok(Self { >> + path: PathBuf::from("/sys/fs/cgroup"), >> + fd: vec![(cgroup_root, exclusive)], >> + }) >> + } >> + >> + pub fn enable_delegation(&self, depth: usize) -> Result<(), Errno= > { >> + let (fd, exclusive) =3D &self.fd[self.fd.len() - depth]; >> + assert!(exclusive); >> + rustix::fs::fsetxattr(fd.as_fd(), c"user.delegate", b"1", Xat= trFlags::empty()) >> + } >> + >> + pub fn enable_subtree_control(&self, depth: usize) -> Result<(), = String> { >> + let (fd, exclusive) =3D &self.fd[self.fd.len() - depth]; >> + assert!(exclusive); >> + let p =3D Path::new("cgroup.controllers"); >> + let mut buf =3D self.read_control_file(fd.as_fd(), p)?; >> + let mut subtree =3D vec![]; >> + if buf.ends_with(b"\n") { >> + buf.pop(); >> + } >> + for controller in buf.split(|&b| b =3D=3D b' ').filter(|e| !e= =2Eis_empty()) { >> + for &c in controller { >> + if c <=3D b' ' || c >=3D 0x7F { >> + return Err(format!("Bad byte {c} in cgroup.contro= llers")); >> + } >> + } >> + if !subtree.is_empty() { >> + subtree.push(b' '); >> + } >> + subtree.push(b'+'); >> + subtree.extend_from_slice(controller); >> + } >> + if !subtree.is_empty() { >> + self.write_cgroup_value("cgroup.subtree_control", str::fr= om_utf8(&subtree).unwrap())?; >> + } >> + Ok(()) >> + } >> + >> + pub fn read_control_file(&self, fd: BorrowedFd, p: &Path) -> Resu= lt, String> { >> + let mut buf =3D Vec::new(); >> + let err =3D |e: &dyn Display, p: &Path, msg: &str| { >> + let path =3D self.path.join(p); >> + format!("Cannot {msg} {path:?}: {e}") >> + }; >> + File::from(open_subtree_raw(Path::new(p), fd.as_fd()).map_err= (|e| err(&e, p, "open"))?) >=20 > If we're using it for opening files, open_subtree_raw is probably misna= med. Yup! Do you have a suggestion for improving it? Maybe open_child()? >> + .read_to_end(&mut buf) >> + .map_err(|e| err(&e, p, "read"))?; >> + Ok(buf) >> + } >> + >> + /// Open a single component as a sub-cgroup >> + fn open_sub_cgroup_raw(&self, access: OFlags, component: &[u8]) -= > Result { >> + assert_single_component(component); >> + rustix::fs::openat2( >> + self.as_fd(), >> + Path::new(OsStr::from_bytes(component)), >> + OFlags::CLOEXEC | OFlags::NOFOLLOW | access, >> + Mode::empty(), >> + ResolveFlags::NO_SYMLINKS >> + | ResolveFlags::NO_MAGICLINKS >> + | ResolveFlags::BENEATH >> + | ResolveFlags::NO_XDEV, >=20 > This is also doing exactly the same thing as open_subtree_raw, except i= t > allows changing the access mode, and also sets NO_MAGICLINKS. I don't > think any of the other callers of open_subtree_raw would need to open > magic links, so maybe this is evidence this should be unified with them= ? It probably should. >> + ) >> + } >> + >> + pub fn open_sub_cgroup( >> + &mut self, >> + path: &std::path::Path, >> + exclusive: bool, >> + allow_missing: bool, >> + ) -> Result { >> + let mut iter =3D path.components().peekable(); >> + while let Some(component) =3D iter.next() { >=20 > Perhaps would be nicer: >=20 > let mut components =3D path.components().peekable(); > for component in components { That results in a borrowcheck error. The for loop takes ownership of the iterator, but .peek() is called inside the loop. >> + let component =3D match component { >> + Component::Normal(component) =3D> component, >> + _ =3D> unreachable!(), >> + }; >> + let sub_fd =3D match self >> + .open_sub_cgroup_raw(OFlags::DIRECTORY | OFlags::RDON= LY, component.as_bytes()) >> + { >> + Ok(sub_fd) =3D> { >> + self.path.push(component); >=20 > I would really like to not try to store self.path. It seems very > complicated to track. It's also very unclear to me from the name (and > the code) what it is. Is it the path to the cgroup itself, or to its > parent? It looks to me like it should be the cgroup itself, but then > what's going on in purge? It's the path to the cgroup itself, relative to /sys/fs/cgroup. Its only= purpose is for logging. > We could actually improve readability of this quite complicated functio= n > even further if you find it acceptable to just use Errno for the error > type. In that case, we'd just return Result<(), Errno>, and callers > would check for Errno::NOENT if they wanted to allow missing. Then we > could just completely drop that argument. In my opinion it would be > worth it to move complexity out of here. I can do this, but it would result in much worse error messages: the error would only reference the file name, not the full cgroup path. Which would you prefer? >> + sub_fd >> + } >> + Err(Errno::NOENT) if allow_missing =3D> return Ok(fal= se), >> + Err(e) =3D> { >> + return Err(format!( >> + "Cannot open sub-cgroup {component:?} of {:?}= : {e}", >> + self.path >> + )); >> + } >> + }; >> + let exclusive =3D exclusive && iter.peek().is_none(); >> + let lock_operation =3D if exclusive { >> + FlockOperation::LockExclusive >> + } else { >> + FlockOperation::LockShared >> + }; >> + rustix::fs::flock(sub_fd.as_fd(), lock_operation).map_err= (|e| { >> + let msg =3D format!("Cannot lock sub-cgroup {:?}: {e}= ", self.path); >> + self.path.pop(); >> + msg >> + })?; >> + self.fd.push((sub_fd, exclusive)); >> + } >> + Ok(true) >> + } >> + >> + pub fn open_subtree(&self, path: &std::path::Path) -> Result { >> + let dirfd =3D self.as_fd(); >> + open_subtree_raw(path, dirfd) >> + } >=20 > If open_subtree_raw just took &dyn AsFd, there'd be no need for this > method. Nice catch! Will change in v5. >> + >> + fn exclusive(&self) -> bool { >> + self.fd.last().unwrap().1 >> + } >> + >> + pub fn joined_path(&self, p: &Path) -> PathBuf { >> + let mut owned_p =3D self.path.clone(); >> + owned_p.push(p); >> + owned_p >> + } >> + >> + pub fn wait_for_empty(&self) -> std::io::Result<()> { >> + assert!(self.exclusive()); >> + let wait_file =3D self.open_subtree(std::path::Path::new("cgr= oup.events"))?; >> + let poll_fd =3D wait_file.as_raw_fd(); >> + let mut wait_fd =3D File::from(wait_file); >> + let mut fds =3D libc::pollfd { >> + fd: poll_fd, >> + events: libc::POLLPRI | libc::POLLERR, >> + revents: 0, >> + }; >> + let mut v =3D vec![]; >> + loop { >> + v.clear(); >> + wait_fd >> + .seek(std::io::SeekFrom::Start(0)) >> + .expect("Seek on control group file should succeed");= >> + wait_fd >> + .read_to_end(&mut v) >> + .expect("reading from control group should work"); >> + if v.split(|&c| c =3D=3D b'\n').any(|line| line =3D=3D b"= populated 0") { >> + break; >> + } >> + // SAFETY: FFI call, valid arguments, fds contains 1 elem= ent >> + if unsafe { libc::poll(&raw mut fds, 1, -1) } !=3D 1 { >> + panic!("poll failed"); >> + } >> + } >=20 > Are you 100% confident that this doesn't race? I don't understand why > poll would be triggered in this scenario: >=20 > 1. "1" is written to cgroup.kill > 2. Every process in the cgroup exits and is reaped. > 3. cgroup.events is opened, with the cgroup already empty. >=20 > Are you not relying on 2 happening after 3? Presumably if you open > cgroup.events for a cgroup that's already empty, you're not going to ge= t > a poll event to tell you it's empty. In that case, cgroup.events will include a "populated 0" line, so poll will not be called. >> + Ok(()) >> + } >> + >> + pub(crate) fn make_child(&mut self, path: &Path) -> Result<(), Er= rno> { >> + assert!(self.exclusive()); >> + let component =3D path.as_os_str().as_bytes(); >> + assert_single_component(component); >> + match rustix::fs::mkdirat( >> + self.as_fd(), >> + path, >> + Mode::RUSR >> + | Mode::WUSR >> + | Mode::XUSR >> + | Mode::RGRP >> + | Mode::XGRP >> + | Mode::ROTH >> + | Mode::XOTH, >> + ) { >> + Ok(()) | Err(Errno::EXIST) =3D> {} >> + bad =3D> return bad, >> + } >> + let p =3D self.open_sub_cgroup_raw(OFlags::RDONLY | OFlags::D= IRECTORY, component)?; >> + // exclusive lock on parent acts as exclusive lock on child >> + self.fd.push((p, true)); >> + self.path.push(path); >> + Ok(()) >> + } >> + >> + pub(super) fn purge(&mut self, path: &Path) -> Result<(), String>= { >=20 > I guess we have to call purge on the parent, rather than on the cgroup > itself, because of the unlink? Maybe we could call it purge_child? It= > confused me for a while. Correct. Will rename in v5. The way to understand this code is that Cgroup has two stacks: one for file descriptors and one for path components. All operations operate at a specified depth from the top of the stack. 1 refers to the top of the stack, 2 to one level below that, and so on. This function is really confusing because it performs multiple pushes and pops on the internal file descriptor stack. The specific algorithm i= s: 1. Start with an exclusive lock. 2. Try to delete the child directly. 3. If deletion succeeds, or if it fails with ENOENT, return success. 4. If deletion fails with anything other than EBUSY, return an error. 5. Open the child cgroup and take an exclusive lock on it. This pushes the child cgroup's FD onto the stack. The open_subtree() method also pushes the child path onto the stack. 6. Take a *shared* lock on the FD that is directly below the top of the stack. This is the file descriptor that was initially on the top of the stack. This releases the exclusive lock, allowing other operations on different children to proceed. Different operations on the cgroup being purged will be blocked by the exclusive lock taken in step 5. 7. Kill all programs in the cgroup by writing 1 to cgroup.kill. 8. Open cgroup.events. 9. Read from the FD opened in step 8. If the file contains the line "populated 0", go to step 11. 10. Call poll() on the FD opened in step 8 to wait for POLLERR or POLLPRI to happen. Then go back to step 9. This is race-free because the kernel will set the "this is ready" flag after every change that affects what would be read from the file. 11. Pop the file descriptor to the being-purged cgroup from the stack. 12. Use the just-popped file descriptor to remove all subdirectories recursively. Files must not be deleted, as the kernel doesn't allow it. Then close the file descriptor, releasing the exclusive lock held on it. =20 13. Take an exclusive lock on the *parent* of the cgroup that was just pu= rged. This must be done after the file descriptor to the cgroup being purged has been closed. Otherwise, there is the potential for an ABBA deadlock: another program might hold a shared lock on the parent, and be waiting to get an exclusive lock on the child. 14. Delete the being-purged cgroup. Treat EBUSY and ENOENT as success: the first means that a concurrently-running program re-created the cgroup, while the second means that a concurrently-running program deleted it. The name of the cgroup being purged is currently at the top of the path stack. 15. Pop the name of the cgroup being purged off of the stack. =20 At the end, self is in the same state it was before the operation. If you are complaining that this is about as readable as Forth, then I agree with you :). =20 >> + assert!(self.exclusive());>> + match rustix::fs::unlin= kat(self.as_fd(), Path::new(path), AtFlags::REMOVEDIR) { >> + // Trying to purge a deleted cgroup is not an error. >> + Ok(()) | Err(Errno::NOENT) =3D> return Ok(()), >> + Err(Errno::BUSY) =3D> {} >=20 > This could use a comment. Will add in v5. >> + Err(e) =3D> return Err(format!("Cannot purge {:?}: {e}", = self.joined_path(path))), >> + } >> + if !self.open_sub_cgroup(path, true, true)? { >> + return Ok(()); >> + } >> + >> + rustix::fs::flock( >> + self.fd[self.fd.len() - 2].0.as_fd(), >> + FlockOperation::LockShared, >=20 > We already must have at least a shared lock on this at this point, no? > I don't think we need another one. We actually have an exclusive lock. If it succeeds, Cgroup::open_sub_cgroup() pushes a file descriptor onto self.fd. Therefore, the fd being locked here is the one that was initially on the top of the stack. We assert that an exclusive lock is held on that FD. Waiting for the control group to become empty is a blocking operation, so this downgrades the lock to a shared one. Otherwise, an in-progress purge of /a/b would prevent /a/c from being created. >> + ) >> + .map_err(|e| format!("Cannot relock {:?}: {e}", self.path.par= ent()))?; >> + self.write_cgroup_value("cgroup.kill", "1")?; >> + self.wait_for_empty() >> + .map_err(|e| format!("Cannot wait for cgroup to become em= pty: {e}"))?; >> + let fd =3D self.fd.pop().unwrap().0; >> + let v =3D (|| { >> + remove_recursively(fd, 1000) >> + .map_err(|e| format!("Cannot remove {:?}: {e}", self.= path))?; >> + rustix::fs::flock(self.as_fd(), FlockOperation::LockExclu= sive) >> + .map_err(|e| format!("Cannot lock {:?}: {e}", self.pa= th))?; >=20 > We already checked self.exclusive above, meaning we already have this > lock on self? self.fd.pop() removes the FD from the stack, and remove_recursively close= s it. self.fd() returns the FD whose lock was downgraded to a shared one above.= >> + match rustix::fs::unlinkat( >> + self.as_fd(), >> + Path::new(self.path.file_name().unwrap()), >=20 > I am too confused about what self.path is to know what to make of this.= > self.as_fd() should be the fd of the cgroup directory, and self.path > sounds like it should be the path to this cgroup, so how can this cgrou= p > directory have self.path.file_name() within it? This needs clearer > names or a refactor or something. self.open_sub_cgroup() pushes both the path and the FD, but self.fd.pop() only pops the FD. This means that self.fd() is currently the path to the *parent* of the cgroup. >> + AtFlags::REMOVEDIR, >> + ) { >> + // something might have re-created the cgroup in the = meantime, which is okay >> + Ok(()) | Err(Errno::BUSY) =3D> Ok(()), >> + Err(e) =3D> Err(format!("Cannot lock {:?}: {e}", self= =2Epath)), >> + } >> + })(); >> + assert!(self.path.pop()); >=20 > I don't get this. Where was it pushed? (But would prefer to just not > attempt to track this, as mentioned above.) It was pushed in self.open_sub_cgroup(). >> + v >> + } >> + >> + pub(crate) fn write_cgroup_value(&self, name: &str, value: &str) = -> Result<(), String> { >> + let path =3D Path::new(name); >> + let fd =3D rustix::fs::openat2( >> + self.as_fd(), >> + path, >> + OFlags::NOATIME | OFlags::CLOEXEC | OFlags::NOFOLLOW | OF= lags::WRONLY, >> + Mode::empty(), >> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | Resol= veFlags::NO_XDEV, >> + ) >> + .map_err(|e| format!("Cannot open {:?}: {}", self.joined_path= (Path::new(name)), e))?; >=20 > Could also use ResolveFlags::MAGIC_LINKS and be unified with the other > openat2 invocations maybe? I don't understand why this is NOATIME but > others aren't. Yes, it indeed should be. >> + File::from(fd).write_all(value.as_bytes()).map_err(|e| { >> + format!( >> + "Cannot write {:?} to {:?}: {}", >> + value, >> + self.joined_path(Path::new(name)), >> + e >> + ) >> + }) >> + } >> +} >> + >> +fn open_subtree_raw(path: &Path, dirfd: BorrowedFd<'_>) -> Result { >> + rustix::fs::openat2( >> + dirfd, >> + path, >> + OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY, >> + Mode::empty(), >> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFl= ags::NO_XDEV, >> + ) >> +} >> + >> +fn remove_recursively(fd: OwnedFd, remaining_depth: usize) -> Result<= (), Errno> { >> + if remaining_depth < 1 { >> + panic!("control groups too deeply nested"); >> + } >> + let mut d =3D rustix::fs::Dir::new(fd).expect("cannot start itera= ting"); >> + while let Some(element) =3D d.next() { >> + let element =3D element.expect("Iterating through a cgroup di= rectory failed?"); >> + if element.file_type() !=3D rustix::fs::FileType::Directory {= >> + continue; >> + } >> + >> + let remaining_depth =3D remaining_depth - 1; >> + let d: &rustix::fs::Dir =3D &d; >> + let dirfd =3D d.fd().unwrap(); >> + let path =3D element.file_name(); >> + remove_all(remaining_depth, dirfd, path)?; >> + } >> + drop(d); >> + Ok(()) >> +} >> + >> +fn remove_all( >> + remaining_depth: usize, >> + dirfd: BorrowedFd<'_>, >> + path: &std::ffi::CStr, >> +) -> Result<(), Errno> { >> + if path =3D=3D c"." || path =3D=3D c".." { >> + return Ok(()); >> + } >> + if rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR).is_ok() = { >> + return Ok(()); >> + } >> + let fd =3D rustix::fs::openat2( >> + dirfd, >> + path, >> + OFlags::CLOEXEC | OFlags::NOFOLLOW | OFlags::RDONLY | OFlags:= :DIRECTORY, >> + Mode::empty(), >> + ResolveFlags::NO_SYMLINKS | ResolveFlags::BENEATH | ResolveFl= ags::NO_XDEV, >> + )?; >> + remove_recursively(fd, remaining_depth)?; >> + rustix::fs::unlinkat(dirfd, path, AtFlags::REMOVEDIR)?; >> + Ok(()) >> +} >=20 > Could we save a lot of code by calling std::fs::remove_dir_all with a > /proc/self/fd path? It's already documented to ignore symlinks. I tried, but that tries to delete files too, and that isn't allowed. >> diff --git a/tools/cgroup-setup/src/main.rs b/tools/cgroup-setup/src/m= ain.rs >> new file mode 100644 >> index 0000000000000000000000000000000000000000..2e7a4e25213a4aa2449b29= 2f8005e1da23cbb1f9 >> --- /dev/null >> +++ b/tools/cgroup-setup/src/main.rs >> @@ -0,0 +1,347 @@ >> +// SPDX-License-Identifier: EUPL-1.2+ >> +// SPDX-FileCopyrightText: 2026 Demi Marie Obenour >> + >> +use std::{ >> + ffi::{OsStr, OsString}, >> + os::unix::prelude::*, >> + path::{Path, PathBuf}, >> +}; >> + >> +use crate::cgroup::Cgroup; >> + >> +mod cgroup; >> + >> +fn check_path(path: &OsStr) -> Result<(), String> { >> + if path.is_empty() { >> + return Ok(()); >> + } >> + >> + for component in path.as_bytes().split(|&b| b =3D=3D b'/') { >=20 > Would be a lot nicer to take &Path and use Path::components. Will change in v5. I'm way more used to doing this in C. >> + match component { >> + b"" | b"." | b".." =3D> { >> + return Err(format!("Path {path:?} has empty, ., or ..= component")); >> + } >> + // Cannot happen: command line arguments have no NUL byte= , >> + // and /proc/self/cgroup having a NUL byte is a kernel bu= g. >> + _ if component.contains(&b'\0') =3D> panic!("Path {path:?= } has NUL byte"), >=20 > It is in fact an invariant of both the Path and OsStr types (on Unix) > that there are no NUL bytes, so there is no need to check for it at all= =2E It actually isn't. OsStr::from_bytes("\0") doesn't panic. >> + _ if component.len() > 255 =3D> { >> + return Err(format!( >> + "Path {path:?} has component {:?} that is longer = than 255 bytes", >> + OsStr::from_bytes(component) >> + )); >> + } >=20 > Why on earth would we need to check for this? This limit is totally up= > to the kernel. Our code won't break with an excessively long path > component. It provides better error messages than ENAMETOOLONG, but I will remove it= in v5. >> + _ =3D> {} >> + } >> + } >> + >> + Ok(()) >> +} >> + >> +/// Get the path of the cgroup for the provided command-line argument= =2E >> +/// Returns an empty path if the path is "/", or if it is "." and the= >> +/// current cgroup is "/". >=20 > Please standardize terminology between "current cgroup" and "local > cgroup", or if those are not the same thing use clearer phrasing. >=20 > I hope all the different modes of this function are actually needed=E2=80= =A6 > (I assume they are but haven't reviewed the patches that make use of it= > yet.) All of them are indeed used. >> +/// >> +/// # Errors >> +/// >> +/// Fails if the provided path is invalid or empty, or if it is relat= ive >> +/// and the local cgroup cannot be determined. >> +fn get_cgroup(cgroup_path: OsString) -> Result { >=20 > Please call this function something clearer. Also using OsString where= > PathBuf would be more appropriate again=E2=80=A6 Fair! Will fix in v5. >> + if cgroup_path.as_bytes().starts_with(b"/") { >=20 > =E2=80=A6 which would allow using Path::is_absolute if fixed. Will fix in v5. >> + let mut cgroup_path =3D cgroup_path.into_vec(); >> + cgroup_path.remove(0); >=20 > Please use Path functions rather than byte manipulation like this. Will change in v5. >> + if cgroup_path.is_empty() { >> + return Err("cgroup path cannot be /".to_owned()); >> + } >> + let cgroup_path =3D OsString::from_vec(cgroup_path); >> + check_path(&cgroup_path)?; >> + Ok(cgroup_path.into()) >> + } else if cgroup_path.is_empty() { >> + Err("cgroup path cannot be empty".to_owned()) >> + } else { >> + check_path(&cgroup_path)?; >> + let mut local_cgroup =3D local_cgroup()?; >=20 > Can you reorder all these function definitions to be something more > sensible? get_cgroup is here, near the start of main.rs, but > local_cgroup, which it is the only direct caller of, is all the way at > the other end. (Personally I like to define utility functions directly= > above their only caller =E2=80=94 I think that creates the most natural= reading > flow.) Will fix in v5. >> + local_cgroup.push(cgroup_path); >> + Ok(local_cgroup) >> + } >> +} >> + >> +/// Open the cgroup corresponding to the provided path. >> +/// It must have already been made relative to `/sys/fs/cgroup`. >> +/// >> +/// # Errors >> +/// >> +/// Fails if the cgroup operation fails. >=20 > This "Errors" section is just stating the obvious. Will remove in v5. >> +fn open_cgroup(path: &Path, exclusive: bool) -> Result { >> + if path.as_os_str().is_empty() { >> + Cgroup::new(exclusive) >> + } else { >> + let mut cgroup =3D Cgroup::new(false)?; >> + cgroup.open_sub_cgroup(path, exclusive, false)?; >> + Ok(cgroup) >> + } >> +} >=20 > I feel like it would be more natural if Cgroup::new just took two > parameters and worked this way. Will change in v5. >> + >> +/// Open the cgroup corresponding to the provided path's parent. >> +/// It is made relative to the process's own cgroup if needed. >> +/// >> +/// # Errors >> +/// >> +/// Fails if the cgroup operation fails. >> +fn open_relative_cgroup(arg: OsString) -> Result<(PathBuf, Cgroup), S= tring> { >> + let path =3D get_cgroup(arg)?; >> + let cgroup =3D open_cgroup(path.parent().expect("always has a par= ent"), true)?; >> + Ok((path, cgroup)) >> +} >=20 > This is a deeply confusing function. Why is opening a cgroup for the > path's _parent_ an operation that we should have a dedicated function > for? Why not have the caller pass in the actual path of the cgroup it > wants to open? This really looks like a function that's doing as many > different things at it has lines, and should just be inlined. Will inline in v5. >> + >> +fn main() { >> + let mut args =3D std::env::args_os(); >> + let Some(prog_name) =3D args.next() else { >> + eprintln!("No command line arguments (argv[0] is NULL)"); >> + std::process::exit(1); >> + }; >> + match main_(&prog_name, args) { >> + Ok(()) =3D> {} >> + Err(e) =3D> { >> + eprintln!("{prog_name:?}: {}", e); >> + std::process::exit(1); >> + } >> + } >> +} >> + >> +fn main_(prog_name: &OsStr, mut args: std::env::ArgsOs) -> Result<(),= String> { >=20 > main_ is a bit confusingly similar to name. I like "run" for this sort= > of function, and we use that elsewhere. Will fix in v5. >> + match prog_name >> + .as_bytes() >> + .split(|&b| b =3D=3D b'/') >> + .next_back() >> + .unwrap() >=20 > Another place that should use Path. Will fix in v5. >> + { >> + b"cgroup-s6-finish" =3D> { >> + return s6_finish(&mut args); >> + } >> + b"cgroup-setup" =3D> {} >=20 > It is inconsistent for this not to also be its own function. Then the > match could just be an expression that returned the result of the > appropriate function. Will fix in v5. >> + b"cgroup-purge" =3D> { >> + if args.len() !=3D 1 { >> + return Err(format!( >> + "cgroup-purge takes one argument, got {}", >> + args.len() >> + )); >> + } >> + let cgroup_path =3D args.next().unwrap(); >> + let (path, mut cgroup) =3D open_relative_cgroup(cgroup_pa= th)?; >> + let cgroup_target =3D Path::new(path.file_name().unwrap()= ); >> + return cgroup.purge(cgroup_target); >> + } >> + _ =3D> { >> + return Err(format!( >> + "must be invoked as \"cgroup-setup\" \ >> + \"cgroup-purge\", or \"cgroup-s6-finish\", \ >> + got {prog_name:?}", >> + )); >> + } >> + }; >> + let mut leaf =3D false; >> + let mut cgroup_path; >> + let mut delegate =3D false; >> + let mut init_subtree =3D false; >> + let mut child_name: Option<&'static OsStr> =3D None; >> + let mut wait =3D true; >> + loop { >> + cgroup_path =3D args.next(); >> + let Some(ref arg_) =3D cgroup_path else { >> + break; >> + }; >> + let arg_ =3D arg_.as_bytes(); >> + if arg_ =3D=3D b"--" { >> + cgroup_path =3D args.next(); >> + break; >> + } >> + if !arg_.starts_with(b"-") { >> + break; >> + } >> + >> + if !arg_.starts_with(b"--") { >> + return Err("takes no short options".to_owned()); >> + } >> + >> + match &arg_[2..] { >> + b"leaf" =3D> leaf =3D true, >> + b"delegate" =3D> delegate =3D true, >> + b"init-subtree" =3D> init_subtree =3D true, >> + b"wait" =3D> wait =3D true, >> + b"no-wait" =3D> wait =3D false, >> + b"child-name" if child_name.is_none() =3D> match args.nex= t() { >> + Some(arg) =3D> child_name =3D Some(arg.leak()), >> + None =3D> return Err("--child-name: missing argument"= =2Eto_owned()), >> + }, >> + b"child-name" =3D> return Err("--child-name: cannot be us= ed twice".to_owned()), >> + arg =3D> match str::from_utf8(arg) { >> + Ok(e) =3D> return Err(format!("unknown long option {e= :?}")), >> + Err(_) =3D> return Err("long option isn't UTF-8".to_o= wned()), >> + }, >> + } >> + } >> + >> + let default_child_name =3D OsStr::from_bytes(b"$inner.service"); >> + >> + let child_name =3D Path::new(child_name.unwrap_or(default_child_n= ame)); >> + >> + let Some(mut cgroup_path) =3D cgroup_path else { >> + return Err("have no positional arguments, expected at least 1= ".to_owned()); >> + }; >> + >> + // Allow --init-subtree . >> + if cgroup_path.as_bytes() =3D=3D b"." && init_subtree && !leaf { >> + cgroup_path =3D child_name.to_owned().into(); >> + leaf =3D true; >> + } >> + >> + let (path, mut cgroup) =3D open_relative_cgroup(cgroup_path)?; >> + let cgroup_target =3D Path::new(path.file_name().unwrap()); >> + cgroup >> + .make_child(cgroup_target) >> + .map_err(|e| format!("Cannot make child cgroup: {e}"))?; >> + if wait { >> + cgroup >> + .wait_for_empty() >> + .map_err(|e| format!("Cannot wait for {path:?} to be empt= y: {e}"))?; >> + } >> + let pid =3D std::process::id().to_string(); >> + if leaf { >> + if args.len() !=3D 0 { >> + // If we aren't delegating any cgroups, don't create a su= b-cgroup. >> + cgroup >> + .write_cgroup_value("cgroup.procs", &pid) >> + .map_err(|e| format!("Cannot write to {path:?}/cgroup= =2Eprocs: {e}"))?; >> + } >> + } else { >> + // If the child process will need to manage cgroups itself, i= t will need >> + // to set up a sub-cgroup due to the "no internal processes" = rule. It's >> + // simplest to just do it automatically. >> + cgroup.make_child(Path::new(child_name)).map_err(|e| { >> + format!( >> + "Cannot create child cgroup {}/{}: {e}", >> + path.display(), >> + child_name.display() >> + ) >> + })?; >> + if args.len() !=3D 0 { >> + cgroup >> + .write_cgroup_value("cgroup.procs", &pid) >> + .map_err(|e| { >> + format!( >> + "Cannot write to {}/{}/cgroup.procs: {e}", >> + path.display(), >> + child_name.display() >> + ) >> + })?; >> + } >> + } >> + if !leaf { >> + cgroup.enable_subtree_control(2)?; >> + } >> + if init_subtree { >> + cgroup.enable_subtree_control(1)?; >> + } >> + if delegate { >> + cgroup >> + .enable_delegation(1) >> + .map_err(|e| format!("Cannot enable cgroup delegation in = {path:?}: {e}"))?; >> + } >> + let Some(program_name) =3D args.next() else { >> + return Ok(()); >> + }; >> + let e =3D std::process::Command::new(&program_name).args(args).ex= ec(); >> + Err(format!("Cannot spawn child {:?}: {}", program_name, e)) >> +} >> + >> +fn s6_finish(args: &mut std::env::ArgsOs) -> Result<(), String> { >> + if args.len() < 3 { >> + return Err(format!( >> + "s6 finish scripts take at least 3 arguments, got {}", >> + args.len() >> + )); >> + } >> + let status =3D parse_digit_string(&args.next().unwrap(), "exit st= atus")?; >> + let signal =3D args.next().unwrap(); >> + let signal =3D if status =3D=3D 256 { >> + Some(parse_digit_string(&signal, "signal number")?) >> + } else { >> + None >> + }; >> + let service =3D args.next().unwrap(); >> + >> + let (path, mut cgroup) =3D open_relative_cgroup(service)?; >> + let cgroup_target =3D Path::new(path.file_name().unwrap()); >> + let exit_125 =3D if let Some(signal) =3D signal { >> + match signal as libc::c_int { >> + libc::SIGBUS >> + | libc::SIGFPE >> + | libc::SIGABRT >> + | libc::SIGTRAP >> + | libc::SIGSEGV >> + | libc::SIGILL =3D> { >> + // Process *crashed*, indicating a *possible exploit = attempt*. >> + // s6 should *not* restart it. This is distinct from= a Rust panic, >> + // which is much less likely to indicate memory corru= ption. >> + true >> + } >=20 > This has absolutely nothing to do with cgroups. If you want to have > some common finish behaviour, a program called cgroup-setup is not the > place for it. I don't think there's any need for a separate > cgroup-s6-finish mode (as opposed to cgroup-purge). This program is a multi-call binary, so the various things it can do aren't necessarily super tightly related. For instance, all of the execline binaries can be built as one program, as can most if not all busybox applets. When invoked as cgroup-setup or cgroup-purge, it indeed only does cgroup-related tasks. cgroup-s6-finish not only handles cgroups, but also other tasks related to being an s6 finish script. That said, using this changes behavior in a way that isn't related to cgroups, so if it is to be used at all it should be in a separate patch series. I'll remove this from v5. >> + _ =3D> false, >> + } >> + } else { >> + false >> + }; >> + if exit_125 { >> + // Ignore panics. Exit status is more important. >> + // We already had a core dump. >> + let _ =3D std::panic::catch_unwind(std::panic::AssertUnwindSa= fe(|| { >> + match cgroup.purge(cgroup_target) { >> + Ok(()) =3D> {} >> + Err(e) =3D> { >> + eprintln!("cgroup-s6-finish: Failed to purge cgro= up: {e}") >> + } >> + }; >> + })); >> + std::process::exit(125) >> + } else { >> + cgroup.purge(cgroup_target) >> + } >> +} >> + >> +fn parse_digit_string(digits: &OsStr, msg: &str) -> Result { >> + let checked =3D match str::from_utf8(digits.as_bytes()) { >> + Ok(s) =3D> s, >> + Err(e) =3D> return Err(format!("{msg} is not UTF-8: {e}")), >> + }; >> + let r =3D checked >> + .parse::() >> + .map_err(|e| format!("{msg} {digits:?} is a bad 16-bit number= : {e}"))?; >> + match checked.as_bytes() { >> + b"0" | [b'1'..=3Db'9', ..] =3D> Ok(r), >> + [b'0', ..] =3D> Err(format!("{msg} {} has a leading 0", digit= s.display())), >> + _ =3D> Err(format!("{msg} {} starts with +", digits.display()= )), >> + } >=20 > Surely we trust s6 to turn a number into a string. These checks add no= thing. Yes, we can. This is part of cgroup-s6-finish, which will be removed in = v5. >> +} >> + >> +fn local_cgroup() -> Result { >> + let mut local_cgroup: Vec =3D std::fs::read("/proc/thread-sel= f/cgroup") >> + .map_err(|e| format!("cannot read /proc/thread-self/cgroup: {= e}"))?; >> + let local_cgroup_len =3D local_cgroup.len(); >> + if local_cgroup_len < 5 >> + || local_cgroup[..4] !=3D *b"0::/" >> + || local_cgroup[local_cgroup_len - 1] !=3D b'\n' >> + || local_cgroup[4..local_cgroup_len - 1].contains(&b'\n') >=20 > Last time I suggested a clearer way of doing this, but it has instead > got even less clear. >=20 > (I'm not sure why we'd care if there's a newline specifically, as > opposed to any other control character.) If cgroups v1 is in use, the file can contain multiple lines, one for each cgroup the program is in. I also am not sure if starting with "0::/" is an invariant in that case. Using this program with cgroups v1 mounted is user error and will never happen on Spectrum, but if this tool is used outside of Spectrum, it could happen. >> + { >> + // It's possible to get here if the cgroup path contains a ne= wline, >> + // but that never happens in Spectrum. >> + return Err(format!( >> + "Invalid contents {local_cgroup:?} of /proc/thread-self/c= group - \ >> + do you have cgroups v1 mounted instead of cgroups v2?" >> + )); >> + } >> + >> + local_cgroup.copy_within(4..local_cgroup_len - 1, 0); >> + local_cgroup.truncate(local_cgroup_len - 5); >> + let local_cgroup =3D OsString::from_vec(local_cgroup); >> + check_path(&local_cgroup).unwrap(); >=20 > Why do we need to do this? You're worried the kernel is going to start= > including .. components in /proc/thread-self/cgroup? Originally, I was going to create a wrapper around `Path` that guaranteed no `.` or `..` components were present. Its constructor would have checked this invariant. However, this turned out to be more work due to the amount of wrapper functions required. >> + Ok(PathBuf::from(local_cgroup)) >> +} >> >> --=20 >> 2.55.0 --=20 Sincerely, Demi Marie Obenour (she/her/hers) --------------Oah20Kd60CV0uZydWctY8V08-- --------------4yMvcemXzzYetUZJvh0GzikH Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEopQtqVJW1aeuo9/sszaHOrMp8lMFAmpinr4ACgkQszaHOrMp 8lMAHA/+NQCYUd26GBmgPoX3rDpu/Q28qOBRnUj6hzcnEuv6wF5GWAd1tmfOq1ll Q4jtWPoJYbXeox1PXiTRCt1yt/y/30b498WREij1VQ6Dwou5YCq7FGk9P/a29y1g z8j70+ji6VmNBONIoqD1S8QrPY8dP0Y/jl9N5z0tRNNciYUnFyWRIAehZRN4LOeM xLFjDP3hpzgHUfybm3XCOx6n0Hwz8uhhrmBOq1d0D5aJQ8wdOQV+dUu6Dxg9tNja 6UwaSTCrvWf3EIglNxjaOYpfnWj2yDVxkk1HIKRL3pgHRA4VVohKMpC44ShptF2S Vdc2q0aLPzpHtimK9NuBP2gJCRemBfNBHdl6ysLUJzPWrtdFxOHGaehV9PFHAkk6 7bvUxD7PU+y2gglD4sdUy0C3ix/Wkqo0MdVDUAgHleVwIzkgtbbcmjPNBxRmgeHy 6VWz9DRne0jShozxAP5KkxSwQguEuoeyFLMDu8wvYdM8eZthIzOKxViwwZ4WTxFe 7I+wV42wzkpfaVVEt7+FgXmxNNO5KtViBzqIM/ZCIxe4RjiZVG/Uf27yD3NwlFVA fFjzkWI+/AbjLX6i77Xo+cBDKwmZhXDeqzPSFt/CA6iwtBuCP4MbEB6CWL2yOC1n 3l5vEvggMq4Yx1wUEUQSceNTPOkALiceVos9LrA38j3OPx1k2aA= =xjF8 -----END PGP SIGNATURE----- --------------4yMvcemXzzYetUZJvh0GzikH--