Pkg xen devel - May 2020 - Bug#961511: xen-utils-common: Protect xenstored/xenconsoled against OOM

Package: xen-utils-common
Version: 4.11.3+24-g14b62ab3e5-1
Severity: important

Hello,

In case memory in dom0 gets tight, the OOM killer tends to try to kill
xenstored and xenconsoled. From then on it's very difficult to control
the VMs running on the system... It'd be good to protect them from the
OOM since they are so precious.

I'm currently using a hack such as

        for i in $(pgrep xenconsoled) ; do
                echo -1000 > /proc/$i/oom_score_adj
        done

in /etc/init.d/xen, but there are cleaner ways to do this :)

Samuel

-- System Information:
Debian Release: bullseye/sid
  APT prefers testing
  APT policy: (990, 'testing'), (500, 'unstable-debug'), (500,
'testing-debug'), (500, 'stable-debug'), (500,
'proposed-updates-debug'), (500, 'proposed-updates'), (500,
'oldstable-proposed-updates-debug'), (500,
'oldstable-proposed-updates'), (500, 'oldoldstable'), (500,
'buildd-unstable'), (500, 'unstable'), (500, 'stable'),
(500, 'oldstable'), (1, 'experimental-debug'), (1,
'buildd-experimental'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 5.6.0-1-amd64 (SMP w/8 CPU cores)
Kernel taint flags: TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=fr_FR.UTF-8, LC_CTYPE=fr_FR.UTF-8 (charmap=UTF-8),
LANGUAGE=fr_FR.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages xen-utils-common depends on:
ii  libc6           2.30-8
ii  libxenstore3.0  4.11.3+24-g14b62ab3e5-1
ii  lsb-base        11.1.0
pn  python          <none>
ii  ucf             3.0038+nmu1
ii  udev            245.5-3
ii  xenstore-utils  4.11.3+24-g14b62ab3e5-1

xen-utils-common recommends no packages.

Versions of packages xen-utils-common suggests:
pn  xen-doc  <none>

-- Configuration Files:
/etc/xen/scripts/vif-common.sh changed [not included]
/etc/xen/scripts/vif-nat changed [not included]
/etc/xen/xl.conf changed [not included]

-- no debconf information

Samuel Thibault, le lun. 25 mai 2020 15:11:44 +0200, a
ecrit:
> I'm currently using a hack such as
> 
>         for i in $(pgrep xenconsoled) ; do
>                 echo -1000 > /proc/$i/oom_score_adj
>         done
> 
> in /etc/init.d/xen, but there are cleaner ways to do this :)
For instance, using choom:

	start-stop-daemon --start --quiet --pidfile "$XENCONSOLED_PIDFILE"
--exec /usr/bin/choom -- \
		-n -1000 "$XENCONSOLED" $XENCONSOLED_ARGS --pid-file
"$XENCONSOLED_PIDFILE" \

Samuel

Hi,

On 5/25/20 3:18 PM, Samuel Thibault wrote:
> Samuel Thibault, le lun. 25 mai 2020 15:11:44 +0200, a ecrit:
>> I'm currently using a hack such as
>>
>>         for i in $(pgrep xenconsoled) ; do
>>                 echo -1000 > /proc/$i/oom_score_adj
>>         done
>>
>> in /etc/init.d/xen, but there are cleaner ways to do this :)
> 
> For instance, using choom:
> 
> 	start-stop-daemon --start --quiet --pidfile
"$XENCONSOLED_PIDFILE" --exec /usr/bin/choom -- \
> 		-n -1000 "$XENCONSOLED" $XENCONSOLED_ARGS --pid-file
"$XENCONSOLED_PIDFILE" \
That's a nice idea! Especially for xenstored, because it only keeps
state in memory.

xenconsoled can be started again if it's ever oom killed. so, I'd like
to limit this to xenstored only.

E.g. in my situation at work, it's mostly openvswitch that gets killed
first, if there's really a situation in which something has to go. If I
can choose between that (which disrupts vm traffic) or xenconsoled
(which does not impact customer stuff directly), then I'd rather see the
last one go temporarily.

I had to insert another -- before $XENCONSOLED_ARGS to actually make it
work.

After reboot:

-# grep . /proc/$(pidof /usr/lib/xen-4.14/bin/oxenstored)/oom_*
/proc/7478/oom_adj:-17
/proc/7478/oom_score:0
/proc/7478/oom_score_adj:-1000

Hans

