From mboxrd@z Thu Jan 1 00:00:00 1970 From: Atzm Watanabe Subject: Re: [PATCH v2] packet: Deliver VLAN TPID to userspace Date: Wed, 11 Dec 2013 21:53:09 +0900 Message-ID: <87txef7dmy.wl%atzm@stratosphere.co.jp> References: <8738m0q3nq.wl%atzm@stratosphere.co.jp> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: , "Stephen Hemminger" , "Ben Hutchings" , "David Miller" , "Daniel Borkmann" To: "David Laight" Return-path: Received: from mail-yh0-f45.google.com ([209.85.213.45]:56523 "EHLO mail-yh0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753227Ab3LKMxF (ORCPT ); Wed, 11 Dec 2013 07:53:05 -0500 Received: by mail-yh0-f45.google.com with SMTP id v1so4987134yhn.32 for ; Wed, 11 Dec 2013 04:53:02 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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.