Re: [PATCH v1 net-next 02/14] net: Add a new socket option for a future transmit time.

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Wed, Jun 27, 2018 at 6:08 PM Jesus Sanchez-Palencia
<jesus.sanchez-palencia@intel.com> wrote:
>
> From: Richard Cochran <rcochran@linutronix.de>
>
> 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 <rcochran@linutronix.de>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> ---

> +#include <asm/unaligned.h>
>  #include <linux/capability.h>
>  #include <linux/errno.h>
>  #include <linux/errqueue.h>
> @@ -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
>