Re: [systemd-devel] [PATCH] journald: add CAP_MAC_OVERRIDE in journald for SMACK issue

2014-09-11 Thread juho son 

On 04/09/14 01:58, Lennart Poettering wrote:

On Wed, 03.09.14 22:16, Juho Son (juho80@samsung.com) wrote:


systemd-journald check the cgroup id to support rate limit option for
every messages. so journald should be available to access cgroup node in
each process send messages to journald.
In system using SMACK, cgroup node in proc is assigned execute label
as each process's execute label.
so if journald don't want to denied for every process, journald
should have all of access rule for all process's label.
It's too heavy. so we could give special smack label for journald te get
all accesses's permission.
'^' label.
When assign '^' execute smack label to systemd-journald,
systemd-journald need to add  CAP_MAC_OVERRIDE capability to get that smack 
privilege.

so I want to notice this information and set default capability to
journald whether system use SMACK or not.


I have no idea about SMACK, hence I cannot really review the
patch. But if I get this right, then only SMACK makes use of
CAP_MAC_OVERRIDE, hence by adding the bit to journald we don't affect
anything but smack behaviour, right?

yes, CAP_MAC_OVERRIDE bit could affects only in smack enabled kernel.
when journald has that cap, journald could get the privilege of smack.
journald should get the all of logs in system based on systemd.


If that's the case then I am happy to apply the patch...



Change-Id: I52e47d6f9b631f365799bb51a66404cf3f1da12b
Signed-off-by: Juho Son juho80@samsung.com


We don't use S-O-b on systemd...

I will send again follow.



---
  units/systemd-journald.service.in | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/units/systemd-journald.service.in 
b/units/systemd-journald.service.in
index 7013979..4de38fa 100644
--- a/units/systemd-journald.service.in
+++ b/units/systemd-journald.service.in
@@ -20,7 +20,7 @@ Restart=always
  RestartSec=0
  NotifyAccess=all
  StandardOutput=null
-CapabilityBoundingSet=CAP_SYS_ADMIN CAP_DAC_OVERRIDE CAP_SYS_PTRACE CAP_SYSLOG 
CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SETUID CAP_SETGID
+CapabilityBoundingSet=CAP_SYS_ADMIN CAP_DAC_OVERRIDE CAP_SYS_PTRACE CAP_SYSLOG 
CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SETUID 
CAP_SETGID CAP_MAC_OVERRIDE
  WatchdogSec=1min

  # Increase the default a bit in order to allow many simultaneous
--
1.9.1

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel



Lennart



___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH v2] journald: add CAP_MAC_OVERRIDE in journald for SMACK issue

2014-09-11 Thread Juho Son 
systemd-journald check the cgroup id to support rate limit option for
every messages. so journald should be available to access cgroup node in
each process send messages to journald.
In system using SMACK, cgroup node in proc is assigned execute label
as each process's execute label.
so if journald don't want to denied for every process, journald
should have all of access rule for all process's label.
It's too heavy. so we could give special smack label for journald te get
all accesses's permission.
'^' label.
When assign '^' execute smack label to systemd-journald,
systemd-journald need to add  CAP_MAC_OVERRIDE capability to get that smack 
privilege.

so I want to notice this information and set default capability to
journald whether system use SMACK or not.
because that capability affect to only smack enabled kernel
---
 units/systemd-journald.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/units/systemd-journald.service.in 
b/units/systemd-journald.service.in
index 7013979..4de38fa 100644
--- a/units/systemd-journald.service.in
+++ b/units/systemd-journald.service.in
@@ -20,7 +20,7 @@ Restart=always
 RestartSec=0
 NotifyAccess=all
 StandardOutput=null
-CapabilityBoundingSet=CAP_SYS_ADMIN CAP_DAC_OVERRIDE CAP_SYS_PTRACE CAP_SYSLOG 
CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SETUID CAP_SETGID
+CapabilityBoundingSet=CAP_SYS_ADMIN CAP_DAC_OVERRIDE CAP_SYS_PTRACE CAP_SYSLOG 
CAP_AUDIT_CONTROL CAP_CHOWN CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SETUID 
CAP_SETGID CAP_MAC_OVERRIDE
 WatchdogSec=1min
 
 # Increase the default a bit in order to allow many simultaneous
-- 
1.9.1

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Preventing automatic seat assignments

2014-09-11 Thread Tanu Kaskinen 
On Wed, 2014-09-10 at 12:44 +0200, David Herrmann wrote:
 Hi
 
 On Tue, Sep 9, 2014 at 10:49 AM, Tanu Kaskinen
 tanu.kaski...@linux.intel.com wrote:
  On Wed, 2014-08-27 at 11:47 +0300, Tanu Kaskinen wrote:
  On Tue, 2014-08-26 at 14:00 +0200, Lennart Poettering wrote:
   On Tue, 26.08.14 12:17, Tanu Kaskinen (tanu.kaski...@linux.intel.com) 
   wrote:
  
Hi,
   
If I want to designate some sound card to be shared between seats,
then I suppose that sound card shouldn't be assigned to any seats.
However, currently /usr/lib/udev/rules.d/71-seat.rules
unconditionally tags all sound cards with the seat tag. How should
this be solved?
  
   What's the rationale here actually? PA doesn't really support sharing
   sound cards between multiple seats.
  
   I mean, If this is something generally useful we can see if we can
   support that in the default rules, but I am not seeing it?
 
  The use case is a car audio system. There are multiple seats, and each
  seat can have dedicated audio hardware (e.g. headphones), but there's
  also the speaker system that is shared by all seats.
 
  It's true that PA needs modifications too to support this. We haven't
  yet decided how to implement this, but probably we will run PA in system
  mode for the shared devices only, and user instances for the per-seat
  hardware. The user instances will use the tunnel module to connect to
  the hardware that is managed by the system instance.
 
  Ping?
 
 Sorry, a lot of people are on vacation..
 
 I think the right solution for this is to support TAGS-=seat in
 udev. That is, the automatic seat-assignment will still be applied,
 but you can revert it in your own udev rules. By dropping the seat
 tag, logind will not treat it as 'seated' device.

Thanks for the reply. The way you put it sounds like udev doesn't
currently support TAGS-=seat. Is that correct? If so, maybe I'll look
into making a patch at some point, but no promises.

-- 
Tanu

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [RFC] runtime configurable timer

2014-09-11 Thread WaLyong Cho 
(I will happy there is already similar method already exist.)

systemd already has similar functionality systemd-run but that is only
for scope or service unit. I think that is useful run a service without
unit file on permanent storage.

As a similar method, is it possible to generate or configure timer unit
on runtime?

Honestly, now, I need a runtime configurable timer interface. If systemd
has this then I can reduce one of daemon.


Thanks,
WaLyong
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] udev database backwards compatibility guarantees

2014-09-11 Thread Alexander Larsson 
Hi, I'm looking at creating a runtime/app thing for Gnome in the style
of:
http://0pointer.net/blog/revisiting-how-we-put-together-linux-systems.html

However, I noticed that some core dependencies like mesa uses libudev.
And in fact, needs user-set additional info not in sysfs. In particular,
it reads ID_PATH_TAG on render device nodes to pick what GPU to use in
multi-gpu situations (PRIME):
http://lists.freedesktop.org/archives/mesa-dev/2014-June/061798.html

It seems to me that this means I need the host /run/udev inside the
application. I know that the udev database format changed in the past,
but can I rely on it being stable in the future, even if the host udev
is rev:ed to a later version than what is in the application runtime?

