Re: [PATCH 5.5 066/189] driver core: Call sync_state() even if supplier has no consumers

[Jiri Slaby]

On 10. 03. 20, 13:38, Greg Kroah-Hartman wrote:
> From: Saravana Kannan <saravanak@xxxxxxxxxx>
> 
> commit 21eb93f432b1a785df193df1a56a59e9eb3a985f upstream.
> 
> The initial patch that added sync_state() support didn't handle the case
> where a supplier has no consumers. This was because when a device is
> successfully bound with a driver, only its suppliers were checked to see
> if they are eligible to get a sync_state(). This is not sufficient for
> devices that have no consumers but still need to do device state clean
> up. So fix this.
> 
> Fixes: fc5a251d0fd7ca90 (driver core: Add sync_state driver/bus callback)

This causes NULL ptr dereferences (in 5.5 only). It is enough to load
the mac80211_hwsim module.

The backport to 5.5 needs at least these two commits:
commit ac338acf514e7b578fa9e3742ec2c292323b4c1a
Author: Saravana Kannan <saravanak@xxxxxxxxxx>
Date:   Fri Feb 21 00:05:09 2020 -0800

    driver core: Add dev_has_sync_state()

commit 77036165d8bcf7c7b2a2df28a601ec2c52bb172d
Author: Saravana Kannan <saravanak@xxxxxxxxxx>
Date:   Fri Feb 21 00:05:10 2020 -0800

    driver core: Skip unnecessary work when device doesn't have sync_state()


and playing with includes.

I am not sure if a revert wouldn't be better -- leaving up to maintainers.

https://bugzilla.suse.com/show_bug.cgi?id=1167245

> BUG: kernel NULL pointer dereference, address: 0000000000000048
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 0 PID: 2433 Comm: modprobe Not tainted 5.5.9-1-default #1
openSUSE Tumbleweed (unreleased)
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.12.0-0-ga698c89-rebuilt.opensuse.org 04/01/2014
> RIP: 0010:device_links_flush_sync_list+0xa7/0xe0
> Code: 48 89 4a 08 48 89 11 48 89 85 d0 00 00 00 48 89 85 d8 00 00 00
49 39 ec 74 0c 48 8d bd 80 00 00 00 e8 ad 5a 2c 00 48 8b 45 60 <48> 8b
40 48 48 85 c0 75 80 48 8b 45 68 48 85 c0 0f 84 7b ff ff ff
> RSP: 0018:ffffa55dc2803b40 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffffa55dc2803a98 RCX: ffffa55dc2803b68
> RDX: ffffa55dc2803b68 RSI: ffff90831c64e800 RDI: ffffa55dc2803b68
> RBP: ffff90831c64e800 R08: 0000000000000000 R09: 0000000000000228
> R10: 0000000000000dc0 R11: 0000000001320122 R12: ffff90831c64e800
> R13: ffffa55dc2803b68 R14: ffffffffa6f20080 R15: 0000000000000000
> FS:  00007f252bf63740(0000) GS:ffff90831e400000(0000)
knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000048 CR3: 00000000123ec000 CR4: 00000000000006f0
> Call Trace:
>  device_links_driver_bound+0x194/0x220
>  driver_bound+0x4c/0xe0
>  device_bind_driver+0x4d/0x60
>  mac80211_hwsim_new_radio+0x14a/0xdc0 [mac80211_hwsim]
>  ? __class_register+0x10c/0x170
>  ? 0xffffffffc092c000
>  init_mac80211_hwsim+0x26f/0x1000 [mac80211_hwsim]
>  ? 0xffffffffc092c000
>  do_one_initcall+0x46/0x200
>  ? _cond_resched+0x15/0x30
>  ? kmem_cache_alloc_trace+0x189/0x280
>  ? do_init_module+0x23/0x230
>  do_init_module+0x5c/0x230
>  load_module+0x14b2/0x1650
>  ? __do_sys_init_module+0x16e/0x1a0
>  __do_sys_init_module+0x16e/0x1a0
>  do_syscall_64+0x64/0x240
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f252c08ed9a
> Code: 48 8b 0d f9 f0 0b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f
84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 af 00 00 00 0f 05 <48> 3d
01 f0 ff ff 73 01 c3 48 8b 0d c6 f0 0b 00 f7 d8 64 89 01 48
> RSP: 002b:00007ffd0bc12378 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
> RAX: ffffffffffffffda RBX: 0000559358949ee0 RCX: 00007f252c08ed9a
> RDX: 000055935894a750 RSI: 000000000002180b RDI: 00007f2527cc8010
> RBP: 00007f2527cc8010 R08: 0000000000000000 R09: 00007f252c4559e0
> R10: 0000000000000001 R11: 0000000000000246 R12: 000055935894a750
> R13: 0000000000000000 R14: 0000559358949f80 R15: 0000559358949ee0
> Modules linked in: mac80211_hwsim(+) mac80211 cfg80211 libarc4
nls_utf8 isofs fuse af_packet rfkill xt_tcpudp ip6t_REJECT
nf_reject_ipv6 ip6t_rpfilter ipt_REJECT nf_reject_ipv4 xt_conntrack
ebtable_nat ebtable_broute ip6table_nat ip6table_mangle ip6table_raw
ip6table_security iptable_nat nf_nat iptable_mangle iptable_raw
iptable_security nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set
nfnetlink ebtable_filter ebtables scsi_transport_iscsi ip6table_filter
ip6_tables iptable_filter ip_tables x_tables bpfilter ppdev
snd_hda_codec_generic ledtrig_audio bochs_drm drm_vram_helper
drm_ttm_helper ttm drm_kms_helper snd_hda_intel snd_intel_dspcfg
snd_hda_codec drm snd_hda_core joydev snd_hwdep pcspkr snd_pcm
parport_pc snd_timer snd parport fb_sys_fops syscopyarea sysfillrect
soundcore sysimgblt i2c_piix4 button hid_generic usbhid btrfs
blake2b_generic libcrc32c xor ehci_pci ata_generic raid6_pq ehci_hcd
sr_mod cdrom usbcore ata_piix virtio_net virtio_blk serio_raw floppy
virtio_scsi
>  net_failover failover qemu_fw_cfg sg dm_multipath dm_mod scsi_dh_rdac
scsi_dh_emc scsi_dh_alua
> CR2: 0000000000000048
> ---[ end trace 085626c17d7c1908 ]---



> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20200221080510.197337-2-saravanak@xxxxxxxxxx
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> 
> ---
>  drivers/base/core.c |   23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -745,25 +745,31 @@ static void __device_links_queue_sync_st
>  /**
>   * device_links_flush_sync_list - Call sync_state() on a list of devices
>   * @list: List of devices to call sync_state() on
> + * @dont_lock_dev: Device for which lock is already held by the caller
>   *
>   * Calls sync_state() on all the devices that have been queued for it. This
> - * function is used in conjunction with __device_links_queue_sync_state().
> + * function is used in conjunction with __device_links_queue_sync_state(). The
> + * @dont_lock_dev parameter is useful when this function is called from a
> + * context where a device lock is already held.
>   */
> -static void device_links_flush_sync_list(struct list_head *list)
> +static void device_links_flush_sync_list(struct list_head *list,
> +					 struct device *dont_lock_dev)
>  {
>  	struct device *dev, *tmp;
>  
>  	list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
>  		list_del_init(&dev->links.defer_sync);
>  
> -		device_lock(dev);
> +		if (dev != dont_lock_dev)
> +			device_lock(dev);
>  
>  		if (dev->bus->sync_state)
>  			dev->bus->sync_state(dev);
>  		else if (dev->driver && dev->driver->sync_state)
>  			dev->driver->sync_state(dev);
>  
> -		device_unlock(dev);
> +		if (dev != dont_lock_dev)
> +			device_unlock(dev);
>  
>  		put_device(dev);
>  	}
> @@ -801,7 +807,7 @@ void device_links_supplier_sync_state_re
>  out:
>  	device_links_write_unlock();
>  
> -	device_links_flush_sync_list(&sync_list);
> +	device_links_flush_sync_list(&sync_list, NULL);
>  }
>  
>  static int sync_state_resume_initcall(void)
> @@ -865,6 +871,11 @@ void device_links_driver_bound(struct de
>  			driver_deferred_probe_add(link->consumer);
>  	}
>  
> +	if (defer_sync_state_count)
> +		__device_links_supplier_defer_sync(dev);
> +	else
> +		__device_links_queue_sync_state(dev, &sync_list);
> +
>  	list_for_each_entry(link, &dev->links.suppliers, c_node) {
>  		if (!(link->flags & DL_FLAG_MANAGED))
>  			continue;
> @@ -883,7 +894,7 @@ void device_links_driver_bound(struct de
>  
>  	device_links_write_unlock();
>  
> -	device_links_flush_sync_list(&sync_list);
> +	device_links_flush_sync_list(&sync_list, dev);
>  }
>  
>  static void device_link_drop_managed(struct device_link *link)
> 
> 

thanks,
-- 
js
suse labs