Return-path: Received: from mail-it0-f65.google.com ([209.85.214.65]:33405 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1033663AbdD0P6m (ORCPT ); Thu, 27 Apr 2017 11:58:42 -0400 Received: by mail-it0-f65.google.com with SMTP id z67so2619478itb.0 for ; Thu, 27 Apr 2017 08:58:42 -0700 (PDT) Subject: Re: [PATCH] staging: rtl8723bs: Revert ignoring_unreachable_code kfree To: Ian W MORRISON , Bastien Nocera References: <1493300827.2281.13.camel@hadess.net> <1493301943.2281.14.camel@hadess.net> Cc: linux-wireless@vger.kernel.org, jes.sorensen@gmail.com, Hans de Goede , gregkh@linuxfoundation.org From: Larry Finger Message-ID: <59070fb5-a5c0-a52f-fb47-2a5caeb9ea30@lwfinger.net> (sfid-20170427_175916_476197_69FCAC06) Date: Thu, 27 Apr 2017 10:58:39 -0500 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 04/27/2017 09:41 AM, Ian W MORRISON wrote: > Here's hoping this is what you want. If not let me know what info you'd like me > to provide. For this info I just built the r8723bs.ko module from linux-next > commit 6557ddf. > > Commit trace from 'git log --oneline --follow drivers/staging/rtl8723bs': > > d9c7dd5 staging: rtl8723bs: clean up identical code on an if statement > ff8d351 staging: rtl8723bs: remove redundant comparisons of unsigned ints with >= 0 > 2e85ccf staging: rtl8723bs: ensure cmd is large enough for %4s scanf format > b388285 staging: rtl8723bs: fix spelling mistake: "acquire" > 103ab68 staging: rtl8723bs: fix spelling mistakes in RT_TRACE messages > 1e7605e staging: rtl8723bs: force driver to be built as a module. > 7ad61a3 staging: rtl8723bs: remove null test before kfree > 9ddf6e3 staging: rtl8723bs: Add missing include to fix compile error > 6cb3d05 staging: rtl8723bs: core: rtw_cmd: drop unneeded null tests > f951e88 staging: rtl8723bs: core: rtw_cmd: drop unneeded null test > 1c21a34 staging: rtl8723bs: Fix indenting error in core/rtw_pwrctrl.c > bd51edb staging: rtl8723bs: Fix indenting problems in core/rtw_odm.c > ef9209b staging: rtl8723bs: Fix indenting errors and an off-by-one mistake in > core/rtw_mlme_ext.c > 3498d68 staging: rtl8723bs: Fix white-space errors in core/rtw_recv.c > 0ba8d4b staging: rtl8723bs: Fix some white-space errors in core/rtw_security.c > aa29d8b staging: rtl8723bs: Fix indenting problem in core/rtw_sta_mgt.c > 130cf72 staging: rtl8723bs: Fix some indenting problems and a potential data overrun > 5004922 staging: rtl8723bs: Fix indenting mistakes in core/rtw_mlme.c > b7126fa staging: rtl8723bs: Fix indenting mistakes in core/rtw_ieee80211.c > 9c63e98 staging: rtl8723bs: Fix indenting mistake in core/rtw_ap.c > 21a9509 staging: rtl8723bs: Fix possible usage of NULL pointer in core/rtw_debug.c > f195f7d staging: rtl8723bs: Fix indenting problems in core/rtw_xmit.c > 1793db1 staging: rtl8723bs: Fix indenting problem for hal/hal_com.c > 0952abb staging: rtl8723bs: Fix indening problem in hal/hal_com_phycfg.c > d9be201 staging: rtl8723bs: Fix indenting problems in hal/HalHWImg8723B_BB.c > 7e2b0aa staging: rtl8723bs: Fix potential usage while NULL error in > hal/rtl8723b_hal_init.c > 6557ddf staging: rtl8723bs: Fix various errors in os_dep/ioctl_cfg80211.c > e62bb92 staging: rtl8723bs: Fix indenting mistake in os_dep/mlme_linux.c > 72544c5f staging: rtl8723bs: Fix indenting warning in os_dep/os_intfs.c > 2679a3f staging: rtl8723bs: Fix dereference before check warning in > os_dep/recv_linux.c > c49c1f2 staging: rtl8723bs: Fix indenting warning in os_dep/rtw_proc.c > 9ad88c7 staging: rtl8723bs: Fix indenting warning in os_dep/xmit_linux.c > 554c0a3 staging: Add rtl8723bs sdio wifi driver > > Any build from commit from d9c7dd5 to 6557ddf fail but commit e62bb92 to 554c0a3 > work. Within commit 6557ddf with the fix for kfree() fails, without it works. > > Command line: > > root@ubuntu:~# cp r8723bs.ko-6557ddf r8723bs.ko > root@ubuntu:~# modinfo r8723bs.ko | grep depends > depends: cfg80211 > root@ubuntu:~# modprobe cfg80211 > root@ubuntu:~# insmod r8723bs.ko > Segmentation fault > root@ubuntu:~# > > Extract from tail of dmesg: > > [ 223.557686] r8723bs: loading out-of-tree module taints kernel. > [ 223.558971] r8723bs: module verification failed: signature and/or required > key missing - tainting kernel > [ 223.565310] RTL8723BS: module init start > [ 223.565318] RTL8723BS: rtl8723bs v4.3.5.5_12290.20140916_BTCOEX20140507-4E40 > [ 223.565322] RTL8723BS: rtl8723bs BT-Coex version = BTCOEX20140507-4E40 > [ 223.665558] pnetdev = ffff8eb19e872000 > [ 223.732589] RTL8723BS: rtw_ndev_init(wlan0) > [ 223.732662] ------------[ cut here ]------------ > [ 223.732741] kernel BUG at /home/kernel/COD/linux/net/wireless/core.h:108! > [ 223.732824] invalid opcode: 0000 [#1] SMP > [ 223.732873] Modules linked in: r8723bs(OE+) cfg80211 bnep axp20x_pek > axp288_charger axp288_fuel_gauge extcon_axp288 axp288_adc gpio_keys > intel_spi_platform intel_spi spi_nor mtd intel_rapl intel_soc_dts_thermal > intel_soc_dts_iosf intel_powerclamp coretemp kvm_intel kvm cdc_ether usbnet > irqbypass punit_atom_debug r8152 crct10dif_pclmul crc32_pclmul joydev mii > ghash_clmulni_intel snd_intel_sst_acpi input_leds pcbc aesni_intel > snd_soc_rt5670 snd_intel_sst_core aes_x86_64 snd_soc_rt5645 crypto_simd > snd_soc_sst_atom_hifi2_platform glue_helper snd_soc_rt5640 cryptd > snd_soc_sst_match bmc150_accel_spi bmc150_accel_i2c intel_cstate snd_seq_midi > snd_soc_rl6231 snd_soc_tlv320aic31xx bmc150_accel_core snd_soc_core > snd_seq_midi_event industrialio_triggered_buffer kfifo_buf axp20x_i2c hci_uart > axp20x snd_rawmidi > [ 223.733721] snd_hdmi_lpe_audio industrialio snd_compress ac97_bus > snd_pcm_dmaengine btbcm btqca snd_pcm btintel snd_seq snd_seq_device bluetooth > snd_timer mac_hid soc_button_array dw_dmac dw_dmac_core rfkill_gpio mei_txe snd > acpi_pad i2c_designware_platform mei soundcore i2c_designware_core lpc_ich > pwm_lpss_platform spi_pxa2xx_platform pwm_lpss 8250_dw parport_pc ppdev lp > parport ip_tables x_tables autofs4 isofs nls_iso8859_1 hid_logitech_hidpp > dm_mirror dm_region_hash dm_log hid_logitech_dj overlay hid_generic usbhid uas > usb_storage i915 i2c_algo_bit mmc_block drm_kms_helper syscopyarea sysfillrect > sysimgblt fb_sys_fops drm video i2c_hid hid sdhci_acpi sdhci > [ 223.734445] CPU: 2 PID: 3164 Comm: insmod Tainted: G OE > 4.11.0-041100rc8-generic #201704232131 > [ 223.734557] Hardware name: Intel Corporation STCK1A32WFC/STCK1A32WFC, BIOS > FCBYT10H.86A.0032.2016.0831.1658 08/31/2016 > [ 223.734680] task: ffff8eb168cf0000 task.stack: ffffad4781238000 > [ 223.734793] RIP: 0010:cfg80211_netdev_notifier_call+0x454/0x620 [cfg80211] > [ 223.734874] RSP: 0018:ffffad478123b928 EFLAGS: 00010246 > [ 223.734937] RAX: 0000000000000000 RBX: ffff8eb18b5a9000 RCX: 0000000000000000 > [ 223.735020] RDX: ffffad478123ba98 RSI: 0000000000000010 RDI: ffffffffc0b5d140 > [ 223.735103] RBP: ffffad478123ba18 R08: 0000000000000000 R09: ffffffffc0b5d140 > [ 223.735186] R10: ffffad478123ba28 R11: ffffad478123b938 R12: 0000000000000000 > [ 223.735269] R13: ffff8eb19e872000 R14: ffffad478123ba98 R15: 0000000000000000 > [ 223.735353] FS: 00007fb8b1b0b700(0000) GS:ffff8eb1b9f00000(0000) > knlGS:0000000000000000 > [ 223.735447] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 223.735515] CR2: 000056544b449498 CR3: 000000004b4fa000 CR4: 00000000001006e0 > [ 223.735597] Call Trace: > [ 223.735638] ? proc_alloc_inum+0x49/0xe0 > [ 223.735690] ? inetdev_event+0x47/0x4e0 > [ 223.735740] ? skb_dequeue+0x59/0x70 > [ 223.735786] notifier_call_chain+0x4a/0x70 > [ 223.735839] raw_notifier_call_chain+0x16/0x20 > [ 223.735895] call_netdevice_notifiers_info+0x35/0x60 > [ 223.735957] register_netdevice+0x1fa/0x500 > [ 223.736009] register_netdev+0x1a/0x30 > [ 223.736095] rtw_drv_register_netdev+0x65/0x90 [r8723bs] > [ 223.736187] rtw_drv_init+0x220/0x250 [r8723bs] > [ 223.736244] sdio_bus_probe+0xec/0x130 > [ 223.736293] driver_probe_device+0x2bb/0x460 > [ 223.736347] __driver_attach+0xdf/0xf0 > [ 223.736395] ? driver_probe_device+0x460/0x460 > [ 223.736451] bus_for_each_dev+0x6c/0xc0 > [ 223.736500] driver_attach+0x1e/0x20 > [ 223.736545] bus_add_driver+0x170/0x270 > [ 223.736595] driver_register+0x60/0xe0 > [ 223.736642] ? 0xffffffffc0c54000 > [ 223.736685] sdio_register_driver+0x20/0x30 > [ 223.736759] rtw_drv_entry+0x58/0x1000 [r8723bs] > [ 223.736816] ? 0xffffffffc0c54000 > [ 223.736860] do_one_initcall+0x52/0x1a0 > [ 223.736909] ? kmem_cache_alloc_trace+0xd7/0x190 > [ 223.736969] do_init_module+0x5f/0x200 > [ 223.737017] load_module+0x1a15/0x1da0 > [ 223.737068] ? ima_post_read_file+0x7e/0xa0 > [ 223.737121] ? security_kernel_post_read_file+0x6b/0x80 > [ 223.737187] SYSC_finit_module+0xdf/0x110 > [ 223.737237] ? SYSC_finit_module+0xdf/0x110 > [ 223.737290] SyS_finit_module+0xe/0x10 > [ 223.737338] entry_SYSCALL_64_fastpath+0x1e/0xad > [ 223.737394] RIP: 0033:0x7fb8b163edf9 > [ 223.737438] RSP: 002b:00007ffd134d7298 EFLAGS: 00000246 ORIG_RAX: > 0000000000000139 > [ 223.737528] RAX: ffffffffffffffda RBX: 00007fb8b18feb58 RCX: 00007fb8b163edf9 > [ 223.737610] RDX: 0000000000000000 RSI: 0000561ecd674f8b RDI: 0000000000000003 > [ 223.737694] RBP: 00007fb8b18feb00 R08: 0000000000000000 R09: 00007fb8b1900ea0 > [ 223.737778] R10: 0000000000000003 R11: 0000000000000246 R12: 00007fb8b18feb58 > [ 223.737862] R13: 0000000000001020 R14: 00007fb8b18feb58 R15: 000000000000270e > [ 223.737945] Code: fc ff ff 49 8b bc 24 90 fd ff ff e8 07 85 5b f8 89 c2 b8 85 > 80 00 00 84 d2 0f 85 3c fc ff ff e9 2d fc ff ff 31 c0 e9 30 fc ff ff <0f> 0b be > 4b 04 00 00 48 c7 c7 18 25 b5 c0 e8 e9 49 d8 f7 e9 f1 > [ 223.738253] RIP: cfg80211_netdev_notifier_call+0x454/0x620 [cfg80211] RSP: > ffffad478123b928 > [ 223.768427] ---[ end trace ff517da1ae586e56 ]--- > ubuntu@ubuntu:~$ > > OS: > Ubuntu 17.04 ISO from http://releases.ubuntu.com/17.04/ > > Kernel: > v4.11-rc8 from http://kernel.ubuntu.com/~kernel-ppa/mainline/v4.11-rc8/ > > > On 28 April 2017 at 00:05, Bastien Nocera > wrote: > > On Fri, 2017-04-28 at 00:00 +1000, Ian W MORRISON wrote: > > With the current code from linux-next an 'insmod r8723bs.ko' > > immediately results in segmentation fault. > > What's the error trace you get? > > > If I apply my 'patch' the insmod works and wifi is perfect. I didn't > > try removing the kfree() call altogether as I didn't feel I had > > enough understanding of possible implications to do so. > > > > On 27 April 2017 at 23:47, Bastien Nocera > wrote: > > > On Thu, 2017-04-27 at 23:44 +1000, Ian W MORRISON wrote: > > > > I tried building the RTL8723BS SDIO wifi driver which has just > > > been > > > > incorporated in staging for linux-next as an external module and > > > > found that it fails with 'Segmentation Fault'. I've tracked this > > > down > > > > to commit 6557ddfec348c13d7798ea9e44f11b6459f2f652 (staging: > > > > rtl8723bs: Fix various errors in os_dep/ioctl_cfg80211.c) which > > > > includes the fix > > > > 'drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:3547 > > > > rtw_wdev_alloc() info: ignoring unreachable code'. The moving of > > > line > > > > 'kfree((u8 *)wdev);' causes the segmentation error so I've > > > included > > > > the following patch to revert it. > > > > > > What error? Both versions look equally incorrect, eg. either it's > > > leaking data in which case the kfree() is necessary and should be > > > in a > > > callable location, or it's not, and the call should be removed. First of all, please do not top post when you send E-mail to a list. When trying to find a bad commit, you should use bisection. You will save a lot of trials. As Bastien says above, both of your commits are wrong. Your experiments appear to show that having that kfree() call there is wrong, thus the fix is to remove it, NOT move it to a place where it can never be called. As I do not have working hardware for this chip, I cannot test any of these patches. In retrospect, I should not have made the change in question, but it now seems that a patch removing the kfree() call is the right one. When the code eventually calls rtw_wdev_free(), the memory allocated to wdev will be freed. In any case, someone with the hardware should enable kmemleak in their kernel and test this driver for memory leaks. Please send a patch with the offending kfree() call removed. Larry