[PATCH nf-next 0/3] netfilter: bridge: add queuing to userspace for AF_BRIDGE family

[PATCH nf-next 0/3] netfilter: bridge: add queuing to userspace for AF_BRIDGE family

[Stephane Bryant]

From: stephane <stephane.ml.bryant@gmail.com>

This series of patches intends to add support for queuing to userspace 
for the AF_BRIDGE family:
-patch 1 just adds and registers the relevant structures: basic queuing 
 is enabled
-patch 2 will pull back the L2 header into the skb sent to the user space
-patch 3 will reinsert the VLAN header when present
Each patch can be applied as a standalone.
No checksum recalculation is performed for transport protocols

stephane (3):
  netfilter: bridge: add nf_afinfo to enable queuing to userspace
  netfilter: bridge: pull back mac header into skb queued to userspace
  netfilter: bridge: copy back VLAN header for bridge packet queued to
    userspace

 net/bridge/br_netfilter_hooks.c | 44 +++++++++++++++++++++++
 net/netfilter/nfnetlink_queue.c | 80 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 119 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH nf-next 1/3] netfilter: bridge: add nf_afinfo to enable queuing to userspace

[Stephane Bryant]

From: stephane <stephane.ml.bryant@gmail.com>

This just adds and registers a nf_afinfo for the ethernet
bridge, which enables queuing to userspace for the AF_BRIDGE
family. No checksum computation is done.

Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
---
 net/bridge/br_netfilter_hooks.c | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 7ddbe7e..7f8f922 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -999,6 +999,46 @@ static struct ctl_table brnf_table[] = {
 };
 #endif
 
