Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Jesper Dangaard Brouer <brouer@redhat.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: "Daniel Borkmann" <daniel@iogearbox.net>,
	"Lorenz Bauer" <lmb@cloudflare.com>,
	"Maciej Żenczykowski" <maze@google.com>,
	"Saeed Mahameed" <saeed@kernel.org>,
	"Daniel Borkmann" <borkmann@iogearbox.net>,
	"Alexei Starovoitov" <alexei.starovoitov@gmail.com>,
	BPF-dev-list <bpf@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"Lorenzo Bianconi" <lorenzo.bianconi@redhat.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Shaun Crampton" <shaun@tigera.io>,
	"David Miller" <davem@davemloft.net>,
	"Marek Majkowski" <marek@cloudflare.com>,
	brouer@redhat.com
Subject: Re: BPF redirect API design issue for BPF-prog MTU feedback?
Date: Tue, 6 Oct 2020 13:45:35 +0200
Message-ID: <20201006134535.08e1dbe5@carbon> (raw)
In-Reply-To: <5f68eb19cc0a2_17370208c9@john-XPS-13-9370.notmuch>

On Mon, 21 Sep 2020 11:04:09 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> Daniel Borkmann wrote:
> > On 9/21/20 2:49 PM, Jesper Dangaard Brouer wrote:  
> > > On Mon, 21 Sep 2020 11:37:18 +0100
> > > Lorenz Bauer <lmb@cloudflare.com> wrote:  
> > >> On Sat, 19 Sep 2020 at 00:06, Maciej Żenczykowski <maze@google.com> wrote:  
> > >>>     
> > >>>> This is a good point.  As bpf_skb_adjust_room() can just be run after
> > >>>> bpf_redirect() call, then a MTU check in bpf_redirect() actually
> > >>>> doesn't make much sense.  As clever/bad BPF program can then avoid the
> > >>>> MTU check anyhow.  This basically means that we have to do the MTU
> > >>>> check (again) on kernel side anyhow to catch such clever/bad BPF
> > >>>> programs.  (And I don't like wasting cycles on doing the same check two
> > >>>> times).  
> > >>>
> > >>> If you get rid of the check in bpf_redirect() you might as well get
> > >>> rid of *all* the checks for excessive mtu in all the helpers that
> > >>> adjust packet size one way or another way.  They *all* then become
> > >>> useless overhead.
> > >>>
[...]
> > 
> > Sorry for jumping in late here... one thing that is not clear to me is that if
> > we are fully sure that skb is dropped by stack anyway due to invalid MTU (redirect
> > to ingress does this via dev_forward_skb(), it's not fully clear to me whether it's
> > also the case for the dev_queue_xmiy()), then why not dropping all the MTU checks
> > aside from SKB_MAX_ALLOC sanity check for BPF helpers and have something like a
> > device object (similar to e.g. TCP sockets) exposed to BPF prog where we can retrieve
> > the object and read dev->mtu from the prog, so the BPF program could then do the
> > "exception" handling internally w/o extra prog needed (we also already expose whether
> > skb is GSO or not).
> > 
> > Thanks,
> > Daniel  
> 
> My $.02 is MTU should only apply to transmitted packets so redirect to
> ingress should be OK. Then on transmit shouldn't the user know the MTU
> on their devices?

I like the point that "MTU should only apply to transmitted packets". 
 
> I'm for dropping all the MTU checks and if a driver tosses a packet then
> the user should be more careful. Having a bpf helper to check MTU of a
> dev seems useful although the workaround would be a map the user could
> put the max MTU in. Of course that would be a bit fragile if the BPF program
> and person managing MTU are not in-sync.

I'm coding this up. Dropping all the MTU checks in helpers, but adding
helper to lookup/check the MTU.  I've also extended the bpf_fib_lookup
to return MTU value (it already does MTU check), as it can be more
specific.

The problematic code path seems to be when TC-ingress redirect packet
to egress on another netdev, then the normal netstack MTU checks are
skipped and driver level will not catch any MTU violation (only checked
ixgbe code path).

First I looked at adding MTU check in the egress code path of
skb_do_redirect() prior to calling dev_queue_xmit(), but I found this
to be the wrong approach.  This is because it is still possible to run
another BPF egress program that will shrink/consume headers, which will
make packet comply with netdev MTU. This use-case might already be in
production use (allowed if ingress MTU is larger than egress MTU).

Instead I'm currently coding up doing the MTU check after
sch_handle_egress() step, for the cases that require this.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer


  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-17 12:38 Jesper Dangaard Brouer
2020-09-17 12:54 ` Maciej Żenczykowski
2020-09-17 19:11   ` Saeed Mahameed
2020-09-18 10:00     ` Jesper Dangaard Brouer
2020-09-18 10:34       ` Toke Høiland-Jørgensen
2020-09-18 23:06       ` Maciej Żenczykowski
2020-09-21 10:37         ` Lorenz Bauer
2020-09-21 12:49           ` Jesper Dangaard Brouer
2020-09-21 15:08             ` Daniel Borkmann
2020-09-21 16:21               ` Marek Zavodsky
2020-09-21 21:17                 ` Willem de Bruijn
2020-09-22  9:15                   ` Jesper Dangaard Brouer
2020-09-21 16:26               ` Jesper Dangaard Brouer
2020-09-22  6:56                 ` Eyal Birger
2020-09-21 18:04               ` John Fastabend
2020-10-06 11:45                 ` Jesper Dangaard Brouer [this message]
2020-09-21 10:42   ` Lorenz Bauer

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=20201006134535.08e1dbe5@carbon \
    --to=brouer@redhat.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=borkmann@iogearbox.net \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=john.fastabend@gmail.com \
    --cc=kuba@kernel.org \
    --cc=lmb@cloudflare.com \
    --cc=lorenzo.bianconi@redhat.com \
    --cc=marek@cloudflare.com \
    --cc=maze@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=saeed@kernel.org \
    --cc=shaun@tigera.io \
    /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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git