- Linux-kselftest-mirror - lists.linaro.org

[PATCH 4/4] selftes/ptp: extend test to include ptp_gettimex64any()

by Mahesh Bandewar

add -y/-Y options to support PTP_SYS_OFFSET_ANY2 op. where -y represents samples to collect while -Y is to choose the timebase from available options of cycles, real, mono, or raw. Signed-off-by: Mahesh Bandewar <maheshb(a)google.com> CC: Shuah Khan <shuah(a)kernel.org> CC: Richard Cochran <richardcochran(a)gmail.com> CC: "David S. Miller" <davem(a)davemloft.net> CC: Rahul Rameshbabu <rrameshbabu(a)nvidia.com> CC: Jakub Kicinski <kuba(a)kernel.org> CC: linux-kselftest(a)vger.kernel.org CC: netdev(a)vger.kernel.org --- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/ptp/testptp.c | 79 ++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 42806add0114..c5e59cfc9830 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -66,6 +66,7 @@ TARGETS += powerpc TARGETS += prctl TARGETS += proc TARGETS += pstore +TARGETS += ptp TARGETS += ptrace TARGETS += openat2 TARGETS += resctrl diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c index c9f6cca4feb4..7000caa39ede 100644 --- a/tools/testing/selftests/ptp/testptp.c +++ b/tools/testing/selftests/ptp/testptp.c @@ -37,6 +37,13 @@ #define NSEC_PER_SEC 1000000000LL +static char *time_base_arr[PTP_TS_MAX] = { + "cycles", + "system time", + "monotonic time", + "raw-monotonic time", +}; + /* clock_adjtime is not available in GLIBC < 2.14 */ #if !__GLIBC_PREREQ(2, 14) #include <sys/syscall.h> @@ -145,8 +152,10 @@ static void usage(char *progname) " -T val set the ptp clock time to 'val' seconds\n" " -x val get an extended ptp clock time with the desired number of samples (up to %d)\n" " -X get a ptp clock cross timestamp\n" + " -y val get an extended-any ptp clock time with the desired number of samples (up to %d) with given time-base for sandwich (with -Y opt)\n" + " -Y val sandwich timebase to use {cycles|real|mono|raw}\n" " -z test combinations of rising/falling external time stamp flags\n", - progname, PTP_MAX_SAMPLES); + progname, PTP_MAX_SAMPLES, PTP_MAX_SAMPLES); } int main(int argc, char *argv[]) @@ -162,6 +171,7 @@ int main(int argc, char *argv[]) struct ptp_sys_offset *sysoff; struct ptp_sys_offset_extended *soe; struct ptp_sys_offset_precise *xts; + struct ptp_sys_offset_any *ats; char *progname; unsigned int i; @@ -182,6 +192,8 @@ int main(int argc, char *argv[]) int pct_offset = 0; int getextended = 0; int getcross = 0; + int get_ext_any = 0; + int ext_any_type = -1; int n_samples = 0; int pin_index = -1, pin_func; int pps = -1; @@ -196,7 +208,7 @@ int main(int argc, char *argv[]) progname = strrchr(argv[0], '/'); progname = progname ? 1+progname : argv[0]; - while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) { + while (EOF != (c = getopt(argc, argv, "cd:e:f:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xy:Y:z"))) { switch (c) { case 'c': capabilities = 1; @@ -273,6 +285,31 @@ int main(int argc, char *argv[]) case 'X': getcross = 1; break; + case 'y': + get_ext_any = atoi(optarg); + if (get_ext_any < 1 || get_ext_any > PTP_MAX_SAMPLES) { + fprintf(stderr, + "number of extended-any timestamp samples must be between 1 and %d; was asked for %d\n", + PTP_MAX_SAMPLES, get_ext_any); + return -1; + } + break; + case 'Y': + if (!strcasecmp(optarg, "cycles")) + ext_any_type = PTP_TS_CYCLES; + else if (!strcasecmp(optarg, "real")) + ext_any_type = PTP_TS_REAL; + else if (!strcasecmp(optarg, "mono")) + ext_any_type = PTP_TS_MONO; + else if (!strcasecmp(optarg, "raw")) + ext_any_type = PTP_TS_RAW; + else { + fprintf(stderr, + "type needs to be one of cycles,real,mono,raw only; was given %s\n", + optarg); + return -1; + } + break; case 'z': flagtest = 1; break; @@ -286,6 +323,14 @@ int main(int argc, char *argv[]) } } + /* For ptp_sys_offset_any both options 'y' and 'Y' must be given */ + if (get_ext_any > 0 && ext_any_type == -1) { + fprintf(stderr, + "For extended-any TS both options -y, and -Y are required.\n"); + usage(progname); + return -1; + } + fd = open(device, O_RDWR); if (fd < 0) { fprintf(stderr, "opening %s: %s\n", device, strerror(errno)); @@ -604,6 +649,36 @@ int main(int argc, char *argv[]) free(xts); } + if (get_ext_any) { + ats = calloc(1, sizeof(*ats)); + if (!ats) { + perror("calloc"); + return -1; + } + + ats->n_samples = get_ext_any; + ats->ts_type = ext_any_type; + + if (ioctl(fd, PTP_SYS_OFFSET_ANY2, ats)) { + perror("PTP_SYS_OFFSET_ANY2"); + } else { + printf("extended-any timestamp request returned %d samples\n", + get_ext_any); + + for (i = 0; i < get_ext_any; i++) { + printf("sample #%2d: %s before: %lld.%09u\n", + i, time_base_arr[ext_any_type], + ats->ts[i][0].sec, ats->ts[i][0].nsec); + printf(" phc time: %lld.%09u\n", + ats->ts[i][1].sec, ats->ts[i][1].nsec); + printf(" %s after: %lld.%09u\n", + time_base_arr[ext_any_type], + ats->ts[i][2].sec, ats->ts[i][2].nsec); + } + } + + free(ats); + } close(fd); return 0; } -- 2.42.0.582.g8ccd20d70d-goog

