[PATCH v2 0/2] fix drbd-utils minor issues

[PATCH v2 0/2] fix drbd-utils minor issues

[Heming Zhao]

This patchset fixes two issues:
- configure reports WARNING for SuSe-release file
- lock file permission issue in SELinux env

v1->v2
factor out shared code for creating lfname
add free lfname job after unlink()

Heming Zhao (2):
  build: changing SUSE release file
  remove lock file after using it

 configure.ac              |  4 ++--
 user/shared/drbdmeta.c    |  3 +++
 user/shared/shared_tool.c | 40 +++++++++++++++++++++++++++++----------
 user/shared/shared_tool.h |  2 +-
 user/v84/drbdsetup.c      |  2 +-
 user/v9/drbdsetup.c       |  2 +-
 6 files changed, 38 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] build: changing SUSE release file

[Heming Zhao]

This commit changes /etc/SuSe-release to /etc/os-release.
The SuSe-release file disappeared a long time ago.

Signed-off-by: Heming Zhao <heming.zhao@suse.com>
---
 configure.ac | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 282fdf1584e3..883fe3e5cdb3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -402,7 +402,7 @@ else
    if test -z $DISTRO; then
       AC_CHECK_FILE(/etc/redhat-release, [DISTRO="redhat"])
       AC_CHECK_FILE(/etc/debian_version, [DISTRO="debian"])
-      AC_CHECK_FILE(/etc/SuSE-release, [DISTRO="suse"])
+      AC_CHECK_FILE(/etc/os-release, [DISTRO="suse"])
    fi
    case "$DISTRO" in
         redhat)
@@ -439,7 +439,7 @@ else
 	    # RPM_REQ_HEARTBEAT="Requires: heartbeat"
 
 	    # Unfortunately gcc on SLES9 is broken with -O2. Works with -O1
-	    if grep -q 'VERSION = 9' /etc/SuSE-release; then
+	    if grep -q 'VERSION = 9' /etc/os-release; then
 		    CFLAGS="-g -O1"
 	    fi
 	    ;;
-- 
2.43.0

Re: [PATCH v2 1/2] build: changing SUSE release file

[Roland Kammerer]

On Thu, May 15, 2025 at 10:48:14AM +0800, Heming Zhao wrote:
> This commit changes /etc/SuSe-release to /etc/os-release.
> The SuSe-release file disappeared a long time ago.
> 
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  configure.ac | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 282fdf1584e3..883fe3e5cdb3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -402,7 +402,7 @@ else
>     if test -z $DISTRO; then
>        AC_CHECK_FILE(/etc/redhat-release, [DISTRO="redhat"])
>        AC_CHECK_FILE(/etc/debian_version, [DISTRO="debian"])
> -      AC_CHECK_FILE(/etc/SuSE-release, [DISTRO="suse"])
> +      AC_CHECK_FILE(/etc/os-release, [DISTRO="suse"])

Hi,

everything that has a /etc/os-release (which by now is basically every
distribution on this planet) is suse? No.

This (not only this) part of the automagic in drbd-utils is a mess and
I'm currently in the process of cleaning that up a bit. I will not take
this patch as is, IMO it is just wrong, please just wait, this should
improve in a week or two.

Regards, rck

Re: [PATCH v2 1/2] build: changing SUSE release file

[Heming Zhao]

On 5/28/25 16:03, Roland Kammerer wrote:
> On Thu, May 15, 2025 at 10:48:14AM +0800, Heming Zhao wrote:
>> This commit changes /etc/SuSe-release to /etc/os-release.
>> The SuSe-release file disappeared a long time ago.
>>
>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>> ---
>>   configure.ac | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 282fdf1584e3..883fe3e5cdb3 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -402,7 +402,7 @@ else
>>      if test -z $DISTRO; then
>>         AC_CHECK_FILE(/etc/redhat-release, [DISTRO="redhat"])
>>         AC_CHECK_FILE(/etc/debian_version, [DISTRO="debian"])
>> -      AC_CHECK_FILE(/etc/SuSE-release, [DISTRO="suse"])
>> +      AC_CHECK_FILE(/etc/os-release, [DISTRO="suse"])
> 
> Hi,
> 
> everything that has a /etc/os-release (which by now is basically every
> distribution on this planet) is suse? No.
> 
> This (not only this) part of the automagic in drbd-utils is a mess and
> I'm currently in the process of cleaning that up a bit. I will not take
> this patch as is, IMO it is just wrong, please just wait, this should
> improve in a week or two.
> 
> Regards, rck

