| 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
| ||
|
Message-ID: <CAL+tcoAKyT6rdBxScEszjCw32XrsShci5a=a_FEg7fWB-ePV2Q@mail.gmail.com>
Date: Thu, 29 Aug 2024 18:02:45 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, dsahern@...nel.org, willemb@...gle.com
Cc: netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net-next v2 2/2] net: make SOF_TIMESTAMPING_RX_SOFTWARE
feature per socket
On Thu, Aug 29, 2024 at 12:01 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> From: Jason Xing <kernelxing@...cent.com>
>
> Like the previous patch in this series, we need to make sure that
> we both set SOF_TIMESTAMPING_SOFTWARE and SOF_TIMESTAMPING_RX_SOFTWARE
> flags together so that we can let the user parse the rx timestamp.
>
> One more important and special thing is that we should take care of
> errqueue recv path because we rely on errqueue to get our timestamps
> for sendmsg(). Or else, If the user wants to read when setting
> SOF_TIMESTAMPING_TX_ACK, something like this, we cannot get timestamps,
> for example, in TCP case. So we should consider those
> SOF_TIMESTAMPING_TX_* flags.
>
> After this patch, we are able to pass the testcase 6 for IP and UDP
> cases when running ./rxtimestamp binary.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
> Documentation/networking/timestamping.rst | 7 +++++++
> include/net/sock.h | 7 ++++---
> net/bluetooth/hci_sock.c | 4 ++--
> net/core/sock.c | 2 +-
> net/ipv4/ip_sockglue.c | 2 +-
> net/ipv4/ping.c | 2 +-
> net/ipv6/datagram.c | 4 ++--
> net/l2tp/l2tp_ip.c | 2 +-
> net/l2tp/l2tp_ip6.c | 2 +-
> net/nfc/llcp_sock.c | 2 +-
> net/rxrpc/recvmsg.c | 2 +-
> net/socket.c | 11 ++++++++---
> net/unix/af_unix.c | 2 +-
> 13 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index 5e93cd71f99f..93378b78c6dd 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -160,6 +160,13 @@ SOF_TIMESTAMPING_RAW_HARDWARE:
> Report hardware timestamps as generated by
> SOF_TIMESTAMPING_TX_HARDWARE when available.
>
> +Please note: previously, if an application starts first which turns on
> +netstamp_needed_key, then another one only passing SOF_TIMESTAMPING_SOFTWARE
> +could also get rx timestamp. Now we handle this case and will not get
> +rx timestamp under this circumstance. We encourage that for each socket
> +we should use the SOF_TIMESTAMPING_RX_SOFTWARE generation flag to time
> +stamp the skb and use SOF_TIMESTAMPING_SOFTWARE report flag to tell
> +the application.
>
> 1.3.3 Timestamp Options
> ^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/include/net/sock.h b/include/net/sock.h
> index cce23ac4d514..b8535692f340 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2600,12 +2600,13 @@ static inline void sock_write_timestamp(struct sock *sk, ktime_t kt)
> }
>
> void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> - struct sk_buff *skb);
> + struct sk_buff *skb, bool errqueue);
> void __sock_recv_wifi_status(struct msghdr *msg, struct sock *sk,
> struct sk_buff *skb);
>
> static inline void
> -sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
> +sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb,
> + bool errqueue)
> {
> struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
> u32 tsflags = READ_ONCE(sk->sk_tsflags);
> @@ -2621,7 +2622,7 @@ sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb)
> (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> (hwtstamps->hwtstamp &&
> (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> - __sock_recv_timestamp(msg, sk, skb);
> + __sock_recv_timestamp(msg, sk, skb, errqueue);
> else
> sock_write_timestamp(sk, kt);
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 69c2ba1e843e..c1b73c5a370b 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1586,11 +1586,11 @@ static int hci_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> break;
> case HCI_CHANNEL_USER:
> case HCI_CHANNEL_MONITOR:
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
> break;
> default:
> if (hci_mgmt_chan_find(hci_pi(sk)->channel))
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
> break;
> }
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 9abc4fe25953..d969a4901300 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3677,7 +3677,7 @@ int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,
> if (err)
> goto out_free_skb;
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, true);
>
> serr = SKB_EXT_ERR(skb);
> put_cmsg(msg, level, type, sizeof(serr->ee), &serr->ee);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index cf377377b52d..b79f859c34bf 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -547,7 +547,7 @@ int ip_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
> kfree_skb(skb);
> return err;
> }
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, true);
>
> serr = SKB_EXT_ERR(skb);
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 619ddc087957..1cf7b0eecd63 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -880,7 +880,7 @@ int ping_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags,
> if (err)
> goto done;
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
>
> /* Copy the address and add cmsg data. */
> if (family == AF_INET) {
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index fff78496803d..1e4c11b2d0ce 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -479,7 +479,7 @@ int ipv6_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
> kfree_skb(skb);
> return err;
> }
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, true);
>
> serr = SKB_EXT_ERR(skb);
>
> @@ -568,7 +568,7 @@ int ipv6_recv_rxpmtu(struct sock *sk, struct msghdr *msg, int len,
> if (err)
> goto out_free_skb;
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
>
> memcpy(&mtu_info, IP6CBMTU(skb), sizeof(mtu_info));
>
> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
> index 4bc24fddfd52..164c8ed7124e 100644
> --- a/net/l2tp/l2tp_ip.c
> +++ b/net/l2tp/l2tp_ip.c
> @@ -567,7 +567,7 @@ static int l2tp_ip_recvmsg(struct sock *sk, struct msghdr *msg,
> if (err)
> goto done;
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
>
> /* Copy the address. */
> if (sin) {
> diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
> index f4c1da070826..b0bb0a1f772e 100644
> --- a/net/l2tp/l2tp_ip6.c
> +++ b/net/l2tp/l2tp_ip6.c
> @@ -712,7 +712,7 @@ static int l2tp_ip6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> if (err)
> goto done;
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
>
> /* Copy the address. */
> if (lsa) {
> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 57a2f97004e1..5c6e671643f6 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -869,7 +869,7 @@ static int llcp_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> return -EFAULT;
> }
>
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
>
> if (sk->sk_type == SOCK_DGRAM && msg->msg_name) {
> struct nfc_llcp_ui_cb *ui_cb = nfc_llcp_ui_skb_cb(skb);
> diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
> index a482f88c5fc5..18fa392011fb 100644
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -200,7 +200,7 @@ static int rxrpc_recvmsg_data(struct socket *sock, struct rxrpc_call *call,
> sp->hdr.serial, seq);
>
> if (msg)
> - sock_recv_timestamp(msg, sock->sk, skb);
> + sock_recv_timestamp(msg, sock->sk, skb, false);
>
> if (rx_pkt_offset == 0) {
> ret2 = rxrpc_verify_data(call, skb);
> diff --git a/net/socket.c b/net/socket.c
> index fcbdd5bc47ac..c02fb9b615b2 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -893,7 +893,7 @@ static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
> * called from sock_recv_timestamp() if sock_flag(sk, SOCK_RCVTSTAMP)
> */
> void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> - struct sk_buff *skb)
> + struct sk_buff *skb, bool errqueue)
> {
> int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP);
> int new_tstamp = sock_flag(sk, SOCK_TSTAMP_NEW);
> @@ -946,7 +946,12 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>
> memset(&tss, 0, sizeof(tss));
> tsflags = READ_ONCE(sk->sk_tsflags);
> - if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
> + /* We have to use the generation flag here to test if we allow the
> + * corresponding application to receive the rx timestamp. Only
> + * using report flag does not hold for receive timestamping case.
> + */
> + if ((tsflags & SOF_TIMESTAMPING_SOFTWARE &&
> + (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE || errqueue)) &&
Hello Willem,
After considering this part implemented in sock_recv_timestamp() in
the previous version over and over again, I think I need to add back
what I removed in sock_recv_timestamp(), because:
supposing we only set SOF_TIMESTAMPING_SOFTWARE, we will go into
__sock_recv_timestamp and do nothing but return, then we will miss
setting sk->sk_stamp in sock_write_timestamp().
In that case, the socket will miss two chances to set sk_stamp.
sk_stamp stands for the timestamp of the last packet we receive, it is
necessary to set sk_stamp in sock_recv_timestamp() one way or another.
I wonder if I understand correctly?
Please also help me review the remaining code, thanks.
Thanks,
Jason
> ktime_to_timespec64_cond(skb->tstamp, tss.ts + 0))
> empty = 0;
> if (shhwtstamps &&
> @@ -1024,7 +1029,7 @@ static void sock_recv_mark(struct msghdr *msg, struct sock *sk,
> void __sock_recv_cmsgs(struct msghdr *msg, struct sock *sk,
> struct sk_buff *skb)
> {
> - sock_recv_timestamp(msg, sk, skb);
> + sock_recv_timestamp(msg, sk, skb, false);
> sock_recv_drops(msg, sk, skb);
> sock_recv_mark(msg, sk, skb);
> }
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index a1894019ebd5..bb33f2994618 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2481,7 +2481,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> goto out_free;
>
> if (sock_flag(sk, SOCK_RCVTSTAMP))
> - __sock_recv_timestamp(msg, sk, skb);
> + __sock_recv_timestamp(msg, sk, skb, false);
>
> memset(&scm, 0, sizeof(scm));
>
> --
> 2.37.3
>
Powered by blists - more mailing lists