Re: [RFC v2 net-next 01/10] net: Add a new socket option for a future transmit time.

Hi,


On 01/19/2018 01:15 PM, Willem de Bruijn wrote:

On Wed, Jan 17, 2018 at 6:06 PM, Jesus Sanchez-Palencia
[off-list ref] wrote:

quoted

From: Richard Cochran <redacted>

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).

A new field is added to struct sockcm_cookie, and the tstamp from
skbuffs will be used later on.

Signed-off-by: Richard Cochran <redacted>
Signed-off-by: Jesus Sanchez-Palencia <redacted>
---

quoted

diff --git a/net/core/sock.c b/net/core/sock.c
index abf4cbff99b2..37ef4b33fd92 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c

@@ -1061,6 +1061,13 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
                        sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool);
                break;

+       case SO_TXTIME:
+               if (ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
+                       sock_valbool_flag(sk, SOCK_TXTIME, valbool);
+               else
+                       ret = -EPERM;
+               break;
+
        default:
                ret = -ENOPROTOOPT;
                break;

Please add getsockopt alongside setsockopt.

I would also restrict input to [0, 1] exactly to allow for future extensions.

Ok, will do.

If using ns_capable, skb->tstamp must continue to be scrubbed when traversing
network namespaces.

I was planning to follow Eric's suggestion and move the tstamp scrubbing out of
skb_scrub_packet() into ____dev_forward_skb() instead. Would that break when
traversing namespaces?

quoted

@@ -2130,6 +2137,15 @@ 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 SO_TXTIME:
+               if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
+                       return -EPERM;
+               if (!sock_flag(sk, SOCK_TXTIME))
+                       return -EINVAL;

No need for ns_capable check on each packet when already required to
toggle socket option.

Ok. SO_MARK is doing the same so it might have "mis-inspired" me. I should
probably fix both.

quoted

+               if (cmsg->cmsg_len != CMSG_LEN(sizeof(ktime_t)))
+                       return -EINVAL;

I don't see any existing reference to ktime_t in include/uapi. Just use a s64?

Sure, will fix.


Thanks,
Jesus