Of course, there is also the question of /dev and /sys management in
sandboxed apps in general. Clearly any modern app will require some
real devices for things like direct rendering. But it would be ideal to
not expose everything. How do we see this working?

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] build-sys: make hibernation support configure option also handle hybrid-sleep; fix indentation

2014-09-11 Thread Ivan Shapovalov 
On Tuesday 09 September 2014 at 01:40:51, Ivan Shapovalov wrote:
 ---
 The patch by Umut did miss at least hybrid-sleep -- it involves hibernation
 as well (hybrid sleep is a hibernation followed by S3 rather than S4 
 powerdown).
 
 Also, it messed up indentation a bit (Makefile.am seems to use tabs), which I
 fixed as well.
 
 And I wonder, maybe it makes sense to conditionalize sleep (suspend) support 
 as well
 as hibernation? Or are there use-cases when suspend is possible, but not 
 hibernation?
 
  Makefile.am | 27 +--
  1 file changed, 13 insertions(+), 14 deletions(-)
 
 diff --git a/Makefile.am b/Makefile.am
 [...]

Ping.

-- 
Ivan Shapovalov / intelfx /

signature.asc
Description: This is a digitally signed message part.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] User systemd unit files

2014-09-11 Thread Colin Guthrie 
Chris Morgan wrote on 11/09/14 02:32:
 
 On Sep 10, 2014 5:46 PM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl mailto:zbys...@in.waw.pl wrote:

 On Tue, Sep 09, 2014 at 07:39:17PM -0400, Chris Morgan wrote:
Specifically, running `systemd --user` directly is not supported
anymore. The user mode still works, but only for one user
 instance
per UID, launched through user@uid.service (recent releases
 start
this automatically upon logging in).
   Try 'systemctl --user'. If this start you can put units
   it ~/.config/systemd/user/ or a similar path (check
 systemd.unit(5) out).
  
   Zbyszek
 
  I tried --user but I get some errors that I pasted above. It looks
  like it is no longer supported but I'm not sure.

 If you're getting the same message as above, you are still typing
 systemd instead of systemctl.

 Zbyszek
 
 Ahh. The man page seemed to indicate that one should run systemd with
 --user first.
 
 I was able to run unit files via systemctl --user but the
 SYSTEMD_USER_PATH environment variable doesn't seem to be working, even
 though it is mentioned in the man page. 

Where are you setting this path? If systemd --user is already running
(which happens automatically with newer systemd's) then you have to make
sure *it* knows about the env var, not just the client side calls in.

This *might* work, but I don't know for sure as I've not fiddled with
this stuff.

systemctl --user set-environment SYSTEMD_USER_PATH=$HOME/somegitrepo/units/
systemctl --user daemon-reload


Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] User systemd unit files

2014-09-11 Thread Chris Morgan 
On Thu, Sep 11, 2014 at 5:03 AM, Colin Guthrie gm...@colin.guthr.ie wrote:
 Chris Morgan wrote on 11/09/14 02:32:

 On Sep 10, 2014 5:46 PM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl mailto:zbys...@in.waw.pl wrote:

 On Tue, Sep 09, 2014 at 07:39:17PM -0400, Chris Morgan wrote:
Specifically, running `systemd --user` directly is not supported
anymore. The user mode still works, but only for one user
 instance
per UID, launched through user@uid.service (recent releases
 start
this automatically upon logging in).
   Try 'systemctl --user'. If this start you can put units
   it ~/.config/systemd/user/ or a similar path (check
 systemd.unit(5) out).
  
   Zbyszek
 
  I tried --user but I get some errors that I pasted above. It looks
  like it is no longer supported but I'm not sure.

 If you're getting the same message as above, you are still typing
 systemd instead of systemctl.

 Zbyszek

 Ahh. The man page seemed to indicate that one should run systemd with
 --user first.

 I was able to run unit files via systemctl --user but the
 SYSTEMD_USER_PATH environment variable doesn't seem to be working, even
 though it is mentioned in the man page.

 Where are you setting this path? If systemd --user is already running
 (which happens automatically with newer systemd's) then you have to make
 sure *it* knows about the env var, not just the client side calls in.

 This *might* work, but I don't know for sure as I've not fiddled with
 this stuff.

 systemctl --user set-environment SYSTEMD_USER_PATH=$HOME/somegitrepo/units/
 systemctl --user daemon-reload


 Col

 --

 Colin Guthrie
 gmane(at)colin.guthr.ie
 http://colin.guthr.ie/

 Day Job:
   Tribalogic Limited http://www.tribalogic.net/
 Open Source:
   Mageia Contributor http://www.mageia.org/
   PulseAudio Hacker http://www.pulseaudio.org/
   Trac Hacker http://trac.edgewall.org/


Hmm. I figured that the environment was used when the systemd user
instance was started.

I tried systemctl --user set-environment and it shows up if I use
show-environment but even if I use SYSTEMD_UNIT_PATH (I was typing on
mobile and mistakenly typed USER instead of UNIT), and reloaded the
daemon it doesn't appear to be taking effect, the test unit file I
created isn't located.

I'm thinking I can use the 'systemctl link' command to link the
scripts into the user folder. Basically after a build something like
startup_and_start.sh could link each unit file and then call 'start'
on them to start them up, simulating what would happen during system
startup on the embedded side.

Would need to build each unit file from a template as the full path
would be needed but that shouldn't be tough.

I appreciate the help from everyone here. It does look like
SYSTEMD_UNIT_PATH was removed from newer versions of systemd and only
XDG_xxxXXX is available, but that variable isn't mentioned at all in
the man pages of my version of systemd, 208. Some of the confusion is
between what systemd provides (systemd --user) and what, due to distro
integration I should be using (systemctl --user), and I'm new to
systemd.

Chris
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH RFC] udev: allow removing tags via TAG-=foobar

2014-09-11 Thread David Herrmann 
This extends the udev parser to support OP_REMOVE (-=) and adds support
for TAG-= to remove previously set tags. We don't fail if the tag didn't
exist.

This is pretty handy if we ship default rules for seat-assignments and
users want to exclude specific devices from that. They can easily add
rules that drop any automatically added seat tags again.

This patch is untested! Comments welcome.
---
 src/libudev/libudev-device.c  | 21 +++-
 src/libudev/libudev-private.h |  1 +
 src/udev/udev-rules.c | 76 +--
 3 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/src/libudev/libudev-device.c b/src/libudev/libudev-device.c
index f26a4c4..d61a2ad 100644
--- a/src/libudev/libudev-device.c
+++ b/src/libudev/libudev-device.c
@@ -1732,9 +1732,14 @@ void udev_device_set_is_initialized(struct udev_device 
*udev_device)
 udev_device-is_initialized = true;
 }
 
+static bool is_valid_tag(const char *tag)
+{
+return !strchr(tag, ':')  !strchr(tag, ' ');
+}
+
 int udev_device_add_tag(struct udev_device *udev_device, const char *tag)
 {
-if (strchr(tag, ':') != NULL || strchr(tag, ' ') != NULL)
+if (!is_valid_tag(tag))
 return -EINVAL;
 udev_device-tags_uptodate = false;
 if (udev_list_entry_add(udev_device-tags_list, tag, NULL) != NULL)
@@ -1742,6 +1747,20 @@ int udev_device_add_tag(struct udev_device *udev_device, 
const char *tag)
 return -ENOMEM;
 }
 
