On Wed, Jul 12, 2023 at 9:11 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@xxxxxxxxxxxxxx> wrote:
> >
> > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source.
> > The canonical way is to recompute the gso_segs to avoid device driver
> > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the
> > egress of later devices, e.g. packets can egress to a vlan device backed
> > by a real NIC.
> >
> > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4
> > packets.") checks DODGY bit for UDP, but for packets that can be fed
> > directly to the device after gso_segs reset, it actually falls through
> > to fragmentation [1].
> >
> > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417
> > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY
> > bit after recomputing gso_segs.
>
> If we try to fix two issues, we'd better use separate patches.
>
> >
> > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit
> > at other places.
> >
> > Fixes: 90017accff61 ("sctp: Add GSO support")
> > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy")
> > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.")
> > Signed-off-by: Yan Zhai <yan@xxxxxxxxxxxxxx>
> >
> > ---
> > [1]:
> > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@xxxxxxxxxxxxxx/
> >
> > ---
> > net/ipv4/tcp_offload.c | 1 +
> > net/ipv4/udp_offload.c | 19 +++++++++++++++----
> > net/ipv6/udp_offload.c | 19 +++++++++++++++----
> > net/sctp/offload.c | 2 ++
> > 4 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 8311c38267b5..f9b93708c22e 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> > /* Packet is from an untrusted source, reset gso_segs. */
> >
> > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
> > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
> >
> > segs = NULL;
> > goto out;
> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> > index 75aa4de5b731..bd29cf19bb6b 100644
> > --- a/net/ipv4/udp_offload.c
> > +++ b/net/ipv4/udp_offload.c
> > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
> > if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> > goto out;
> >
> > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
> > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST))
> > - return __udp_gso_segment(skb, features, false);
> > -
> > mss = skb_shinfo(skb)->gso_size;
> > +
> > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
> > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
> > + /* Packet is from an untrusted source, reset actual gso_segs */
> > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh),
> > + mss);
> > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
> > +
> > + segs = NULL;
> > + goto out;
> > + } else {
> > + return __udp_gso_segment(skb, features, false);
>
> I think it's better and cleaner to move those changes in
> __udp_gso_segment() as Willem suggests.
>
> > + }
> > + }
> > +
> > if (unlikely(skb->len <= mss))
> > goto out;
> >
> > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
> > index ad3b8726873e..6857d9f7bd06 100644
> > --- a/net/ipv6/udp_offload.c
> > +++ b/net/ipv6/udp_offload.c
> > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
> > if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> > goto out;
> >
> > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 &&
> > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST))
> > - return __udp_gso_segment(skb, features, true);
> > -
> > mss = skb_shinfo(skb)->gso_size;
> > +
> > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
> > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
> > + /* Packet is from an untrusted source, reset actual gso_segs */
> > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh),
> > + mss);
> > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY;
>
> Any reason you want to remove the DODGY here? Is this an optimization?
> We will lose the chance to recognize/validate it elsewhere.
>
It is intended as a small optimization. And this is in fact the piece
I am not fully confident about: after validating the gso_segs at a
trusted location (i.e. assuming the kernel is the trusted computing
base), do we need to validate it somewhere else? For example, in our
scenario, we have a tun/tap device in a net namespace, so the packet
going out will enter from the tap, get forwarded through an veth, and
then a vlan backed by a real ethernet interface. If the bit is carried
over, then at each egress of these devices, we need to enter the GSO
code, which feels pretty redundant as long as the packet does not
leave kernel space. WDYT?
thanks
> Thanks
>
> > +
> > + segs = NULL;
> > + goto out;
> > + } else {
> > + return __udp_gso_segment(skb, features, true);
> > + }
> > + }
> > +
> > if (unlikely(skb->len <= mss))
> > goto out;
> >
> > diff --git a/net/sctp/offload.c b/net/sctp/offload.c
> > index 502095173d88..3d2b44db0d42 100644
> > --- a/net/sctp/offload.c
> > +++ b/net/sctp/offload.c
> > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
> > skb_walk_frags(skb, frag_iter)
> > pinfo->gso_segs++;
> >
> > + pinfo->gso_type &= ~SKB_GSO_DODGY;
> > +
> > segs = NULL;
> > goto out;
> > }
> > --
> > 2.30.2
> >
>
--
Yan
