[PATCH nf-next v2 0/2] netfilter: bridge: add queuing to userspace for AF_

[PATCH nf-next v2 0/2] netfilter: bridge: add queuing to userspace for AF_

[stephane.ml.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 passed VLAN and L2 header as netlink attribute, to and back
 from userspace.

stephane (2):
  netfilter: bridge: add nf_afinfo to enable queuing to userspace
  netfilter: bridge: pass L2 header and VLAN as netlink attributes in
    queues to userspace

 include/uapi/linux/netfilter/nfnetlink_queue.h |   7 ++
 net/bridge/netfilter/nf_tables_bridge.c        |  47 ++++++++-
 net/netfilter/nfnetlink_queue.c                | 130 ++++++++++++++++++++++++-
 3 files changed, 177 insertions(+), 7 deletions(-)

-- 
2.1.4

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

[stephane.ml.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/netfilter/nf_tables_bridge.c | 47 +++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/net/bridge/netfilter/nf_tables_bridge.c b/net/bridge/netfilter/nf_tables_bridge.c
index 7fcdd72..a78c4e2 100644
--- a/net/bridge/netfilter/nf_tables_bridge.c
+++ b/net/bridge/netfilter/nf_tables_bridge.c
@@ -162,15 +162,57 @@ static const struct nf_chain_type filter_bridge = {
 			  (1 << NF_BR_POST_ROUTING),
 };
 
+static void nf_br_saveroute(const struct sk_buff *skb,
+			    struct nf_queue_entry *entry)
+{
+}
+
+static int nf_br_reroute(struct net *net, struct sk_buff *skb,
+			 const struct nf_queue_entry *entry)
+{
+	return 0;
+}
+
+static __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 nf_tables_bridge_init(void)
 {
 	int ret;
 
+	nf_register_afinfo(&nf_br_afinfo);
 	nft_register_chain_type(&filter_bridge);
 	ret = register_pernet_subsys(&nf_tables_bridge_net_ops);
-	if (ret < 0)
+	if (ret < 0) {
 		nft_unregister_chain_type(&filter_bridge);
-
+		nf_unregister_afinfo(&nf_br_afinfo);
+	}
 	return ret;
 }
 
@@ -178,6 +220,7 @@ static void __exit nf_tables_bridge_exit(void)
 {
 	unregister_pernet_subsys(&nf_tables_bridge_net_ops);
 	nft_unregister_chain_type(&filter_bridge);
+	nf_unregister_afinfo(&nf_br_afinfo);
 }
 
 module_init(nf_tables_bridge_init);
-- 
2.1.4

[PATCH nf-next v2 2/2] netfilter: bridge: pass L2 header and VLAN as netlink attributes in queues to userspace

[stephane.ml.bryant]

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

-this creates 2 netlink attribute NLQA_VLAN and NLQA_L2HDR
-these are filled up for the PF_BRIDGE family on the way to userspace, and
 used on the way back to modify the original skb accordingly

Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |   7 ++
 net/netfilter/nfnetlink_queue.c                | 130 ++++++++++++++++++++++++-
 2 files changed, 132 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index b67a853..211fcdc 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -30,6 +30,11 @@ struct nfqnl_msg_packet_timestamp {
 	__aligned_be64	usec;
 };
 
+struct nfqnl_msg_vlan {
+	__be16                  proto;
+	__u16                   tci;
+} __attribute__ ((packed));
+
 enum nfqnl_attr_type {
 	NFQA_UNSPEC,
 	NFQA_PACKET_HDR,
@@ -50,6 +55,8 @@ enum nfqnl_attr_type {
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
 	NFQA_SECCTX,			/* security context string */
+	NFQA_VLAN,                      /* packet vlan info */
+	NFQA_L2HDR,                     /* full L2 header */
 
 	__NFQA_MAX
 };
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 1d39365..cb30b2e 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -295,6 +295,59 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
 	return seclen;
 }
 
+static u32 nfqnl_get_bridge_nla_len(struct nf_queue_entry *entry)
+{
+	u32 nlalen = 0;
+
+	struct sk_buff *entskb = entry->skb;
+
+	if (skb_vlan_tag_present(entskb))
+		nlalen += nla_total_size(sizeof(struct nfqnl_msg_vlan));
+
+	if (entry->state.in && entskb->dev &&
+	    (entskb->network_header > entskb->mac_header)) {
+		nlalen += nla_total_size((entskb->network_header -
+					  entskb->mac_header));
+	}
+
+	return nlalen;
+}
+
+static int nfqnl_put_bridge_nla(struct nf_queue_entry *entry,
+				struct sk_buff *skb)
+{
+	struct sk_buff *entskb = entry->skb;
+
+	if (skb_vlan_tag_present(entskb)) {
+		struct nfqnl_msg_vlan pvlan;
+
+		pvlan.tci = entskb->vlan_tci;
+		pvlan.proto = entskb->vlan_proto;
+		if (nla_put(skb, NFQA_VLAN, sizeof(pvlan), &pvlan))
+			goto nla_put_failure;
+	}
+	if (entry->state.in && entskb->dev &&
+	    (entskb->mac_header < entskb->network_header)) {
+		int len = (int)(entskb->network_header - entskb->mac_header);
+		struct nlattr *nla;
+
+		if (skb_tailroom(skb) <
+		    NLA_ALIGN((sizeof(struct nlattr) + len)))
+			goto nla_put_failure;
+
+		nla = (struct nlattr *)
+			skb_put(skb, NLA_ALIGN(sizeof(struct nlattr) + len));
+		nla->nla_type = NFQA_L2HDR;
+		nla->nla_len = nla_attr_size(len);
+		memcpy(nla_data(nla), skb_mac_header(entskb), len);
+	}
+
+	return 0;
+
+nla_put_failure:
+	return -1;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -334,6 +387,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	if (entskb->tstamp.tv64)
 		size += nla_total_size(sizeof(struct nfqnl_msg_packet_timestamp));
 
+	if (entry->state.pf == PF_BRIDGE)
+		size += nfqnl_get_bridge_nla_len(entry);
+
 	if (entry->state.hook <= NF_INET_FORWARD ||
 	   (entry->state.hook == NF_INET_POST_ROUTING && entskb->sk == NULL))
 		csum_verify = !skb_csum_unnecessary(entskb);
@@ -499,6 +555,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 		}
 	}
 
+	if (entry->state.pf == PF_BRIDGE) {
+		if (nfqnl_put_bridge_nla(entry, skb))
+			goto nla_put_failure;
+	}
+
 	if (entskb->tstamp.tv64) {
 		struct nfqnl_msg_packet_timestamp ts;
 		struct timespec64 kts = ktime_to_timespec64(skb->tstamp);
@@ -536,7 +597,6 @@ 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);
-
 		if (skb_zerocopy(skb, entskb, data_len, hlen))
 			goto nla_put_failure;
 	}
