linux-kernel - Re: [PATCH net-next v5] l2tp: fix double dst_release() on sk_dst

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ff955e6f-234e-4e0c-89bd-36c8518ca4fd@redhat.com>
Date: Tue, 2 Dec 2025 13:18:54 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Mikhail Lobanov <m.lobanov@...a.ru>, "David S. Miller"
 <davem@...emloft.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, David Bauer <mail@...id-bauer.net>,
 James Chapman <jchapman@...alix.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH net-next v5] l2tp: fix double dst_release() on
 sk_dst_cache race

On 11/28/25 2:22 PM, Mikhail Lobanov wrote:
> @@ -1206,15 +1207,55 @@ static int l2tp_build_l2tpv3_header(struct l2tp_session *session, void *buf)
>  static int l2tp_xmit_queue(struct l2tp_tunnel *tunnel, struct sk_buff *skb, struct flowi *fl)
>  {
>  	int err;
> +	struct sock *sk = tunnel->sock;
>  
>  	skb->ignore_df = 1;
>  	skb_dst_drop(skb);
>  #if IS_ENABLED(CONFIG_IPV6)
> -	if (l2tp_sk_is_v6(tunnel->sock))
> -		err = inet6_csk_xmit(tunnel->sock, skb, NULL);
> -	else
> +	if (l2tp_sk_is_v6(sk)) {
> +		struct ipv6_pinfo *np = inet6_sk(sk);
> +		struct inet_sock *inet = inet_sk(sk);
> +		struct flowi6 fl6;
> +		struct dst_entry *dst;
> +		struct in6_addr *final_p, final;
> +		struct ipv6_txoptions *opt;

I'm sorry to nit-pick, but please respect the reverse christmas tree
order above.

> +
> +		memset(&fl6, 0, sizeof(fl6));
> +		fl6.flowi6_proto = sk->sk_protocol;
> +		fl6.daddr        = sk->sk_v6_daddr;
> +		fl6.saddr        = np->saddr;
> +		fl6.flowlabel    = np->flow_label;
> +		IP6_ECN_flow_xmit(sk, fl6.flowlabel);
> +
> +		fl6.flowi6_oif   = READ_ONCE(sk->sk_bound_dev_if);
> +		fl6.flowi6_mark  = READ_ONCE(sk->sk_mark);
> +		fl6.fl6_sport    = inet->inet_sport;
> +		fl6.fl6_dport    = inet->inet_dport;
> +		fl6.flowi6_uid   = sk_uid(sk);
> +
> +		security_sk_classify_flow(sk, flowi6_to_flowi_common(&fl6));
> +
> +		rcu_read_lock();
> +		opt = rcu_dereference(np->opt);
> +		final_p = fl6_update_dst(&fl6, opt, &final);
> +
> +		dst = ip6_sk_dst_lookup_flow(sk, &fl6, final_p, true);
> +		if (IS_ERR(dst)) {
> +			rcu_read_unlock();
> +			kfree_skb(skb);
> +			return NET_XMIT_DROP;
> +		}
> +
> +		skb_dst_set(skb, dst);
> +		fl6.daddr = sk->sk_v6_daddr;
> +
> +		err = ip6_xmit(sk, skb, &fl6, READ_ONCE(sk->sk_mark),
> +			       opt, np->tclass,
> +			       READ_ONCE(sk->sk_priority));
> +		rcu_read_unlock();
> +	} else
>  #endif
> -		err = ip_queue_xmit(tunnel->sock, skb, fl);
> +		err = ip_queue_xmit(sk, skb, fl);

This break the kernel style, as brackets are now needed around the else
statement, too.

If you move the xmit code to a separate helper, the problem will go away.

Also I *think*/wild guess the same issue affects ipv4, too. Why the ipv4
path does not need a similar fix?

Thanks,

Paolo