+static void nf_br_saveroute(const struct sk_buff *skb,
+			    struct nf_queue_entry *entry)
+{
+}
+
+static int nf_br_reroute(struct sk_buff *skb,
+			 const struct nf_queue_entry *entry)
+{
+	return 0;
+}
+
+__sum16 nf_br_checksum(struct sk_buff *skb, unsigned int hook,
+		       unsigned int dataoff, u_int8_t protocol)
+{
+	return 0;
+}
+
+static __sum16 nf_br_checksum_partial(struct sk_buff *skb, unsigned int hook,
+				      unsigned int dataoff, unsigned int len,
+				      u_int8_t protocol)
+{
+	return 0;
+}
+
+static int nf_br_route(struct net *net, struct dst_entry **dst,
+		       struct flowi *fl, bool strict __always_unused)
+{
+	return 0;
+}
+
+static const struct nf_afinfo nf_br_afinfo = {
+	.family			= AF_BRIDGE,
+	.checksum		= nf_br_checksum,
+	.checksum_partial	= nf_br_checksum_partial,
+	.route			= nf_br_route,
+	.saveroute		= nf_br_saveroute,
+	.reroute		= nf_br_reroute,
+	.route_key_size		= 0,
+};
+
 static int __init br_netfilter_init(void)
 {
 	int ret;
@@ -1018,12 +1058,16 @@ static int __init br_netfilter_init(void)
 #endif
 	RCU_INIT_POINTER(nf_br_ops, &br_ops);
 	printk(KERN_NOTICE "Bridge firewalling registered\n");
+
+	nf_register_afinfo(&nf_br_afinfo);
+
 	return 0;
 }
 
 static void __exit br_netfilter_fini(void)
 {
 	RCU_INIT_POINTER(nf_br_ops, NULL);
+	nf_unregister_afinfo(&nf_br_afinfo);
 	nf_unregister_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
 #ifdef CONFIG_SYSCTL
 	unregister_net_sysctl_table(brnf_sysctl_header);
-- 
2.1.4

Re: [PATCH nf-next 1/3] netfilter: bridge: add nf_afinfo to enable queuing to userspace

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi stephane,

[auto build test WARNING on nf-next/master]

url:    https://github.com/0day-ci/linux/commits/Stephane-Bryant/netfilter-bridge-add-queuing-to-userspace-for-AF_BRIDGE-family/20160115-165136
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next master
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

>> net/bridge/br_netfilter_hooks.c:1038:2: warning: initialization from incompatible pointer type
     .reroute  = nf_br_reroute,
     ^
   net/bridge/br_netfilter_hooks.c:1038:2: warning: (near initialization for 'nf_br_afinfo.reroute')

vim +1038 net/bridge/br_netfilter_hooks.c

  1022	{
  1023		return 0;
  1024	}
  1025	
  1026	static int nf_br_route(struct net *net, struct dst_entry **dst,
  1027			       struct flowi *fl, bool strict __always_unused)
  1028	{
  1029		return 0;
  1030	}
  1031	
  1032	static const struct nf_afinfo nf_br_afinfo = {
  1033		.family			= AF_BRIDGE,
  1034		.checksum		= nf_br_checksum,
  1035		.checksum_partial	= nf_br_checksum_partial,
  1036		.route			= nf_br_route,
  1037		.saveroute		= nf_br_saveroute,
> 1038		.reroute		= nf_br_reroute,
  1039		.route_key_size		= 0,
  1040	};
  1041	
  1042	static int __init br_netfilter_init(void)
  1043	{
  1044		int ret;
  1045	
  1046		ret = nf_register_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 43663 bytes --]

Re: [PATCH nf-next 1/3] netfilter: bridge: add nf_afinfo to enable queuing to userspace

[Florian Westphal]

Stephane Bryant <stephane.ml.bryant@gmail.com> wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> This just adds and registers a nf_afinfo for the ethernet
> bridge, which enables queuing to userspace for the AF_BRIDGE
> family. No checksum computation is done.
> 
> Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
> ---
>  net/bridge/br_netfilter_hooks.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index 7ddbe7e..7f8f922 100644
> --- a/net/bridge/br_netfilter_hooks.c
> +++ b/net/bridge/br_netfilter_hooks.c
> @@ -999,6 +999,46 @@ static struct ctl_table brnf_table[] = {
>  };
>  #endif
>  
> +static void nf_br_saveroute(const struct sk_buff *skb,
> +			    struct nf_queue_entry *entry)

Please put all of these functions into a new file.
CONFIG_BRIDGE_NETFILTER will be deprecated, this is the glue part
that allows a bridge to use arp and /ip(6)tables.

Re: [PATCH nf-next 1/3] netfilter: bridge: add nf_afinfo to enable queuing to userspace

[stéphane bryant]

On 01/15/2016 10:49 AM, Florian Westphal wrote:
> 
>> --- a/net/bridge/br_netfilter_hooks.c
>> +++ b/net/bridge/br_netfilter_hooks.c
>> @@ -999,6 +999,46 @@ static struct ctl_table brnf_table[] = {
>>  };
>>  #endif
>>  
>> +static void nf_br_saveroute(const struct sk_buff *skb,
>> +			    struct nf_queue_entry *entry)
> 
> Please put all of these functions into a new file.
> CONFIG_BRIDGE_NETFILTER will be deprecated, this is the glue part
> that allows a bridge to use arp and /ip(6)tables.
> 

Sorry if i am a bit of a slow-learner, but i assume you mean following
what's done for, e.g., ipv4, and have this into a bridge/netfilter.c and
initializing this part of the subsystem.

Re: [PATCH nf-next 1/3] netfilter: bridge: add nf_afinfo to enable queuing to userspace

[Florian Westphal]

stéphane bryant <stephane.ml.bryant@gmail.com> wrote:
> >> --- a/net/bridge/br_netfilter_hooks.c
> >> +++ b/net/bridge/br_netfilter_hooks.c
> >> @@ -999,6 +999,46 @@ static struct ctl_table brnf_table[] = {
> >>  };
> >>  #endif
> >>  
> >> +static void nf_br_saveroute(const struct sk_buff *skb,
> >> +			    struct nf_queue_entry *entry)
> > 
> > Please put all of these functions into a new file.
> > CONFIG_BRIDGE_NETFILTER will be deprecated, this is the glue part
> > that allows a bridge to use arp and /ip(6)tables.
> > 
> 
> Sorry if i am a bit of a slow-learner, but i assume you mean following
> what's done for, e.g., ipv4, and have this into a bridge/netfilter.c and
> initializing this part of the subsystem.

bridge/netfilter.c sounds good.
Another option would be net/bridge/netfilter/nf_tables_bridge.c
(assuming that this is only added for use with nft as opposed to adding
 a nfqueue target to ebtables).

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

[PATCH nf-next 2/3] netfilter: bridge: pull back mac header into skb queued to userspace

[Stephane Bryant]

From: stephane <stephane.ml.bryant@gmail.com>

This pulls back the mac header into the skb when queuing packets to
userspace in the bridge, and conversely when processing the verdict.

Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
---
 net/netfilter/nfnetlink_queue.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 1d39365..b299236 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -317,6 +317,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	bool csum_verify;
 	char *secdata = NULL;
 	u32 seclen = 0;
+	int mac_header_len = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -353,6 +354,18 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 		    skb_checksum_help(entskb))
 			return NULL;
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+		if ((entry->state.pf == PF_BRIDGE) &&
+		    (entskb->dev && (entskb->data > skb_mac_header(entskb)))) {
+			/* push back the mac header into the data so that
+			 * it gets copied in
+			 */
+			mac_header_len =
+				(int)(entskb->data - skb_mac_header(entskb));
+			skb_push(entskb, mac_header_len);
+		}
+#endif
+
 		data_len = ACCESS_ONCE(queue->copy_range);
 		if (data_len > entskb->len)
 			data_len = entskb->len;
@@ -542,6 +555,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	}
 
 	nlh->nlmsg_len = skb->len;
+
+	if (mac_header_len > 0)
+		skb_pull(entskb, mac_header_len);
+
 	return skb;
 
 nla_put_failure:
@@ -1070,12 +1087,29 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
 
 	if (nfqa[NFQA_PAYLOAD]) {
 		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
-		int diff = payload_len - entry->skb->len;
+		int diff = 0;
+		int mac_header_len = 0;
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+		if ((entry->state.pf == PF_BRIDGE) &&
+		    (entry->skb->dev &&
+		     (entry->skb->data > skb_mac_header(entry->skb)))) {
+			/* push back the mac header into the data so that
+			 * it gets copied in before mangling
+			 */
+			mac_header_len = (int)(entry->skb->data -
+					       skb_mac_header(entry->skb));
+			skb_push(entry->skb, mac_header_len);
+	}
+#endif
+		diff = payload_len - entry->skb->len;
 
 		if (nfqnl_mangle(nla_data(nfqa[NFQA_PAYLOAD]),
 				 payload_len, entry, diff) < 0)
 			verdict = NF_DROP;
 
+		if (mac_header_len > 0) /* pull mac header again */
+			skb_pull(entry->skb, mac_header_len);
+
 		if (ct && diff)
 			nfnl_ct->seq_adjust(entry->skb, ct, ctinfo, diff);
 	}
-- 
2.1.4

[PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Stephane Bryant]

From: stephane <stephane.ml.bryant@gmail.com>

For bridge packets queued to userspace, this uses the skb tci info
to reinstate the VLAN header, and conversely parses and removes it
to fill the tci info on the way back.

Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
---
 net/netfilter/nfnetlink_queue.c | 72 ++++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index b299236..07723f9 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -317,7 +317,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	bool csum_verify;
 	char *secdata = NULL;
 	u32 seclen = 0;
-	int mac_header_len = 0;
+	size_t mac_header_len = 0;
+	size_t vlan_len = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -357,12 +358,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 		if ((entry->state.pf == PF_BRIDGE) &&
 		    (entskb->dev && (entskb->data > skb_mac_header(entskb)))) {
-			/* push back the mac header into the data so that
-			 * it gets copied in
-			 */
 			mac_header_len =
 				(int)(entskb->data - skb_mac_header(entskb));
-			skb_push(entskb, mac_header_len);
+			if (skb_vlan_tag_present(entskb))
+				vlan_len = VLAN_HLEN;
 		}
 #endif
 
@@ -372,7 +371,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 
 		hlen = skb_zerocopy_headlen(entskb);
 		hlen = min_t(unsigned int, hlen, data_len);
-		size += sizeof(struct nlattr) + hlen;
+		size += sizeof(struct nlattr) + hlen + mac_header_len +
+			vlan_len;
 		cap_len = entskb->len;
 		rem_len = data_len - hlen;
 		break;
@@ -548,7 +548,26 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 
 		nla = (struct nlattr *)skb_put(skb, sizeof(*nla));
 		nla->nla_type = NFQA_PAYLOAD;
