netdev - Re: [PATCH bpf-next V5 3/5] bpf: add BPF-helper for MTU checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201102211034.563ef994@carbon>
Date:   Mon, 2 Nov 2020 21:10:34 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        brouer@...hat.com
Subject: Re: [PATCH bpf-next V5 3/5] bpf: add BPF-helper for MTU checking

On Mon, 02 Nov 2020 10:04:44 -0800
John Fastabend <john.fastabend@...il.com> wrote:

> > > > +
> > > > +	/*  Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */
> > > > +	if (flags & BPF_MTU_CHK_RELAX)
> > > > +		mtu += VLAN_HLEN;    
> > > 
> > > I'm trying to think about the use case where this might be used?
> > > Compared to just adjusting MTU in BPF program side as needed for
> > > packet encapsulation/headers/etc.  
> > 
> > As I wrote above, this were added because the kernels own forwarding
> > have this relaxation in it's checks (in is_skb_forwardable()).  I even
> > tried to dig through the history, introduced in [1] and copy-pasted
> > in[2].  And this seems to be a workaround, that have become standard,
> > that still have practical implications.
> > 
> > My practical experiments showed, that e.g. ixgbe driver with MTU=1500
> > (L3-size) will allow and fully send packets with 1504 (L3-size). But
> > i40e will not, and drops the packet in hardware/firmware step.  So,
> > what is the correct action, strict or relaxed?
> > 
> > My own conclusion is that we should inverse the flag.  Meaning to
> > default add this VLAN_HLEN (4 bytes) relaxation, and have a flag to do
> > more strict check,  e.g. BPF_MTU_CHK_STRICT. As for historical reasons
> > we must act like kernels version of MTU check. Unless you object, I will
> > do this in V6.  
> 
> I'm fine with it either way as long as its documented in the helper
> description so I have a chance of remembering this discussion in 6 months.
> But, if you make it default won't this break for XDP cases? I assume the
> XDP use case doesn't include the VLAN 4-bytes. Would you need to prevent
> the flag from being used from XDP?

XDP actually do include the VLAN_HLEN 4-bytes, see xdp_ok_fwd_dev(). I
was so certain that you John added this code, but looking through git
blame it pointed back to myself.  Going 5 levels git history deep and
3+ years, does seem like I move/reused some of Johns code containing
VLAN_HLEN in the MTU check, introduced for xdp-generic (6103aa96ec077)
which I acked.  Thus, I guess I cannot push this away and have to take
blame myself ;-)

I conclude that we default need to include this VLAN_HLEN, else the XDP
bpf_check_mtu could say deny, while it would have passed the check in
xdp_ok_fwd_dev().  As i40e will drop 1504 this at HW/FW level, I still
see a need for a BPF_MTU_CHK_STRICT flag for programs that want to
catch this.

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