Re: [PATCH v2 3/3] staging: rtl8192u: fix rmmod warn when wlan0 is renamed

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Jul 18, 2022 at 10:50:38PM -0700, Tong Zhang wrote:
> This driver creates 4 debug files under [devname] folder. The devname
> could be wlan0 initially, however it could be renamed later to e.g.
> enx00e04c00000. This will cause problem during debug file teardown since
> it uses  netdev->name which is no longer wlan0. To solve this problem,
> add a notifier to handle device renaming.
> 
> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> Tested-by: Zheyu Ma <zheyuma97@gmail.com>
> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> ---
>  drivers/staging/rtl8192u/r8192U.h         |  1 +
>  drivers/staging/rtl8192u/r8192U_core.c    | 35 ++++++++++++++++++++++-
>  drivers/staging/rtl8192u/r8192U_debugfs.c | 13 +++++++++
>  3 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U.h b/drivers/staging/rtl8192u/r8192U.h
> index e8860bb2b607..ccce37b7e2ae 100644
> --- a/drivers/staging/rtl8192u/r8192U.h
> +++ b/drivers/staging/rtl8192u/r8192U.h
> @@ -1122,4 +1122,5 @@ void setKey(struct net_device *dev, u8 EntryNo, u8 KeyIndex, u16 KeyType,
>  
>  void rtl8192_debugfs_init(struct net_device *dev);
>  void rtl8192_debugfs_exit(struct net_device *dev);
> +void rtl8192_debugfs_rename(struct net_device *dev);
>  #endif
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index ac3716550505..5cc78c5bd706 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -4606,6 +4606,30 @@ static void rtl8192_usb_disconnect(struct usb_interface *intf)
>  	RT_TRACE(COMP_DOWN, "wlan driver removed\n");
>  }
>  
> +static int rtl8192_usb_netdev_event(struct notifier_block *nb, unsigned long event,
> +				    void *data)
> +{
> +	struct net_device *netdev = netdev_notifier_info_to_dev(data);
> +
> +	if (netdev->netdev_ops != &rtl8192_netdev_ops)
> +		goto out;
> +
> +	switch (event) {
> +	case NETDEV_CHANGENAME:
> +		rtl8192_debugfs_rename(netdev);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +out:
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block rtl8192_usb_netdev_notifier = {
> +	.notifier_call = rtl8192_usb_netdev_event,
> +};
> +
>  static int __init rtl8192_usb_module_init(void)
>  {
>  	int ret;
> @@ -4615,10 +4639,16 @@ static int __init rtl8192_usb_module_init(void)
>  	RT_TRACE(COMP_INIT, "Initializing module");
>  	RT_TRACE(COMP_INIT, "Wireless extensions version %d", WIRELESS_EXT);
>  
> +	ret = register_netdevice_notifier(&rtl8192_usb_netdev_notifier);
> +	if (ret) {
> +		pr_err("register_netdevice_notifier failed %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = ieee80211_debug_init();
>  	if (ret) {
>  		pr_err("ieee80211_debug_init() failed %d\n", ret);
> -		return ret;
> +		goto unregister_notifier;
>  	}
>  
>  	ret = ieee80211_crypto_init();
> @@ -4660,6 +4690,8 @@ static int __init rtl8192_usb_module_init(void)
>  	ieee80211_crypto_deinit();
>  debug_exit:
>  	ieee80211_debug_exit();
> +unregister_notifier:
> +	unregister_netdevice_notifier(&rtl8192_usb_netdev_notifier);
>  	return ret;
>  }
>  
> @@ -4671,6 +4703,7 @@ static void __exit rtl8192_usb_module_exit(void)
>  	ieee80211_crypto_tkip_exit();
>  	ieee80211_crypto_deinit();
>  	ieee80211_debug_exit();
> +	unregister_netdevice_notifier(&rtl8192_usb_netdev_notifier);
>  	RT_TRACE(COMP_DOWN, "Exiting");
>  }
>  
> diff --git a/drivers/staging/rtl8192u/r8192U_debugfs.c b/drivers/staging/rtl8192u/r8192U_debugfs.c
> index 5c9376e50889..c94f5dfac96b 100644
> --- a/drivers/staging/rtl8192u/r8192U_debugfs.c
> +++ b/drivers/staging/rtl8192u/r8192U_debugfs.c
> @@ -173,3 +173,16 @@ void rtl8192_debugfs_exit(struct net_device *dev)
>  	priv->debugfs_dir = NULL;
>  }
>  
> +void rtl8192_debugfs_rename(struct net_device *dev)
> +{
> +	struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);

No need to cast.

> +
> +	if (!priv->debugfs_dir)
> +		return;

Why does it matter?

> +
> +	if (!strcmp(priv->debugfs_dir->d_name.name, dev->name))

Ick, never poke around in a dentry from debugfs if you can help it.  You
know you are being renamed, why does it matter to check or not?

> +		return;
> +
> +	debugfs_rename(priv->debugfs_dir->d_parent, priv->debugfs_dir,
> +		       priv->debugfs_dir->d_parent, dev->name);

Don't poke around in the dentry for the parent here either.  That feels
racy and wrong.  Isn't there some other way to get the parent?

Also you can look up the dentry for this, no need to have it saved, like
I said in patch 2.

thanks,

greg k-h