Re: [PATCH net] tcp: Always cleanup skb before sending

[Christoph Paasch <cpaasch@apple.com>]

On 02/11/17 - 11:26:21, Eric Dumazet wrote:
> On Thu, Nov 2, 2017 at 11:24 AM, Christoph Paasch <cpaasch@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