Re: [PATCH] ipmi: fix crash on reading version from proc after unregisted bmc

[Corey Minyard]

Queued for the next release.  From looking at the code, this
was probably the original intent.

Thanks for fixing this.

-corey

On 09/27/2016 02:07 AM, Xie XiuQi wrote:
> I meet a crash, which could be reproduce:
> 1) while true; do cat /proc/ipmi/0/version; done
> 2) modprobe -rv ipmi_si ipmi_msghandler ipmi_devintf
>
> [82761.021137] IPMI BT: req2rsp=5 secs retries=2
> [82761.034524] ipmi device interface
> [82761.222218] ipmi_si ipmi_si.0: Found new BMC (man_id: 0x0007db, prod_id: 0x0001, dev_id: 0x01)
> [82761.222230] ipmi_si ipmi_si.0: IPMI bt interface initialized
> [82903.922740] BUG: unable to handle kernel NULL pointer dereference at 00000000000002d4
> [82903.930952] IP: [<ffffffffa030d9e8>] smi_version_proc_show+0x18/0x40 [ipmi_msghandler]
> [82903.939220] PGD 86693a067 PUD 865304067 PMD 0
> [82903.943893] Thread overran stack, or stack corrupted
> [82903.949034] Oops: 0000 [#1] SMP
> [82903.983091] Modules linked in: ipmi_si(-) ipmi_msghandler binfmt_misc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter
> ...
> [82904.057285]  pps_core scsi_transport_sas dm_mod vfio_iommu_type1 vfio xt_sctp nf_conntrack_proto_sctp nf_nat_proto_sctp
>                  nf_nat nf_conntrack sctp libcrc32c [last unloaded: ipmi_devintf]
> [82904.073169] CPU: 37 PID: 28089 Comm: cat Tainted: GF          O   ---- -------   3.10.0-327.28.3.el7.x86_64 #1
> [82904.083373] Hardware name: Huawei RH2288H V3/BC11HGSA0, BIOS 3.22 05/16/2016
> [82904.090592] task: ffff880101cc2e00 ti: ffff880369c54000 task.ti: ffff880369c54000
> [82904.098414] RIP: 0010:[<ffffffffa030d9e8>]  [<ffffffffa030d9e8>] smi_version_proc_show+0x18/0x40 [ipmi_msghandler]
> [82904.109124] RSP: 0018:ffff880369c57e70  EFLAGS: 00010203
> [82904.114608] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000024688470
> [82904.121912] RDX: fffffffffffffff4 RSI: ffffffffa0313404 RDI: ffff8808670ce200
> [82904.129218] RBP: ffff880369c57e70 R08: 0000000000019720 R09: ffffffff81204a27
> [82904.136521] R10: ffff88046f803300 R11: 0000000000000246 R12: ffff880662399700
> [82904.143828] R13: 0000000000000001 R14: ffff880369c57f48 R15: ffff8808670ce200
> [82904.151128] FS:  00007fb70c9ca740(0000) GS:ffff88086e340000(0000) knlGS:0000000000000000
> [82904.159557] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [82904.165473] CR2: 00000000000002d4 CR3: 0000000864c0c000 CR4: 00000000003407e0
> [82904.172778] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [82904.180084] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [82904.187385] Stack:
> [82904.189573]  ffff880369c57ee0 ffffffff81204f1a 00000000122a2427 0000000001426000
> [82904.197392]  ffff8808670ce238 0000000000010000 0000000000000000 0000000000000fff
> [82904.205198]  00000000122a2427 ffff880862079600 0000000001426000 ffff880369c57f48
> [82904.212962] Call Trace:
> [82904.219667]  [<ffffffff81204f1a>] seq_read+0xfa/0x3a0
> [82904.224893]  [<ffffffff8124ce2d>] proc_reg_read+0x3d/0x80
> [82904.230468]  [<ffffffff811e102c>] vfs_read+0x9c/0x170
> [82904.235689]  [<ffffffff811e1b7f>] SyS_read+0x7f/0xe0
> [82904.240816]  [<ffffffff81649209>] system_call_fastpath+0x16/0x1b
> [82904.246991] Code: 30 a0 e8 0c 6f ef e0 5b 5d c3 66 0f 1f 84 00 00 00 00 00 0f 1f
>                 44 00 00 48 8b 47 78 55 48 c7 c6 04 34 31 a0 48 89 e5 48 8b 40 50 <0f>
> 	       b6 90 d4 02 00 00 31 c0 89 d1 83 e2 0f c0 e9 04 0f b6 c9 e8
> [82904.267710] RIP  [<ffffffffa030d9e8>] smi_version_proc_show+0x18/0x40 [ipmi_msghandler]
> [82904.276079]  RSP <ffff880369c57e70>
> [82904.279734] CR2: 00000000000002d4
> [82904.283731] ---[ end trace a69e4328b49dd7c4 ]---
> [82904.328118] Kernel panic - not syncing: Fatal exception
>
> Reading versin from /proc need bmc device struct available. So in this patch
> we move add/remove_proc_entries between ipmi_bmc_register and ipmi_bmc_unregister.
>
> Cc: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> Signed-off-by: Xie XiuQi <xiexiuqi@xxxxxxxxxx>
> ---
>   drivers/char/ipmi/ipmi_msghandler.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index d8619998..fcdd886 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -2891,11 +2891,11 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers,
>   		intf->curr_channel = IPMI_MAX_CHANNELS;
>   	}
>   
> +	rv = ipmi_bmc_register(intf, i);
> +
>   	if (rv == 0)
>   		rv = add_proc_entries(intf, i);
>   
> -	rv = ipmi_bmc_register(intf, i);
> -
>    out:
>   	if (rv) {
>   		if (intf->proc_dir)
> @@ -2982,8 +2982,6 @@ int ipmi_unregister_smi(ipmi_smi_t intf)
>   	int intf_num = intf->intf_num;
>   	ipmi_user_t user;
>   
> -	ipmi_bmc_unregister(intf);
> -
>   	mutex_lock(&smi_watchers_mutex);
>   	mutex_lock(&ipmi_interfaces_mutex);
>   	intf->intf_num = -1;
> @@ -3007,6 +3005,7 @@ int ipmi_unregister_smi(ipmi_smi_t intf)
>   	mutex_unlock(&ipmi_interfaces_mutex);
>   
>   	remove_proc_entries(intf);
> +	ipmi_bmc_unregister(intf);
>   
>   	/*
>   	 * Call all the watcher interfaces to tell them that