From 73484ecff90f2cc235d827c0e955999bffe64dd0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 26 Aug 2020 18:47:11 +0200 Subject: [PATCH 1/9] test-functions: make sure we test our own libudev instead of the host libudev When invoking "ldd" to find dependency libraries we already set $LD_LIBRARY_PATH to point to our own build tree, so that our libraries are checked, not the host libraries. This is not sufficient howeever, as libudev is built in a subdir. Add that, too. --- test/test-functions | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-functions b/test/test-functions index a93bba4b07..9893864bcd 100644 --- a/test/test-functions +++ b/test/test-functions @@ -673,7 +673,7 @@ get_ldpath() { install_missing_libraries() { # install possible missing libraries for i in $initdir{,/usr}/{sbin,bin}/* $initdir{,/usr}/lib/systemd/{,tests/{,manual/,unsafe/}}*; do - LD_LIBRARY_PATH="${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(get_ldpath $i)" inst_libs $i + LD_LIBRARY_PATH="${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}$(get_ldpath $i):$(get_ldpath $i)/src/udev" inst_libs $i done } From e77b146f825ef1bb63c297cc713962b94422d2c6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 17:55:14 +0100 Subject: [PATCH 2/9] udev: make tags "sticky" This tries to address the "bind"/"unbind" uevent kernel API breakage, by changing the semantics of device tags. Previously, tags would be applied on uevents (and the database entries they result in) only depending on the immediate context. This means that if one uevent causes the tag to be set and the next to be unset, this would immediately effect what apps would see and the database entries would contain each time. This is problematic however, as tags are a filtering concept, and if tags vanish then clients won't hence notice when a device stops being relevant to them since not only the tags disappear but immediately also the uevents for it are filtered including the one necessary for the app to notice that the device lost its tag and hence relevance. With this change tags become "sticky". If a tag is applied is once applied to a device it will stay in place forever, until the device is removed. Tags can never be removed again. This means that an app watching a specific set of devices by filtering for a tag is guaranteed to not only see the events where the tag is set but also all follow-up events where the tags might be removed again. This change of behaviour is unfortunate, but is required due to the kernel introducing new "bind" and "unbind" uevents that generally have the effect that tags and properties disappear and apps hence don't notice when a device looses relevance to it. "bind"/"unbind" events were introduced in kernel 4.12, and are now used in more and more subsystems. The introduction broke userspace widely, and this commit is an attempt to provide a way for apps to deal with it. While tags are now "sticky" a new automatic device property CURRENT_TAGS is introduced (matching the existing TAGS property) that always reflects the precise set of tags applied on the most recent events. Thus, when subscribing to devices through tags, all devices that ever had the tag put on them will be be seen, and by CURRENT_TAGS it may be checked whether the device right at the moment matches the tag requirements. See: #7587 #7018 #8221 --- src/libsystemd/libsystemd.sym | 4 + src/libsystemd/sd-device/device-internal.h | 8 +- src/libsystemd/sd-device/device-private.c | 21 ++-- src/libsystemd/sd-device/device-private.h | 2 +- src/libsystemd/sd-device/device-util.h | 5 + src/libsystemd/sd-device/sd-device.c | 135 ++++++++++++++++----- src/systemd/sd-device.h | 3 + src/udev/udev-event.c | 22 ++++ src/udev/udev-rules.c | 2 +- 9 files changed, 160 insertions(+), 42 deletions(-) diff --git a/src/libsystemd/libsystemd.sym b/src/libsystemd/libsystemd.sym index a3659d00b3..1bee5318d6 100644 --- a/src/libsystemd/libsystemd.sym +++ b/src/libsystemd/libsystemd.sym @@ -728,4 +728,8 @@ global: sd_event_source_set_time_relative; sd_bus_error_has_names_sentinel; + + sd_device_get_current_tag_first; + sd_device_get_current_tag_next; + sd_device_has_current_tag; } LIBSYSTEMD_246; diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index 1fe5c1a6bf..9c6b8a345f 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -28,10 +28,10 @@ struct sd_device { Set *sysattrs; /* names of sysattrs */ Iterator sysattrs_iterator; - Set *tags; - Iterator tags_iterator; + Set *all_tags, *current_tags; + Iterator all_tags_iterator, current_tags_iterator; + uint64_t all_tags_iterator_generation, current_tags_iterator_generation; /* generation when iteration was started */ uint64_t tags_generation; /* changes whenever the tags are changed */ - uint64_t tags_iterator_generation; /* generation when iteration was started */ Set *devlinks; Iterator devlinks_iterator; @@ -71,7 +71,7 @@ struct sd_device { bool parent_set:1; /* no need to try to reload parent */ bool sysattrs_read:1; /* don't try to re-read sysattrs once read */ - bool property_tags_outdated:1; /* need to update TAGS= property */ + bool property_tags_outdated:1; /* need to update TAGS= or CURRENT_TAGS= property */ bool property_devlinks_outdated:1; /* need to update DEVLINKS= property */ bool properties_buf_outdated:1; /* need to reread hashmap */ bool sysname_set:1; /* don't reread sysname */ diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 1e61732dfe..1ad7713ec7 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -329,7 +329,7 @@ static int device_amend(sd_device *device, const char *key, const char *value) { if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to add devlink '%s': %m", devlink); } - } else if (streq(key, "TAGS")) { + } else if (STR_IN_SET(key, "TAGS", "CURRENT_TAGS")) { const char *word, *state; size_t l; @@ -339,10 +339,11 @@ static int device_amend(sd_device *device, const char *key, const char *value) { (void) strncpy(tag, word, l); tag[l] = '\0'; - r = device_add_tag(device, tag); + r = device_add_tag(device, tag, streq(key, "CURRENT_TAGS")); if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to add tag '%s': %m", tag); } + } else { r = device_add_property_internal(device, key, value); if (r < 0) @@ -759,8 +760,8 @@ int device_copy_properties(sd_device *device_dst, sd_device *device_src) { void device_cleanup_tags(sd_device *device) { assert(device); - set_free_free(device->tags); - device->tags = NULL; + device->all_tags = set_free_free(device->all_tags); + device->current_tags = set_free_free(device->current_tags); device->property_tags_outdated = true; device->tags_generation++; } @@ -778,7 +779,7 @@ void device_remove_tag(sd_device *device, const char *tag) { assert(device); assert(tag); - free(set_remove(device->tags, tag)); + free(set_remove(device->current_tags, tag)); device->property_tags_outdated = true; device->tags_generation++; } @@ -846,7 +847,10 @@ static bool device_has_info(sd_device *device) { if (!ordered_hashmap_isempty(device->properties_db)) return true; - if (!set_isempty(device->tags)) + if (!set_isempty(device->all_tags)) + return true; + + if (!set_isempty(device->current_tags)) return true; if (device->watch_handle >= 0) @@ -939,7 +943,10 @@ int device_update_db(sd_device *device) { fprintf(f, "E:%s=%s\n", property, value); FOREACH_DEVICE_TAG(device, tag) - fprintf(f, "G:%s\n", tag); + fprintf(f, "G:%s\n", tag); /* Any tag */ + + SET_FOREACH(tag, device->current_tags, i) + fprintf(f, "Q:%s\n", tag); /* Current tag */ } r = fflush_and_check(f); diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 2bde53e969..1f1c4ca107 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -45,7 +45,7 @@ void device_set_devlink_priority(sd_device *device, int priority); int device_ensure_usec_initialized(sd_device *device, sd_device *device_old); int device_add_devlink(sd_device *device, const char *devlink); int device_add_property(sd_device *device, const char *property, const char *value); -int device_add_tag(sd_device *device, const char *tag); +int device_add_tag(sd_device *device, const char *tag, bool both); void device_remove_tag(sd_device *device, const char *tag); void device_cleanup_tags(sd_device *device); void device_cleanup_devlinks(sd_device *device); diff --git a/src/libsystemd/sd-device/device-util.h b/src/libsystemd/sd-device/device-util.h index 1a1795d974..eda0f2f49e 100644 --- a/src/libsystemd/sd-device/device-util.h +++ b/src/libsystemd/sd-device/device-util.h @@ -11,6 +11,11 @@ tag; \ tag = sd_device_get_tag_next(device)) +#define FOREACH_DEVICE_CURRENT_TAG(device, tag) \ + for (tag = sd_device_get_current_tag_first(device); \ + tag; \ + tag = sd_device_get_current_tag_next(device)) + #define FOREACH_DEVICE_SYSATTR(device, attr) \ for (attr = sd_device_get_sysattr_first(device); \ attr; \ diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 3bba17aff8..3041ce2e9c 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -69,7 +69,8 @@ static sd_device *device_free(sd_device *device) { ordered_hashmap_free_free_free(device->properties_db); hashmap_free_free_free(device->sysattr_values); set_free(device->sysattrs); - set_free(device->tags); + set_free(device->all_tags); + set_free(device->current_tags); set_free(device->devlinks); return mfree(device); @@ -1062,8 +1063,8 @@ static bool is_valid_tag(const char *tag) { return !strchr(tag, ':') && !strchr(tag, ' '); } -int device_add_tag(sd_device *device, const char *tag) { - int r; +int device_add_tag(sd_device *device, const char *tag, bool both) { + int r, added; assert(device); assert(tag); @@ -1071,9 +1072,21 @@ int device_add_tag(sd_device *device, const char *tag) { if (!is_valid_tag(tag)) return -EINVAL; - r = set_put_strdup(&device->tags, tag); - if (r < 0) - return r; + /* Definitely add to the "all" list of tags (i.e. the sticky list) */ + added = set_put_strdup(&device->all_tags, tag); + if (added < 0) + return added; + + /* And optionally, also add it to the current list of tags */ + if (both) { + r = set_put_strdup(&device->current_tags, tag); + if (r < 0) { + if (added > 0) + (void) set_remove(device->all_tags, tag); + + return r; + } + } device->tags_generation++; device->property_tags_outdated = true; @@ -1151,8 +1164,9 @@ static int handle_db_line(sd_device *device, char key, const char *value) { assert(value); switch (key) { - case 'G': - r = device_add_tag(device, value); + case 'G': /* Any tag */ + case 'Q': /* Current tag */ + r = device_add_tag(device, value, key == 'Q'); if (r < 0) return r; @@ -1407,10 +1421,10 @@ _public_ const char *sd_device_get_tag_first(sd_device *device) { (void) device_read_db(device); - device->tags_iterator_generation = device->tags_generation; - device->tags_iterator = ITERATOR_FIRST; + device->all_tags_iterator_generation = device->tags_generation; + device->all_tags_iterator = ITERATOR_FIRST; - (void) set_iterate(device->tags, &device->tags_iterator, &v); + (void) set_iterate(device->all_tags, &device->all_tags_iterator, &v); return v; } @@ -1421,10 +1435,38 @@ _public_ const char *sd_device_get_tag_next(sd_device *device) { (void) device_read_db(device); - if (device->tags_iterator_generation != device->tags_generation) + if (device->all_tags_iterator_generation != device->tags_generation) return NULL; - (void) set_iterate(device->tags, &device->tags_iterator, &v); + (void) set_iterate(device->all_tags, &device->all_tags_iterator, &v); + return v; +} + +_public_ const char *sd_device_get_current_tag_first(sd_device *device) { + void *v; + + assert_return(device, NULL); + + (void) device_read_db(device); + + device->current_tags_iterator_generation = device->tags_generation; + device->current_tags_iterator = ITERATOR_FIRST; + + (void) set_iterate(device->current_tags, &device->current_tags_iterator, &v); + return v; +} + +_public_ const char *sd_device_get_current_tag_next(sd_device *device) { + void *v; + + assert_return(device, NULL); + + (void) device_read_db(device); + + if (device->current_tags_iterator_generation != device->tags_generation) + return NULL; + + (void) set_iterate(device->current_tags, &device->current_tags_iterator, &v); return v; } @@ -1456,6 +1498,31 @@ _public_ const char *sd_device_get_devlink_next(sd_device *device) { return v; } +static char *join_string_set(Set *s) { + size_t ret_allocated = 0, ret_len; + _cleanup_free_ char *ret = NULL; + const char *tag; + Iterator i; + + if (!GREEDY_REALLOC(ret, ret_allocated, 2)) + return NULL; + + strcpy(ret, ":"); + ret_len = 1; + + SET_FOREACH(tag, s, i) { + char *e; + + if (!GREEDY_REALLOC(ret, ret_allocated, ret_len + strlen(tag) + 2)) + return NULL; + + e = stpcpy(stpcpy(ret + ret_len, tag), ":"); + ret_len = e - ret; + } + + return TAKE_PTR(ret); +} + int device_properties_prepare(sd_device *device) { int r; @@ -1494,26 +1561,27 @@ int device_properties_prepare(sd_device *device) { if (device->property_tags_outdated) { _cleanup_free_ char *tags = NULL; - size_t tags_allocated = 0, tags_len = 0; - const char *tag; - if (!GREEDY_REALLOC(tags, tags_allocated, 2)) + tags = join_string_set(device->all_tags); + if (!tags) return -ENOMEM; - stpcpy(tags, ":"); - tags_len++; - for (tag = sd_device_get_tag_first(device); tag; tag = sd_device_get_tag_next(device)) { - char *e; - - if (!GREEDY_REALLOC(tags, tags_allocated, tags_len + strlen(tag) + 2)) - return -ENOMEM; - e = stpcpy(stpcpy(tags + tags_len, tag), ":"); - tags_len = e - tags; + if (!streq(tags, ":")) { + r = device_add_property_internal(device, "TAGS", tags); + if (r < 0) + return r; } - r = device_add_property_internal(device, "TAGS", tags); - if (r < 0) - return r; + free(tags); + tags = join_string_set(device->current_tags); + if (!tags) + return -ENOMEM; + + if (!streq(tags, ":")) { + r = device_add_property_internal(device, "CURRENT_TAGS", tags); + if (r < 0) + return r; + } device->property_tags_outdated = false; } @@ -1689,7 +1757,16 @@ _public_ int sd_device_has_tag(sd_device *device, const char *tag) { (void) device_read_db(device); - return !!set_contains(device->tags, tag); + return set_contains(device->all_tags, tag); +} + +_public_ int sd_device_has_current_tag(sd_device *device, const char *tag) { + assert_return(device, -EINVAL); + assert_return(tag, -EINVAL); + + (void) device_read_db(device); + + return set_contains(device->current_tags, tag); } _public_ int sd_device_get_property_value(sd_device *device, const char *key, const char **_value) { diff --git a/src/systemd/sd-device.h b/src/systemd/sd-device.h index 3c5c88c56b..d720ce50da 100644 --- a/src/systemd/sd-device.h +++ b/src/systemd/sd-device.h @@ -64,6 +64,8 @@ int sd_device_get_usec_since_initialized(sd_device *device, uint64_t *usec); const char *sd_device_get_tag_first(sd_device *device); const char *sd_device_get_tag_next(sd_device *device); +const char *sd_device_get_current_tag_first(sd_device *device); +const char *sd_device_get_current_tag_next(sd_device *device); const char *sd_device_get_devlink_first(sd_device *device); const char *sd_device_get_devlink_next(sd_device *device); const char *sd_device_get_property_first(sd_device *device, const char **value); @@ -72,6 +74,7 @@ const char *sd_device_get_sysattr_first(sd_device *device); const char *sd_device_get_sysattr_next(sd_device *device); int sd_device_has_tag(sd_device *device, const char *tag); +int sd_device_has_current_tag(sd_device *device, const char *tag); int sd_device_get_property_value(sd_device *device, const char *key, const char **value); int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **_value); diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index e1c2baf7f2..e1daac21ed 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -958,6 +958,24 @@ static int udev_event_on_move(UdevEvent *event) { return 0; } +static int copy_all_tags(sd_device *d, sd_device *s) { + const char *tag; + int r; + + assert(d); + + if (!s) + return 0; + + for (tag = sd_device_get_tag_first(s); tag; tag = sd_device_get_tag_next(s)) { + r = device_add_tag(d, tag, false); + if (r < 0) + return r; + } + + return 0; +} + int udev_event_execute_rules(UdevEvent *event, usec_t timeout_usec, int timeout_signal, @@ -990,6 +1008,10 @@ int udev_event_execute_rules(UdevEvent *event, if (r < 0) return log_device_debug_errno(dev, r, "Failed to clone sd_device object: %m"); + r = copy_all_tags(dev, event->dev_db_clone); + if (r < 0) + log_device_warning_errno(dev, r, "Failed to copy all tags from old database entry, ignoring: %m"); + if (sd_device_get_devnum(dev, NULL) >= 0) /* Disable watch during event processing. */ (void) udev_watch_end(event->dev_db_clone); diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index eb28431325..018478c986 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -2017,7 +2017,7 @@ static int udev_rule_apply_token_to_event( if (token->op == OP_REMOVE) device_remove_tag(dev, buf); else { - r = device_add_tag(dev, buf); + r = device_add_tag(dev, buf, true); if (r < 0) return log_rule_error_errno(dev, rules, r, "Failed to add tag '%s': %m", buf); } From 3b684be04b95bd5264dff0f2af343c52b4dba86b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 18:08:45 +0100 Subject: [PATCH 3/9] libudev: also expose API to check for current tags in libudev --- meson.build | 2 +- src/libudev/libudev-device.c | 62 +++++++++++++++++++++++++++--------- src/libudev/libudev.h | 2 ++ src/libudev/libudev.sym | 6 ++++ 4 files changed, 56 insertions(+), 16 deletions(-) diff --git a/meson.build b/meson.build index 6837540313..217b17423b 100644 --- a/meson.build +++ b/meson.build @@ -14,7 +14,7 @@ project('systemd', 'c', ) libsystemd_version = '0.29.0' -libudev_version = '1.6.18' +libudev_version = '1.7.0' # We need the same data in two different formats, ugh! # Also, for hysterical reasons, we use different variable diff --git a/src/libudev/libudev-device.c b/src/libudev/libudev-device.c index b993309911..704a09d01c 100644 --- a/src/libudev/libudev-device.c +++ b/src/libudev/libudev-device.c @@ -56,12 +56,13 @@ struct udev_device { struct udev_list *properties; uint64_t properties_generation; - struct udev_list *tags; - uint64_t tags_generation; + struct udev_list *all_tags, *current_tags; + uint64_t all_tags_generation, current_tags_generation; struct udev_list *devlinks; uint64_t devlinks_generation; bool properties_read:1; - bool tags_read:1; + bool all_tags_read:1; + bool current_tags_read:1; bool devlinks_read:1; struct udev_list *sysattrs; bool sysattrs_read; @@ -199,7 +200,7 @@ _public_ const char *udev_device_get_property_value(struct udev_device *udev_dev } struct udev_device *udev_device_new(struct udev *udev, sd_device *device) { - _cleanup_(udev_list_freep) struct udev_list *properties = NULL, *tags = NULL, *sysattrs = NULL, *devlinks = NULL; + _cleanup_(udev_list_freep) struct udev_list *properties = NULL, *all_tags = NULL, *current_tags = NULL, *sysattrs = NULL, *devlinks = NULL; struct udev_device *udev_device; assert(device); @@ -207,8 +208,11 @@ struct udev_device *udev_device_new(struct udev *udev, sd_device *device) { properties = udev_list_new(true); if (!properties) return_with_errno(NULL, ENOMEM); - tags = udev_list_new(true); - if (!tags) + all_tags = udev_list_new(true); + if (!all_tags) + return_with_errno(NULL, ENOMEM); + current_tags = udev_list_new(true); + if (!current_tags) return_with_errno(NULL, ENOMEM); sysattrs = udev_list_new(true); if (!sysattrs) @@ -226,7 +230,8 @@ struct udev_device *udev_device_new(struct udev *udev, sd_device *device) { .udev = udev, .device = sd_device_ref(device), .properties = TAKE_PTR(properties), - .tags = TAKE_PTR(tags), + .all_tags = TAKE_PTR(all_tags), + .current_tags = TAKE_PTR(current_tags), .sysattrs = TAKE_PTR(sysattrs), .devlinks = TAKE_PTR(devlinks), }; @@ -475,7 +480,8 @@ static struct udev_device *udev_device_free(struct udev_device *udev_device) { udev_list_free(udev_device->properties); udev_list_free(udev_device->sysattrs); - udev_list_free(udev_device->tags); + udev_list_free(udev_device->all_tags); + udev_list_free(udev_device->current_tags); udev_list_free(udev_device->devlinks); return mfree(udev_device); @@ -834,21 +840,41 @@ _public_ int udev_device_get_is_initialized(struct udev_device *udev_device) { _public_ struct udev_list_entry *udev_device_get_tags_list_entry(struct udev_device *udev_device) { assert_return_errno(udev_device, NULL, EINVAL); - if (device_get_tags_generation(udev_device->device) != udev_device->tags_generation || - !udev_device->tags_read) { + if (device_get_tags_generation(udev_device->device) != udev_device->all_tags_generation || + !udev_device->all_tags_read) { const char *tag; - udev_list_cleanup(udev_device->tags); + udev_list_cleanup(udev_device->all_tags); FOREACH_DEVICE_TAG(udev_device->device, tag) - if (!udev_list_entry_add(udev_device->tags, tag, NULL)) + if (!udev_list_entry_add(udev_device->all_tags, tag, NULL)) return_with_errno(NULL, ENOMEM); - udev_device->tags_read = true; - udev_device->tags_generation = device_get_tags_generation(udev_device->device); + udev_device->all_tags_read = true; + udev_device->all_tags_generation = device_get_tags_generation(udev_device->device); } - return udev_list_get_entry(udev_device->tags); + return udev_list_get_entry(udev_device->all_tags); +} + +_public_ struct udev_list_entry *udev_device_get_current_tags_list_entry(struct udev_device *udev_device) { + assert_return_errno(udev_device, NULL, EINVAL); + + if (device_get_tags_generation(udev_device->device) != udev_device->current_tags_generation || + !udev_device->current_tags_read) { + const char *tag; + + udev_list_cleanup(udev_device->current_tags); + + FOREACH_DEVICE_CURRENT_TAG(udev_device->device, tag) + if (!udev_list_entry_add(udev_device->current_tags, tag, NULL)) + return_with_errno(NULL, ENOMEM); + + udev_device->current_tags_read = true; + udev_device->current_tags_generation = device_get_tags_generation(udev_device->device); + } + + return udev_list_get_entry(udev_device->current_tags); } /** @@ -866,6 +892,12 @@ _public_ int udev_device_has_tag(struct udev_device *udev_device, const char *ta return sd_device_has_tag(udev_device->device, tag) > 0; } +_public_ int udev_device_has_current_tag(struct udev_device *udev_device, const char *tag) { + assert_return(udev_device, 0); + + return sd_device_has_current_tag(udev_device->device, tag) > 0; +} + sd_device *udev_device_get_sd_device(struct udev_device *udev_device) { assert(udev_device); diff --git a/src/libudev/libudev.h b/src/libudev/libudev.h index 02c2e5e8ed..c9d0bf233e 100644 --- a/src/libudev/libudev.h +++ b/src/libudev/libudev.h @@ -82,6 +82,7 @@ int udev_device_get_is_initialized(struct udev_device *udev_device); struct udev_list_entry *udev_device_get_devlinks_list_entry(struct udev_device *udev_device); struct udev_list_entry *udev_device_get_properties_list_entry(struct udev_device *udev_device); struct udev_list_entry *udev_device_get_tags_list_entry(struct udev_device *udev_device); +struct udev_list_entry *udev_device_get_current_tags_list_entry(struct udev_device *udev_device); struct udev_list_entry *udev_device_get_sysattr_list_entry(struct udev_device *udev_device); const char *udev_device_get_property_value(struct udev_device *udev_device, const char *key); const char *udev_device_get_driver(struct udev_device *udev_device); @@ -92,6 +93,7 @@ unsigned long long int udev_device_get_usec_since_initialized(struct udev_device const char *udev_device_get_sysattr_value(struct udev_device *udev_device, const char *sysattr); int udev_device_set_sysattr_value(struct udev_device *udev_device, const char *sysattr, const char *value); int udev_device_has_tag(struct udev_device *udev_device, const char *tag); +int udev_device_has_current_tag(struct udev_device *udev_device, const char *tag); /* * udev_monitor diff --git a/src/libudev/libudev.sym b/src/libudev/libudev.sym index fb2e03e432..bad8313904 100644 --- a/src/libudev/libudev.sym +++ b/src/libudev/libudev.sym @@ -118,3 +118,9 @@ global: udev_queue_flush; udev_queue_get_fd; } LIBUDEV_199; + +LIBUDEV_247 { +global: + udev_device_has_current_tag; + udev_device_get_current_tags_list_entry; +} LIBUDEV_215; From 31abedbb0325dee21cfdd9ec398918e85739c67e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 17:22:01 +0100 Subject: [PATCH 4/9] test: add test for new "sticky" tags logic --- test/TEST-55-UDEV-TAGS/Makefile | 1 + test/TEST-55-UDEV-TAGS/test.sh | 8 ++++ test/units/testsuite-55.service | 7 ++++ test/units/testsuite-55.sh | 66 +++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+) create mode 120000 test/TEST-55-UDEV-TAGS/Makefile create mode 100755 test/TEST-55-UDEV-TAGS/test.sh create mode 100644 test/units/testsuite-55.service create mode 100755 test/units/testsuite-55.sh diff --git a/test/TEST-55-UDEV-TAGS/Makefile b/test/TEST-55-UDEV-TAGS/Makefile new file mode 120000 index 0000000000..e9f93b1104 --- /dev/null +++ b/test/TEST-55-UDEV-TAGS/Makefile @@ -0,0 +1 @@ +../TEST-01-BASIC/Makefile \ No newline at end of file diff --git a/test/TEST-55-UDEV-TAGS/test.sh b/test/TEST-55-UDEV-TAGS/test.sh new file mode 100755 index 0000000000..325d70f011 --- /dev/null +++ b/test/TEST-55-UDEV-TAGS/test.sh @@ -0,0 +1,8 @@ +#!/usr/bin/env bash +set -e +TEST_DESCRIPTION="UDEV tags management" +TEST_NO_NSPAWN=1 + +. $TEST_BASE_DIR/test-functions + +do_test "$@" 55 diff --git a/test/units/testsuite-55.service b/test/units/testsuite-55.service new file mode 100644 index 0000000000..2127a4db7f --- /dev/null +++ b/test/units/testsuite-55.service @@ -0,0 +1,7 @@ +[Unit] +Description=TESTSUITE-55-UDEV-TAGS + +[Service] +ExecStartPre=rm -f /failed /testok +ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh +Type=oneshot diff --git a/test/units/testsuite-55.sh b/test/units/testsuite-55.sh new file mode 100755 index 0000000000..ffceefb6a5 --- /dev/null +++ b/test/units/testsuite-55.sh @@ -0,0 +1,66 @@ +#!/bin/bash +set -ex +set -o pipefail + +mkdir -p /run/udev/rules.d/ + +! test -f /run/udev/tags/added/c1:3 && + ! test -f /run/udev/tags/changed/c1:3 && + udevadm info /dev/null | grep -q -v 'E: TAGS=.*:added:.*' && + udevadm info /dev/null | grep -q -v 'E: CURRENT_TAGS=.*:added:.*' && + udevadm info /dev/null | grep -q -v 'E: TAGS=.*:changed:.*' && + udevadm info /dev/null | grep -q -v 'E: CURRENT_TAGS=.*:changed:.*' + +cat > /run/udev/rules.d/50-testsuite.rules < /testok + +exit 0 From 242c1c075aa284c8a8657c5aca36147f528146ba Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 18:32:03 +0100 Subject: [PATCH 5/9] core: make sure to recheck current udev tag "systemd" before considering a device ready Let's ensure that a device once tagged can become active/inactive simply by toggling the current tag. Note that this means that a device once tagged with "systemd" will always have a matching .device unit. However, the active/inactive state of the unit reflects whether it is currently tagged that way (and doesn't have SYSTEMD_READY=0 set). Fixes: #7587 --- src/core/device.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/device.c b/src/core/device.c index 5b8134159a..53bc549bd3 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -736,6 +736,10 @@ static bool device_is_ready(sd_device *dev) { if (device_is_renaming(dev) > 0) return false; + /* Is it really tagged as 'systemd' right now? */ + if (sd_device_has_current_tag(dev, "systemd") <= 0) + return false; + if (sd_device_get_property_value(dev, "SYSTEMD_READY", &ready) < 0) return true; From fccb48b286dcfc07f86d3e376829086294dfd978 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 18:35:06 +0100 Subject: [PATCH 6/9] logind: only apply ACLs for device currently tagged with "uaccess" This is about security, hence let's be particularly careful here: only devices currenlty tagged with "uaccess" will get ACL management, and it's not sufficient if they once were (though that is used for filtering). --- src/login/logind-acl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/login/logind-acl.c b/src/login/logind-acl.c index 76af208af1..5b75d8f362 100644 --- a/src/login/logind-acl.c +++ b/src/login/logind-acl.c @@ -195,6 +195,10 @@ int devnode_acl_all(const char *seat, FOREACH_DEVICE(e, d) { const char *node, *sn; + /* Make sure the tag is still in place */ + if (sd_device_has_current_tag(d, "uaccess") <= 0) + continue; + if (sd_device_get_property_value(d, "ID_SEAT", &sn) < 0 || isempty(sn)) sn = "seat0"; From 643bb9240832c238a28cb36efc9d1a6c8d7e6ea0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 18:36:57 +0100 Subject: [PATCH 7/9] logind: always check current tag list before using a device --- src/login/logind-core.c | 8 +++++--- src/login/logind-dbus.c | 2 +- src/login/sysfs-show.c | 6 +++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/login/logind-core.c b/src/login/logind-core.c index e0d61f3e75..0487182225 100644 --- a/src/login/logind-core.c +++ b/src/login/logind-core.c @@ -243,7 +243,8 @@ int manager_process_seat_device(Manager *m, sd_device *d) { assert(m); - if (device_for_action(d, DEVICE_ACTION_REMOVE)) { + if (device_for_action(d, DEVICE_ACTION_REMOVE) || + sd_device_has_current_tag(d, "seat") <= 0) { const char *syspath; r = sd_device_get_syspath(d, &syspath); @@ -271,7 +272,7 @@ int manager_process_seat_device(Manager *m, sd_device *d) { } seat = hashmap_get(m->seats, sn); - master = sd_device_has_tag(d, "master-of-seat") > 0; + master = sd_device_has_current_tag(d, "master-of-seat") > 0; /* Ignore non-master devices for unknown seats */ if (!master && !seat) @@ -313,7 +314,8 @@ int manager_process_button_device(Manager *m, sd_device *d) { if (r < 0) return r; - if (device_for_action(d, DEVICE_ACTION_REMOVE)) { + if (device_for_action(d, DEVICE_ACTION_REMOVE) || + sd_device_has_current_tag(d, "power-switch") <= 0) { b = hashmap_get(m->buttons, sysname); if (!b) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index d883288b25..bbec23c850 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1361,7 +1361,7 @@ static int attach_device(Manager *m, const char *seat, const char *sysfs) { if (r < 0) return r; - if (sd_device_has_tag(d, "seat") <= 0) + if (sd_device_has_current_tag(d, "seat") <= 0) return -ENODEV; if (sd_device_get_property_value(d, "ID_FOR_SEAT", &id_for_seat) < 0) diff --git a/src/login/sysfs-show.c b/src/login/sysfs-show.c index a66be28ad9..9b7fc20396 100644 --- a/src/login/sysfs-show.c +++ b/src/login/sysfs-show.c @@ -53,14 +53,14 @@ static int show_sysfs_one( /* Explicitly also check for tag 'seat' here */ if (!streq(seat, sn) || - sd_device_has_tag(dev_list[*i_dev], "seat") <= 0 || + sd_device_has_current_tag(dev_list[*i_dev], "seat") <= 0 || sd_device_get_subsystem(dev_list[*i_dev], &subsystem) < 0 || sd_device_get_sysname(dev_list[*i_dev], &sysname) < 0) { (*i_dev)++; continue; } - is_master = sd_device_has_tag(dev_list[*i_dev], "master-of-seat") > 0; + is_master = sd_device_has_current_tag(dev_list[*i_dev], "master-of-seat") > 0; if (sd_device_get_sysattr_value(dev_list[*i_dev], "name", &name) < 0) (void) sd_device_get_sysattr_value(dev_list[*i_dev], "id", &name); @@ -80,7 +80,7 @@ static int show_sysfs_one( isempty(lookahead_sn)) lookahead_sn = "seat0"; - if (streq(seat, lookahead_sn) && sd_device_has_tag(dev_list[lookahead], "seat") > 0) + if (streq(seat, lookahead_sn) && sd_device_has_current_tag(dev_list[lookahead], "seat") > 0) break; } } From bf6e5c574b08e934cdfab18317c51987b001db58 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 13 Dec 2018 19:13:59 +0100 Subject: [PATCH 8/9] NEWS: explain the "bind"/"unbind" situation a bit --- NEWS | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/NEWS b/NEWS index ac29a5e0d4..6fc5815293 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,90 @@ systemd System and Service Manager +CHANGES WITH 247 in spe: + + * KERNEL API INCOMPATIBILTY: Linux 4.12 introduced two new uevents + "bind" and "unbind" to the Linux device model. When this kernel + change was made, systemd-udevd was only minimally updated to handle + and propagate these new event types. The introduction of these new + uevents (which are typically generated for USB devices and devices + needing a firmware upload before being functional) resulted in a + number of software issues, we so far didn't address (mostly because + there was hope the kernel maintainers would themeselves address these + issues in some form – which did not happen). To handle them properly, + many (if not most) udev rules files shipped in various packages need + updating, and so do many programs that monitor or enumerate devices + with libudev or sd-device, or otherwise process uevents. Please note + that this incompatibility is not fault of systemd or udev, but caused + by an incompatible kernel change that happened back in Linux 4.12. + + To minimize issues resulting from this kernel change (but not avoid + them entirely) starting with systemd-udevd 247 the udev "tags" + concept (which is a concept for marking and filtering devices during + enumeration and monitoring) has been reworked: udev tags are now + "sticky", meaning that once a tag is assigned to a device it will not + be removed from the device again until the device itself is removed + (i.e. unplugged). This makes sure that any application monitoring + devices that match a specific tag is guaranteed to both see uevents + where the device starts being relevant, and those where it stops + being relevant (the latter now regularly happening due to the new + "unbind" uevent type). The udev tags concept is hence now a concept + tied to a *device* instead of a device *event* — unlike for example + udev properties whose lifecycle (as before) is generally tied to a + device event, meaning that the previously determined properties are + forgotten whenever a new uevent is processed. + + With the newly redefined udev tags concept, sometimes it's necessary + to determine which tags are the ones applied by the most recent + uevent/database update, in order to discern them from those + originating from earlier uevents/database updates of the same + device. To accommodate for this a new automatic property CURRENT_TAGS + has been added that works similar to the existing TAGS property but + only lists tags set by the most recent uevent/database + update. Similar, the libudev/sd-device API has been updated with new + functions to enumerate these 'current' tags, in addition to the + existing APIs that now enumerate the 'sticky' ones. + + To properly handle "bind"/"unbind" on Linux 4.12 and newer it is + essential that all udev rules files and applications are updated to + handle the new events. Specifically: + + • All rule files that currently use a header guard similar to + ACTION!="add|change",GOTO="xyz_end" should be updated to use + ACTION=="remove",GOTO="xyz_end" instead, so that the + properties/tags they add are also applied whenever "bind" (or + "unbind") is seen. (This is most important for all physical device + types — as that's for which "bind" and "unbind" are currently + usually generated, for all other device types this change is still + recommended but not as important — but certainly prepares for + future kernel uevent type additions). + + • Similar, all code monitoring devices that contains an 'if' branch + discerning the "add" + "change" uevent actions from all other + uevents actions (i.e. considering devices only relevant after "add" + or "change", and irrelevant on all other events) should be reworked + to instead negatively check for "remove" only (i.e. considering + devices relevant after all event types, except for "remove", which + invalidates the device). Note that this also means that devices + should be considered relevant on "unbind", even though conceptually + this — in some form — invalidates the device. Since the precise + effect of "unbind" is not generically defined, devices should be + considered relevant even after "unbind", however I/O errors + accessing the device should then be handled gracefully. + + • Any code that uses device tags for deciding whether a device is + relevant or not most likely needs to be updated to use the new + udev_device_has_current_tag() API (or sd_device_has_current_tag() + in case sd-device is used), to check whether the tag is set + at the moment an uevent is seen (as opposed to the existing + udev_device_has_tag() API which checks if the tag ever existed on + the device, following the API concept redefinition explained + above). + + We are very sorry for this breakage and the requirement to update + packages using these interfaces. We'd again like to underline that + this is not caused by systemd/udev changes, but result of a kernel + behaviour change. + CHANGES WITH 246: * The service manager gained basic support for cgroup v2 freezer. Units From 278fdd064df071cca2cd3bcae882a9a5a965c8b5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 26 Aug 2020 18:30:52 +0200 Subject: [PATCH 9/9] man: document the new libudev APIs --- man/udev_device_has_tag.xml | 67 ++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/man/udev_device_has_tag.xml b/man/udev_device_has_tag.xml index 9c64a4b45b..2e5b67e750 100644 --- a/man/udev_device_has_tag.xml +++ b/man/udev_device_has_tag.xml @@ -21,9 +21,11 @@ udev_device_has_tag + udev_device_has_current_tag udev_device_get_devlinks_list_entry udev_device_get_properties_list_entry udev_device_get_tags_list_entry + udev_device_get_current_tags_list_entry udev_device_get_sysattr_list_entry udev_device_get_property_value udev_device_get_sysattr_value @@ -36,6 +38,18 @@ #include <libudev.h> + + int udev_device_has_tag + struct udev_device *udev_device + const char *tag + + + + int udev_device_has_current_tag + struct udev_device *udev_device + const char *tag + + struct udev_list_entry *udev_device_get_devlinks_list_entry struct udev_device *udev_device @@ -51,6 +65,11 @@ struct udev_device *udev_device + + struct udev_list_entry *udev_device_get_current_tags_list_entry + struct udev_device *udev_device + + struct udev_list_entry *udev_device_get_sysattr_list_entry struct udev_device *udev_device @@ -62,12 +81,6 @@ const char *key - - int udev_device_has_tag - struct udev_device *udev_device - const char *tag - - const char *udev_device_get_sysattr_value struct udev_device *udev_device @@ -84,22 +97,40 @@ - + udev_device_has_tag() returns a valuer larger than zero if the specified + device object has the indicated tag assigned to it, and zero otherwise. See + udev7 for details on + the tags concept. udev_device_has_current_tag() executes a similar check, however + only determines whether the indicated tag was set as result of the most recent event seen for the + device. Tags are "sticky", i.e. once set for a device they remain on the device until the device is + unplugged, even if the rules run for later events of the same device do not set them anymore. Any tag for + which udev_device_has_current_tag() returns true will hence also return true when + passed to udev_device_has_tag(), but the opposite might not be true, in case a tag is + no longer configured by the rules applied to the most recent device even. + + udev_device_get_tags_list_entry() returns a a + udev_list_entry object, encapsulating a list of tags set for the specified + device. Similar, udev_device_get_current_tags_list_entry() returns a list of tags + set for the specified device as effect of the most recent device event seen (see above for details on the + difference). + Return Value - On success, - udev_device_get_devlinks_list_entry(), + On success, udev_device_has_tag() and + udev_device_has_current_tag() return positive or 0, depending + on whether the device has the given tag or not. On failure, a negative error code is returned. + + On success, udev_device_get_devlinks_list_entry(), udev_device_get_properties_list_entry(), - udev_device_get_tags_list_entry() and - udev_device_get_sysattr_list_entry() return - a pointer to the first entry of the retrieved list. If that list - is empty, or if an error occurred, NULL is + udev_device_get_tags_list_entry(), + udev_device_get_current_tags_list_entry() and + udev_device_get_sysattr_list_entry() return a pointer to the first entry of the + retrieved list. If that list is empty, or if an error occurred, NULL is returned. On success, @@ -119,17 +150,13 @@ contain NUL bytes should not be set with this function; instead, write them directly to the files within the device's syspath. - - On success, udev_device_has_tag() - returns 1 or 0, - depending on whether the device has the given tag or not. - On failure, a negative error code is returned. See Also + udev7, udev_new3, udev_device_new_from_syspath3, udev_device_get_syspath3,