[PATCH librados2-perl] do not shut down the handle twice if a signal arrives at exit

[PATCH librados2-perl] do not shut down the handle twice if a signal arrives at exit

[Kefu Chai]

When a pveceph worker finishes, the CLI sends SIGTERM to its process
group, which includes the pverados child. That child is often already
shutting down the handle after seeing EOF, so the handler then calls
rados_shutdown() on the freed handle and the child segfaults.

The handlers outlived the eval anyway, as the 'local' only covered
$SIG{HUP}. So ignore the signals before shutting down, and install the
handlers first thing in the child.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 PVE/RADOS.pm | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/PVE/RADOS.pm b/PVE/RADOS.pm
index 5a79235..deb4912 100644
--- a/PVE/RADOS.pm
+++ b/PVE/RADOS.pm
@@ -200,6 +200,17 @@ sub new {
     } else { # child
 	$0 = 'pverados';
 
+	my @signals = qw(HUP INT QUIT TERM);
+
+	# ignore signals from here on, the handler must not shut down a freed handle
+	my $shutdown = sub {
+	    @SIG{@signals} = ('IGNORE') x @signals;
+	    pve_rados_shutdown($self->{conn}) if $self->{conn};
+	};
+
+	# override signal handlers inherited from the parent
+	@SIG{@signals} = (sub { $shutdown->(); POSIX::_exit(1); }) x @signals;
+
 	eval {
 	    PVE::INotify::inotify_close();
 
@@ -207,12 +218,6 @@ sub new {
 		&$atfork();
 	    }
 
-	    # override signal handlers inherited from the parent
-	    local $SIG{HUP} = $SIG{INT} = $SIG{QUIT} = $SIG{TERM} = sub {
-		pve_rados_shutdown($self->{conn}) if $self->{conn};
-		POSIX::_exit(1);
-	    };
-
 	    # fixme: timeout?
 
 	    close $child;
@@ -222,7 +227,7 @@ sub new {
 	my $err = $@;
 	warn $err if $err;
 
-	pve_rados_shutdown($self->{conn}) if $self->{conn};
+	$shutdown->();
 	POSIX::_exit($err ? 1 : 0);
     }
 
-- 
2.47.3

Re: [PATCH librados2-perl] do not shut down the handle twice if a signal arrives at exit

[Maximiliano Sandoval]

Kefu Chai <k.chai@proxmox.com> writes:

> When a pveceph worker finishes, the CLI sends SIGTERM to its process
> group, which includes the pverados child. That child is often already
> shutting down the handle after seeing EOF, so the handler then calls
> rados_shutdown() on the freed handle and the child segfaults.
>
> The handlers outlived the eval anyway, as the 'local' only covered
> $SIG{HUP}. So ignore the signals before shutting down, and install the
> handlers first thing in the child.
>
> Signed-off-by: Kefu Chai <k.chai@proxmox.com>
> ---
>  PVE/RADOS.pm | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> [..]

I ran into this today when trying to create a EC pool and allocating an
image on a freshly installed PVE 9 3-node cluster with the latest
packages via:

pveceph pool create test-ec --erasure-coding k=4,m=2
pvesm alloc test-ec 999 vm-999-disk-0 1G

The pool creation succeeds and the alloc times out, one can see in the
journal:

Oct 20 14:45:33 c3-pve-201 kernel: pverados[2811]: segfault at 3b40 ip 00007d006bd44ba0 sp 00007ffd43e96c28 error 4 in libc.so.6[95ba0,7d006bcd7000+163000] likely on CPU 1 (core 1, socket 0)
Oct 20 14:45:33 c3-pve-201 kernel: Code: 00 e8 64 27 fa ff 48 8d 0d ed 9c 11 00 ba c2 01 00 00 48 8d 35 47 13 11 00 48 8d 3d 55 13 11 00 e8 45 27 fa ff 0f 1f 44 00 00 <8b> 47 10 89 c2 81 e2 7f 01 00 00 83 e0 7c 0f 85 ac 00 00 00 53 48

and the health status will be stuck "Reduced data availability" in.

One thing that would be good to have in the commit message is what
setups were affected and since when, if this is a regression.

-- 
Maximiliano

Re: [PATCH librados2-perl] do not shut down the handle twice if a signal arrives at exit

[Maximiliano Sandoval]

Kefu Chai <k.chai@proxmox.com> writes:

> When a pveceph worker finishes, the CLI sends SIGTERM to its process
> group, which includes the pverados child. That child is often already
> shutting down the handle after seeing EOF, so the handler then calls
> rados_shutdown() on the freed handle and the child segfaults.
>
> The handlers outlived the eval anyway, as the 'local' only covered
> $SIG{HUP}. So ignore the signals before shutting down, and install the
> handlers first thing in the child.
>
> Signed-off-by: Kefu Chai <k.chai@proxmox.com>

Tested that librados does not segfault anymore when doing the following

- creating an EC pool with --erasure-coding k=4,m=2 --pg_num=64
- ceph -s
- allocating a 1G image in the pool
- destroying the data and metadata pools
- ceph -s

At least on my 3-node cluster the image cannot be allocated due to the
issue described at
https://lore.proxmox.com/pve-devel/20261021065240.3495098-1-k.chai@proxmox.com/T/,
but that is a different issue.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano