linux-kernel - Re: [PATCH] staging: rtl8192u: cleanup proc fs entries upon exit

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 21 Feb 2022 16:00:06 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Tong Zhang <ztong0001@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Colin Ian King <colin.i.king@...glemail.com>,
        Saurav Girepunje <saurav.girepunje@...il.com>,
        Johan Hovold <johan@...nel.org>,
        Cláudio Maia <clrrm@...p.ipp.pt>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8192u: cleanup proc fs entries upon exit

On Sun, Feb 20, 2022 at 03:15:53PM -0800, Tong Zhang wrote:
> proc fs entries need to be removed when module is removed, otherwise
> when we try to insert the module again, kernel will complain
> 
> [  493.068012] proc_dir_entry 'net/ieee80211' already registered
> [  493.271973]  proc_mkdir+0x18/0x20
> [  493.272136]  ieee80211_debug_init+0x28/0xde8 [r8192u_usb]
> [  493.272404]  rtl8192_usb_module_init+0x10/0x161 [r8192u_usb]
> 
> [   13.910616] proc_dir_entry 'net/rtl819xU' already registered
> [   13.918931]  proc_mkdir+0x18/0x20
> [   13.919098]  rtl8192_usb_module_init+0x142/0x16d [r8192u_usb]
> 
> Signed-off-by: Tong Zhang <ztong0001@...il.com>
> ---

This is a partial fix but there is a lot wrong with both the init() and
exit() function.  It's not hard to just fix everything and it saves
time.

Here is how to write Free the Last thing style error handling for init()
and when you finish writing the error handling code then the exit()
function is just a matter of cut and paste.

The rules are: 1) Free the last successful allocation.  2) Every
function must have a matching release function. 3) Every function must
clean up after itself.  No partial allocations.  4) Name your labels
with descriptive names to say what the goto does.

	ret = ieee80211_debug_init();
	if (ret) {

Here nothing has been allocated.  Nothing to clean up.  Just return an
error code.

		pr_err("ieee80211_debug_init() failed %d\n", ret);
		return ret;
	}

	ret = ieee80211_crypto_init();
	if (ret) {

The last successful allocation was ieee80211_debug_init().  So we need
to goto debug_exit;
		pr_err("ieee80211_crypto_init() failed %d\n", ret);
		goto debug_exit;
	}

	ret = ieee80211_crypto_tkip_init();
	if (ret) {
The last successful allocation was ieee80211_crypto_init() so we need to
goto crypto_exit;
		pr_err("ieee80211_crypto_tkip_init() failed %d\n", ret);
		goto crypto_exit;
	}

Etc.  At the end of the function it prints the pr_info() even though
usb_register(&rtl8192_usb_driver); can fail.  It needs to do:

	ret = usb_register(&rtl8192_usb_driver);
	if (ret)
		goto crypto_wep_exit;

	pr_info("\nLinux kernel driver for RTL8192 based WLAN cards\n");
	pr_info("Copyright (c) 2007-2008, Realsil Wlan\n");

	return 0;

crypto_wep_exit:
	ieee80211_crypto_wep_exit();
other_stuff:
	free_other_stuff();
crypto_exit:
	ieee80211_crypto_deinit();
debug_exit:
	ieee80211_debug_exit();

	return ret;
}

Now you copy and paste the cleanup block, delete the labels, and add a
usb_deregister() to create the rtl8192_usb_module_exit() function.

static void __exit rtl8192_usb_module_exit(void)
{
	usb_deregister(&rtl8192_usb_driver);
	ieee80211_crypto_wep_exit();
	free_other_stuff();
	ieee80211_crypto_deinit();
	ieee80211_debug_exit();
}

Remember to replace "free other stuff" with a the correct code.  ;)

regards,
dan carpenter