+void udev_device_remove_tag(struct udev_device *udev_device, const char *tag)
+{
+struct udev_list_entry *e;
+
+if (!is_valid_tag(tag))
+return;
+e = udev_list_get_entry(udev_device-tags_list);
+e = udev_list_entry_get_by_name(e, tag);
+if (e) {
+udev_device-tags_uptodate = false;
+udev_list_entry_delete(e);
+}
+}
+
 void udev_device_cleanup_tags_list(struct udev_device *udev_device)
 {
 udev_device-tags_uptodate = false;
diff --git a/src/libudev/libudev-private.h b/src/libudev/libudev-private.h
index ae97557..bc44bfb 100644
--- a/src/libudev/libudev-private.h
+++ b/src/libudev/libudev-private.h
@@ -73,6 +73,7 @@ const char *udev_device_get_devpath_old(struct udev_device 
*udev_device);
 const char *udev_device_get_id_filename(struct udev_device *udev_device);
 void udev_device_set_is_initialized(struct udev_device *udev_device);
 int udev_device_add_tag(struct udev_device *udev_device, const char *tag);
+void udev_device_remove_tag(struct udev_device *udev_device, const char *tag);
 void udev_device_cleanup_tags_list(struct udev_device *udev_device);
 usec_t udev_device_get_usec_initialized(struct udev_device *udev_device);
 void udev_device_set_usec_initialized(struct udev_device *udev_device, usec_t 
usec_initialized);
diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c
index cc56215..a2dca05 100644
--- a/src/udev/udev-rules.c
+++ b/src/udev/udev-rules.c
@@ -82,7 +82,7 @@ static unsigned int rules_add_string(struct udev_rules 
*rules, const char *s) {
 return strbuf_add_string(rules-strbuf, s, strlen(s));
 }
 
-/* KEY==, KEY!=, KEY+=, KEY=, KEY:= */
+/* KEY==, KEY!=, KEY+=, KEY-=, KEY=, KEY:= */
 enum operation_type {
 OP_UNSET,
 
@@ -91,6 +91,7 @@ enum operation_type {
 OP_MATCH_MAX,
 
 OP_ADD,
+OP_REMOVE,
 OP_ASSIGN,
 OP_ASSIGN_FINAL,
 };
@@ -224,6 +225,7 @@ static const char *operation_str(enum operation_type type) {
 [OP_MATCH_MAX] =MATCH_MAX,
 
 [OP_ADD] =  add,
+[OP_REMOVE] =   remove,
 [OP_ASSIGN] =   assign,
 [OP_ASSIGN_FINAL] = assign-final,
 };
@@ -785,6 +787,9 @@ static int get_key(struct udev *udev, char **line, char 
**key, enum operation_ty
 } else if (linepos[0] == '+'  linepos[1] == '=') {
 *op = OP_ADD;
 linepos += 2;
+} else if (linepos[0] == '-'  linepos[1] == '=') {
+*op = OP_REMOVE;
+linepos += 2;
 } else if (linepos[0] == '=') {
 *op = OP_ASSIGN;
 linepos++;
@@ -1121,6 +1126,10 @@ static int add_rule(struct udev_rules *rules, char *line,
 log_error(error parsing ATTR attribute);
 goto invalid;
 }
+if (op == OP_REMOVE) {
+log_error(invalid ATTR operation);
+goto invalid;
+}
 if (op  OP_MATCH_MAX) {
 rule_add_key(rule_tmp, TK_M_ATTR, op, value, 
attr);
 } else {
@@ -1135,6 +1144,10 @@ static int add_rule(struct udev_rules *rules, char *line,
 log_error(error parsing SECLABEL attribute);

Re: [systemd-devel] Preventing automatic seat assignments

2014-09-11 Thread David Herrmann 
Hi

On Thu, Sep 11, 2014 at 9:16 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
 On Wed, 2014-09-10 at 12:44 +0200, David Herrmann wrote:
 Hi

 On Tue, Sep 9, 2014 at 10:49 AM, Tanu Kaskinen
 tanu.kaski...@linux.intel.com wrote:
  On Wed, 2014-08-27 at 11:47 +0300, Tanu Kaskinen wrote:
  On Tue, 2014-08-26 at 14:00 +0200, Lennart Poettering wrote:
   On Tue, 26.08.14 12:17, Tanu Kaskinen (tanu.kaski...@linux.intel.com) 
   wrote:
  
Hi,
   
If I want to designate some sound card to be shared between seats,
then I suppose that sound card shouldn't be assigned to any seats.
However, currently /usr/lib/udev/rules.d/71-seat.rules
unconditionally tags all sound cards with the seat tag. How should
this be solved?
  
   What's the rationale here actually? PA doesn't really support sharing
   sound cards between multiple seats.
  
   I mean, If this is something generally useful we can see if we can
   support that in the default rules, but I am not seeing it?
 
  The use case is a car audio system. There are multiple seats, and each
  seat can have dedicated audio hardware (e.g. headphones), but there's
  also the speaker system that is shared by all seats.
 
  It's true that PA needs modifications too to support this. We haven't
  yet decided how to implement this, but probably we will run PA in system
  mode for the shared devices only, and user instances for the per-seat
  hardware. The user instances will use the tunnel module to connect to
  the hardware that is managed by the system instance.
 
  Ping?

 Sorry, a lot of people are on vacation..

 I think the right solution for this is to support TAGS-=seat in
 udev. That is, the automatic seat-assignment will still be applied,
 but you can revert it in your own udev rules. By dropping the seat
 tag, logind will not treat it as 'seated' device.

 Thanks for the reply. The way you put it sounds like udev doesn't
 currently support TAGS-=seat. Is that correct? If so, maybe I'll look
 into making a patch at some point, but no promises.

Exactly, it's currently not supported. I wrote a patch and sent it to the ML:
  [PATCH RFC] udev: allow removing tags via TAG-=foobar

Maybe you can have a look whether this solves your issues.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH RFC] udev: allow removing tags via TAG-=foobar

2014-09-11 Thread Michael Biebl 
2014-09-11 13:28 GMT+02:00 David Herrmann dh.herrm...@gmail.com:
 This patch is untested! Comments welcome.

Should probably be documented in man udev(7) as well.

-- 
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH RFC] udev: allow removing tags via TAG-=foobar

2014-09-11 Thread David Herrmann 
Hi

On Thu, Sep 11, 2014 at 2:15 PM, Michael Biebl mbi...@gmail.com wrote:
 2014-09-11 13:28 GMT+02:00 David Herrmann dh.herrm...@gmail.com:
 This patch is untested! Comments welcome.

 Should probably be documented in man udev(7) as well.

Indeed, now fixed.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH RFC] udev: allow removing tags via TAG-=foobar

2014-09-11 Thread David Herrmann 
Hi

On Thu, Sep 11, 2014 at 1:28 PM, David Herrmann dh.herrm...@gmail.com wrote:
 This extends the udev parser to support OP_REMOVE (-=) and adds support
 for TAG-= to remove previously set tags. We don't fail if the tag didn't
 exist.

 This is pretty handy if we ship default rules for seat-assignments and
 users want to exclude specific devices from that. They can easily add
 rules that drop any automatically added seat tags again.

 This patch is untested! Comments welcome.

I fixed up a bug in the udev-parser, tested the patch and pushed it upstream.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/5] [use after free] Avoid using m-kdbus after freeing it.

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 11:20 AM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 m-kdbus could be freed before it is released. Changing the
 order fixes the issue.

 Found with Coverity. Fixes: CID#1237798

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com

Few notes:
- no punctuation characters in commit-heads
- try to prefix commit-heads with the subsystem name, like bus: foo
bar bar bar
- no signed-off-by for systemd

I fixed those and applied the patch.

Thanks!
David

 ---
  src/libsystemd/sd-bus/bus-message.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

 diff --git a/src/libsystemd/sd-bus/bus-message.c 
 b/src/libsystemd/sd-bus/bus-message.c
 index d00455a..bfb14fc 100644
 --- a/src/libsystemd/sd-bus/bus-message.c
 +++ b/src/libsystemd/sd-bus/bus-message.c
 @@ -127,9 +127,6 @@ static void message_free(sd_bus_message *m) {

  message_reset_parts(m);

 -if (m-free_kdbus)
 -free(m-kdbus);
 -
  if (m-release_kdbus) {
  uint64_t off;

 @@ -137,6 +134,9 @@ static void message_free(sd_bus_message *m) {
  ioctl(m-bus-input_fd, KDBUS_CMD_FREE, off);
  }

 +if (m-free_kdbus)
 +free(m-kdbus);
 +
  sd_bus_unref(m-bus);

  if (m-free_fds) {
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/5] [use after free] pattern is already freed, so do not dereference it in the error print

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 11:20 AM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 In case set_consume goes wrong, the pattern name has already been
 freed. So we do not try to print it in the logs, assuming the pattern
 addition print will be printed just before the failure anyway. Found
 with coverity. Fixes: CID#1237798

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com

Applied!

Thanks
David

 ---
  src/journal/coredumpctl.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/src/journal/coredumpctl.c b/src/journal/coredumpctl.c
 index f5cf85a..4b12ec3 100644
 --- a/src/journal/coredumpctl.c
 +++ b/src/journal/coredumpctl.c
 @@ -110,8 +110,8 @@ static int add_match(Set *set, const char *match) {
  log_debug(Adding pattern: %s, pattern);
  r = set_consume(set, pattern);
  if (r  0) {
 -log_error(Failed to add pattern '%s': %s,
 -  pattern, strerror(-r));
 +log_error(Failed to add pattern : %s,
 +  strerror(-r));
  goto fail;
  }

 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 3/5] [uninitialized] No need to check if num is 0

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 11:20 AM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 When num is  0 we jump to the error handling. However at that time
 r is not set yet by keyboard_fill so we most likely get a nonsensical
 error. However the num check is not needed as the xkb_state_key_get_syms
 will not return negative values. From its documentation:
 @returns The number of keysyms in the syms_out array.  If no keysyms
  are produced by the key in the given keyboard state, returns 0 and sets
  syms_out to NULL.

I think it's totally legitimate for libxkbcommon to return -ENOMEM in
the future, in case a memory allocation is needed and fails. It's
unlikely, as the state should be pre-allocated to speed up runtime
keymap handling, but it's kinda nasty to silently ignore the error in
idev-keyboard.c

The uninitialized value warning for strerror() is right, though. I
fixed it by setting r = num before jumping to error.

Thanks
David

 Found with coverity. Fixes: CID#1237784

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com
 ---
  src/libsystemd-terminal/idev-keyboard.c | 3 ---
  1 file changed, 3 deletions(-)

 diff --git a/src/libsystemd-terminal/idev-keyboard.c 
 b/src/libsystemd-terminal/idev-keyboard.c
 index ab9e481..1eafbb1 100644
 --- a/src/libsystemd-terminal/idev-keyboard.c
 +++ b/src/libsystemd-terminal/idev-keyboard.c
 @@ -770,9 +770,6 @@ static int keyboard_feed_evdev(idev_keyboard *k, 
 idev_data *data) {
  /* TODO: update LEDs */
  }

 -if (num  0)
 -goto error;
 -
  r = keyboard_fill(k, k-evdata, data-resync, ev-code, ev-value, 
 num, keysyms);
  if (r  0)
  goto error;
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 4/5] [memleak] Do not leak mmapped area when other memory allocations fail.

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 11:20 AM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 After a section of memory is succesfully allocated, some of the following
 actions can still fail due to lack of memory. In this case -ENOMEM is
 returned without actually freeing the already mapped memory.
 Found with coverity. Fixes: CID#1237762

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com

Applied!

Thanks
David

 ---
  src/journal/mmap-cache.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

 diff --git a/src/journal/mmap-cache.c b/src/journal/mmap-cache.c
 index 7dbbb5e..908562d 100644
 --- a/src/journal/mmap-cache.c
 +++ b/src/journal/mmap-cache.c
 @@ -496,15 +496,15 @@ static int add_mmap(

  c = context_add(m, context);
  if (!c)
 -return -ENOMEM;
 +goto outofmem;

  f = fd_add(m, fd);
  if (!f)
 -return -ENOMEM;
 +goto outofmem;

  w = window_add(m);
  if (!w)
 -return -ENOMEM;
 +goto outofmem;

  w-keep_always = keep_always;
  w-ptr = d;
 @@ -522,6 +522,10 @@ static int add_mmap(
  if (ret)
  *ret = (uint8_t*) w-ptr + (offset - w-offset);
  return 1;
 +
 +outofmem:
 +munmap(d, wsize);
 +return -ENOMEM;
  }

  int mmap_cache_get(
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 5/5] [memleak] Actually unref the buscreds on failure.

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 11:20 AM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 Actually unref the buscreds when we are not going to return a
 pointer to them. As when bus_creds_add_more fails we immediately
 return the error code otherwise and leak the new buscreds.
 Found with coverity. Fixes: CID#1237761

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com

Applied!

Thanks
David

 ---
  src/libsystemd/sd-bus/sd-bus.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

 diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
 index 78e91b9..83b3aa1 100644
 --- a/src/libsystemd/sd-bus/sd-bus.c
 +++ b/src/libsystemd/sd-bus/sd-bus.c
 @@ -3339,8 +3339,10 @@ _public_ int sd_bus_get_peer_creds(sd_bus *bus, 
 uint64_t mask, sd_bus_creds **re
  }

  r = bus_creds_add_more(c, mask, pid, 0);
 -if (r  0)
 +if (r  0) {
 +sd_bus_creds_unref(c);
  return r;
 +}

  *ret = c;
  return 0;
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/3] [file handle leak] Close file handle when we're done with it

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 9:14 PM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 In test_read_one_char the filehandle does not get its fclose
 at the end of the function, thus we are leaking fd's.

 Found with Coverity. Fixes: CID#1237749

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com
 ---
  src/test/test-util.c | 1 +
  1 file changed, 1 insertion(+)

 diff --git a/src/test/test-util.c b/src/test/test-util.c
 index 72a8a6b..0d36711 100644
 --- a/src/test/test-util.c
 +++ b/src/test/test-util.c
 @@ -947,6 +947,7 @@ static void test_read_one_char(void) {
  assert_se(read_one_char(file, r, 100, need_nl)  0);

  unlink(name);
 +fclose(file);

This does not work. If you close file, you also close the underlying
fd. Therefore, the _cleanup_close_ on fd will close it again. This
is fine here as we're single-threaded, but we should do it right. I
fixed it up by using _cleanup_fclose_.

Thanks
David

  }

  static void test_ignore_signals(void) {
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 3/3] [fd leak] Stop leaking fd in do_accept()

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 9:14 PM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 Found with Coverity.

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com

Looks good, applied!

Thanks
David

 ---
  src/activate/activate.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/activate/activate.c b/src/activate/activate.c
 index 8942773..0a1df37 100644
 --- a/src/activate/activate.c
 +++ b/src/activate/activate.c
 @@ -242,7 +242,7 @@ static int launch1(const char* child, char** argv, char 
 **env, int fd) {

  static int do_accept(const char* name, char **argv, char **envp, int fd) {
  _cleanup_free_ char *local = NULL, *peer = NULL;
 -int fd2;
 +_cleanup_close_ int fd2 = -1;

  fd2 = accept(fd, NULL, NULL);
  if (fd2  0) {
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/3] [fd leak] Stop leaking an fd in sd_journal_sendv

2014-09-11 Thread David Herrmann 
Hi

On Wed, Sep 10, 2014 at 9:14 PM,  philippedesw...@gmail.com wrote:
 From: Philippe De Swert philippedesw...@gmail.com

 Found with Coverity. Fixes: CID#996435

 Signed-off-by: Philippe De Swert philippedesw...@gmail.com
 ---
  src/journal/journal-send.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/journal/journal-send.c b/src/journal/journal-send.c
 index bb1ef66..b7cc4bd 100644
 --- a/src/journal/journal-send.c
 +++ b/src/journal/journal-send.c
 @@ -198,7 +198,7 @@ finish:

  _public_ int sd_journal_sendv(const struct iovec *iov, int n) {
  PROTECT_ERRNO;
 -int fd;
 +_cleanup_close_ int fd = -1;

This does not work. fd is used to hold the journal fd, but this is a
global fd shared between all callers. See journal_fd().
coverity might complain about this as we never close the fd. However,
that is totally fine as it will get closed on execve() automatically.
There is no reason to close it manually.

Thanks
David

  _cleanup_close_ int buffer_fd = -1;
  struct iovec *w;
  uint64_t *l;
 --
 1.8.3.2

 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC] runtime configurable timer

2014-09-11 Thread David Herrmann 
Hi

On Thu, Sep 11, 2014 at 10:45 AM, WaLyong Cho walyong@samsung.com wrote:
 (I will happy there is already similar method already exist.)

 systemd already has similar functionality systemd-run but that is only
 for scope or service unit. I think that is useful run a service without
 unit file on permanent storage.

 As a similar method, is it possible to generate or configure timer unit
 on runtime?

 Honestly, now, I need a runtime configurable timer interface. If systemd
 has this then I can reduce one of daemon.

Something like at(1)? That is, executing a command at a later time?

Currently, timer units do not support transient units. So to implement
something like systemd-at, you'd first have to add support for that,
then you can make systemd-run do this. I don't think anyone has worked
on this so far.

Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Suppressing automounting

2014-09-11 Thread Colin Guthrie 
Dale R. Worley wrote on 10/09/14 20:56:
 From: Mantas Mikulėnas graw...@gmail.com
 
 What I was thinking of is, what is the program that reads (directly or
 indirectly) the Store.mount file and from that decides exactly how to
 call mount(8), and when to call it?

 It's systemd itself (pid 1).

 My guess was that the name of this program would be listed *in* the
 *.mount file, but that does not appear to be so.

 That would not make any sense: if the program wasn't running yet, how
 could it possibly start itself in order to read the .mount file which
 just tells it to start itself?
 
 In many systems, there's a framework program that reads a lot of
 control files which describe the bits of work to be done.  The
 framework program figures out the sequencing and dependencies of the
 various bits of work, while there are separate programs that *execute*
 the bits of work.  And the names of the execution programs are not
 hard-coded within the framework program, they are specified in the
 control files.  That sort of structure makes the framework program
 simpler, and enforces a defined interface between the framework
 program and the execution programs.  It also makes it easy to add new
 execution programs without changing the framework program.

I'm maybe missing something, but in the case of mount units, isn't that
framework program mount(8)?

It has a mechanism for parsing default options that apply to all mounts
and then calling out to the appropriate, filesystem specific mount
program (e.g. mount.nfs, mount.cifs, mount.crypt, mount.fuse etc. etc.)
if appropriate.

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Suppressing automounting

2014-09-11 Thread Dale R. Worley 
 From: Colin Guthrie gm...@colin.guthr.ie

 I'm maybe missing something, but in the case of mount units, isn't that
 framework program mount(8)?
 
 It has a mechanism for parsing default options that apply to all mounts
 and then calling out to the appropriate, filesystem specific mount
 program (e.g. mount.nfs, mount.cifs, mount.crypt, mount.fuse etc. etc.)
 if appropriate.

mount does the real work, of course, but it isn't the complete
solution, because *something* has to figure out not only that
/bin/mount should be invoked, but what its arguments are.  And as you
can see from the unit file

# Automatically generated by systemd-fstab-generator

[Unit]
SourcePath=/etc/fstab
DefaultDependencies=no
After=local-fs-pre.target
Conflicts=umount.target
Before=umount.target

[Mount]
What=/dev/Freeze02/Store2
Where=/Store
Type=ext4
FsckPassNo=0
Options=nofail,x-systemd.device-timeout=1m,defaults

none of that is specified *directly* in the unit file.  Some piece of
code has to know to assemble the What, Where, Type, and Options values
(at the very least) -- and that piece of code is what contains special
knowledge of how to handle mount units.

Dale
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Suppressing automounting

2014-09-11 Thread Tobias Geerinckx-Rice 
Hallo,

On 11 September 2014 19:41, Dale R. Worley wor...@alum.mit.edu wrote:
  From: Colin Guthrie gm...@colin.guthr.ie
  I'm maybe missing something, but in the case of mount units, isn't that
  framework program mount(8)?
 
  It has a mechanism for parsing default options that apply to all mounts
  and then calling out to the appropriate, filesystem specific mount
  program (e.g. mount.nfs, mount.cifs, mount.crypt, mount.fuse etc. etc.)
  if appropriate.

 mount does the real work, of course, but it isn't the complete
 solution,

Er, solution to what, exactly? People here seem (rightly)
unconvinced there's any problem at all.

And yes, calling mount -- by definition -- actually is the complete
solution to mounting a file system under Linux.

 because *something* has to figure out not only that
 /bin/mount should be invoked, but what its arguments are.

 And as you can see from the unit file

[SNIP]
 What=/dev/Freeze02/Store2
 Where=/Store
 Type=ext4
[SNIP]
 Options=nofail,x-systemd.device-timeout=1m,defaults

 none of that is specified *directly* in the unit file.

Now surely, calling

/usr/bin/mount $What $Where [-t $Type] [-o $Options]

is about as direct as it gets. Being able to substitute the only
constant in that equation with /usr/bin/gimp really isn't going to
solve any problem, real or imagined.

 Some piece of code has to know to assemble the What, Where, Type,
 and Options values (at the very least) -- and that piece of code is what
 contains special knowledge of how to handle mount units.

Again: straightforward use of a standard Unix API that just happens to
be in a separate binary != special knowledge. Really. It's
programming.

Step back, and define exactly what it is you actually need^Wwant to do.

From my reading of the thread, this is to emulate as closely ye olde
initscripts' unreliable and flawed behaviour of attempting to mount
one or more devices exactly once (i.e., a one-shot mount -a), but
not until an arbitrary time-out has elapsed after which all external
devices are blindly assumed to have been initialised for no good
reason.

This isn't hard to achieve with systemd, but there's a good reason I
wrote it in such a silly manner, that can't simply be ignored.

Regards (and good luck nonetheless),

T G-R
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] udev: warn instead of killing kmod loading

2014-09-11 Thread Tom Gundersen 
On Mon, Aug 11, 2014 at 7:19 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 On Mon, Aug 11, 2014 at 12:57 PM, Lennart Poettering
 lenn...@poettering.net wrote:
 On Mon, 11.08.14 18:39, Luis R. Rodriguez (mcg...@suse.com) wrote:

  This looks really wrong. We shouldn't permit worker processes to be
  blocked indefinitely without any timeout applied. Designing a worker
  process system like that is simply wrong. It's one thing to allow
  changing the specific timeout applied, it's a very different thing to
  allow broken drivers to completely stall the worker process logic.

 OK what if we enable customizations then on the timeout by the built-in
 cmd type and we use a high multiplier for now for kmod ? A multiplier
 for kmod of 10 would set the kmod timeout to 5 minutes then, as we
 sweep up and clean drivers we can reduce this over time. This would also
 enable us to keep the default timeout for the other type of workers.

 Why this complexity?

 I mean, it sounds much simpler to simply increase the default timeout a
 bit, if it turns out to be too low for the current cases...

 True, there's two things here and one of which this v2 patch didn't address:

 1) It'd be good for defaults on systemd to work on most systems based
 on upstream kernels today, right now folks deploying systemd would
 need to modify the default timeout. Are we up to bump the default up
 considerably? If its high, would that be unfair for classes of workers
 we know shouldn't take that long, or wouldn't that allow folks
 developing new workers to take longer?

 2) We want chatty logs to allow us to keep track of drivers that need
 attention. Ideally right now we should strive for 30 seconds init and
 work on asynching most work, we want to do this in a non fatal way.
 Overriding the timeout won't let us to keep track of buggy drivers
 that need love from systemd's perspective to stay within the 30 second
 bound time. We can have chatty logs from the kernel but using a
 timeout on the driver core seems a bit overkill specially if systemd
 is already keeping track of driver's init time, so it'd be better if
 we could collect offending drivers from systemd. I could have
 implemented support for this in this v2 patch by simply adding another
 check using the default timeout.

Hi Luis,

Just following up on this old thread: I have now bumped the default
kill timeout (for all workers) to 180 seconds, and added a warning
which is triggered after a third of the timeout.

Let me know if this covers what you need, or if there is anything else
we should do on the systemd side.

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] udev: warn instead of killing kmod loading

