[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#950166: buster-pu: package systemd/241-7~deb10u3



Package: release.debian.org
Severity: normal
Tags: buster
User: release.debian.org@packages.debian.org
Usertags: pu

Hi,

first of all, apologies, that this pu request comes rather late.
I first didn't plan to prepare a release for 10.3, but today #930178 was
reassigned to systemd, which reminded me, that this is an issue we
should fix for stable. So here it goes:

I'd like to fix the following two issues in systemd:


systemd (241-7~deb10u3) buster; urgency=medium

  * core: set fs.file-max sysctl to LONG_MAX rather than ULONG_MAX.
    Since kernel 5.2 (but also stable kernels like 4.19.53) the kernel
    thankfully returns proper errors when we write a value out of range to
    the sysctl. Which however breaks writing ULONG_MAX to request the
    maximum value. Hence let's write the new maximum value instead,
    LONG_MAX. (Closes: #945018)

https://salsa.debian.org/systemd-team/systemd/commit/673e108907baf1a242c4842ace6e9e3a23b11d52

Upstream cherry-pick, fixed in unstable/testing. Rather straight-forward
fix. I wasn't planning doing a stable upload for this issue alone but
only in combination with other fixes.

  * core: change ownership/mode of the execution directories also for static
    users.
    This ensures that execution directories like CacheDirectory and
    StateDirectory are properly chowned to the user specified in User= before
    launching the service. (Closes: #919231)

https://salsa.debian.org/systemd-team/systemd/commit/e9c8637d06e373430b8986643cfb537a23b0b1fd

This is an upstream cherry-pick from https://github.com/systemd/systemd/pull/12005
I'm a bit undecided whether to cherry-pick all changes from this PR
(which look like worthwile changes to have) or only commit
206e9864de460dd79d9edd7bedb47dee168765e1.

I decided for the latter for now, as it keeps the changes minimal and
seems to fix the issue at hand. That said, would welcome your feedback
here. Would you prefer that we pull in the complete upstream PR #12005
or keep the changes minimal?

PR #12005 is part of v242, i.e. fixed in unstable/testing.


Those changes don't touch udev, but will need an ack from kibi (which
I've CCed).

Regards,
Michael

-- System Information:
Debian Release: bullseye/sid
  APT prefers unstable
  APT policy: (500, 'unstable')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 5.4.0-3-amd64 (SMP w/4 CPU cores)
Locale: LANG=de_DE.UTF-8, LC_CTYPE=de_DE.UTF-8 (charmap=UTF-8), LANGUAGE=de_DE.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled
diff --git a/debian/changelog b/debian/changelog
index f63e21d..1d263f7 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,19 @@
+systemd (241-7~deb10u3) buster; urgency=medium
+
+  * core: set fs.file-max sysctl to LONG_MAX rather than ULONG_MAX.
+    Since kernel 5.2 (but also stable kernels like 4.19.53) the kernel
+    thankfully returns proper errors when we write a value out of range to
+    the sysctl. Which however breaks writing ULONG_MAX to request the
+    maximum value. Hence let's write the new maximum value instead,
+    LONG_MAX. (Closes: #945018)
+  * core: change ownership/mode of the execution directories also for static
+    users.
+    This ensures that execution directories like CacheDirectory and
+    StateDirectory are properly chowned to the user specified in User= before
+    launching the service. (Closes: #919231)
+
+ -- Michael Biebl <biebl@debian.org>  Wed, 29 Jan 2020 19:07:53 +0100
+
 systemd (241-7~deb10u2) buster; urgency=medium
 
   * core: never propagate reload failure to service result.
diff --git a/debian/gbp.conf b/debian/gbp.conf
index b0e0001..9591e25 100644
--- a/debian/gbp.conf
+++ b/debian/gbp.conf
@@ -1,7 +1,8 @@
 [DEFAULT]
 pristine-tar = True
 patch-numbers = False
-debian-branch = buster
+debian-branch = debian/buster
+upstream-branch = upstream/latest
 
 [dch]
 full = True
diff --git a/debian/patches/core-change-ownership-mode-of-the-execution-directories-a.patch b/debian/patches/core-change-ownership-mode-of-the-execution-directories-a.patch
new file mode 100644
index 0000000..6f8b0fc
--- /dev/null
+++ b/debian/patches/core-change-ownership-mode-of-the-execution-directories-a.patch
@@ -0,0 +1,85 @@
+From: Lennart Poettering <lennart@poettering.net>
+Date: Thu, 14 Mar 2019 17:19:30 +0100
+Subject: core: change ownership/mode of the execution directories also for
+ static users
+
+It's probably unexpected if we do a recursive chown() when dynamic users
+are used but not on static users.
+
+hence, let's tweak the logic slightly, and recursively chown in both
+cases, except when operating on the configuration directory.
+
+Fixes: #11842
+(cherry picked from commit 206e9864de460dd79d9edd7bedb47dee168765e1)
+---
+ src/core/execute.c | 47 ++++++++++++++++++++++++++---------------------
+ 1 file changed, 26 insertions(+), 21 deletions(-)
+
+diff --git a/src/core/execute.c b/src/core/execute.c
+index 5486e37..5c3930e 100644
+--- a/src/core/execute.c
++++ b/src/core/execute.c
+@@ -2151,37 +2151,42 @@ static int setup_exec_directory(
+                         if (r < 0)
+                                 goto fail;
+ 
+-                        /* Lock down the access mode */
+-                        if (chmod(pp, context->directories[type].mode) < 0) {
+-                                r = -errno;
+-                                goto fail;
+-                        }
+                 } else {
+                         r = mkdir_label(p, context->directories[type].mode);
+                         if (r < 0) {
+-                                struct stat st;
+-
+                                 if (r != -EEXIST)
+                                         goto fail;
+ 
+-                                if (stat(p, &st) < 0) {
+-                                        r = -errno;
+-                                        goto fail;
+-                                }
+-                                if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0)
+-                                        log_warning("%s \'%s\' already exists but the mode is different. "
+-                                                    "(filesystem: %o %sMode: %o)",
+-                                                    exec_directory_type_to_string(type), *rt,
+-                                                    st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777);
+-                                if (!context->dynamic_user)
++                                if (type == EXEC_DIRECTORY_CONFIGURATION) {
++                                        struct stat st;
++
++                                        /* Don't change the owner/access mode of the configuration directory,
++                                         * as in the common case it is not written to by a service, and shall
++                                         * not be writable. */
++
++                                        if (stat(p, &st) < 0) {
++                                                r = -errno;
++                                                goto fail;
++                                        }
++
++                                        /* Still complain if the access mode doesn't match */
++                                        if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0)
++                                                log_warning("%s \'%s\' already exists but the mode is different. "
++                                                            "(File system: %o %sMode: %o)",
++                                                            exec_directory_type_to_string(type), *rt,
++                                                            st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777);
++
+                                         continue;
++                                }
+                         }
+                 }
+ 
+-                /* Don't change the owner of the configuration directory, as in the common case it is not written to by
+-                 * a service, and shall not be writable. */
+-                if (type == EXEC_DIRECTORY_CONFIGURATION)
+-                        continue;
++                /* Lock down the access mode (we use chmod_and_chown() to make this idempotent. We don't
++                 * specifiy UID/GID here, so that path_chown_recursive() can optimize things depending on the
++                 * current UID/GID ownership.) */
++                r = chmod_and_chown(pp ?: p, context->directories[type].mode, UID_INVALID, GID_INVALID);
++                if (r < 0)
++                        goto fail;
+ 
+                 /* Then, change the ownership of the whole tree, if necessary */
+                 r = path_chown_recursive(pp ?: p, uid, gid);
diff --git a/debian/patches/core-set-fs.file-max-sysctl-to-LONG_MAX-rather-than-ULONG.patch b/debian/patches/core-set-fs.file-max-sysctl-to-LONG_MAX-rather-than-ULONG.patch
new file mode 100644
index 0000000..6465a1f
--- /dev/null
+++ b/debian/patches/core-set-fs.file-max-sysctl-to-LONG_MAX-rather-than-ULONG.patch
@@ -0,0 +1,34 @@
+From: Lennart Poettering <lennart@poettering.net>
+Date: Mon, 17 Jun 2019 10:51:25 +0200
+Subject: core: set fs.file-max sysctl to LONG_MAX rather than ULONG_MAX
+
+Since kernel 5.2 the kernel thankfully returns proper errors when we
+write a value out of range to the sysctl. Which however breaks writing
+ULONG_MAX to request the maximum value. Hence let's write the new
+maximum value instead, LONG_MAX.
+
+/cc @brauner
+
+Fixes: #12803
+(cherry picked from commit 6e2f78948403a4cce45b9e34311c9577c624f066)
+---
+ src/core/main.c | 6 +++---
+ 1 file changed, 3 insertions(+), 3 deletions(-)
+
+diff --git a/src/core/main.c b/src/core/main.c
+index bc7fcc6..255e204 100644
+--- a/src/core/main.c
++++ b/src/core/main.c
+@@ -1200,9 +1200,9 @@ static void bump_file_max_and_nr_open(void) {
+ #endif
+ 
+ #if BUMP_PROC_SYS_FS_FILE_MAX
+-        /* I so wanted to use STRINGIFY(ULONG_MAX) here, but alas we can't as glibc/gcc define that as
+-         * "(0x7fffffffffffffffL * 2UL + 1UL)". Seriously. 😢 */
+-        if (asprintf(&t, "%lu\n", ULONG_MAX) < 0) {
++        /* The maximum the kernel allows for this since 5.2 is LONG_MAX, use that. (Previously thing where
++         * different but the operation would fail silently.) */
++        if (asprintf(&t, "%li\n", LONG_MAX) < 0) {
+                 log_oom();
+                 return;
+         }
diff --git a/debian/patches/debian/Revert-core-set-RLIMIT_CORE-to-unlimited-by-default.patch b/debian/patches/debian/Revert-core-set-RLIMIT_CORE-to-unlimited-by-default.patch
index f48d841..9e1ab13 100644
--- a/debian/patches/debian/Revert-core-set-RLIMIT_CORE-to-unlimited-by-default.patch
+++ b/debian/patches/debian/Revert-core-set-RLIMIT_CORE-to-unlimited-by-default.patch
@@ -19,7 +19,7 @@ Bug-Debian: https://bugs.debian.org/815020
  2 files changed, 1 insertion(+), 3 deletions(-)
 
 diff --git a/src/core/main.c b/src/core/main.c
-index bc7fcc6..87bee9f 100644
+index 255e204..7f8dfe4 100644
 --- a/src/core/main.c
 +++ b/src/core/main.c
 @@ -2459,8 +2459,6 @@ int main(int argc, char *argv[]) {
diff --git a/debian/patches/execute-remove-one-redundant-comparison-check.patch b/debian/patches/execute-remove-one-redundant-comparison-check.patch
new file mode 100644
index 0000000..d29ece3
--- /dev/null
+++ b/debian/patches/execute-remove-one-redundant-comparison-check.patch
@@ -0,0 +1,29 @@
+From: Lennart Poettering <lennart@poettering.net>
+Date: Thu, 14 Mar 2019 17:01:46 +0100
+Subject: execute: remove one redundant comparison check
+
+(cherry picked from commit d484580ca6f0e79abe6f3f5c677323a22d9e22d7)
+---
+ src/core/execute.c | 7 ++++---
+ 1 file changed, 4 insertions(+), 3 deletions(-)
+
+diff --git a/src/core/execute.c b/src/core/execute.c
+index f2a4c54..5486e37 100644
+--- a/src/core/execute.c
++++ b/src/core/execute.c
+@@ -2158,11 +2158,12 @@ static int setup_exec_directory(
+                         }
+                 } else {
+                         r = mkdir_label(p, context->directories[type].mode);
+-                        if (r < 0 && r != -EEXIST)
+-                                goto fail;
+-                        if (r == -EEXIST) {
++                        if (r < 0) {
+                                 struct stat st;
+ 
++                                if (r != -EEXIST)
++                                        goto fail;
++
+                                 if (stat(p, &st) < 0) {
+                                         r = -errno;
+                                         goto fail;
diff --git a/debian/patches/series b/debian/patches/series
index 5e8940a..d1a5bd2 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -39,6 +39,9 @@ login-add-a-missing-error-check-for-session_set_leader.patch
 namespace-make-MountFlags-shared-work-again.patch
 mount-generators-do-not-make-unit-wanted-by-its-device-un.patch
 mount-remove-unused-mount_is_auto-and-mount_is_automount.patch
+core-set-fs.file-max-sysctl-to-LONG_MAX-rather-than-ULONG.patch
+execute-remove-one-redundant-comparison-check.patch
+core-change-ownership-mode-of-the-execution-directories-a.patch
 debian/Use-Debian-specific-config-files.patch
 debian/Bring-tmpfiles.d-tmp.conf-in-line-with-Debian-defaul.patch
 debian/Make-run-lock-tmpfs-an-API-fs.patch

Reply to: