* [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace
@ 2013-12-16 8:12 Atzm Watanabe
2013-12-16 10:18 ` Daniel Borkmann
2013-12-16 17:25 ` Stephen Hemminger
0 siblings, 2 replies; 5+ messages in thread
From: Atzm Watanabe @ 2013-12-16 8:12 UTC (permalink / raw)
To: netdev
Cc: Stephen Hemminger, Ben Hutchings, David Miller, Daniel Borkmann,
David Laight
This enables userspace to get VLAN TPID as well as the VLAN TCI.
Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
---
include/uapi/linux/if_packet.h | 23 +++++++++++++----------
net/packet/af_packet.c | 13 +++++++++----
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index 9185dc9..22d5192 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -84,17 +84,18 @@ struct tpacket_auxdata {
__u16 tp_mac;
__u16 tp_net;
__u16 tp_vlan_tci;
- __u16 tp_padding;
+ __u16 tp_vlan_tpid;
};
/* Rx ring - header status */
-#define TP_STATUS_KERNEL 0
-#define TP_STATUS_USER (1 << 0)
-#define TP_STATUS_COPY (1 << 1)
-#define TP_STATUS_LOSING (1 << 2)
-#define TP_STATUS_CSUMNOTREADY (1 << 3)
-#define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid tp_vlan_tci */
-#define TP_STATUS_BLK_TMO (1 << 5)
+#define TP_STATUS_KERNEL 0
+#define TP_STATUS_USER (1 << 0)
+#define TP_STATUS_COPY (1 << 1)
+#define TP_STATUS_LOSING (1 << 2)
+#define TP_STATUS_CSUMNOTREADY (1 << 3)
+#define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid tp_vlan_tci */
+#define TP_STATUS_BLK_TMO (1 << 5)
+#define TP_STATUS_VLAN_TPID_VALID (1 << 6) /* auxdata has valid tp_vlan_tpid */
/* Tx ring - header status */
#define TP_STATUS_AVAILABLE 0
@@ -133,12 +134,14 @@ struct tpacket2_hdr {
__u32 tp_sec;
__u32 tp_nsec;
__u16 tp_vlan_tci;
- __u8 tp_padding[6];
+ __u16 tp_vlan_tpid;
+ __u8 tp_padding[4];
};
struct tpacket_hdr_variant1 {
__u32 tp_rxhash;
__u32 tp_vlan_tci;
+ __u32 tp_vlan_tpid;
};
struct tpacket3_hdr {
@@ -154,7 +157,7 @@ struct tpacket3_hdr {
union {
struct tpacket_hdr_variant1 hv1;
};
- __u8 tp_padding[12];
+ __u8 tp_padding[8];
};
struct tpacket_bd_ts {
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 5c75a1d..58d4fd7 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -977,9 +977,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->tp_status = TP_STATUS_VLAN_VALID;
+ ppd->hv1.tp_vlan_tpid = (__force __u32)ntohs(pkc->skb->vlan_proto);
+ ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
} else {
ppd->hv1.tp_vlan_tci = 0;
+ ppd->hv1.tp_vlan_tpid = 0;
ppd->tp_status = TP_STATUS_AVAILABLE;
}
}
@@ -1925,9 +1927,11 @@ 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);
- status |= TP_STATUS_VLAN_VALID;
+ h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto);
+ status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
} else {
h.h2->tp_vlan_tci = 0;
+ h.h2->tp_vlan_tpid = 0;
}
hdrlen = sizeof(*h.h2);
memset(h.h2->tp_padding, 0,
@@ -2877,11 +2881,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_status |= TP_STATUS_VLAN_VALID;
+ aux.tp_vlan_tpid = ntohs(skb->vlan_proto);
+ aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_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] 5+ messages in thread* Re: [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace
2013-12-16 8:12 [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace Atzm Watanabe
@ 2013-12-16 10:18 ` Daniel Borkmann
2013-12-16 12:41 ` Atzm Watanabe
2013-12-16 17:25 ` Stephen Hemminger
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Borkmann @ 2013-12-16 10:18 UTC (permalink / raw)
To: Atzm Watanabe
Cc: netdev, Stephen Hemminger, Ben Hutchings, David Miller,
David Laight
On 12/16/2013 09:12 AM, Atzm Watanabe wrote:
> This enables userspace to get VLAN TPID as well as the VLAN TCI.
>
> Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
Looks good, one comment below though:
> ---
> include/uapi/linux/if_packet.h | 23 +++++++++++++----------
> net/packet/af_packet.c | 13 +++++++++----
> 2 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> index 9185dc9..22d5192 100644
> --- a/include/uapi/linux/if_packet.h
> +++ b/include/uapi/linux/if_packet.h
> @@ -84,17 +84,18 @@ struct tpacket_auxdata {
> __u16 tp_mac;
> __u16 tp_net;
> __u16 tp_vlan_tci;
> - __u16 tp_padding;
> + __u16 tp_vlan_tpid;
> };
>
> /* Rx ring - header status */
> -#define TP_STATUS_KERNEL 0
> -#define TP_STATUS_USER (1 << 0)
> -#define TP_STATUS_COPY (1 << 1)
> -#define TP_STATUS_LOSING (1 << 2)
> -#define TP_STATUS_CSUMNOTREADY (1 << 3)
> -#define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid tp_vlan_tci */
> -#define TP_STATUS_BLK_TMO (1 << 5)
> +#define TP_STATUS_KERNEL 0
> +#define TP_STATUS_USER (1 << 0)
> +#define TP_STATUS_COPY (1 << 1)
> +#define TP_STATUS_LOSING (1 << 2)
> +#define TP_STATUS_CSUMNOTREADY (1 << 3)
> +#define TP_STATUS_VLAN_VALID (1 << 4) /* auxdata has valid tp_vlan_tci */
> +#define TP_STATUS_BLK_TMO (1 << 5)
> +#define TP_STATUS_VLAN_TPID_VALID (1 << 6) /* auxdata has valid tp_vlan_tpid */
>
> /* Tx ring - header status */
> #define TP_STATUS_AVAILABLE 0
> @@ -133,12 +134,14 @@ struct tpacket2_hdr {
> __u32 tp_sec;
> __u32 tp_nsec;
> __u16 tp_vlan_tci;
> - __u8 tp_padding[6];
> + __u16 tp_vlan_tpid;
> + __u8 tp_padding[4];
> };
>
> struct tpacket_hdr_variant1 {
> __u32 tp_rxhash;
> __u32 tp_vlan_tci;
> + __u32 tp_vlan_tpid;
Why is this not __u16? If this needs to be __u32 aligned, then
probably we should pad with another __u16. I think we should not
waste any space here in case there are future additions of new
members.
> };
>
> struct tpacket3_hdr {
> @@ -154,7 +157,7 @@ struct tpacket3_hdr {
> union {
> struct tpacket_hdr_variant1 hv1;
> };
> - __u8 tp_padding[12];
> + __u8 tp_padding[8];
> };
>
> struct tpacket_bd_ts {
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 5c75a1d..58d4fd7 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -977,9 +977,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->tp_status = TP_STATUS_VLAN_VALID;
> + ppd->hv1.tp_vlan_tpid = (__force __u32)ntohs(pkc->skb->vlan_proto);
Nit: ^^^^ one space here, resp. with __u16
you won't need the cast.
> + ppd->tp_status = TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> } else {
> ppd->hv1.tp_vlan_tci = 0;
> + ppd->hv1.tp_vlan_tpid = 0;
> ppd->tp_status = TP_STATUS_AVAILABLE;
> }
> }
> @@ -1925,9 +1927,11 @@ 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);
> - status |= TP_STATUS_VLAN_VALID;
> + h.h2->tp_vlan_tpid = ntohs(skb->vlan_proto);
> + status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> } else {
> h.h2->tp_vlan_tci = 0;
> + h.h2->tp_vlan_tpid = 0;
> }
> hdrlen = sizeof(*h.h2);
> memset(h.h2->tp_padding, 0,
> @@ -2877,11 +2881,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_status |= TP_STATUS_VLAN_VALID;
> + aux.tp_vlan_tpid = ntohs(skb->vlan_proto);
> + aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_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);
> }
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace
2013-12-16 10:18 ` Daniel Borkmann
@ 2013-12-16 12:41 ` Atzm Watanabe
0 siblings, 0 replies; 5+ messages in thread
From: Atzm Watanabe @ 2013-12-16 12:41 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, Stephen Hemminger, Ben Hutchings, David Miller,
David Laight
At Mon, 16 Dec 2013 11:18:09 +0100,
Daniel Borkmann wrote:
>
> On 12/16/2013 09:12 AM, Atzm Watanabe wrote:
> > This enables userspace to get VLAN TPID as well as the VLAN TCI.
> >
> > Signed-off-by: Atzm Watanabe <atzm@stratosphere.co.jp>
>
> Looks good, one comment below though:
Thank you for comments.
> > __u32 tp_rxhash;
> > __u32 tp_vlan_tci;
> > + __u32 tp_vlan_tpid;
>
> Why is this not __u16? If this needs to be __u32 aligned, then
> probably we should pad with another __u16. I think we should not
> waste any space here in case there are future additions of new
> members.
Ah, indeed. I think it does not need to be __u32 aligned too.
I'll fix it next time.
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 5c75a1d..58d4fd7 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -977,9 +977,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->tp_status = TP_STATUS_VLAN_VALID;
> > + ppd->hv1.tp_vlan_tpid = (__force __u32)ntohs(pkc->skb->vlan_proto);
>
> Nit: ^^^^ one space here, resp. with __u16
> you won't need the cast.
Yes, will remove the cast next time.
Thank you again!
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace
2013-12-16 8:12 [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace Atzm Watanabe
2013-12-16 10:18 ` Daniel Borkmann
@ 2013-12-16 17:25 ` Stephen Hemminger
2013-12-17 5:51 ` Atzm Watanabe
1 sibling, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2013-12-16 17:25 UTC (permalink / raw)
To: Atzm Watanabe
Cc: netdev, Ben Hutchings, David Miller, Daniel Borkmann,
David Laight
On Mon, 16 Dec 2013 17:12:09 +0900
Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
> struct tpacket_hdr_variant1 {
> __u32 tp_rxhash;
> __u32 tp_vlan_tci;
> + __u32 tp_vlan_tpid;
> };
Is this safe? Won't you break ABI for existing userspace applications.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace
2013-12-16 17:25 ` Stephen Hemminger
@ 2013-12-17 5:51 ` Atzm Watanabe
0 siblings, 0 replies; 5+ messages in thread
From: Atzm Watanabe @ 2013-12-17 5:51 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev, Ben Hutchings, David Miller, Daniel Borkmann,
David Laight
At Mon, 16 Dec 2013 09:25:09 -0800,
Stephen Hemminger wrote:
>
> On Mon, 16 Dec 2013 17:12:09 +0900
> Atzm Watanabe <atzm@stratosphere.co.jp> wrote:
>
> > struct tpacket_hdr_variant1 {
> > __u32 tp_rxhash;
> > __u32 tp_vlan_tci;
> > + __u32 tp_vlan_tpid;
> > };
>
> Is this safe? Won't you break ABI for existing userspace applications.
I think we can do it safely without breaking userspace.
struct tpacket_hdr_variant1 is an union member of struct tpacket3_hdr,
and it is not used for any others in the kernel-userspace ABI.
struct tpacket3_hdr is aligned to a multiple of 16 bytes, its size is
currently 48 bytes. Against that, the size before aligning is
currently 36 bytes, so there is a gap of 12 bytes that is unused.
Previous patch 2/3 adds padding members for filling the gap, after the
patch 1/3 which adds BUILD_BUG_ON() as sentinel for aligned size.
The patch 3/3 adds tp_vlan_tpid by picking area from the padding added
in the patch 2/3.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-12-17 5:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-16 8:12 [PATCH v3 3/3] packet: Deliver VLAN TPID to userspace Atzm Watanabe
2013-12-16 10:18 ` Daniel Borkmann
2013-12-16 12:41 ` Atzm Watanabe
2013-12-16 17:25 ` Stephen Hemminger
2013-12-17 5: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;
as well as URLs for NNTP newsgroup(s).