Re: [PATCH] tools/latency-collector: fix -Wformat-security compile warns
On 4/3/24 19:10, Shuah Khan wrote:
Fix the following -Wformat-security compile warnings adding missing
format arguments:
latency-collector.c: In function ‘show_available’:
latency-collector.c:938:17: warning: format not a string literal and
no format arguments [-Wformat-security]
938 | warnx(no_tracer_msg);
| ^
latency-collector.c:943:17: warning: format not a string literal and
no format arguments [-Wformat-security]
943 | warnx(no_latency_tr_msg);
| ^
latency-collector.c: In function ‘find_default_tracer’:
latency-collector.c:986:25: warning: format not a string literal and
no format arguments [-Wformat-security]
986 | errx(EXIT_FAILURE, no_tracer_msg);
|
^~~~
latency-collector.c: In function ‘scan_arguments’:
latency-collector.c:1881:33: warning: format not a string literal and
no format arguments [-Wformat-security]
1881 | errx(EXIT_FAILURE, no_tracer_msg);
| ^~~~
Signed-off-by: Shuah Khan
---
tools/tracing/latency/latency-collector.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/tracing/latency/latency-collector.c
b/tools/tracing/latency/latency-collector.c
index 0fd9c747d396..cf263fe9deaf 100644
--- a/tools/tracing/latency/latency-collector.c
+++ b/tools/tracing/latency/latency-collector.c
@@ -935,12 +935,12 @@ static void show_available(void)
}
if (!tracers) {
- warnx(no_tracer_msg);
+ warnx("%s", no_tracer_msg);
return;
}
if (!found) {
- warnx(no_latency_tr_msg);
+ warnx("%s", no_latency_tr_msg);
tracefs_list_free(tracers);
return;
}
@@ -983,7 +983,7 @@ static const char *find_default_tracer(void)
for (i = 0; relevant_tracers[i]; i++) {
valid = tracer_valid(relevant_tracers[i], );
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (valid)
return relevant_tracers[i];
}
@@ -1878,7 +1878,7 @@ static void scan_arguments(int argc, char *argv[])
}
valid = tracer_valid(current_tracer, );
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (!valid)
errx(EXIT_FAILURE,
"The tracer %s is not supported by your kernel!\n", current_tracer);
Any thoughts on this patch?
thanks,
-- Shuah
[PATCH] tools/latency-collector: fix -Wformat-security compile warns
Fix the following -Wformat-security compile warnings adding missing
format arguments:
latency-collector.c: In function ‘show_available’:
latency-collector.c:938:17: warning: format not a string literal and
no format arguments [-Wformat-security]
938 | warnx(no_tracer_msg);
| ^
latency-collector.c:943:17: warning: format not a string literal and
no format arguments [-Wformat-security]
943 | warnx(no_latency_tr_msg);
| ^
latency-collector.c: In function ‘find_default_tracer’:
latency-collector.c:986:25: warning: format not a string literal and
no format arguments [-Wformat-security]
986 | errx(EXIT_FAILURE, no_tracer_msg);
|
^~~~
latency-collector.c: In function ‘scan_arguments’:
latency-collector.c:1881:33: warning: format not a string literal and
no format arguments [-Wformat-security]
1881 | errx(EXIT_FAILURE, no_tracer_msg);
| ^~~~
Signed-off-by: Shuah Khan
---
tools/tracing/latency/latency-collector.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/tracing/latency/latency-collector.c
b/tools/tracing/latency/latency-collector.c
index 0fd9c747d396..cf263fe9deaf 100644
--- a/tools/tracing/latency/latency-collector.c
+++ b/tools/tracing/latency/latency-collector.c
@@ -935,12 +935,12 @@ static void show_available(void)
}
if (!tracers) {
- warnx(no_tracer_msg);
+ warnx("%s", no_tracer_msg);
return;
}
if (!found) {
- warnx(no_latency_tr_msg);
+ warnx("%s", no_latency_tr_msg);
tracefs_list_free(tracers);
return;
}
@@ -983,7 +983,7 @@ static const char *find_default_tracer(void)
for (i = 0; relevant_tracers[i]; i++) {
valid = tracer_valid(relevant_tracers[i], );
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (valid)
return relevant_tracers[i];
}
@@ -1878,7 +1878,7 @@ static void scan_arguments(int argc, char *argv[])
}
valid = tracer_valid(current_tracer, );
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (!valid)
errx(EXIT_FAILURE,
"The tracer %s is not supported by your kernel!\n", current_tracer);
--
2.40.1
Re: [PATCH v3] Documentation: dev-tools: Add Testing Overview
On 4/14/21 11:40 PM, David Gow wrote: The kernel now has a number of testing and debugging tools, and we've seen a bit of confusion about what the differences between them are. Add a basic documentation outlining the testing tools, when to use each, and how they interact. This is a pretty quick overview rather than the idealised "kernel testing guide" that'd probably be optimal, but given the number of times questions like "When do you use KUnit and when do you use Kselftest?" are being asked, it seemed worth at least having something. Hopefully this can form the basis for more detailed documentation later. Signed-off-by: David Gow Reviewed-by: Marco Elver Reviewed-by: Daniel Latypov --- Thanks again. Assuming no-one has any objections, I think this is good to go. -- David Changes since v2: https://lore.kernel.org/linux-kselftest/20210414081428.337494-1-david...@google.com/ - A few typo fixes (Thanks Daniel) - Reworded description of dynamic analysis tools. - Updated dev-tools index page to not use ':doc:' syntax, but to provide a path instead. - Added Marco and Daniel's Reviewed-by tags. Changes since v1: https://lore.kernel.org/linux-kselftest/20210410070529.4113432-1-david...@google.com/ - Note KUnit's speed and that one should provide selftests for syscalls - Mention lockdep as a Dynamic Analysis Tool - Refer to "Dynamic Analysis Tools" instead of "Sanitizers" - A number of minor formatting tweaks and rewordings for clarity Documentation/dev-tools/index.rst| 4 + Documentation/dev-tools/testing-overview.rst | 117 +++ 2 files changed, 121 insertions(+) create mode 100644 Documentation/dev-tools/testing-overview.rst diff --git a/Documentation/dev-tools/index.rst b/Documentation/dev-tools/index.rst index 1b1cf4f5c9d9..929d916ffd4c 100644 --- a/Documentation/dev-tools/index.rst +++ b/Documentation/dev-tools/index.rst @@ -7,6 +7,9 @@ be used to work on the kernel. For now, the documents have been pulled together without any significant effort to integrate them into a coherent whole; patches welcome! +A brief overview of testing-specific tools can be found in +Documentation/dev-tools/testing-overview.rst + .. class:: toc-title Table of contents @@ -14,6 +17,7 @@ whole; patches welcome! .. toctree:: :maxdepth: 2 + testing-overview coccinelle sparse kcov diff --git a/Documentation/dev-tools/testing-overview.rst b/Documentation/dev-tools/testing-overview.rst new file mode 100644 index ..b5b46709969c --- /dev/null +++ b/Documentation/dev-tools/testing-overview.rst @@ -0,0 +1,117 @@ +.. SPDX-License-Identifier: GPL-2.0 + + +Kernel Testing Guide + + + +There are a number of different tools for testing the Linux kernel, so knowing +when to use each of them can be a challenge. This document provides a rough +overview of their differences, and how they fit together. + + +Writing and Running Tests += + +The bulk of kernel tests are written using either the kselftest or KUnit +frameworks. These both provide infrastructure to help make running tests and +groups of tests easier, as well as providing helpers to aid in writing new +tests. + +If you're looking to verify the behaviour of the Kernel — particularly specific +parts of the kernel — then you'll want to use KUnit or kselftest. + + +The Difference Between KUnit and kselftest +-- + +KUnit (Documentation/dev-tools/kunit/index.rst) is an entirely in-kernel system +for "white box" testing: because test code is part of the kernel, it can access +internal structures and functions which aren't exposed to userspace. + +KUnit tests therefore are best written against small, self-contained parts +of the kernel, which can be tested in isolation. This aligns well with the +concept of 'unit' testing. + +For example, a KUnit test might test an individual kernel function (or even a +single codepath through a function, such as an error handling case), rather +than a feature as a whole. + +This also makes KUnit tests very fast to build and run, allowing them to be +run frequently as part of the development process. + +There is a KUnit test style guide which may give further pointers in +Documentation/dev-tools/kunit/style.rst + + +kselftest (Documentation/dev-tools/kselftest.rst), on the other hand, is +largely implemented in userspace, and tests are normal userspace scripts or +programs. + +This makes it easier to write more complicated tests, or tests which need to +manipulate the overall system state more (e.g., spawning processes, etc.). +However, it's not possible to call kernel functions directly from kselftest. +This means that only kernel functionality which is exposed to userspace somehow +(e.g. by a syscall, device, filesystem, etc.) can be tested with kselftest. To +work around this, some tests include a companion kernel module which exposes +more
Re: [PATCH 5.4 00/73] 5.4.114-rc1 review
On 4/19/21 7:05 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.114 release. There are 73 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.114-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 000/103] 5.10.32-rc1 review
On 4/19/21 7:05 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.32 release. There are 103 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.32-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/122] 5.11.16-rc1 review
On 4/19/21 7:04 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.16 release. There are 122 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.16-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/16/21 1:33 PM, Igor Torrente wrote: On 4/15/21 2:25 PM, Shuah Khan wrote: On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h | 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. I tried and everything worked fine. Thank you. Reviewed-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.4 00/38] 4.4.267-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.4.267 release. There are 38 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.267-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.14 00/68] 4.14.231-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.14.231 release. There are 68 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.231-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y and the diffstat can be found below. thanks, greg k-h - Pseudo-Shortlog of commits: Greg Kroah-Hartman Linux 4.14.231-rc1 Juergen Gross xen/events: fix setting irq affinity Arnaldo Carvalho de Melo perf map: Tighten snprintf() string precision to pass gcc check on some 32-bit arches Florian Westphal netfilter: x_tables: fix compat match/target pad out-of-bound write Florian Fainelli net: phy: broadcom: Only advertise EEE for supported modes Yufen Yu block: only update parent bi_status when bio fail Bob Peterson gfs2: report "already frozen/thawed" errors Arnd Bergmann drm/imx: imx-ldb: fix out of bounds array access warning Suzuki K Poulose KVM: arm64: Disable guest access to trace filter controls Suzuki K Poulose KVM: arm64: Hide system instruction access to Trace registers Greg Kroah-Hartman Revert "cifs: Set CIFS_MOUNT_USE_PREFIX_PATH flag on setting cifs_sb->prepath." Alexander Aring net: ieee802154: stop dump llsec params for monitors Alexander Aring net: ieee802154: forbid monitor for del llsec seclevel Alexander Aring net: ieee802154: forbid monitor for set llsec params Alexander Aring net: ieee802154: fix nl802154 del llsec devkey Alexander Aring net: ieee802154: fix nl802154 add llsec key Alexander Aring net: ieee802154: fix nl802154 del llsec dev Alexander Aring net: ieee802154: fix nl802154 del llsec key Alexander Aring net: ieee802154: nl-mac: fix check on panid Pavel Skripkin net: mac802154: Fix general protection fault Pavel Skripkin drivers: net: fix memory leak in peak_usb_create_dev Pavel Skripkin drivers: net: fix memory leak in atusb_probe Phillip Potter net: tun: set tun->dev->addr_len during TUNSETLINK processing Du Cheng cfg80211: remove WARN_ON() in cfg80211_sme_connect Shuah Khan usbip: fix vudc usbip_sockfd_store races leading to gpf Samuel Mendoza-Jonas net/ncsi: Avoid GFP_KERNEL in response handler Samuel Mendoza-Jonas net/ncsi: Refactor MAC, VLAN filters Samuel Mendoza-Jonas net/ncsi: Add generic netlink family Samuel Mendoza-Jonas net/ncsi: Don't return error on normal response Samuel Mendoza-Jonas net/ncsi: Improve general state logging Wei Yongjun net/ncsi: Make local function ncsi_get_filter() static Krzysztof Kozlowski clk: socfpga: fix iomem pointer cast on 64-bit Potnuri Bharat Teja RDMA/cxgb4: check for ipv6 address properly while destroying listener Raed Salem net/mlx5: Fix placement of log_max_flow_counter Alexander Gordeev s390/cpcmd: fix inline assembly register clobbering Zqiang workqueue: Move the position of debug_work_activate() in __queue_work() Lukasz Bartosik clk: fix invalid usage of list cursor in unregister Lukasz Bartosik clk: fix invalid usage of list cursor in register Arnd Bergmann soc/fsl: qbman: fix conflicting alignment attributes Bastian Germann ASoC: sunxi: sun4i-codec: fill ASoC card owner Milton Miller net/ncsi: Avoid channel_monitor hrtimer deadlock Stefan Riedmueller ARM: dts: imx6: pbab01: Set vmmc supply for both SD interfaces Lv Yunlong net:tipc: Fix a double free in tipc_sk_mcast_rcv Claudiu Manoil gianfar: Handle error code at MAC address change Eric Dumazet sch_red: fix off-by-one checks in red_check_params() Shyam Sundar S K amd-xgbe: Update DMA coherency values Shengjiu Wang ASoC: wm8960: Fix wrong bclk and lrclk with pll enabled for some chips Geert Uytterhoeven regulator: bd9571mwv: Fix AVS and DVFS voltage range Wolfram Sang i2c: turn recovery error on init to debug Shuah Khan usbip: synchronize event handler with sysfs code paths Shuah Khan usbip: stub-dev synchronize sysfs code paths Shuah Khan usbip: add sysfs_lock to synchronize sysfs code paths Pavel Tikhomirov net: sched: sch_teql: fix null-pointer dereference Eric Dumazet net: ensure mac header is set in virtio_net_hdr_to_skb() Tetsuo Handa batman-adv: initialize "struct batadv_tvlv_tt_vlan_data"->reserved field Marek Behún ARM: dts: turris-omnia: configure LED[2]/INTn pin as in
Re: [PATCH 4.9 00/47] 4.9.267-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.267 release. There are 47 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.267-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.14 00/68] 4.14.231-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.14.231 release. There are 68 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.231-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 00/18] 5.4.113-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.113 release. There are 18 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.113-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/13] 4.19.188-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.188 release. There are 13 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.188-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/25] 5.10.31-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.31 release. There are 25 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.31-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 00/23] 5.11.15-rc1 review
On 4/15/21 8:48 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.15 release. There are 23 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.15-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h| 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. thanks, -- Shuah
Re: [PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs
On 4/14/21 9:26 AM, Shuah Khan wrote:
On 4/14/21 6:55 AM, Luis Chamberlain wrote:
Shuah, a question for you toward the end here.
On Wed, Apr 14, 2021 at 02:24:05PM +0530, Anirudh Rayabharam wrote:
This use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.
The root cause here is that all code paths that abort the fw load
don't delete it from the pending list. For example:
_request_firmware()
-> fw_abort_batch_reqs()
-> fw_state_aborted()
To fix this, delete the fw_priv from the list in __fw_set_state() if
the new state is DONE or ABORTED. This way, all aborts will remove
the fw_priv from the list. Accordingly, remove calls to list_del_init
that were being made before calling fw_state_(aborted|done).
Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
list if it is already aborted. Instead, just jump out and return early.
Fixes: bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam
---
Changes in v3:
Modified the patch to incorporate suggestions by Luis Chamberlain in
order to fix the root cause instead of applying a "band-aid" kind of
fix.
https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/
Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
checkpatch.pl. The error was regarding the format for referring to
another commit "commit ("oneline")". The warning was for line
longer than 75 chars.
---
drivers/base/firmware_loader/fallback.c | 8 ++--
drivers/base/firmware_loader/firmware.h | 6 +-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..73581b6998b4 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
if (fw_sysfs_done(fw_priv))
return;
- list_del_init(_priv->pending_list);
fw_state_aborted(fw_priv);
}
@@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct
device *dev,
* Same logic as fw_load_abort, only the DONE bit
* is ignored and we set ABORT only on failure.
*/
- list_del_init(_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
@@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct
fw_sysfs *fw_sysfs, long timeout)
}
mutex_lock(_lock);
+ if (fw_state_is_aborted(fw_priv)) {
+ mutex_unlock(_lock);
+ retval = -EAGAIN;
+ goto out;
+ }
Thanks for the quick follow up!
This would regress commit 76098b36b5db1 ("firmware: send -EINTR on
signal abort on fallback mechanism") which I had mentioned in my follow
up email you posted a link to. It would regress it since the condition
is just being met earlier and you nullify the effort. So essentially
on Android you would make not being able to detect signal handlers
like the SIGCHLD signal sent to init, if init was the same process
dealing with the sysfs fallback firmware upload.
The way I dealt with this in my patch was I decided to return -EINTR
in the earlier case in the hunk you added, instead of -EAGAIN. In
addition to this, later on fw_load_sysfs_fallback() when
fw_sysfs_wait_timeout() is used that would also deal with checking
for error codes on wait, and only then check if it was a signal
that cancelled things (the check for -ERESTARTSYS). We therefore
only send to userspace -EAGAIN when the wait really did hit the
timeout.
But also note that my change added a check for
fw_state_is_aborted(fw_priv) inside fw_sysfs_wait_timeout(),
as that was a recently intended goal.
In either case I documented well *why* we do these error checks
before sending a code to userspace on fw_sysfs_wait_timeout() since
otherwise it would be easy to regress that code, so please also
document that as I did.
I'll re-iterate again also:
Shuah's commit 0542ad88fbdd81bb ("firmware loader: Fix
_request_firmware_load() return val for fw load abort") also
wanted to
distinguish the timeout vs -ENOMEM, but for some reason in the
timeout
case -EAGAIN was being sent back to userspace. I am no longer sure if
that is a good idea, but since we started doing that at some point I
guess we want to keep that behaviour.
Shuah, can you think of any reason to retain -EAGAIN other than you
in
Re: [PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs
On 4/14/21 6:55 AM, Luis Chamberlain wrote:
Shuah, a question for you toward the end here.
On Wed, Apr 14, 2021 at 02:24:05PM +0530, Anirudh Rayabharam wrote:
This use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.
The root cause here is that all code paths that abort the fw load
don't delete it from the pending list. For example:
_request_firmware()
-> fw_abort_batch_reqs()
-> fw_state_aborted()
To fix this, delete the fw_priv from the list in __fw_set_state() if
the new state is DONE or ABORTED. This way, all aborts will remove
the fw_priv from the list. Accordingly, remove calls to list_del_init
that were being made before calling fw_state_(aborted|done).
Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
list if it is already aborted. Instead, just jump out and return early.
Fixes: bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam
---
Changes in v3:
Modified the patch to incorporate suggestions by Luis Chamberlain in
order to fix the root cause instead of applying a "band-aid" kind of
fix.
https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/
Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
checkpatch.pl. The error was regarding the format for referring to
another commit "commit ("oneline")". The warning was for line
longer than 75 chars.
---
drivers/base/firmware_loader/fallback.c | 8 ++--
drivers/base/firmware_loader/firmware.h | 6 +-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..73581b6998b4 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
if (fw_sysfs_done(fw_priv))
return;
- list_del_init(_priv->pending_list);
fw_state_aborted(fw_priv);
}
@@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct device *dev,
* Same logic as fw_load_abort, only the DONE bit
* is ignored and we set ABORT only on failure.
*/
- list_del_init(_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
@@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct fw_sysfs
*fw_sysfs, long timeout)
}
mutex_lock(_lock);
+ if (fw_state_is_aborted(fw_priv)) {
+ mutex_unlock(_lock);
+ retval = -EAGAIN;
+ goto out;
+ }
Thanks for the quick follow up!
This would regress commit 76098b36b5db1 ("firmware: send -EINTR on
signal abort on fallback mechanism") which I had mentioned in my follow
up email you posted a link to. It would regress it since the condition
is just being met earlier and you nullify the effort. So essentially
on Android you would make not being able to detect signal handlers
like the SIGCHLD signal sent to init, if init was the same process
dealing with the sysfs fallback firmware upload.
The way I dealt with this in my patch was I decided to return -EINTR
in the earlier case in the hunk you added, instead of -EAGAIN. In
addition to this, later on fw_load_sysfs_fallback() when
fw_sysfs_wait_timeout() is used that would also deal with checking
for error codes on wait, and only then check if it was a signal
that cancelled things (the check for -ERESTARTSYS). We therefore
only send to userspace -EAGAIN when the wait really did hit the
timeout.
But also note that my change added a check for
fw_state_is_aborted(fw_priv) inside fw_sysfs_wait_timeout(),
as that was a recently intended goal.
In either case I documented well *why* we do these error checks
before sending a code to userspace on fw_sysfs_wait_timeout() since
otherwise it would be easy to regress that code, so please also
document that as I did.
I'll re-iterate again also:
Shuah's commit 0542ad88fbdd81bb ("firmware loader: Fix
_request_firmware_load() return val for fw load abort") also wanted to
distinguish the timeout vs -ENOMEM, but for some reason in the timeout
case -EAGAIN was being sent back to userspace. I am no longer sure if
that is a good idea, but since we started doing that at some point I
guess we want to keep that behaviour.
Shuah, can you think of any
Re: [PATCH 4.19 00/66] 4.19.187-rc1 review
On 4/12/21 2:40 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.187 release. There are 66 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.187-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. No problems with wifi this time. I will be on the lookout for this in the future. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 000/111] 5.4.112-rc1 review
On 4/12/21 2:39 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.112 release. There are 111 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.112-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 000/188] 5.10.30-rc1 review
On 4/12/21 2:38 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.30 release. There are 188 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.30-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/210] 5.11.14-rc1 review
On 4/12/21 2:38 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.14 release. There are 210 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.14-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/13] 4.9.266-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.266 release. There are 13 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.266-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/18] 4.19.186-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.186 release. There are 18 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.186-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h I am seeing a new warn - will debug later on today and let you know what I find: WARNING: CPU: 9 PID: 0 at drivers/net/wireless/ath/ath10k/htt_rx.c:46 ath10k_htt_rx_pop_paddr+0xde/0x100 [ath10k_core] Modules linked in: cmac algif_hash algif_skcipher af_alg bnep arc4 nls_iso8859_1 wmi_bmof snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi edac_mce_amd snd_hda_intel snd_hda_codec kvm_amd snd_hda_core ccp snd_hwdep kvm snd_pcm snd_seq_midi crct10dif_pclmul snd_seq_midi_event ghash_clmulni_intel pcbc snd_rawmidi ath10k_pci snd_seq ath10k_core aesni_intel ath snd_seq_device rtsx_usb_ms btusb aes_x86_64 snd_timer crypto_simd btrtl cryptd joydev btbcm glue_helper memstick mac80211 snd btintel input_leds bluetooth soundcore cfg80211 ecdh_generic video wmi mac_hid sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 hid_generic rtsx_usb_sdmmc usbhid rtsx_usb hid crc32_pclmul uas i2c_piix4 r8169 ahci realtek usb_storage libahci gpio_amdpt gpio_generic CPU: 9 PID: 0 Comm: swapper/9 Not tainted 4.19.186-rc1+ #24 Hardware name: LENOVO 90Q30008US/3728, BIOS O4ZKT1CA 09/16/2020 RIP: 0010:ath10k_htt_rx_pop_paddr+0xde/0x100 [ath10k_core] Code: 02 00 00 48 85 c9 74 30 4c 8b 49 28 4d 85 c9 74 1e 48 8b 30 45 31 c0 b9 02 00 00 00 e8 9b 27 ca cc 4c 89 e0 4c 8b 65 f8 c9 c3 <0f> 0b 45 31 e4 4c 89 e0 4c 8b 65 f8 c9 c3 48 8b 0d 1d df 4c cd eb RSP: 0018:8d81bf043da0 EFLAGS: 00010246 RAX: RBX: 8d81927b2150 RCX: 8d81b8c01580 RDX: 0008 RSI: ff708a80 RDI: 8d81b8c01e78 RBP: 8d81bf043da8 R08: 0020 R09: R10: dd1f0f40f300 R11: 000ff000 R12: 8d81b8c02068 R13: 8d81b8c01580 R14: 8d81927b2148 R15: 8d81b8c01580 FS: () GS:8d81bf04() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 55a2c99a2000 CR3: 0003de8d4000 CR4: 00340ee0 Call Trace: ath10k_htt_txrx_compl_task+0x58d/0xe70 [ath10k_core] ath10k_pci_napi_poll+0x52/0x110 [ath10k_pci] net_rx_action+0x13c/0x350 __do_softirq+0xd4/0x2ae irq_exit+0x9c/0xe0 do_IRQ+0x86/0xe0 common_interrupt+0xf/0xf RIP: 0010:cpuidle_enter_state+0x10b/0x2c0 Code: ff e8 f9 68 85 ff 80 7d c7 00 74 17 9c 58 0f 1f 44 00 00 f6 c4 02 0f 85 97 01 00 00 31 ff e8 6c 5d 8b ff fb 66 0f 1f 44 00 00 <48> b8 ff ff ff ff f3 01 00 00 4c 2b 7d c8 ba ff ff ff 7f 49 39 c7 RSP: 0018:b11e01a77e50 EFLAGS: 0246 ORIG_RAX: ffda RAX: 8d81bf0626c0 RBX: 8d81b2690400 RCX: 0006e8cb49d2 RDX: 0689 RSI: 0006e8cb49d2 RDI: RBP: b11e01a77e90 R08: 0006e8cb505b R09: 0e29 R10: 0f04 R11: 8d81bf061528 R12: 0003 R13: 8df9e860 R14: 8df9e980 R15: 0006e8cb505b cpuidle_enter+0x17/0x20 thanks, -- Shuah
Re: [PATCH 5.4 00/23] 5.4.111-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.111 release. There are 23 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.111-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/41] 5.10.29-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.29 release. There are 41 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.29-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 00/45] 5.11.13-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.13 release. There are 45 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.13-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:00 PM, Shuah Khan wrote:
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be
programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
Unfortunatly, there is no documentation of since which generation
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
that the buggy platforms are less likely to be in used, and it should
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Tested-by: Shuah Khan
Revert + this patch - same as my test on Ryzen 5
On AMD Ryzen 7 4700G with Radeon Graphics
These look real odd to me. Let me know if I should look further.
sudo ./perf stat -e 'amd_iommu_0/cmd_processed/,
amd_iommu_0/cmd_processed_inv/, amd_iommu_0/ign_rd_wr_mmio_1ff8h/,
amd_iommu_0/int_dte_hit/, amd_iommu_0/int_dte_mis/,
amd_iommu_0/mem_dte_hit/, amd_iommu_0/mem_dte_mis/,
amd_iommu_0/mem_iommu_tlb_pde_hit/, amd_iommu_0/mem_iommu_tlb_pde_mis/,
amd_iommu_0/mem_iommu_tlb_pte_hit/, amd_iommu_0/mem_iommu_tlb_pte_mis/,
amd_iommu_0/mem_pass_excl/, amd_iommu_0/mem_pass_pretrans/,
amd_iommu_0/mem_pass_untrans/, amd_iommu_0/mem_target_abort/,
amd_iommu_0/mem_trans_total/, amd_iommu_0/page_tbl_read_gst/,
amd_iommu_0/page_tbl_read_nst/, amd_iommu_0/page_tbl_read_tot/,
amd_iommu_0/smi_blk/, amd_iommu_0/smi_recv/, amd_iommu_0/tlb_inv/,
amd_iommu_0/vapic_int_guest/, amd_iommu_0/vapic_int_non_guest/' sleep 10
Performance counter stats for 'system wide':
17,761,952,514,865,374 amd_iommu_0/cmd_processed/
(33.28%)
18,582,155,570,607,472 amd_iommu_0/cmd_processed_inv/
(33.32%)
0 amd_iommu_0/ign_rd_wr_mmio_1ff8h/
(33.36%)
5,056,087,645,262,255 amd_iommu_0/int_dte_hit/
(33.40%)
32,831,106,446,308,888 amd_iommu_0/int_dte_mis/
(33.44%)
13,461,819,655,591,296 amd_iommu_0/mem_dte_hit/
(33.45%)
208,555,436,221,050,464 amd_iommu_0/mem_dte_mis/
(33.47%)
196,824,154,635,609,888 amd_iommu_0/mem_iommu_tlb_pde_hit/
(33.46%)
193,552,630,440,410,144 amd_iommu_0/mem_iommu_tlb_pde_mis/
(33.45%)
176,936,647,809,098,368 amd_iommu_0/mem_iommu_tlb_pte_hit/
(33.41%)
184,737,401,623,626,464 amd_iommu_0/mem_iommu_tlb_pte_mis/
(33.37%)
0 amd_iommu_0/mem_pass_excl/
(33.33%)
0 amd_iommu_0/mem_pass_pretrans/
(33.30%)
0 amd_iommu_0/mem_pass_untrans/
(33.28%)
0 amd_iommu_0/mem_target_abort/
(33.27%)
245,383,212,924,004,288 amd_iommu_0/mem_trans_total/
(33.27%)
0 amd_iommu_0/page_tbl_read_gst/
(33.28%)
262,267,045,917,967,264 amd_iommu_0/page_tbl_read_nst/
(33.27%)
256,308,216,913,137,600 amd_iommu_0/page_tbl_read_tot/
(33.28%)
0 amd_iommu_0/smi_blk/
(33.27%)
0 amd_iommu_0/smi_recv/
(33.27%)
0 amd_iommu_0/tlb_inv/
(33.27%)
0 amd_iommu_0/vapic_int_guest/
(33.26%)
38,913,544,420,579,888 amd_iommu_0/vapic_int_non_guest/
(33.27%)
10.003967760 seconds time elapsed
thanks,
-- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
Unfortunatly, there is no documentation of since which generation
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
that the buggy platforms are less likely to be in used, and it should
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Tested-by: Shuah Khan
thanks,
-- Shuah
Results with this patch on AMD Ryzen 5 PRO 2400GE w/ Radeon Vega
Graphics
sudo ./perf stat -e 'amd_iommu_0/cmd_processed/,
amd_iommu_0/cmd_processed_inv/, amd_iommu_0/ign_rd_wr_mmio_1ff8h/,
amd_iommu_0/int_dte_hit/, amd_iommu_0/int_dte_mis/,
amd_iommu_0/mem_dte_hit/, amd_iommu_0/mem_dte_mis/,
amd_iommu_0/mem_iommu_tlb_pde_hit/, amd_iommu_0/mem_iommu_tlb_pde_mis/,
amd_iommu_0/mem_iommu_tlb_pte_hit/, amd_iommu_0/mem_iommu_tlb_pte_mis/,
amd_iommu_0/mem_pass_excl/, amd_iommu_0/mem_pass_pretrans/,
amd_iommu_0/mem_pass_untrans/, amd_iommu_0/mem_target_abort/,
amd_iommu_0/mem_trans_total/, amd_iommu_0/page_tbl_read_gst/,
amd_iommu_0/page_tbl_read_nst/, amd_iommu_0/page_tbl_read_tot/,
amd_iommu_0/smi_blk/, amd_iommu_0/smi_recv/, amd_iommu_0/tlb_inv/,
amd_iommu_0/vapic_int_guest/, amd_iommu_0/vapic_int_non_guest/' sleep 10
Performance counter stats for 'system wide':
156 amd_iommu_0/cmd_processed/
(33.30%)
80 amd_iommu_0/cmd_processed_inv/
(33.38%)
0 amd_iommu_0/ign_rd_wr_mmio_1ff8h/
(33.40%)
0 amd_iommu_0/int_dte_hit/
(33.43%)
325 amd_iommu_0/int_dte_mis/
(33.44%)
1,951 amd_iommu_0/mem_dte_hit/
(33.45%)
7,589 amd_iommu_0/mem_dte_mis/
(33.49%)
325 amd_iommu_0/mem_iommu_tlb_pde_hit/
(33.45%)
2,460 amd_iommu_0/mem_iommu_tlb_pde_mis/
(33.41%)
2,510 amd_iommu_0/mem_iommu_tlb_pte_hit/
(33.38%)
5,526 amd_iommu_0/mem_iommu_tlb_pte_mis/
(33.33%)
0 amd_iommu_0/mem_pass_excl/
(33.29%)
0 amd_iommu_0/mem_pass_pretrans/
(33.28%)
1,556 amd_iommu_0/mem_pass_untrans/
(33.27%)
0 amd_iommu_0/mem_target_abort/
(33.26%)
3,112 amd_iommu_0/mem_trans_total/
(33.29%)
0 amd_iommu_0/page_tbl_read_gst/
(33.29%)
1,813 amd_iommu_0/page_tbl_read_nst/
(33.25%)
2,242 amd_iommu_0/page_tbl_read_tot/
(33.27%)
0 amd_iommu_0/smi_blk/
(33.29%)
0 amd_iommu_0/smi_recv/
(33.28%)
0 amd_iommu_0/tlb_inv/
(33.28%)
0 amd_iommu_0/vapic_int_guest/
(33.25%)
0 amd_iommu_0/vapic_int_non_guest/
(33.26%)
10.003200316 seconds time elapsed
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 10:37 AM, Shuah Khan wrote:
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be
programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
unnecessarily
Unfortunatly, there is no documentation of since which generation
Unfortunately,
Rephrase suggestion:
Unfortunately, it is unclear when the PMC HW bug fixed.
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
Based
that the buggy platforms are less likely to be in used, and it should
in use
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/amd/init.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 648cdfd03074..247cdda5d683 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct
acpi_table_header *table)
return 0;
}
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8
cntr,
- u8 fxn, u64 *value, bool is_write);
-
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
+ u64 val;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg = 0;
Why not leave this u64 val here? Having the pdev assignment as the
first line makes it easier to read/follow.
if (!iommu_feature(iommu, FEATURE_PC))
return;
amd_iommu_pc_present = true;
- /* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, false))
- goto pc_false;
-
- /* Check if the performance counters can be written to */
- if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, , true)) ||
- (iommu_pc_get_set_reg(iommu, 0, 0, 0, , false)) ||
- (val != val2))
Aha - this is going away anyway. Please ignore my comment on 1/2
about parenthesis around (val != val2) being unnecessary.
- goto pc_false;
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, true))
- goto pc_false;
-
pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
@@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct
amd_iommu *iommu)
iommu->max_counters = (u8) ((val >> 7) & 0xf);
return;
-
-pc_false:
- pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
- amd_iommu_pc_present = false;
- return;
}
static ssize_t amd_iommu_show_cap(struct device *dev,
thanks,
-- Shuah
Re: [PATCH 1/2] Revert "iommu/amd: Fix performance counter initialization"
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
From: Paul Menzel
This reverts commit 6778ff5b21bd8e78c8bd547fd66437cf2657fd9b.
The original commit tries to address an issue, where PMC power-gating
causing the IOMMU PMC pre-init test to fail on certain desktop/mobile
platforms where the power-gating is normally enabled.
There have been several reports that the workaround still does not
guarantee to work, and can add up to 100 ms (on the worst case)
to the boot process on certain platforms such as the MSI B350M MORTAR
with AMD Ryzen 3 2200G.
Therefore, revert this commit as a prelude to removing the pre-init
test.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Signed-off-by: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Note: I have revised the commit message to add more detail
and remove uncessary information.
drivers/iommu/amd/init.c | 45 ++--
1 file changed, 11 insertions(+), 34 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..648cdfd03074 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -12,7 +12,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -257,8 +256,6 @@ static enum iommu_init_state init_state = IOMMU_START_STATE;
static int amd_iommu_enable_interrupts(void);
static int __init iommu_go_to_state(enum iommu_init_state state);
static void init_device_table_dma(void);
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
- u8 fxn, u64 *value, bool is_write);
static bool amd_iommu_pre_enabled = true;
@@ -1717,11 +1714,13 @@ static int __init init_iommu_all(struct acpi_table_header *table)
return 0;
}
-static void __init init_iommu_perf_ctr(struct amd_iommu *iommu)
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+ u8 fxn, u64 *value, bool is_write);
+
+static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
- int retry;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg, save_src;
+ u64 val = 0xabcd, val2 = 0, save_reg = 0;
if (!iommu_feature(iommu, FEATURE_PC))
return;
@@ -1729,39 +1728,17 @@ static void __init init_iommu_perf_ctr(struct amd_iommu
*iommu)
amd_iommu_pc_present = true;
/* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, false) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 8, _src, false))
- goto pc_false;
-
- /*
-* Disable power gating by programing the performance counter
-* source to 20 (i.e. counts the reads and writes from/to IOMMU
-* Reserved Register [MMIO Offset 1FF8h] that are ignored.),
-* which never get incremented during this init phase.
-* (Note: The event is also deprecated.)
-*/
- val = 20;
- if (iommu_pc_get_set_reg(iommu, 0, 0, 8, , true))
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, false))
goto pc_false;
/* Check if the performance counters can be written to */
- val = 0xabcd;
- for (retry = 5; retry; retry--) {
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, , true) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 0, , false) ||
- val2)
- break;
-
- /* Wait about 20 msec for power gating to disable and retry. */
- msleep(20);
- }
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, true) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 8, _src, true))
+ if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, , true)) ||
+ (iommu_pc_get_set_reg(iommu, 0, 0, 0, , false)) ||
+ (val != val2))
Probably don't need parentheses around 'val != val2'
goto pc_false;
- if (val != val2)
+ /* restore */
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, true))
goto pc_false;
pci_info(pdev, "IOMMU performance counters supported\n");
thanks,
-- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
unnecessarily
Unfortunatly, there is no documentation of since which generation
Unfortunately,
Rephrase suggestion:
Unfortunately, it is unclear when the PMC HW bug fixed.
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
Based
that the buggy platforms are less likely to be in used, and it should
in use
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/amd/init.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 648cdfd03074..247cdda5d683 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct
acpi_table_header *table)
return 0;
}
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
- u8 fxn, u64 *value, bool is_write);
-
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
+ u64 val;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg = 0;
if (!iommu_feature(iommu, FEATURE_PC))
return;
amd_iommu_pc_present = true;
- /* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, false))
- goto pc_false;
-
- /* Check if the performance counters can be written to */
- if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, , true)) ||
- (iommu_pc_get_set_reg(iommu, 0, 0, 0, , false)) ||
- (val != val2))
- goto pc_false;
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, true))
- goto pc_false;
-
pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
@@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
iommu->max_counters = (u8) ((val >> 7) & 0xf);
return;
-
-pc_false:
- pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
- amd_iommu_pc_present = false;
- return;
}
static ssize_t amd_iommu_show_cap(struct device *dev,
I will test this patch and the revert on my two AMD systems and update
you in a day or two.
thanks,
-- Shuah
Re: [PATCH] media: em28xx: Fix race condition between open and init function
On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
Fixes a race condition - for lack of a more precise term - between
em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
media_pad and vdev structs from the em28xx_v4l2, and managing the
lifetime of those objects more dynamicaly.
The race happens when a thread[1] - containing the em28xx_v4l2_init()
code - calls the v4l2_mc_create_media_graph(), and it return a error,
if a thread[2] - running v4l2_open() - pass the verification point
and reaches the em28xx_v4l2_open() before the thread[1] finishes
the v4l2 subsystem deregistration, thread[1] will free all resources
before the em28xx_v4l2_open() can process their things,
because the em28xx_v4l2_init() has the dev->lock. And all this lead
the thread[2] to cause a user-after-free.
Have you tried this patch with em28xx device? You will have to take into
account the dependencies between the subdevs using the v4l2_dev.
Also try rmmod invidual drivers - what happens if you were to rmmod a
subdev driver? With v4l2_dev is not embedded in v4l2, this could open
up memory leaks or user-after-frees.
Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com
Signed-off-by: Igor Matheus Andrade Torrente
---
drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
drivers/media/usb/em28xx/em28xx-video.c | 188 ++-
drivers/media/usb/em28xx/em28xx.h| 6 +-
3 files changed, 123 insertions(+), 75 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
b/drivers/media/usb/em28xx/em28xx-camera.c
index d1e66b503f4d..436c5a8cbbb6 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_xtal = 430;
pdata.xtal = v4l2->sensor_xtal;
if (NULL ==
- v4l2_i2c_new_subdev_board(>v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
_info, NULL))
return -ENODEV;
v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
@@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_yres = 480;
subdev =
-v4l2_i2c_new_subdev_board(>v4l2_dev, adap,
+v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
_info, NULL);
if (!subdev)
return -ENODEV;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c
b/drivers/media/usb/em28xx/em28xx-video.c
index 6b84c3413e83..e1febb2bf06b 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
*/
static void em28xx_wake_i2c(struct em28xx *dev)
{
- struct v4l2_device *v4l2_dev = >v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
@@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int ret, i;
+ v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
+ if (!v4l2->video_pad) {
+ dev_err(>intf->dev,
+ "failed to allocate video pad memory!\n");
+ v4l2->vdev->entity.num_pads = 0;
+ return;
+ }
+
/* Initialize Video, VBI and Radio pads */
- v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
- ret = media_entity_pads_init(>vdev.entity, 1, >video_pad);
+ v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(>vdev->entity, 1, v4l2->video_pad);
if (ret < 0)
dev_err(>intf->dev,
"failed to initialize video media entity!\n");
@@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq,
unsigned int count)
f.type = V4L2_TUNER_RADIO;
else
f.type = V4L2_TUNER_ANALOG_TV;
- v4l2_device_call_all(>v4l2_dev,
+ v4l2_device_call_all(v4l2->v4l2_dev,
0, tuner, s_frequency, );
/* Enable video stream at TV decoder */
- v4l2_device_call_all(>v4l2_dev, 0, video, s_stream, 1);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
}
v4l2->streaming_users++;
@@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(>v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
Re: [PATCH] usbip: vhci_hcd: do proper error handling
On 3/31/21 5:23 AM, Muhammad Usama Anjum wrote: On Fri, 2021-03-26 at 14:24 -0600, Shuah Khan wrote: On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote: The driver was assuming that all the parameters would be valid. But it is possible that parameters are sent from userspace. For those cases, appropriate error checks have been added. Are you sure this change is necessary for vhci_hcd? Is there a scenario where vhci_hcd will receive these values from userspace? I'm not sure if these changes are necessary for vhci_hcd. I'd sent this patch following the motivation of a patch (c318840fb2) from dummy_hcd to handle some cases. Yeah, there is scenario where vhci_hcd will receive these values from userspace. For example, typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION can be received from userspace. As USB_PORT_FEAT_C_CONNECTION case isn't being handled, default case will is executed for it. So I'm assuming USB_PORT_FEAT_C_CONNECTION isn't supported and default case shouldn't be executed. The way dummy_hcd handles USB_PORT_FEAT_C_CONNECTION isn't applicable to vhci_hcd. rh_port_connect() and rh_port_disconnect() set the USB_PORT_FEAT_C_CONNECTION this flag to initiate port status polling. This flag is set by the driver as a result of attach/deatch request from the user-space. Current handling of this flag is correct. Is there a way to reproduce the problem? I'm not able to reproduce any problem. But typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION may trigger some behavior which isn't intended as USB_PORT_FEAT_C_CONNECTION may not be supported for vhci_hcd. If you reproduce the problem and it impacts attach/detach and device remote device access, we can to look into this further. thanks, -- Shuah
Re: [PATCH v2] selftests/resctrl: Change a few printed messages
On 4/7/21 1:57 PM, Fenghua Yu wrote:
Change a few printed messages to report test progress more clearly.
Add a missing "\n" at the end of one printed message.
Suggested-by: Shuah Khan
Signed-off-by: Fenghua Yu
---
Change log:
v2:
- Add "Pass:" and "Fail:" sub-strings back (Shuah).
This is a follow-up patch of recent resctrl selftest patches and can be
applied cleanly to:
git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
branch next.
tools/testing/selftests/resctrl/cache.c | 2 +-
tools/testing/selftests/resctrl/mba_test.c | 6 +++---
tools/testing/selftests/resctrl/mbm_test.c | 2 +-
tools/testing/selftests/resctrl/resctrlfs.c | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c
b/tools/testing/selftests/resctrl/cache.c
index 362e3a418caa..68ff856d36f0 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -301,7 +301,7 @@ int show_cache_info(unsigned long sum_llc_val, int
no_of_bits,
ret = platform && abs((int)diff_percent) > max_diff_percent &&
(cmt ? (abs(avg_diff) > max_diff) : true);
- ksft_print_msg("%s cache miss rate within %d%%\n",
+ ksft_print_msg("%s Check cache miss rate within %d%%\n",
ret ? "Fail:" : "Pass:", max_diff_percent);
ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
diff --git a/tools/testing/selftests/resctrl/mba_test.c
b/tools/testing/selftests/resctrl/mba_test.c
index 26f12ad4c663..1a1bdb6180cf 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -80,7 +80,7 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
avg_diff_per = (int)(avg_diff * 100);
- ksft_print_msg("%s MBA: diff within %d%% for schemata %u\n",
+ ksft_print_msg("%s Check MBA diff within %d%% for schemata
%u\n",
avg_diff_per > MAX_DIFF_PERCENT ?
"Fail:" : "Pass:",
MAX_DIFF_PERCENT,
@@ -93,10 +93,10 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
failed = true;
}
- ksft_print_msg("%s schemata change using MBA\n",
+ ksft_print_msg("%s Check schemata change using MBA\n",
failed ? "Fail:" : "Pass:");
if (failed)
- ksft_print_msg("At least one test failed");
+ ksft_print_msg("At least one test failed\n");
}
static int check_results(void)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c
b/tools/testing/selftests/resctrl/mbm_test.c
index 02b1ed03f1e5..8392e5c55ed0 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -37,7 +37,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc,
int span)
avg_diff_per = (int)(avg_diff * 100);
ret = avg_diff_per > MAX_DIFF_PERCENT;
- ksft_print_msg("%s MBM: diff within %d%%\n",
+ ksft_print_msg("%s Check MBM diff within %d%%\n",
ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
ksft_print_msg("Span (MB): %d\n", span);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index ade5f2b8b843..5f5a166ade60 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,14 +570,14 @@ bool check_resctrlfs_support(void)
fclose(inf);
- ksft_print_msg("%s kernel supports resctrl filesystem\n",
+ ksft_print_msg("%s Check kernel supports resctrl filesystem\n",
ret ? "Pass:" : "Fail:");
if (!ret)
return ret;
dp = opendir(RESCTRL_PATH);
- ksft_print_msg("%s resctrl mountpoint \"%s\" exists\n",
+ ksft_print_msg("%s Check resctrl mountpoint \"%s\" exists\n",
dp ? "Pass:" : "Fail:", RESCTRL_PATH);
if (dp)
closedir(dp);
Thank you. Applied to linux-kseftest next branch for 5.13-rc1
thanks,
-- Shuah
Re: [PATCH] Documentation: kunit: add tips for using current->kunit_test
On 4/7/21 2:07 PM, Brendan Higgins wrote:
On Tue, Apr 6, 2021 at 3:51 PM Daniel Latypov wrote:
As of commit 359a376081d4 ("kunit: support failure from dynamic analysis
tools"), we can use current->kunit_test to find the current kunit test.
Mention this in tips.rst and give an example of how this can be used in
conjunction with `test->priv` to pass around state and specifically
implement something like mocking.
There's a lot more we could go into on that topic, but given that
example is already longer than every other "tip" on this page, we just
point to the API docs and leave filling in the blanks as an exercise to
the reader.
Also give an example of kunit_fail_current_test().
Signed-off-by: Daniel Latypov
Reviewed-by: Brendan Higgins
Thank you. Applied to linux-kseftest kunit branch for 5.13-rc1
thanks,
-- Shuah
Re: [PATCH] selftests/resctrl: Change a few printed messages
On 4/7/21 11:12 AM, Fenghua Yu wrote:
Hi, Shuah,
On Wed, Apr 07, 2021 at 08:33:23AM -0600, Shuah Khan wrote:
On 4/5/21 6:52 PM, Fenghua Yu wrote:
- ksft_print_msg("%s cache miss rate within %d%%\n",
- ret ? "Fail:" : "Pass:", max_diff_percent);
+ ksft_print_msg("Check cache miss rate within %d%%\n", max_diff_percent);
You need %s and pass in the ret ? "Fail:" : "Pass:" result for the
message to read correctly.
Should I keep the ":" after "Pass"/"Fail"?
Yes please.
I am seeing:
# Check kernel support for resctrl filesystem
It should say the following:
# Fail Check kernel support for resctrl filesystem
i.e. should the printed messages be like the following?
# Fail: Check kernel support for resctrl filesystem
or
# Pass: Check kernel support for resctrl filesystem
This looks good.
thanks,
-- Shuah
Re: [PATCH] selftests/resctrl: Change a few printed messages
On 4/5/21 6:52 PM, Fenghua Yu wrote:
A few printed messages contain pass/fail strings which should be shown
in test results. Remove the pass/fail strings in the messages to avoid
confusion.
Add "\n" at the end of one printed message.
Suggested-by: Shuah Khan
Signed-off-by: Fenghua Yu
---
This is a follow-up patch of recent resctrl selftest patches and can be
applied cleanly to:
git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
branch next.
tools/testing/selftests/resctrl/cache.c | 3 +--
tools/testing/selftests/resctrl/mba_test.c | 9 +++--
tools/testing/selftests/resctrl/mbm_test.c | 3 +--
tools/testing/selftests/resctrl/resctrlfs.c | 7 ++-
4 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c
b/tools/testing/selftests/resctrl/cache.c
index 362e3a418caa..310bbc997c60 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -301,8 +301,7 @@ int show_cache_info(unsigned long sum_llc_val, int
no_of_bits,
ret = platform && abs((int)diff_percent) > max_diff_percent &&
(cmt ? (abs(avg_diff) > max_diff) : true);
- ksft_print_msg("%s cache miss rate within %d%%\n",
- ret ? "Fail:" : "Pass:", max_diff_percent);
+ ksft_print_msg("Check cache miss rate within %d%%\n", max_diff_percent);
You need %s and pass in the ret ? "Fail:" : "Pass:" result for the
message to read correctly.
I am seeing:
# Check kernel support for resctrl filesystem
It should say the following:
# Fail Check kernel support for resctrl filesystem
Same for other such messages.
ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
ksft_print_msg("Number of bits: %d\n", no_of_bits);
diff --git a/tools/testing/selftests/resctrl/mba_test.c
b/tools/testing/selftests/resctrl/mba_test.c
index 26f12ad4c663..a909a745754f 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -80,9 +80,7 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
avg_diff_per = (int)(avg_diff * 100);
- ksft_print_msg("%s MBA: diff within %d%% for schemata %u\n",
- avg_diff_per > MAX_DIFF_PERCENT ?
- "Fail:" : "Pass:",
+ ksft_print_msg("Check MBA diff within %d%% for schemata %u\n",
MAX_DIFF_PERCENT,
ALLOCATION_MAX - ALLOCATION_STEP * allocation);
@@ -93,10 +91,9 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
failed = true;
}
- ksft_print_msg("%s schemata change using MBA\n",
- failed ? "Fail:" : "Pass:");
+ ksft_print_msg("Check schemata change using MBA\n");
if (failed)
- ksft_print_msg("At least one test failed");
+ ksft_print_msg("At least one test failed\n");
}
static int check_results(void)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c
b/tools/testing/selftests/resctrl/mbm_test.c
index 02b1ed03f1e5..e2e7ee4ec630 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -37,8 +37,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc,
int span)
avg_diff_per = (int)(avg_diff * 100);
ret = avg_diff_per > MAX_DIFF_PERCENT;
- ksft_print_msg("%s MBM: diff within %d%%\n",
- ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
+ ksft_print_msg("Check MBM diff within %d%%\n", MAX_DIFF_PERCENT);
Here
ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
ksft_print_msg("Span (MB): %d\n", span);
ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index ade5f2b8b843..91cb3c48a7da 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,15 +570,12 @@ bool check_resctrlfs_support(void)
fclose(inf);
- ksft_print_msg("%s kernel supports resctrl filesystem\n",
- ret ? "Pass:" : "Fail:");
-
+ ksft_print_msg("Check kernel support for resctrl filesystem\n");
Here
if (!ret)
return ret;
dp = opendir(RESCTRL_PATH);
- ksft_print_msg("%s resctrl mountpoint \"%s\" exists\n",
- dp ? "Pass:" : "Fail:", RESCTRL_PATH);
+ ksft_print_msg("Check resctrl mountpoint \"%s\"\n", RESCTRL_PATH);
Here
if (dp)
closedir(dp);
thanks,
-- Shuah
[PATCH] ath10k: Fix ath10k_wmi_tlv_op_pull_peer_stats_info() unlock without lock
ath10k_wmi_tlv_op_pull_peer_stats_info() could try to unlock RCU lock
winthout locking it first when peer reason doesn't match the valid
cases for this function.
Add a default case to return without unlocking.
Fixes: 09078368d516 ("ath10k: hold RCU lock when calling
ieee80211_find_sta_by_ifaddr()")
Reported-by: Pavel Machek
Signed-off-by: Shuah Khan
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index d97b33f789e4..7efbe03fbca8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -592,6 +592,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
GFP_ATOMIC
);
break;
+ default:
+ kfree(tb);
+ return;
}
exit:
--
2.27.0
Re: [PATCH] kunit: fix -Wunused-function warning for __kunit_fail_current_test
On 4/6/21 2:50 PM, Brendan Higgins wrote:
On Tue, Apr 6, 2021 at 10:29 AM Daniel Latypov wrote:
When CONFIG_KUNIT is not enabled, __kunit_fail_current_test() an empty
static function.
But GCC complains about unused static functions, *unless* they're static inline.
So add inline to make GCC happy.
Signed-off-by: Daniel Latypov
Fixes: 359a376081d4 ("kunit: support failure from dynamic analysis tools")
Signed-off-by comes after Fixes. Also good to add Reported-by for
Stephen acknowledging the reporter.
I will fix this up when I apply - for future reference.
Reviewed-by: Brendan Higgins
thanks,
-- Shuah
Re: [PATCH 4.4 00/28] 4.4.265-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.4.265 release. There are 28 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.265-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/35] 4.9.265-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.265 release. There are 35 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.265-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 00/74] 5.4.110-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.110 release. There are 74 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.110-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/56] 4.19.185-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.185 release. There are 56 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.185-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()
On 4/5/21 9:34 AM, Pavel Machek wrote:
Hi!
Fix ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU lock before it
calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.
It does that. But is also does the unlock even if it did not take the
lock:
There is only one path after it takes the lock.
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
case WMI_TDLS_TEARDOWN_REASON_TX:
case WMI_TDLS_TEARDOWN_REASON_RSSI:
case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
+ rcu_read_lock();
Takes the lock here:
station = ieee80211_find_sta_by_ifaddr(ar->hw,
ev->peer_macaddr.addr,
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer
event");
- kfree(tb);
- return;
+ goto exit;
Releases it if something fails
}
arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
ieee80211_tdls_oper_request(
@@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
);
break;
}
+
falls through here.
+exit:
+ rcu_read_unlock();
kfree(tb);
}
The switch only takes the lock in 3 branches, but it is released
unconditionally at the end.
I don't see any other switch cases. However, default is missing:
It could be done this way perhaps: (simpler than what you proposed)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index d97b33f789e4..7efbe03fbca8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -592,6 +592,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k
*ar, struct sk_buff *skb)
GFP_ATOMIC
);
break;
+ default:
+ kfree(tb);
+ return;
}
exit:
thanks,
-- Shuah
Re: [PATCH 5.10 000/126] 5.10.28-rc1 review
On 4/5/21 2:52 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.28 release. There are 126 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.28-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/152] 5.11.12-rc1 review
On 4/5/21 2:52 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.12 release. There are 152 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.12-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH] firmware_loader: Remove unnecessary conversion to bool
On 2/18/21 2:12 AM, Jiapeng Chong wrote:
Fix the following coccicheck warnings:
./tools/testing/selftests/firmware/fw_namespace.c:98:54-59: WARNING:
conversion to bool not needed here.
Reported-by: Abaci Robot
Signed-off-by: Jiapeng Chong
---
tools/testing/selftests/firmware/fw_namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/firmware/fw_namespace.c
b/tools/testing/selftests/firmware/fw_namespace.c
index 5ebc1ae..0e393cb 100644
--- a/tools/testing/selftests/firmware/fw_namespace.c
+++ b/tools/testing/selftests/firmware/fw_namespace.c
@@ -95,7 +95,7 @@ static bool test_fw_in_ns(const char *fw_name, const char
*sys_path, bool block_
}
if (block_fw_in_parent_ns)
umount("/lib/firmware");
- return WEXITSTATUS(status) == EXIT_SUCCESS ? true : false;
This looks right to me. test_fw_in_ns() returns true or false and
test_fw_in_ns() callers print appropriate message.
I don't think this patch is necessary.
thanks,
-- Shuah
Re: [PATCH v4 1/2] kunit: support failure from dynamic analysis tools
On 4/2/21 3:44 PM, Shuah Khan wrote:
On 4/2/21 3:25 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:53 AM Shuah Khan
wrote:
On 4/2/21 2:55 AM, Brendan Higgins wrote:
On Thu, Mar 11, 2021 at 7:23 AM Daniel Latypov
wrote:
From: Uriel Guajardo
Add a kunit_fail_current_test() function to fail the currently running
test, if any, with an error message.
This is largely intended for dynamic analysis tools like UBSAN and for
fakes.
E.g. say I had a fake ops struct for testing and I wanted my `free`
function to complain if it was called with an invalid argument, or
caught a double-free. Most return void and have no normal means of
signalling failure (e.g. super_operations, iommu_ops, etc.).
Key points:
* Always update current->kunit_test so anyone can use it.
* commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated
it for
CONFIG_KASAN=y
* Create a new header so non-test code doesn't have
to include all of (e.g. lib/ubsan.c)
* Forward the file and line number to make it easier to track down
failures
* Declare the helper function for nice __printf() warnings about
mismatched
format strings even when KUnit is not enabled.
Example output from kunit_fail_current_test("message"):
[15:19:34] [FAILED] example_simple_test
[15:19:34] # example_simple_test: initializing
[15:19:34] # example_simple_test:
lib/kunit/kunit-example-test.c:24: message
[15:19:34] not ok 1 - example_simple_test
Co-developed-by: Daniel Latypov
Signed-off-by: Daniel Latypov
Signed-off-by: Uriel Guajardo
Reviewed-by: Alan Maguire
Reviewed-by: Brendan Higgins
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Thanks for the catch.
checkpatch.pl --strict should now be happy (aside from complaining
about line wrapping)
v5 here:
https://lore.kernel.org/linux-kselftest/20210402212131.835276-1-dlaty...@google.com
Note: Brendan didn't give an explicit Reviewed-by on the second patch,
not sure if that was intentional.
No worries. I applied this one as well. I was able to fix it with just
checkpatch --fix option.
Clarification. Applied 1/2 - I will wait for Brendan's ack on 2/2
thanks,
-- Shuah
Re: [PATCH v4 1/2] kunit: support failure from dynamic analysis tools
On 4/2/21 3:25 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:53 AM Shuah Khan wrote:
On 4/2/21 2:55 AM, Brendan Higgins wrote:
On Thu, Mar 11, 2021 at 7:23 AM Daniel Latypov wrote:
From: Uriel Guajardo
Add a kunit_fail_current_test() function to fail the currently running
test, if any, with an error message.
This is largely intended for dynamic analysis tools like UBSAN and for
fakes.
E.g. say I had a fake ops struct for testing and I wanted my `free`
function to complain if it was called with an invalid argument, or
caught a double-free. Most return void and have no normal means of
signalling failure (e.g. super_operations, iommu_ops, etc.).
Key points:
* Always update current->kunit_test so anyone can use it.
* commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for
CONFIG_KASAN=y
* Create a new header so non-test code doesn't have
to include all of (e.g. lib/ubsan.c)
* Forward the file and line number to make it easier to track down
failures
* Declare the helper function for nice __printf() warnings about mismatched
format strings even when KUnit is not enabled.
Example output from kunit_fail_current_test("message"):
[15:19:34] [FAILED] example_simple_test
[15:19:34] # example_simple_test: initializing
[15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message
[15:19:34] not ok 1 - example_simple_test
Co-developed-by: Daniel Latypov
Signed-off-by: Daniel Latypov
Signed-off-by: Uriel Guajardo
Reviewed-by: Alan Maguire
Reviewed-by: Brendan Higgins
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Thanks for the catch.
checkpatch.pl --strict should now be happy (aside from complaining
about line wrapping)
v5 here:
https://lore.kernel.org/linux-kselftest/20210402212131.835276-1-dlaty...@google.com
Note: Brendan didn't give an explicit Reviewed-by on the second patch,
not sure if that was intentional.
No worries. I applied this one as well. I was able to fix it with just
checkpatch --fix option.
All set now.
thanks,
-- Shuah
Re: [PATCH v6 00/21] Miscellaneous fixes for resctrl selftests
On 4/2/21 12:18 PM, Fenghua Yu wrote: Hi, Shuah, On Fri, Apr 02, 2021 at 12:17:17PM -0600, Shuah Khan wrote: On 3/26/21 1:45 PM, Fenghua Yu wrote: Hi, Shuah, On Wed, Mar 17, 2021 at 02:22:34AM +, Fenghua Yu wrote: This patch set has several miscellaneous fixes to resctrl selftest tool that are easily visible to user. V1 had fixes to CAT test and CMT test but they were dropped in V2 because having them here made the patchset humongous. So, changes to CAT test and CMT test will be posted in another patchset. Change Log: v6: - Add Tested-by: Babu Moger . - Replace "cat" by CAT_STR etc (Babu). - Capitalize the first letter of printed message (Babu). Any comment on this series? Will you push it into linux-kselftest.git? Yes. Will apply for 5.13-rc1 Great! Thank you very much for your help! Done. Now applied to linux-selftest next. https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/ next Ran sanity test and suggested a change in message for 12/21. Please take a look other such messages and improve them as well and send follow-on patches. thanks, -- Shuah
Re: [PATCH v6 12/21] selftests/resctrl: Check for resctrl mount point only if resctrl FS is supported
On 3/16/21 8:22 PM, Fenghua Yu wrote:
check_resctrlfs_support() does the following
1. Checks if the platform supports resctrl file system or not by looking
for resctrl in /proc/filesystems
2. Calls opendir() on default resctrl file system path
(i.e. /sys/fs/resctrl)
3. Checks if resctrl file system is mounted or not by looking at
/proc/mounts
Steps 2 and 3 will fail if the platform does not support resctrl file
system. So, there is no need to check for them if step 1 fails.
Fix this by returning immediately if the platform does not support
resctrl file system.
Tested-by: Babu Moger
Signed-off-by: Fenghua Yu
---
tools/testing/selftests/resctrl/resctrlfs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index 6b22a186790a..87195eb78356 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,6 +570,9 @@ bool check_resctrlfs_support(void)
ksft_print_msg("%s kernel supports resctrl filesystem\n",
ret ? "Pass:" : "Fail:");
This message is a bit confusing. Please change this to read
and send a follow-on patch on top of linux-kselftest next
"Check kernel support for resctrl filesystem"
thanks,
-- Shuah
Re: [PATCH] kunit: tool: make --kunitconfig accept dirs, add lib/kunit fragment
On 4/2/21 1:27 PM, Daniel Latypov wrote: On Fri, Apr 2, 2021 at 11:00 AM Shuah Khan wrote: On 4/2/21 3:32 AM, Brendan Higgins wrote: TL;DR $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit Per suggestion from Ted [1], we can reduce the amount of typing by assuming a convention that these files are named '.kunitconfig'. In the case of [1], we now have $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4 Also add in such a fragment for kunit itself so we can give that as an example more close to home (and thus less likely to be accidentally broken). [1] https://lore.kernel.org/linux-ext4/ycnf4yp1db97z...@mit.edu/ Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Should this be captured in documentation. Especially since this is file is .* file. Do you want to include doc in this patch? Might be better that way. It definitely should be documented, yes. The only real example hadn't landed yet when I sent this patch (fs/ext4/.kunitconfig was going in through the ext4 tree), but now it's in linus/master. There's still some uncertainties about what best practices for this feature should be, i.e. * how granular should these be? * how should configs in parent dirs be handled? Should they be supersets of all the subdirs? * E.g. should fs/.kunitconfig be a superset of fs/ext4/.kunitconfig and any other hypothetical subdir configs? * Should we wait on saying "you should do this" until we have "import" statements/other mechanisms to make this less manual? * how should we handle non-UML tests, like the KASAN tests? * ideally, kunit.py run will eventually support running tests on x86 (using qemu) If it's fine with you, I was hoping to come back and add a section to kunit/start.rst when we've had some of those questions more figured out. Sound good. I will apply this patch and you can document later. thanks, -- Shuah
Re: [PATCH] kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals
On 4/2/21 1:09 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:47 AM Shuah Khan wrote:
On 4/2/21 3:35 AM, Brendan Higgins wrote:
On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov wrote:
Before:
Expected str == "world", but
str == hello
"world" == world
After:
Expected str == "world", but
str == "hello"
Note: like the literal ellision for integers, this doesn't handle the
case of
KUNIT_EXPECT_STREQ(test, "hello", "world")
since we don't expect it to realistically happen in checked in tests.
(If you really wanted a test to fail, KUNIT_FAIL("msg") exists)
In that case, you'd get:
Expected "hello" == "world", but
Signed-off-by: Daniel Latypov
Reviewed-by: Brendan Higgins
Hi Daniel,
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Are there some flags you'd like me to pass to checkpatch?
$ ./scripts/checkpatch.pl --git HEAD
total: 0 errors, 0 warnings, 42 lines checked
My commit script uses --strict which shows readability errors.
Commit f66884e8b831 ("kunit: make KUNIT_EXPECT_STREQ() quote values,
don't print literals") has no obvious style problems and is ready for
submission.
I just rebased onto linus/master again since I know checkpatch.pl's
default behavior had changed recently, but I didn't see any errors
there.
I know this commit made some lines go just over 80 characters, so
$ ./scripts/checkpatch.pl --max-line-length=80 --git HEAD
...
total: 0 errors, 4 warnings, 42 lines checked
Don't worry about line wrap warns. I just ignore them. :)
thanks,
-- Shuah
Re: [PATCH v6 00/21] Miscellaneous fixes for resctrl selftests
On 3/26/21 1:45 PM, Fenghua Yu wrote: Hi, Shuah, On Wed, Mar 17, 2021 at 02:22:34AM +, Fenghua Yu wrote: This patch set has several miscellaneous fixes to resctrl selftest tool that are easily visible to user. V1 had fixes to CAT test and CMT test but they were dropped in V2 because having them here made the patchset humongous. So, changes to CAT test and CMT test will be posted in another patchset. Change Log: v6: - Add Tested-by: Babu Moger . - Replace "cat" by CAT_STR etc (Babu). - Capitalize the first letter of printed message (Babu). Any comment on this series? Will you push it into linux-kselftest.git? Yes. Will apply for 5.13-rc1 thanks, -- Shuah
Re: [PATCH v5 00/21] Miscellaneous fixes for resctrl selftests
On 3/12/21 3:11 PM, Fenghua Yu wrote: Hi, Babu, On Fri, Mar 12, 2021 at 01:08:11PM -0600, Babu Moger wrote: Hi Fenghua, Thanks for the patches. Sanity tested them on AMD systems. Appears to work fine. Few minor comments in few patches. Tested-by: Babu Moger I will add Tested-by: Babu Moger in the series and address your comments. Thank you for your review! Looks like a few patches in this series needs updates. Do you plan to send the new revision for consideration for 5.13? thanks, -- Shuah
Re: [PATCH] kunit: tool: make --kunitconfig accept dirs, add lib/kunit fragment
On 4/2/21 3:32 AM, Brendan Higgins wrote: TL;DR $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit Per suggestion from Ted [1], we can reduce the amount of typing by assuming a convention that these files are named '.kunitconfig'. In the case of [1], we now have $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4 Also add in such a fragment for kunit itself so we can give that as an example more close to home (and thus less likely to be accidentally broken). [1] https://lore.kernel.org/linux-ext4/ycnf4yp1db97z...@mit.edu/ Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Should this be captured in documentation. Especially since this is file is .* file. Do you want to include doc in this patch? Might be better that way. thanks, -- Shuah
Re: [PATCH v4 1/2] kunit: support failure from dynamic analysis tools
On 4/2/21 2:55 AM, Brendan Higgins wrote:
On Thu, Mar 11, 2021 at 7:23 AM Daniel Latypov wrote:
From: Uriel Guajardo
Add a kunit_fail_current_test() function to fail the currently running
test, if any, with an error message.
This is largely intended for dynamic analysis tools like UBSAN and for
fakes.
E.g. say I had a fake ops struct for testing and I wanted my `free`
function to complain if it was called with an invalid argument, or
caught a double-free. Most return void and have no normal means of
signalling failure (e.g. super_operations, iommu_ops, etc.).
Key points:
* Always update current->kunit_test so anyone can use it.
* commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for
CONFIG_KASAN=y
* Create a new header so non-test code doesn't have
to include all of (e.g. lib/ubsan.c)
* Forward the file and line number to make it easier to track down
failures
* Declare the helper function for nice __printf() warnings about mismatched
format strings even when KUnit is not enabled.
Example output from kunit_fail_current_test("message"):
[15:19:34] [FAILED] example_simple_test
[15:19:34] # example_simple_test: initializing
[15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message
[15:19:34] not ok 1 - example_simple_test
Co-developed-by: Daniel Latypov
Signed-off-by: Daniel Latypov
Signed-off-by: Uriel Guajardo
Reviewed-by: Alan Maguire
Reviewed-by: Brendan Higgins
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
thanks,
-- Shuah
Re: [PATCH] kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals
On 4/2/21 3:35 AM, Brendan Higgins wrote:
On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov wrote:
Before:
Expected str == "world", but
str == hello
"world" == world
After:
Expected str == "world", but
str == "hello"
Note: like the literal ellision for integers, this doesn't handle the
case of
KUNIT_EXPECT_STREQ(test, "hello", "world")
since we don't expect it to realistically happen in checked in tests.
(If you really wanted a test to fail, KUNIT_FAIL("msg") exists)
In that case, you'd get:
Expected "hello" == "world", but
Signed-off-by: Daniel Latypov
Reviewed-by: Brendan Higgins
Hi Daniel,
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
thanks,
-- Shuah
[PATCH 0/4] usbip synchronize sysfs code paths
Fuzzing uncovered race condition between sysfs code paths in usbip drivers. Device connect/disconnect code paths initiated through sysfs interface are prone to races if disconnect happens during connect and vice versa. This problem is common to all drivers while it can be reproduced easily in vhci_hcd. Add a sysfs_lock to usbip_device struct to protect the paths. For a complete fix, all usbip drivers have to use sysfs_lock to protect sysfs code paths and common event handler will have to use this lock to synchonize with the sysfs paths in drivers. This patch series adds sysfs_lock and uses it in vhci_hcd in the first patch. Subsequent patches fix usbip_host, vudc and the last patch fixes the common event handler code path. Shuah Khan (4): usbip: add sysfs_lock to synchronize sysfs code paths usbip: stub-dev synchronize sysfs code paths usbip: vudc synchronize sysfs code paths usbip: synchronize event handler with sysfs code paths drivers/usb/usbip/stub_dev.c | 11 +-- drivers/usb/usbip/usbip_common.h | 3 +++ drivers/usb/usbip/usbip_event.c | 2 ++ drivers/usb/usbip/vhci_hcd.c | 1 + drivers/usb/usbip/vhci_sysfs.c | 30 +- drivers/usb/usbip/vudc_dev.c | 1 + drivers/usb/usbip/vudc_sysfs.c | 5 + 7 files changed, 46 insertions(+), 7 deletions(-) -- 2.27.0
[PATCH 1/4] usbip: add sysfs_lock to synchronize sysfs code paths
Fuzzing uncovered race condition between sysfs code paths in usbip
drivers. Device connect/disconnect code paths initiated through
sysfs interface are prone to races if disconnect happens during
connect and vice versa.
This problem is common to all drivers while it can be reproduced easily
in vhci_hcd. Add a sysfs_lock to usbip_device struct to protect the paths.
Use this in vhci_hcd to protect sysfs paths. For a complete fix, usip_host
and usip-vudc drivers and the event handler will have to use this lock to
protect the paths. These changes will be done in subsequent patches.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/usbip_common.h | 3 +++
drivers/usb/usbip/vhci_hcd.c | 1 +
drivers/usb/usbip/vhci_sysfs.c | 30 +-
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index d60ce17d3dd2..ea2a20e6d27d 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -263,6 +263,9 @@ struct usbip_device {
/* lock for status */
spinlock_t lock;
+ /* mutex for synchronizing sysfs store paths */
+ struct mutex sysfs_lock;
+
int sockfd;
struct socket *tcp_socket;
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index a20a8380ca0c..4ba6bcdaa8e9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1101,6 +1101,7 @@ static void vhci_device_init(struct vhci_device *vdev)
vdev->ud.side = USBIP_VHCI;
vdev->ud.status = VDEV_ST_NULL;
spin_lock_init(>ud.lock);
+ mutex_init(>ud.sysfs_lock);
INIT_LIST_HEAD(>priv_rx);
INIT_LIST_HEAD(>priv_tx);
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4b4256e5dad..e2847cd3e6e3 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -185,6 +185,8 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd,
__u32 rhport)
usbip_dbg_vhci_sysfs("enter\n");
+ mutex_lock(>ud.sysfs_lock);
+
/* lock */
spin_lock_irqsave(>lock, flags);
spin_lock(>ud.lock);
@@ -195,6 +197,7 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd,
__u32 rhport)
/* unlock */
spin_unlock(>ud.lock);
spin_unlock_irqrestore(>lock, flags);
+ mutex_unlock(>ud.sysfs_lock);
return -EINVAL;
}
@@ -205,6 +208,8 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd,
__u32 rhport)
usbip_event_add(>ud, VDEV_EVENT_DOWN);
+ mutex_unlock(>ud.sysfs_lock);
+
return 0;
}
@@ -349,30 +354,36 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
else
vdev = >vhci_hcd_hs->vdev[rhport];
+ mutex_lock(>ud.sysfs_lock);
+
/* Extract socket from fd. */
socket = sockfd_lookup(sockfd, );
if (!socket) {
dev_err(dev, "failed to lookup sock");
- return -EINVAL;
+ err = -EINVAL;
+ goto unlock_mutex;
}
if (socket->type != SOCK_STREAM) {
dev_err(dev, "Expecting SOCK_STREAM - found %d",
socket->type);
sockfd_put(socket);
- return -EINVAL;
+ err = -EINVAL;
+ goto unlock_mutex;
}
/* create threads before locking */
tcp_rx = kthread_create(vhci_rx_loop, >ud, "vhci_rx");
if (IS_ERR(tcp_rx)) {
sockfd_put(socket);
- return -EINVAL;
+ err = -EINVAL;
+ goto unlock_mutex;
}
tcp_tx = kthread_create(vhci_tx_loop, >ud, "vhci_tx");
if (IS_ERR(tcp_tx)) {
kthread_stop(tcp_rx);
sockfd_put(socket);
- return -EINVAL;
+ err = -EINVAL;
+ goto unlock_mutex;
}
/* get task structs now */
@@ -397,7 +408,8 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
* Will be retried from userspace
* if there's another free port.
*/
- return -EBUSY;
+ err = -EBUSY;
+ goto unlock_mutex;
}
dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
@@ -423,7 +435,15 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
rh_port_connect(vdev, speed);
+ dev_info(dev, "Device attached\n");
+
+ mutex_unlock(>ud.sysfs_lock);
+
return count;
+
+unlock_mutex:
+ mutex_unlock(>ud.sysfs_lock);
+ return err;
}
static DEVICE_ATTR_WO(attach);
--
2.27.0
[PATCH 3/4] usbip: vudc synchronize sysfs code paths
Fuzzing uncovered race condition between sysfs code paths in usbip
drivers. Device connect/disconnect code paths initiated through
sysfs interface are prone to races if disconnect happens during
connect and vice versa.
Use sysfs_lock to protect sysfs paths in vudc.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/vudc_dev.c | 1 +
drivers/usb/usbip/vudc_sysfs.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index c8eeabdd9b56..2bc428f2e261 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -572,6 +572,7 @@ static int init_vudc_hw(struct vudc *udc)
init_waitqueue_head(>tx_waitq);
spin_lock_init(>lock);
+ mutex_init(>sysfs_lock);
ud->status = SDEV_ST_AVAILABLE;
ud->side = USBIP_VUDC;
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index 7383a543c6d1..f7633ee655a1 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -112,6 +112,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
dev_err(dev, "no device");
return -ENODEV;
}
+ mutex_lock(>ud.sysfs_lock);
spin_lock_irqsave(>lock, flags);
/* Don't export what we don't have */
if (!udc->driver || !udc->pullup) {
@@ -187,6 +188,8 @@ static ssize_t usbip_sockfd_store(struct device *dev,
wake_up_process(udc->ud.tcp_rx);
wake_up_process(udc->ud.tcp_tx);
+
+ mutex_unlock(>ud.sysfs_lock);
return count;
} else {
@@ -207,6 +210,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
}
spin_unlock_irqrestore(>lock, flags);
+ mutex_unlock(>ud.sysfs_lock);
return count;
@@ -216,6 +220,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
spin_unlock_irq(>ud.lock);
unlock:
spin_unlock_irqrestore(>lock, flags);
+ mutex_unlock(>ud.sysfs_lock);
return ret;
}
--
2.27.0
[PATCH 2/4] usbip: stub-dev synchronize sysfs code paths
Fuzzing uncovered race condition between sysfs code paths in usbip
drivers. Device connect/disconnect code paths initiated through
sysfs interface are prone to races if disconnect happens during
connect and vice versa.
Use sysfs_lock to protect sysfs paths in stub-dev.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/stub_dev.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 8f1de1fbbeed..d8d3892e5a69 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -63,6 +63,7 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct
device_attribute *a
dev_info(dev, "stub up\n");
+ mutex_lock(>ud.sysfs_lock);
spin_lock_irq(>ud.lock);
if (sdev->ud.status != SDEV_ST_AVAILABLE) {
@@ -87,13 +88,13 @@ static ssize_t usbip_sockfd_store(struct device *dev,
struct device_attribute *a
tcp_rx = kthread_create(stub_rx_loop, >ud, "stub_rx");
if (IS_ERR(tcp_rx)) {
sockfd_put(socket);
- return -EINVAL;
+ goto unlock_mutex;
}
tcp_tx = kthread_create(stub_tx_loop, >ud, "stub_tx");
if (IS_ERR(tcp_tx)) {
kthread_stop(tcp_rx);
sockfd_put(socket);
- return -EINVAL;
+ goto unlock_mutex;
}
/* get task structs now */
@@ -112,6 +113,8 @@ static ssize_t usbip_sockfd_store(struct device *dev,
struct device_attribute *a
wake_up_process(sdev->ud.tcp_rx);
wake_up_process(sdev->ud.tcp_tx);
+ mutex_unlock(>ud.sysfs_lock);
+
} else {
dev_info(dev, "stub down\n");
@@ -122,6 +125,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
struct device_attribute *a
spin_unlock_irq(>ud.lock);
usbip_event_add(>ud, SDEV_EVENT_DOWN);
+ mutex_unlock(>ud.sysfs_lock);
}
return count;
@@ -130,6 +134,8 @@ static ssize_t usbip_sockfd_store(struct device *dev,
struct device_attribute *a
sockfd_put(socket);
err:
spin_unlock_irq(>ud.lock);
+unlock_mutex:
+ mutex_unlock(>ud.sysfs_lock);
return -EINVAL;
}
static DEVICE_ATTR_WO(usbip_sockfd);
@@ -270,6 +276,7 @@ static struct stub_device *stub_device_alloc(struct
usb_device *udev)
sdev->ud.side = USBIP_STUB;
sdev->ud.status = SDEV_ST_AVAILABLE;
spin_lock_init(>ud.lock);
+ mutex_init(>ud.sysfs_lock);
sdev->ud.tcp_socket = NULL;
sdev->ud.sockfd = -1;
--
2.27.0
[PATCH 4/4] usbip: synchronize event handler with sysfs code paths
Fuzzing uncovered race condition between sysfs code paths in usbip
drivers. Device connect/disconnect code paths initiated through
sysfs interface are prone to races if disconnect happens during
connect and vice versa.
Use sysfs_lock to synchronize event handler with sysfs paths
in usbip drivers.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/usbip_event.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 5d88917c9631..086ca76dd053 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -70,6 +70,7 @@ static void event_handler(struct work_struct *work)
while ((ud = get_event()) != NULL) {
usbip_dbg_eh("pending event %lx\n", ud->event);
+ mutex_lock(>sysfs_lock);
/*
* NOTE: shutdown must come first.
* Shutdown the device.
@@ -90,6 +91,7 @@ static void event_handler(struct work_struct *work)
ud->eh_ops.unusable(ud);
unset_event(ud, USBIP_EH_UNUSABLE);
}
+ mutex_unlock(>sysfs_lock);
wake_up(>eh_waitq);
}
--
2.27.0
Re: [PATCH 4.4 00/33] 4.4.264-rc1 review
On 3/29/21 1:57 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.4.264 release. There are 33 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 07:55:56 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.264-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/53] 4.9.264-rc1 review
On 3/29/21 1:57 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.264 release. There are 53 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 07:55:56 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.264-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/72] 4.19.184-rc1 review
On 3/29/21 1:57 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.184 release. There are 72 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 07:55:56 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.184-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 000/111] 5.4.109-rc1 review
On 3/29/21 1:57 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.109 release. There are 111 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 07:55:56 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.109-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 000/219] 5.10.27-rc2 review
On 3/29/21 4:14 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.27 release. There are 219 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 10:13:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.27-rc2.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/252] 5.11.11-rc2 review
On 3/29/21 4:14 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.11 release. There are 252 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 31 Mar 2021 10:13:07 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.11-rc2.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH] sched/psi.c: Rudimentary typo fixes
On 3/26/21 1:42 PM, Bhaskar Chowdhury wrote: On 19:41 Fri 26 Mar 2021, Peter Zijlstra wrote: On Fri, Mar 26, 2021 at 02:00:18PM -0400, Johannes Weiner wrote: On Fri, Mar 26, 2021 at 06:12:33PM +0530, Bhaskar Chowdhury wrote: > > s/possible/possible/ > s/ exceution/execution/ > s/manupulations/manipulations/ > > Signed-off-by: Bhaskar Chowdhury > --- > kernel/sched/psi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index 967732c0766c..316ebc57a115 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -59,7 +59,7 @@ > * states, we would have to conclude a CPU SOME pressure number of > * 100%, since *somebody* is waiting on a runqueue at all > * times. However, that is clearly not the amount of contention the > - * workload is experiencing: only one out of 256 possible exceution > + * workload is experiencing: only one out of 256 possible execution I thought this was the french spelling. Joking aside, the corrections look right, but I also had no trouble understanding what those sentences mean. Typos happen, plus we have a lot of non-native speakers who won't use perfect spelling or grammar. So for me, the bar is "this can be easily understood", not "needs to be perfect English", and I'd say let's skip patches like these unless they fix something truly unintelligble. Ignore this robot, lots of people already have a special mail rule for him. On top of that, this spelling mistake was already fixed by Ingo in: Dude, this is a human being and you are suggesting a fucking stupid solution to others. I know what Ingo did and very appreciable. Please try to be cooperative and help. Stop spreading FUD ...for fuck's sake ...man.. I am not doing it fun...you need to understand that . ... ..and I have some special rule for some people to ...who the fuck care ...ahh Bhaskar Chowdhury, Please be advised that we have received a CoC complaint about your responses to the following patch discussions: - https://lore.kernel.org/lkml/YGFrvwX8QngvwPbA@Gentoo/ - https://lore.kernel.org/lkml/YF45Qi+%2FeB+%2Fm7y%2F@Gentoo/ - https://lore.kernel.org/lkml/YF44jiYiAVkuwE0P@Gentoo/ - https://lore.kernel.org/r/YGHOxwiqwhGAs819@Gentoo There is no requirement that anyone accept your patches. Accepting feedback and working with the community will ensure that your work will be taken seriously now and in the future. Your responses go against the CoC. If this continues, we will take action to add you to the kernel wide blacklist. Please refer to: https://www.kernel.org/doc/html/latest/process/code-of-conduct.html thanks, -- Shuah (on behalf of the Code of Conduct Committee)
Re: [PATCH] usbip: vhci_hcd: do proper error handling
On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote: The driver was assuming that all the parameters would be valid. But it is possible that parameters are sent from userspace. For those cases, appropriate error checks have been added. Are you sure this change is necessary for vhci_hcd? Is there a scenario where vhci_hcd will receive these values from userspace? Is there a way to reproduce the problem? thanks, -- Shuah
Re: [PATCH] selftests/timers: remove unneeded semicolon
On 3/9/21 12:49 AM, Jiapeng Chong wrote: Fix the following coccicheck warnings: ./tools/testing/selftests/timers/nanosleep.c:75:2-3: Unneeded semicolon Reported-by: Abaci Robot Signed-off-by: Jiapeng Chong --- tools/testing/selftests/timers/nanosleep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/timers/nanosleep.c b/tools/testing/selftests/timers/nanosleep.c index 71b5441..433a096 100644 --- a/tools/testing/selftests/timers/nanosleep.c +++ b/tools/testing/selftests/timers/nanosleep.c @@ -72,7 +72,7 @@ char *clockstring(int clockid) return "CLOCK_BOOTTIME_ALARM"; case CLOCK_TAI: return "CLOCK_TAI"; - }; + } return "UNKNOWN_CLOCKID"; } Can you send one single patch for all these timers fixes. All of these patches have the same subject line and it is becoming hard to tell them apart. thanks, -- Shuah
Re: [PATCH] selftests/timers: Fix spelling mistake "clocksourc" -> "clocksource"
On 3/15/21 12:41 PM, John Stultz wrote: On Mon, Mar 15, 2021 at 5:33 AM Colin King wrote: From: Colin Ian King There is a spelling mistake in a comment. Fix it. Signed-off-by: Colin Ian King Akcde-yb: John Stultz I kid, I kid! My apologies and thanks! Acked-by: John Stultz Thank you both. Applied now for 5.13-rc1 thanks, -- Shuah
Re: [PATCH v5 2/2] usbip: tools: add usage of device mode in usbip_list.c
On 3/24/21 1:56 AM, Hongren Zheng (Zenithal) wrote:
The option '-d/--device' was implemented in 'usbip list' but not
shown in usage. Hence this commit adds this option to usage.
Signed-off-by: Hongren Zheng
---
tools/usb/usbip/src/usbip_list.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
PATCH v2:
Add signed-off-by line
PATCH v3:
Move patch changelog after the marker line
Remove nickname in signed-off-by line
PATCH v4:
Use commit short hash and message instead of long hash only when
referring to commit in the kernel
PATCH v5:
Add documentation of `usbip port` and its usage in examples
Add flow of detaching in examples
Rephrase some description and add punctuations
Fix typo of `usbip attach --ev-id` to `--dev-id`
diff --git a/tools/usb/usbip/src/usbip_list.c b/tools/usb/usbip/src/usbip_list.c
index 8625b0f514ee..3d810bcca02f 100644
--- a/tools/usb/usbip/src/usbip_list.c
+++ b/tools/usb/usbip/src/usbip_list.c
@@ -33,7 +33,8 @@ static const char usbip_list_usage_string[] =
"usbip list [-p|--parsable] \n"
"-p, --parsable Parsable list format\n"
"-r, --remote=List the exportable USB devices on \n"
- "-l, --localList the local USB devices\n";
+ "-l, --localList the local USB devices\n"
+ "-d, --device List the local USB gadgets bound to
usbip-vudc\n";
void usbip_list_usage(void)
{
Thank you. Looks good.
Acked-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH v5 1/2] usbip: tools: add options and examples in man page related to device mode
On 3/24/21 12:35 AM, Hongren Zheng (Zenithal) wrote:
The commit e0546fd8b748 ("usbip: tools: Start using VUDC backend in
usbip tools") implemented device mode for user space tools, however the
corresponding options are not documented in man page.
This commit documents the options and provides examples on device mode.
Also the command `usbip port` is documented.
Signed-off-by: Hongren Zheng
---
tools/usb/usbip/doc/usbip.8 | 42 +++-
tools/usb/usbip/doc/usbipd.8 | 26 ++
2 files changed, 67 insertions(+), 1 deletion(-)
PATCH v2:
Add signed-off-by line
PATCH v3:
Move patch changelog after the marker line
Remove nickname in signed-off-by line
PATCH v4:
Use commit short hash and message instead of long hash only when
referring to commit in the kernel
PATCH v5:
Add documentation of `usbip port` and its usage in examples
Add flow of detaching in examples
Rephrase some description and add punctuations
Fix typo of `usbip attach --ev-id` to `--dev-id`
diff --git a/tools/usb/usbip/doc/usbip.8 b/tools/usb/usbip/doc/usbip.8
index a15d20063b98..1f26e4a00638 100644
--- a/tools/usb/usbip/doc/usbip.8
+++ b/tools/usb/usbip/doc/usbip.8
@@ -49,10 +49,17 @@ then exit.
Attach a remote USB device.
.PP
+.HP
+\fBattach\fR \-\-remote=<\fIhost\fR> \-\-device=<\fIdev_id\fR>
+.IP
+Attach a remote USB gadget.
+Only used when the remote usbipd is in device mode.
+.PP
+
.HP
\fBdetach\fR \-\-port=<\fIport\fR>
.IP
-Detach an imported USB device.
+Detach an imported USB device/gadget.
.PP
.HP
@@ -73,12 +80,26 @@ Stop exporting a device so it can be used by a local driver.
List USB devices exported by a remote host.
.PP
+.HP
+\fBlist\fR \-\-device
+.IP
+List USB gadgets of local usbip-vudc.
+Only used when the local usbipd is in device mode.
+Note that this can not list usbip-vudc USB gadgets of the remote device mode
usbipd.
+.PP
+
.HP
\fBlist\fR \-\-local
.IP
List local USB devices.
.PP
+.HP
+\fBport\fR
+.IP
+List imported devices/gadgets.
+.PP
+
.SH EXAMPLES
@@ -90,8 +111,27 @@ List local USB devices.
client:# usbip attach --remote=server --busid=1-2
- Connect the remote USB device.
+client:# usbip port
+- List imported devices/gadgets.
+
client:# usbip detach --port=0
- Detach the usb device.
+The following example shows the usage of device mode
+
+server:# usbip list --device
+- List gadgets exported by local usbipd server.
+
+client:# modprobe vhci-hcd
+
+client:# usbip attach --remote=server --device=usbip-vudc.0
+- Connect the remote USB gadget.
+
+client:# usbip port
+- List imported devices/gadgets.
+
+client:# usbip detach --port=0
+- Detach the usb gadget.
+
.SH "SEE ALSO"
\fBusbipd\fP\fB(8)\fB\fP
diff --git a/tools/usb/usbip/doc/usbipd.8 b/tools/usb/usbip/doc/usbipd.8
index fb62a756893b..d974394f86a1 100644
--- a/tools/usb/usbip/doc/usbipd.8
+++ b/tools/usb/usbip/doc/usbipd.8
@@ -29,6 +29,12 @@ Bind to IPv4. Default is both.
Bind to IPv6. Default is both.
.PP
+.HP
+\fB\-e\fR, \fB\-\-device\fR
+.IP
+Run in device mode. Rather than drive an attached device, create a virtual UDC
to bind gadgets to.
+.PP
+
.HP
\fB\-D\fR, \fB\-\-daemon\fR
.IP
@@ -86,6 +92,26 @@ USB/IP client can connect and use exported devices.
- A usb device 1-2 is now exportable to other hosts!
- Use 'usbip unbind --busid=1-2' when you want to shutdown exporting
and use the device locally.
+The following example shows the usage of device mode
+
+server:# modprobe usbip-vudc
+- Use /sys/class/udc/ interface.
+- usbip-host is independent of this module.
+
+server:# usbipd -e -D
+- Start usbip daemon in device mode.
+
+server:# modprobe g_mass_storage file=/tmp/tmp.img
+- Bind a gadget to usbip-vudc.
+- in this example, a mass storage gadget is bound.
+
+server:# usbip list --device
+- List gadgets exported by local usbipd server.
+
+server:# modprobe -r g_mass_storage
+- Unbind a gadget from usbip-vudc.
+- in this example, the previous mass storage gadget is unbound.
+
.SH "SEE ALSO"
\fBusbip\fP\fB(8)\fB\fP
Thank you. Looks good.
Acked-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH] tools: usbip: list.h: fix kernel-doc for list_del()
On 3/23/21 6:02 PM, Randy Dunlap wrote:
In list.h, the kernel-doc for list_del() should be immediately
preceding the implementation and not separated from it by
another function implementation.
Eliminates this kernel-doc error:
list.h:1: warning: 'list_del' not found
Signed-off-by: Randy Dunlap
Cc: Valentina Manea
Cc: Shuah Khan
Cc: Shuah Khan
Cc: linux-...@vger.kernel.org
---
tools/usb/usbip/libsrc/list.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
--- linux-next-20210323.orig/tools/usb/usbip/libsrc/list.h
+++ linux-next-20210323/tools/usb/usbip/libsrc/list.h
@@ -77,17 +77,17 @@ static inline void __list_del(struct lis
#define LIST_POISON1 ((void *) 0x00100100 + POISON_POINTER_DELTA)
#define LIST_POISON2 ((void *) 0x00200200 + POISON_POINTER_DELTA)
+static inline void __list_del_entry(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+}
+
/**
* list_del - deletes entry from list.
* @entry: the element to delete from the list.
* Note: list_empty() on entry does not return true after this, the entry is
* in an undefined state.
*/
-static inline void __list_del_entry(struct list_head *entry)
-{
- __list_del(entry->prev, entry->next);
-}
-
static inline void list_del(struct list_head *entry)
{
__list_del(entry->prev, entry->next);
Thank you for fixing this.
Acked-by: Shuah Khan
thanks,
-- Shuah
[PATCH] usbip: vhci_hcd fix shift out-of-bounds in vhci_hub_control()
Fix shift out-of-bounds in vhci_hub_control() SetPortFeature handling.
UBSAN: shift-out-of-bounds in drivers/usb/usbip/vhci_hcd.c:605:42
shift exponent 768 is too large for 32-bit type 'int'
Reported-by: syzbot+3dea30b047f41084d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/vhci_hcd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..a20a8380ca0c 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -594,6 +594,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16
typeReq, u16 wValue,
pr_err("invalid port number %d\n", wIndex);
goto error;
}
+ if (wValue >= 32)
+ goto error;
if (hcd->speed == HCD_USB3) {
if ((vhci_hcd->port_status[rhport] &
USB_SS_PORT_STAT_POWER) != 0) {
--
2.27.0
Re: [syzbot] UBSAN: shift-out-of-bounds in vhci_hub_control (2)
On 3/24/21 2:05 PM, Muhammad Usama Anjum wrote:
On Wed, 2021-03-24 at 10:36 -0700, syzbot wrote:
Hello,
syzbot found the following issue on:
HEAD commit:84196390 Merge tag 'selinux-pr-20210322' of git://git.kern..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12ea778ad0
kernel config: https://syzkaller.appspot.com/x/.config?x=5adab0bdee099d7a
dashboard link: https://syzkaller.appspot.com/bug?extid=3dea30b047f41084de66
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15449662d0
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14f81026d0
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+3dea30b047f41084d...@syzkaller.appspotmail.com
UBSAN: shift-out-of-bounds in drivers/usb/usbip/vhci_hcd.c:605:42
shift exponent 768 is too large for 32-bit type 'int'
CPU: 0 PID: 8421 Comm: syz-executor852 Not tainted 5.12.0-rc4-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:79 [inline]
dump_stack+0x141/0x1d7 lib/dump_stack.c:120
ubsan_epilogue+0xb/0x5a lib/ubsan.c:148
__ubsan_handle_shift_out_of_bounds.cold+0xb1/0x181 lib/ubsan.c:327
vhci_hub_control.cold+0x20b/0x5f0 drivers/usb/usbip/vhci_hcd.c:605
rh_call_control drivers/usb/core/hcd.c:683 [inline]
rh_urb_enqueue drivers/usb/core/hcd.c:841 [inline]
usb_hcd_submit_urb+0xcaf/0x22d0 drivers/usb/core/hcd.c:1544
usb_submit_urb+0x6e4/0x1540 drivers/usb/core/urb.c:585
usb_start_wait_urb+0x101/0x4c0 drivers/usb/core/message.c:58
usb_internal_control_msg drivers/usb/core/message.c:102 [inline]
usb_control_msg+0x31c/0x4a0 drivers/usb/core/message.c:153
do_proc_control+0x4af/0x980 drivers/usb/core/devio.c:1165
proc_control drivers/usb/core/devio.c:1191 [inline]
usbdev_do_ioctl drivers/usb/core/devio.c:2535 [inline]
usbdev_ioctl+0x10e2/0x36c0 drivers/usb/core/devio.c:2708
vfs_ioctl fs/ioctl.c:48 [inline]
__do_sys_ioctl fs/ioctl.c:753 [inline]
__se_sys_ioctl fs/ioctl.c:739 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x443499
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6
48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48
c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:7ffd96535f58 EFLAGS: 0246 ORIG_RAX: 0010
RAX: ffda RBX: 004004a0 RCX: 00443499
RDX: 2000 RSI: c0185500 RDI: 0003
RBP: 00403040 R08: R09: 004004a0
R10: 000f R11: 0246 R12: 004030d0
R13: R14: 004b1018 R15: 004004a0
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkal...@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
I've tested with following patch locally and issue is solved.
Porting fix from: c318840fb2 ("USB: Gadget: dummy-hcd: Fix shift-out-of-bounds
bug")
Lets ask the syzbot to test the patch also.
#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
84196390
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..6e12b60d4f5c 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -393,13 +393,24 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16
typeReq, u16 wValue,
else
vhci_hcd->port_status[rhport] &=
~USB_PORT_STAT_POWER;
break;
- default:
- usbip_dbg_vhci_rh(" ClearPortFeature: default %x\n",
- wValue);
+ case USB_PORT_FEAT_ENABLE:
+ case USB_PORT_FEAT_C_ENABLE:
+ case USB_PORT_FEAT_C_SUSPEND:
+ /* Not allowed for USB-3 */
+ if (hcd->speed == HCD_USB3)
+ goto error;
+ fallthrough;
+ case USB_PORT_FEAT_C_CONNECTION:
+ case USB_PORT_FEAT_C_RESET:
if (wValue >= 32)
goto error;
vhci_hcd->port_status[rhport] &= ~(1 << wValue);
break;
+ default:
+ /* Disallow INDICATOR and C_OVER_CURRENT */
+
Re: [PATCH v4 1/2] usbip: tools: add options and examples in man page related to device mode
On 3/23/21 6:55 AM, Hongren Zheng (Zenithal) wrote:
The commit e0546fd8b748 ("usbip: tools: Start using VUDC backend in
usbip tools") implemented device mode for user space tools, however the
corresponding options are not documented in man page.
This commit documents the options and provides examples on device mode.
Signed-off-by: Hongren Zheng
---
tools/usb/usbip/doc/usbip.8 | 25 +
tools/usb/usbip/doc/usbipd.8 | 22 ++
2 files changed, 47 insertions(+)
PATCH v2:
Add signed-off-by line
PATCH v3:
Move patch changelog after the marker line
Remove nickname in signed-off-by line
PATCH v4:
Use commit short hash and message instead of long hash only when
referring to commit in the kernel
Thank you for the patch. Please see comments below:
diff --git a/tools/usb/usbip/doc/usbip.8 b/tools/usb/usbip/doc/usbip.8
index a15d20063b98..385b0eda8746 100644
--- a/tools/usb/usbip/doc/usbip.8
+++ b/tools/usb/usbip/doc/usbip.8
@@ -49,6 +49,13 @@ then exit.
Attach a remote USB device.
.PP
+.HP
+\fBattach\fR \-\-remote=<\fIhost\fR> \-\-device=<\fdev_id\fR>
+.IP
+Attach a remote USB gadget.
+Only used when the remote usbipd is in device mode.
+.PP
+
.HP
\fBdetach\fR \-\-port=<\fIport\fR>
.IP
This is a bit confusing. Please add a separate section for
Attach a remote USB gadget complete with attach and detach
instructions.
@@ -73,6 +80,14 @@ Stop exporting a device so it can be used by a local driver.
List USB devices exported by a remote host.
.PP
+.HP
+\fBlist\fR \-\-device
+.IP
+List USB gadgets of local usbip-vudc.
+Only used when the local usbipd is in device mode.
+This can not list usbip-vudc USB gadgets of the remote device mode usbipd.
+.PP
+
.HP
\fBlist\fR \-\-local
.IP
@@ -93,5 +108,15 @@ List local USB devices.
client:# usbip detach --port=0
- Detach the usb device.
+The following example shows the use of device mode
+
+server:# usbip list --device
+- Note this is the server side
+
+client:# modprobe vhci-hcd
+
+client:# usbip attach --remote=server --device=usbip-vudc.0
+- Connect the remote USB gadget
+
.SH "SEE ALSO"
\fBusbipd\fP\fB(8)\fB\fP
diff --git a/tools/usb/usbip/doc/usbipd.8 b/tools/usb/usbip/doc/usbipd.8
index fb62a756893b..53c8d5792de6 100644
--- a/tools/usb/usbip/doc/usbipd.8
+++ b/tools/usb/usbip/doc/usbipd.8
@@ -29,6 +29,12 @@ Bind to IPv4. Default is both.
Bind to IPv6. Default is both.
.PP
+.HP
+\fB\-e\fR, \fB\-\-device\fR
+.IP
+Run in device mode. Rather than drive an attached device, create a virtual UDC
to bind gadgets to.
+.PP
+
.HP
\fB\-D\fR, \fB\-\-daemon\fR
.IP
@@ -86,6 +92,22 @@ USB/IP client can connect and use exported devices.
- A usb device 1-2 is now exportable to other hosts!
- Use 'usbip unbind --busid=1-2' when you want to shutdown exporting
and use the device locally.
+The following example shows the use of device mode
+
+server:# modprobe usbip-vudc
+- Use /sys/class/udc/ interface
+- usbip-host is independent of this module.
+
+server:# usbipd -e -D
+- Start usbip daemon in device mode.
+
+server:# modprobe g_mass_storage file=/tmp/tmp.img
+- Bind a gadget to usbip-vudc
+- in this example, a mass storage gadget is bound
+
+server:# usbip list --device
+- Note this is the server side
+
.SH "SEE ALSO"
\fBusbip\fP\fB(8)\fB\fP
thanks,
-- Shuah
[GIT PULL] KUnit fixes update for Linux 5.12-rc5
Hi Linus,
Please pull the following KUnit fixes update for Linux 5.12-rc5.
This KUnit update for Linux 5.12-rc5 consists of two fixes to kunit
tool from David Gow.
diff is attached.
thanks,
-- Shuah
The following changes since commit a38fd8748464831584a19438cbb3082b5a2dab15:
Linux 5.12-rc2 (2021-03-05 17:33:41 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
tags/linux-kselftest-kunit-fixes-5.12-rc5.1
for you to fetch changes up to 7fd53f41f771d250eb08db08650940f017e37c26:
kunit: tool: Disable PAGE_POISONING under --alltests (2021-03-11
14:37:37 -0700)
linux-kselftest-kunit-fixes-5.12-rc5.1
This KUnit update for Linux 5.12-rc5 consists of two fixes to kunit
tool from David Gow.
David Gow (2):
kunit: tool: Fix a python tuple typing error
kunit: tool: Disable PAGE_POISONING under --alltests
tools/testing/kunit/configs/broken_on_uml.config | 2 ++
tools/testing/kunit/kunit_config.py | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/kunit/configs/broken_on_uml.config b/tools/testing/kunit/configs/broken_on_uml.config
index a7f0603d33f6..690870043ac0 100644
--- a/tools/testing/kunit/configs/broken_on_uml.config
+++ b/tools/testing/kunit/configs/broken_on_uml.config
@@ -40,3 +40,5 @@
# CONFIG_RESET_BRCMSTB_RESCAL is not set
# CONFIG_RESET_INTEL_GW is not set
# CONFIG_ADI_AXI_ADC is not set
+# CONFIG_DEBUG_PAGEALLOC is not set
+# CONFIG_PAGE_POISONING is not set
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index 0b550cbd667d..1e2683dcc0e7 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -13,7 +13,7 @@ from typing import List, Set
CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
-KconfigEntryBase = collections.namedtuple('KconfigEntry', ['name', 'value'])
+KconfigEntryBase = collections.namedtuple('KconfigEntryBase', ['name', 'value'])
class KconfigEntry(KconfigEntryBase):
Re: [PATCH v4 2/2] usbip: tools: add usage of device mode in usbip_list.c
On 3/23/21 7:01 AM, Hongren Zheng (Zenithal) wrote:
The option '-d/--device' was implemented in 'usbip list' but not
shown in usage. Hence this commit adds this option to usage.
Signed-off-by: Hongren Zheng
---
tools/usb/usbip/src/usbip_list.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
PATCH v2:
Add signed-off-by line
PATCH v3:
Move patch changelog after the marker line
Remove nickname in signed-off-by line
PATCH v4:
Use commit short hash and message instead of long hash only when
referring to commit in the kernel
diff --git a/tools/usb/usbip/src/usbip_list.c b/tools/usb/usbip/src/usbip_list.c
index 8625b0f514ee..3d810bcca02f 100644
--- a/tools/usb/usbip/src/usbip_list.c
+++ b/tools/usb/usbip/src/usbip_list.c
@@ -33,7 +33,8 @@ static const char usbip_list_usage_string[] =
"usbip list [-p|--parsable] \n"
"-p, --parsable Parsable list format\n"
"-r, --remote=List the exportable USB devices on \n"
- "-l, --localList the local USB devices\n";
+ "-l, --localList the local USB devices\n"
+ "-d, --device List the local USB gadgets bound to
usbip-vudc\n";
void usbip_list_usage(void)
{
Looks good to me. Thanks for adding this.
Acked-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH 0/6] usbip fixes to crashes found by syzbot
On 3/17/21 9:06 AM, Shuah Khan wrote:
On 3/17/21 12:21 AM, Tetsuo Handa wrote:
Shuah, this driver is getting more and more cryptic and buggy.
Please explain the strategy for serialization before you write patches.
- Fix attach_store() to check usbip_event_happened() before
waking up threads.
No, this helps nothing.
diff --git a/drivers/usb/usbip/vhci_sysfs.c
b/drivers/usb/usbip/vhci_sysfs.c
index c4b4256e5dad3..f0a770adebd97 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -418,6 +418,15 @@ static ssize_t attach_store(struct device *dev,
struct device_attribute *attr,
spin_unlock_irqrestore(>lock, flags);
/* end the lock */
+ if (usbip_event_happened(>ud)) {
+ /*
+ * something went wrong - event handler shutting
+ * the connection and doing reset - bail out
+ */
+ dev_err(dev, "Event happended - handler is active\n");
+ return -EAGAIN;
+ }
+
detach_store() can queue shutdown event as soon as reaching "/* end
the lock */" line
but attach_store() might be preempted immediately after verifying that
usbip_event_happened() was false (i.e. at this location) in order to
wait for
shutdown event posted by detach_store() to be processed.
Please don't review code that isn't sent upstream. This repo you are
looking at is a private branch created just to verify fixes on syzbot.
I understand the race window you are talking about. I have my way of
working to resolve it.
thanks,
-- Shuah
Re: [PATCH 0/6] usbip fixes to crashes found by syzbot
On 3/17/21 9:38 AM, Tetsuo Handa wrote: On 2021/03/18 0:06, Shuah Khan wrote: Yes. I haven't sent the patch for that reason. I am trying to test a solution. I haven't come up with a solution yet. Holding event_lock isn't the right solution. I am not going to accept that. This is a window that gets triggered by syzbot injecting errors in a sequence. Fixing this should be done taking other moving parts of the driver into account. What is event_lock ? I questioned you what the event_lock is at https://lkml.kernel.org/r/3dab66dc-2981-bc88-a370-4b3178dfd...@i-love.sakura.ne.jp , but you haven't responded to that post. Also, you need to send https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux.git/commit/?h=usbip_test=f345de0d2e51a20a2a1c30fc22fa1527670d2095 because Greg already sent this regression into upstream and stable kernels. I acked it when it came in and it will be picked up. thanks, -- Shuah
Re: [PATCH 0/6] usbip fixes to crashes found by syzbot
On 3/17/21 12:21 AM, Tetsuo Handa wrote:
Shuah, this driver is getting more and more cryptic and buggy.
Please explain the strategy for serialization before you write patches.
- Fix attach_store() to check usbip_event_happened() before
waking up threads.
No, this helps nothing.
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c4b4256e5dad3..f0a770adebd97 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -418,6 +418,15 @@ static ssize_t attach_store(struct device *dev, struct
device_attribute *attr,
spin_unlock_irqrestore(>lock, flags);
/* end the lock */
+ if (usbip_event_happened(>ud)) {
+ /*
+* something went wrong - event handler shutting
+* the connection and doing reset - bail out
+*/
+ dev_err(dev, "Event happended - handler is active\n");
+ return -EAGAIN;
+ }
+
detach_store() can queue shutdown event as soon as reaching "/* end the lock
*/" line
but attach_store() might be preempted immediately after verifying that
usbip_event_happened() was false (i.e. at this location) in order to wait for
shutdown event posted by detach_store() to be processed.
Yes. I haven't sent the patch for that reason. I am trying to test a
solution. I haven't come up with a solution yet.
Holding event_lock isn't the right solution. I am not going to accept
that. This is a window that gets triggered by syzbot injecting errors
in a sequence. Fixing this should be done taking other moving parts of
the driver into account.
thanks,
-- Shuah
Re: [PATCH] usbip: fix vhci races in connection tear down
On 3/12/21 12:08 AM, Hillf Danton wrote: On Thu, 11 Mar 2021 19:27:37 -0700 Shuah Khan wrote: vhci_shutdown_connection() references connection state (tcp_socket, tcp_rx, tcp_tx, sockfd) saved in usbpip_device without holding the lock. Current connection tear down sequence: Step 1: shutdown the socket Step 2: stop rx thread and reset tcp_rx pointer Step 3: stop tx thread and reset tcp_tx pointer Step 4: Reset tcp_socket and sockfd There are several race windows between these steps. In addition, device reset routine (vhci_device_reset) resets tcp_socket and sockfd holding the lock. Can you specify the scenario where reset runs in race with teardown as both are parts of usbip_work on a singlethread workqueue? Hmm. I can't think of one. I was concerned about any async paths that potentially interfere with shutdown. With vhci_shutdown_connection() being so relaxed with locking, this is a cautious approach on my part. I am also keeping in mind that this problem shows up in a limited scope fuzzing test that doesn't trigger any other normal paths that would be active if there is real device on the other side. As for the tcp_socket check in the reset routine, I am not positive what purpose it serves. I introduced the in_disconnect flag so shutdown and reset don't collide, in case I am missing some scenario in the normal path when we actually have a actual device attached. With the other locking and error path problems in addressed, both shutdown and reset could be made simpler. In any case, I think in_disconnect might be too big a hammer. I will redo the patch without it and also remove tcp_socket handling from the reset routine. I don't see USBIP_EH_RESET getting set without USBIP_EH_SHUTDOWN. thanks, -- Shuah
Re: [PATCH] usbip: fix vhci races in connection tear down
On 3/12/21 3:45 AM, Johan Hovold wrote:
On Thu, Mar 11, 2021 at 07:27:37PM -0700, Shuah Khan wrote:
vhci_shutdown_connection() references connection state (tcp_socket,
tcp_rx, tcp_tx, sockfd) saved in usbpip_device without holding the
lock.
Current connection tear down sequence:
Step 1: shutdown the socket
Step 2: stop rx thread and reset tcp_rx pointer
Step 3: stop tx thread and reset tcp_tx pointer
Step 4: Reset tcp_socket and sockfd
There are several race windows between these steps. In addition, device
reset routine (vhci_device_reset) resets tcp_socket and sockfd holding
the lock.
Fix these races:
- Introduce in_disconnect flag to ensure vhci_shutdown_connection() runs
only once.
- Change attach_store() to initialize in_disconnect to false while
initializing connection status (tcp_socket, tcp_rx, tcp_tx, sockfd)
- Change vhci_shutdown_connection() to check in_disconnect and bail
out if disconnect is in progress.
- Change vhci_shutdown_connection() to
-- hold lock to save connection state pointers and unlock.
-- Shutdown the socket and stop threads.
-- Hold lock to clear connection status and in_disconnect flag.
- Change vhci_device_reset() to reset tcp_socket and sockfd.
if !in_disconnect
Tested syzbot and the reproducer did not trigger any issue.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/usbip_common.h | 1 +
drivers/usb/usbip/vhci_hcd.c | 55 +++-
drivers/usb/usbip/vhci_sysfs.c | 4 +++
3 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..c1917efe5737 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1007,31 +1007,54 @@ static void vhci_device_unlink_cleanup(struct
vhci_device *vdev)
static void vhci_shutdown_connection(struct usbip_device *ud)
{
struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
+ unsigned long flags;
+ struct socket *socket;
+ struct task_struct *tcp_rx = NULL;
+ struct task_struct *tcp_tx = NULL;
+ int sockfd = 0;
+
+ spin_lock_irqsave(>lock, flags);
+ if (vdev->ud.in_disconnect) {
+ pr_info("%s: Disconnect in progress for sockfd %d\n",
+ __func__, ud->sockfd);
Looks like you forgot to remove all you debug printks like this one
before submitting.
Some printks were already in there and helped with debug. Yes I added
a few more when I submitted for syzbot testing.
I will clean them up i v2.
thanks,
-- Shuah
[PATCH] usbip: fix vhci races in connection tear down
vhci_shutdown_connection() references connection state (tcp_socket,
tcp_rx, tcp_tx, sockfd) saved in usbpip_device without holding the
lock.
Current connection tear down sequence:
Step 1: shutdown the socket
Step 2: stop rx thread and reset tcp_rx pointer
Step 3: stop tx thread and reset tcp_tx pointer
Step 4: Reset tcp_socket and sockfd
There are several race windows between these steps. In addition, device
reset routine (vhci_device_reset) resets tcp_socket and sockfd holding
the lock.
Fix these races:
- Introduce in_disconnect flag to ensure vhci_shutdown_connection() runs
only once.
- Change attach_store() to initialize in_disconnect to false while
initializing connection status (tcp_socket, tcp_rx, tcp_tx, sockfd)
- Change vhci_shutdown_connection() to check in_disconnect and bail
out if disconnect is in progress.
- Change vhci_shutdown_connection() to
-- hold lock to save connection state pointers and unlock.
-- Shutdown the socket and stop threads.
-- Hold lock to clear connection status and in_disconnect flag.
- Change vhci_device_reset() to reset tcp_socket and sockfd.
if !in_disconnect
Tested syzbot and the reproducer did not trigger any issue.
Reported-and-tested-by: syzbot+a93fba6d384346a76...@syzkaller.appspotmail.com
Signed-off-by: Shuah Khan
---
drivers/usb/usbip/usbip_common.h | 1 +
drivers/usb/usbip/vhci_hcd.c | 55 +++-
drivers/usb/usbip/vhci_sysfs.c | 4 +++
3 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index d60ce17d3dd2..f6261c5a8c91 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -268,6 +268,7 @@ struct usbip_device {
struct task_struct *tcp_rx;
struct task_struct *tcp_tx;
+ bool in_disconnect; /* run device disconnect just once */
unsigned long event;
wait_queue_head_t eh_waitq;
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..c1917efe5737 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1007,31 +1007,54 @@ static void vhci_device_unlink_cleanup(struct
vhci_device *vdev)
static void vhci_shutdown_connection(struct usbip_device *ud)
{
struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
+ unsigned long flags;
+ struct socket *socket;
+ struct task_struct *tcp_rx = NULL;
+ struct task_struct *tcp_tx = NULL;
+ int sockfd = 0;
+
+ spin_lock_irqsave(>lock, flags);
+ if (vdev->ud.in_disconnect) {
+ pr_info("%s: Disconnect in progress for sockfd %d\n",
+ __func__, ud->sockfd);
+ spin_unlock_irqrestore(>lock, flags);
+ return;
+ }
+ vdev->ud.in_disconnect = true;
+ socket = ud->tcp_socket;
+ tcp_rx = vdev->ud.tcp_rx;
+ tcp_tx = vdev->ud.tcp_tx;
+ sockfd = ud->sockfd;
+ spin_unlock_irqrestore(>lock, flags);
/* need this? see stub_dev.c */
- if (ud->tcp_socket) {
- pr_debug("shutdown tcp_socket %d\n", ud->sockfd);
- kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
+ if (socket) {
+ pr_info("%s: shutdown tcp_socket %d\n", __func__, sockfd);
+ kernel_sock_shutdown(socket, SHUT_RDWR);
}
- /* kill threads related to this sdev */
- if (vdev->ud.tcp_rx) {
- kthread_stop_put(vdev->ud.tcp_rx);
- vdev->ud.tcp_rx = NULL;
+ /* kill threads related to this vdev */
+ if (tcp_rx) {
+ pr_info("%s: stop rx thread\n", __func__);
+ kthread_stop_put(tcp_rx);
}
- if (vdev->ud.tcp_tx) {
- kthread_stop_put(vdev->ud.tcp_tx);
- vdev->ud.tcp_tx = NULL;
+ if (tcp_tx) {
+ pr_info("%s: stop tx thread\n", __func__);
+ kthread_stop_put(tcp_tx);
}
- pr_info("stop threads\n");
+ spin_lock_irqsave(>lock, flags);
/* active connection is closed */
- if (vdev->ud.tcp_socket) {
+ if (ud->tcp_socket) {
+ vdev->ud.tcp_rx = NULL;
+ vdev->ud.tcp_tx = NULL;
sockfd_put(vdev->ud.tcp_socket);
vdev->ud.tcp_socket = NULL;
vdev->ud.sockfd = -1;
}
- pr_info("release socket\n");
+ vdev->ud.in_disconnect = false;
+ spin_unlock_irqrestore(>lock, flags);
+ pr_info("%s: release socket\n", __func__);
vhci_device_unlink_cleanup(vdev);
@@ -1057,7 +1080,7 @@ static void vhci_shutdown_connection(struct usbip_device
*ud)
*/
rh_port_disconnect(vdev);
- pr_info("disconnect device\n"
Re: [PATCH][next] usbip: Fix incorrect double assignment to udc->ud.tcp_rx
On 3/11/21 7:16 AM, Shuah Khan wrote:
On 3/11/21 3:44 AM, Colin King wrote:
From: Colin Ian King
Currently udc->ud.tcp_rx is being assigned twice, the second assignment
is incorrect, it should be to udc->ud.tcp_tx instead of rx. Fix this.
Addresses-Coverity: ("Unused value")
Fixes: 46613c9dfa96 ("usbip: fix vudc usbip_sockfd_store races leading
to gpf")
Signed-off-by: Colin Ian King
---
drivers/usb/usbip/vudc_sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/vudc_sysfs.c
b/drivers/usb/usbip/vudc_sysfs.c
index a3ec39fc6177..7383a543c6d1 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -174,7 +174,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
udc->ud.tcp_socket = socket;
udc->ud.tcp_rx = tcp_rx;
- udc->ud.tcp_rx = tcp_tx;
+ udc->ud.tcp_tx = tcp_tx;
udc->ud.status = SDEV_ST_USED;
spin_unlock_irq(>ud.lock);
Thank you for finding and fixing this. This is my bad.
Acked-by: Shuah Khan
Applies to stables as well since the 46613c9dfa96 is marked for
stables.
thanks,
-- Shuah
Re: [PATCH][next] usbip: Fix incorrect double assignment to udc->ud.tcp_rx
On 3/11/21 3:44 AM, Colin King wrote:
From: Colin Ian King
Currently udc->ud.tcp_rx is being assigned twice, the second assignment
is incorrect, it should be to udc->ud.tcp_tx instead of rx. Fix this.
Addresses-Coverity: ("Unused value")
Fixes: 46613c9dfa96 ("usbip: fix vudc usbip_sockfd_store races leading to gpf")
Signed-off-by: Colin Ian King
---
drivers/usb/usbip/vudc_sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
index a3ec39fc6177..7383a543c6d1 100644
--- a/drivers/usb/usbip/vudc_sysfs.c
+++ b/drivers/usb/usbip/vudc_sysfs.c
@@ -174,7 +174,7 @@ static ssize_t usbip_sockfd_store(struct device *dev,
udc->ud.tcp_socket = socket;
udc->ud.tcp_rx = tcp_rx;
- udc->ud.tcp_rx = tcp_tx;
+ udc->ud.tcp_tx = tcp_tx;
udc->ud.status = SDEV_ST_USED;
spin_unlock_irq(>ud.lock);
Thank you for finding and fixing this. This is my bad.
Acked-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH 4.4 0/7] 4.4.261-rc1 review
On 3/10/21 6:25 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 4.4.261 release. There are 7 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.261-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/39] 4.19.180-rc1 review
On 3/10/21 6:24 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 4.19.180 release. There are 39 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.180-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/11] 4.9.261-rc1 review
On 3/10/21 6:24 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 4.9.261 release. There are 11 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.261-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 00/24] 5.4.105-rc1 review
On 3/10/21 6:24 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 5.4.105 release. There are 24 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.105-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 00/36] 5.11.6-rc1 review
On 3/10/21 6:23 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 5.11.6 release. There are 36 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.6-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/47] 5.10.23-rc2 review
On 3/10/21 11:29 AM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 5.10.23 release. There are 47 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 18:28:23 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.23-rc2.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/49] 5.10.23-rc1 review
On 3/10/21 11:27 AM, Greg Kroah-Hartman wrote: On Wed, Mar 10, 2021 at 11:08:10PM +0530, Naresh Kamboju wrote: On Wed, 10 Mar 2021 at 18:54, wrote: From: Greg Kroah-Hartman This is the start of the stable review cycle for the 5.10.23 release. There are 49 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Fri, 12 Mar 2021 13:23:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.23-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h While building stable rc 5.10 for arm64 the build failed due to the following errors / warnings. make --silent --keep-going --jobs=8 O=/home/tuxbuild/.cache/tuxmake/builds/1/tmp ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- 'HOSTCC=sccache clang' 'CC=sccache clang' drivers/net/ipa/gsi.c:1074:7: error: use of undeclared identifier 'GENERIC_EE_CHANNEL_NOT_RUNNING_FVAL' case GENERIC_EE_CHANNEL_NOT_RUNNING_FVAL: ^ 1 error generated. make[4]: *** [scripts/Makefile.build:279: drivers/net/ipa/gsi.o] Error 1 Offending patch dropped, thanks. Thanks. Saw the same error on my system. Moving on to rc2. thanks, -- Shuah
Re: [PATCH v2] kunit: fix checkpatch warning
On 3/4/21 4:12 PM, Shuah Khan wrote: On 3/3/21 9:35 PM, Lucas Stankus wrote: On Wed, Mar 03, 2021 at 12:56:05PM -0800, Brendan Higgins wrote: Did you change anything other than fixing the Signed-off-by that Shuah requested? No, I only fixed the Signed-off-by warning. Generally when you make a small change after receiving a Reviewed-by (especially one so small as here), you are supposed to include the Reviewed-by with the other git commit message footers directly below the "Signed-off-by". Please remember to do so in the future. Also, when you make a change to a patch and send out a subsequent revision, it is best practice to make note explaining the changes you made since the last revision in the "comment section" [1] of the git-diff, right after the three dashes and before the change log as you can see in this example [2] (note that everything after "Signed-off-by: David Gow \n ---" and before "tools/testing/kunit/configs/broken_on_uml.config | 2 ++" is discarded by git am). Sorry for the incovenience regarding best practices, I'll keep that noted for further contributions. Sorry I should have asked you about this. I like to see what is being fixed in the subject line. Can you update the subject line. The current one doesn't say anything about the nature of the fix. Also please run the checkpatch script on your patches. This tool useful and can offer you tips on improving your commit log as well as code. thanks, -- Shuah
Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf
On 3/9/21 6:02 PM, Tetsuo Handa wrote: On 2021/03/10 9:29, Shuah Khan wrote: It is not a large grain lock. Since event_handler() is exclusively executed, this lock does _NOT_ block event_handler() unless attach/detach operations run concurrently. event handler queues the events. It shouldn't be blocked by attach and detach. The events could originate for various reasons during the host and vhci operations. I don't like using this lock for attach and detach. How can attach/detach deadlock event_handler()? event_handler() calls e.g. vhci_shutdown_connection() via ud->eh_ops.shutdown(ud). vhci_shutdown_connection() e.g. waits for termination of tx/rx threads via kthread_stop_put(). event_handler() is already blocked by detach operation. How it can make situation worse to wait for creation of tx/rx threads in attach operation? event_lock shouldn't be held during event ops. usbip_event_add() uses it to add events. Protecting shutdown path needs a different approach. In any case, do you have comments on this patch which doesn't even touch vhci driver? I understand you are identifying additional race condition that the vhci patches in this series might not fix. That doesn't mean that these patches aren't valid. Do you have any comments specific to the patches in this series? thanks, -- Shuah
Re: [PATCH 4/6] usbip: fix stub_dev usbip_sockfd_store() races leading to gpf
On 3/9/21 5:03 PM, Tetsuo Handa wrote: On 2021/03/10 8:52, Shuah Khan wrote: On 3/9/21 4:40 PM, Tetsuo Handa wrote: On 2021/03/10 4:50, Shuah Khan wrote: On 3/9/21 4:04 AM, Tetsuo Handa wrote: On 2021/03/09 1:27, Shuah Khan wrote: Yes. We might need synchronization between events, threads, and shutdown in usbip_host side and in connection polling and threads in vhci. I am also looking at the shutdown sequences closely as well since the local state is referenced without usbip_device lock in these paths. I am approaching these problems as peeling the onion an expression so we can limit the changes and take a spot fix approach. We have the goal to address these crashes and not introduce regressions. I think my [PATCH v4 01/12]-[PATCH v4 06/12] simplify your further changes without introducing regressions. While ud->lock is held when checking ud->status, current attach/detach code is racy about read/update of ud->status . I think we can close race in attach/detach code via a simple usbip_event_mutex serialization. Do you mean patches 1,2,3,3,4,5,6? Yes, my 1,2,3,4,5,6. Since you think that usbip_prepare_threads() does not worth introducing, I'm fine with replacing my 7,8,9,10,11,12 with your "[PATCH 0/6] usbip fixes to crashes found by syzbot". Using event lock isn't the right approach to solve the race. It is a large grain lock. I am not looking to replace patches. It is not a large grain lock. Since event_handler() is exclusively executed, this lock does _NOT_ block event_handler() unless attach/detach operations run concurrently. event handler queues the events. It shouldn't be blocked by attach and detach. The events could originate for various reasons during the host and vhci operations. I don't like using this lock for attach and detach. I still haven't seen any response from you about if you were able to verify the fixes I sent in fix the problem you are seeing. > I won't be able to verify your fixes, for it is syzbot who is seeing the problem. Thank you for letting me know. thanks, -- Shuah
