From: Paolo Abeni <pabeni@redhat.com>
To: Yan Zhai <yan@cloudflare.com>, Jason Wang <jasowang@redhat.com>
Cc: "open list:NETWORKING [TCP]" <netdev@vger.kernel.org>,
kernel-team@cloudflare.com, Eric Dumazet <edumazet@google.com>,
"David S. Miller" <davem@davemloft.net>,
David Ahern <dsahern@kernel.org>,
Jakub Kicinski <kuba@kernel.org>,
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
Xin Long <lucien.xin@gmail.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
Andrew Melnychenko <andrew@daynix.com>,
Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
open list <linux-kernel@vger.kernel.org>,
"open list:SCTP PROTOCOL" <linux-sctp@vger.kernel.org>
Subject: Re: [PATCH net] gso: fix GSO_DODGY bit handling for related protocols
Date: Thu, 13 Jul 2023 09:11:44 +0200 [thread overview]
Message-ID: <31b64f509a9b4b8badf8925dddff4269ad572d39.camel@redhat.com> (raw)
In-Reply-To: <CAO3-PboQ1WL4wu+znnrF4kEdNnx42xPNJ_+Oc88bEejW2J-A+Q@mail.gmail.com>
On Wed, 2023-07-12 at 21:58 -0500, Yan Zhai wrote:
> On Wed, Jul 12, 2023 at 9:11 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@cloudflare.com> 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@cloudflare.com>
> > >
> > > ---
> > > [1]:
> > > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/
> > >
> > > ---
> > > 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?
As an optimization, I think it should land on a different (net-next)
patch. Additionally I think it should be possible to get a greater gain
adding the ROBUST feature to virtual devices (but I'm not sure if
syzkaller will be able to use that in nasty ways).
Cheers,
Paolo
prev parent reply other threads:[~2023-07-13 7:14 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-13 1:55 [PATCH net] gso: fix GSO_DODGY bit handling for related protocols Yan Zhai
2023-07-13 2:01 ` Willem de Bruijn
2023-07-13 2:11 ` Jason Wang
2023-07-13 2:57 ` Yan Zhai
2023-07-13 2:11 ` Jason Wang
2023-07-13 2:58 ` Yan Zhai
2023-07-13 7:11 ` Paolo Abeni [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=31b64f509a9b4b8badf8925dddff4269ad572d39.camel@redhat.com \
--to=pabeni@redhat.com \
--cc=andrew@daynix.com \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=herbert@gondor.apana.org.au \
--cc=jasowang@redhat.com \
--cc=kernel-team@cloudflare.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=lucien.xin@gmail.com \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=willemdebruijn.kernel@gmail.com \
--cc=yan@cloudflare.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).