| Message ID | 1416938286-14147-2-git-send-email-willemb@google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Willem de Bruijn <willemb@google.com> Date: Tue, 25 Nov 2014 12:58:03 -0500 > From: Willem de Bruijn <willemb@google.com> > > When returning timestamped packets on the error queue, only return > the data that the application initially sent: not the protocol > headers. > > This changes the ABI. The TCP interface is new enough that it should > be safe to do so. The UDP interface could be changed analogously with > > + else if (sk->sk_protocol == IPPROTO_UDP) > + skb_pull(skb, skb_transport_offset(skb) + sizeof(struct udphdr)); > > Tested with Documentation/networking/timestamping/txtimestamp -l 60 -x > > Signed-off-by: Willem de Bruijn <willemb@google.com> What's the harm in exposing the headers? Either it's harmful, and therefore doing so for UDP is bad too, or it's harmless and we should probably leave it alone to not risk breaking anyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 25, 2014 at 1:42 PM, David Miller <davem@davemloft.net> wrote: > From: Willem de Bruijn <willemb@google.com> > Date: Tue, 25 Nov 2014 12:58:03 -0500 > >> From: Willem de Bruijn <willemb@google.com> >> >> When returning timestamped packets on the error queue, only return >> the data that the application initially sent: not the protocol >> headers. >> >> This changes the ABI. The TCP interface is new enough that it should >> be safe to do so. The UDP interface could be changed analogously with >> >> + else if (sk->sk_protocol == IPPROTO_UDP) >> + skb_pull(skb, skb_transport_offset(skb) + sizeof(struct udphdr)); >> >> Tested with Documentation/networking/timestamping/txtimestamp -l 60 -x >> >> Signed-off-by: Willem de Bruijn <willemb@google.com> > > What's the harm in exposing the headers? Either it's harmful, and > therefore doing so for UDP is bad too, or it's harmless and Headers may expose information not available otherwise. I don't immediately see critical problems, but that does not mean that they might not lurk there. We so far avoid exposing the sequence number, though keeping it hidden is more about third parties. More in general, unprivileged processes may start requesting timestamps only to learn tcp state that they should either get from tcpinfo or cannot currently get at all, likely for good reason. A far-fetched example is identifying admin iptables tos mangling rules by reading the tos bits at the driver layer. At least on my machine, iptables -L is privileged. > we should probably leave it alone to not risk breaking anyone. That's fair. I sent it for rfc first for that reason. I won't resubmit unless more serious concerns are raised. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Willem de Bruijn <willemb@google.com> Date: Tue, 25 Nov 2014 14:52:00 -0500 > On Tue, Nov 25, 2014 at 1:42 PM, David Miller <davem@davemloft.net> wrote: >> From: Willem de Bruijn <willemb@google.com> >> Date: Tue, 25 Nov 2014 12:58:03 -0500 >> >> What's the harm in exposing the headers? Either it's harmful, and >> therefore doing so for UDP is bad too, or it's harmless and > > Headers may expose information not available otherwise. I don't > immediately see critical problems, but that does not mean that they > might not lurk there. > > We so far avoid exposing the sequence number, though keeping it hidden > is more about third parties. More in general, unprivileged processes > may start requesting timestamps only to learn tcp state that they > should either get from tcpinfo or cannot currently get at all, likely > for good reason. A far-fetched example is identifying admin iptables > tos mangling rules by reading the tos bits at the driver layer. At least > on my machine, iptables -L is privileged. > >> we should probably leave it alone to not risk breaking anyone. > > That's fair. I sent it for rfc first for that reason. I won't resubmit > unless more serious concerns are raised. I just worry about the potential breakage. Your concerns are valid... I honestly don't know what we should do here. Both choices have merit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 25, 2014 at 11:54 AM, David Miller <davem@davemloft.net> wrote: > From: Willem de Bruijn <willemb@google.com> > Date: Tue, 25 Nov 2014 14:52:00 -0500 > >> On Tue, Nov 25, 2014 at 1:42 PM, David Miller <davem@davemloft.net> wrote: >>> From: Willem de Bruijn <willemb@google.com> >>> Date: Tue, 25 Nov 2014 12:58:03 -0500 >>> >>> What's the harm in exposing the headers? Either it's harmful, and >>> therefore doing so for UDP is bad too, or it's harmless and >> >> Headers may expose information not available otherwise. I don't >> immediately see critical problems, but that does not mean that they >> might not lurk there. >> >> We so far avoid exposing the sequence number, though keeping it hidden >> is more about third parties. More in general, unprivileged processes >> may start requesting timestamps only to learn tcp state that they >> should either get from tcpinfo or cannot currently get at all, likely >> for good reason. A far-fetched example is identifying admin iptables >> tos mangling rules by reading the tos bits at the driver layer. At least >> on my machine, iptables -L is privileged. >> >>> we should probably leave it alone to not risk breaking anyone. >> >> That's fair. I sent it for rfc first for that reason. I won't resubmit >> unless more serious concerns are raised. > > I just worry about the potential breakage. > > Your concerns are valid... I honestly don't know what we should do here. > Both choices have merit. Here's a scenario in which giving the headers might be dangerous: Suppose I create a network namespace that's designed to contain something, e.g. a Tor or Tor-like client, that shouldn't know any of its public addressing information. I might assign something like a tunnel interface to the namespace, but, if the contained code can get lower-level headers, it might learn something that would identify the *other* end of the tunnel, which wouldn't be so good. Admittedly, this would be just one of several things that would require care to get this right. Also, what happens if the output is transformed by ipsec? Does the timestamp message show the ciphertext? TBH, I'd rather send no payload at all and have an scm message that the sender provides that specifies a cookie identifying the particular sent data. But that ship mostly sailed awhile ago. For bytestreams, though, isn't this all new in 3.18? Or am I off by a release. --Andy
On Tue, Nov 25, 2014 at 4:39 PM, Andy Lutomirski <luto@amacapital.net> wrote: > On Tue, Nov 25, 2014 at 11:54 AM, David Miller <davem@davemloft.net> wrote: >> From: Willem de Bruijn <willemb@google.com> >> Date: Tue, 25 Nov 2014 14:52:00 -0500 >> >>> On Tue, Nov 25, 2014 at 1:42 PM, David Miller <davem@davemloft.net> wrote: >>>> From: Willem de Bruijn <willemb@google.com> >>>> Date: Tue, 25 Nov 2014 12:58:03 -0500 >>>> >>>> What's the harm in exposing the headers? Either it's harmful, and >>>> therefore doing so for UDP is bad too, or it's harmless and >>> >>> Headers may expose information not available otherwise. I don't >>> immediately see critical problems, but that does not mean that they >>> might not lurk there. >>> >>> We so far avoid exposing the sequence number, though keeping it hidden >>> is more about third parties. More in general, unprivileged processes >>> may start requesting timestamps only to learn tcp state that they >>> should either get from tcpinfo or cannot currently get at all, likely >>> for good reason. A far-fetched example is identifying admin iptables >>> tos mangling rules by reading the tos bits at the driver layer. At least >>> on my machine, iptables -L is privileged. >>> >>>> we should probably leave it alone to not risk breaking anyone. >>> >>> That's fair. I sent it for rfc first for that reason. I won't resubmit >>> unless more serious concerns are raised. >> >> I just worry about the potential breakage. >> >> Your concerns are valid... I honestly don't know what we should do here. >> Both choices have merit. > > Here's a scenario in which giving the headers might be dangerous: > > Suppose I create a network namespace that's designed to contain > something, e.g. a Tor or Tor-like client, that shouldn't know any of > its public addressing information. I might assign something like a > tunnel interface to the namespace, but, if the contained code can get > lower-level headers, it might learn something that would identify the > *other* end of the tunnel, which wouldn't be so good. Admittedly, > this would be just one of several things that would require care to > get this right. network namespaces are an interesting case, indeed. > > Also, what happens if the output is transformed by ipsec? Does the > timestamp message show the ciphertext? > > TBH, I'd rather send no payload at all and have an scm message that > the sender provides that specifies a cookie identifying the particular > sent data. But that ship mostly sailed awhile ago. > > For bytestreams, though, isn't this all new in 3.18? Or am I off by a release. It was added in 3.17. That is still very recent. One third option, though hardly pretty, is to put display of headers under administrator control. An application cannot easily infer whether headers are stripped, and legacy applications do not even know to try. So, this is a bit too crude: + if (sk->sk_protocol == IPPROTO_TCP && sysctl_net_blind_errqueue) + skb_pull(skb, skb_transport_offset(skb) + tcp_hdrlen(skb)); + else if (sk->sk_protocol == IPPROTO_UDP && sysctl_net_blind_errqueue >= 2) + skb_pull(skb, skb_transport_offset(skb) + sizeof(struct udphdr)); An alternative is to add a timestamping option to skip headers (or even full payload, basically http://patchwork.ozlabs.org/patch/366967/) and give the administrator a sysctl to drop all requests that do not pass this flag. The intent is that future proof applications will start requesting the flag, and relying on the ts counter. Hardened installations can set the sysctl from the start, accepting possible breakage. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 26, 2014 at 1:03 PM, Willem de Bruijn <willemb@google.com> wrote: > On Tue, Nov 25, 2014 at 4:39 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> On Tue, Nov 25, 2014 at 11:54 AM, David Miller <davem@davemloft.net> wrote: >>> From: Willem de Bruijn <willemb@google.com> >>> Date: Tue, 25 Nov 2014 14:52:00 -0500 >>> >>>> On Tue, Nov 25, 2014 at 1:42 PM, David Miller <davem@davemloft.net> wrote: >>>>> From: Willem de Bruijn <willemb@google.com> >>>>> Date: Tue, 25 Nov 2014 12:58:03 -0500 >>>>> >>>>> What's the harm in exposing the headers? Either it's harmful, and >>>>> therefore doing so for UDP is bad too, or it's harmless and >>>> >>>> Headers may expose information not available otherwise. I don't >>>> immediately see critical problems, but that does not mean that they >>>> might not lurk there. >>>> >>>> We so far avoid exposing the sequence number, though keeping it hidden >>>> is more about third parties. More in general, unprivileged processes >>>> may start requesting timestamps only to learn tcp state that they >>>> should either get from tcpinfo or cannot currently get at all, likely >>>> for good reason. A far-fetched example is identifying admin iptables >>>> tos mangling rules by reading the tos bits at the driver layer. At least >>>> on my machine, iptables -L is privileged. >>>> >>>>> we should probably leave it alone to not risk breaking anyone. >>>> >>>> That's fair. I sent it for rfc first for that reason. I won't resubmit >>>> unless more serious concerns are raised. >>> >>> I just worry about the potential breakage. >>> >>> Your concerns are valid... I honestly don't know what we should do here. >>> Both choices have merit. >> >> Here's a scenario in which giving the headers might be dangerous: >> >> Suppose I create a network namespace that's designed to contain >> something, e.g. a Tor or Tor-like client, that shouldn't know any of >> its public addressing information. I might assign something like a >> tunnel interface to the namespace, but, if the contained code can get >> lower-level headers, it might learn something that would identify the >> *other* end of the tunnel, which wouldn't be so good. Admittedly, >> this would be just one of several things that would require care to >> get this right. > > network namespaces are an interesting case, indeed. > >> >> Also, what happens if the output is transformed by ipsec? Does the >> timestamp message show the ciphertext? >> >> TBH, I'd rather send no payload at all and have an scm message that >> the sender provides that specifies a cookie identifying the particular >> sent data. But that ship mostly sailed awhile ago. >> >> For bytestreams, though, isn't this all new in 3.18? Or am I off by a release. > > It was added in 3.17. That is still very recent. > > One third option, though hardly pretty, is to put display of headers > under administrator control. An application cannot easily infer whether > headers are stripped, and legacy applications do not even know to try. > So, this is a bit too crude: > > + if (sk->sk_protocol == IPPROTO_TCP && sysctl_net_blind_errqueue) > + skb_pull(skb, skb_transport_offset(skb) + tcp_hdrlen(skb)); > + else if (sk->sk_protocol == IPPROTO_UDP && sysctl_net_blind_errqueue >= 2) > + skb_pull(skb, skb_transport_offset(skb) + sizeof(struct udphdr)); > > An alternative is to add a timestamping option to skip headers (or > even full payload, basically > http://patchwork.ozlabs.org/patch/366967/) and give the administrator > a sysctl to drop all requests that do not pass this flag. The intent > is that future proof applications will start requesting the flag, and > relying on the ts counter. Hardened installations can set the sysctl > from the start, accepting possible breakage. Is there any reason to believe that unconditionally dropping the headers would break anything? I find it a bit hard to believe that anyone has actually implemented logic to figure out *what* L2 header type should be decoded and decode it. I can imagine that someone has hardcoded an assumption that the underlying interface is Ethernet, but there's still the whole pile of vlan, random datacenter encapsulation protocols and such to worry about. --Andy
On Wed, Nov 26, 2014 at 04:36:39PM -0800, Andy Lutomirski wrote: > Is there any reason to believe that unconditionally dropping the > headers would break anything? I find it a bit hard to believe that > anyone has actually implemented logic to figure out *what* L2 header > type should be decoded and decode it. Documentation/networking/timestamping/timestamping.c else if (!memcmp(sync, data + res - sizeof(sync), sizeof(sync))) printf(" => GOT OUR DATA BACK (HURRAY!)"); The example program looks from the end of the buffer, ignoring the lower headers. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 92116df..70a8596 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3580,6 +3580,7 @@ static void __skb_complete_tx_timestamp(struct sk_buff *skb, int tstype) { struct sock_exterr_skb *serr; + bool is_tcp = sk->sk_protocol == IPPROTO_TCP; int err; serr = SKB_EXT_ERR(skb); @@ -3589,10 +3590,13 @@ static void __skb_complete_tx_timestamp(struct sk_buff *skb, serr->ee.ee_info = tstype; if (sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID) { serr->ee.ee_data = skb_shinfo(skb)->tskey; - if (sk->sk_protocol == IPPROTO_TCP) + if (is_tcp) serr->ee.ee_data -= sk->sk_tskey; } + if (is_tcp) + skb_pull(skb, skb_transport_offset(skb) + tcp_hdrlen(skb)); + err = sock_queue_err_skb(sk, skb); if (err)