linux-kernel - Re: drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874jovotu3.wl-tiwai@suse.de>
Date:   Tue, 02 May 2023 13:28:52 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     oe-kbuild@...ts.linux.dev, Takashi Iwai <tiwai@...e.de>,
        lkp@...el.com, oe-kbuild-all@...ts.linux.dev,
        linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>
Subject: Re: drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?

On Tue, 02 May 2023 13:19:04 +0200,
Dan Carpenter wrote:
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   2caeeb9d4a1bccd923b7918427f9e9ef7151ddd8
> commit: c5c354a3a4728045e1342166394c615d75d45377 drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
> config: parisc-randconfig-m031-20230421 (https://download.01.org/0day-ci/archive/20230423/202304230801.ncoG1XDr-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 12.1.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@...el.com>
> | Reported-by: Dan Carpenter <error27@...il.com>
> | Link: https://lore.kernel.org/r/202304230801.ncoG1XDr-lkp@intel.com/
> 
> smatch warnings:
> drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked() warn: can 'unode' even be NULL?
> 
> vim +/unode +259 drivers/gpu/drm/udl/udl_main.c
> 
> c5c354a3a47280 Takashi Iwai 2022-09-08  236  static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
> 5320918b9a8786 Dave Airlie  2010-12-15  237  {
> c5c354a3a47280 Takashi Iwai 2022-09-08  238  	struct urb_node *unode;
> 5320918b9a8786 Dave Airlie  2010-12-15  239  
> c5c354a3a47280 Takashi Iwai 2022-09-08  240  	assert_spin_locked(&udl->urbs.lock);
> 5320918b9a8786 Dave Airlie  2010-12-15  241  
> acd45c56790a3b Takashi Iwai 2022-08-04  242  	/* Wait for an in-flight buffer to complete and get re-queued */
> acd45c56790a3b Takashi Iwai 2022-08-04  243  	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> c5c354a3a47280 Takashi Iwai 2022-09-08  244  					 !udl->urbs.count ||
> acd45c56790a3b Takashi Iwai 2022-08-04  245  					 !list_empty(&udl->urbs.list),
> acd45c56790a3b Takashi Iwai 2022-08-04  246  					 udl->urbs.lock, timeout)) {
> acd45c56790a3b Takashi Iwai 2022-08-04  247  		DRM_INFO("wait for urb interrupted: available: %d\n",
> acd45c56790a3b Takashi Iwai 2022-08-04  248  			 udl->urbs.available);
> c5c354a3a47280 Takashi Iwai 2022-09-08  249  		return NULL;
> acd45c56790a3b Takashi Iwai 2022-08-04  250  	}
> 5320918b9a8786 Dave Airlie  2010-12-15  251  
> c5c354a3a47280 Takashi Iwai 2022-09-08  252  	if (!udl->urbs.count)
> c5c354a3a47280 Takashi Iwai 2022-09-08  253  		return NULL;
> c5c354a3a47280 Takashi Iwai 2022-09-08  254  
> acd45c56790a3b Takashi Iwai 2022-08-04  255  	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
> 
> Use list_first_entry_or_null() if this list can be empty.
> 
> ed9605a66b62f2 Takashi Iwai 2022-09-08  256  	list_del_init(&unode->entry);
>                                                                ^^^^^^^^^^^^
> dereference.
> 
> 5320918b9a8786 Dave Airlie  2010-12-15  257  	udl->urbs.available--;
> 5320918b9a8786 Dave Airlie  2010-12-15  258  
> acd45c56790a3b Takashi Iwai 2022-08-04 @259  	return unode ? unode->urb : NULL;
> 
> Returns from list_first_entry() should never be checked for NULL.
> 
> 5320918b9a8786 Dave Airlie  2010-12-15  260  }

This is rather a leftover at commit c5c354a3a472 ("drm/udl: Fix
inconsistent urbs.count value during udl_free_urb_list()").
The NULL check of unode at return became superfluous by the code
change there, hence it should be simply like

	return ucode->urb;

OTOH, the current code doesn't cause any practical problem.


thanks,

Takashi