2 years, 1 month

1
0
0 0

[PATCH 1/2] kunit: Improve format of the NOT_ERR_OR_NULL assertion

by Michal Wajdeczko

Diagnostic message for failed KUNIT_ASSERT|EXPECT_NOT_ERR_OR_NULL shows only raw error code, like in this example: [ ] # example_all_expect_macros_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:126 [ ] Expected myptr is not error, but is: -12 but we can improve it by using more friendly error pointer format: [ ] # example_all_expect_macros_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:126 [ ] Expected myptr is not error, but is -ENOMEM Signed-off-by: Michal Wajdeczko <michal.wajdeczko(a)intel.com> Cc: David Gow <davidgow(a)google.com> Cc: Rae Moar <rmoar(a)google.com> --- lib/kunit/assert.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index dd1d633d0fe2..96ef236d3ca3 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -80,9 +80,9 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, ptr_assert->text); } else if (IS_ERR(ptr_assert->value)) { string_stream_add(stream, - KUNIT_SUBTEST_INDENT "Expected %s is not error, but is: %ld\n", + KUNIT_SUBTEST_INDENT "Expected %s is not error, but is %pe\n", ptr_assert->text, - PTR_ERR(ptr_assert->value)); + ptr_assert->value); } kunit_assert_print_msg(message, stream); } -- 2.25.1

2 years, 1 month

1
1
0 0

[PATCH] bridge: MTU auto tuning ignores IFLA_MTU on NEWLINK

by Trent Lloyd

