| Message ID | 20240712201258.99070-1-kiryushin@ancud.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | tracing: remove unreachable trace_array_put | expand |
On Fri, 12 Jul 2024 23:12:58 +0300 Nikita Kiryushin <kiryushin@ancud.ru> wrote: > There is a trace_array_put() in check result for > nonseekable_open() in tracing_buffers_open(). However, > it would be never executed as nonseekable_open never fails > (by design). > > Remove the check and associated unreachable code. Then why does it return a value? If someday it can return a failure, this would then cause a leak. It doesn't hurt to leave it in. So NACK. -- Steve > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 7b85af630348 ("tracing: Get trace_array ref counts when accessing trace files") > Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru> > --- > kernel/trace/trace.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 578a49ff5c32..7e480501b509 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -7883,11 +7883,7 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) > > mutex_unlock(&trace_types_lock); > > - ret = nonseekable_open(inode, filp); > - if (ret < 0) > - trace_array_put(tr); > - > - return ret; > + return nonseekable_open(inode, filp); > } > > static __poll_t
As nonseekable_open() documentation states: "The function is not supposed to ever fail, the only reason it returns an 'int' and not 'void' is so that it can be plugged directly into file_operations structure." So it seems, that it will not fail anytime as it is not meant to? Otherwise, there will be a huge problem with leaks in many other parts of code, as there are plenty of places, where nonseekable_open() is not checked after resource allocations. On 7/13/24 02:33, Steven Rostedt wrote: > Then why does it return a value? > > If someday it can return a failure, this would then cause a leak. It > doesn't hurt to leave it in. > > So NACK. > > -- Steve >
On 15.07.2024 16:47, Nikita Kiryushin wrote: > As nonseekable_open() documentation states: > "The function is not supposed to ever fail, the only > reason it returns an 'int' and not 'void' is so that it can be plugged > directly into file_operations structure." > > So it seems, that it will not fail anytime as it is not meant to? > Otherwise, > there will be a huge problem with leaks in many other parts of code, as > there are plenty of places, where nonseekable_open() is not checked after > resource allocations. Yes, but there is another possible modification: replacement of call to nonseekable_open() by a call to some other function that returns error. Current code is already ready for such modification. -- Alexey
On 7/16/24 12:45, Alexey Khoroshilov wrote: > Yes, but there is another possible modification: replacement of call to > nonseekable_open() by a call to some other function that returns error. > Current code is already ready for such modification. The change of which function is called would change the behavior indeed, but, TBH, I do not see it as a valid point: If we assume that nonseekable_open() changes to something else in the future, we may assume as well that some other call will be added later with a risk of resource leaking. This is a thing, that whoever would do such changes should be careful about. For me, the code as it is now, is not uniform with the other places that use nonseekable_open().
On Tue, 16 Jul 2024 22:19:05 +0300 Nikita Kiryushin <kiryushin@ancud.ru> wrote: > On 7/16/24 12:45, Alexey Khoroshilov wrote: > > Yes, but there is another possible modification: replacement of call to > > nonseekable_open() by a call to some other function that returns error. > > Current code is already ready for such modification. > > The change of which function is called would change the behavior indeed, but, > TBH, I do not see it as a valid point: If we assume that nonseekable_open() changes to something else in the future, we may assume as well that some other call will be > added later with a risk of resource leaking. This is a thing, that whoever would do > such changes should be careful about. > > For me, the code as it is now, is not uniform with the other places that use > nonseekable_open(). The point is moot. If something returns a value, even if it says it will never return failure, there's no harm in checking it. If we ignore the return value, that is a unneeded coupling of design between the function and its users. It does no harm in checking the value, so I rather just keep doing so. -- Steve
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 578a49ff5c32..7e480501b509 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -7883,11 +7883,7 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) mutex_unlock(&trace_types_lock); - ret = nonseekable_open(inode, filp); - if (ret < 0) - trace_array_put(tr); - - return ret; + return nonseekable_open(inode, filp); } static __poll_t
There is a trace_array_put() in check result for nonseekable_open() in tracing_buffers_open(). However, it would be never executed as nonseekable_open never fails (by design). Remove the check and associated unreachable code. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 7b85af630348 ("tracing: Get trace_array ref counts when accessing trace files") Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru> --- kernel/trace/trace.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)