@@ -1027,6 +1087,46 @@ static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
 	return ct;
 }
 
+static int nfqnl_push_l2hdr(unsigned char **payload, int *payload_len,
+			    struct nf_queue_entry *entry,
+			    const struct nlattr * const nfqa[])
+{
+	int mac_header_len = 0;
+
+	if (entry->skb->dev && nfqa[NFQA_L2HDR]) {
+		mac_header_len = nla_len(nfqa[NFQA_L2HDR]);
+		if (mac_header_len > 0) {
+			unsigned char *full_payload;
+			/* realloc provided payload to have room for the l2
+			 *  header
+			 */
+			full_payload = (char *)
+				kmalloc(((*payload_len) + mac_header_len),
+					GFP_ATOMIC);
+			if (!full_payload)
+				goto err_push;
+
+			memcpy(full_payload, nla_data(nfqa[NFQA_L2HDR]),
+			       mac_header_len);
+			memcpy(full_payload + mac_header_len, *payload,
+			       *payload_len);
+			*payload = full_payload;
+			(*payload_len) += mac_header_len;
+			/* push back mac header */
+			if (entry->skb->network_header > entry->skb->mac_header)
+				skb_push(entry->skb,
+					 (entry->skb->network_header -
+					  entry->skb->mac_header));
+			else
+				mac_header_len = 0;
+		}
+	}
+	return mac_header_len;
+
+err_push:
+	return 0;
+}
+
 static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
 			      struct sk_buff *skb,
 			      const struct nlmsghdr *nlh,
@@ -1068,14 +1168,34 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
 			ct = nfqnl_ct_parse(nfnl_ct, nlh, nfqa, entry, &ctinfo);
 	}
 