2014-09-11 Thread Luis R. Rodriguez 
On Thu, Sep 11, 2014 at 1:07 PM, Tom Gundersen t...@jklm.no wrote:
 On Mon, Aug 11, 2014 at 7:19 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 On Mon, Aug 11, 2014 at 12:57 PM, Lennart Poettering
 lenn...@poettering.net wrote:
 On Mon, 11.08.14 18:39, Luis R. Rodriguez (mcg...@suse.com) wrote:

  This looks really wrong. We shouldn't permit worker processes to be
  blocked indefinitely without any timeout applied. Designing a worker
  process system like that is simply wrong. It's one thing to allow
  changing the specific timeout applied, it's a very different thing to
  allow broken drivers to completely stall the worker process logic.

 OK what if we enable customizations then on the timeout by the built-in
 cmd type and we use a high multiplier for now for kmod ? A multiplier
 for kmod of 10 would set the kmod timeout to 5 minutes then, as we
 sweep up and clean drivers we can reduce this over time. This would also
 enable us to keep the default timeout for the other type of workers.

 Why this complexity?

 I mean, it sounds much simpler to simply increase the default timeout a
 bit, if it turns out to be too low for the current cases...

 True, there's two things here and one of which this v2 patch didn't address:

 1) It'd be good for defaults on systemd to work on most systems based
 on upstream kernels today, right now folks deploying systemd would
 need to modify the default timeout. Are we up to bump the default up
 considerably? If its high, would that be unfair for classes of workers
 we know shouldn't take that long, or wouldn't that allow folks
 developing new workers to take longer?

 2) We want chatty logs to allow us to keep track of drivers that need
 attention. Ideally right now we should strive for 30 seconds init and
 work on asynching most work, we want to do this in a non fatal way.
 Overriding the timeout won't let us to keep track of buggy drivers
 that need love from systemd's perspective to stay within the 30 second
 bound time. We can have chatty logs from the kernel but using a
 timeout on the driver core seems a bit overkill specially if systemd
 is already keeping track of driver's init time, so it'd be better if
 we could collect offending drivers from systemd. I could have
 implemented support for this in this v2 patch by simply adding another
 check using the default timeout.

 Hi Luis,

 Just following up on this old thread: I have now bumped the default
 kill timeout (for all workers) to 180 seconds,

