On 10/24/23 8:27 PM, Stanislav Fomichev wrote:
On 10/24, Daniel Borkmann wrote:
On 10/24/23 6:40 PM, Stanislav Fomichev wrote:
On 10/23, Daniel Borkmann wrote:
[...]
The series looks great! FWIW:
Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
Thanks for review!
One small question I have is:
We now (and after introduction of tcx) seem to store non-refcounted
dev pointers in the bpf_link(s). Is it guaranteed that the dev will
outlive the link?
The semantics are the same as it was done in XDP, meaning, the link is in
detached state so link->dev is NULL when dev goes away, see also the
dev_xdp_uninstall(). We cannot hold a refcount on the dev as otherwise
if the link outlives it we get the infamous "unregister_netdev...waiting
for <dev>... refcnt = 1" bug.
Yeah, I remember I've had a similar issue with holding netdev when
adding dev-bound programs, so I was wondering what are we doing here.
Thanks for the pointers!
And here, I guess the assumption that the device shutdown goes via
dellink (netkit_del_link) and there is no special path that reaches
unregister_netdevice_many_notify otherwise, right?
Correct, this is where various netdevices do their internal cleanup.
What about that ndo_uninit btw? Would it be more safe/clear to make
netkit_release_all be ndo_uninit? Looks like it's being triggered
in a place similar to dev_xdp_uninstall/dev_tcx_uninstall.
Looking into it, that is a better location for netkit_release_all()
indeed, and ndo_uninit is under rtnl. I'll spin a v4 with this and the
commit message fixups that Toke suggested. Thanks for the pointer!
Thanks,
Daniel
