Re: [PATCH] Implementation of tap device for Ethernet level 2 in pfinet

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.