Willem de Bruijn wrote:
> Richard Gobert wrote:
> > Commits a602456 ("udp: Add GRO functions to UDP socket") and 57c67ff ("udp:
> > additional GRO support") introduce incorrect usage of {ip,ipv6}_hdr in the
> > complete phase of gro. The functions always return skb->network_header,
> > which in the case of encapsulated packets at the gro complete phase, is
> > always set to the innermost L3 of the packet. That means that calling
> > {ip,ipv6}_hdr for skbs which completed the GRO receive phase (both in
> > gro_list and *_gro_complete) when parsing an encapsulated packet's _outer_
> > L3/L4 may return an unexpected value.
> >
> > This incorrect usage leads to a bug in GRO's UDP socket lookup.
> > udp{4,6}_lib_lookup_skb functions use ip_hdr/ipv6_hdr respectively. These
> > *_hdr functions return network_header which will point to the innermost L3,
> > resulting in the wrong offset being used in __udp{4,6}_lib_lookup with
> > encapsulated packets.
> >
> > To fix this issue p_off param is used in *_gro_complete to pass off the
> > offset of the previous layer.
>
> What exactly does this mean?
>
> This patch changes the definition of gro_complete to add a thoff
> alongside the existing "nhoff"..
>
> > - int (*gro_complete)(struct sk_buff *skb, int nhoff);
> > + int (*gro_complete)(struct sk_buff *skb, int nhoff,
> > + int thoff);
>
> .. but also fixes up implementations to interpret the existing
> argument as a thoff
>
> > -INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff)
> > +INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int nhoff,
> > + int thoff)
> > {
> > - const struct iphdr *iph = ip_hdr(skb);
> > - struct tcphdr *th = tcp_hdr(skb);
> > + const struct iphdr *iph = (const struct iphdr *)(skb->data + nhoff);
> > + struct tcphdr *th = (struct tcphdr *)(skb->data + thoff);
>
> But in some cases the new argument is not nhoff but p_off, e.g.,
>
> > static int geneve_gro_complete(struct sock *sk, struct sk_buff *skb,
> > - int nhoff)
> > + int p_off, int nhoff)
>
> Really, the argument is the start of the next header, each callback
> just casts to its expected header (ethhdr, tcphdr, etc.)
>
> The only place where we need to pass an extra argument is in udp,
> because that needs a pointer to the network header right before the
> transport header pointed to by nhoff.
>
> And only due to possible IPv4 options or IPv6 extension headers, we
> cannot just do
>
> + struct udphdr *iph = (struct iphdr *)(skb->data + nhoff - sizeof(*iph));
> struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>
> I also do not immediately see an a way to avoid all the boilerplate
> of a new argument in every callback. Aside from a per_cpu var -- but
> that is excessive.
>
> But it can just be left zero in all callsites, except for
> inet_gro_complete/ipv6_gro_complete, which pass in nhoff.
Actually, we can avoid the boilerplate changes that add an extra arg.
By changing the contract between network layer callbacks
(inet_gro_complete/ipv6_gro_complete) and transport layer callbacks
(tcp4_gro_complete et al).
If the first pass their own unmodified offset, nhoff:
err = INDIRECT_CALL_2(ops->callbacks.gro_complete,
tcp4_gro_complete, udp4_gro_complete,
- skb, nhoff + sizeof(*iph));
+ skb, nhoff);
And the latter parse the network header for total_len/payload_len, to
find their original offset.
It's also a bit of a hack. But a lot smaller patch, probably.