Thanks for the explanation.
Indeed, I didn't check other distribution styles. Let's wait for your
fix for this build issue.

- Heming

[PATCH v2 2/2] remove lock file after using it

[Heming Zhao]

In a SELinux env, if a user runs as root to set up a DRBD device,
it will leave a lock file in "/var/run/drbd/lock/". Then HA pacemaker
will fail to set up DRBD because of a permission issue.

This commit removes the lock file when drbdsetup and drbdmeta close
the lock fd.

Signed-off-by: Heming Zhao <heming.zhao@suse.com>
---
 user/shared/drbdmeta.c    |  3 +++
 user/shared/shared_tool.c | 40 +++++++++++++++++++++++++++++----------
 user/shared/shared_tool.h |  2 +-
 user/v84/drbdsetup.c      |  2 +-
 user/v9/drbdsetup.c       |  2 +-
 5 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/user/shared/drbdmeta.c b/user/shared/drbdmeta.c
index 099592a530da..c84c09abef86 100644
--- a/user/shared/drbdmeta.c
+++ b/user/shared/drbdmeta.c
@@ -5482,6 +5482,9 @@ int main(int argc, char **argv)
 	if (minor_attached)
 		fprintf(stderr, "# Output might be stale, since minor %d is attached\n", cfg->minor);
 
+	if ((cfg->minor != -1) && (cfg->lock_fd != -1))
+		dt_unlock_drbd(cfg->lock_fd, cfg->minor);
+
 	// dummy bool normalization to not return negative values, the usual "FIXME sane exit codes" still applies */
 	return !!rv;
 	/* and if we want an explicit free,
diff --git a/user/shared/shared_tool.c b/user/shared/shared_tool.c
index 20598132a0ac..b45d71379d96 100644
--- a/user/shared/shared_tool.c
+++ b/user/shared/shared_tool.c
@@ -759,9 +759,26 @@ void ensure_dir(const char *dirname)
 	}
 }
 
+static char *get_lock_filename(int minor, int check_dir)
+{
+	int sz;
+	char *lfname;
+
+	if (check_dir)
+		ensure_dir(drbd_lock_dir());
+
+	sz = asprintf(&lfname, "%s/drbd-%d-%d",
+		      drbd_lock_dir(), LANANA_DRBD_MAJOR, minor);
+	if (sz < 0) {
+		perror("");
+		exit(20);
+	}
+	return lfname;
+}
+
 int dt_lock_drbd(int minor)
 {
-	int sz, lfd;
+	int lfd;
 	char *lfname;
 
 	/* THINK.
@@ -777,14 +794,7 @@ int dt_lock_drbd(int minor)
 	 * We should store something in the meta data to detect such abuses.
 	 */
 
-	ensure_dir(drbd_lock_dir());
-	sz = asprintf(&lfname, "%s/drbd-%d-%d",
-		      drbd_lock_dir(), LANANA_DRBD_MAJOR, minor);
-	if (sz < 0) {
-		perror("");
-		exit(20);
-	}
-
+	lfname = get_lock_filename(minor, 1);
 	lfd = get_fd_lockfile_timeout(lfname, 1);
 	free (lfname);
 	if (lfd < 0)
@@ -793,10 +803,20 @@ int dt_lock_drbd(int minor)
 }
 
 /* ignore errors */
-void dt_unlock_drbd(int lock_fd)
+void dt_unlock_drbd(int lock_fd, int minor)
 {
+	int ret;
+	char *lfname;
+
 	if (lock_fd >= 0)
 		unlock_fd(lock_fd);
+
+	lfname = get_lock_filename(minor, 0);
+	ret = unlink(lfname);
+	free(lfname);
+	if (ret < 0) {
+		perror("unlink");
+	}
 }
 
 void dt_print_gc(const uint32_t* gen_cnt)
diff --git a/user/shared/shared_tool.h b/user/shared/shared_tool.h
index 3dedf79683f5..44f10b549a77 100644
--- a/user/shared/shared_tool.h
+++ b/user/shared/shared_tool.h
@@ -108,7 +108,7 @@ extern bool addr_scope_local(const char *input);
 extern unsigned long long m_strtoll(const char* s,const char def_unit);
 extern int only_digits(const char *s);
 extern int dt_lock_drbd(int minor);
