Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 21 Feb 2022 16:00:06 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Tong Zhang <ztong0001@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        Colin Ian King <colin.i.king@googlemail.com>,
        Saurav Girepunje <saurav.girepunje@gmail.com>,
        Johan Hovold <johan@kernel.org>,
        =?iso-8859-1?Q?Cl=E1udio?= Maia <clrrm@isep.ipp.pt>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: rtl8192u: cleanup proc fs entries upon exit
Message-ID: <20220221130006.GJ3965@kadam>
References: <20220220231554.2510567-1-ztong0001@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220220231554.2510567-1-ztong0001@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?y+zo1mcGdDw0DHW1tJBQqv60/vsKmNoUUjWjXcYYD/dEJptpGWd6XI4VkMfV?=
 =?us-ascii?Q?naF91zA5mlFKC5YD+rm8lOv+0cU4SSqJ4su+FvJdY8sr107FeiYx0ZAP+7bL?=
 =?us-ascii?Q?vfMBxjMBdjDIJoRHVPbUXND0xdxmp630NHXtNRPmpBRGbp5jL46zd6xWmE9F?=
 =?us-ascii?Q?d1UmQnMlEHre4vshk/JdhBXAegLjkdIBrsH/cWvOzR/hwlj8KYghIK2YL0on?=
 =?us-ascii?Q?4i+YcINk3ZScJEa/xNSWQCoK58p9WZ7wyoE5WSAX/0AntXFR7/mbDCD2sroH?=
 =?us-ascii?Q?MjU9GQM1iFJd0J1KUnCY5cP2IwlLz4UDgpjnncb+WxWX90ZSz25C6Zu5LiQs?=
 =?us-ascii?Q?si0e99Qd2+tXZ73thwwmsOr7bMlaNlahfyXRJWtD242YDqCS34Fz5U1VSQEh?=
 =?us-ascii?Q?XSmDju9Vr7O2oTwS7dn06tsMxrCi9fXJSl9sZeg6I0odMb7aO7gbXxSV92PS?=
 =?us-ascii?Q?NDBpnXqzCpDMR88WG6vQR2hmLfGJRdbJYhXwTau6gB4qLs+h7Dws1TMEJim4?=
 =?us-ascii?Q?jXoSu1YQncioB1Baq7yEOPEY+9Bje5/lxr6aNUcB0BDnJdU95DecavdedzxL?=
 =?us-ascii?Q?b72MR4crY9R/olkVd/iVzRkw+9CDviVXzc4hS7EYiQYOdCyK9dxpZ6LS4W6d?=
 =?us-ascii?Q?9FeEnzSZ8+gWmcL2RHcuMg80cFEUyIGdsym4mbkxRrKDOqWH+sANpkjoBWW9?=
 =?us-ascii?Q?KIvOsqV40PpZ9f7arCLrCRcHRe8c3wU5PG32TDnNPDgiahnIdNTvko11usz+?=
 =?us-ascii?Q?cLDPYC98ZlmcpO6J6cn756iRIXUdT9RXDChlY9ghSJ8HGXVEa8oXpR3jPqJl?=
 =?us-ascii?Q?9MWjtJrx7CS8t/UlQo5erZmu7+V0TIuRL5pHFhucXcTvp+JpcSsr99sUmtOY?=
 =?us-ascii?Q?y4o4XgFjbahP80Uw0PE8Z3GRQIWxDAzlL2oC8iQ8M5OCCdVIhcFjAkzjKriU?=
 =?us-ascii?Q?RYTpjV6MQUVivkN///U7ThDXv8VBzB85LEZtMXKE/p8/igv5CBytErhGkIsY?=
 =?us-ascii?Q?3ahBwQOxDaTxXg984kcE+HoEIdcoXkEjB6j7a0sHD6JDUgp7STFbvqYEWj1u?=
 =?us-ascii?Q?pt/Z4iH+88u/dr35mMvV8LsAxA+HjThmOTByU6UfOAvyLLbkXq6rWv7sPamI?=
 =?us-ascii?Q?5iTN747jbapN4wByVw/0vHMxqYeCVEMIbK5xLl07zji6UbDltlmSwwGeEFrU?=
 =?us-ascii?Q?0VNkypJPVomRRP72Uo9UzDVZLF+CnAFEwUABYPIOZ62yoHUV0x4ZS4E2rYNU?=
 =?us-ascii?Q?09SKdkzlP+chhlfKK/0DN14X7pZrvFeOuGrKJThhmlW1LANioZohkNuoAuWL?=
 =?us-ascii?Q?ziZAxnTiyAmv0r7gqH/y02/jLVPYNNUCWgG2nDsJGQimjDCtsAD34GdRb42s?=
 =?us-ascii?Q?OuHe4fzt2clLP6o7helpweGV6LaTe/QZGMba/CJmQlw/BtYcnUl93i4fZO7T?=
 =?us-ascii?Q?3E2c6uUp8roEROmUV5Wqvt4ZySNRwPCnVgoLKxe4xVa9msGb8u0pArODMwKh?=
 =?us-ascii?Q?NYZ2mKEKWLEr/tVLX2NQXqg1MOeBNDB1DnrL7/piU/1iBPZ/BXtzZNHEH3ej?=
 =?us-ascii?Q?zWkZ+usQfCHz7gfUbhCTOGJdC8qg51AmaLbR9nKVvcIrQ7iPrvKZYJUt6UW4?=
 =?us-ascii?Q?jL0zCgkp37cnumvmFHo8lNORIBpfkUfKEr/Yjdfy/qEqUZUd4B9cupEcdUPL?=
 =?us-ascii?Q?dXFmUQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ecd0730e-b244-4d35-f634-08d9f53a213e
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2022 13:00:26.7287
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fxQloQvIahKXLXKiRjL2Lny0VaaGEdOnW7XeA0cKgNZpCbLe7KG7g/U8ROpaOjhHYZBgTfRIvWVVvjjLgNg512LtRobQst1vshFzLw8F5ho=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3095
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10264 signatures=677614
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 spamscore=0
 mlxscore=0 adultscore=0 mlxlogscore=957 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202210078
X-Proofpoint-ORIG-GUID: ubxQXIggvLE23npSL3jTglbAb3yV110v
X-Proofpoint-GUID: ubxQXIggvLE23npSL3jTglbAb3yV110v
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@gmail.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