netdev - Re: [net-next 2/2] tcp: Add receive timestamp support for receive zerocopy.

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

Message-ID: <20201214162547.110e7a5e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 14 Dec 2020 16:25:47 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Arjun Roy <arjunroy.kdev@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, arjunroy@...gle.com,
        edumazet@...gle.com, soheil@...gle.com
Subject: Re: [net-next 2/2] tcp: Add receive timestamp support for receive
 zerocopy.

On Fri, 11 Dec 2020 10:44:19 -0800 Arjun Roy wrote:
> From: Arjun Roy <arjunroy@...gle.com>
> 
> tcp_recvmsg() uses the CMSG mechanism to receive control information
> like packet receive timestamps. This patch adds CMSG fields to
> struct tcp_zerocopy_receive, and provides receive timestamps
> if available to the user.
> 
> Signed-off-by: Arjun Roy <arjunroy@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@...gle.com>

>  			      struct scm_timestamping_internal *tss,
>  			      int *cmsg_flags);
>  static int receive_fallback_to_copy(struct sock *sk,
> -				    struct tcp_zerocopy_receive *zc, int inq)
> +				    struct tcp_zerocopy_receive *zc, int inq,
> +				    struct scm_timestamping_internal *tss)
>  {
>  	unsigned long copy_address = (unsigned long)zc->copybuf_address;
> -	struct scm_timestamping_internal tss_unused;
> -	int err, cmsg_flags_unused;
> +	int err;
>  	struct msghdr msg = {};
>  	struct iovec iov;
>  

rev xmas tree

> @@ -1913,17 +1927,24 @@ static int tcp_zerocopy_handle_leftover_data(struct tcp_zerocopy_receive *zc,
>  					     struct sock *sk,
>  					     struct sk_buff *skb,
>  					     u32 *seq,
> -					     s32 copybuf_len)
> +					     s32 copybuf_len,
> +					     struct scm_timestamping_internal
> +						*tss)

I appreciate the attempt to make the code fit 80 chars, but this
particular wrapping does more harm than good.

> @@ -4116,6 +4138,30 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
>  		default:
>  			goto zerocopy_rcv_out;
>  		}
> +zerocopy_rcv_cmsg:
> +		if (zc.msg_flags & TCP_CMSG_TS) {
> +			unsigned long msg_control_addr;
> +			struct msghdr cmsg_dummy;
> +
> +			msg_control_addr = (unsigned long)zc.msg_control;
> +			cmsg_dummy.msg_control = (void *)msg_control_addr;
> +			cmsg_dummy.msg_controllen =
> +				(__kernel_size_t)zc.msg_controllen;
> +			cmsg_dummy.msg_flags = in_compat_syscall()
> +				? MSG_CMSG_COMPAT : 0;
> +			zc.msg_flags = 0;
> +			if (zc.msg_control == msg_control_addr &&
> +			    zc.msg_controllen == cmsg_dummy.msg_controllen) {
> +				tcp_recv_timestamp(&cmsg_dummy, sk, &tss);
> +				zc.msg_control = (__u64)
> +					((uintptr_t)cmsg_dummy.msg_control);
> +				zc.msg_controllen =
> +					(__u64)cmsg_dummy.msg_controllen;
> +				zc.msg_flags = (__u32)cmsg_dummy.msg_flags;

This is indented by 4 levels. Time to create a helper?

Do we really need to cast each of these assignments explicitly?