Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <ZRcOXJ0pkuph6fko@debian.debian> <20230930110854.GA13787@breakpoint.cc>
In-Reply-To: <20230930110854.GA13787@breakpoint.cc>
From:   Yan Zhai <yan@cloudflare.com>
Date:   Mon, 2 Oct 2023 10:47:35 -0500
Message-ID: <CAO3-Pbp2onn+EUhKRrB5an_tyzLcaH+1FUqrThsxXqqpBAxshA@mail.gmail.com>
Subject: Re: [PATCH net] ipv6: avoid atomic fragment on GSO packets
To:     Florian Westphal <fw@strlen.de>
Cc:     netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        David Ahern <dsahern@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Aya Levin <ayal@nvidia.com>,
        Tariq Toukan <tariqt@nvidia.com>, linux-kernel@vger.kernel.org,
        kernel-team@cloudflare.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Sep 30, 2023 at 6:09=E2=80=AFAM Florian Westphal <fw@strlen.de> wro=
te:
>
> Yan Zhai <yan@cloudflare.com> wrote:
> > GSO packets can contain a trailing segment that is smaller than
> > gso_size. When examining the dst MTU for such packet, if its gso_size
> > is too large, then all segments would be fragmented. However, there is =
a
> > good chance the trailing segment has smaller actual size than both
> > gso_size as well as the MTU, which leads to an "atomic fragment".
> > RFC-8021 explicitly recommend to deprecate such use case. An Existing
> > report from APNIC also shows that atomic fragments can be dropped
> > unexpectedly along the path [1].
> >
> > Add an extra check in ip6_fragment to catch all possible generation of
> > atomic fragments. Skip atomic header if it is called on a packet no
> > larger than MTU.
> >
> > Link: https://www.potaroo.net/presentations/2022-03-01-ipv6-frag.pdf [1=
]
> > Fixes: b210de4f8c97 ("net: ipv6: Validate GSO SKB before finish IPv6 pr=
ocessing")
> > Reported-by: David Wragg <dwragg@cloudflare.com>
> > Signed-off-by: Yan Zhai <yan@cloudflare.com>
> > ---
> >  net/ipv6/ip6_output.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> > index 951ba8089b5b..42f5f68a6e24 100644
> > --- a/net/ipv6/ip6_output.c
> > +++ b/net/ipv6/ip6_output.c
> > @@ -854,6 +854,13 @@ int ip6_fragment(struct net *net, struct sock *sk,=
 struct sk_buff *skb,
> >       __be32 frag_id;
> >       u8 *prevhdr, nexthdr =3D 0;
> >
> > +     /* RFC-8021 recommended atomic fragments to be deprecated. Double=
 check
> > +      * the actual packet size before fragment it.
> > +      */
> > +     mtu =3D ip6_skb_dst_mtu(skb);
> > +     if (unlikely(skb->len <=3D mtu))
> > +             return output(net, sk, skb);
> > +
>
> This helper is also called for skbs where IP6CB(skb)->frag_max_size
> exceeds the MTU, so this check looks wrong to me.
>
> Same remark for dst_allfrag() check in __ip6_finish_output(),
> after this patch, it would be ignored.
>
Thanks for covering my carelessness. I was just considering the GSO
case so frag_max_size was overlooked. dst_allfrag is indeed a case
based on the code logic. But just out of curiosity, do we still see
any use of this feature? From commit messages it is set when PMTU
values signals smaller than min IPv6 MTU. But such PMTU values are
just dropped in __ip6_rt_update_pmtu now. Iproute2 code also does not
provide this route feature anymore. So it might be actually a dead
check?

> I think you should consider to first refactor __ip6_finish_output to make
> the existing checks more readable (e.g. handle gso vs. non-gso in separat=
e
> branches) and then add the check to last seg in
> ip6_finish_output_gso_slowpath_drop().
>
Agree with refactoring to mirror what IPv4 code is doing. It might not
hurt if we check every segments in this case, since it is already the
slowpath and it will make code more compact.

> Alternatively you might be able to pass more info down to
> ip6_fragment and move decisions there.
>
> In any case we should make same frag-or-no-frag decisions,
> regardless of this being the orig skb or a segmented one,