[strace/strace] Add lirc ioctl decoding (PR #208)
Eugene Syromyatnikov
evgsyr at gmail.com
Fri Feb 4 06:10:25 UTC 2022
On Thu, Feb 03, 2022 at 07:51:18AM -0800, Sean Young via Strace-devel wrote:
> Ready for the next round of review
On Wed, Jan 26, 2022 at 10:22:01AM +0000, Sean Young wrote:
> Signed-off-by: Sean Young <sean at mess.org>
> ---
> NEWS | 1 +
> bundled/Makefile.am | 1 +
> bundled/linux/include/uapi/linux/lirc.h | 232 ++++++++++++++++++++++++++++++++
> src/Makefile.am | 1 +
> src/defs.h | 1 +
> src/ioctl.c | 2 +
> src/lirc.c | 46 +++++++
> src/xlat/lirc_features.in | 13 ++
> src/xlat/lirc_modes.in | 6 +
> tests/.gitignore | 2 +
> tests/Makefile.am | 1 +
> tests/gen_tests.in | 2 +
> tests/ioctl_lirc-success.c | 2 +
> tests/ioctl_lirc.c | 173 ++++++++++++++++++++++++
> tests/pure_executables.list | 1 +
> 15 files changed, 484 insertions(+)
> create mode 100644 bundled/linux/include/uapi/linux/lirc.h
> create mode 100644 src/lirc.c
> create mode 100644 src/xlat/lirc_features.in
> create mode 100644 src/xlat/lirc_modes.in
> create mode 100644 tests/ioctl_lirc-success.c
> create mode 100644 tests/ioctl_lirc.c
>
> diff --git a/NEWS b/NEWS
> index 6c8f859..c933d11 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -12,6 +12,7 @@ Noteworthy changes in release ?.?? (????-??-??)
> * Implemented decoding of PR_SET_VMA operation of prctl syscall.
> * Updated lists of FAN_*, IORING_*, IOSQE_*, KVM_*, MODULE_INIT_*, TCA_ACT_*,
> and *_MAGIC constants.
> + * Implemented decoding of lirc ioctl commands.
"LIRC", maybe? I've thought it's an abbreviation.
> diff --git a/src/Makefile.am b/src/Makefile.am
> index dc9fd93..c000593 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -184,6 +184,7 @@ libstrace_a_SOURCES = \
> link.c \
> linux/x32/asm_stat.h \
> linux/x86_64/asm_stat.h \
> + lirc.c \
I think for ioctl interfaces the naming is *_ioctl.c as of late,
but I have no strong preference here (and there are files like dm.c).
> diff --git a/src/lirc.c b/src/lirc.c
> new file mode 100644
> index 0000000..6459dc2
> --- /dev/null
> +++ b/src/lirc.c
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright (c) 2022 Sean Young <sean at mess.org>
> + *
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#include "defs.h"
> +#include <linux/lirc.h>
> +#include "xlat/lirc_features.h"
> +#include "xlat/lirc_modes.h"
> +
> +int
> +lirc_ioctl(struct tcb *const tcp, const unsigned int code,
> + const kernel_ulong_t arg)
> +{
> + unsigned int value;
> +
> + if (_IOC_DIR(code) == _IOC_READ && entering(tcp))
> + return 0;
> +
> + tprint_arg_next();
> +
It is nicer to return after printing the comma (even though it leads
to a bit more ugly code), as this leads to
scname(arg1, <unfinished...>
<resumed scname> arg2...
output instead of
scname(arg1<unfinished>
<resumed scname> , arg2...
> + switch (code) {
> + case LIRC_GET_FEATURES:
> + printflags(lirc_features, value, "LIRC_CAN_???");
> + break;
> + case LIRC_GET_SEND_MODE:
> + case LIRC_GET_REC_MODE:
> + case LIRC_SET_SEND_MODE:
> + case LIRC_SET_REC_MODE:
> + printxval(lirc_modes, value, "LIRC_MODE_???");
> + break;
> + default:
> + PRINT_VAL_U(value);
The main issue here is that this code assumes that only LIRC_* ioctls
have 'i' IOC type, which is generally not true (it is shared with i8k,
IPMI, IIO and a couple of i915 ioctl commands). This can be approached
either by performing early filtering by ioctl cmd (like it is done in
dm), or by explicitly listing all supported ioctls in the decoding switch
statements and returning RVAL_DECODED in the default case (as it is done
in seccomp and kd ioctl decoders, for example). Both options will lead
to some duplication, though, either with listing all of the LIRC
commands, or re-factoring code to handle tracee memory reading and
printing inside
I think that at least LIRC_SET_TRANSMITTER_MASK command argument is worth
printing in hexadecimal and not decimal as it seems to be a bit mask.
There is a set of functions for printing an integer referenced
by an address, printnum_*, but I'm not sure if it is worth using here.
> diff --git a/src/xlat/lirc_features.in b/src/xlat/lirc_features.in
> new file mode 100644
> index 0000000..6bd45a4
> --- /dev/null
> +++ b/src/xlat/lirc_features.in
> @@ -0,0 +1,13 @@
> +#unconditional
> +LIRC_CAN_REC_SCANCODE
> +LIRC_CAN_REC_MODE2
> +LIRC_CAN_GET_REC_RESOLUTION
> +LIRC_CAN_SEND_PULSE
> +LIRC_CAN_SET_TRANSMITTER_MASK
> +LIRC_CAN_SET_SEND_CARRIER
> +LIRC_CAN_SET_SEND_DUTY_CYCLE
> +LIRC_CAN_SET_REC_CARRIER
> +LIRC_CAN_SET_REC_CARRIER_RANGE
> +LIRC_CAN_USE_WIDEBAND_RECEIVER
> +LIRC_CAN_MEASURE_CARRIER
> +LIRC_CAN_SET_REC_TIMEOUT
I'm unable grok the order of this xlat; usually, flags are sorted from the
LSB to MSB, unless there are reasons to have a different order (usually,
when a symbolic value represents a combination of flags). There are also
LIRC_CAN_SEND_RAW, LIRC_CAN_SEND_MODE2, LIRC_CAN_SEND_SCANCODE,
LIRC_CAN_SEND_LIRCCODE, LIRC_CAN_REC_RAW, LIRC_CAN_REC_PULSE,
LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SET_REC_FILTER, LIRC_CAN_GET_REC_RESOLUTION,
and LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE flags seem to be missing.
> diff --git a/tests/ioctl_lirc.c b/tests/ioctl_lirc.c
> new file mode 100644
> index 0000000..6f7fdc0
> --- /dev/null
> +++ b/tests/ioctl_lirc.c
> + value = 12000;
> + do_ioctl(LIRC_SET_REC_TIMEOUT, &value);
> + printf("ioctl(-1, %s, [12000]) = %s\n",
> + XLAT_STR(LIRC_SET_REC_TIMEOUT), errstr);
XLAT_STR macro is used for generating a string in accordance with
XLAT_VERBOSE and XLAT_RAW macro constant settings, which are used
for testing output of strace with different xlat verbosity settings (see
*-X{abbrev,raw,verbose}.c tests for an example), otherwise it is
useless. On the other hand, having such tests would be nice
for a decoder that prints named constants, even though xlat usage
in the lirc decoder currently rather trivial.
It is also worth checking strace behaviour when the memory can't
be accessed or the pointer is NULL.
> + value = 40000;
> + do_ioctl(LIRC_SET_REC_CARRIER_RANGE, &value);
> + printf("ioctl(-1, IPMICTL_SET_MAINTENANCE_MODE_CMD or %s, [40000]) = %s\n",
> + XLAT_STR(LIRC_SET_REC_CARRIER_RANGE), errstr);
This will generate incorrect if ether XLAT_VERBOSE or XLAT_RAW are set.
Also, this output probably signifies the issue with assuming that
every command is a LIRC command, as IPMICTL_SET_MAINTENANCE_MODE_CMD
argument is a named constant (IPMI_MAINTENANCE_MODE_*). But, alas,
strace doesn't currently implement capabilities that would allow
to narrow down the ioctl cmd selection based on ioctl fd.
> + value = 2;
> + do_ioctl(LIRC_SET_SEND_MODE, &value);
> + printf("ioctl(-1, %s, [LIRC_MODE_PULSE]) = %s\n",
> + XLAT_STR(LIRC_SET_SEND_MODE), errstr);
Either LIRC_MODE_PULSE is to be wrapped within XLAT_STR in order to
enable proper string generation in different xla modes, or XLAT_STR
usage for LIRC_SET_SEND_MODE is redundant. Also, it is worth checking
that 3 (and probably some other values) is decoded as an unknown mode
and not a combination of LIRC_MODE_RAW and LIRC_MODE_PULSE: it is not
uncommon to mix up printxlat and printflags usage.
Also, LIRC_SET_TRANSMITTER_MASK doesn't seem to be checked.
> + // read ioctl
C++-style comments are not generally used in strace code.
Also, it is worth checking decoding of the read ioctl command when they
can't read (due to non-zero syscall return value) as well, to avoid accidentally
decoding of an argument on entering when it is supposed to be decoded in exiting.
> +#ifdef INJECT_RETVAL
> + value = LIRC_CAN_SEND_PULSE | LIRC_CAN_SET_SEND_DUTY_CYCLE | LIRC_CAN_GET_REC_RESOLUTION | LIRC_CAN_USE_WIDEBAND_RECEIVER;
> + do_ioctl(LIRC_GET_FEATURES, &value);
> + printf("ioctl(-1, %s, [LIRC_CAN_GET_REC_RESOLUTION|LIRC_CAN_SEND_PULSE|LIRC_CAN_SET_SEND_DUTY_CYCLE|LIRC_CAN_USE_WIDEBAND_RECEIVER]) = %s\n",
> + XLAT_STR(LIRC_GET_FEATURES), errstr);
For flag sets, it is also worth checking how 0 or unknown flags are
decoded.
Also, these lines are over-length, strace source code tends to keep
line length under 80.
> + value = 1;
> + do_ioctl(LIRC_GET_REC_MODE, &value);
> + printf("ioctl(-1, %s, [LIRC_MODE_RAW]) = %s\n",
> + XLAT_STR(LIRC_GET_REC_MODE), errstr);
> +
> + do_ioctl(LIRC_GET_SEND_MODE, &value);
> + printf("ioctl(-1, %s, [LIRC_MODE_PULSE]) = %s\n",
> + XLAT_STR(LIRC_GET_SEND_MODE), errstr);
> +
> + value = 120;
> + do_ioctl(LIRC_GET_REC_RESOLUTION, &value);
> + printf("ioctl(-1, %s, [120]) = %s\n",
> + XLAT_STR(LIRC_GET_REC_RESOLUTION), errstr);
> +
> + value = 120000;
> + do_ioctl(LIRC_GET_REC_TIMEOUT, &value);
> + printf("ioctl(-1, %s, [120000]) = %s\n",
> + XLAT_STR(LIRC_GET_REC_TIMEOUT), errstr);
> +
> + value = 1100;
> + do_ioctl(LIRC_GET_MIN_TIMEOUT, &value);
> + printf("ioctl(-1, I2OVALIDATE or %s, [1100]) = %s\n",
> + XLAT_STR(LIRC_GET_MIN_TIMEOUT), errstr);
> +
> + value = 10100;
> + do_ioctl(LIRC_GET_MAX_TIMEOUT, &value);
> + printf("ioctl(-1, %s, [10100]) = %s\n",
> + XLAT_STR(LIRC_GET_MAX_TIMEOUT), errstr);
LIRC_GET_LENGTH commands doesn't seem to be checked.
More information about the Strace-devel
mailing list