Re: [PATCH net v1 1/2] net/rds: Optimize rds_ib_laddr_check

[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
---
net/rds: Optimize rds_ib_laddr_check

This patch optimizes rds_ib_laddr_check() to avoid excessive
rdma_bind_addr() calls by checking if an address has been used before.
It renames the original function to rds_ib_laddr_check_cm() and
introduces a new wrapper that first checks rds_ib_get_device() before
falling back to the CM-based check.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index ac6affa33ce7..73e01984ee9a 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -401,8 +401,8 @@ static void rds6_ib_ic_info(struct socket *sock, unsigned int len,
>   * allowed to influence which paths have priority.  We could call userspace
>   * asserting this policy "routing".
>   */
> -static int rds_ib_laddr_check(struct net *net, const struct in6_addr *addr,
> -			      __u32 scope_id)
> +static int rds_ib_laddr_check_cm(struct net *net, const struct in6_addr *addr,
> +				 __u32 scope_id)
>  {
>  	int ret;
>  	struct rdma_cm_id *cm_id;

[ ... ]

> @@ -487,6 +487,20 @@ static int rds_ib_laddr_check(struct net *net, const struct in6_addr *addr,
>  	return ret;
>  }
>
> +static int rds_ib_laddr_check(struct net *net, const struct in6_addr *addr,
> +			      __u32 scope_id)
> +{
> +	struct rds_ib_device *rds_ibdev = rds_ib_get_device(addr->s6_addr32[3]);
> +
> +	if (rds_ibdev) {
> +		rds_ib_dev_put(rds_ibdev);
> +
> +		return 0;
> +	}
> +
> +	return rds_ib_laddr_check_cm(net, addr, scope_id);
> +}
> +

Can this code handle pure IPv6 addresses correctly? The function extracts
addr->s6_addr32[3] and passes it to rds_ib_get_device(), which looks up
devices by IPv4 address.

For IPv4-mapped IPv6 addresses, addr->s6_addr32[3] contains the IPv4
address. But for pure IPv6 addresses (when CONFIG_IPV6 is enabled),
addr->s6_addr32[3] contains the last 32 bits of the IPv6 address, which
could accidentally match a stored IPv4 address.

When this collision occurs, the function returns 0 (success) without
calling rds_ib_laddr_check_cm(), which performs the full validation
including rdma_bind_addr() and IPv6 link-local scope_id checks. This
means a pure IPv6 address could be incorrectly accepted as valid for IB
transport.

Should this be guarded with an ipv6_addr_v4mapped(addr) check? For
example:

    if (ipv6_addr_v4mapped(addr)) {
        rds_ibdev = rds_ib_get_device(addr->s6_addr32[3]);
        ...
    }

The existing callers of rds_ib_get_device (rds_ib_get_mr,
rds_ib_update_ipaddr) operate in post-connection contexts where the
address type is already validated, but rds_ib_laddr_check is the
validation gate called during bind/transport-selection for any address
type.

>  static void rds_ib_unregister_client(void)
>  {
>  	ib_unregister_client(&rds_ib_client);

[ ... ]
-- 
pw-bot: cr