Re: [v2,net-next,01/15] selftest: net: Add basic functionality tests for ipmr.

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
selftest: net: Add basic functionality tests for ipmr.

This commit adds self-tests for IPv4 multicast routing (ipmr) that
exercise various setsockopt and rtnetlink paths to catch lockdep issues.
The tests cover MRT_INIT/DONE, VIF management, MFC operations, and RTNL
integration.

> diff --git a/tools/testing/selftests/net/forwarding/.gitignore b/tools/testing/selftests/net/forwarding/.gitignore
> index 2dea317f12e7..418ff96c52ef 100644
> --- a/tools/testing/selftests/net/forwarding/.gitignore
> +++ b/tools/testing/selftests/net/forwarding/.gitignore
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  forwarding.config
> +ipmr
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index ff4a00d91a26..bbaf4d937dd8 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile

[ ... ]

> diff --git a/tools/testing/selftests/net/forwarding/ipmr.c b/tools/testing/selftests/net/forwarding/ipmr.c
> new file mode 100644
> index 000000000000..b6b00e36b3ce
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/ipmr.c
> @@ -0,0 +1,455 @@

[ ... ]

> +static struct rtattr *nl_add_rtattr(struct nlmsghdr *nlmsg, struct rtattr *rta,
> +				    int type, const void *data, int len)
> +{
> +	int unused;
                ^^^^^^

Does this code have undefined behavior?  The variable 'unused' is declared
here but never initialized, then passed to RTA_NEXT() which expands to a
macro that reads its value to perform arithmetic.

In nl_add_rtattr():

	int unused;

	rta->rta_type = type;
	rta->rta_len = RTA_LENGTH(len);
	memcpy(RTA_DATA(rta), data, len);

	nlmsg->nlmsg_len += NLMSG_ALIGN(rta->rta_len);

	return RTA_NEXT(rta, unused);

The RTA_NEXT macro performs '(attrlen) -= RTA_ALIGN((rta)->rta_len)', which
reads the uninitialized 'unused' value.

While the modified value is never subsequently used, the read itself is
undefined behavior that could trigger compiler warnings with -Wuninitialized
or unpredictable behavior with certain optimizations.

Initializing unused to 0 at declaration would resolve this: 'int unused = 0;'
-- 
pw-bot: cr