Re: Enable to send packets on if_loop via bpf

[Ryota Ozaki <ozaki.ryota%gmail.com@localhost>]

On Wed, Nov 9, 2022 at 9:21 PM Greg Troxel <gdt%lexort.com@localhost> wrote:
>
>
> Ryota Ozaki <ozaki.ryota%gmail.com@localhost> writes:
>
> > NetBSD can't do this because a loopback interface
> > registers itself to bpf as DLT_NULL and bpf treats
> > packets being sent over the interface as AF_UNSPEC.
> > Packets of AF_UNSPEC are just dropped by loopback
> > interfaces.
> >
> > FreeBSD and OpenBSD enable to do that by letting users
> > prepend a protocol family to a sending data.  bpf (or if_loop)
> > extracts it and handles the packet as an extracted protocol
> > family.  The following patch follows them (the implementation
> > is inspired by OpenBSD).
> >
> > http://www.netbsd.org/~ozaki-r/loop-bpf.patch
> >
> > The patch changes if_loop to register itself to bpf
> > as DLT_LOOP and bpf to handle a prepending protocol
> > family on bpf_write if a sender interface is DLT_LOOP.
>
> I am surprised that there is not already a DLT_foo that already has this
> concept, an AF word followed by data.  But I guess every interface
> already has a more-specific format.
>
> Looking at if_tun.c, I see DLT_NULL.  This should have the same ability
> to write.   I have forgotten the details of how tun encodes AF when
> transmitting, but I know you can have v4 or v6 inside, and tcpdump works
> now. so obviously I must be missing something.
>
> My suggestion is to look at the rest of the drivers that register
> DLT_NULL and see if they are amenable to the same fix, and choose a new
> DLT_SOMETHING that accomodates the broader situation.
>
> I am not demanding that you add features to the rest of the drivers.  I
> am only asking that you think about the architectural issue of how the
> rest of them would be updated, so we don't end up with DLT_LOOP,
> DLT_TUN, and so on, where they all do almost the same thing, when they
> could be the same.
>
> I don't really have an opinion on host vs network for AF, but I think
> your choice of aligning with FreeBSD is reasonable.

Thank you for your suggestion and I'm sorry for my late reply.

I've investigated the DLT specification(*), DLT_NULL users including tun
and the implementation of bpf and others.

(*) https://www.tcpdump.org/linktypes.html

At first, my patch was wrong because DLT_LOOP assumes a protocol family
in the network byte order.  So prepending a protocol family in
the host byte order was wrong and also changing DLT_LOOP broke mbuf
tapping on if_loop (i.e., tcpdump).


In the specification DLT_NULL assumes a protocol family in the host
byte order followed by a payload.  Interfaces of DLT_NULL uses
bpf_mtap_af to pass a mbuf prepending a protocol family.  All interfaces
follow the spec and work well.

OTOH, bpf_write to interfaces of DLT_NULL is a bit of a sad situation.
A writing data to an interface of DLT_NULL is treated as a raw data
(I don't know why); the data is passed to the interface's output routine
as is with dst (sa_family=AF_UNSPEC).  tun seems to be able
to handle such raw data but the others can't handle the data (probably
the data will be dropped like if_loop).


Correcting bpf_write to assume a prepending protocol family will
save some interfaces like gif and gre but won't save others like stf
and wg.  Even worse, the change may break existing users of tun
that want to treat data as is (though I don't know if users exist).

BTW, prepending a protocol family on tun is a different protocol from
DLT_NULL of bpf.  tun has three protocol modes and doesn't always prepend
a protocol family.  (And also the network byte order is used on tun
as gert says while DLT_NULL assumes the host byte order.)


So my fix will:
- keep DLT_NULL of if_loop to not break bpf_mtap_af, and
- unchange DLT_NULL handling in bpf_write except for if_loop to bother
existing users.
The patch looks like this:

@@ -447,6 +448,14 @@ bpf_movein(struct uio *uio, int linktype,
uint64_t mtu, struct mbuf **mp,
                m0->m_len -= hlen;
        }

+       if (linktype == DLT_NULL && ifp->if_type == IFT_LOOP) {
+               uint32_t af;
+               memcpy(&af, mtod(m0, void *), sizeof(af));
+               sockp->sa_family = af;
+               m0->m_data += sizeof(af);
+               m0->m_len -= sizeof(af);
+       }
+
        *mp = m0;
        return (0);


If we want to support another interface, we can add it to the condition.

Any comments?
  ozaki-r