Thanks Tom, this should help quite a lot while we try to get in the
kernel what systemd had assumed was there which is async probe for all
drivers. I'll poke back once we have that option available on the
kernel and that should help mitigate other issues I suppose.

 and added a warning
 which is triggered after a third of the timeout.

This is great! What commit merged this? I just looked at the latest
commits and couldn't find one associated with this. I also checked the
mailing list for pending patches. Also what would we use to hunt for
these? Would they show up on:

journalctl -b -0 -u systemd-udevd

I ask as when I tried to add something similar I was unable to capture
anything with the above. We'd use this to then monitor for these prior
to assuming other issues then.

 Let me know if this covers what you need, or if there is anything else
 we should do on the systemd side.

Thanks! I realize that at this point perhaps removing the sigkill may
not be possible at all given that this is part of an original design
consideration but if we can at least address all other possible
concerns this would be great. I know for instance James has concerns
with any arbitrary timeout in general, but perhaps he can elaborate
more on that. Also Jiri made this one point about reboot and the
timeout:

 This aproach is actually introducing a user-visible regressions. Look, for
 example, exec() never times out. Therefore if your system is on its knees,
 heavily overloaded (or completely broken), you are likely to be able to
 `reboot' it, because exec(/sbin/reboot) ultimately succeeds.

 But with all the timeouts, dbus, Failed to issue method call: Did
 not receive a reply messages, this is getting close to impossible.

Thanks again.

  Luis
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM

2014-09-11 Thread Tom Gundersen 
On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
mcg...@do-not-panic.com wrote:
 More than two years
 have gone by on growing design and assumptions on top of that original
 commit. I'm not sure if *systemd folks* yet believe its was a design
 regression?

 I don't think so. udev should not allow its workers to run for an
 unbounded length of time. Whether the upper bound should be 30, 60,
 180 seconds or something else is up for debate (currently it is 60,
 but if that is too short for some drivers we could certainly revisit
 that).

 That's the thing -- the timeout was put in place under the assumption
 probe was asyncronous and its not, the driver core issues both module
 init *and* probe together, the loader has to wait. That alone makes
 the timeout a design flaw, and then systemd carried on top of that
 design over two years after that. Its not systemd's fault, its just
 that we never spoke about this as a design thing broadly and we should
 have, and I will mention that even when the first issues creeped up,
 the issue was still tossed back a driver problems. It was only until
 recently that we realized that both init and probe run together that
 we've been thinking about this problem differently. Systemd was trying
 to ensure init on driver don't take long but its not init that is
 taking long, its probe, and probe gets then penalized as the driver
 core batches both init and probe synchronously before finishing module
 loading.

Just to clarify: udev/systemd is not trying to get into the business
of what the kernel does on finit_module(), we just need to make sure
that none of our workers stay around forever, which is why we have a
global timeout. If necessary we can bump this higher (as mentioned in
another thread I just bumped it to 180 secs), but we cannot abolish it
entirely.

 Furthermore as clarified by Tejun random userland is known to
 exist that will wait indefinitely for module loading under the simple
 assumption things *are done synchronously*, and its precisely why we
 can't just blindly enable async probe upstream through a new driver
 boolean as it can be unfair to this old userland. What is being
 evaluated is to enable aync probe for *all* drivers through a new
 general system-wide option. We cannot regress old userspace and
 assumptions but we can create a new shiny universe.

How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.

 Moreover, it seems from this discussion that the aim is (still)
 that insmod should be near-instantaneous (i.e., not wait for probe),

 The only reason that is being discussed is that systemd has not
 accepted the timeout as a system design despite me pointing out the
 original design flaw recently and at this point even if was accepted
 as a design flaw it seems its too late. The approach taken to help
 make all drivers async is simply an afterthought to give systemd what
 it *thought* was in place, and it by no measure should be considered
 the proper fix to the regression introduced by systemd, it may perhaps
 be the right step long term for systemd systems given it goes with
 what it assumed was there, but the timeout was flawed. Its not clear
 if systemd can help with old kernels, it seems the ship has sailed and
 there seems no options but for folks to work around that -- unless of
 course some reasonable solution is found which doesn't break current
 systemd design?

If I read the git logs correctly the hard timeout was introduced in
April 2011, so reverting it now seems indeed not to help much with all
the running systems out there.

 As part of this series I addressed hunting for the  misbehaving
 drivers in-kernel as I saw no progress on the systemd side of things
 to non-fatally detect misbehaving drivers despite my original RFCs
 and request for advice. I quote  misbehaving drivers as its a flawed
 view to consider them misbehaving now in light of clarifications of
 how the driver core works in that it batches both init and probe
 together always and we can't be penalizing long probes due to the fact
 long probes are simply fine. My patch to pick up misbehaving drivers
 drivers on the kernel front by picking up systemd's signal was
 reactive but it was also simply braindead given the same exact reasons
 why systemd's original timeout was flawed. We want a general solution
 and we don't want to work around the root cause, in this case it was
 systemd's assumption on how drivers work.

Would your ongoing work to make probing asynchronous solve this
problem in the long-term? In the short-term I guess bumping the udev
timeout should be sufficient.

 Keep in mind that the above just addresses kmod built-in cmd on
 systemd, its where the timeout was introduced but as has been
 clarified here assuming the same timeout on *all* other built-in
 likely is likely pretty flawed as

Re: [systemd-devel] [PATCH] udev: warn instead of killing kmod loading

2014-09-11 Thread Tom Gundersen 
On Thu, Sep 11, 2014 at 11:02 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 On Thu, Sep 11, 2014 at 1:07 PM, Tom Gundersen t...@jklm.no wrote:
 On Mon, Aug 11, 2014 at 7:19 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 On Mon, Aug 11, 2014 at 12:57 PM, Lennart Poettering
 lenn...@poettering.net wrote:
 On Mon, 11.08.14 18:39, Luis R. Rodriguez (mcg...@suse.com) wrote:

  This looks really wrong. We shouldn't permit worker processes to be
  blocked indefinitely without any timeout applied. Designing a worker
  process system like that is simply wrong. It's one thing to allow
  changing the specific timeout applied, it's a very different thing to
  allow broken drivers to completely stall the worker process logic.

 OK what if we enable customizations then on the timeout by the built-in
 cmd type and we use a high multiplier for now for kmod ? A multiplier
 for kmod of 10 would set the kmod timeout to 5 minutes then, as we
 sweep up and clean drivers we can reduce this over time. This would also
 enable us to keep the default timeout for the other type of workers.

 Why this complexity?

 I mean, it sounds much simpler to simply increase the default timeout a
 bit, if it turns out to be too low for the current cases...

 True, there's two things here and one of which this v2 patch didn't address:

 1) It'd be good for defaults on systemd to work on most systems based
 on upstream kernels today, right now folks deploying systemd would
 need to modify the default timeout. Are we up to bump the default up
 considerably? If its high, would that be unfair for classes of workers
 we know shouldn't take that long, or wouldn't that allow folks
 developing new workers to take longer?

 2) We want chatty logs to allow us to keep track of drivers that need
 attention. Ideally right now we should strive for 30 seconds init and
 work on asynching most work, we want to do this in a non fatal way.
 Overriding the timeout won't let us to keep track of buggy drivers
 that need love from systemd's perspective to stay within the 30 second
 bound time. We can have chatty logs from the kernel but using a
 timeout on the driver core seems a bit overkill specially if systemd
 is already keeping track of driver's init time, so it'd be better if
 we could collect offending drivers from systemd. I could have
 implemented support for this in this v2 patch by simply adding another
 check using the default timeout.

 Hi Luis,

 Just following up on this old thread: I have now bumped the default
 kill timeout (for all workers) to 180 seconds,

 Thanks Tom, this should help quite a lot while we try to get in the
 kernel what systemd had assumed was there which is async probe for all
 drivers. I'll poke back once we have that option available on the
 kernel and that should help mitigate other issues I suppose.

Great!

 and added a warning
 which is triggered after a third of the timeout.

 This is great! What commit merged this?I just looked at the latest
 commits and couldn't find one associated with this. I also checked the
 mailing list for pending patches.

Oops, my push failed. Just pushed now:
http://cgit.freedesktop.org/systemd/systemd/commit/?id=671174136525ddf208cdbe75d6d6bd159afa961f.

 Also what would we use to hunt for
 these? Would they show up on:

 journalctl -b -0 -u systemd-udevd

Precisely. Generated by making processing hang for all network devices:

Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [406]
/devices/virtual/net/lo is taking a long time
Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [407]
/devices/pci:00/:00:1c.1/:03:00.0/net/wlan0 is taking a
long time
Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [411]
/devices/pci:00/:00:19.0/net/enp0s25 is taking a long time
Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [412]
/devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0 is taking
a long time
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [406]
/devices/virtual/net/lo timeout; kill it
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2214
'/devices/virtual/net/lo' killed
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [407]
/devices/pci:00/:00:1c.1/:03:00.0/net/wlan0 timeout; kill
it
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2502
'/devices/pci:00/:00:1c.1/:03:00.0/net/wlan0' killed
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [411]
/devices/pci:00/:00:19.0/net/enp0s25 timeout; kill it
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2015
'/devices/pci:00/:00:19.0/net/enp0s25' killed
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [412]
/devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0 timeout;
kill it
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2455
'/devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0' killed
Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [406]
terminated by signal 9 (Killed)
Sep 11 21:28:58 tomegun-x240

Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM

2014-09-11 Thread Luis R. Rodriguez 
On Thu, Sep 11, 2014 at 2:43 PM, Tom Gundersen t...@jklm.no wrote:
 On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
 mcg...@do-not-panic.com wrote:
 More than two years
 have gone by on growing design and assumptions on top of that original
 commit. I'm not sure if *systemd folks* yet believe its was a design
 regression?

 I don't think so. udev should not allow its workers to run for an
 unbounded length of time. Whether the upper bound should be 30, 60,
 180 seconds or something else is up for debate (currently it is 60,
 but if that is too short for some drivers we could certainly revisit
 that).

 That's the thing -- the timeout was put in place under the assumption
 probe was asyncronous and its not, the driver core issues both module
 init *and* probe together, the loader has to wait. That alone makes
 the timeout a design flaw, and then systemd carried on top of that
 design over two years after that. Its not systemd's fault, its just
 that we never spoke about this as a design thing broadly and we should
 have, and I will mention that even when the first issues creeped up,
 the issue was still tossed back a driver problems. It was only until
 recently that we realized that both init and probe run together that
 we've been thinking about this problem differently. Systemd was trying
 to ensure init on driver don't take long but its not init that is
 taking long, its probe, and probe gets then penalized as the driver
 core batches both init and probe synchronously before finishing module
 loading.

 Just to clarify: udev/systemd is not trying to get into the business
 of what the kernel does on finit_module(), we just need to make sure
 that none of our workers stay around forever, which is why we have a
 global timeout. If necessary we can bump this higher (as mentioned in
 another thread I just bumped it to 180 secs), but we cannot abolish it
 entirely.

180 seconds is certainly better than 30, but let me be clear here on
the extent to which the timeout at least for kmod built-in command can
be an issue. The driver core not only batches init and probe together
synchronously, it also runs probe for *all* devices that the device
driver can claim and all those series of probes run synchronously
within itself, that is bus_for_each_dev() runs synchronously on each
device. So, if a init takes 1 second and probe for each device takes
120 seconds and the system has 2 devices with the new timeout the
second device would not be successfully probed (and in fact I'm not
sure if this would kill the first).

 Furthermore as clarified by Tejun random userland is known to
 exist that will wait indefinitely for module loading under the simple
 assumption things *are done synchronously*, and its precisely why we
 can't just blindly enable async probe upstream through a new driver
 boolean as it can be unfair to this old userland. What is being
 evaluated is to enable aync probe for *all* drivers through a new
 general system-wide option. We cannot regress old userspace and
 assumptions but we can create a new shiny universe.

 How about simply introducing a new flag to finit_module() to indicate
 that the caller does not care about asynchronicity. We could then pass
 this from udev, but existing scripts calling modprobe/insmod will not
 be affected.

Do you mean that you *do want asynchronicity*?

 Moreover, it seems from this discussion that the aim is (still)
 that insmod should be near-instantaneous (i.e., not wait for probe),

 The only reason that is being discussed is that systemd has not
 accepted the timeout as a system design despite me pointing out the
 original design flaw recently and at this point even if was accepted
 as a design flaw it seems its too late. The approach taken to help
 make all drivers async is simply an afterthought to give systemd what
 it *thought* was in place, and it by no measure should be considered
 the proper fix to the regression introduced by systemd, it may perhaps
 be the right step long term for systemd systems given it goes with
 what it assumed was there, but the timeout was flawed. Its not clear
 if systemd can help with old kernels, it seems the ship has sailed and
 there seems no options but for folks to work around that -- unless of
 course some reasonable solution is found which doesn't break current
 systemd design?

 If I read the git logs correctly the hard timeout was introduced in
 April 2011, so reverting it now seems indeed not to help much with all
 the running systems out there.

yeah figured :(

 As part of this series I addressed hunting for the  misbehaving
 drivers in-kernel as I saw no progress on the systemd side of things
 to non-fatally detect misbehaving drivers despite my original RFCs
 and request for advice. I quote  misbehaving drivers as its a flawed
 view to consider them misbehaving now in light of clarifications of
 how the driver core works in that it batches both init and probe
 together always and we can't be penalizing long probes

Re: [systemd-devel] [PATCH] udev: warn instead of killing kmod loading

2014-09-11 Thread Luis R. Rodriguez 
On Thu, Sep 11, 2014 at 11:50:19PM +0200, Tom Gundersen wrote:
 On Thu, Sep 11, 2014 at 11:02 PM, Luis R. Rodriguez mcg...@suse.com wrote:
  and added a warning
  which is triggered after a third of the timeout.
 
  This is great! What commit merged this?I just looked at the latest
  commits and couldn't find one associated with this. I also checked the
  mailing list for pending patches.
 
 Oops, my push failed. Just pushed now:
 http://cgit.freedesktop.org/systemd/systemd/commit/?id=671174136525ddf208cdbe75d6d6bd159afa961f.

Looks super sexy to me, thanks!

  Also what would we use to hunt for
  these? Would they show up on:
 
  journalctl -b -0 -u systemd-udevd
 
 Precisely. Generated by making processing hang for all network devices:
 
 Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [406]
 /devices/virtual/net/lo is taking a long time
 Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [407]
 /devices/pci:00/:00:1c.1/:03:00.0/net/wlan0 is taking a
 long time
 Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [411]
 /devices/pci:00/:00:19.0/net/enp0s25 is taking a long time
 Sep 11 21:26:58 tomegun-x240 systemd-udevd[404]: worker [412]
 /devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0 is taking
 a long time
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [406]
 /devices/virtual/net/lo timeout; kill it
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2214
 '/devices/virtual/net/lo' killed
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [407]
 /devices/pci:00/:00:1c.1/:03:00.0/net/wlan0 timeout; kill
 it
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2502
 '/devices/pci:00/:00:1c.1/:03:00.0/net/wlan0' killed
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [411]
 /devices/pci:00/:00:19.0/net/enp0s25 timeout; kill it
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2015
 '/devices/pci:00/:00:19.0/net/enp0s25' killed
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [412]
 /devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0 timeout;
 kill it
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: seq 2455
 '/devices/pci:00/:00:14.0/usb2/2-4/2-4:1.6/net/wwan0' killed
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [406]
 terminated by signal 9 (Killed)
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [407]
 terminated by signal 9 (Killed)
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [411]
 terminated by signal 9 (Killed)
 Sep 11 21:28:58 tomegun-x240 systemd-udevd[404]: worker [412]
 terminated by signal 9 (Killed)

Exactly what we needed thanks so much!

  Luis
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [RFC v2 3/6] kthread: warn on kill signal if not OOM

2014-09-11 Thread Tom Gundersen 
On Fri, Sep 12, 2014 at 12:26 AM, Luis R. Rodriguez
mcg...@do-not-panic.com wrote:
 On Thu, Sep 11, 2014 at 2:43 PM, Tom Gundersen t...@jklm.no wrote:
 How about simply introducing a new flag to finit_module() to indicate
 that the caller does not care about asynchronicity. We could then pass
 this from udev, but existing scripts calling modprobe/insmod will not
 be affected.

 Do you mean that you *do want asynchronicity*?

Precisely, udev would opt-in, but existing scripts etc would not.

 But isn't finit_module() taking a long time a serious problem given
 that it means no other module can be loaded in parallel?

 Indeed but having a desire to make the init() complete fast is
 different than the desire to have the combination of both init and
 probe fast synchronously.

I guess no one is arguing that probe should somehow be required to be
fast, but rather:

 If userspace wants init to be fast and let
 probe be async then userspace has no option but to deal with the fact
 that async probe will be async, and it should then use other methods
 to match any dependencies if its doing that itself.

Correct. And this therefore likely needs to be opt-in behaviour per
finit_module() invocation to avoid breaking old assumptions.

 For example
 networking should not kick off after a network driver is loaded but
 rather one the device creeps up on udev. We should be good with
 networking dealing with this correctly today but not sure about other
 subsystems. depmod should be able to load the required modules in
 order and if bus drivers work right then probe of the remnant devices
 should happen asynchronously. The one case I can think of that is a
 bit different is modules-load.d things but those *do not rely on the
 timeout*, but are loaded prior to a service requirement. Note though
 that if those modules had probe and they then run async'd then systemd
 service would probably need to consider that the requirements may not
 be there until later. If this is not carefully considered that could
 introduce regression to users of modules-load.d when async probe is
 fully deployed. The same applies to systemd making assumptions of kmod
 loading a module and a dependency being complete as probe would have
 run it before.

Yeah, these all needs to be considered when deciding whether or not to
enable async in each specific case.

 I believe one concern here lies in on whether or not userspace
 is properly equipped to deal with the requirements on module loading
 doing async probing and that possibly failing. Perhaps systemd might
 think all userspace is ready for that but are we sure that's the case?

There almost certainly are custom things out there relying on the
synchronous behaviour, but if we make it opt-in we should not have a
problem.

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel