On Thu, 2024-01-25 at 12:04 +0100, Alexis Lothoré wrote:
> On 1/25/24 07:23, Ajay.Kathat@xxxxxxxxxxxxx wrote:
> > On 1/24/24 11:45, David Mosberger-Tang wrote:
> >
> > > >
> > > > OK, I think I see what's going on: it's the list traversal. Here is what
> > > > wilc_netdev_cleanup() does:
> > > >
> > > > list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
> > > > if (vif->ndev)
> > > > unregister_netdev(vif->ndev);
> > > > }
> > > >
> > > > The problem is that "vif" is the private part of the netdev, so when the netdev
> > > > is freed, the vif structure is no longer valid and list_for_each_entry_rcu()
> > > > ends up dereferencing a dangling pointer.
>
> Your diagnostic sounds relevant :)
Yeah, it's definitely what's going on. And it's not just the list traversal:
afterwards, wilc_netdev_cleanup() continues to access the vif structure while
removing them from the vif-list.
I think the original idea of calling unregister_netdev() is probably the right
one as, like you said, you want to remove the device from being visible to the
user before tearing down anything else. If I understood the problem correctly,
the use-after-free caused by this line in wilc_netdev_ifc_init():
ndev->needs_free_netdev = true;
This causes unregister_netdev() to implicitly call free_netdev(). Without that
code, I think you could call unregister_netdev() early on (as it is right now)
and when done with using the vifs, call free_netdev() while avoiding any
dangling references.
In any case, this is definitely not my area of expertise. I don't fully
understand the motivition behind needs_free_netdev, even after reading
https://docs.kernel.org/networking/netdevices.html - I suspect the use of that
flag has evolved over the years and the docs may not be entirely up-to-date
anymore. One driver I looked at (wireless/ath/wil6210/netdev.c) sets
needs_free_netdev only for secondary vifs (i.e., all but the first vif).
Hopefully someone else on this list can figure out what the right solution is
here.
Thanks,
--david
