| Message ID | 20220408092803.3188-1-h0tc0d3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: Fix NULL pointer dereference | expand |
Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? If not, there's no point in adding NULL checks.
On Fri, Apr 8, 2022 at 12:01 PM Simon Ser <contact@emersion.fr> wrote: > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > If not, there's no point in adding NULL checks. It isn't called with NULL anywhere, the NULL checks that already exist seem redundant.
On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > > If not, there's no point in adding NULL checks. > > It isn't called with NULL anywhere, the NULL checks that already exist > seem redundant. Grigory, would be be willing to submit a v2 which removes the unnecessary NULL checks?
Simon Ser and Bas Nieuwenhuizen, do you understand that you are proposing to make the code less safe in the future? In the future, someone might rewrite the code and we'll get an error. пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: > > On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > > > On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > > > > > Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > > > If not, there's no point in adding NULL checks. > > > > It isn't called with NULL anywhere, the NULL checks that already exist > > seem redundant. > > Grigory, would be be willing to submit a v2 which removes the unnecessary > NULL checks?
On Friday, April 8th, 2022 at 15:21, Grigory Vasilyev <h0tc0d3@gmail.com> wrote: > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > proposing to make the code less safe in the future? In the future, > someone might rewrite the code and we'll get an error. I don't think we should blindly add NULL checks for all functions which take a pointer as argument. This makes it way more complicated to find a bug when the function is mis-used. Crashing is better because it indicates a programmer error. In the future, any new call with a NULL pointer will produce a clear error. Using pointers for output values is a common pattern in C, it allows a function to return multiple values.
Am 08.04.22 um 15:21 schrieb Grigory Vasilyev: > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > proposing to make the code less safe in the future? In the future, > someone might rewrite the code and we'll get an error. Which is perfectly fine. See error handling is to handle userspace or hardware errors and *not* coding errors. Testing all pointers for NULL without any reason is not defensive, but rather the exactly opposite since it helps hiding real bugs. Regards, Christian. > > пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: >> On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: >> >>> On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: >>> >>>> Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? >>>> If not, there's no point in adding NULL checks. >>> It isn't called with NULL anywhere, the NULL checks that already exist >>> seem redundant. >> Grigory, would be be willing to submit a v2 which removes the unnecessary >> NULL checks?
Christian König, Simon Ser In fact, the code looks strange, we return the return code, but for some reason we also write false and 0. In my opinion, the caller should do this. Of course, you are right, but I look from the position that nothing should fall in the user system. There may be strange errors that occur periodically under certain conditions and do not affect developers. Also, the user will not always be able to correctly write a bug report. Regards, Grigory. пт, 8 апр. 2022 г. в 17:30, Christian König <christian.koenig@amd.com>: > > Am 08.04.22 um 15:21 schrieb Grigory Vasilyev: > > Simon Ser and Bas Nieuwenhuizen, do you understand that you are > > proposing to make the code less safe in the future? In the future, > > someone might rewrite the code and we'll get an error. > > Which is perfectly fine. > > See error handling is to handle userspace or hardware errors and *not* > coding errors. > > Testing all pointers for NULL without any reason is not defensive, but > rather the exactly opposite since it helps hiding real bugs. > > Regards, > Christian. > > > > > пт, 8 апр. 2022 г. в 14:48, Simon Ser <contact@emersion.fr>: > >> On Friday, April 8th, 2022 at 13:28, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > >> > >>> On Fri, Apr 8, 2022 at 12:01 PM Simon Ser contact@emersion.fr wrote: > >>> > >>>> Is amdgpu_display_get_fb_info ever called with NULL tiling_flags/tmz_surface? > >>>> If not, there's no point in adding NULL checks. > >>> It isn't called with NULL anywhere, the NULL checks that already exist > >>> seem redundant. > >> Grigory, would be be willing to submit a v2 which removes the unnecessary > >> NULL checks? >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c index d26810e7311d..c773a92dd4e5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c @@ -1017,8 +1017,10 @@ static int amdgpu_display_get_fb_info(const struct amdgpu_framebuffer *amdgpu_fb int r; if (!amdgpu_fb) { - *tiling_flags = 0; - *tmz_surface = false; + if (tiling_flags) + *tiling_flags = 0; + if (tmz_surface) + *tmz_surface = false; return 0; }
The code below check for NULL, but is no check at this place, which is potentially dangerous. Signed-off-by: Grigory Vasilyev <h0tc0d3@gmail.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)