Now the writing operation return the count of writes whether events are enabled or disabled. Fix this by just return -EFAULT when events are disabled.
sunliming (3): tracing/user_events: Fix incorrect return value for writing operation when events are disabled selftests/user_events: Enable the event before write_fault test in ftrace self-test selftests/user_events: Add test cases when event is disabled
kernel/trace/trace_events_user.c | 3 ++- tools/testing/selftests/user_events/ftrace_test.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
The writing operation return the count of writes whether events are enabled or disabled. This is incorrect when events are disabled. Fix this by just return -EFAULT when events are disabled.
Signed-off-by: sunliming sunliming@kylinos.cn --- kernel/trace/trace_events_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 1ac5ba5685ed..970bac0503fd 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1957,7 +1957,8 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
if (unlikely(faulted)) return -EFAULT; - } + } else + return -EFAULT;
return ret; }
On Thu, Jun 08, 2023 at 09:15:52AM +0800, sunliming wrote:
The writing operation return the count of writes whether events are enabled or disabled. This is incorrect when events are disabled. Fix this by just return -EFAULT when events are disabled.
Signed-off-by: sunliming sunliming@kylinos.cn
kernel/trace/trace_events_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 1ac5ba5685ed..970bac0503fd 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1957,7 +1957,8 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i) if (unlikely(faulted)) return -EFAULT;
- }
- } else
return -EFAULT;
I'm not sure this is a good idea. Imagine this scenario: A user process writes out a user_event and it hits a fault that gets returned as errno (EFAULT).
The user process is likely to either forget it and say, not worth retrying, or it will retry (potentially in a loop).
If the process does retry and it's now disabled, it might try many times.
I think that -ENOENT is a better error to use here. That way a user process will know it got disabled mid-write vs a fault that might want to be re-attempted.
Thanks, -Beau
return ret; } -- 2.25.1
Beau Belgrave beaub@linux.microsoft.com 于2023年6月9日周五 01:19写道：
On Thu, Jun 08, 2023 at 09:15:52AM +0800, sunliming wrote:
The writing operation return the count of writes whether events are enabled or disabled. This is incorrect when events are disabled. Fix this by just return -EFAULT when events are disabled.
Signed-off-by: sunliming sunliming@kylinos.cn
kernel/trace/trace_events_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 1ac5ba5685ed..970bac0503fd 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1957,7 +1957,8 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)
if (unlikely(faulted)) return -EFAULT;
}
} elsereturn -EFAULT;I'm not sure this is a good idea. Imagine this scenario: A user process writes out a user_event and it hits a fault that gets returned as errno (EFAULT).
The user process is likely to either forget it and say, not worth retrying, or it will retry (potentially in a loop).
If the process does retry and it's now disabled, it might try many times.
I think that -ENOENT is a better error to use here. That way a user process will know it got disabled mid-write vs a fault that might want to be re-attempted.
Thanks, -Beau
I think you are right. I have resend the V2 version of this series of patches based on suggestions, patches link : https://lore.kernel.org/linux-trace-kernel/20230609030302.1278716-1-sunlimin... Thanks.
return ret;}
2.25.1
The user_event has not be enabled in write_fault test in ftrace self-test, Just enable it.
Signed-off-by: sunliming sunliming@kylinos.cn --- tools/testing/selftests/user_events/ftrace_test.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c index abfb49558a26..d33bd31425db 100644 --- a/tools/testing/selftests/user_events/ftrace_test.c +++ b/tools/testing/selftests/user_events/ftrace_test.c @@ -373,6 +373,10 @@ TEST_F(user, write_fault) { ASSERT_EQ(0, ioctl(self->data_fd, DIAG_IOCSREG, ®)); ASSERT_EQ(0, reg.write_index);
+ /* Enable event */ + self->enable_fd = open(enable_file, O_RDWR); + ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1"))) + /* Write should work normally */ ASSERT_NE(-1, writev(self->data_fd, (const struct iovec *)io, 2));
When user_events are disabled, it's write operation should be fail. Add this test cases.
Signed-off-by: sunliming sunliming@kylinos.cn --- tools/testing/selftests/user_events/ftrace_test.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c index d33bd31425db..d3240a97f23d 100644 --- a/tools/testing/selftests/user_events/ftrace_test.c +++ b/tools/testing/selftests/user_events/ftrace_test.c @@ -297,6 +297,9 @@ TEST_F(user, write_events) { io[0].iov_base = ®.write_index; io[0].iov_len = sizeof(reg.write_index);
+ /* Write should fail when event is not enabled */ + ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3)); + /* Enable event */ self->enable_fd = open(enable_file, O_RDWR); ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1")))
On Thu, Jun 08, 2023 at 09:15:54AM +0800, sunliming wrote:
When user_events are disabled, it's write operation should be fail. Add this test cases.
Signed-off-by: sunliming sunliming@kylinos.cn
tools/testing/selftests/user_events/ftrace_test.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/user_events/ftrace_test.c b/tools/testing/selftests/user_events/ftrace_test.c index d33bd31425db..d3240a97f23d 100644 --- a/tools/testing/selftests/user_events/ftrace_test.c +++ b/tools/testing/selftests/user_events/ftrace_test.c @@ -297,6 +297,9 @@ TEST_F(user, write_events) { io[0].iov_base = ®.write_index; io[0].iov_len = sizeof(reg.write_index);
- /* Write should fail when event is not enabled */
- ASSERT_EQ(-1, writev(self->data_fd, (const struct iovec *)io, 3));
We should also check errno value in this case. See my reply in the other patch about -EFAULT vs -ENOENT in this case.
Thanks, -Beau
/* Enable event */ self->enable_fd = open(enable_file, O_RDWR); ASSERT_NE(-1, write(self->enable_fd, "1", sizeof("1"))) -- 2.25.1
linux-kselftest-mirror@lists.linaro.org
-
Beau Belgrave -
sunliming -
sunliming