Re: [PATCH v2] packet: Deliver VLAN TPID to userspace

[Atzm Watanabe <atzm@stratosphere.co.jp>]

At Tue, 10 Dec 2013 12:59:15 -0000,
David Laight wrote:
> 
> > From: Atzm Watanabe
> > Sent: 10 December 2013 12:41
> > After the 802.1AD support, userspace packet receivers (packet dumper,
> > software switch, and the like) need how to know VLAN TPID in order to
> > reconstruct original tagged frame.
> ...
> > -#define TP_STATUS_VLAN_VALID	(1 << 4) /* auxdata has valid tp_vlan_tci */
> > +#define TP_STATUS_VLAN_VALID	(1 << 4) /* auxdata has valid tp_vlan_tci and tp_vlan_tpid */
> 
> How can userland (I presume) determine whether tp_vlan_tpid is valid?

Thank you for pointing it.
I'll add a definition which indicates whether tp_vlan_tpid is valid.


> > +struct tpacket_hdr_variant2 {
> > +	__u32	tp_rxhash;
> > +	__u32	tp_vlan_tci;
> > +	__u32	tp_vlan_tpid;
> > +};
> > +
> >  struct tpacket3_hdr {
> >  	__u32		tp_next_offset;
> >  	__u32		tp_sec;
> > @@ -153,6 +159,7 @@ struct tpacket3_hdr {
> >  	/* pkt_hdr variants */
> >  	union {
> >  		struct tpacket_hdr_variant1 hv1;
> > +		struct tpacket_hdr_variant2 hv2;
> >  	};
> >  };
> 
> You've defined a new header variant, but all the code seems to rely
> on the fact that the 'new' variant is identical to the old one
> with the addition of an extra field at the end.
> 
> In which case it ought to be valid to just extend the old header variant.
> If the header really can change format there ought to be a discriminating
> member somewhere - which you don't seem to have changed.

Yes.  I think that struct tpacket3_hdr can grow safely until 48 bytes,
so I'd just like to extend tpacket_hdr_variant1 like you said.

But in the past discussions, there were mentions that a new member
cannot be added into struct tpacket_hdr_variant1, and possibly the
variant which includes a new member should be separated from the old
one.

  http://patchwork.ozlabs.org/patch/284671/
  http://patchwork.ozlabs.org/patch/285382/


Hmm...  I'll resend a patch v3 which includes fixes for your comments
and is using the method that just extends struct tpacket_hdr_variant1. 
If this has any problems, I'll add definition or flag that indicates
whether a new variant exists, and send it as the v3.
If you have any thoughts on that, would you please tell me?


> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 9d70f13..3c75878 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -975,11 +975,15 @@ static void prb_clear_rxhash(struct tpacket_kbdq_core *pkc,
> >  static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc,
> >  			struct tpacket3_hdr *ppd)
> >  {
> > +	BUILD_BUG_ON(TPACKET_ALIGN(sizeof(*ppd)) != 48);
> 
> I'd add a comment about why check matters.
> (ie the fact that it can safely grow until that is no longer true.)

Agreed, will add.


Thank you.