Commit 804b854d374e ("net: bridge: disable bridge MTU auto tuning if it was set manually") disabled auto-tuning of the bridge MTU when the MTU was explicitly set by the user, however that would only happen when the MTU was set after creation. This commit ensures auto-tuning is also disabled when the MTU is set during bridge creation. Currently when the br_netdev_ops br_change_mtu function is called, the flag BROPT_MTU_SET_BY_USER is set. However this function is only called when the MTU is changed after interface creation and is not called if the MTU is specified during creation with IFLA_MTU (br_dev_newlink). br_change_mtu also does not get called if the MTU is set to the same value it currently has, which makes it difficult to work around this issue (especially for the default MTU of 1500) as you have to first change the MTU to some other value and then back to the desired value. Add new selftests to ensure the bridge MTU is handled correctly: - Bridge created with user-specified MTU (1500) - Bridge created with user-specified MTU (2000) - Bridge created without user-specified MTU - Bridge created with user-specified MTU set after creation (2000) Regression risk: Any workload which erroneously specified an MTU during creation but accidentally relied upon auto-tuning to a different value may be broken by this change. Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2034099 Fixes: 804b854d374e ("net: bridge: disable bridge MTU auto tuning if it was set manually") Signed-off-by: Trent Lloyd <trent.lloyd(a)canonical.com> --- net/bridge/br_netlink.c | 3 + .../selftests/drivers/net/bridge/Makefile | 10 ++ .../drivers/net/bridge/bridge-user-mtu.sh | 148 ++++++++++++++++++ .../drivers/net/bridge/net_forwarding_lib.sh | 1 + 4 files changed, 162 insertions(+) create mode 100644 tools/testing/selftests/drivers/net/bridge/Makefile create mode 100755 tools/testing/selftests/drivers/net/bridge/bridge-user-mtu.sh create mode 120000 tools/testing/selftests/drivers/net/bridge/net_forwarding_lib.sh diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 10f0d33d8ccf..8aff7d077848 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -1559,6 +1559,9 @@ static int br_dev_newlink(struct net *src_net, struct net_device *dev, spin_unlock_bh(&br->lock); } + if (tb[IFLA_MTU]) + br_opt_toggle(br, BROPT_MTU_SET_BY_USER, true); + err = br_changelink(dev, tb, data, extack); if (err) br_dev_delete(dev, NULL); diff --git a/tools/testing/selftests/drivers/net/bridge/Makefile b/tools/testing/selftests/drivers/net/bridge/Makefile new file mode 100644 index 000000000000..23e407c75a7f --- /dev/null +++ b/tools/testing/selftests/drivers/net/bridge/Makefile @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +# Makefile for net selftests + +TEST_PROGS := \ + bridge-user-mtu.sh + +TEST_FILES := \ + net_forwarding_lib.sh + +include ../../../lib.mk diff --git a/tools/testing/selftests/drivers/net/bridge/bridge-user-mtu.sh b/tools/testing/selftests/drivers/net/bridge/bridge-user-mtu.sh new file mode 100755 index 000000000000..07e0ac972b00 --- /dev/null +++ b/tools/testing/selftests/drivers/net/bridge/bridge-user-mtu.sh @@ -0,0 +1,148 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Ensure a bridge MTU does not automatically change when it has been specified +# by the user. +# +# To run independently: +# make TARGETS=drivers/net/bridge kselftest + +ALL_TESTS=" + bridge_created_with_user_specified_mtu + bridge_created_without_user_specified_mtu + bridge_with_late_user_specified_mtu +" + +REQUIRE_MZ=no +NUM_NETIFS=0 +lib_dir=$(dirname "$0") +source "${lib_dir}"/net_forwarding_lib.sh + +setup_prepare() +{ + for i in 1 3 5; do + ip link add "vtest${i}" mtu 9000 type veth peer name "vtest${i}b" mtu 9000 + done +} + +cleanup() +{ + for interface in vtest1 vtest3 vtest5 br-test0 br-test1 br-test2; do + if [[ -d "/sys/class/net/${interface}" ]]; then + ip link del "${interface}" &> /dev/null + fi + done +} + +check_mtu() +{ + cur_mtu=$(<"/sys/class/net/$1/mtu") + [[ ${cur_mtu} -eq $2 ]] + exit_status=$? + return "${exit_status}" +} + +check_bridge_user_specified_mtu() +{ + if [[ -z $1 ]] + then + exit 1 + fi + mtu=$1 + + RET=0 + + ip link add dev br-test0 mtu "${mtu}" type bridge + ip link set br-test0 up + check_mtu br-test0 "${mtu}" + check_err $? "Bridge was not created with the user-specified MTU" + + check_mtu vtest1 9000 + check_err $? "vtest1 does not have MTU 9000" + + ip link set dev vtest1 master br-test0 + check_mtu br-test0 "${mtu}" + check_err $? "Bridge user-specified MTU incorrectly changed after adding an interface" + + log_test "Bridge created with user-specified MTU (${mtu})" + + ip link del br-test0 +} + +bridge_created_with_user_specified_mtu() { + # Check two user-specified MTU values + # - 1500: To ensure the default MTU (1500) is not special-cased, you + # should be able to lock a bridge to the default MTU. + # - 2000: Ensure bridges are actually created with a user-specified MTU + check_bridge_user_specified_mtu 1500 + check_bridge_user_specified_mtu 2000 +} + +bridge_created_without_user_specified_mtu() +{ + RET=0 + ip link add dev br-test1 type bridge + ip link set br-test1 up + check_mtu br-test1 1500 + check_err $? "Bridge was not created with the user-specified MTU" + + ip link set dev vtest3 master br-test1 + check_mtu br-test1 9000 + check_err $? "Bridge without user-specified MTU did not change MTU" + + log_test "Bridge created without user-specified MTU" + + ip link del br-test1 +} + +check_bridge_late_user_specified_mtu() +{ + if [[ -z $1 ]] + then + exit 1 + fi + mtu=$1 + + RET=0 + ip link add dev br-test2 type bridge + ip link set br-test2 up + check_mtu br-test2 1500 + check_err $? "Bridge was not created with default MTU (1500)" + + ip link set br-test2 mtu "${mtu}" + check_mtu br-test2 "${mtu}" + check_err $? "User-specified MTU set after creation was not set" + check_mtu vtest5 9000 + check_err $? "vtest5 does not have MTU 9000" + + ip link set dev vtest5 master br-test2 + check_mtu br-test2 "${mtu}" + check_err $? "Bridge late-specified MTU incorrectly changed after adding an interface" + + log_test "Bridge created without user-specified MTU and changed after (${mtu})" + + ip link del br-test2 +} + +bridge_with_late_user_specified_mtu() +{ + # Note: Unfortunately auto-tuning is not disabled when you set the MTU + # to it's current value, including the default of 1500. The reason is + # that dev_set_mtu_ext skips notifying any handlers if the MTU is set + # to the current value. Normally that makes sense, but is confusing + # since you might expect "ip link set br0 mtu 1500" to lock the MTU to + # 1500 but that will only happen if the MTU was not already 1500. So we + # only check a non-default value of 2000 here unlike the earlier + # bridge_created_with_user_specified_mtu test + + # Check one user-specified MTU value + # - 2000: Ensure bridges actually change to a user-specified MTU + check_bridge_late_user_specified_mtu 2000 +} + +trap cleanup EXIT + +setup_prepare +tests_run + +exit "${EXIT_STATUS}" diff --git a/tools/testing/selftests/drivers/net/bridge/net_forwarding_lib.sh b/tools/testing/selftests/drivers/net/bridge/net_forwarding_lib.sh new file mode 120000 index 000000000000..39c96828c5ef --- /dev/null +++ b/tools/testing/selftests/drivers/net/bridge/net_forwarding_lib.sh @@ -0,0 +1 @@ +../../../net/forwarding/lib.sh \ No newline at end of file -- 2.34.1

