Re: [PATCH v1 net-next 1/7] ipmr: Call ->dellink() to remove DVMRP tunnel device.

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The commit message of "ipmr: Call ->dellink() to remove DVMRP
  tunnel device." states the rationale inverted: "This is fine because…

--

ipmr: Call ->dellink() to remove DVMRP tunnel device.

The patch converts the two places in net/ipv4/ipmr.c that removed DVMRP
tunnel devices with unregister_netdevice()/unregister_netdevice_queue()
into calls to the device's ->dellink() followed by unregistration, in
preparation for moving ip_tunnel_del() from ip_tunnel_uninit() to
ip_tunnel_dellink().

ipmr.c uses unregister_netdevice() to remove DVMRP tunnel devices
created in ipmr_new_tunnel().

This is fine because currently ipip_link_ops.dellink() is no-op
for non-fallback tunnel devices.

[Severity: Low]
Is this sentence inverted?  ipip_link_ops sets .dellink = ip_tunnel_dellink,
and that function looks like this:

net/ipv4/ip_tunnel.c:ip_tunnel_dellink() {
	itn = net_generic(tunnel->net, tunnel->ip_tnl_net_id);

	if (itn->fb_tunnel_dev != dev) {
		ip_tunnel_del(itn, netdev_priv(dev));
		unregister_netdevice_queue(dev, head);
	}
}

So it does real work (unhash plus queue unregister) for non-fallback
devices, and is a no-op only for the fallback device (itn->fb_tunnel_dev).

The dvmrp%d devices ipmr creates go through tunl0's
ndo_tunnel_ctl(SIOCADDTUNNEL) -> ip_tunnel_create -> ip_tunnel_add, so
they are non-fallback and hashed in itn->tunnels[].

Isn't the reason the current unregister_netdevice() calls in ipmr.c are
harmless instead that ip_tunnel_uninit(), which runs as ndo_uninit from
the unregister path, also calls ip_tunnel_del()?

net/ipv4/ip_tunnel.c:ip_tunnel_uninit() {
	itn = net_generic(net, tunnel->ip_tnl_net_id);
	ip_tunnel_del(itn, netdev_priv(dev));
	...
}

ip_tunnel_del() uses hlist_del_init_rcu(), so calling it from both
->dellink() and ndo_uninit is idempotent and this conversion is
behaviour-preserving today.

Could the message be reworded to cite ip_tunnel_uninit() rather than
describing ->dellink() as a no-op for non-fallback devices?  As written it
suggests this preparatory patch has no interaction with ipmr, which is the
opposite of what the follow-up "ip_tunnel: Centralise ip_tunnel_del() to
ip_tunnel_dellink()." depends on.  The code change itself looks fine, and
dev->rtnl_link_ops is always set for ioctl-created tunnels by
__ip_tunnel_create(), so the new ->dellink() dereferences in
ipmr_new_tunnel() and vif_delete() cannot be NULL.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907225846.3787676-1-kuniyu%40google.com