-extern void dt_unlock_drbd(int lock_fd);
+extern void dt_unlock_drbd(int lock_fd, int minor);
 extern int dt_minor_of_dev(const char *device);
 extern void dt_print_gc(const uint32_t* gen_cnt);
 extern void dt_pretty_print_gc(const uint32_t* gen_cnt);
diff --git a/user/v84/drbdsetup.c b/user/v84/drbdsetup.c
index 9127a4462a0e..530af7b70b95 100644
--- a/user/v84/drbdsetup.c
+++ b/user/v84/drbdsetup.c
@@ -3715,7 +3715,7 @@ int main(int argc, char **argv)
 	rv = cmd->function(cmd, argc, argv);
 
 	if ((context & CTX_MINOR) && !cmd->lockless)
-		dt_unlock_drbd(lock_fd);
+		dt_unlock_drbd(lock_fd, minor);
 	return rv;
 }
 #endif
diff --git a/user/v9/drbdsetup.c b/user/v9/drbdsetup.c
index 8aa9b7c48a6a..e7fa4d8bad7b 100644
--- a/user/v9/drbdsetup.c
+++ b/user/v9/drbdsetup.c
@@ -4779,6 +4779,6 @@ int drbdsetup_main(int argc, char **argv)
 	rv = cmd->function(cmd, argc, argv);
 
 	if ((context & CTX_MINOR) && !cmd->lockless)
-		dt_unlock_drbd(lock_fd);
+		dt_unlock_drbd(lock_fd, minor);
 	return rv;
 }
-- 
2.43.0

Re: [PATCH v2 2/2] remove lock file after using it

[Roland Kammerer]

On Thu, May 15, 2025 at 10:48:15AM +0800, Heming Zhao wrote:
> In a SELinux env, if a user runs as root to set up a DRBD device,
> it will leave a lock file in "/var/run/drbd/lock/". Then HA pacemaker
> will fail to set up DRBD because of a permission issue.

Sorry, but that needs more explanation. I wonder why it works for
everybody else including our SuSE customers but would require changes
for you? What exactly (sles versions, pacemaker versions,...) do I need
to reproduce this?

>  /* ignore errors */
> -void dt_unlock_drbd(int lock_fd)
> +void dt_unlock_drbd(int lock_fd, int minor)
>  {
> +	int ret;
> +	char *lfname;
> +
>  	if (lock_fd >= 0)
>  		unlock_fd(lock_fd);
> +
> +	lfname = get_lock_filename(minor, 0);
> +	ret = unlink(lfname);
> +	free(lfname);
> +	if (ret < 0) {
> +		perror("unlink");
> +	}
>  }

As I don't understand why you would need these changes in the first
place I did not look into the patch too closely, but: You first unlock
and then unlink? Shouldn't that be the other way round? As is you would
create a race I guess. But first things first: why do you need that at
all?

Regards, rck

P.S.: it is fine to send user land patches here, but the main focus is
on kernel level coordination, feel free to send patches via github.

Re: [PATCH v2 2/2] remove lock file after using it

[Heming Zhao]

Hello,

On 5/28/25 16:11, Roland Kammerer wrote:
> On Thu, May 15, 2025 at 10:48:15AM +0800, Heming Zhao wrote:
>> In a SELinux env, if a user runs as root to set up a DRBD device,
>> it will leave a lock file in "/var/run/drbd/lock/". Then HA pacemaker
>> will fail to set up DRBD because of a permission issue.
> 
> Sorry, but that needs more explanation. I wonder why it works for
> everybody else including our SuSE customers but would require changes
> for you? What exactly (sles versions, pacemaker versions,...) do I need
> to reproduce this?

Sorry for the late reply, I took some time to set up a reproducible env
for this issue.

This issue was originally reported by the SUSE QA team, and occurs in
SLE16 (which is not yet released). However, I also reproduced it on the
latest openSUSE tumbleweed.

please refer to the final section of this mail for the reproduction steps.

