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;
MIME-Version: 1.0
References: <20220718120149.GD2338@kadam> <20220719055047.322355-4-ztong0001@gmail.com>
 <YtamHKt7e/xqk+Jk@kroah.com>
In-Reply-To: <YtamHKt7e/xqk+Jk@kroah.com>
From:   Tong Zhang <ztong0001@gmail.com>
Date:   Tue, 19 Jul 2022 23:16:25 -0700
Message-ID: <CAA5qM4BC00XusFV3Upd8L0Z-m1gRW45HrZvVFLhQfjFp6EVxoA@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] staging: rtl8192u: fix rmmod warn when wlan0 is renamed
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        Colin Ian King <colin.king@intel.com>,
        Saurav Girepunje <saurav.girepunje@gmail.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Johan Hovold <johan@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        linux-staging@lists.linux.dev,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Zheyu Ma <zheyuma97@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 19, 2022 at 5:53 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> 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.

I think we need to save the old entry here otherwise we don't know the
original name  since dev->name has already been overwritten.

> thanks,
>
> greg k-h