On 02/11/17 - 11:26:21, Eric Dumazet wrote: > On Thu, Nov 2, 2017 at 11:24 AM, Christoph Paasch <cpaa...@apple.com> wrote: > > Hi Eric, > > > > On 01/11/17 - 19:21:31, Eric Dumazet wrote: > >> On Wed, 2017-11-01 at 18:00 -0700, Eric Dumazet wrote: > >> > On Wed, 2017-11-01 at 17:10 -0700, Christoph Paasch wrote: > >> > > >> > > Yes, that looks good to me. Thanks! > >> > > > >> > > But we still need to clean up the skb in tcp_v4_send_reset and > >> > > tcp_v4_send_ack, as the fields (e.g., tcp_skb_cb->seq) have been set > >> > > when > >> > > coming from tcp_v4_rcv. > >> > > >> > You might be confused : ip_send_unicast_reply() does not send back the > >> > incoming skb. > >> > > >> > A fresh skb is allocated, then appended/sent. > >> > > >> > And commit 24a2d43d8886f5a29c did the changes to provide to > >> > __ip_options_echo() the proper IPCB header location. > >> > > >> > >> More details : > >> > >> Fields written by tcp_init_nondata_skb() on the synack packet : > >> > >> ->seq (32bits) at offset 0 of skb->cb[] > >> ->end_seq (32bits) at offset 4 of skb->cb[] > >> ->tcp_gso_segs (16bits) at offset 8 > >> ->tcp_flags (8bits) at offset 12 value (TCPHDR_SYN | TCPHDR_ACK -> > >> 0x12) > > > > In my build I see tcp_flags at offset 16, because ktime_t is 64 bits. > > Yes, but this ktime was removed in net-next for my rb-tree work. > > > > >> ->sacked (8bits) at offset 13 > >> > >> IPCB fields sharing these 14 bytes : > >> > >> iif /* 32bits, offset 0 */ > >> opt.faddr (32bits) offset 4 > >> opt.nexthop (32bits) offset 8 /* value 1 */ > >> opt.optlen (8bits) offset 12 /* value 0x12 */ > >> opt.srr (8bits) offset 13 > >> > >> IP6CB fields sharing these 14 bytes : > >> > >> iif /* 32bits, offset 0 */ > >> ra /* 16 bits, offset 4 */ > >> dst0 /* 16 bits offset 6 */ > >> srcrt /* 16 bits offset 8 */ -> 0x0001 > >> dst1 /* 16 bits offset 10 */ (not mangled -> 0) > >> lastopt /* 16 bits offset 12 */ -> 0x12 > > > > Thus, because what I mention above, we are writing here into flags which > > sits > > at offset 16. > > > > So, IP6SKB_FORWARDED and IP6SKB_FRAGMENTED will be set on the outgoing skb. > > Especially for IP6SKB_FORWARDED, which is checked at ip6_finish_output2() I > > am concerned. Even in general, having these fields set looks brittle to me. > > > > What do you think? > > I think I will submit my patch, which should clear the issue, without > breaking IPv4 options handling as your patch did ;)
Sounds good! :) Thanks for your feedback. Christoph