> 
>>   /* ignore errors */
>> -void dt_unlock_drbd(int lock_fd)
>> +void dt_unlock_drbd(int lock_fd, int minor)
>>   {
>> +	int ret;
>> +	char *lfname;
>> +
>>   	if (lock_fd >= 0)
>>   		unlock_fd(lock_fd);
>> +
>> +	lfname = get_lock_filename(minor, 0);
>> +	ret = unlink(lfname);
>> +	free(lfname);
>> +	if (ret < 0) {
>> +		perror("unlink");
>> +	}
>>   }
> 
> As I don't understand why you would need these changes in the first
> place I did not look into the patch too closely, but: You first unlock
> and then unlink? Shouldn't that be the other way round? As is you would
> create a race I guess. But first things first: why do you need that at
> all?

I am not quit following you. From your description, it appears that
drbd-utils expects the lock file (/var/run/drbd/lock/xxx) to persist
throughout the system's runtime.
If drbd-utils deletes the file after every close() call, this could
potentially trigger a race condition in DRBD, correct?

This patch removes the lock file after close(). From the code logic
perspective, the lock file can be safely removed after close().
Could you point out a potential race condition?

> 
> Regards, rck
> 
> P.S.: it is fine to send user land patches here, but the main focus is
> on kernel level coordination, feel free to send patches via github.

OK, I will submit the next patch via github.


---- following are the reproducible steps -----

## setup vm

create two VMs:
- 4 VCPU, 4G mem, one 100M *shared* disk for sbd, one 1G disk for drbd.
- the sbd is using scsi bus type, drbd disk is using virtio bus type

## install softwares

I use opensuse tumbleweed latest ISO:
> openSUSE-Tumbleweed-DVD-x86_64-Snapshot20250522-Media.iso

on both nodes:
zypper in pdsh crmsh pacemaker corosync libcsync-plugin-sftp drbd-utils

## setup cluster

setup vm hostname.
node1: hostnamectl set-hostname tw25-1
node2: hostnamectl set-hostname tw25-2

edit "/etc/hosts", add 2-nodes ip and hostname in it:
192.168.122.175 tw25-1
192.168.122.214 tw25-2

cleanup disk:
wipefs -a /dev/sdb  /dev/vdb
(sdb is 100MB shared disk for sbd, vdb is 1GB disk for drbd.)

on node1:
crm cluster init -s <sbd dev> -y

from my env: crm cluster init -s /dev/sda -y

on node2:
crm cluster join -c <node1-ip>

from my env: crm cluster join -c 192.168.122.175

use "crm status full" to check cluster status.

ref: https://documentation.suse.com/sle-ha/15-SP6/html/SLE-HA-all/article-installation.html

## setup drbd

on both node, create "/etc/drbd.d/drbd_passive.res"

```
resource drbd_passive {
   protocol  C;
   device    /dev/drbd_passive minor 0;
   meta-disk internal;

   on tw25-1 {
     address 192.168.122.134:7788;
     disk    "/dev/disk/by-path/virtio-pci-0000:0b:00.0";
     node-id 0;
   }

   on tw25-2 {
     address 192.168.122.177:7788;
     disk    "/dev/disk/by-path/virtio-pci-0000:0b:00.0";
     node-id 1;
   }

   disk {
     resync-rate 10M;
     no-md-flushes;
     no-disk-flushes;
     no-disk-barrier;
   }

   connection-mesh {
     hosts tw25-1 tw25-2;
   }
}
```

- Run in both nodes: wipefs -a /dev/<drbd-dev>
- Run in both nodes: `drbdadm create-md drbd_passive`
- Run in both nodes: `drbdadm up drbd_passive`
- Run in node 1: `drbdadm -- --overwrite-data-of-peer --force primary drbd_passive`
- Run in both nodes: `drbdadm status drbd_passive`. Continue running this command in both nodes until the devices are in sync.
- Run in node 1: `drbdadm secondary drbd_passive`
- Run in both nodes: `drbdadm down drbd_passive`

please note, after `drbdadm down drbd_passive`, drbdadm left a lock file in
"/var/run/drbd/lock/". This is the key to reproduce this bug. if you remove
the lock file, the issue will never be triggered.

## create/start the drbd resource

on node1:
> # crm config edit

add following config:
```
primitive drbd_passive ocf:linbit:drbd \
          params drbd_resource=drbd_passive ignore_missing_notifications=true
clone ms_drbd_passive drbd_passive \
          meta clone-max=2 clone-node-max=1 notify=true promotable=true
```

