netdev - RE: [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on restore

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: <5e4cd35df0f1c_404b2ac01efba5b442@john-XPS-13-9370.notmuch>
Date:   Tue, 18 Feb 2020 22:19:09 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>, bpf@...r.kernel.org
Cc:     netdev@...r.kernel.org, kernel-team@...udflare.com,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>
Subject: RE: [PATCH bpf-next 2/3] bpf, sk_msg: Don't clear saved sock proto on
 restore

Jakub Sitnicki wrote:
> There is no need to clear psock->sk_proto when restoring socket protocol
> callbacks in sk->sk_prot. The psock is about to get detached from the sock
> and eventually destroyed. At worst we will restore the protocol callbacks
> and the write callback twice.
> 
> This makes reasoning about psock state easier. Once psock is initialized,
> we can count on psock->sk_proto always being set.
> 
> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> ---
>  include/linux/skmsg.h | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 8605947d6c08..d90ef61712a1 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -359,13 +359,7 @@ static inline void sk_psock_restore_proto(struct sock *sk,
>  					  struct sk_psock *psock)
>  {
>  	sk->sk_prot->unhash = psock->saved_unhash;
> -
> -	if (psock->sk_proto) {
> -		tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);
> -		psock->sk_proto = NULL;
> -	} else {
> -		sk->sk_write_space = psock->saved_write_space;
> -	}
> +	tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);
>  }
>  
>  static inline void sk_psock_set_state(struct sk_psock *psock,
> -- 
> 2.24.1
> 

Agreed, also the next line in sk_psock_drop is to NULL user data so
the psock will no longer be attached as far as the sock is concerned.

Acked-by: John Fastabend <john.fastabend@...il.com>