From mboxrd@z Thu Jan 1 00:00:00 1970 From: Willem de Bruijn Subject: Re: [PATCH v1 net-next 02/14] net: Add a new socket option for a future transmit time. Date: Thu, 28 Jun 2018 10:26:03 -0400 Message-ID: References: <20180627215950.6719-1-jesus.sanchez-palencia@intel.com> <20180627215950.6719-3-jesus.sanchez-palencia@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Network Development , Thomas Gleixner , jan.altenberg@linutronix.de, Vinicius Gomes , kurt.kanzenbach@linutronix.de, Henrik Austad , Richard Cochran , Levi Pearson , ilias.apalodimas@linaro.org, ivan.khoronzhuk@linaro.org, Miroslav Lichvar , Willem de Bruijn , Jamal Hadi Salim , Cong Wang , =?UTF-8?B?SmnFmcOtIFDDrXJrbw==?= , Richard Cochran To: Jesus Sanchez-Palencia Return-path: Received: from mail-io0-f195.google.com ([209.85.223.195]:44512 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965966AbeF1O0j (ORCPT ); Thu, 28 Jun 2018 10:26:39 -0400 Received: by mail-io0-f195.google.com with SMTP id g7-v6so5416230ioh.11 for ; Thu, 28 Jun 2018 07:26:39 -0700 (PDT) In-Reply-To: <20180627215950.6719-3-jesus.sanchez-palencia@intel.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jun 27, 2018 at 6:08 PM Jesus Sanchez-Palencia wrote: > > From: Richard Cochran > > This patch introduces SO_TXTIME. User space enables this option in > order to pass a desired future transmit time in a CMSG when calling > sendmsg(2). The argument to this socket option is a 6-bytes long struct > defined as: > > struct sock_txtime { > clockid_t clockid; > u16 flags; > }; clockid_t is __kernel_clockid_t is int is a variable length field. Please use fixed length fields. Also, as MAX_CLOCKS is 16, only 4 bits are needed. A single u16 is probably sufficient as cmsg argument. To future proof, a u32 will allow for more than 4 flags. But in struct sock, 16 bits should be sufficient to encode both clock id and flags. > Note that two new fields were added to struct sock by filling a 4-bytes > hole found in the struct. For that reason, neither the struct size or > number of cachelines were altered. > > Signed-off-by: Richard Cochran > Signed-off-by: Jesus Sanchez-Palencia > --- > +#include > #include > #include > #include > @@ -697,6 +698,7 @@ EXPORT_SYMBOL(sk_mc_loop); > int sock_setsockopt(struct socket *sock, int level, int optname, > char __user *optval, unsigned int optlen) > { > + struct sock_txtime sk_txtime; > struct sock *sk = sock->sk; > int val; > int valbool; > @@ -1070,6 +1072,22 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > } > break; > > + case SO_TXTIME: > + if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) { > + ret = -EPERM; > + } else if (optlen != sizeof(struct sock_txtime)) { > + ret = -EINVAL; > + } else if (copy_from_user(&sk_txtime, optval, > + sizeof(struct sock_txtime))) { > + ret = -EFAULT; > + sock_valbool_flag(sk, SOCK_TXTIME, false); Why change sk state on failure? This is not customary. > + } else { > + sock_valbool_flag(sk, SOCK_TXTIME, true); > + sk->sk_clockid = sk_txtime.clockid; > + sk->sk_txtime_flags = sk_txtime.flags; Validate input and fail on undefined flags. > @@ -2137,6 +2162,13 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg, > sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK; > sockc->tsflags |= tsflags; > break; > + case SCM_TXTIME: > + if (!sock_flag(sk, SOCK_TXTIME)) > + return -EINVAL; Note that on lockfree datapaths like udp this test can race with the setsockopt above. It seems harmless here. > + if (cmsg->cmsg_len != CMSG_LEN(sizeof(u64))) > + return -EINVAL; > + sockc->transmit_time = get_unaligned((u64 *)CMSG_DATA(cmsg)); > + break; > /* SCM_RIGHTS and SCM_CREDENTIALS are semantically in SOL_UNIX. */ > case SCM_RIGHTS: > case SCM_CREDENTIALS: > -- > 2.17.1 >