Re: [PATCH net v3] 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 3 potential
issue(s) to consider.

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

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: the new occupied-device check in
  tipc_enable_bearer() (net/tipc/bearer.c:308-311) can be bypassed.
- [Low] The commit message does not match the code.
- [Low] The new check in tipc_enable_bearer() (net/tipc/bearer.c:309-311)
  compares two possibly-NULL results of __dev_get_by_name() without a…

--

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

This patch adds a check to tipc_enable_bearer(). It resolves the interface
names of the existing and the requested bearer to net_devices, and rejects
the new bearer when both resolve to the same device. The goal is to stop
the dev_refcnt leak when, for example, "eth:eth0" and "ib:eth0" are both
enabled.

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..f480fffca843e 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -285,17 +285,42 @@ static int tipc_enable_bearer(struct net *net, const char *name,

[ ... ]

> +		/* udp media can be used together with any media */
> +		if (b->media->type_id == TIPC_MEDIA_TYPE_UDP ||
> +		    m->type_id == TIPC_MEDIA_TYPE_UDP)
> +			goto priority;
> +
> +		if_name = strchr((const char *)b->name, ':') + 1;
> +		dev1 = __dev_get_by_name(net, if_name);
> +		dev2 = __dev_get_by_name(net, b_names.if_name);

[Severity: High]
Can this check be bypassed when the existing bearer's saved name no longer
resolves to the device the bearer is bound to?

dev1 comes from looking up b->name again. It does not use the device the
bearer actually holds (rtnl_dereference(b->media_ptr) for L2 media).
tipc_enable_l2_media() uses dev_get_by_name(), so a bearer can be enabled
by an altname. That altname can later be deleted while the bearer stays
enabled.

Deleting an altname goes through:

rtnl_linkprop()
  rtnl_alt_ifname()
    netdev_name_node_alt_destroy()
  netdev_state_change()

This sends only NETDEV_CHANGE, and only when the device is up. On
NETDEV_CHANGE, tipc_l2_device_event() only sets the bearer up or resets
it. Only NETDEV_UNREGISTER, NETDEV_CHANGENAME and a bad NETDEV_CHANGEMTU
disable it.

For example:

  ip link property add dev eth0 altname foo
  tipc bearer enable media eth dev foo
      (b1->name = "eth:foo", holds eth0, eth0->tipc_ptr = b1)
  ip link property del dev eth0 altname foo
      (b1 stays enabled)
  tipc bearer enable media ib dev eth0

In the new loop strcmp() does not match. dev1 =
__dev_get_by_name("foo") is NULL and dev2 is eth0, so the check is
skipped. tipc_enable_l2_media() then does:

	dev = dev_get_by_name(net, dev_name);
	...
	rcu_assign_pointer(dev->tipc_ptr, b);

This takes a second reference on eth0 and overwrites eth0->tipc_ptr with
b2. Isn't this the same dev_refcnt leak the commit message describes?

On NETDEV_UNREGISTER only b2 is disabled, because it is the one found
through dev->tipc_ptr. b1's reference and packet_type stay in place, so
netdev_wait_allrefs() waits until b1 is disabled by hand.

If b1 is disabled first instead, that clears dev->tipc_ptr while it points
to b2, and b2 is left orphaned in the same way.

TIPC_NL_BEARER_ENABLE is GENL_UNS_ADMIN_PERM. CAP_NET_ADMIN in a user
namespace is enough to do this with dummy or veth devices.

The name-based lookup can also reject requests wrongly. Suppose the altname
"foo" is moved to eth1, or eth1 is renamed to "foo". A request for eth1
then gives dev1 == dev2 == eth1 and is rejected, although eth1 has no
bearer.

Would it be more robust to compare against the real binding? One option is
rtnl_dereference(b->media_ptr) for L2 media. Another is a check in
tipc_enable_l2_media() that returns -EBUSY when
rtnl_dereference(dev->tipc_ptr) is already set.

> +		if (dev1 == dev2) {

[Severity: Low]
What happens here when both lookups fail? __dev_get_by_name() returns NULL
for a name that does not resolve, and nothing checks for NULL before the
comparison.

Suppose an existing eth/ib bearer's saved altname has been deleted, so
dev1 is NULL as described above. If the request is for an interface that
does not exist, dev2 is also NULL. NULL == NULL then matches, and the
request is rejected with -EINVAL and "Already enabled" or "Same dev for
different media".

Without this check the request would reach tipc_enable_l2_media(), which
returns -ENODEV:

	dev = dev_get_by_name(net, dev_name);
	if (!dev)
		return -ENODEV;

Should a NULL dev1 or dev2 count as no match here?

> +			/* Not allow eth and ib to attach to the same device */
> +			if (b->media != m) {
> +				errstr = "same dev for different media";
> +				NL_SET_ERR_MSG(extack,
> +					       "Same dev for different media");
> +				goto rejected;
> +			} else {
> +				goto duplicate;
> +			}

[Severity: Low]
This isn't a bug, but the commit message only describes the different
media case:

  Fix this by rejecting a bearer with a different media type when the
  device is already occupied.

The else branch also rejects a second bearer of the same media on the same
device when it is named by an altname. An example is "eth:eth0" plus
"eth:foo", where foo is an altname of eth0. This case fails with -EINVAL
and "Already enabled". Before this patch that configuration was accepted
and leaked dev_refcnt in the same way.

The only mention of this is the v3 changelog below the "---" line, which is
dropped when the patch is applied. Could the commit message describe this
case too?

> +		}
> +
> +priority:

[ ... ]

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