the journal log output:
```
drbd(drbd_passive)[4860]: ERROR: drbd_passive: Called drbdadm -c /etc/drbd.conf check-resize drbd_passive
drbd(drbd_passive)[4864]: ERROR: drbd_passive: Exit code 20
drbd(drbd_passive)[4868]: ERROR: drbd_passive: Command output:
drbd(drbd_passive)[4872]: ERROR: drbd_passive: Command stderr: open(/var/run/drbd/lock/drbd-147-0): Permission denied
                  Maybe 'rm -f Maybe 'rm -f /drbd/lock/drbd-147-0): Permission denied
                  ', or check sysctl fs.protected_regular
                  Command 'drbdmeta 0 v09 /dev/disk/by-path/virtio-pci-0000:06:00.0 internal check-resize' terminated with exit code 20
```

you can re-trigger this issue by
```
# crm resource stop ms_drbd_passive
# crm resource stop drbd_passive
# crm resource cleanup drbd_passive
# crm resource cleanup ms_drbd_passive
# crm resource start drbd_passive  <== failure, block next command
# crm resource start ms_drbd_passive <== failure
```

## softwares version

```
# rpm -qa | grep pacemaker
pacemaker-schemas-3.0.0+20250331.008a7d1784-1.1.noarch
pacemaker-libs-3.0.0+20250331.008a7d1784-1.1.x86_64
pacemaker-cli-3.0.0+20250331.008a7d1784-1.1.x86_64
pacemaker-3.0.0+20250331.008a7d1784-1.1.x86_64

# rpm -qa | grep corosync
corosync-libs-3.1.9-2.1.x86_64
corosync-3.1.9-2.1.x86_64

# rpm -qa | grep crmsh
crmsh-scripts-5.0.0+20250522.ffecd6cb-1.1.noarch
crmsh-5.0.0+20250522.ffecd6cb-1.1.noarch

# rpm -qa | grep drbd
drbd-selinux-9.29.0-3.1.x86_64
drbd-utils-9.29.0-3.1.x86_64
drbd-9.1.23-4.4.x86_64
drbd-kmp-default-9.1.23_k6.14.6_1-4.4.x86_64

# uname -a
Linux tw25-3 6.14.6-1-default #1 SMP PREEMPT_DYNAMIC Tue May 13 09:56:22 UTC 2025 (ad69173) x86_64 x86_64 x86_64 GNU/Linux
```

Thanks,
Heming

Re: [PATCH v2 2/2] remove lock file after using it

[Heming Zhao]

On 5/29/25 14:59, Heming Zhao wrote:
> Hello,
> 
> On 5/28/25 16:11, Roland Kammerer wrote:
>> On Thu, May 15, 2025 at 10:48:15AM +0800, Heming Zhao wrote:
>>> In a SELinux env, if a user runs as root to set up a DRBD device,
>>> it will leave a lock file in "/var/run/drbd/lock/". Then HA pacemaker
>>> will fail to set up DRBD because of a permission issue.
>>
>> ... ...
> 
> 
> ---- following are the reproducible steps -----
> 
> ## setup vm
> 
> create two VMs:
> - 4 VCPU, 4G mem, one 100M *shared* disk for sbd, one 1G disk for drbd.
> - the sbd is using scsi bus type, drbd disk is using virtio bus type
> 
> 

I forgot to mention that to reproduce this issue, please make sure SELinux
is enabled (default configuration) and the port 7799 is open (or disable firewalld).

- Heming

Re: [PATCH v2 2/2] remove lock file after using it

[Heming Zhao]

On 5/29/25 14:59, Heming Zhao wrote:
> Hello,
> 
> On 5/28/25 16:11, Roland Kammerer wrote:
>> On Thu, May 15, 2025 at 10:48:15AM +0800, Heming Zhao wrote:
>>> In a SELinux env, if a user runs as root to set up a DRBD device,
>>> it will leave a lock file in "/var/run/drbd/lock/". Then HA pacemaker
>>> will fail to set up DRBD because of a permission issue.
>>
>> ... ...
> 
> 
> ---- following are the reproducible steps -----
> 
> ## setup vm
> 
> create two VMs:
> - 4 VCPU, 4G mem, one 100M *shared* disk for sbd, one 1G disk for drbd.
> - the sbd is using scsi bus type, drbd disk is using virtio bus type
> 
> 

I forgot to mention that to reproduce this issue, please make sure SELinux
is enabled (default configuration) and the port 7799 is open (or disable firewalld).

- Heming