-		nla->nla_len = nla_attr_size(data_len);
+		nla->nla_len =
+			nla_attr_size(data_len + mac_header_len + vlan_len);
+		if (mac_header_len > 0) {
+			unsigned char *mac_header;
+
+			mac_header = skb_put(skb, mac_header_len + vlan_len);
+			memcpy(mac_header, skb_mac_header(entskb),
+			       mac_header_len);
+			if (vlan_len > 0) {
+				struct vlan_ethhdr *veth =
+					(struct vlan_ethhdr *)mac_header;
+
+				u16 proto = veth->h_vlan_proto;
+
+				veth->h_vlan_proto = entskb->vlan_proto;
+				veth->h_vlan_TCI =
+					htons(skb_vlan_tag_get(entskb));
+				veth->h_vlan_encapsulated_proto = proto;
+			}
+		}
 
 		if (skb_zerocopy(skb, entskb, data_len, hlen))
 			goto nla_put_failure;
@@ -556,9 +575,6 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 
 	nlh->nlmsg_len = skb->len;
 
-	if (mac_header_len > 0)
-		skb_pull(entskb, mac_header_len);
-
 	return skb;
 
 nla_put_failure:
@@ -1087,24 +1103,44 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
 
 	if (nfqa[NFQA_PAYLOAD]) {
 		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
+		unsigned char *payload = nla_data(nfqa[NFQA_PAYLOAD]);
 		int diff = 0;
 		int mac_header_len = 0;
+
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 		if ((entry->state.pf == PF_BRIDGE) &&
 		    (entry->skb->dev &&
 		     (entry->skb->data > skb_mac_header(entry->skb)))) {
-			/* push back the mac header into the data so that
-			 * it gets copied in before mangling
-			 */
-			mac_header_len = (int)(entry->skb->data -
-					       skb_mac_header(entry->skb));
+			struct vlan_ethhdr *veth =
+				(struct vlan_ethhdr *)payload;
+
+			mac_header_len = (int)
+				(entry->skb->data - skb_mac_header(entry->skb));
+			/* push back mac header before mangling */
 			skb_push(entry->skb, mac_header_len);
-	}
+			/* check for VLAN in NFQA_PAYLOAD */
+			if ((payload_len >= VLAN_ETH_HLEN) &&
+			    ((veth->h_vlan_proto == htons(ETH_P_8021Q)) ||
+			     (veth->h_vlan_proto == htons(ETH_P_8021AD)))) {
+				entry->skb->vlan_proto = veth->h_vlan_proto;
+				entry->skb->vlan_tci =
+					ntohs(veth->h_vlan_TCI) |
+					VLAN_TAG_PRESENT;
+				memmove(payload + VLAN_HLEN, payload,
+					2 * ETH_ALEN);
+				entry->skb->protocol =
+					veth->h_vlan_encapsulated_proto;
+				payload += VLAN_HLEN;
+				payload_len -= VLAN_HLEN;
+			} else {
+				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
+				entry->skb->protocol = veth->h_vlan_proto;
+			}
+		}
 #endif
 		diff = payload_len - entry->skb->len;
 
-		if (nfqnl_mangle(nla_data(nfqa[NFQA_PAYLOAD]),
-				 payload_len, entry, diff) < 0)
+		if (nfqnl_mangle(payload, payload_len, entry, diff) < 0)
 			verdict = NF_DROP;
 
 		if (mac_header_len > 0) /* pull mac header again */
-- 
2.1.4

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Florian Westphal]

Stephane Bryant <stephane.ml.bryant@gmail.com> wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> For bridge packets queued to userspace, this uses the skb tci info
> to reinstate the VLAN header, and conversely parses and removes it
> to fill the tci info on the way back.
> -			 * it gets copied in
> -			 */
>  			mac_header_len =
>  				(int)(entskb->data - skb_mac_header(entskb));
> -			skb_push(entskb, mac_header_len);
> +			if (skb_vlan_tag_present(entskb))
> +				vlan_len = VLAN_HLEN;

I wondered if we could use the saveroute and reroute hooks in the nf
afinfo to perform the push/pull.

It would keep the bridge specific parts out of the generic code.

Thanks for working on this!

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Stephane Bryant <stephane.ml.bryant@gmail.com> wrote:
> > From: stephane <stephane.ml.bryant@gmail.com>
> > 
> > For bridge packets queued to userspace, this uses the skb tci info
> > to reinstate the VLAN header, and conversely parses and removes it
> > to fill the tci info on the way back.
> > -			 * it gets copied in
> > -			 */
> >  			mac_header_len =
> >  				(int)(entskb->data - skb_mac_header(entskb));
> > -			skb_push(entskb, mac_header_len);
> > +			if (skb_vlan_tag_present(entskb))
> > +				vlan_len = VLAN_HLEN;
> 
> I wondered if we could use the saveroute and reroute hooks in the nf
> afinfo to perform the push/pull.
> 
> It would keep the bridge specific parts out of the generic code.