2 years, 1 month

2
3
0 0

[PATCH] kunit: debugfs: Fix unchecked dereference in debugfs_print_results()

by Richard Fitzgerald

Move the call to kunit_suite_has_succeeded() after the check that the kunit_suite pointer is valid. This was found by smatch: lib/kunit/debugfs.c:66 debugfs_print_results() warn: variable dereferenced before check 'suite' (see line 63) Signed-off-by: Richard Fitzgerald <rf(a)opensource.cirrus.com> Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Fixes: 38289a26e1b8 ("kunit: fix debugfs code to use enum kunit_status, not bool") --- lib/kunit/debugfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 9d167adfa746..382706dfb47d 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -60,12 +60,14 @@ static void debugfs_print_result(struct seq_file *seq, struct string_stream *log static int debugfs_print_results(struct seq_file *seq, void *v) { struct kunit_suite *suite = (struct kunit_suite *)seq->private; - enum kunit_status success = kunit_suite_has_succeeded(suite); + enum kunit_status success; struct kunit_case *test_case; if (!suite) return 0; + success = kunit_suite_has_succeeded(suite); + /* Print KTAP header so the debugfs log can be parsed as valid KTAP. */ seq_puts(seq, "KTAP version 1\n"); seq_puts(seq, "1..1\n"); -- 2.30.2

2 years, 1 month

1
0
0 0

[PATCH 2/3] Revert "bpf: Fix issue in verifying allow_ptr_leaks"

by Luis Gerhorst

This reverts commit d75e30dddf73449bc2d10bb8e2f1a2c446bc67a2. To mitigate Spectre v1, the verifier relies on static analysis to deduct constant pointer bounds, which can then be enforced by rewriting pointer arithmetic [1] or index masking [2]. This relies on the fact that every memory region to be accessed has a static upper bound and every date below that bound is accessible. The verifier can only rewrite pointer arithmetic or insert masking instructions to mitigate Spectre v1 if a static upper bound, below of which every access is valid, can be given. When allowing packet pointer comparisons, this introduces a way for the program to effectively construct an accessible pointer for which no static upper bound is known. Intuitively, this is obvious as a packet might be of any size and therefore 0 is the only statically known upper bound below of which every date is always accessible (i.e., none). To clarify, the problem is not that comparing two pointers can be used for pointer leaks in the same way in that comparing a pointer to a known scalar can be used for pointer leaks. That is because the "secret" components of the addresses cancel each other out if the pointers are into the same region. With [3] applied, the following malicious BPF program can be loaded into the kernel without CAP_PERFMON: r2 = *(u32 *)(r1 + 76) // data r3 = *(u32 *)(r1 + 80) // data_end r4 = r2 r4 += 1 if r4 > r3 goto exit r5 = *(u8 *)(r2 + 0) // speculatively read secret r5 &= 1 // choose bit to leak // ... side channel to leak secret bit exit: // ... This is jited to the following amd64 code which still contains the gadget: 0: endbr64 4: nopl 0x0(%rax,%rax,1) 9: xchg %ax,%ax b: push %rbp c: mov %rsp,%rbp f: endbr64 13: push %rbx 14: mov 0xc8(%rdi),%rsi // data 1b: mov 0x50(%rdi),%rdx // data_end 1f: mov %rsi,%rcx 22: add $0x1,%rcx 26: cmp %rdx,%rcx 29: ja 0x000000000000003f // branch to mispredict 2b: movzbq 0x0(%rsi),%r8 // speculative load of secret 30: and $0x1,%r8 // choose bit to leak 34: xor %ebx,%ebx 36: cmp %rbx,%r8 39: je 0x000000000000003f // branch based on secret 3b: imul $0x61,%r8,%r8 // leak using port contention side channel 3f: xor %eax,%eax 41: pop %rbx 42: leaveq 43: retq Here I'm using a port contention side channel because storing the secret to the stack causes the verifier to insert an lfence for unrelated reasons (SSB mitigation) which would terminate the speculation. As Daniel already pointed out to me, data_end is even attacker controlled as one could send many packets of sufficient length to train the branch prediction into assuming data_end >= data will never be true. When the attacker then sends a packet with insufficient data, the Spectre v1 gadget leaks the chosen bit of some value that lies behind data_end. To make it clear that the problem is not the pointer comparison but the missing masking instruction, it can be useful to transform the code above into the following equivalent pseudocode: r2 = data r3 = data_end r6 = ... // index to access, constant does not help r7 = data_end - data // only known at runtime, could be [0,PKT_MAX) if !(r6 < r7) goto exit // no masking of index in r6 happens r2 += r6 // addr. to access r5 = *(u8 *)(r2 + 0) // speculatively read secret // ... leak secret as above One idea to resolve this while still allowing for unprivileged packet access would be to always allocate a power of 2 for the packet data and then also pass the respective index mask in the skb structure. The verifier would then have to check that this index mask is always applied to the offset before a packet pointer is dereferenced. This patch does not implement this extension, but only reverts [3]. [1] 979d63d50c0c0f7bc537bf821e056cc9fe5abd38 ("bpf: prevent out of bounds speculation on pointer arithmetic") [2] b2157399cc9898260d6031c5bfe45fe137c1fbe7 ("bpf: prevent out-of-bounds speculation") [3] d75e30dddf73449bc2d10bb8e2f1a2c446bc67a2 ("bpf: Fix issue in verifying allow_ptr_leaks") Reported-by: Daniel Borkmann <daniel(a)iogearbox.net> Signed-off-by: Luis Gerhorst <gerhorst(a)amazon.de> Signed-off-by: Luis Gerhorst <gerhorst(a)cs.fau.de> Acked-by: Hagar Gamal Halim Hemdan <hagarhem(a)amazon.de> Cc: Puranjay Mohan <puranjay12(a)gmail.com> --- kernel/bpf/verifier.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index bb78212fa5b2..b415a81149ed 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -14050,12 +14050,6 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, return -EINVAL; } - /* check src2 operand */ - err = check_reg_arg(env, insn->dst_reg, SRC_OP); - if (err) - return err; - - dst_reg = &regs[insn->dst_reg]; if (BPF_SRC(insn->code) == BPF_X) { if (insn->imm != 0) { verbose(env, "BPF_JMP/JMP32 uses reserved fields\n"); @@ -14067,13 +14061,12 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, if (err) return err; - src_reg = &regs[insn->src_reg]; - if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) && - is_pointer_value(env, insn->src_reg)) { + if (is_pointer_value(env, insn->src_reg)) { verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg); return -EACCES; } + src_reg = &regs[insn->src_reg]; } else { if (insn->src_reg != BPF_REG_0) { verbose(env, "BPF_JMP/JMP32 uses reserved fields\n"); @@ -14081,6 +14074,12 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, } } + /* check src2 operand */ + err = check_reg_arg(env, insn->dst_reg, SRC_OP); + if (err) + return err; + + dst_reg = &regs[insn->dst_reg]; is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32; if (BPF_SRC(insn->code) == BPF_K) { -- 2.40.1 Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879

2 years, 1 month

5
10
0 0

[PATCH] kunit: string-stream: Allow ERR_PTR to be passed to string_stream_destroy()

by Richard Fitzgerald

Check the stream pointer passed to string_stream_destroy() for IS_ERR_OR_NULL() instead of only NULL. Whatever alloc_string_stream() returns should be safe to pass to string_stream_destroy(), and that will be an ERR_PTR. It's obviously good practise and generally helpful to also check for NULL pointers so that client cleanup code can call string_stream_destroy() unconditionally - which could include pointers that have never been set to anything and so are NULL. Signed-off-by: Richard Fitzgerald <rf(a)opensource.cirrus.com> --- lib/kunit/string-stream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index a6f3616c2048..54f4fdcbfac8 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -173,7 +173,7 @@ void string_stream_destroy(struct string_stream *stream) { KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream); - if (!stream) + if (IS_ERR_OR_NULL(stream)) return; string_stream_clear(stream); -- 2.30.2

2 years, 1 month

1
0
0 0

[PATCH v2] kunit: debugfs: Handle errors from alloc_string_stream()

by Richard Fitzgerald

In kunit_debugfs_create_suite() give up and skip creating the debugfs file if any of the alloc_string_stream() calls return an error or NULL. Only put a value in the log pointer of kunit_suite and kunit_test if it is a valid pointer to a log. This prevents the potential invalid dereference reported by smatch: lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' dereferencing possible ERR_PTR() lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' dereferencing possible ERR_PTR() Signed-off-by: Richard Fitzgerald <rf(a)opensource.cirrus.com> Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") Reviewed-by: Rae Moar <rmoar(a)google.com> --- Changes from V1: - If the alloc_string_stream() for the suite->log fails just return. Nothing has been created at this point so there's nothing to clean up. - Re-word the explanation of why the log pointers are only set if they point to a valid log. As these changes are trivial I've carried Rae Moar's Reviewed-by from V1. --- lib/kunit/debugfs.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 270d185737e6..9d167adfa746 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -109,14 +109,28 @@ static const struct file_operations debugfs_results_fops = { void kunit_debugfs_create_suite(struct kunit_suite *suite) { struct kunit_case *test_case; + struct string_stream *stream; - /* Allocate logs before creating debugfs representation. */ - suite->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(suite->log, true); + /* + * Allocate logs before creating debugfs representation. + * The suite->log and test_case->log pointer are expected to be NULL + * if there isn't a log, so only set it if the log stream was created + * successfully. + */ + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + return; + + string_stream_set_append_newlines(stream, true); + suite->log = stream; kunit_suite_for_each_test_case(suite, test_case) { - test_case->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(test_case->log, true); + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + test_case->log = stream; } suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); @@ -124,6 +138,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, suite->debugfs, suite, &debugfs_results_fops); + return; + +err: + string_stream_destroy(suite->log); + kunit_suite_for_each_test_case(suite, test_case) + string_stream_destroy(test_case->log); } void kunit_debugfs_destroy_suite(struct kunit_suite *suite) -- 2.30.2

2 years, 1 month

1
0
0 0

[PATCH 0/3] selftests: timers: Convert some more of the timers tests to KTAP

by Mark Brown

KTAP is the standard output format for selftests, providing a method for systems running the selftests to get results from individual tests rather than just a pass/fail for the test program as a whole. While many of the timers tests use KTAP some have custom output formats, let's convert a few more to KTAP to make them work better in automation. The posix_timers test made use of perror(), I've added a generic helper to kselftest.h for that since it seems like it'll be useful elsewhere. There are more tests that don't use KTAP, several of them just run a single test so don't really benefit from KTAP and there were a couple where the conversion was a bit more complex so I've left them for now. Signed-off-by: Mark Brown <broonie(a)kernel.org> --- Mark Brown (3): kselftest: Add a ksft_perror() helper selftests: timers: Convert posix_timers test to generate KTAP output selftests: timers: Convert nsleep-lat test to generate KTAP output tools/testing/selftests/kselftest.h | 14 +++++ tools/testing/selftests/timers/nsleep-lat.c | 26 ++++----- tools/testing/selftests/timers/posix_timers.c | 81 ++++++++++++++------------- 3 files changed, 67 insertions(+), 54 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230926-ktap-posix-timers-67e978466185 Best regards, -- Mark Brown <broonie(a)kernel.org>

2 years, 1 month

2
4
0 0

[PATCH] kunit: debugfs: Handle errors from alloc_string_stream()

by Richard Fitzgerald

In kunit_debugfs_create_suite() give up and skip creating the debugfs file if any of the alloc_string_stream() calls return an error or NULL. Only put a value in the log pointer of kunit_suite and kunit_test if it is a valid pointer to a log. This prevents the potential invalid dereference reported by smatch: lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' dereferencing possible ERR_PTR() lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' dereferencing possible ERR_PTR() Signed-off-by: Richard Fitzgerald <rf(a)opensource.cirrus.com> Reported-by: Dan Carpenter <dan.carpenter(a)linaro.org> Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") --- lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 270d185737e6..73075ca6e88c 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { void kunit_debugfs_create_suite(struct kunit_suite *suite) { struct kunit_case *test_case; + struct string_stream *stream; - /* Allocate logs before creating debugfs representation. */ - suite->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(suite->log, true); + /* + * Allocate logs before creating debugfs representation. + * The log pointer must be NULL if there isn't a log so only + * set it if the log stream was created successfully. + */ + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + suite->log = stream; kunit_suite_for_each_test_case(suite, test_case) { - test_case->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(test_case->log, true); + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + test_case->log = stream; } suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, suite->debugfs, suite, &debugfs_results_fops); + return; + +err: + string_stream_destroy(suite->log); + kunit_suite_for_each_test_case(suite, test_case) + string_stream_destroy(test_case->log); } void kunit_debugfs_destroy_suite(struct kunit_suite *suite) -- 2.30.2

2 years, 1 month

3
3
0 0

[PATCH v4 0/4] kunit: Fix some bugs in kunit

by Jinjie Ruan

The test_cases is not freed in kunit_free_suite_set(). And the copy pointer may be moved in kunit_filter_suites(). The filtered_suite and filtered_suite->test_cases allocated in the last kunit_filter_attr_tests() in last inner for loop may be leaked if kunit_filter_suites() fails. If kunit_filter_suites() succeeds, not only copy but also filtered_suite and filtered_suite->test_cases should be freed. Changes in v4: - Make free_suite_set() take a void * for the 4th patch. - Add Suggested-by and Reviewed-by. - Correct the fix tag. Changes in v3: - Update the kfree_at_end() to use kunit_free_suite_set() for 4th patch. - Update the commit message for the 4th patch. Changes in v2: - Add Reviewed-by. - Add the memory leak backtrace for the 4th patch. - Remove the unused func kernel test robot noticed for the 4th patch. - Update the commit message for the 4th patch. Jinjie Ruan (4): kunit: Fix missed memory release in kunit_free_suite_set() kunit: Fix the wrong kfree of copy for kunit_filter_suites() kunit: Fix possible memory leak in kunit_filter_suites() kunit: test: Fix the possible memory leak in executor_test lib/kunit/executor.c | 23 +++++++++++++++++------ lib/kunit/executor_test.c | 36 ++++++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 20 deletions(-) -- 2.34.1

2 years, 1 month

2
5
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

Linux-kselftest-mirror