On Fri, Apr 17, 2015 at 12:34 PM, Jesse Gross <je...@nicira.com> wrote:
> On Thu, Apr 16, 2015 at 7:17 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> +static struct sk_buff *push_stt_header(struct sk_buff *head, __be64 tun_id,
>> +                                      __be16 s_port, __be16 d_port,
>> +                                      __be32 saddr, __be32 dst,
>> +                                      __be16 l3_proto, u8 l4_proto,
>> +                                      int dst_mtu)
>> +{
>> +       struct sk_buff *skb;
>> +
>> +       if (skb_shinfo(head)->frag_list) {
>> +               bool ipv4 = (l3_proto == htons(ETH_P_IP));
>> +               bool tcp = (l4_proto == IPPROTO_TCP);
>> +               int l4_offset = skb_transport_offset(head);
>> +
>> +               if (unlikely(segment_skb(&head, ipv4, tcp, l4_offset)))
>> +                       goto error;
>> +       }
>
> I don't know that it's necessarily guaranteed that we have all of this
> information in the transmit path, especially given that we pass in
> checksum partial as true later on. In the event that we have just a
> bare Ethernet packet, we might not know the transport offset or that
> the TCP header is fully formed.
>
Bare Ethernet packet will get caught in can_segment() and will be linearized.
I am setting checksum partial true since stt push header will set it
later on anyways.

>> +int stt_xmit_skb(struct sk_buff *skb, struct rtable *rt,
>> +                __be32 src, __be32 dst, __u8 tos,
>> +                __u8 ttl, __be16 df, __be16 src_port, __be16 dst_port,
>> +                __be64 tun_id)
> [...]
>> +       while (skb) {
>> +               struct sk_buff *next_skb = skb->next;
>> +
>> +               skb->next = NULL;
>> +
>> +               if (next_skb)
>> +                       dst_clone(&rt->dst);
>> +
>> +               /* Push STT and TCP header. */
>> +               skb = push_stt_header(skb, tun_id, src_port, dst_port, src,
>> +                                     dst, inner_l3_proto, inner_l4_proto,
>> +                                     dst_mtu(&rt->dst));
>> +               if (unlikely(!skb))
>> +                       goto next;
>
> Does this leak the dst reference on error?
>
ok.

>> +               /* Push IP header. */
>> +               ret += skb_list_xmit(rt, skb, src, dst, tos, ttl, df);
>
> I think that 'ret' is overloaded here as it contains both the return
> code from stt_can_offload() and a size from skb_list_xmit() without
> being cleared in between. Maybe use separate variables?

ok.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to