Addendum: If its not possible I'd prefer to add afinfo helpers for it to
keep this out of the generic part.

F.e. we will likely also want netdev family support later on.

As for complications wrt. nf_bridge_adjust_skb_data() (the software
segmentation part) I think the best way would be to reject attempts to
bind a queue for families other than NFPROTO_IPV4|6 without
NFQA_CFG_F_GSO flag present.

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Pablo Neira Ayuso]

On Fri, Jan 15, 2016 at 09:48:54AM +0100, Stephane Bryant wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> For bridge packets queued to userspace, this uses the skb tci info
> to reinstate the VLAN header, and conversely parses and removes it
> to fill the tci info on the way back.
> 
> Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
> ---
>  net/netfilter/nfnetlink_queue.c | 72 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index b299236..07723f9 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -548,7 +548,26 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  
>  		nla = (struct nlattr *)skb_put(skb, sizeof(*nla));
>  		nla->nla_type = NFQA_PAYLOAD;
> -		nla->nla_len = nla_attr_size(data_len);
> +		nla->nla_len =
> +			nla_attr_size(data_len + mac_header_len + vlan_len);
> +		if (mac_header_len > 0) {
> +			unsigned char *mac_header;
> +
> +			mac_header = skb_put(skb, mac_header_len + vlan_len);
> +			memcpy(mac_header, skb_mac_header(entskb),
> +			       mac_header_len);
> +			if (vlan_len > 0) {
> +				struct vlan_ethhdr *veth =
> +					(struct vlan_ethhdr *)mac_header;
> +
> +				u16 proto = veth->h_vlan_proto;
> +
> +				veth->h_vlan_proto = entskb->vlan_proto;
> +				veth->h_vlan_TCI =
> +					htons(skb_vlan_tag_get(entskb));
> +				veth->h_vlan_encapsulated_proto = proto;
> +			}
> +		}

For the specific case of nfnetlink_queue, I would expose the vlan
information through a new netlink attribute NFQA_VLAN (similar to what
we do for NFQA_HWADDR for the layer 3).

>  		if (skb_zerocopy(skb, entskb, data_len, hlen))
>  			goto nla_put_failure;
> @@ -556,9 +575,6 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  
>  	nlh->nlmsg_len = skb->len;
>  
> -	if (mac_header_len > 0)
> -		skb_pull(entskb, mac_header_len);
> -
>  	return skb;
>  
>  nla_put_failure:
> @@ -1087,24 +1103,44 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
>  
>  	if (nfqa[NFQA_PAYLOAD]) {
>  		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
> +		unsigned char *payload = nla_data(nfqa[NFQA_PAYLOAD]);
>  		int diff = 0;
>  		int mac_header_len = 0;
> +
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>  		if ((entry->state.pf == PF_BRIDGE) &&
>  		    (entry->skb->dev &&
>  		     (entry->skb->data > skb_mac_header(entry->skb)))) {
> -			/* push back the mac header into the data so that
> -			 * it gets copied in before mangling
> -			 */
> -			mac_header_len = (int)(entry->skb->data -
> -					       skb_mac_header(entry->skb));
> +			struct vlan_ethhdr *veth =
> +				(struct vlan_ethhdr *)payload;
> +
> +			mac_header_len = (int)
> +				(entry->skb->data - skb_mac_header(entry->skb));
> +			/* push back mac header before mangling */
>  			skb_push(entry->skb, mac_header_len);
> -	}
> +			/* check for VLAN in NFQA_PAYLOAD */
> +			if ((payload_len >= VLAN_ETH_HLEN) &&
> +			    ((veth->h_vlan_proto == htons(ETH_P_8021Q)) ||
> +			     (veth->h_vlan_proto == htons(ETH_P_8021AD)))) {
> +				entry->skb->vlan_proto = veth->h_vlan_proto;
> +				entry->skb->vlan_tci =
> +					ntohs(veth->h_vlan_TCI) |
> +					VLAN_TAG_PRESENT;
> +				memmove(payload + VLAN_HLEN, payload,
> +					2 * ETH_ALEN);
> +				entry->skb->protocol =
> +					veth->h_vlan_encapsulated_proto;
> +				payload += VLAN_HLEN;
> +				payload_len -= VLAN_HLEN;
> +			} else {
> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> +				entry->skb->protocol = veth->h_vlan_proto;
> +			}
> +		}

