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

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.

Thanks!
Samuel