Re: [PATCH net v2] tipc: reject attaching bearer of different media to the same occupied device

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: two bearers can still be attached to one
  net_device, so the dev_refcnt leak the commit claims to fix can still…
- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:300-301)
  detects UDP in two different ways.

--

tipc: reject attaching bearer of different media to the same occupied device

This commit adds a check to tipc_enable_bearer(). The check rejects a new
eth or ib bearer when an existing bearer of a different media type uses
the same interface name. The goal is to stop a dev_refcnt leak where a
second bearer takes another dev_hold() in tipc_enable_l2_media() and
replaces the first one. UDP bearers are exempt from the check.

Closes: http://lore.kernel.org/netdev/20260918013509.1853952-1-xujiakai24@mails.ucas.ac.cn/

> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 05dcd2f9e887a..2c0ceee4aa6b9 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c

[ ... ]

> @@ -296,6 +297,21 @@ static int tipc_enable_bearer(struct net *net, const char *name,
>  			goto rejected;
>  		}
>  
> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
> +		    !strcmp(b_names.media_name, "udp"))
> +			goto priority;

[Severity: Low]
This isn't a bug, but the new bearer's media is already known at this
point, because earlier in tipc_enable_bearer() it was looked up with:

	m = tipc_media_find(b_names.media_name);

Could this use m->type_id == TIPC_MEDIA_TYPE_UDP instead of comparing
against the "udp" string? That would match the check on the existing
bearer and the other UDP checks in bearer.c.

For the same reason, the media difference test below could be written as
b->media != m.

> +
> +		/* Not allow eth and ib to attach to the same device */
> +		if_name = strchr((const char *)b->name, ':') + 1;
> +		if (!strcmp(if_name, b_names.if_name) &&
> +		    strcmp(b->media->name, b_names.media_name)) {

[Severity: High]
Does this string comparison catch every case where two bearers end up on
the same net_device?

tipc_enable_l2_media() finds the device with dev_get_by_name(). That
lookup also matches altnames, because netdev_name_node_alt_create() adds
them to the same per-netns name hash. Consider this sequence:

  enable bearer "eth:dummy0"
  ip link property add dev dummy0 altname foo
  enable bearer "ib:foo"   (or "eth:foo")

The if_name strings differ, so this check passes. The earlier "already
enabled" check on the full name passes too. This check never looks at the
same-media case ("eth:dummy0" plus "eth:foo"), and that case needs only
the eth media.

The second attach then takes another device reference and overwrites
dev->tipc_ptr without checking it:

net/tipc/bearer.c:tipc_enable_l2_media() {
    ...
	dev = dev_get_by_name(net, dev_name);
    ...
	rcu_assign_pointer(dev->tipc_ptr, b);
	return 0;
}

On NETDEV_UNREGISTER, tipc_l2_device_event() disables only the bearer
found in dev->tipc_ptr. tipc_disable_l2_media() then clears it:

	RCU_INIT_POINTER(dev->tipc_ptr, NULL);

As a result, the NETDEV_UNREGISTER that netdev_wait_allrefs_any()
rebroadcasts gets NOTIFY_DONE. The other bearer's device reference is
never dropped.

Would ip link del dummy0 then loop forever in netdev_wait_allrefs_any()?
Disabling the second bearer first gives the same result, because the
first bearer is then invisible to the notifier.

An altname can also be added after the first bearer already exists. Any
name-based check at enable time would miss that case.

TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM, and adding an altname needs
only CAP_NET_ADMIN in the netns's user namespace. That makes this look
reachable from an unprivileged user namespace.

Would it be more robust to replace this string check with an occupancy
check in tipc_enable_l2_media(), right after dev_get_by_name()? For
example, when rtnl_dereference(dev->tipc_ptr) is non-NULL, call dev_put()
and return -EBUSY. UDP bearers never set dev->tipc_ptr, so the udp
special case above would no longer be needed either.

> +			errstr = "same device for different media";
> +			NL_SET_ERR_MSG(extack,
> +				       "Same device for different media");
> +			goto rejected;
> +		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924162016.1258485-1-tung.quang.nguyen%40est.tech