I'm awar it's more work, but it would be good to reduce ifdef pollution
by placing all this bridge netfilter code wrapped into functions under
one single ifdef in this file to improve maintainability.

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> For the specific case of nfnetlink_queue, I would expose the vlan
> information through a new netlink attribute NFQA_VLAN (similar to what
> we do for NFQA_HWADDR for the layer 3).

If we do this I think it does make sense to consider putting
the entire L2 mac header under its own attr too.

This is especially good if we'd later add support for NETDEV
family.  Since drivers already pull the L2 header userspace
would not need to handle arbirary L2 protocols.

> > +				payload += VLAN_HLEN;
> > +				payload_len -= VLAN_HLEN;
> > +			} else {
> > +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> > +				entry->skb->protocol = veth->h_vlan_proto;
> > +			}
> > +		}
> 
> I'm awar it's more work, but it would be good to reduce ifdef pollution
> by placing all this bridge netfilter code wrapped into functions under
> one single ifdef in this file to improve maintainability.

Right, but for anything family specifiy it would be even better to push
it into nf afinfo. In case thats too much work or too cumbersome (e.g.
because you'd need 12 function arguments ...) then the ifdef-wrapped
helper is fine of course.

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Pablo Neira Ayuso]

On Fri, Jan 15, 2016 at 03:04:12PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > For the specific case of nfnetlink_queue, I would expose the vlan
> > information through a new netlink attribute NFQA_VLAN (similar to what
> > we do for NFQA_HWADDR for the layer 3).
> 
> If we do this I think it does make sense to consider putting
> the entire L2 mac header under its own attr too.
> 
> This is especially good if we'd later add support for NETDEV
> family.  Since drivers already pull the L2 header userspace
> would not need to handle arbirary L2 protocols.

Good point, fine with me.

> > > +				payload += VLAN_HLEN;
> > > +				payload_len -= VLAN_HLEN;
> > > +			} else {
> > > +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> > > +				entry->skb->protocol = veth->h_vlan_proto;
> > > +			}
> > > +		}
> > 
> > I'm awar it's more work, but it would be good to reduce ifdef pollution
> > by placing all this bridge netfilter code wrapped into functions under
> > one single ifdef in this file to improve maintainability.
> 
> Right, but for anything family specifiy it would be even better to push
> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> because you'd need 12 function arguments ...) then the ifdef-wrapped
> helper is fine of course.

I'm fine with pushing this info afinfo if it fits fine there.

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[stéphane bryant]

I will rework the patches taking all these remarks into consideration.

Regarding the entire L2 header attribute. I assume it would be a
separate attribute from the existing NFQA_HWADDR? or would it make sense
to collapse these 2 into one (potentially with helpers to access each
relevant field) to avoid duplicating information (and redundancy, and
potential discrepancies)?

On 01/15/2016 05:33 PM, Pablo Neira Ayuso wrote:
> On Fri, Jan 15, 2016 at 03:04:12PM +0100, Florian Westphal wrote:
>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>> For the specific case of nfnetlink_queue, I would expose the vlan
>>> information through a new netlink attribute NFQA_VLAN (similar to what
>>> we do for NFQA_HWADDR for the layer 3).
>>
>> If we do this I think it does make sense to consider putting
>> the entire L2 mac header under its own attr too.
>>
>> This is especially good if we'd later add support for NETDEV
>> family.  Since drivers already pull the L2 header userspace
>> would not need to handle arbirary L2 protocols.
> 
> Good point, fine with me.
> 
>>>> +				payload += VLAN_HLEN;
>>>> +				payload_len -= VLAN_HLEN;
>>>> +			} else {
>>>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
>>>> +				entry->skb->protocol = veth->h_vlan_proto;
>>>> +			}
>>>> +		}
>>>
>>> I'm awar it's more work, but it would be good to reduce ifdef pollution
>>> by placing all this bridge netfilter code wrapped into functions under
>>> one single ifdef in this file to improve maintainability.
>>
>> Right, but for anything family specifiy it would be even better to push
>> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
>> because you'd need 12 function arguments ...) then the ifdef-wrapped
>> helper is fine of course.
> 
> I'm fine with pushing this info afinfo if it fits fine there.
> 

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Florian Westphal]

