mhm I see. Problem is that zero initializing things just to silence the warning is considered really bad practice.
Have you tried to use "goto out_suspend" instead of the "if(r)" at the end of the good case?
That might silence the compiler warning, but might look a bit better than initializing all the local variables.
Christian.
Am 01.12.21 um 13:48 schrieb Qingyang
Zhou:
Hi Christian:
The warning is from the kernel test robot, I quote it here. The key point is that vm may be used in radeon_vm_fini(rdev, vm) without initialization. Although it is not possible in practice, I still add initializations to silence the warning of llvm.
---------- Forwarded message ---------
From: kernel test robot <lkp@xxxxxxxxx>
Date: Wed, Dec 1, 2021 at 4:32 AM
Subject: drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable 'vm' is used uninitialized whenever 'if' condition is false
To: Zhou Qingyang <zhou1615@xxxxxxx>
Cc: <llvm@xxxxxxxxxxxxxxx>, <kbuild-all@xxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, 0day robot <lkp@xxxxxxxxx>
tree: https://github.com/0day-ci/linux/commits/UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
head: 123fb3d217e89c4388fdb08b63991bac7c324219
commit: 123fb3d217e89c4388fdb08b63991bac7c324219 drm/radeon/radeon_kms: Fix a NULL pointer dereference in radeon_driver_open_kms()
date: 5 hours ago
config: mips-randconfig-r014-20211128 (https://download.01.org/0day-ci/archive/20211201/202112010420.xkXHciHS-lkp@xxxxxxxxx/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/0day-ci/linux/commit/123fb3d217e89c4388fdb08b63991bac7c324219
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review UPDATE-20211130-233655/Zhou-Qingyang/drm-radeon-radeon_kms-Fix-a-NULL-pointer-dereference-in-radeon_driver_open_kms/20211130-231509
git checkout 123fb3d217e89c4388fdb08b63991bac7c324219
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/gpu/drm/radeon/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@xxxxxxxxx>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/radeon/radeon_kms.c:672:7: warning: variable 'vm' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (rdev->accel_working) {
^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use occurs here
radeon_vm_fini(rdev, vm);
^~
drivers/gpu/drm/radeon/radeon_kms.c:672:3: note: remove the 'if' if its condition is always true
if (rdev->accel_working) {
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/radeon/radeon_kms.c:664:6: warning: variable 'vm' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (rdev->family >= CHIP_CAYMAN) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/radeon/radeon_kms.c:705:23: note: uninitialized use occurs here
radeon_vm_fini(rdev, vm);
^~
drivers/gpu/drm/radeon/radeon_kms.c:664:2: note: remove the 'if' if its condition is always true
if (rdev->family >= CHIP_CAYMAN) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/radeon/radeon_kms.c:653:22: note: initialize the variable 'vm' to silence this warning
struct radeon_vm *vm;
^
= NULL
2 warnings generated.
vim +672 drivers/gpu/drm/radeon/radeon_kms.c
771fe6b912fca54 Jerome Glisse 2009-06-05 638
f482a1419545ded Alex Deucher 2012-07-17 639 /**
f482a1419545ded Alex Deucher 2012-07-17 640 * radeon_driver_open_kms - drm callback for open
f482a1419545ded Alex Deucher 2012-07-17 641 *
f482a1419545ded Alex Deucher 2012-07-17 642 * @dev: drm dev pointer
f482a1419545ded Alex Deucher 2012-07-17 643 * @file_priv: drm file
f482a1419545ded Alex Deucher 2012-07-17 644 *
f482a1419545ded Alex Deucher 2012-07-17 645 * On device open, init vm on cayman+ (all asics).
f482a1419545ded Alex Deucher 2012-07-17 646 * Returns 0 on success, error on failure.
f482a1419545ded Alex Deucher 2012-07-17 647 */
771fe6b912fca54 Jerome Glisse 2009-06-05 648 int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
771fe6b912fca54 Jerome Glisse 2009-06-05 649 {
721604a15b934f0 Jerome Glisse 2012-01-05 650 struct radeon_device *rdev = dev->dev_private;
10ebc0bc09344ab Dave Airlie 2012-09-17 651 int r;
123fb3d217e89c4 Zhou Qingyang 2021-11-30 652 struct radeon_fpriv *fpriv;
123fb3d217e89c4 Zhou Qingyang 2021-11-30 653 struct radeon_vm *vm;
721604a15b934f0 Jerome Glisse 2012-01-05 654
721604a15b934f0 Jerome Glisse 2012-01-05 655 file_priv->driver_priv = NULL;
721604a15b934f0 Jerome Glisse 2012-01-05 656
10ebc0bc09344ab Dave Airlie 2012-09-17 657 r = pm_runtime_get_sync(dev->dev);
9fb10671011143d Aditya Pakki 2020-06-13 658 if (r < 0) {
9fb10671011143d Aditya Pakki 2020-06-13 659 pm_runtime_put_autosuspend(dev->dev);
10ebc0bc09344ab Dave Airlie 2012-09-17 660 return r;
9fb10671011143d Aditya Pakki 2020-06-13 661 }
10ebc0bc09344ab Dave Airlie 2012-09-17 662
721604a15b934f0 Jerome Glisse 2012-01-05 663 /* new gpu have virtual address space support */
721604a15b934f0 Jerome Glisse 2012-01-05 664 if (rdev->family >= CHIP_CAYMAN) {
721604a15b934f0 Jerome Glisse 2012-01-05 665
721604a15b934f0 Jerome Glisse 2012-01-05 666 fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
721604a15b934f0 Jerome Glisse 2012-01-05 667 if (unlikely(!fpriv)) {
32c59dc14b72803 Alex Deucher 2016-08-31 668 r = -ENOMEM;
32c59dc14b72803 Alex Deucher 2016-08-31 669 goto out_suspend;
721604a15b934f0 Jerome Glisse 2012-01-05 670 }
721604a15b934f0 Jerome Glisse 2012-01-05 671
544143f9e01a60a Alex Deucher 2015-01-28 @672 if (rdev->accel_working) {
cc9e67e3d7000c1 Christian König 2014-07-18 673 vm = &fpriv->vm;
cc9e67e3d7000c1 Christian König 2014-07-18 674 r = radeon_vm_init(rdev, vm);
74073c9dd299056 Quentin Casasnovas 2014-03-18 675 if (r) {
123fb3d217e89c4 Zhou Qingyang 2021-11-30 676 goto out_fpriv;
74073c9dd299056 Quentin Casasnovas 2014-03-18 677 }
d72d43cfc5847c1 Christian König 2012-10-09 678
f1e3dc708aaadb9 Christian König 2014-02-20 679 r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
74073c9dd299056 Quentin Casasnovas 2014-03-18 680 if (r) {
123fb3d217e89c4 Zhou Qingyang 2021-11-30 681 goto out_vm_fini;
74073c9dd299056 Quentin Casasnovas 2014-03-18 682 }
f1e3dc708aaadb9 Christian König 2014-02-20 683
d72d43cfc5847c1 Christian König 2012-10-09 684 /* map the ib pool buffer read only into
d72d43cfc5847c1 Christian König 2012-10-09 685 * virtual address space */
cc9e67e3d7000c1 Christian König 2014-07-18 686 vm->ib_bo_va = radeon_vm_bo_add(rdev, vm,
d72d43cfc5847c1 Christian König 2012-10-09 687 rdev->ring_tmp_bo.bo);
123fb3d217e89c4 Zhou Qingyang 2021-11-30 688 if (!vm->ib_bo_va) {
123fb3d217e89c4 Zhou Qingyang 2021-11-30 689 r = -ENOMEM;
123fb3d217e89c4 Zhou Qingyang 2021-11-30 690 goto out_vm_fini;
123fb3d217e89c4 Zhou Qingyang 2021-11-30 691 }
123fb3d217e89c4 Zhou Qingyang 2021-11-30 692
cc9e67e3d7000c1 Christian König 2014-07-18 693 r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
cc9e67e3d7000c1 Christian König 2014-07-18 694 RADEON_VA_IB_OFFSET,
d72d43cfc5847c1 Christian König 2012-10-09 695 RADEON_VM_PAGE_READABLE |
d72d43cfc5847c1 Christian König 2012-10-09 696 RADEON_VM_PAGE_SNOOPED);
721604a15b934f0 Jerome Glisse 2012-01-05 697 if (r) {
123fb3d217e89c4 Zhou Qingyang 2021-11-30 698 goto out_vm_fini;
721604a15b934f0 Jerome Glisse 2012-01-05 699 }
24f47acc78b0ab5 Jérôme Glisse 2014-05-07 700 }
721604a15b934f0 Jerome Glisse 2012-01-05 701 file_priv->driver_priv = fpriv;
721604a15b934f0 Jerome Glisse 2012-01-05 702 }
10ebc0bc09344ab Dave Airlie 2012-09-17 703
123fb3d217e89c4 Zhou Qingyang 2021-11-30 704 out_vm_fini:
123fb3d217e89c4 Zhou Qingyang 2021-11-30 705 radeon_vm_fini(rdev, vm);
123fb3d217e89c4 Zhou Qingyang 2021-11-30 706 out_fpriv:
123fb3d217e89c4 Zhou Qingyang 2021-11-30 707 kfree(fpriv);
32c59dc14b72803 Alex Deucher 2016-08-31 708 out_suspend:
10ebc0bc09344ab Dave Airlie 2012-09-17 709 pm_runtime_mark_last_busy(dev->dev);
10ebc0bc09344ab Dave Airlie 2012-09-17 710 pm_runtime_put_autosuspend(dev->dev);
32c59dc14b72803 Alex Deucher 2016-08-31 711 return r;
771fe6b912fca54 Jerome Glisse 2009-06-05 712 }
771fe6b912fca54 Jerome Glisse 2009-06-05 713
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx
On Wed, Dec 1, 2021 at 3:20 PM Christian König <christian.koenig@xxxxxxx> wrote:
Am 01.12.21 um 04:22 schrieb Zhou Qingyang:
> In radeon_driver_open_kms(), radeon_vm_bo_add() is assigned to
> vm->ib_bo_va and passes and used in radeon_vm_bo_set_addr(). In
> radeon_vm_bo_set_addr(), there is a dereference of vm->ib_bo_va,
> which could lead to a NULL pointer dereference on failure of
> radeon_vm_bo_add().
>
> Fix this bug by adding a check of vm->ib_bo_va.
>
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
>
> Builds with CONFIG_DRM_RADEON=m show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: cc9e67e3d700 ("drm/radeon: fix VM IB handling")
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Signed-off-by: Zhou Qingyang <zhou1615@xxxxxxx>
> ---
> Changes in v2:
> - Initialize the variables to silence warning
What warning do you get? Double checking the code that shouldn't be
necessary and is usually rather frowned upon.
Thanks,
Christian.
>
> Changes in v3:
> - Fix the bug that good case will also be freed
> - Improve code style
>
> Changes in v2:
> - Improve the error handling into goto style
>
> drivers/gpu/drm/radeon/radeon_kms.c | 37 ++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 482fb0ae6cb5..9d0f840286a1 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -648,7 +648,9 @@ void radeon_driver_lastclose_kms(struct drm_device *dev)
> int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> {
> struct radeon_device *rdev = dev->dev_private;
> - int r;
> + struct radeon_fpriv *fpriv = NULL;
> + struct radeon_vm *vm = NULL;
> + int r = 0;
>
> file_priv->driver_priv = NULL;
>
> @@ -660,8 +662,6 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>
> /* new gpu have virtual address space support */
> if (rdev->family >= CHIP_CAYMAN) {
> - struct radeon_fpriv *fpriv;
> - struct radeon_vm *vm;
>
> fpriv = kzalloc(sizeof(*fpriv), GFP_KERNEL);
> if (unlikely(!fpriv)) {
> @@ -672,35 +672,38 @@ int radeon_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> if (rdev->accel_working) {
> vm = &fpriv->vm;
> r = radeon_vm_init(rdev, vm);
> - if (r) {
> - kfree(fpriv);
> - goto out_suspend;
> - }
> + if (r)
> + goto out_fpriv;
>
> r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false);
> - if (r) {
> - radeon_vm_fini(rdev, vm);
> - kfree(fpriv);
> - goto out_suspend;
> - }
> + if (r)
> + goto out_vm_fini;
>
> /* map the ib pool buffer read only into
> * virtual address space */
> vm->ib_bo_va = radeon_vm_bo_add(rdev, vm,
> rdev->ring_tmp_bo.bo);
> + if (!vm->ib_bo_va) {
> + r = -ENOMEM;
> + goto out_vm_fini;
> + }
> +
> r = radeon_vm_bo_set_addr(rdev, vm->ib_bo_va,
> RADEON_VA_IB_OFFSET,
> RADEON_VM_PAGE_READABLE |
> RADEON_VM_PAGE_SNOOPED);
> - if (r) {
> - radeon_vm_fini(rdev, vm);
> - kfree(fpriv);
> - goto out_suspend;
> - }
> + if (r)
> + goto out_vm_fini;
> }
> file_priv->driver_priv = fpriv;
> }
>
> +out_vm_fini:
> + if (r)
> + radeon_vm_fini(rdev, vm);
> +out_fpriv:
> + if (r)
> + kfree(fpriv);
> out_suspend:
> pm_runtime_mark_last_busy(dev->dev);
> pm_runtime_put_autosuspend(dev->dev);
