netdev - Re: [PATCH RESEND] packet: Deliver VLAN TPID to userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 08 Nov 2013 02:22:16 +0900
From:	Atzm Watanabe <atzm@...atosphere.co.jp>
To:	Daniel Borkmann <dborkman@...hat.com>
Cc:	Ben Hutchings <bhutchings@...arflare.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	stephen@...workplumber.org
Subject: Re: [PATCH RESEND] packet: Deliver VLAN TPID to userspace

At Mon, 04 Nov 2013 19:33:52 +0100,
Daniel Borkmann wrote:
> 
> On 11/04/2013 06:16 PM, Ben Hutchings wrote:
> > On Wed, 2013-10-30 at 16:03 +0900, Atzm Watanabe wrote:
> > [...]
> >> Should it be structures as below?
> >>
> >> struct tpacket_hdr_variant1 {
> >>          __u32   tp_rxhash;
> >>          __u32   tp_vlan_tci;
> >> };
> >>
> >> 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;
> >>          __u32           tp_nsec;
> >>          __u32           tp_snaplen;
> >>          __u32           tp_len;
> >>          __u32           tp_status;
> >>          __u16           tp_mac;
> >>          __u16           tp_net;
> >>          /* pkt_hdr variants */
> >>          union {
> >>                  struct tpacket_hdr_variant1 hv1;
> >>                  struct tpacket_hdr_variant2 hv2;
> >>          };
> >> };
> >>
> >> If it's ok, I'd like to send the patch v2.
> > [...]
> >
> > I think this makes sense.
> >
> > Also add a BUILD_BUG_ON(TPACKET3_HDRLEN != 48) somewhere.
> 
> Sorry for coming late into this discussion.
> 
> I think the packet_mmap API with its 3 different API versions
> already is a "bit" terrible, and should only be further extended
> for user space if there is a _*huge*_ (!) improvement somewhere
> (e.g. in terms of packet capturing/transmission performance) that
> would justify it. I'm thinking that this could e.g. be in terms
> of packet capturing and transmission performance.
> 
> Otherwise, each time we want to add a new member, we add yet
> another subheader for userspace into tpacket, making it even
> more complicated to use, and adding more "legacy" API fragments?

Thank you for the comments.

Indeed, your worry will become reality if we often want to add new
members as other aims, but I also think that VLAN related members
perhaps won't be added anymore because the VLAN only contains TCI
and ethertype.


> To solve your problem, why don't you add a socket option that,
> if _enabled_, would reconstruct the vlan header as it was seen
> on the "wire" and push that up to userspace, just like libpcap
> does internally. It's not perfect either, but perhaps a better
> way to go. What do you think?

Sounds good to me, but I also think that a socket option should be
added when we want to add new members to enable userspace to
reassemble the pakcet for reasons other than the VLAN.  Because TCI
is already put into the tpacket headers or auxdata, userspace may
want to get TPID by same way.

However, if we cannot add new members into the header anyway,
we need to avoid that by other ways like you proposed.


Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html