linux-kernel - Re: [PATCHv2 net] ipvs: Always clear ipvs_property flag in skb_scrub

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61a40de5-3b11-e84b-90a5-fefd8da3bb23@ssi.bg>
Date: Tue, 25 Feb 2025 12:55:40 +0200 (EET)
From: Julian Anastasov <ja@....bg>
To: Philo Lu <lulie@...ux.alibaba.com>
cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
        asml.silence@...il.com, willemb@...gle.com, almasrymina@...gle.com,
        chopps@...n.net, aleksander.lobakin@...el.com,
        nicolas.dichtel@...nd.com, dust.li@...ux.alibaba.com,
        hustcat@...il.com, horms@...ge.net.au, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 net] ipvs: Always clear ipvs_property flag in
 skb_scrub_packet()


	Hello,

On Sat, 22 Feb 2025, Philo Lu wrote:

> We found an issue when using bpf_redirect with ipvs NAT mode after
> commit ff70202b2d1a ("dev_forward_skb: do not scrub skb mark within
> the same name space"). Particularly, we use bpf_redirect to return
> the skb directly back to the netif it comes from, i.e., xnet is
> false in skb_scrub_packet(), and then ipvs_property is preserved
> and SNAT is skipped in the rx path.
> 
> ipvs_property has been already cleared when netns is changed in
> commit 2b5ec1a5f973 ("netfilter/ipvs: clear ipvs_property flag when
> SKB net namespace changed"). This patch just clears it in spite of
> netns.
> 
> Fixes: 2b5ec1a5f973 ("netfilter/ipvs: clear ipvs_property flag when SKB net namespace changed")
> Signed-off-by: Philo Lu <lulie@...ux.alibaba.com>

	Looks good to me, thanks!

Acked-by: Julian Anastasov <ja@....bg>

	It was safer to reset the flag when netns changes but
it has role only before output device is reached or while
packet is looped over lo device. New tunnel headers should
be safe to reset it because nf ct and dst are dropped too.

> ---
> v1 -> v2:
>  - Add Fixes tag as suggested by Julian Anastasov
> ---
>  net/core/skbuff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7b03b64fdcb2..b1c81687e9d8 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6033,11 +6033,11 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>  	skb->offload_fwd_mark = 0;
>  	skb->offload_l3_fwd_mark = 0;
>  #endif
> +	ipvs_reset(skb);
>  
>  	if (!xnet)
>  		return;
>  
> -	ipvs_reset(skb);
>  	skb->mark = 0;
>  	skb_clear_tstamp(skb);
>  }
> -- 
> 2.32.0.3.g01195cf9f

Regards

--
Julian Anastasov <ja@....bg>