| Message ID | 20230608011554.1181097-2-sunliming@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tracing/user_events: Fix incorrect return value for | expand |
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; > > - } > > + } 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 > 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-sunliming@kylinos.cn/T/#t Thanks. > > return ret; > > } > > -- > > 2.25.1
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; }
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(-)