stéphane bryant <stephane.ml.bryant@gmail.com> wrote:

Please don't top-post, thanks.

> Regarding the entire L2 header attribute. I assume it would be a
> separate attribute from the existing NFQA_HWADDR?

Yes, it would just contain mac header start to skb->data, i.e. without
VLAN header.  VLAN header would go into a new attribute too.

> Or would it make sense
> to collapse these 2 into one (potentially with helpers to access each
> relevant field) to avoid duplicating information (and redundancy, and
> potential discrepancies)?

Yes but we cannot do it due to backwards compatibility.
We *can* omit NFQA_HWADDR in the family == NFPROTO_BRIDGE case of
course.

Thanks for working on this,
Florian
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[stéphane bryant]

On 01/15/2016 03:04 PM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> For the specific case of nfnetlink_queue, I would expose the vlan
>> information through a new netlink attribute NFQA_VLAN (similar to what
>> we do for NFQA_HWADDR for the layer 3).
> 
> If we do this I think it does make sense to consider putting
> the entire L2 mac header under its own attr too.
> 
> This is especially good if we'd later add support for NETDEV
> family.  Since drivers already pull the L2 header userspace
> would not need to handle arbirary L2 protocols.
> 
>>> +				payload += VLAN_HLEN;
>>> +				payload_len -= VLAN_HLEN;
>>> +			} else {
>>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
>>> +				entry->skb->protocol = veth->h_vlan_proto;
>>> +			}
>>> +		}
>>
>> I'm awar it's more work, but it would be good to reduce ifdef pollution
>> by placing all this bridge netfilter code wrapped into functions under
>> one single ifdef in this file to improve maintainability.
> 
> Right, but for anything family specifiy it would be even better to push
> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> because you'd need 12 function arguments ...) then the ifdef-wrapped
> helper is fine of course.

As the nf_afinfo saveroute/reroute hooks are called on the original
packet skb, at a location where netlink attributes are not in existence,
it only seems possible to use these hooks to hide the L2 code if we
pull-in/pull out the L2 header into/from the original skb, and forego
the new attributes -- which is fine by me as it is precisely what i was
doing in the original patches (albeit in a different location).
If we use new netlink attributes (NFQA_VLAN, NFQA_L2HDR) we will have to
wrap them in a #ifder helper, it seems.
I can go either way.

> .
> 

Re: [PATCH nf-next 3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

[Florian Westphal]

stéphane bryant <stephane.ml.bryant@gmail.com> wrote:
> On 01/15/2016 03:04 PM, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> For the specific case of nfnetlink_queue, I would expose the vlan
> >> information through a new netlink attribute NFQA_VLAN (similar to what
> >> we do for NFQA_HWADDR for the layer 3).
> > 
> > If we do this I think it does make sense to consider putting
> > the entire L2 mac header under its own attr too.
> > 
> > This is especially good if we'd later add support for NETDEV
> > family.  Since drivers already pull the L2 header userspace
> > would not need to handle arbirary L2 protocols.
> > 
> >>> +				payload += VLAN_HLEN;
> >>> +				payload_len -= VLAN_HLEN;
> >>> +			} else {
> >>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> >>> +				entry->skb->protocol = veth->h_vlan_proto;
> >>> +			}
> >>> +		}
> >>
> >> I'm awar it's more work, but it would be good to reduce ifdef pollution
> >> by placing all this bridge netfilter code wrapped into functions under
> >> one single ifdef in this file to improve maintainability.
> > 
> > Right, but for anything family specifiy it would be even better to push
> > it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> > because you'd need 12 function arguments ...) then the ifdef-wrapped
> > helper is fine of course.
> 
> As the nf_afinfo saveroute/reroute hooks are called on the original
> packet skb, at a location where netlink attributes are not in existence,
> it only seems possible to use these hooks to hide the L2 code if we
> pull-in/pull out the L2 header into/from the original skb, and forego
> the new attributes -- which is fine by me as it is precisely what i was
> doing in the original patches (albeit in a different location).

Yes.  Problem with push/pull is that it will be impossible to do packet
rewriting of the l2 header since we'd have to pull a different amount.

> If we use new netlink attributes (NFQA_VLAN, NFQA_L2HDR) we will have to
> wrap them in a #ifder helper, it seems.

ifdef helper is fine.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html