In case of oom killer terminating some process, we'd rather not see
xenstored go. Xenstored has an in-memory database, and when starting the
process again, it would be empty, which is very inconvenient. Xenstored
should already score quite low and have a fairly low memory footprint,
but according to the user report, it happened.

Closes: #961511
Suggested-by: Samuel Thibault <sthibault at debian.org>
Signed-off-by: Hans van Kranenburg <hans at knorrie.org>
---
Cc: Ian Jackson <ijackson at chiark.greenend.org.uk>
---
This is in my knorrie/4.14-extra branch now. I think we should do this.
---
 debian/xen-utils-common.xen.init | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/debian/xen-utils-common.xen.init b/debian/xen-utils-common.xen.init
index 54aaba89d320..2a4c09fa3f71 100644
--- a/debian/xen-utils-common.xen.init
+++ b/debian/xen-utils-common.xen.init
@@ -226,7 +226,8 @@ xenstored_start()
 		eval "try_xenstored=\$$try_xenstored_var"
 		if [ -x $try_xenstored ]; then
 			if start-stop-daemon --start --quiet \
-				--pidfile "$XENSTORED_PIDFILE" --exec "$try_xenstored"
-- \
+				--pidfile "$XENSTORED_PIDFILE" \
+				--exec /usr/bin/choom -- -n -1000 "$try_xenstored" -- \
 				$XENSTORED_ARGS --pid-file "$XENSTORED_PIDFILE"; then
 					started_xenstored=$try_xenstored
 					break
-- 
2.20.1