+	if (nfqa[NFQA_VLAN]) {
+		struct nfqnl_msg_vlan *pvlan = nla_data(nfqa[NFQA_VLAN]);
+
+		entry->skb->vlan_tci = pvlan->tci;
+		entry->skb->vlan_proto = pvlan->proto;
+	}
+
 	if (nfqa[NFQA_PAYLOAD]) {
-		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
-		int diff = payload_len - entry->skb->len;
+		int payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
+		unsigned char *payload = nla_data(nfqa[NFQA_PAYLOAD]);
+		int mac_header_len = 0;
+		int diff = 0;
+
+		if (entry->state.pf == PF_BRIDGE)
+			mac_header_len = nfqnl_push_l2hdr(&payload,
+							  &payload_len,
+							  entry, nfqa);
 
-		if (nfqnl_mangle(nla_data(nfqa[NFQA_PAYLOAD]),
-				 payload_len, entry, diff) < 0)
+		diff = payload_len - entry->skb->len;
+
+		if (nfqnl_mangle(payload, payload_len, entry, diff) < 0)
 			verdict = NF_DROP;
 
+		if (payload != nla_data(nfqa[NFQA_PAYLOAD]))
+			kfree(payload);
+		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

Re: [PATCH nf-next v2 2/2] netfilter: bridge: pass L2 header and VLAN as netlink attributes in queues to userspace

[Florian Westphal]

stephane.ml.bryant@gmail.com <stephane.ml.bryant@gmail.com> wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> -this creates 2 netlink attribute NLQA_VLAN and NLQA_L2HDR
> -these are filled up for the PF_BRIDGE family on the way to userspace, and
>  used on the way back to modify the original skb accordingly

Looks good, some comments below.

> @@ -295,6 +295,59 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>  	return seclen;
>  }
>  
> +static u32 nfqnl_get_bridge_nla_len(struct nf_queue_entry *entry)
> +{
> +	u32 nlalen = 0;
> +
> +	struct sk_buff *entskb = entry->skb;
> +
> +	if (skb_vlan_tag_present(entskb))
> +		nlalen += nla_total_size(sizeof(struct nfqnl_msg_vlan));
> +
> +	if (entry->state.in && entskb->dev &&

I don't think the state.in and entskb->dev tests are needed.

> +static int nfqnl_put_bridge_nla(struct nf_queue_entry *entry,
> +				struct sk_buff *skb)
> +{
> +	struct sk_buff *entskb = entry->skb;
> +

> +	if (entry->state.in && entskb->dev &&
> +	    (entskb->mac_header < entskb->network_header)) {

I'd suggest:

if (!skb_mac_header_was_set(entskb))
	return 0;

Another idea is to move the if (entry->state.pf == PF_BRIDGE) check
to this function to avoid increasing nfqnl_build_packet_message() size
too much.

> +		int len = (int)(entskb->network_header - entskb->mac_header);

> +		if (skb_tailroom(skb) <
> +		    NLA_ALIGN((sizeof(struct nlattr) + len)))
> +			goto nla_put_failure;
[..]

I'd suggest

return nla_put(skb, NFQA_L2HDR, len, skb_mac_header(entskb), len);

>  	if (nfqa[NFQA_PAYLOAD]) {
> -		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
> -		int diff = payload_len - entry->skb->len;
> +		int payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
> +		unsigned char *payload = nla_data(nfqa[NFQA_PAYLOAD]);
> +		int mac_header_len = 0;
> +		int diff = 0;
> +
> +		if (entry->state.pf == PF_BRIDGE)
> +			mac_header_len = nfqnl_push_l2hdr(&payload,
> +							  &payload_len,
> +							  entry, nfqa);
> 

Is there a reason why NFQA_L2HDR depends on NFQA_PAYLOAD presence?
I'd handle NFQA_L2HDR independently, after NFQA_PAYLOAD.

The replace procedure could look like this:

mac_header_len = entskb->mac_header - entskb->network_header;

if (mac_header_len != nla_len(nfqa[NFQA_L2HDR]))
	/* err */

memcpy(entskb->mac_header, nla_data(nfqa[NFQA_L2HDR], mac_header_len);

Yes, this doesn't allow increase/change of l2 header.
But I think we should not allow it for the time being.

We can always be more permissive later, possibly while adding
checks that there is a valid l2 eth header and that nothing will go wrong
with e.g. segmentation or fragmentation of skb on tx side.

You also might want to split this patch into two, one that adds the
'passive' features to make nfqueue work for logging, another one to add
the NFQA_L2HDR and VLAN manipulation.

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

[Florian Westphal]

stephane.ml.bryant@gmail.com <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.

I think that using empty stub functions is ok for now,
we can add checksum manipulation or mac header save/restore later on.

Re: [PATCH nf-next v2 2/2] netfilter: bridge: pass L2 header and VLAN as netlink attributes in queues to userspace

[Pablo Neira Ayuso]

On Thu, Feb 11, 2016 at 03:53:16PM +0100, stephane.ml.bryant@gmail.com wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> -this creates 2 netlink attribute NLQA_VLAN and NLQA_L2HDR
> -these are filled up for the PF_BRIDGE family on the way to userspace, and
>  used on the way back to modify the original skb accordingly
> 
> Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_queue.h |   7 ++
>  net/netfilter/nfnetlink_queue.c                | 130 ++++++++++++++++++++++++-
>  2 files changed, 132 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index b67a853..211fcdc 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -30,6 +30,11 @@ struct nfqnl_msg_packet_timestamp {
>  	__aligned_be64	usec;
>  };
>  
> +struct nfqnl_msg_vlan {
> +	__be16                  proto;
> +	__u16                   tci;
> +} __attribute__ ((packed));

I'd rather use nested attributes instead of passing structures through
netlink.

I'm aware we're using structure in the existing code, that decision
was made long time ago and we cannot change it. But as I said for new
code I'd rather see nested attributes. See nla_nest_start and nla_nest_end.

Thanks.