Re: [net-next,4/9] ipvs: do not keep dest_dst after dest is removed

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "FYI"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ipvs: do not keep dest_dst after dest is removed

Before now dest->dest_dst is not released when server is moved into
dest_trash list after removal. As result, we can keep dst/dev
references for long time without actively using them. It is better to
avoid walking the dest_trash list when ip_vs_dst_event() receives dev
events. So, make sure we do not hold dev references in dest_trash
list. As packets can be flying while server is being removed, check
the IP_VS_DEST_F_AVAILABLE flag in slow path to ensure we do not save
new dev references to removed servers.

> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index 2ef1f99dada6..7c0e2d9b5b98 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c

[ ... ]

> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 4389bfe3050d..394b5b5f2ccd 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -336,9 +336,11 @@ __ip_vs_get_out_rt(struct netns_ipvs *ipvs, int skb_af, struct sk_buff *skb,
>  				goto err_unreach;
>  			}
>  			/* It is forbidden to attach dest->dest_dst if
> -			 * device is going down.
> +			 * device is going down or if server is removed and
> +			 * stored in dest_trash.
>  			 */
> -			if (!rt_dev_is_down(dst_dev_rcu(&rt->dst)))
> +			if (!rt_dev_is_down(dst_dev_rcu(&rt->dst)) &&
> +			    dest->flags & IP_VS_DEST_F_AVAILABLE)
>  				__ip_vs_dst_set(dest, dest_dst, &rt->dst, 0);

Should this be wrapped in parentheses?

In C operator precedence, the logical AND (&&) binds more tightly than
the bitwise AND (&). The expression here parses as:

    (!rt_dev_is_down(...) && dest->flags) & IP_VS_DEST_F_AVAILABLE

This works correctly only because IP_VS_DEST_F_AVAILABLE is 0x0001, and
the logical AND produces 0 or 1, so the bitwise AND with 0x0001 still
produces the expected result.

Every other check of IP_VS_DEST_F_AVAILABLE in the IPVS codebase wraps
the bitwise operation in parentheses. For example:

net/netfilter/ipvs/ip_vs_core.c:ip_vs_out():
    if (likely(dest && (dest->flags & IP_VS_DEST_F_AVAILABLE))) {
        ...
    }

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_fill_param_sync():
    if (unlikely(dest && !(dest->flags & IP_VS_DEST_F_AVAILABLE))) {
        ...
    }

Would it be clearer to write this as:

    if (!rt_dev_is_down(dst_dev_rcu(&rt->dst)) &&
        (dest->flags & IP_VS_DEST_F_AVAILABLE))

>  			else
>  				noref = 0;

[ ... ]

> @@ -513,9 +515,11 @@ __ip_vs_get_out_rt_v6(struct netns_ipvs *ipvs, int skb_af, struct sk_buff *skb,
>  			rt = dst_rt6_info(dst);
>  			cookie = rt6_get_cookie(rt);
>  			/* It is forbidden to attach dest->dest_dst if
> -			 * device is going down.
> +			 * device is going down or if server is removed and
> +			 * stored in dest_trash.
>  			 */
> -			if (!rt_dev_is_down(dst_dev_rcu(&rt->dst)))
> +			if (!rt_dev_is_down(dst_dev_rcu(&rt->dst)) &&
> +			    dest->flags & IP_VS_DEST_F_AVAILABLE)
>  				__ip_vs_dst_set(dest, dest_dst, &rt->dst, cookie);

Same question here in __ip_vs_get_out_rt_v6().