* [PATCH] packet: Deliver VLAN TPID to userspace
@ 2013-10-18 17:08 Atzm Watanabe
2013-10-18 17:56 ` Stephen Hemminger
0 siblings, 1 reply; 7+ messages in thread
From: Atzm Watanabe @ 2013-10-18 17:08 UTC (permalink / raw)
To: netdev
After the 802.1AD support, userspace packet receivers
(packet dumper, software switch, and the like) need how to know
VLAN TPID in order to reassemble original tagged frame.
Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
---
include/uapi/linux/if_packet.h | 5 +++--
net/packet/af_packet.c | 8 ++++++--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index dbf0666..6e36e0a 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -83,7 +83,7 @@ struct tpacket_auxdata {
__u16 tp_mac;
__u16 tp_net;
__u16 tp_vlan_tci;
- __u16 tp_padding;
+ __u16 tp_vlan_tpid;
};
/* Rx ring - header status */
@@ -132,12 +132,13 @@ struct tpacket2_hdr {
__u32 tp_sec;
__u32 tp_nsec;
__u16 tp_vlan_tci;
- __u16 tp_padding;
+ __u16 tp_vlan_tpid;
};
struct tpacket_hdr_variant1 {
__u32 tp_rxhash;
__u32 tp_vlan_tci;
+ __u32 tp_vlan_tpid;
};
struct tpacket3_hdr {
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2e8286b..fbcc882 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -895,9 +895,11 @@ static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc,
{
if (vlan_tx_tag_present(pkc->skb)) {
ppd->hv1.tp_vlan_tci = vlan_tx_tag_get(pkc->skb);
+ ppd->hv1.tp_vlan_tpid = (__force __u32)ntohs(pkc->skb->vlan_proto);
ppd->tp_status = TP_STATUS_VLAN_VALID;
} else {
ppd->hv1.tp_vlan_tci = 0;
+ ppd->hv1.tp_vlan_tpid = 0;
ppd->tp_status = TP_STATUS_AVAILABLE;
}
}
@@ -1836,11 +1838,12 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
h.h2->tp_nsec = ts.tv_nsec;
if (vlan_tx_tag_present(skb)) {
h.h2->tp_vlan_tci = vlan_tx_tag_get(skb);
+ h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto);
status |= TP_STATUS_VLAN_VALID;
} else {
h.h2->tp_vlan_tci = 0;
+ h.h2->tp_vlan_tpid = 0;
}
- h.h2->tp_padding = 0;
hdrlen = sizeof(*h.h2);
break;
case TPACKET_V3:
@@ -2788,11 +2791,12 @@ static int packet_recvmsg(struct kiocb *iocb, struct socket *sock,
aux.tp_net = skb_network_offset(skb);
if (vlan_tx_tag_present(skb)) {
aux.tp_vlan_tci = vlan_tx_tag_get(skb);
+ aux.tp_vlan_tpid = ntohs(skb->vlan_proto);
aux.tp_status |= TP_STATUS_VLAN_VALID;
} else {
aux.tp_vlan_tci = 0;
+ aux.tp_vlan_tpid = 0;
}
- aux.tp_padding = 0;
put_cmsg(msg, SOL_PACKET, PACKET_AUXDATA, sizeof(aux), &aux);
}
--
1.8.1.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-18 17:08 [PATCH] packet: Deliver VLAN TPID to userspace Atzm Watanabe
@ 2013-10-18 17:56 ` Stephen Hemminger
2013-10-19 6:19 ` Atzm Watanabe
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2013-10-18 17:56 UTC (permalink / raw)
To: Atzm Watanabe; +Cc: netdev
On Sat, 19 Oct 2013 02:08:11 +0900
Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> index dbf0666..6e36e0a 100644
> --- a/include/uapi/linux/if_packet.h
> +++ b/include/uapi/linux/if_packet.h
> @@ -83,7 +83,7 @@ struct tpacket_auxdata {
> __u16 tp_mac;
> __u16 tp_net;
> __u16 tp_vlan_tci;
> - __u16 tp_padding;
> + __u16 tp_vlan_tpid;
> };
>
> /* Rx ring - header status */
> @@ -132,12 +132,13 @@ struct tpacket2_hdr {
> __u32 tp_sec;
> __u32 tp_nsec;
> __u16 tp_vlan_tci;
> - __u16 tp_padding;
> + __u16 tp_vlan_tpid;
> };
>
> struct tpacket_hdr_variant1 {
> __u32 tp_rxhash;
> __u32 tp_vlan_tci;
> + __u32 tp_vlan_tpid;
> };
The last change will break ABI to userspace applications.
You can reuse padding elements; but you can't increase (or shrink)
an existing structure.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-18 17:56 ` Stephen Hemminger
@ 2013-10-19 6:19 ` Atzm Watanabe
2013-10-21 9:24 ` Ben Hutchings
0 siblings, 1 reply; 7+ messages in thread
From: Atzm Watanabe @ 2013-10-19 6:19 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
At Fri, 18 Oct 2013 10:56:55 -0700,
Stephen Hemminger wrote:
>
> On Sat, 19 Oct 2013 02:08:11 +0900
> Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
>
> > diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> > index dbf0666..6e36e0a 100644
> > --- a/include/uapi/linux/if_packet.h
> > +++ b/include/uapi/linux/if_packet.h
> > @@ -83,7 +83,7 @@ struct tpacket_auxdata {
> > __u16 tp_mac;
> > __u16 tp_net;
> > __u16 tp_vlan_tci;
> > - __u16 tp_padding;
> > + __u16 tp_vlan_tpid;
> > };
> >
> > /* Rx ring - header status */
> > @@ -132,12 +132,13 @@ struct tpacket2_hdr {
> > __u32 tp_sec;
> > __u32 tp_nsec;
> > __u16 tp_vlan_tci;
> > - __u16 tp_padding;
> > + __u16 tp_vlan_tpid;
> > };
> >
> > struct tpacket_hdr_variant1 {
> > __u32 tp_rxhash;
> > __u32 tp_vlan_tci;
> > + __u32 tp_vlan_tpid;
> > };
>
> The last change will break ABI to userspace applications.
> You can reuse padding elements; but you can't increase (or shrink)
> an existing structure.
Thank you for pointing.
But I have two questions:
- The patch that increases existing structures was posted and
accepted in the past (e.g 393e52e33c6c26ec7db290dab803bac1bed962d4
"packet: deliver VLAN TCI to userspace").
What is the difference between them and my patch?
- I tested using tools/testing/selftests/net/psock_tpacket.c built
before applying my patch, and all test cases were passed.
Also I tested by the code that was listed in
Documentation/networking/packet_mmap.txt "AF_PACKET TPACKET_V3
example". It seems that problem was not caused.
What situation causes the problem that you assumed?
Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-19 6:19 ` Atzm Watanabe
@ 2013-10-21 9:24 ` Ben Hutchings
2013-10-22 2:56 ` Atzm Watanabe
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2013-10-21 9:24 UTC (permalink / raw)
To: Atzm Watanabe; +Cc: Stephen Hemminger, netdev
On Sat, 2013-10-19 at 15:19 +0900, Atzm Watanabe wrote:
> At Fri, 18 Oct 2013 10:56:55 -0700,
> Stephen Hemminger wrote:
> >
> > On Sat, 19 Oct 2013 02:08:11 +0900
> > Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
> >
> > > diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> > > index dbf0666..6e36e0a 100644
> > > --- a/include/uapi/linux/if_packet.h
> > > +++ b/include/uapi/linux/if_packet.h
> > > @@ -83,7 +83,7 @@ struct tpacket_auxdata {
> > > __u16 tp_mac;
> > > __u16 tp_net;
> > > __u16 tp_vlan_tci;
> > > - __u16 tp_padding;
> > > + __u16 tp_vlan_tpid;
> > > };
> > >
> > > /* Rx ring - header status */
> > > @@ -132,12 +132,13 @@ struct tpacket2_hdr {
> > > __u32 tp_sec;
> > > __u32 tp_nsec;
> > > __u16 tp_vlan_tci;
> > > - __u16 tp_padding;
> > > + __u16 tp_vlan_tpid;
> > > };
> > >
> > > struct tpacket_hdr_variant1 {
> > > __u32 tp_rxhash;
> > > __u32 tp_vlan_tci;
> > > + __u32 tp_vlan_tpid;
> > > };
> >
> > The last change will break ABI to userspace applications.
> > You can reuse padding elements; but you can't increase (or shrink)
> > an existing structure.
>
> Thank you for pointing.
> But I have two questions:
>
> - The patch that increases existing structures was posted and
> accepted in the past (e.g 393e52e33c6c26ec7db290dab803bac1bed962d4
> "packet: deliver VLAN TCI to userspace").
> What is the difference between them and my patch?
struct tpacket_auxdata is allowed to grow as it will be written/read
using the cmsg API where the length of each structure is explicit at
run-time.
> - I tested using tools/testing/selftests/net/psock_tpacket.c built
> before applying my patch, and all test cases were passed.
> Also I tested by the code that was listed in
> Documentation/networking/packet_mmap.txt "AF_PACKET TPACKET_V3
> example". It seems that problem was not caused.
> What situation causes the problem that you assumed?
Yes, it looks like it would be safe to grow struct tpacket3_hdr too, as
the length is also explicit at run-time.
(And TPACKET{1,2,3}_HDRLEN should be removed from
include/uapi/linux/if_packet.h, as they are not relevant to userland.)
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-21 9:24 ` Ben Hutchings
@ 2013-10-22 2:56 ` Atzm Watanabe
2013-10-22 22:41 ` Stephen Hemminger
0 siblings, 1 reply; 7+ messages in thread
From: Atzm Watanabe @ 2013-10-22 2:56 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Stephen Hemminger, netdev
At Mon, 21 Oct 2013 10:24:54 +0100,
Ben Hutchings wrote:
>
> On Sat, 2013-10-19 at 15:19 +0900, Atzm Watanabe wrote:
> > At Fri, 18 Oct 2013 10:56:55 -0700,
> > Stephen Hemminger wrote:
> > >
> > > On Sat, 19 Oct 2013 02:08:11 +0900
> > > Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
> > >
> > > > diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> > > > index dbf0666..6e36e0a 100644
> > > > --- a/include/uapi/linux/if_packet.h
> > > > +++ b/include/uapi/linux/if_packet.h
> > > > @@ -83,7 +83,7 @@ struct tpacket_auxdata {
> > > > __u16 tp_mac;
> > > > __u16 tp_net;
> > > > __u16 tp_vlan_tci;
> > > > - __u16 tp_padding;
> > > > + __u16 tp_vlan_tpid;
> > > > };
> > > >
> > > > /* Rx ring - header status */
> > > > @@ -132,12 +132,13 @@ struct tpacket2_hdr {
> > > > __u32 tp_sec;
> > > > __u32 tp_nsec;
> > > > __u16 tp_vlan_tci;
> > > > - __u16 tp_padding;
> > > > + __u16 tp_vlan_tpid;
> > > > };
> > > >
> > > > struct tpacket_hdr_variant1 {
> > > > __u32 tp_rxhash;
> > > > __u32 tp_vlan_tci;
> > > > + __u32 tp_vlan_tpid;
> > > > };
> > >
> > > The last change will break ABI to userspace applications.
> > > You can reuse padding elements; but you can't increase (or shrink)
> > > an existing structure.
> >
> > Thank you for pointing.
> > But I have two questions:
> >
> > - The patch that increases existing structures was posted and
> > accepted in the past (e.g 393e52e33c6c26ec7db290dab803bac1bed962d4
> > "packet: deliver VLAN TCI to userspace").
> > What is the difference between them and my patch?
>
> struct tpacket_auxdata is allowed to grow as it will be written/read
> using the cmsg API where the length of each structure is explicit at
> run-time.
>
> > - I tested using tools/testing/selftests/net/psock_tpacket.c built
> > before applying my patch, and all test cases were passed.
> > Also I tested by the code that was listed in
> > Documentation/networking/packet_mmap.txt "AF_PACKET TPACKET_V3
> > example". It seems that problem was not caused.
> > What situation causes the problem that you assumed?
>
> Yes, it looks like it would be safe to grow struct tpacket3_hdr too, as
> the length is also explicit at run-time.
Thank you for the reply. That's same as my thought!
I'll repost the patch with such additional comments.
> (And TPACKET{1,2,3}_HDRLEN should be removed from
> include/uapi/linux/if_packet.h, as they are not relevant to userland.)
Hmm... I think TPACKET{,2,3}_HDRLEN should not be removed without
careful considerations. Because some userspace programs (e.g libpcap)
are using them in order to check mmap ability of the kernel...
Thanks again!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-22 2:56 ` Atzm Watanabe
@ 2013-10-22 22:41 ` Stephen Hemminger
2013-10-23 6:51 ` Atzm Watanabe
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2013-10-22 22:41 UTC (permalink / raw)
To: Atzm Watanabe; +Cc: Ben Hutchings, netdev
On Tue, 22 Oct 2013 11:56:31 +0900
Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
> Hmm... I think TPACKET{,2,3}_HDRLEN should not be removed without
> careful considerations. Because some userspace programs (e.g libpcap)
> are using them in order to check mmap ability of the kernel...
That's bad because it means the library then depends on the headers
of the machine it was built on, not the machine it is running on. I often
build software on boxes where /usr/include version of kernel headers is out dated.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] packet: Deliver VLAN TPID to userspace
2013-10-22 22:41 ` Stephen Hemminger
@ 2013-10-23 6:51 ` Atzm Watanabe
0 siblings, 0 replies; 7+ messages in thread
From: Atzm Watanabe @ 2013-10-23 6:51 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Ben Hutchings, netdev
At Tue, 22 Oct 2013 15:41:10 -0700,
Stephen Hemminger wrote:
>
> On Tue, 22 Oct 2013 11:56:31 +0900
> Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
>
> > Hmm... I think TPACKET{,2,3}_HDRLEN should not be removed without
> > careful considerations. Because some userspace programs (e.g libpcap)
> > are using them in order to check mmap ability of the kernel...
>
>
> That's bad because it means the library then depends on the headers
> of the machine it was built on, not the machine it is running on. I often
> build software on boxes where /usr/include version of kernel headers is out dated.
Yes, this means that building such library with the new header
(TPACKET{,2,3}_HDRLEN were deleted) will make the binary that does
not support PACKET_MMAP. :-(
So, IMHO, we should be prudent in deleting TPACKET{,2,3}_HDRLEN,
otherwise we will break such userspace programs very easily...
e.g. snippet of the latest libpcap-1.4.0 (pcap-linux.c):
/* check for memory mapped access avaibility. We assume every needed
* struct is defined if the macro TPACKET_HDRLEN is defined, because it
* uses many ring related structs and macros */
# ifdef TPACKET_HDRLEN
# define HAVE_PACKET_RING
# ifdef TPACKET2_HDRLEN
# define HAVE_TPACKET2
# else
# define TPACKET_V1 0
# endif /* TPACKET2_HDRLEN */
# endif /* TPACKET_HDRLEN */
It looks like PACKET_MMAP is used when HAVE_PACKET_RING is defined.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-23 6:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-18 17:08 [PATCH] packet: Deliver VLAN TPID to userspace Atzm Watanabe
2013-10-18 17:56 ` Stephen Hemminger
2013-10-19 6:19 ` Atzm Watanabe
2013-10-21 9:24 ` Ben Hutchings
2013-10-22 2:56 ` Atzm Watanabe
2013-10-22 22:41 ` Stephen Hemminger
2013-10-23 6:51 ` Atzm Watanabe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox