netdev - Re: [PATCH net-next v3] l2tp: use skb_queue_purge in l2tp_ip_destroy

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] [day] [month] [year] [list]

Message-ID: <CANn89i+Y3m7fQhV+C5JKp+axhNez2fpH6TMewU6k+sLJtM8uNg@mail.gmail.com>
Date: Mon, 19 Aug 2024 09:24:47 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: James Chapman <jchapman@...alix.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, tparkin@...alix.com, 
	xiyou.wangcong@...il.com
Subject: Re: [PATCH net-next v3] l2tp: use skb_queue_purge in l2tp_ip_destroy_sock

On Fri, Aug 16, 2024 at 10:07 AM James Chapman <jchapman@...alix.com> wrote:
>
> Recent commit ed8ebee6def7 ("l2tp: have l2tp_ip_destroy_sock use
> ip_flush_pending_frames") was incorrect in that l2tp_ip does not use
> socket cork and ip_flush_pending_frames is for sockets that do. Use
> skb_queue_purge instead and remove the unnecessary lock.
>
> Also unexport ip_flush_pending_frames since it was originally exported
> in commit 4ff8863419cd ("ipv4: export ip_flush_pending_frames") for
> l2tp and is not used by other modules.
>
> Suggested-by: xiyou.wangcong@...il.com
> Signed-off-by: James Chapman <jchapman@...alix.com>
> ---
>   v3:
>     - put signoff above change history
>   v2: https://lore.kernel.org/all/20240815074311.1238511-1-jchapman@katalix.com/
>     - also unexport ip_flush_pending_frames (cong)
>   v1: https://lore.kernel.org/all/20240813093914.501183-1-jchapman@katalix.com/
> ---
>  net/ipv4/ip_output.c | 1 -
>  net/l2tp/l2tp_ip.c   | 4 +---
>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 8a10a7c67834..b90d0f78ac80 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1534,7 +1534,6 @@ void ip_flush_pending_frames(struct sock *sk)
>  {
>         __ip_flush_pending_frames(sk, &sk->sk_write_queue, &inet_sk(sk)->cork.base);
>  }
> -EXPORT_SYMBOL_GPL(ip_flush_pending_frames);
>
>  struct sk_buff *ip_make_skb(struct sock *sk,
>                             struct flowi4 *fl4,
> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
> index 39f3f1334c4a..ad659f4315df 100644
> --- a/net/l2tp/l2tp_ip.c
> +++ b/net/l2tp/l2tp_ip.c
> @@ -258,9 +258,7 @@ static void l2tp_ip_destroy_sock(struct sock *sk)
>  {
>         struct l2tp_tunnel *tunnel;
>
> -       lock_sock(sk);
> -       ip_flush_pending_frames(sk);
> -       release_sock(sk);
> +       skb_queue_purge(&sk->sk_write_queue);

It seems __skb_queue_purge() would be enough ?

If not, a comment explaining why another thread might access sk->sk_write_queue
while l2tp_ip_destroy_sock() is running would be nice.