Bug 178396 - [kernel] [patch] Add jid to kernel log when a process has been forced closed
Summary: [kernel] [patch] Add jid to kernel log when a process has been forced closed
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-07 15:10 UTC by freebsd
Modified: 2018-05-21 00:00 UTC (History)
4 users (show)

See Also:


Attachments
file.diff (1.04 KB, patch)
2013-05-07 15:10 UTC, freebsd
no flags Details | Diff
updated patch against stable/10 r293209 (1.33 KB, patch)
2016-01-06 00:42 UTC, Thomas Steen Rasmussen / Tykling
no flags Details | Diff
updated patch with jailname instead of hostname (899 bytes, patch)
2016-01-06 09:00 UTC, Thomas Steen Rasmussen / Tykling
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description freebsd 2013-05-07 15:10:00 UTC
When process is forced killed, kernel log "process %pid (%processname), uid %uid: exited on signal %sid".

In a system hosting jails, this message, since it is produced by kernel, is logged on the host.

I'm suggesting to also log JID for "debugging" purpose: in my case, i have several jails, each with an apache process, and without JID information, i'm not able to find which one is causing "httpd: exiting on signal 10", without reading every apache error logs of each jail...

Fix: See proposal patch in attachment.

Patch attached with submission follows:
Comment 1 Clément Moulin 2013-05-07 15:13:36 UTC
I attached the wrong patch version.

The good one is:

begin-base64 644 kern_sig.c.patch
LS0tIHN5cy9rZXJuL2tlcm5fc2lnLmMub3JpZwkyMDEzLTA1LTA3IDE1OjU4OjEwLjAwMDAwMDA=
w
MCArMDIwMAorKysgc3lzL2tlcm4va2Vybl9zaWcuYwkyMDEzLTA1LTA3IDE1OjU4OjM2LjAwMDA=
w
MDAwMCArMDIwMApAQCAtMjg2OCwxMyArMjg2OCwyMyBAQAogCQkgKi8KIAkJaWYgKGNvcmVkdW1=
w
KHRkKSA9PSAwKQogCQkJc2lnIHw9IFdDT1JFRkxBRzsKLQkJaWYgKGtlcm5fbG9nc2lnZXhpdCk=
K
LQkJCWxvZyhMT0dfSU5GTywKLQkJCSAgICAicGlkICVkICglcyksIHVpZCAlZDogZXhpdGVkIG9=
u
IHNpZ25hbCAlZCVzXG4iLAotCQkJICAgIHAtPnBfcGlkLCBwLT5wX2NvbW0sCi0JCQkgICAgdGQ=
t
PnRkX3VjcmVkID8gdGQtPnRkX3VjcmVkLT5jcl91aWQgOiAtMSwKLQkJCSAgICBzaWcgJn4gV0N=
P
UkVGTEFHLAotCQkJICAgIHNpZyAmIFdDT1JFRkxBRyA/ICIgKGNvcmUgZHVtcGVkKSIgOiAiIik=
7
CisJCWlmIChrZXJuX2xvZ3NpZ2V4aXQpIHsKKwkJICAJaWYgKHRkLT50ZF91Y3JlZCAmJiBqYWl=
s
ZWQodGQtPnRkX3VjcmVkKSkKKwkJCQlsb2coTE9HX0lORk8sCisJCQkJICAgICJwaWQgJWQgKCV=
z
KSwgdWlkICVkLCBqaWQgJWQ6IGV4aXRlZCBvbiBzaWduYWwgJWQlc1xuIiwKKwkJCQkgICAgcC0=
+
cF9waWQsIHAtPnBfY29tbSwKKwkJCQkgICAgdGQtPnRkX3VjcmVkLT5jcl91aWQsCisJCQkJICA=
g
IHRkLT50ZF91Y3JlZC0+Y3JfcHJpc29uLT5wcl9pZCwKKwkJCQkgICAgc2lnICZ+IFdDT1JFRkx=
B
RywKKwkJCQkgICAgc2lnICYgV0NPUkVGTEFHID8gIiAoY29yZSBkdW1wZWQpIiA6ICIiKTsKKwk=
J
CWVsc2UKKwkJCQlsb2coTE9HX0lORk8sCisJCQkJICAgICJwaWQgJWQgKCVzKSwgdWlkICVkOiB=
l
eGl0ZWQgb24gc2lnbmFsICVkJXNcbiIsCisJCQkJICAgIHAtPnBfcGlkLCBwLT5wX2NvbW0sCis=
J
CQkJICAgIHRkLT50ZF91Y3JlZCA/IHRkLT50ZF91Y3JlZC0+Y3JfdWlkIDogLTEsCisJCQkJICA=
g
IHNpZyAmfiBXQ09SRUZMQUcsCisJCQkJICAgIHNpZyAmIFdDT1JFRkxBRyA/ICIgKGNvcmUgZHV=
t
cGVkKSIgOiAiIik7CisJCX0KIAl9IGVsc2UKIAkJUFJPQ19VTkxPQ0socCk7CiAJZXhpdDEodGQ=
s
IFdfRVhJVENPREUoMCwgc2lnKSk7Cg=3D=3D
=3D=3D=3D=3D
Comment 2 Terje Elde 2016-01-05 18:52:13 UTC
This looks like a good idea to me, and people are in need of it as well:
https://twitter.com/tykling/status/684359369102868480
Comment 3 Thomas Steen Rasmussen / Tykling 2016-01-06 00:42:37 UTC
Created attachment 165129 [details]
updated patch against stable/10 r293209

The original attachment in this bug no longer works since it doesn't use the newish jailed() to check if the process is jailed. 

