On Thu, Jul 30, 2026 at 11:32:09PM +0200, Samuel Thibault wrote:
> Hello,
>
> This looks nice :)
>
> Ralph Ronnquist, le jeu. 30 juil. 2026 19:57:02 +1000, a ecrit:
> > ---
> > pfinet/linux-src/include/linux/if_ether.h | 44 +++++++++++++
> > pfinet/linux-src/net/core/dev.c | 4 +-
> > pfinet/main.c | 2 +
> > pfinet/tunnel.c | 77 ++++++++++++++++++-----
> > 4 files changed, 110 insertions(+), 17 deletions(-)
> >
> > diff --git a/pfinet/linux-src/include/linux/if_ether.h
> > b/pfinet/linux-src/include/linux/if_ether.h
> > index 99bb97fa2..efb566e05 100644
> > --- a/pfinet/linux-src/include/linux/if_ether.h
> > +++ b/pfinet/linux-src/include/linux/if_ether.h
> > @@ -87,6 +87,50 @@ struct ethhdr
> > unsigned short h_proto; /* packet type ID field */
> > };
> >
> > +/* Alternative Ethernet header with VLAN tagging. The header without
> > + VLAN tagging (the original header) is found in <net/ethernet.h>.
> > + This header modelling accounts for packets optionally having VLAN
> > + tagging inserted before the ether_type field.
> > +*/
> > +
> > +struct ethhdr_vlan
> > +{
> > + uint8_t h_dest[ETH_ALEN]; /* destination eth addr */
>
> Better make this header include <stdint.h> to make sure to have the
> definition of uint8_t etc.
>
> > + uint8_t h_source[ETH_ALEN]; /* source ether addr */
> > + uint16_t h_vlan_tag; /* htons( 0x8100 ) */
> > + uint16_t h_vlan_id; /* VLAN ID */
> > + uint16_t h_proto; /* packet type ID field */
> > +};
> > +
> > diff --git a/pfinet/linux-src/net/core/dev.c
> > b/pfinet/linux-src/net/core/dev.c
> > index b47c50270..93e5aab1f 100644
> > --- a/pfinet/linux-src/net/core/dev.c
> > +++ b/pfinet/linux-src/net/core/dev.c
> > @@ -923,7 +923,9 @@ void net_bh(void)
> > */
> >
> > /* XXX until we figure out every place to modify.. */
> > - skb->h.raw = skb->nh.raw = skb->data;
> > + if ( skb->nh.raw < skb->data ||
> > + skb->nh.raw > skb->data + sizeof(struct ethhdr_vlan))
> > + skb->h.raw = skb->nh.raw = skb->data;
>
> This deserves getting its own patch with explanation why this if needs
> to be introduced like this, because it looks like putting brown tape on
> top of brown tape, and honestly looks ugly, because it is saying "if it
> looks bad, try to fix it".
>
> I guess you had to introduce it because it overwrites the pointers
> you have set in trivfs_S_io_write. But then as the comment says, we'd
> just need to "figure out every place to modify". There aren't that
> many since pfinet only has dummy, ethernet, and tunnel which use
> register_netdevice, and dummy does not produce frames, so it looks to me
> like you only need to fix ethernet.c's ethernet_demuxer into setting
> skb->h.raw and skb->nh.raw, and then just remove these lines in dev.c
> and be done rather than adding brown tape.
>
> > diff --git a/pfinet/tunnel.c b/pfinet/tunnel.c
> > index 4301d054f..6532cac5c 100644
> > --- a/pfinet/tunnel.c
> > +++ b/pfinet/tunnel.c
> > @@ -168,16 +175,17 @@ setup_tunnel_device (char *name, struct device
> > **device)
> > tunnel_dev = tdev;
> >
> > *device = dev = &tdev->dev;
> > + dev->priv = tdev;
> > + skb_queue_head_init(&tdev->xq); // Initialise tdev->xq
>
> ? We already do this in tunnel_open. Either this is not needed, or it
> should be removed from tunnel_open. Also, why moving the initialization?
> Usually we rather keep the initialization of fields in the structure
> order, so initializing ->name would go first.
>
> > @@ -186,21 +194,44 @@ setup_tunnel_device (char *name, struct device
> > **device)
> > + if ( strncmp( base_name, "tap", 3 ) == 0 ) {
> > + /* The setup for a tap is most similar the ethernet.c */
> > + dev->hard_header = eth_header;
> > + dev->rebuild_header = eth_rebuild_header;
> > + dev->hard_header_cache = eth_header_cache;
> > + dev->header_cache_update = eth_header_cache_update;
> > + dev->hard_header_parse = eth_header_parse;
> > + dev->hard_header_len = ETH_HLEN;
>
> It's nice that we can just reuse that :)
>
> > + // We *know* that ETH_ALEN == 6
> > + unsigned char num[ 4 ]; //
> > + sscanf( name, "tap%u", (unsigned int*)&num );
> > + memcpy( dev->dev_addr, (char[6]) {
> > + 0x06, 'T', num[3], num[2], num[1], num[0] }, 6 );
>
> I'd rather say cast the converse way: pass the address of what is sure
> to be an unsigned int to sscanf so we are sure it writes an unsigned
> int. And then cast into unsigned char * to forge a MAC address out of
> it.
>
> Why 06:54:...?
>
> > @@ -388,6 +421,10 @@ trivfs_S_io_write (struct trivfs_protid *cred,
> >
> > tdev = (struct tunnel_device *) cred->po->cntl->hook;
> >
> > + if ((tdev->dev.hard_header != 0) && (datalen < 18)) // small packet
>
> Why 18? There "could" be payloads with less than 4 bytes.
>
> Also, better use sizeof(struct ethhdr) rather than a hardcoded number
> without meaning.
>
> > + return EBADF;
>
> No, this is rather EINVAL. We use EBADF in the other cases because of a
> read/write mismatch.
I'm afraid I've run out of time for a while and won't be able to take
on pfinet maintenance. Perhaps someone is able to take it up and, with
needed changes, merges into the actual code base.
Ralph.
