Hi Eric,
Thanks for the comments. I should add "RFC" in subject next time for the
uncertain fix patch.
On Wed, Oct 07, 2020 at 11:35:41AM +0200, Eric Dumazet wrote:
>
>
> On 10/7/20 5:55 AM, Hangbin Liu wrote:
>
> > kfree_skb(skb);
> > @@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff
> > *skb, struct net_device *dev,
> > }
> > }
> >
> > + /* RFC 8200, Section 4.5 Fragment Header:
> > + * If the first fragment does not include all headers through an
> > + * Upper-Layer header, then that fragment should be discarded and
> > + * an ICMP Parameter Problem, Code 3, message should be sent to
> > + * the source of the fragment, with the Pointer field set to zero.
> > + */
> > + nexthdr = hdr->nexthdr;
> > + offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr,
> > &frag_off);
> > + if (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
> > + __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > + icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> > + rcu_read_unlock();
> > + return NULL;
> > + }
> > +
> > rcu_read_unlock();
> >
> > /* Must drop socket now because of tproxy. */
> >
>
> Ouch, this is quite a buggy patch.
>
> I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.
>
> Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?
Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6
defragment?
>
> Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be
> strange
> to force a pull of hundreds of bytes just because you want to check if an
> extra byte is there,
> if the packet could be forwarded as is, without additional memory allocations.
>
> Testing skb->len should be more than enough at this stage.
Ah, yes, I shouldn't call pskb_may_pull here.
>
> Also ipv6_skip_exthdr() can return an error.
it returns -1 as error, If we tested it by (offset + 1 > skb->len), does
that count as an error handler?
Thanks
Hangbin