The attachment I'm adding is a patch against stable/10 base r293209 which fixes the jailed detection, and also adds jail hostname to the log entry (when a jail hostname is available).

For this patch to work on HEAD the exit1() call needs to be changed, no other changes needed I think.

This patch could be a few lines shorter by only having one log() call, which includes a empty string for nonjailed processes and something like ", jid: xxx (example.com)" for jailed processes. The current is arguably more readable though.

Testing with this patch applied:

-------------------------------------------------
[tykling@test /usr/src]$ sudo jail -c path=/ command=/bin/sh
# perl -e 'dump'
Abort trap (core dumped)
# ^Djail: /bin/sh: failed
[tykling@test /usr/src]$ sudo jail -c path=/ host.hostname=example.com command=/bin/sh
# perl -e 'dump'
Abort trap (core dumped)
# ^Djail: /bin/sh: failed
[tykling@test /usr/src]$ perl -e 'dump'
Abort trap
[tykling@test /usr/src]$ dmesg | tail -3
pid 847 (perl), uid 0, jid 3: exited on signal 6 (core dumped)
pid 853 (perl), uid 0, jid 4 (example.com): exited on signal 6 (core dumped)
pid 857 (perl), uid 1001: exited on signal 6
[tykling@test /usr/src]$
-------------------------------------------------

I am not good with C so please feel free to comment on style and other problems. Thanks :)
Comment 4 Mateusz Guzik freebsd_committer 2016-01-06 05:32:44 UTC
The feature is definitely desirable.

I would argue the complete solution would just support jail-aware dmesgs and print jail-specific stuff specific stuff with appropriate prefix to the 'main' dmesg. This would require some effort and may be off the table for now.

Regardless, it would be good if the new message here got the format one would expect to see in the more advanced case.

There is further issue with increased infoleaks - now not only you learn what segfaulting programs are being used by other jails, you get their (host)names.

Either way, the patch is wrong:

+			if (jailed(p->p_ucred)) {
+				char buf[MAXHOSTNAMELEN + 3];
+				if (strcmp(p->p_ucred->cr_prison->pr_hostname, "") != 0) {
+					sprintf(buf, " (%s)", p->p_ucred->cr_prison->pr_hostname);
+				} else {
+					*buf = '\0';
+				}

This should have used getcredhostname, assuming hostname is desirable. I would argue hostname is not the field you are interested in - after all, jail can change it. Instead, you should obtain jail name.

Also, this patch does not handle hierarchical jails.

+				log(LOG_INFO,
+				    "pid %d (%s), uid %d, jid %d%s: exited on signal %d%s\n",
+				    p->p_pid, p->p_comm,
+				    td->td_ucred->cr_uid,
+				    p->p_ucred->cr_prison->pr_id,
+				    buf,
+				    sig &~ WCOREFLAG,
+				    sig & WCOREFLAG ? " (core dumped)" : "");
+			} else {
+				log(LOG_INFO,
+				    "pid %d (%s), uid %d: exited on signal %d%s\n",
+				    p->p_pid, p->p_comm,
+				    td->td_ucred ? td->td_ucred->cr_uid : -1,
+				    sig &~ WCOREFLAG,
+				    sig & WCOREFLAG ? " (core dumped)" : "");
+			}

As a nit, just should have been handled with one log() call. Missing optional jail bit could be provided with a pointer to "".

That said, I would be in favor of messages like this one:
[$jailname] $msg

That is:
pid 857 (perl), uid 1001: exited on signal 6

is turned into:
[foo] pid 857 (perl), uid 1001: exited on signal 6

Assuming jail name is 'foo'. For hierarchical jails this would be [foo.bar].
Comment 5 Thomas Steen Rasmussen / Tykling 2016-01-06 09:00:05 UTC
Created attachment 165148 [details]
updated patch with jailname instead of hostname

Thanks for the review! I've updated the patch to:
- use the jailname instead of hostname
- leave out the jid
- show jail info in the beginning of the line

Testing now looks like this with the patch applied:
----------------------------------------------------
[tykling@test /usr/src]$ sudo jail -n testname -c path=/ command=/bin/sh
# perl -e 'dump'
Abort trap (core dumped)
# ^Djail: /bin/sh: failed
[tykling@test /usr/src]$ sudo jail -c path=/ command=/bin/sh
# perl -e 'dump'
Abort trap (core dumped)
# ^Djail: /bin/sh: failed
[tykling@test /usr/src]$ perl -e 'dump'
Abort trap
[tykling@test /usr/src]$ dmesg | tail -3
[testname] pid 823 (perl), uid 0: exited on signal 6 (core dumped)
[6] pid 827 (perl), uid 0: exited on signal 6 (core dumped)
pid 828 (perl), uid 1001: exited on signal 6
[tykling@test /usr/src]$
----------------------------------------------------

There doesn't seem to be a getcred* function to get the jail name, so I've left that code as is, but getting pr_name instead of pr_hostname.

Only thing missing is getting the parent jail names (recursively) where relevant. Do you happen to know if there is an existing function to do that, or do I have to make one and call it recursively, prepending to a . seperated string until pr_parent is null?
Comment 6 SimpleRezo 2016-07-06 09:01:57 UTC
Hi

Does the patch can be reviewed?
Because we still have the issue and would be great to see this MFCed ;)

Regards
Comment 7 Eitan Adler freebsd_committer freebsd_triage 2018-05-21 00:00:13 UTC
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"
Comment 8 Eitan Adler freebsd_committer freebsd_triage 2018-05-21 00:00:39 UTC
For bugs matching the following conditions:
- Status == In Progress
- Assignee == "bugs@FreeBSD.org"
- Last Modified Year <= 2017

Do
- Set Status to "Open"