[strace/strace] Add lirc ioctl decoding (PR #208)
Sean Young
notifications at github.com
Mon Feb 7 18:58:04 UTC 2022
> A copy of an e-mail with `Message-ID: <20220204061024.GA10415 at obsidian>`[1] that for whatever reason hasn't been forwarded to github:
>
> ```
> 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.
Fixed.
> > 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).
Rernamed to lirc_ioctl.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...
If I tried to fix this, the output of:
```
do_ioctl(LIRC_GET_FEATURES, (unsigned int*)0x40000);
```
becomes
```
ioctl(-1, LIRC_GET_FEATURES, , 0x40000)
```
Which we don't want.
> > + 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've added an early switch like dm does.
>
> 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.
Fixed. Binary would be nicer than hex though.
>
> 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 was no order. I've ordered them.
> 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.
I've added LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SEND_LIRCCODE (may be used in
pre-4.16 kernels) and LIRC_CAN_GET_REC_RESOLUTION was just missing. The other
onces were never implemented, out-of-tree or not. They should have never
been added to the header in the first place.
>
> > 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.
Removed XLAT_STR()
> It is also worth checking strace behaviour when the memory can't
> be accessed or the pointer is NULL.
Added tests.
> > + 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.
Alas.
> > + 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.
XLAT_STR() removed.
> 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.
Add tests for this.
>
> Also, LIRC_SET_TRANSMITTER_MASK doesn't seem to be checked.
>
> > + // read ioctl
>
> C++-style comments are not generally used in strace code.
I've changed to 80s style comments.
> 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.
I think that is tested now.
> > +#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.
Tests added.
> Also, these lines are over-length, strace source code tends to keep
> line length under 80.
Fixed.
> > + 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.
Yes, this is true. LIRC_GET_LENGTH is not a thing anymore since kernel v4.16,
however I've added a test.
--
Reply to this email directly or view it on GitHub:
https://github.com/strace/strace/pull/208#issuecomment-1031807778
You are receiving this because you are subscribed to this thread.
Message ID: <strace/strace/pull/208/c1031807778 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20220207/d0f49413/attachment.htm>
More information about the Strace-devel
mailing list