From 8a41f6e105a053a600ac5a47e1607f30b3b53146 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 24 Jan 2025 23:47:44 +0900 Subject: [PATCH 1/3] sd-device: make device_get_cached_sysattr_value() static --- src/libsystemd/sd-device/device-private.h | 1 - src/libsystemd/sd-device/sd-device.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index 11a27b01d6..4165c85023 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -35,7 +35,6 @@ int device_get_devnode_gid(sd_device *device, gid_t *ret); int device_chase(sd_device *device, const char *path, ChaseFlags flags, char **ret_resolved, int *ret_fd); void device_clear_sysattr_cache(sd_device *device); int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error); -int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value); void device_seal(sd_device *device); void device_set_is_initialized(sd_device *device); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index ec129facba..a40f0a6333 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2350,7 +2350,7 @@ int device_cache_sysattr_value(sd_device *device, char *key, char *value, int er return 0; } -int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) { +static int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) { SysAttrCacheEntry *entry; assert(device); From 2b3d4e3d9416399d4dd43d27dee4e9148760d536 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 24 Jan 2025 23:58:02 +0900 Subject: [PATCH 2/3] sd-device: move the check if sysattr entry should be cached into device_cache_sysattr_value() No functional change. Preparation for the next commit. --- src/libsystemd/sd-device/sd-device.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index a40f0a6333..033a9eb875 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2315,15 +2315,19 @@ DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( char, path_hash_func, path_compare, SysAttrCacheEntry, sysattr_cache_entry_free); -int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error) { +static int device_cache_sysattr_value_full(sd_device *device, char *key, char *value, int error, bool ignore_uevent) { int r; assert(device); assert(key); assert(value || error > 0); - /* This takes the reference of the input arguments on success. The input value may be NULL. - * This replaces an already existing entry. */ + /* This takes the reference of the input arguments when cached, hence the caller must not free them + * when a positive return value is returned. The input value may be NULL. This replaces an already + * existing entry. */ + + if (ignore_uevent && streq(last_path_component(key), "uevent")) + return 0; /* not cached */ /* Remove the old cache entry. So, we do not need to clear cache on error. */ sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, key)); @@ -2347,7 +2351,11 @@ int device_cache_sysattr_value(sd_device *device, char *key, char *value, int er return r; TAKE_PTR(entry); - return 0; + return 1; /* cached */ +} + +int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error) { + return device_cache_sysattr_value_full(device, key, value, error, /* ignore_uevent = */ true); } static int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) { @@ -2478,7 +2486,7 @@ cache_result: return RET_GATHER(r, -ENOMEM); } - int k = device_cache_sysattr_value(device, resolved, value, -r); + int k = device_cache_sysattr_value_full(device, resolved, value, -r, /* ignore_uevent = */ false); if (k < 0) { if (r < 0) log_device_debug_errno(device, k, @@ -2496,11 +2504,12 @@ cache_result: return r; } + assert(k > 0); if (ret_value && r >= 0) *ret_value = value; - /* device_cache_sysattr_value() takes 'resolved' and 'value' on success. */ + /* device_cache_sysattr_value_full() takes 'resolved' and 'value' on success. */ TAKE_PTR(resolved); TAKE_PTR(value); return r; @@ -2628,16 +2637,12 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, return r; } - /* Do not cache action string written into uevent file. */ - if (streq(last_path_component(resolved), "uevent")) - return 0; - r = device_cache_sysattr_value(device, resolved, copied, 0); if (r < 0) log_device_debug_errno(device, r, "sd-device: failed to cache written attribute '%s' with '%s', ignoring: %m", resolved, copied); - else { + else if (r > 0) { TAKE_PTR(resolved); TAKE_PTR(copied); } From a9559ebcbcf3098b2c14e5e10e99d05aaffe4ac3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 12 Jan 2025 08:22:53 +0900 Subject: [PATCH 3/3] udev-rules: use sd_device_set_sysattr_value() to write sysfs attribute Then, we can avoid that files outside of sysfs are written by udev ATTR key. This also makes - logs failure in udev_resolve_subsys_kernel(), - failure in sd_device_get_syspath() critical, as that should not happen, - cache the value to be write when running on test mode, to make it shown by OPTIONS="dump" or obtained by ATTR match token. --- src/udev/udev-rules.c | 75 +++++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index f8afe923d8..3c0133d55e 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -2960,50 +2960,85 @@ static int udev_rule_apply_token_to_event( } case TK_A_ATTR: { char buf[UDEV_PATH_SIZE], value[UDEV_NAME_SIZE]; - const char *val, *key_name = token->data; + const char *key = token->data; + + /* First, try to resolve "[/]" format. */ + r = udev_resolve_subsys_kernel(key, buf, sizeof(buf), false); + if (r == -ENOMEM) + return log_oom(); + if (ERRNO_IS_NEG_DEVICE_ABSENT(r)) { + log_event_warning_errno(event, token, r, "Failed to resolve sysfs attribute \"%s\", ignoring: %m", key); + return true; + } + if (r < 0) { + /* If not, make the path to sysfs attribute absolute, to make '*' resolvable by attr_subst_subdir(). */ + const char *syspath; + r = sd_device_get_syspath(dev, &syspath); + if (r < 0) + return log_event_error_errno(event, token, r, "Failed to get syspath: %m"); - if (udev_resolve_subsys_kernel(key_name, buf, sizeof(buf), false) < 0 && - sd_device_get_syspath(dev, &val) >= 0) { bool truncated; - strscpyl_full(buf, sizeof(buf), &truncated, val, "/", key_name, NULL); + strscpyl_full(buf, sizeof(buf), &truncated, syspath, "/", key, NULL); if (truncated) { log_event_warning(event, token, "The path to the attribute \"%s/%s\" is too long, refusing to set the attribute.", - val, key_name); + syspath, key); + return true; + } + + /* Resolve '*' in the path. */ + r = attr_subst_subdir(buf); + if (r < 0) { + log_event_error_errno(event, token, r, "Could not find file matches \"%s\", ignoring: %m", buf); return true; } } - r = attr_subst_subdir(buf); + /* Then, make the path relative again. This also checks if the path being inside of the sysfs. */ + _cleanup_free_ char *resolved = NULL; + _cleanup_close_ int fd = -EBADF; + r = device_chase(dev, buf, /* flags = */ 0, &resolved, &fd); if (r < 0) { - log_event_error_errno(event, token, r, "Could not find file matches \"%s\", ignoring: %m", buf); + log_event_error_errno(event, token, r, "Could not chase sysfs attribute \"%s\", ignoring: %m", buf); return true; } + /* Apply formatting to the value. */ if (!apply_format_value(event, token, value, sizeof(value), "attribute value")) return true; if (EVENT_MODE_DESTRUCTIVE(event)) { - log_event_debug(event, token, "Writing \"%s\" to sysfs attribute \"%s\".", value, buf); - r = write_string_file(buf, value, - WRITE_STRING_FILE_VERIFY_ON_FAILURE | - WRITE_STRING_FILE_DISABLE_BUFFER | - WRITE_STRING_FILE_AVOID_NEWLINE | - WRITE_STRING_FILE_VERIFY_IGNORE_NEWLINE); + log_event_debug(event, token, "Writing \"%s\" to sysfs attribute \"%s\".", value, resolved); + r = sd_device_set_sysattr_value(dev, resolved, value); if (r < 0) - log_event_error_errno(event, token, r, "Failed to write \"%s\" to sysfs attribute \"%s\", ignoring: %m", value, buf); + log_event_error_errno(event, token, r, "Failed to write \"%s\" to sysfs attribute \"%s\", ignoring: %m", value, resolved); else { - event_cache_written_sysattr(event, buf, value); + event_cache_written_sysattr(event, resolved, value); log_event_done(event, token); } } else { - log_event_debug(event, token, "Running in test mode, skipping writing \"%s\" to sysfs attribute \"%s\".", value, buf); + log_event_debug(event, token, "Running in test mode, skipping writing \"%s\" to sysfs attribute \"%s\".", value, resolved); - r = verify_regular_at(AT_FDCWD, buf, /* follow = */ false); + /* We assume the attribute is writable if the path points to a regular file, and cache + * the value to make it shown by OPTIONS="dump" or obtained by later ATTR match token. */ + r = fd_verify_regular(fd); if (r < 0 && !ERRNO_IS_NEG_PRIVILEGE(r)) - log_event_error_errno(event, token, r, "Failed to verify sysfs attribute \"%s\" is a regular file: %m", buf); - else - event_cache_written_sysattr(event, buf, value); + log_event_error_errno(event, token, r, "Failed to verify sysfs attribute \"%s\" is a regular file: %m", resolved); + else { + event_cache_written_sysattr(event, resolved, value); + + _cleanup_free_ char *copied = strdup(value); + if (!copied) + return log_oom(); + + r = device_cache_sysattr_value(dev, resolved, value, /* error = */ 0); + if (r < 0) + log_event_warning_errno(event, token, r, "Failed to cache sysfs attribute \"%s\", ignoring: %m", resolved); + else if (r > 0) { + TAKE_PTR(resolved); + TAKE_PTR(copied); + } + } } return true; }