strncpy does not null-terminate the destination buffer if the source
string is longer than the count parameter. Since wwn and
wwn_vendor_extension are char[17] and we copy up to 16 bytes, there's
a risk of missing null termination. Use strscpy which always
null-terminates.
CID#1469706
Follow-up for 4e9fdfccbd
The fix in 8f1751a111 made me wonder if we could automatically detect
when pointers are accessed but when this might not be safe. Systemd
is already using a lot of `assert(dst)` and this change now forces
us to use them.
So this commit (ab)uses coccinelle to flag any pointer parameter
dereference not preceded by assert(param), ASSERT_PTR(param), or an
explicit NULL check. It adds integration into meson as a new "coccinelle"
test suite (just like clang-tidy) and is run in CI. The check is not
perfect but seems a reasonable heuristic.
For this RFC commit it is scoped to a subset, it excludes 25 dirs right
now and includes around 100. About 300 warnings left. Busywork that I am
happy to do if there is agreement that it is worth it.
With this in place we would have caught the bug from 8f1751a111 in CI:
```
FAIL: check-pointer-deref.cocci found issues in systemd/src/boot:
diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c
--- systemd/src/boot/measure.c
+++ /tmp/nothing/measure.c
@@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event(
if (err != EFI_SUCCESS)
return err;
- *ret_measured = true;
return EFI_SUCCESS;
}
```
This also adds a new POINTER_MAY_BE_NULL() for the cases when the
called function will do the NULL check (like `iovec_is_set()`).
A 10G Marvell AQC113 included in an ASRock TRX50WS motherboard NIC claims to
support tx/rx queues as large as 8184.
After boot 'ethtool -g eth0' outputs:
Ring parameters for eth0:
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
TX push buff len: n/a
HDS thresh: n/a
RX: 2048
RX Mini: n/a
RX Jumbo: n/a
TX: 4096
RX Buf Len: n/a
CQE Size: n/a
TX Push: off
RX Push: off
TX push buff len: n/a
TCP data split: n/a
HDS thresh: n/a
'ethtool --set-ring eth0 rx 8184 tx 8184 && ethtool -g eth0' yields:
Ring parameters for eth0:
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
TX push buff len: n/a
HDS thresh: n/a
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
RX Buf Len: n/a
CQE Size: n/a
TX Push: off
RX Push: off
TX push buff len: n/a
TCP data split: n/a
HDS thresh: n/a
I can measure a throughput difference between using using buffer sizes
4096 and 8184 on my hardware, so it really seems that this is doing
something beyond buggy firmware.
Original PR https://github.com/systemd/systemd/pull/17635 didn't give any
explanation for the limit of 4096, but that's probably what was supported by
the kernel drivers at the time.
A web search shows that CISCO VIC 15000 supports 16k, so allow up to that.
[zjs: edited the message]
We often have a pattern where the same verb function is used for
multiple actions. This leads to an antipattern where we figure out what
action needs to be taken from argv[0] multiple times: often once in
parse_argv() to figure out what options are allowed, then once again
implicitly in dispatch_verb(), and then again in the action verb itself.
Let's allow passing a parameter into the verb to simplify this.
This matches a pattern we have in conf-parser.h, where we have both
void *userdata (more global) and void *data (per-config item). Here,
I opted for uintptr_t userdata. It seems that most of the time we'll
want to just pass an enum value. This works OK with no casts. I also
tried a void* and union. In both cases, much more boilerplate is needed:
either a cast or a macro to help avoid compiler warnings. uintptr_t
seems generic enough to cover foreseeable usecases with no fuss.
This is a noop refactoring. See next commit for an example.
The previous version in the PR changed variable and sanitized it
in place. The second version switched to skip if CCs are in the
string instead, but didn't move back to the original variable.
Because it's an existing variable, no CI caught it.
Follow-up for 16325b35fa
The removed code in device_tag_index() in fact does nothing,
as sd_device.all_tags is never cleared. Moreover, not only the code
is meaningless, but it is theoretically/logically wrong, as the symlinks
in /run/udev/tags/ should be 'sticky', hence we should even not try to
remove them.
The current tag concept has been introduced by
e77b146f82 (v247) to make symlinks in
/run/udev/tags/ are 'sticky'.
However, when TAG= (rather than TAG+=) is specified, then the tags
assigned in the previous events were also cleared.
This fixes the issue and now symlinks in /run/udev/tags/ are really
'sticky'.
Fortunately, TAG= is mostly unused. So, the issue should not affect
and the fix should not change anything on almost all systems.
XDG_SESSION_EXTRA_DEVICE_ACCESS will now take a colon-separated list of
identifiers. For every identifier $ID, the session is granted access to all
devices tagged as "xaccess-$ID" in udev.
Fixes: #40634
This reverts (in sprit) commit f5cdf9515a,
"udev-rules: ignore non-system user/group in OWNER=/GROUP=".
The original change was done to clean up a situation where we added a
new group, but the group could already have been used for some other
purposes, and now the some unexpected entity would own the device.
Unfortunately, this check doesn't really address the issue, since the
existing account might as well be a system account, which might be
equally bad. In addition, this change is a big compatiblity break,
causing existing rules to stop working. Since quite a lot of systems
have local configuration to assign devices to users for various
purposes, this is very noticable to users. In a way, the original change
to add a new group was the compat break, and follow-up patch to cahnge
the rule parsing evolved a small compat break into a much bigger one.
There is merit to the change though, since device nodes shouldn't be
owned by users and groups and different mechanisms should be used
instead. To avoid breaking users systems, and since the original goal
cannot be achieved by this patch, let's downgrade this to a warning
to guide users towards different solutions.
When systemd is compiled with group-render-mode=0660, only the active
seat gets access to the render devices through uaccess. Remote desktop
sessions like gnome-remote-desktop would be left with no hardware
rendering, because those sessions are not associated with a seat.
We solve the issue by granting uaccess to specifically tagged devices on
session start, if the session is marked with
XDG_SESSION_EXTRA_DEVICE_ACCESS.
udev-builtin-uaccess is refactored to grant multiple users access to a
device, taking into account the device's seat and all the active
EXTRA_DEVICE_ACCESS sessions.
This partially reverts 36c557f7d4, which
introduced notify_remove_fd() that logs in LOG_DEBUG. However,
notify_remove_fd_warn() is still called other library functions, e.g.
notify_push_fd(), and produces warning message about the failure in
removing fd from fdstore on shutdown.
During shutdown process, we get the following logs:
```
systemd-udevd[370]: Failed to send notify message to '/run/systemd/notify': Connection refused
systemd-udevd[370]: Failed to remove file descriptor "config-serialization" from the store, ignoring: Connection refused
systemd-udevd[370]: Failed to send notify message to '/run/systemd/notify': Connection refused
systemd-udevd[370]: Failed to push serialization fd to service manager: Connection refused
```
Here, the 1st, 3rd, and 4th messages are in LOG_DEBUG, but the 2nd one
was in LOG_WARNING before this commit, and this makes it also in LOG_DEBUG.
Follow-up for 472404aca5.
Grant access to devices tagged with "xaccess" on session start, if the session
was created with XDG_SESSION_EXTRA_DEVICE_ACCESS=1.
udev-builtin-uaccess is refactored to grant multiple users access to a device,
taking into account the device's seat and all the active EXTRA_DEVICE_ACCESS
sessions.
Suppose the following dependent events are queued:
A(1000) -> C(1002)
\
--> E(1004)
/
B(1001) -> D(1003)
Here, each number is the seqnum of the event, and A -> B means that
event B depends on event A, that is, event B cannot be started until
event A is processed.
In this case, to know if event E can be started, we only need to check
if event C and D are processed, and not necessary to check the state of
event A or B.
However, the commit e1ae931064 introduced
full dependency list (in the above example, all A, B, C, D are listed as
the blocker for E), but it is overkill and most information is not necessary.
Also, before the commit e1ae931064, we
found blocker from the beginning of the queued events, but that's also
not necessary, as even A is processed, still C may be queued, and we
anyway need to check if C is processed or not.
This makes each event only stores the last blocker event for the event,
and finds the blocker from the end of the queue. With this change, the
memory cost can be reduced from O(n^2) to O(n). Also, as previously we
used Set for managing blockers, but now we only increment/decrement
the reference counter of events, so the speed should be also improved.
Fixes#39583 and #39817.
When udevd received a uevent or inotify event, we will queue or trigger
synthesized events. To minimize the race between processing these events
by udevd and user invocation of 'udevadm settle', let's create /run/udev/queue
file earlier.
On some errors, no event may be queued, but in that case, the file will
be removed by the post event source if nothing queued. See on_post().
Hopefully mitigate the issue #40499.
Otherwise, /run/udev/queue file may be removed before udevd receives
the triggered uevents. This avoids the following race:
1. uevent A is being processed,
2. received an inotify event and trigger a synthesized uevent,
3. uevent A has been processed,
4. the uevent queue becomes empty, and /run/udev/queue is removed,
5. received and queued the synthesized uevent, and /run/udev/queue is recreated.
With this patch, /run/udev/queue is not removed in step 4. Hence,
'udevadm settle' will wait for the synthesized event being processed.
This is similar to waiting for child processes that reread partitions
(thus trigger synthetic events) being terminated, but for synthetic
events triggered by the manager.
Let's make sure we can safely load configuration files based on a root
fd, including XAT_FDROOT.
(While we are at it, always pass path to root fs before fd to root fs,
following our recently updated CODING_STYLE)
Major distributions already have blkid >= 2.37.
Let's bump the minimal required version.
Note, util-linux (which provides blkid) 2.37 was released on 2021-06-01.
See also #38608.
Follow-up for 179dcf924f
After the mentioned commit, event_add_child_pidref() duplicates
the pidfd internally, hence the original pidfd would be leaked
with TAKE_PIDREF.
Let's introduce these for the following two reasons:
- Using them makes sure the parameter names match between declaration
and definition (if both use the corresponding macros).
- They make sure developers can't forget the _const_ and _pure_
attributes for the declarations.
This commit also includes some include sorting fixes
The current MAX_SERIAL_LEN value of 256 is insufficient for some SCSI
devices with non-standard serial number lengths. In do_scsi_page80_inquiry(),
the required buffer length is calculated as:
len = 1 + VENDOR_LENGTH + MODEL_LENGTH + buf[3]
= 1 + 8 + 16 + buf[3]
= 25 + buf[3]
where buf[3] contains the serial number length reported by the device.
According to the SCSI specification, this field is an unsigned 8-bit
value, meaning it can theoretically be up to 255 bytes. This results
in a maximum required length of 280 bytes, exceeding the current limit
of 256 bytes.
When this occurs, scsi_id fails with an error message like:
"length 256 too short - need 280"
This has been observed with certain vendor devices that report unusually
long serial numbers in VPD page 0x80.
Increase MAX_SERIAL_LEN to 512 to accommodate the maximum possible
serial number length plus all required prefixes (vendor, model, and
type identifiers), providing sufficient headroom for non-compliant
devices while maintaining reasonable memory usage.
Signed-off-by: tuhaowen <tuhaowen@uniontech.com>
Now that sd-event doesn't insist on blocking
SIGCHLD anymore when watching for process exit,
let's stop blocking SIGCHLD tree-wide where not
required anymore.
Now that we have Management Component Transport Protocol (MCTP) transports
available over USB, it would be helpful to apply udev's persistent
naming rules to MCTP interfaces, to follow the USB hub/port topology.
Enable persistent naming for ARPHRD_MCTP-type devices, using a "mc" name
prefix, and add appropriate definitions for the v260 naming sheme.
Usually we gracefully ignore unknown configuration parameters, so that
service files can be written by upstreams and used across a variegated
range of distributions with various versions of systemd, to avoid
forcing users to the minimum common denominator and only adding settings
that are supported by the oldest distro supported.
Image policies do not behave like this, and any unknown partition or
policy designator causes the whole unit to fail to parse and a hard
error.
Change it so that parsing RootImagePolicy and friends via unit file or
D-Bus logs but otherwise ignores unknown specifiers, like other options
do.
This allows us to add new specifiers in the future, and users to adopt
them immediately.
Follow-up for d452335aa4
Luca Boccassi
Michael Vogt
Walter McKelvie
Zbigniew Jędrzejewski-Szmek
Yu Watanabe
Frantisek Sumsal
Lennart Poettering
Alessandro Astone
Lennart Poettering
Mike Yuan
Daan De Meyer
tuhaowen
Jeremy Kerr
Luca Boccassi
David Tardon