* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 2:59 ` Jakub Kicinski
@ 2025-04-16 14:40 ` Stanislav Fomichev
2025-04-17 0:15 ` Jakub Kicinski
2025-04-16 15:01 ` Taehee Yoo
2025-04-16 15:47 ` Mina Almasry
2 siblings, 1 reply; 18+ messages in thread
From: Stanislav Fomichev @ 2025-04-16 14:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Mina Almasry, Taehee Yoo, davem, pabeni, edumazet, andrew+netdev,
horms, asml.silence, dw, sdf, skhawaja, simona.vetter, kaiyuanz,
netdev
On 04/15, Jakub Kicinski wrote:
> On Tue, 15 Apr 2025 11:59:40 -0700 Stanislav Fomichev wrote:
> > > commit 42f342387841 ("net: fix use-after-free in the
> > > netdev_nl_sock_priv_destroy()") and rolling back a few fixes, it's
> > > really introduced by commit 1d22d3060b9b ("net: drop rtnl_lock for
> > > queue_mgmt operations").
> > >
> > > My first question, does this issue still reproduce if you remove the
> > > per netdev locking and go back to relying on rtnl_locking? Or do we
> > > crash somewhere else in net_devmem_unbind_dmabuf? If so, where?
> > > Looking through the rest of the unbinding code, it's not clear to me
> > > any of it actually uses dev, so it may just be the locking...
> >
> > A proper fix, most likely, will involve resetting binding->dev to NULL
> > when the device is going away.
>
> Right, tho a bit of work and tricky handling will be necessary to get
> that right. We're not holding a ref on binding->dev.
>
> I think we need to invert the socket mutex vs instance lock ordering.
> Make the priv mutex protect the binding->list and binding->dev.
> For that to work the binding needs to also store a pointer to its
> owning socket?
>
> Then in both uninstall paths (from socket and from netdev unreg) we can
> take the socket mutex, delete from list, clear the ->dev pointer,
> unlock, release the ref on the binding.
>
> The socket close path would probably need to lock the socket, look at
> the first entry, if entry has ->dev call netdev_hold(), release the
> socket, lock the netdev, lock the socket again, look at the ->dev, if
> NULL we raced - done. If not NULL release the socket, call unbind.
> netdev_put(). Restart this paragraph.
>
> I can't think of an easier way.
An alternative might be to have a new extra lock to just protect
the binding->bound_rxq? And we can move the netdev_lock/unlock inside
the xa_for_each loop in net_devmem_unbind_dmabuf. This will make sure
we don't touch the outdated 'dev'. But I think you're right, the same
lock ordering issue is gonna happen in this case as well.
> > Replacing rtnl with dev lock exposes the fact that we can't assume
> > that the binding->dev is still valid by the time we do unbind.
>
> Note that binding->dev is never accessed by net_devmem_unbind_dmabuf().
> So if the device was unregistered and its queues flushed, the only thing
> we touch the netdev pointer for is the instance lock :(
I was assuming that bound_rxq is also protected by the instance lock.
But as you were saying earlier, xa has its own lock, so I might
be wrong with that assumption..
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 14:40 ` Stanislav Fomichev
@ 2025-04-17 0:15 ` Jakub Kicinski
0 siblings, 0 replies; 18+ messages in thread
From: Jakub Kicinski @ 2025-04-17 0:15 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Mina Almasry, Taehee Yoo, davem, pabeni, edumazet, andrew+netdev,
horms, asml.silence, dw, sdf, skhawaja, simona.vetter, kaiyuanz,
netdev
On Wed, 16 Apr 2025 07:40:07 -0700 Stanislav Fomichev wrote:
> > The socket close path would probably need to lock the socket, look at
> > the first entry, if entry has ->dev call netdev_hold(), release the
> > socket, lock the netdev, lock the socket again, look at the ->dev, if
> > NULL we raced - done. If not NULL release the socket, call unbind.
> > netdev_put(). Restart this paragraph.
> >
> > I can't think of an easier way.
>
> An alternative might be to have a new extra lock to just protect
> the binding->bound_rxq? And we can move the netdev_lock/unlock inside
> the xa_for_each loop in net_devmem_unbind_dmabuf. This will make sure
> we don't touch the outdated 'dev'. But I think you're right, the same
> lock ordering issue is gonna happen in this case as well.
Yea, I was wondering about that but unless we're holding something that
prevents the netdev from going away - a lock or a ref - we'll just
circle back to the same race.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 2:59 ` Jakub Kicinski
2025-04-16 14:40 ` Stanislav Fomichev
@ 2025-04-16 15:01 ` Taehee Yoo
2025-04-17 0:35 ` Jakub Kicinski
2025-04-16 15:47 ` Mina Almasry
2 siblings, 1 reply; 18+ messages in thread
From: Taehee Yoo @ 2025-04-16 15:01 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, Mina Almasry, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Wed, Apr 16, 2025 at 11:59 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
Hi Mina, Stanislav and Jakub,
Thank you so much for the reviews!
> On Tue, 15 Apr 2025 11:59:40 -0700 Stanislav Fomichev wrote:
> > > commit 42f342387841 ("net: fix use-after-free in the
> > > netdev_nl_sock_priv_destroy()") and rolling back a few fixes, it's
> > > really introduced by commit 1d22d3060b9b ("net: drop rtnl_lock for
> > > queue_mgmt operations").
Yes, you're right.
The real fix would be commit 1d22d3060b9b
("net: drop rtnl_lock for queue_mgmt operations").
> > >
> > > My first question, does this issue still reproduce if you remove the
> > > per netdev locking and go back to relying on rtnl_locking? Or do we
> > > crash somewhere else in net_devmem_unbind_dmabuf? If so, where?
> > > Looking through the rest of the unbinding code, it's not clear to me
> > > any of it actually uses dev, so it may just be the locking...
Accessing binding->dev causes a crash after unreg.
I thought binding->dev is needed in the net_devmem_unbind_dmabuf(),
but it's not.
binding->dev is used only for locking, as you mentioned.
> >
> > A proper fix, most likely, will involve resetting binding->dev to NULL
> > when the device is going away.
>
> Right, tho a bit of work and tricky handling will be necessary to get
> that right. We're not holding a ref on binding->dev.
>
> I think we need to invert the socket mutex vs instance lock ordering.
> Make the priv mutex protect the binding->list and binding->dev.
> For that to work the binding needs to also store a pointer to its
> owning socket?
>
> Then in both uninstall paths (from socket and from netdev unreg) we can
> take the socket mutex, delete from list, clear the ->dev pointer,
> unlock, release the ref on the binding.
>
> The socket close path would probably need to lock the socket, look at
> the first entry, if entry has ->dev call netdev_hold(), release the
> socket, lock the netdev, lock the socket again, look at the ->dev, if
> NULL we raced - done. If not NULL release the socket, call unbind.
> netdev_put(). Restart this paragraph.
>
> I can't think of an easier way.
Thank you so much for a detailed guide :)
I tried what you suggested, then I tested cases A, B, and C.
I can't see any splats from lockdep, kasan, etc.
Also, I checked that bindings are released well by checking
/sys/kernel/debug/dma_buf/bufinfo.
I think this approach works well.
However, I tested this simply. So I'm not sure yet about race condition.
I need more tests targeting race condition.
I modified the locking order in the netdev_nl_bind_rx_doit().
And modified netdev_nl_sock_priv_destroy() code looks like:
void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
{
struct net_devmem_dmabuf_binding *binding;
struct net_devmem_dmabuf_binding *temp;
struct net_device *dev;
mutex_lock(&priv->lock);
list_for_each_entry_safe(binding, temp, &priv->bindings, list) {
dev = binding->dev;
if (dev) {
netdev_hold(dev, &priv->dev_tracker, GFP_KERNEL);
mutex_unlock(&priv->lock);
netdev_lock(dev);
mutex_lock(&priv->lock);
if (binding->dev)
net_devmem_unbind_dmabuf(binding);
mutex_unlock(&priv->lock);
netdev_unlock(dev);
netdev_put(dev, &priv->dev_tracker);
mutex_lock(&priv->lock);
}
}
mutex_unlock(&priv->lock);
}
Also modified the uninstall code looks like:
static void mp_dmabuf_devmem_uninstall(void *mp_priv,
struct netdev_rx_queue *rxq)
{
struct net_devmem_dmabuf_binding *binding = mp_priv;
struct netdev_rx_queue *bound_rxq;
unsigned long xa_idx;
mutex_lock(&binding->priv->lock);
xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
if (bound_rxq == rxq) {
xa_erase(&binding->bound_rxqs, xa_idx);
if (xa_empty(&binding->bound_rxqs)) {
list_del(&binding->list);
binding->dev = NULL;
net_devmem_dmabuf_binding_put(binding);
}
break;
}
}
mutex_unlock(&binding->priv->lock);
}
I think the uninstall code looks good to me, but
netdev_nl_sock_priv_destroy() is longer than I expected.
If this is okay with you, I would like to stabilize it with more tests.
>
> > Replacing rtnl with dev lock exposes the fact that we can't assume
> > that the binding->dev is still valid by the time we do unbind.
>
> Note that binding->dev is never accessed by net_devmem_unbind_dmabuf().
> So if the device was unregistered and its queues flushed, the only thing
> we touch the netdev pointer for is the instance lock :(
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 15:01 ` Taehee Yoo
@ 2025-04-17 0:35 ` Jakub Kicinski
2025-04-17 6:57 ` Taehee Yoo
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-04-17 0:35 UTC (permalink / raw)
To: Taehee Yoo
Cc: Stanislav Fomichev, Mina Almasry, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Thu, 17 Apr 2025 00:01:57 +0900 Taehee Yoo wrote:
> Thank you so much for a detailed guide :)
> I tried what you suggested, then I tested cases A, B, and C.
> I can't see any splats from lockdep, kasan, etc.
> Also, I checked that bindings are released well by checking
> /sys/kernel/debug/dma_buf/bufinfo.
> I think this approach works well.
> However, I tested this simply. So I'm not sure yet about race condition.
> I need more tests targeting race condition.
>
> I modified the locking order in the netdev_nl_bind_rx_doit().
> And modified netdev_nl_sock_priv_destroy() code looks like:
>
> void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
> {
> struct net_devmem_dmabuf_binding *binding;
> struct net_devmem_dmabuf_binding *temp;
> struct net_device *dev;
>
> mutex_lock(&priv->lock);
> list_for_each_entry_safe(binding, temp, &priv->bindings, list) {
Not sure you can "for each entry safe here. Since you drop the lock in
the loop what this helper saves as the "temp" / next struct may be
freed by the time we get to it. I think we need:
mutex_lock()
while (!list_empty())
binding = list_first..
> dev = binding->dev;
> if (dev) {
nit: flip the condition to avoid the indent
but I think the condition is too early, we should protect the pointer
itself with the same lock as the list. So if the entry is on the list
dev must not be NULL.
> netdev_hold(dev, &priv->dev_tracker, GFP_KERNEL);
I think you can declare the tracker on the stack, FWIW
> mutex_unlock(&priv->lock);
> netdev_lock(dev);
> mutex_lock(&priv->lock);
> if (binding->dev)
> net_devmem_unbind_dmabuf(binding);
Mina suggests that we should only release the ref from the socket side.
I guess that'd be good, it will prevent the binding itself from going
away. Either way you need to make sure you hold a ref on the binding.
Either by letting mp_dmabuf_devmem_uninstall() be as is, or taking
a new ref before you release the socket lock here.
> mutex_unlock(&priv->lock);
> netdev_unlock(dev);
> netdev_put(dev, &priv->dev_tracker);
> mutex_lock(&priv->lock);
> }
> }
> mutex_unlock(&priv->lock);
> }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-17 0:35 ` Jakub Kicinski
@ 2025-04-17 6:57 ` Taehee Yoo
2025-04-17 14:09 ` Jakub Kicinski
0 siblings, 1 reply; 18+ messages in thread
From: Taehee Yoo @ 2025-04-17 6:57 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, Mina Almasry, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Thu, Apr 17, 2025 at 9:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 17 Apr 2025 00:01:57 +0900 Taehee Yoo wrote:
> > Thank you so much for a detailed guide :)
> > I tried what you suggested, then I tested cases A, B, and C.
> > I can't see any splats from lockdep, kasan, etc.
> > Also, I checked that bindings are released well by checking
> > /sys/kernel/debug/dma_buf/bufinfo.
> > I think this approach works well.
> > However, I tested this simply. So I'm not sure yet about race condition.
> > I need more tests targeting race condition.
> >
> > I modified the locking order in the netdev_nl_bind_rx_doit().
> > And modified netdev_nl_sock_priv_destroy() code looks like:
> >
> > void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
> > {
> > struct net_devmem_dmabuf_binding *binding;
> > struct net_devmem_dmabuf_binding *temp;
> > struct net_device *dev;
> >
> > mutex_lock(&priv->lock);
> > list_for_each_entry_safe(binding, temp, &priv->bindings, list) {
>
> Not sure you can "for each entry safe here. Since you drop the lock in
> the loop what this helper saves as the "temp" / next struct may be
> freed by the time we get to it. I think we need:
>
> mutex_lock()
> while (!list_empty())
> binding = list_first..
>
> > dev = binding->dev;
> > if (dev) {
>
Thanks. I will try to use that you suggested.
> nit: flip the condition to avoid the indent
>
> but I think the condition is too early, we should protect the pointer
> itself with the same lock as the list. So if the entry is on the list
> dev must not be NULL.
Yes, I think it would be okay to remove this condition.
>
> > netdev_hold(dev, &priv->dev_tracker, GFP_KERNEL);
>
> I think you can declare the tracker on the stack, FWIW
Okay, I will use stack instead.
>
> > mutex_unlock(&priv->lock);
> > netdev_lock(dev);
> > mutex_lock(&priv->lock);
> > if (binding->dev)
> > net_devmem_unbind_dmabuf(binding);
>
> Mina suggests that we should only release the ref from the socket side.
> I guess that'd be good, it will prevent the binding itself from going
> away. Either way you need to make sure you hold a ref on the binding.
> Either by letting mp_dmabuf_devmem_uninstall() be as is, or taking
> a new ref before you release the socket lock here.
Thanks Mina for the suggestion!
What I would like to do is like that
If binding->dev is NULL, it skips locking, but it still keeps calling
net_devmem_unbind_dmabuf().
Calling net_devmem_unbind_dmabuf() is safe even if after module unload,
because binding->bound_rxq is deleted by the uninstall path.
If bound_rxq is empty, binding->dev will not be accessed.
The only uninstall side code change is to set binding->dev to NULL and
add priv->lock.
This approach was already suggested by Stanislav earlier in this thread.
This is based on an inverse locking order from
priv lock -> instance lock to instance lock -> priv lock.
Mina, Stanislav, and Jakub, can you confirm this?
>
> > mutex_unlock(&priv->lock);
> > netdev_unlock(dev);
> > netdev_put(dev, &priv->dev_tracker);
> > mutex_lock(&priv->lock);
> > }
> > }
> > mutex_unlock(&priv->lock);
> > }
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-17 6:57 ` Taehee Yoo
@ 2025-04-17 14:09 ` Jakub Kicinski
2025-04-18 10:46 ` Taehee Yoo
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-04-17 14:09 UTC (permalink / raw)
To: Taehee Yoo
Cc: Stanislav Fomichev, Mina Almasry, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Thu, 17 Apr 2025 15:57:47 +0900 Taehee Yoo wrote:
> Thanks Mina for the suggestion!
> What I would like to do is like that
> If binding->dev is NULL, it skips locking, but it still keeps calling
> net_devmem_unbind_dmabuf().
note that the current code in net_devmem_unbind_dmabuf() is also not
safe against double removal from the socket list.
> Calling net_devmem_unbind_dmabuf() is safe even if after module unload,
> because binding->bound_rxq is deleted by the uninstall path.
> If bound_rxq is empty, binding->dev will not be accessed.
> The only uninstall side code change is to set binding->dev to NULL and
> add priv->lock.
> This approach was already suggested by Stanislav earlier in this thread.
> Mina, Stanislav, and Jakub, can you confirm this?
Maybe just send the code, even if it's not perfect. It's a bit hard
to track all the suggested changes :)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-17 14:09 ` Jakub Kicinski
@ 2025-04-18 10:46 ` Taehee Yoo
0 siblings, 0 replies; 18+ messages in thread
From: Taehee Yoo @ 2025-04-18 10:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, Mina Almasry, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Thu, Apr 17, 2025 at 11:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 17 Apr 2025 15:57:47 +0900 Taehee Yoo wrote:
> > Thanks Mina for the suggestion!
> > What I would like to do is like that
> > If binding->dev is NULL, it skips locking, but it still keeps calling
> > net_devmem_unbind_dmabuf().
>
> note that the current code in net_devmem_unbind_dmabuf() is also not
> safe against double removal from the socket list.
Okay, I will look into this too.
>
> > Calling net_devmem_unbind_dmabuf() is safe even if after module unload,
> > because binding->bound_rxq is deleted by the uninstall path.
> > If bound_rxq is empty, binding->dev will not be accessed.
> > The only uninstall side code change is to set binding->dev to NULL and
> > add priv->lock.
> > This approach was already suggested by Stanislav earlier in this thread.
>
> > Mina, Stanislav, and Jakub, can you confirm this?
>
> Maybe just send the code, even if it's not perfect. It's a bit hard
> to track all the suggested changes :)
Thanks! I will send a patch soon.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 2:59 ` Jakub Kicinski
2025-04-16 14:40 ` Stanislav Fomichev
2025-04-16 15:01 ` Taehee Yoo
@ 2025-04-16 15:47 ` Mina Almasry
2025-04-17 0:27 ` Jakub Kicinski
2 siblings, 1 reply; 18+ messages in thread
From: Mina Almasry @ 2025-04-16 15:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, Taehee Yoo, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Tue, Apr 15, 2025 at 7:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 15 Apr 2025 11:59:40 -0700 Stanislav Fomichev wrote:
> > > commit 42f342387841 ("net: fix use-after-free in the
> > > netdev_nl_sock_priv_destroy()") and rolling back a few fixes, it's
> > > really introduced by commit 1d22d3060b9b ("net: drop rtnl_lock for
> > > queue_mgmt operations").
> > >
> > > My first question, does this issue still reproduce if you remove the
> > > per netdev locking and go back to relying on rtnl_locking? Or do we
> > > crash somewhere else in net_devmem_unbind_dmabuf? If so, where?
> > > Looking through the rest of the unbinding code, it's not clear to me
> > > any of it actually uses dev, so it may just be the locking...
> >
> > A proper fix, most likely, will involve resetting binding->dev to NULL
> > when the device is going away.
>
> Right, tho a bit of work and tricky handling will be necessary to get
> that right. We're not holding a ref on binding->dev.
>
> I think we need to invert the socket mutex vs instance lock ordering.
> Make the priv mutex protect the binding->list and binding->dev.
> For that to work the binding needs to also store a pointer to its
> owning socket?
>
> Then in both uninstall paths (from socket and from netdev unreg) we can
> take the socket mutex, delete from list, clear the ->dev pointer,
> unlock, release the ref on the binding.
>
I don't like that the ref obtained by the socket can be released by
both the socket and the netdev unreg :( It creates a weird mental
model where the ref owned by the socket can actually be dropped by the
netdev unreg path and then the socket close needs to detect that
something else dropped its ref. It also creates a weird scenario where
the device got unregistered and reregistered (I assume that's
possible? Or no?) and the socket is alive and the device is registered
but actually the binding is not active.
> The socket close path would probably need to lock the socket, look at
> the first entry, if entry has ->dev call netdev_hold(), release the
> socket, lock the netdev, lock the socket again, look at the ->dev, if
> NULL we raced - done. If not NULL release the socket, call unbind.
> netdev_put(). Restart this paragraph.
>
> I can't think of an easier way.
>
How about, roughly:
- the binding holds a ref on dev, making sure that the dev is alive
until the last ref on the binding is dropped and the binding is freed.
- The ref owned by the socket is only ever dropped by the socket close.
- When we netdev_lock(binding->dev) to later do a
net_devmem_dmabuf_unbind, we must first grab another ref on the
binding->dev, so that it doesn't get freed if the unbind drops the
last ref.
I think that would work too?
Can you remind me why we do a dev_memory_provider_uninstall on a
device unregister? If the device gets unregistered then re-registered
(again, I'm kinda assuming that is possible, I'm not sure) I expect it
to still be memory provider bound, because the netlink socket is still
alive and the userspace is still expecting a live binding. Maybe
delete the dev_memory_provider_uninstall code I added on unregister,
and sorry I put it there...? Or is there some reason I'm forgetting
that we have to uninstall the memory provider on unregister?
--
Thanks,
Mina
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-16 15:47 ` Mina Almasry
@ 2025-04-17 0:27 ` Jakub Kicinski
2025-04-17 21:07 ` Mina Almasry
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-04-17 0:27 UTC (permalink / raw)
To: Mina Almasry
Cc: Stanislav Fomichev, Taehee Yoo, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Wed, 16 Apr 2025 08:47:14 -0700 Mina Almasry wrote:
> > Right, tho a bit of work and tricky handling will be necessary to get
> > that right. We're not holding a ref on binding->dev.
> >
> > I think we need to invert the socket mutex vs instance lock ordering.
> > Make the priv mutex protect the binding->list and binding->dev.
> > For that to work the binding needs to also store a pointer to its
> > owning socket?
> >
> > Then in both uninstall paths (from socket and from netdev unreg) we can
> > take the socket mutex, delete from list, clear the ->dev pointer,
> > unlock, release the ref on the binding.
>
> I don't like that the ref obtained by the socket can be released by
> both the socket and the netdev unreg :( It creates a weird mental
> model where the ref owned by the socket can actually be dropped by the
> netdev unreg path and then the socket close needs to detect that
> something else dropped its ref. It also creates a weird scenario where
> the device got unregistered and reregistered (I assume that's
> possible? Or no?) and the socket is alive and the device is registered
> but actually the binding is not active.
I agree. But it's be best I could come up with (and what we ended up
with in io-uring)...
> > The socket close path would probably need to lock the socket, look at
> > the first entry, if entry has ->dev call netdev_hold(), release the
> > socket, lock the netdev, lock the socket again, look at the ->dev, if
> > NULL we raced - done. If not NULL release the socket, call unbind.
> > netdev_put(). Restart this paragraph.
> >
> > I can't think of an easier way.
> >
>
> How about, roughly:
>
> - the binding holds a ref on dev, making sure that the dev is alive
> until the last ref on the binding is dropped and the binding is freed.
> - The ref owned by the socket is only ever dropped by the socket close.
> - When we netdev_lock(binding->dev) to later do a
> net_devmem_dmabuf_unbind, we must first grab another ref on the
> binding->dev, so that it doesn't get freed if the unbind drops the
> last ref.
Right now you can't hold a reference on a netdevice forever.
You have to register a notifier and when NETDEV_UNREGISTER triggers
you must give up the reference you took. Also, fun note, it is illegal
to take an "additional reference". You must re-lookup the device or
otherwise safely ensure device is not getting torn down.
See netdev_wait_allrefs_any(), that blocks whoever called unregister
until all refs are reclaimed.
> I think that would work too?
>
> Can you remind me why we do a dev_memory_provider_uninstall on a
> device unregister? If the device gets unregistered then re-registered
> (again, I'm kinda assuming that is possible, I'm not sure)
It's not legal right now. I think there's a BUG_ON() somewhere.
> I expect it
> to still be memory provider bound, because the netlink socket is still
> alive and the userspace is still expecting a live binding. Maybe
> delete the dev_memory_provider_uninstall code I added on unregister,
> and sorry I put it there...? Or is there some reason I'm forgetting
> that we have to uninstall the memory provider on unregister?
IIRC bound_rxqs will point to freed memory once the netdev is gone.
If we had a ref on the netdev then yeah we could possibly potentially
keep the queues around. But holding refs on a netdev is.. a topic for
another time. I'm trying to limit amount of code we'd need to revert
if the instance locking turns out to be fundamentally broken :S
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-17 0:27 ` Jakub Kicinski
@ 2025-04-17 21:07 ` Mina Almasry
2025-04-18 10:52 ` Taehee Yoo
0 siblings, 1 reply; 18+ messages in thread
From: Mina Almasry @ 2025-04-17 21:07 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, Taehee Yoo, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Wed, Apr 16, 2025 at 5:27 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 16 Apr 2025 08:47:14 -0700 Mina Almasry wrote:
> > > Right, tho a bit of work and tricky handling will be necessary to get
> > > that right. We're not holding a ref on binding->dev.
> > >
> > > I think we need to invert the socket mutex vs instance lock ordering.
> > > Make the priv mutex protect the binding->list and binding->dev.
> > > For that to work the binding needs to also store a pointer to its
> > > owning socket?
> > >
> > > Then in both uninstall paths (from socket and from netdev unreg) we can
> > > take the socket mutex, delete from list, clear the ->dev pointer,
> > > unlock, release the ref on the binding.
> >
> > I don't like that the ref obtained by the socket can be released by
> > both the socket and the netdev unreg :( It creates a weird mental
> > model where the ref owned by the socket can actually be dropped by the
> > netdev unreg path and then the socket close needs to detect that
> > something else dropped its ref. It also creates a weird scenario where
> > the device got unregistered and reregistered (I assume that's
> > possible? Or no?) and the socket is alive and the device is registered
> > but actually the binding is not active.
>
> I agree. But it's be best I could come up with (and what we ended up
> with in io-uring)...
>
> > > The socket close path would probably need to lock the socket, look at
> > > the first entry, if entry has ->dev call netdev_hold(), release the
> > > socket, lock the netdev, lock the socket again, look at the ->dev, if
> > > NULL we raced - done. If not NULL release the socket, call unbind.
> > > netdev_put(). Restart this paragraph.
> > >
> > > I can't think of an easier way.
> > >
> >
> > How about, roughly:
> >
> > - the binding holds a ref on dev, making sure that the dev is alive
> > until the last ref on the binding is dropped and the binding is freed.
> > - The ref owned by the socket is only ever dropped by the socket close.
> > - When we netdev_lock(binding->dev) to later do a
> > net_devmem_dmabuf_unbind, we must first grab another ref on the
> > binding->dev, so that it doesn't get freed if the unbind drops the
> > last ref.
>
> Right now you can't hold a reference on a netdevice forever.
> You have to register a notifier and when NETDEV_UNREGISTER triggers
> you must give up the reference you took. Also, fun note, it is illegal
> to take an "additional reference". You must re-lookup the device or
> otherwise safely ensure device is not getting torn down.
>
> See netdev_wait_allrefs_any(), that blocks whoever called unregister
> until all refs are reclaimed.
>
> > I think that would work too?
> >
> > Can you remind me why we do a dev_memory_provider_uninstall on a
> > device unregister? If the device gets unregistered then re-registered
> > (again, I'm kinda assuming that is possible, I'm not sure)
>
> It's not legal right now. I think there's a BUG_ON() somewhere.
>
Thanks, if re-registering is not possible, that makes this a lot simpler.
> > I expect it
> > to still be memory provider bound, because the netlink socket is still
> > alive and the userspace is still expecting a live binding. Maybe
> > delete the dev_memory_provider_uninstall code I added on unregister,
> > and sorry I put it there...? Or is there some reason I'm forgetting
> > that we have to uninstall the memory provider on unregister?
>
> IIRC bound_rxqs will point to freed memory once the netdev is gone.
> If we had a ref on the netdev then yeah we could possibly potentially
> keep the queues around. But holding refs on a netdev is.. a topic for
> another time. I'm trying to limit amount of code we'd need to revert
> if the instance locking turns out to be fundamentally broken :S
OK, no need to hold a reference, please ignore that suggestion. Thanks
for the detailed explanation.
There are a lot of suggestions flying around at the moment as you
noted so I'll refrain from adding more and I'll just review the next
version. Agree with what Jakub says below, please do send Taehee even
if it's not perfect, this may need some iteration.
--
Thanks,
Mina
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-17 21:07 ` Mina Almasry
@ 2025-04-18 10:52 ` Taehee Yoo
2025-05-05 17:34 ` Jakub Kicinski
0 siblings, 1 reply; 18+ messages in thread
From: Taehee Yoo @ 2025-04-18 10:52 UTC (permalink / raw)
To: Mina Almasry
Cc: Jakub Kicinski, Stanislav Fomichev, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Fri, Apr 18, 2025 at 6:07 AM Mina Almasry <almasrymina@google.com> wrote:
>
> On Wed, Apr 16, 2025 at 5:27 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 16 Apr 2025 08:47:14 -0700 Mina Almasry wrote:
> > > > Right, tho a bit of work and tricky handling will be necessary to get
> > > > that right. We're not holding a ref on binding->dev.
> > > >
> > > > I think we need to invert the socket mutex vs instance lock ordering.
> > > > Make the priv mutex protect the binding->list and binding->dev.
> > > > For that to work the binding needs to also store a pointer to its
> > > > owning socket?
> > > >
> > > > Then in both uninstall paths (from socket and from netdev unreg) we can
> > > > take the socket mutex, delete from list, clear the ->dev pointer,
> > > > unlock, release the ref on the binding.
> > >
> > > I don't like that the ref obtained by the socket can be released by
> > > both the socket and the netdev unreg :( It creates a weird mental
> > > model where the ref owned by the socket can actually be dropped by the
> > > netdev unreg path and then the socket close needs to detect that
> > > something else dropped its ref. It also creates a weird scenario where
> > > the device got unregistered and reregistered (I assume that's
> > > possible? Or no?) and the socket is alive and the device is registered
> > > but actually the binding is not active.
> >
> > I agree. But it's be best I could come up with (and what we ended up
> > with in io-uring)...
> >
> > > > The socket close path would probably need to lock the socket, look at
> > > > the first entry, if entry has ->dev call netdev_hold(), release the
> > > > socket, lock the netdev, lock the socket again, look at the ->dev, if
> > > > NULL we raced - done. If not NULL release the socket, call unbind.
> > > > netdev_put(). Restart this paragraph.
> > > >
> > > > I can't think of an easier way.
> > > >
> > >
> > > How about, roughly:
> > >
> > > - the binding holds a ref on dev, making sure that the dev is alive
> > > until the last ref on the binding is dropped and the binding is freed.
> > > - The ref owned by the socket is only ever dropped by the socket close.
> > > - When we netdev_lock(binding->dev) to later do a
> > > net_devmem_dmabuf_unbind, we must first grab another ref on the
> > > binding->dev, so that it doesn't get freed if the unbind drops the
> > > last ref.
> >
> > Right now you can't hold a reference on a netdevice forever.
> > You have to register a notifier and when NETDEV_UNREGISTER triggers
> > you must give up the reference you took. Also, fun note, it is illegal
> > to take an "additional reference". You must re-lookup the device or
> > otherwise safely ensure device is not getting torn down.
> >
> > See netdev_wait_allrefs_any(), that blocks whoever called unregister
> > until all refs are reclaimed.
> >
> > > I think that would work too?
> > >
> > > Can you remind me why we do a dev_memory_provider_uninstall on a
> > > device unregister? If the device gets unregistered then re-registered
> > > (again, I'm kinda assuming that is possible, I'm not sure)
> >
> > It's not legal right now. I think there's a BUG_ON() somewhere.
> >
>
> Thanks, if re-registering is not possible, that makes this a lot simpler.
>
> > > I expect it
> > > to still be memory provider bound, because the netlink socket is still
> > > alive and the userspace is still expecting a live binding. Maybe
> > > delete the dev_memory_provider_uninstall code I added on unregister,
> > > and sorry I put it there...? Or is there some reason I'm forgetting
> > > that we have to uninstall the memory provider on unregister?
> >
> > IIRC bound_rxqs will point to freed memory once the netdev is gone.
> > If we had a ref on the netdev then yeah we could possibly potentially
> > keep the queues around. But holding refs on a netdev is.. a topic for
> > another time. I'm trying to limit amount of code we'd need to revert
> > if the instance locking turns out to be fundamentally broken :S
>
> OK, no need to hold a reference, please ignore that suggestion. Thanks
> for the detailed explanation.
>
> There are a lot of suggestions flying around at the moment as you
> noted so I'll refrain from adding more and I'll just review the next
> version. Agree with what Jakub says below, please do send Taehee even
> if it's not perfect, this may need some iteration.
Thanks for the review!
I’ll send the patch over shortly.
Thanks!
Taehee Yoo
>
> --
> Thanks,
> Mina
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-04-18 10:52 ` Taehee Yoo
@ 2025-05-05 17:34 ` Jakub Kicinski
2025-05-06 11:41 ` Taehee Yoo
0 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2025-05-05 17:34 UTC (permalink / raw)
To: Taehee Yoo
Cc: Mina Almasry, Stanislav Fomichev, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Fri, 18 Apr 2025 19:52:43 +0900 Taehee Yoo wrote:
> I’ll send the patch over shortly.
Hi Taehee!
Are you still working on this problem? I haven't seen any patches,
I think it's the last known instance locking problem we have.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net] net: devmem: fix kernel panic when socket close after module unload
2025-05-05 17:34 ` Jakub Kicinski
@ 2025-05-06 11:41 ` Taehee Yoo
0 siblings, 0 replies; 18+ messages in thread
From: Taehee Yoo @ 2025-05-06 11:41 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Mina Almasry, Stanislav Fomichev, davem, pabeni, edumazet,
andrew+netdev, horms, asml.silence, dw, sdf, skhawaja,
simona.vetter, kaiyuanz, netdev
On Tue, May 6, 2025 at 2:34 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 18 Apr 2025 19:52:43 +0900 Taehee Yoo wrote:
> > I’ll send the patch over shortly.
>
> Hi Taehee!
>
> Are you still working on this problem? I haven't seen any patches,
> I think it's the last known instance locking problem we have.
Oh, sorry for the late!
I will send the v2 patch today!
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 18+ messages in thread