~Hans van Kranenburg writes ("[PATCH] d/xen-utils-common.xen.init: disable
oom killer for xenstored"):
> In case of oom killer terminating some process, we'd rather not see
> xenstored go. Xenstored has an in-memory database, and when starting the
> process again, it would be empty, which is very inconvenient. Xenstored
> should already score quite low and have a fairly low memory footprint,
> but according to the user report, it happened.
> 
> Closes: #961511
> Suggested-by: Samuel Thibault <sthibault at debian.org>
> Signed-off-by: Hans van Kranenburg <hans at knorrie.org>
Acked-by: Ian Jackson <ijackson at chiark.greenend.org.uk>

tag -1 + pending
thanks

On 9/7/20 12:40 PM, Ian Jackson wrote:
> ~Hans van Kranenburg writes ("[PATCH] d/xen-utils-common.xen.init:
disable oom killer for xenstored"):
>> In case of oom killer terminating some process, we'd rather not see
>> xenstored go. Xenstored has an in-memory database, and when starting
the
>> process again, it would be empty, which is very inconvenient. Xenstored
>> should already score quite low and have a fairly low memory footprint,
>> but according to the user report, it happened.
>>
>> Closes: #961511
>> Suggested-by: Samuel Thibault <sthibault at debian.org>
>> Signed-off-by: Hans van Kranenburg <hans at knorrie.org>
> 
> Acked-by: Ian Jackson <ijackson at chiark.greenend.org.uk>
Thanks, added.

Hans

This is fun.  Actually isn't too difficult to trigger, simply slowly
reduce the memory Xen allocates to Dom0 and eventually the oom-killer is
likely to trigger (having tried to shrink Dom0 as far as possible,
believe me, I know).  I had been wondering which of the Xen daemons could
be safely restarted since it is handy to restart daemons instead of whole
machine for security updates...

Interestingly running `xenstored --help` mentions:
  -I, --internal-db       store database in memory, not on disk

There is a run/xenstored/tdb file so I end up wondering if newer versions
are in fact storing everything in a file and restarting isn't so bad.

The patch switches the arguments from:
--exec "$try_xenstored" -- ...
to:
--exec /usr/bin/choom -- -n -1000 "$try_xenstored" -- ...

I'm pretty sure start-stop-daemon is consuming the "--" and the
second
"--" shouldn't be there.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Your message dated Mon, 21 Sep 2020 10:00:33 +0000
with message-id <E1kKIcb-000Iwx-Ah at fasolo.debian.org>
and subject line Bug#961511: fixed in xen 4.14.0-1~exp1
has caused the Debian Bug report #961511,
regarding xen-utils-common: Protect xenstored/xenconsoled against OOM
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner at bugs.debian.org
immediately.)


-- 
961511: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=961511
Debian Bug Tracking System
Contact owner at bugs.debian.org with problems
-------------- next part --------------
An embedded message was scrubbed...
From: Samuel Thibault <sthibault at debian.org>
Subject: xen-utils-common: Protect xenstored/xenconsoled against OOM
Date: Mon, 25 May 2020 15:11:44 +0200
Size: 4279
URL:
<http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20200921/b1aafc8c/attachment.mht>
-------------- next part --------------
An embedded message was scrubbed...
From: Debian FTP Masters <ftpmaster at ftp-master.debian.org>
Subject: Bug#961511: fixed in xen 4.14.0-1~exp1
Date: Mon, 21 Sep 2020 10:00:33 +0000
Size: 19825
URL:
<http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20200921/b1aafc8c/attachment-0001.mht>

notfixed 961511 xen/4.14.0-1~exp1
thanks

Right... so in the end I made an off-by-one error while rebasing and
totally lost that commit. It's not actually in 4.14.0-1~exp1 now. That's
bad.

On 9/21/20 3:50 AM, Elliott Mitchell wrote:
> This is fun.  Actually isn't too difficult to trigger, simply slowly
> reduce the memory Xen allocates to Dom0 and eventually the oom-killer is
> likely to trigger (having tried to shrink Dom0 as far as possible,
> believe me, I know).  I had been wondering which of the Xen daemons could
> be safely restarted since it is handy to restart daemons instead of whole
> machine for security updates...
> 
> Interestingly running `xenstored --help` mentions:
>   -I, --internal-db       store database in memory, not on disk
> 
> There is a run/xenstored/tdb file so I end up wondering if newer versions
> are in fact storing everything in a file and restarting isn't so bad.
Not by default, and I don't know if it's actually considered best
practice. I could not find any info about this yet. I suspect it's not
recommended.

oxenstored has the following option in /etc/xen/oxenstored.conf:

# Activate filed base backend
persistent = false

When enabling this, the file /run/xenstored/db gets rewritten a lot and
I also see it's out of sync with what's in xenstore-ls after doing some
things. So, it might me inconsistent when the process is oom-killed.
> The patch switches the arguments from:
> --exec "$try_xenstored" -- ...
> to:
> --exec /usr/bin/choom -- -n -1000 "$try_xenstored" -- ...
> 
> I'm pretty sure start-stop-daemon is consuming the "--" and
the second
> "--" shouldn't be there.
Well, I tested it and found out that it's needed...

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/bin/choom -- -n -1000 \
   /usr/lib/xen-4.14/bin/oxenstored --pid-file "/run/xenstore.pid"
/usr/bin/choom: unrecognized option '--pid-file'
Try 'choom --help' for more information.

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/lib/xen-4.14/bin/oxenstored --test
Would start /usr/lib/xen-4.14/bin/oxenstored .

and with the extra separator:

-# start-stop-daemon --start \
   --pidfile "/run/xenstore.pid" \
   --exec /usr/bin/choom -- -n -1000 \
   /usr/lib/xen-4.14/bin/oxenstored -- --pid-file "/run/xenstore.pid"

-# grep . /proc/$(pidof /usr/lib/xen-4.14/bin/oxenstored)/oom_*
/proc/363043/oom_adj:-17
/proc/363043/oom_score:0
/proc/363043/oom_score_adj:-1000

-# cat /proc/$(pidof /usr/lib/xen-4.14/bin/oxenstored)/cmdline
/usr/lib/xen-4.14/bin/oxenstored--pid-file/run/xenstore.pid

How did you test it and how did you get a working process without the --?

Hans

Processing commands for control at bugs.debian.org:
> notfixed 961511 xen/4.14.0-1~exp1
Bug #961511 {Done: Hans van Kranenburg <hans at knorrie.org>}
[xen-utils-common] xen-utils-common: Protect xenstored/xenconsoled against OOM
No longer marked as fixed in versions xen/4.14.0-1~exp1.
> thanks
Stopping processing here.

Please contact me if you need assistance.
-- 
961511: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=961511
Debian Bug Tracking System
Contact owner at bugs.debian.org with problems

On Tue, Sep 22, 2020 at 02:39:09PM +0200, Hans van Kranenburg
wrote:
> How did you test it and how did you get a working process without the --?
By reading the man page, noticing there was no mention of "--" and
then
trying `choom -n +5 sleep 5` and found that worked.  When you sent this
message I checked and GNU `sleep` does have "--version", thus I tried
`choom -n +5 sleep 5 --version` and found *that* failed.

A "--" seemed natural, but documentation omitting crucial details is a
problem.  Never mind.

Nice find, I did at one point have the oom-killer get the wrong process
and saw *problems*.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445