[PATCH/RFC net-next 0/2] net/sched: cls_flower: Support matching on ND

[PATCH/RFC net-next 0/2] net/sched: cls_flower: Support matching on ND

[Simon Horman]

Allow matching on Neighbour Discovery target IP, and source and
destination link-layer addresses for neighbour solicitation and
advertisement messages.

This is achieved by enhancing the flow dissector.

Sample usage:

tc qdisc add dev eth0 ingress

tc filter add dev eth0 protocol ipv6 parent ffff: flower \
	indev eth0 ip_proto icmpv6 type 136 code 0 \
	nd_target 2001:470:7eb3:403:201:8eff:fe22:8fea \
	nd_tll 00:01:8e:22:8f:ea action drop


Dependency:

This series depends on the following patch which is present in net
040587af3122 ("net/sched: cls_flower: Correct matching on ICMPv6 code")

Simon Horman (2):
  flow dissector: ND support
  net/sched: cls_flower: Support matching on ND

 include/net/flow_dissector.h | 14 ++++++++
 include/uapi/linux/pkt_cls.h |  7 ++++
 net/core/flow_dissector.c    | 83 +++++++++++++++++++++++++++++++++++++++++++-
 net/sched/cls_flower.c       | 42 +++++++++++++++++++++-
 4 files changed, 144 insertions(+), 2 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

Allow dissection of Neighbour Discovery target IP, and source and
destination link-layer addresses for neighbour solicitation and
advertisement messages.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/net/flow_dissector.h | 14 ++++++++
 net/core/flow_dissector.c    | 83 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index ac9703018a3a..b62e6484dd6b 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -140,6 +140,19 @@ struct flow_dissector_key_icmp {
 };
 
 /**
+ * flow_dissector_key_nd:
+ *	@ports: neighbour discovery fields
+ *		target: target IP address
+ *		sll: source link-layer address
+ *		tll: target link-layer address
+ */
+struct flow_dissector_key_nd {
+	struct in6_addr target;
+	unsigned char sll[ETH_ALEN];
+	unsigned char tll[ETH_ALEN];
+};
+
+/**
  * struct flow_dissector_key_eth_addrs:
  * @src: source Ethernet address
  * @dst: destination Ethernet address
@@ -157,6 +170,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
 	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
+	FLOW_DISSECTOR_KEY_ND, /* struct flow_dissector_key_nd */
 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
 	FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
 	FLOW_DISSECTOR_KEY_ARP, /* struct flow_dissector_key_arp */
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index c35aae13c8d2..745e03f180d0 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -113,6 +113,68 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static bool skb_flow_dissect_nd(const struct sk_buff *skb,
+				struct flow_dissector_key_nd *key_nd,
+				void *data, int nhoff, int hlen,
+				int icmp_len)
+{
+	const struct nd_msg *nd;
+	struct nd_msg *_nd;
+	int off;
+
+	nd = __skb_header_pointer(skb, nhoff, sizeof(_nd), data, hlen, &_nd);
+	if (!nd)
+		return false;
+
+	key_nd->target = nd->target;
+
+	off = sizeof(_nd);
+	while (icmp_len - off >= sizeof(struct nd_opt_hdr) + ETH_ALEN) {
+		const struct nd_opt_hdr *opt_hdr;
+		struct nd_opt_hdr *_opt_hdr;
+		unsigned char *ll_addr = NULL;
+		int opt_data_len;
+
+		opt_hdr = __skb_header_pointer(skb, nhoff + off,
+					       sizeof(_opt_hdr), data,
+					       hlen, &_opt_hdr);
+		if (!opt_hdr)
+			return false;
+		opt_data_len = opt_hdr->nd_opt_len * 8 - sizeof(_opt_hdr);
+		if (opt_data_len < 0)
+			return false;
+		off += sizeof(_opt_hdr);
+
+		if (opt_data_len == ETH_ALEN) {
+			if (opt_hdr->nd_opt_type == ND_OPT_SOURCE_LL_ADDR)
+				ll_addr = key_nd->sll;
+			else if (opt_hdr->nd_opt_type == ND_OPT_TARGET_LL_ADDR)
+				ll_addr = key_nd->tll;
+		}
+
+		if (ll_addr) {
+			const unsigned char *opt_data;
+			unsigned char _opt_data[ETH_ALEN];
+
+			/* Fail if the option is a duplicate */
+			if (!is_zero_ether_addr(ll_addr))
+				return false;
+
+			opt_data = __skb_header_pointer(skb, nhoff + off,
+							opt_data_len, data,
+							hlen, &_opt_data);
+			if (!opt_data)
+				return false;
+
+			ether_addr_copy(ll_addr, opt_data);
+		}
+
+		off += opt_data_len;
+	}
+
+	return true;
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
@@ -141,9 +203,11 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	struct flow_dissector_key_arp *key_arp;
 	struct flow_dissector_key_ports *key_ports;
 	struct flow_dissector_key_icmp *key_icmp;
+	struct flow_dissector_key_nd *key_nd;
 	struct flow_dissector_key_tags *key_tags;
 	struct flow_dissector_key_vlan *key_vlan;
 	struct flow_dissector_key_keyid *key_keyid;
+	int ipv6_payload_len = 0;
 	bool skip_vlan = false;
 	u8 ip_proto = 0;
 	bool ret;
@@ -232,6 +296,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 			goto out_bad;
 
 		ip_proto = iph->nexthdr;
+		ipv6_payload_len = ntohs(iph->payload_len);
 		nhoff += sizeof(struct ipv6hdr);
 
 		if (dissector_uses_key(flow_dissector,
@@ -556,6 +621,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	case NEXTHDR_HOP:
 	case NEXTHDR_ROUTING:
 	case NEXTHDR_DEST: {
+		int hdr_ext_len;
 		u8 _opthdr[2], *opthdr;
 
 		if (proto != htons(ETH_P_IPV6))
@@ -567,7 +633,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 			goto out_bad;
 
 		ip_proto = opthdr[0];
-		nhoff += (opthdr[1] + 1) << 3;
+		hdr_ext_len = (opthdr[1] + 1) << 3;
+		nhoff += hdr_ext_len;
+		ipv6_payload_len -= hdr_ext_len;
 
 		goto ip_proto_again;
 	}
@@ -586,6 +654,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		key_control->flags |= FLOW_DIS_IS_FRAGMENT;
 
 		nhoff += sizeof(_fh);
+		ipv6_payload_len -= sizeof(_fh);
 		ip_proto = fh->nexthdr;
 
 		if (!(fh->frag_off & htons(IP6_OFFSET))) {
@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 						     FLOW_DISSECTOR_KEY_ICMP,
 						     target_container);
 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
+
+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
+			key_nd = skb_flow_dissector_target(flow_dissector,
+							   FLOW_DISSECTOR_KEY_ND,
+							   target_container);
+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
+						  hlen, ipv6_payload_len)))
+				goto out_bad;
+		}
 	}
 
 out_good:
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Eric Dumazet]

On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
> Allow dissection of Neighbour Discovery target IP, and source and
> destination link-layer addresses for neighbour solicitation and
> advertisement messages.
> 
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> ---

Hi Simon

Why is this needed ?

Any code added in flow dissector needs to be extra careful,
we had various packet of deaths errors recently in this area.

Thanks.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

[Repost due to gmail account problem]

On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
> > Allow dissection of Neighbour Discovery target IP, and source and
> > destination link-layer addresses for neighbour solicitation and
> > advertisement messages.
> > 
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > ---
> 
> Hi Simon
> 
> Why is this needed ?
> 
> Any code added in flow dissector needs to be extra careful,
> we had various packet of deaths errors recently in this area.

Hi Eric,

there some activity to allow programming of OvS flows in hardware via TC
with the flower classifier. As the ND fields in this patch are part of the
OvS flow key I would like them considered for additions to flower and thus
the dissector to allow compatibility with OvS.

I apologise if any 'deaths' have resulted from my recent work on the
dissector. I am of course very open to ideas on how to avoid any future
incidents.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
> [Repost due to gmail account problem]
>
> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>> > Allow dissection of Neighbour Discovery target IP, and source and
>> > destination link-layer addresses for neighbour solicitation and
>> > advertisement messages.
>> >
>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> > ---
>>
>> Hi Simon
>>
>> Why is this needed ?
>>
>> Any code added in flow dissector needs to be extra careful,
>> we had various packet of deaths errors recently in this area.
>
> Hi Eric,
>
> there some activity to allow programming of OvS flows in hardware via TC
> with the flower classifier. As the ND fields in this patch are part of the
> OvS flow key I would like them considered for additions to flower and thus
> the dissector to allow compatibility with OvS.
>
Given that ARP is already there it seems only "fair" to have ND also.
But Eric is correct, this is quite a sensitive area of code.

> I apologise if any 'deaths' have resulted from my recent work on the
> dissector. I am of course very open to ideas on how to avoid any future
> incidents.

That's a tough problem. flow_dissector started off as simple mechanism
to just identify actual flows (really just TCP and UDP packets) for
the purposes of packet steering. But given the benefits of its
location low in the stack and the open ended capabilities for parsing
it seems to have mushroomed into a general catchall to parse a whole
bunch of different protocols. A lot of these go beyond simply
identifying flows (ICMP parsing, ARP, or ND as in your patches). These
new use cases may be valid, but the result is a convoluted function (>
500 LOC by my count) and it seems to be quite easy to have subtle bugs
mostly in edge cases, several of which could have been exploited in
DDOS attacks.

At some point we need to stop adding new protocols to parse in
__skb_flow_dissect and push the processing back into the protocol
modules with a callout interface from flow_dissector (for instance if
we ever want VXLAN parsing in flow dissector this is the only
reasonable way to do it). That moves the complexity but doesn't solve
the problem of buggy code in this critical path. An alternative might
be to put a cap on flow_dissector and add a hook to BPF program to
allow parsing of new protocols. This has the advantage of providing an
constrained interface that could eliminate possibility of some types
of bugs we've seen. Also, this allows adding support for "user"
protocols that the kernel might not even know about (QUIC comes to
mind).

Tom

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:
>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> [Repost due to gmail account problem]
>>
>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>>> > Allow dissection of Neighbour Discovery target IP, and source and
>>> > destination link-layer addresses for neighbour solicitation and
>>> > advertisement messages.
>>> >
>>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
>>> > ---
>>>
>>> Hi Simon
>>>
>>> Why is this needed ?
>>>
>>> Any code added in flow dissector needs to be extra careful,
>>> we had various packet of deaths errors recently in this area.
>>
>> Hi Eric,
>>
>> there some activity to allow programming of OvS flows in hardware via TC
>> with the flower classifier. As the ND fields in this patch are part of the
>> OvS flow key I would like them considered for additions to flower and thus
>> the dissector to allow compatibility with OvS.
>>
>Given that ARP is already there it seems only "fair" to have ND also.
>But Eric is correct, this is quite a sensitive area of code.
>
>> I apologise if any 'deaths' have resulted from my recent work on the
>> dissector. I am of course very open to ideas on how to avoid any future
>> incidents.
>
>That's a tough problem. flow_dissector started off as simple mechanism
>to just identify actual flows (really just TCP and UDP packets) for
>the purposes of packet steering. But given the benefits of its
>location low in the stack and the open ended capabilities for parsing
>it seems to have mushroomed into a general catchall to parse a whole
>bunch of different protocols. A lot of these go beyond simply
>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
>new use cases may be valid, but the result is a convoluted function (>
>500 LOC by my count) and it seems to be quite easy to have subtle bugs
>mostly in edge cases, several of which could have been exploited in
>DDOS attacks.

Agreed that we probably came to a point when we need to split
__skb_flow_dissect into modular and pluggable pieces. Will not be
trivial though.

Also note that it depends on the __skb_flow_dissect user which code is
actually used or not. For the critical path, that keys are defined by:
flow_keys_dissector_keys

Most of the code Simon is adding is noop for non-flower usecase if:
dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false


>
>At some point we need to stop adding new protocols to parse in
>__skb_flow_dissect and push the processing back into the protocol
>modules with a callout interface from flow_dissector (for instance if
>we ever want VXLAN parsing in flow dissector this is the only
>reasonable way to do it). That moves the complexity but doesn't solve
>the problem of buggy code in this critical path. An alternative might
>be to put a cap on flow_dissector and add a hook to BPF program to

Loks like BPF is becoming an answer for everything these days :O


>allow parsing of new protocols. This has the advantage of providing an
>constrained interface that could eliminate possibility of some types
>of bugs we've seen. Also, this allows adding support for "user"
>protocols that the kernel might not even know about (QUIC comes to
>mind).

Not sure it is wise to make life easier for the proprietary
out-of-tree beasts...

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Thu, Feb 2, 2017 at 9:48 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:
>>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>> [Repost due to gmail account problem]
>>>
>>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>>>> > Allow dissection of Neighbour Discovery target IP, and source and
>>>> > destination link-layer addresses for neighbour solicitation and
>>>> > advertisement messages.
>>>> >
>>>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
>>>> > ---
>>>>
>>>> Hi Simon
>>>>
>>>> Why is this needed ?
>>>>
>>>> Any code added in flow dissector needs to be extra careful,
>>>> we had various packet of deaths errors recently in this area.
>>>
>>> Hi Eric,
>>>
>>> there some activity to allow programming of OvS flows in hardware via TC
>>> with the flower classifier. As the ND fields in this patch are part of the
>>> OvS flow key I would like them considered for additions to flower and thus
>>> the dissector to allow compatibility with OvS.
>>>
>>Given that ARP is already there it seems only "fair" to have ND also.
>>But Eric is correct, this is quite a sensitive area of code.
>>
>>> I apologise if any 'deaths' have resulted from my recent work on the
>>> dissector. I am of course very open to ideas on how to avoid any future
>>> incidents.
>>
>>That's a tough problem. flow_dissector started off as simple mechanism
>>to just identify actual flows (really just TCP and UDP packets) for
>>the purposes of packet steering. But given the benefits of its
>>location low in the stack and the open ended capabilities for parsing
>>it seems to have mushroomed into a general catchall to parse a whole
>>bunch of different protocols. A lot of these go beyond simply
>>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
>>new use cases may be valid, but the result is a convoluted function (>
>>500 LOC by my count) and it seems to be quite easy to have subtle bugs
>>mostly in edge cases, several of which could have been exploited in
>>DDOS attacks.
>
> Agreed that we probably came to a point when we need to split
> __skb_flow_dissect into modular and pluggable pieces. Will not be
> trivial though.
>
> Also note that it depends on the __skb_flow_dissect user which code is
> actually used or not. For the critical path, that keys are defined by:
> flow_keys_dissector_keys
>
True, but the code doesn't separate out the critical path from all
these extended features which is resulted in a jumbled mess with no
modularity to speak of :-(

> Most of the code Simon is adding is noop for non-flower usecase if:
> dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false
>
Sure, but that just makes this code corner cases which means it's hard
to maintain and harder to find bugs in the long run.

>
>>
>>At some point we need to stop adding new protocols to parse in
>>__skb_flow_dissect and push the processing back into the protocol
>>modules with a callout interface from flow_dissector (for instance if
>>we ever want VXLAN parsing in flow dissector this is the only
>>reasonable way to do it). That moves the complexity but doesn't solve
>>the problem of buggy code in this critical path. An alternative might
>>be to put a cap on flow_dissector and add a hook to BPF program to
>
> Loks like BPF is becoming an answer for everything these days :O
>
In this case it makes sense though, we can't just continue accepting
every poor little narrow use-case protocol that comes along into the
kernel :-)

>
>>allow parsing of new protocols. This has the advantage of providing an
>>constrained interface that could eliminate possibility of some types
>>of bugs we've seen. Also, this allows adding support for "user"
>>protocols that the kernel might not even know about (QUIC comes to
>>mind).
>
> Not sure it is wise to make life easier for the proprietary
> out-of-tree beasts...

It's going to be a problem with a whole host of application level
protocols especially those run over UDP. QUIC is a great example. The
actual protocol will probably only ever run in userspace, but it is
inevitable that we want to provide targeted kernel support for packet
steering. filtering, GRO/GSO if they have such things. Instead of
implementing this in a specialized QUIC module, it will most likely
make everyone happier to add these in a generic protocol-agnostic way.
>From QUIC POV they want to minimize any dependencies on the kernel and
be able to iterate quickly, from a kernel POV we really don't want to
have to explicitly support an endless stream of protocols like this.

Tom

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Eric Dumazet]

On Thu, 2017-02-02 at 10:36 -0800, Tom Herbert wrote:

> It's going to be a problem with a whole host of application level
> protocols especially those run over UDP. QUIC is a great example. The
> actual protocol will probably only ever run in userspace, but it is
> inevitable that we want to provide targeted kernel support for packet
> steering. filtering, GRO/GSO if they have such things. Instead of
> implementing this in a specialized QUIC module, it will most likely
> make everyone happier to add these in a generic protocol-agnostic way.
> From QUIC POV they want to minimize any dependencies on the kernel and
> be able to iterate quickly, from a kernel POV we really don't want to
> have to explicitly support an endless stream of protocols like this.

Small note here : Google does not intend to add QUIC knowledge anywhere
in the kernel.

SO_ATTACH_REUSEPORT_EBPF can be used if someone wants to take a deep
look at QUIC header (and presumably hash the connection-id) to select
one socket among the group.

Thanks.

 

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Thu, Feb 2, 2017 at 10:56 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-02-02 at 10:36 -0800, Tom Herbert wrote:
>
>> It's going to be a problem with a whole host of application level
>> protocols especially those run over UDP. QUIC is a great example. The
>> actual protocol will probably only ever run in userspace, but it is
>> inevitable that we want to provide targeted kernel support for packet
>> steering. filtering, GRO/GSO if they have such things. Instead of
>> implementing this in a specialized QUIC module, it will most likely
>> make everyone happier to add these in a generic protocol-agnostic way.
>> From QUIC POV they want to minimize any dependencies on the kernel and
>> be able to iterate quickly, from a kernel POV we really don't want to
>> have to explicitly support an endless stream of protocols like this.
>
> Small note here : Google does not intend to add QUIC knowledge anywhere
> in the kernel.
>
Someone else might want to do that. It's also conceivable that we
could see NIC vendors trying to offload parts of QUIC and they would
want support in the kernel for that.

Tom

> SO_ATTACH_REUSEPORT_EBPF can be used if someone wants to take a deep
> look at QUIC header (and presumably hash the connection-id) to select
> one socket among the group.
>
> Thanks.
>
>
>

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Thu, Feb 02, 2017 at 10:36:31AM -0800, Tom Herbert wrote:
> On Thu, Feb 2, 2017 at 9:48 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> > Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:
> >>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >>> [Repost due to gmail account problem]
> >>>
> >>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
> >>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
> >>>> > Allow dissection of Neighbour Discovery target IP, and source and
> >>>> > destination link-layer addresses for neighbour solicitation and
> >>>> > advertisement messages.
> >>>> >
> >>>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >>>> > ---
> >>>>
> >>>> Hi Simon
> >>>>
> >>>> Why is this needed ?
> >>>>
> >>>> Any code added in flow dissector needs to be extra careful,
> >>>> we had various packet of deaths errors recently in this area.
> >>>
> >>> Hi Eric,
> >>>
> >>> there some activity to allow programming of OvS flows in hardware via TC
> >>> with the flower classifier. As the ND fields in this patch are part of the
> >>> OvS flow key I would like them considered for additions to flower and thus
> >>> the dissector to allow compatibility with OvS.
> >>>
> >>Given that ARP is already there it seems only "fair" to have ND also.
> >>But Eric is correct, this is quite a sensitive area of code.
> >>
> >>> I apologise if any 'deaths' have resulted from my recent work on the
> >>> dissector. I am of course very open to ideas on how to avoid any future
> >>> incidents.
> >>
> >>That's a tough problem. flow_dissector started off as simple mechanism
> >>to just identify actual flows (really just TCP and UDP packets) for
> >>the purposes of packet steering. But given the benefits of its
> >>location low in the stack and the open ended capabilities for parsing
> >>it seems to have mushroomed into a general catchall to parse a whole
> >>bunch of different protocols. A lot of these go beyond simply
> >>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
> >>new use cases may be valid, but the result is a convoluted function (>
> >>500 LOC by my count) and it seems to be quite easy to have subtle bugs
> >>mostly in edge cases, several of which could have been exploited in
> >>DDOS attacks.
> >
> > Agreed that we probably came to a point when we need to split
> > __skb_flow_dissect into modular and pluggable pieces. Will not be
> > trivial though.
> >
> > Also note that it depends on the __skb_flow_dissect user which code is
> > actually used or not. For the critical path, that keys are defined by:
> > flow_keys_dissector_keys
> >
> True, but the code doesn't separate out the critical path from all
> these extended features which is resulted in a jumbled mess with no
> modularity to speak of :-(
> 
> > Most of the code Simon is adding is noop for non-flower usecase if:
> > dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false
> >
> Sure, but that just makes this code corner cases which means it's hard
> to maintain and harder to find bugs in the long run.

I think that there is a bit of tension here between having generic reusable
code on the one hand and having a small robust implementation for critical
code.

>From my point of view having the flow dissector shared makes complete sense
when all the users need a similar set of keys. This was mostly the case
until one user, flower, started growing the number of keys it can use - I am
partly responsible for that. So now we have one user that is placing a
burden on the complexity and recently the robustness of code that is relied
on by many users.

I think that Jiri's point regarding dissector_uses_key() is the nub of the
issue. On the one hand I believe, given my recent updates to the code in
question, that covering more code by such conditionals - without necessarily
adding more instances of the conditionals - would lead to more robust code
for the current users. But as Tom points out paradoxically it may also lead
to less robust code for paths that aren't executed much - specifically
paths being added to support keys only used by flower.

I suspect a similar paradox would exists for other, likely more complex,
refactoring efforts.

>From my side I think that using dissector_uses_key() to cover more code
makes sense as it allow features to be added to flower and for bugs in them
to be ironed out with lower risk of effecting other users along the way.

> >>At some point we need to stop adding new protocols to parse in
> >>__skb_flow_dissect and push the processing back into the protocol
> >>modules with a callout interface from flow_dissector (for instance if
> >>we ever want VXLAN parsing in flow dissector this is the only
> >>reasonable way to do it). That moves the complexity but doesn't solve
> >>the problem of buggy code in this critical path. An alternative might
> >>be to put a cap on flow_dissector and add a hook to BPF program to
> >
> > Loks like BPF is becoming an answer for everything these days :O
> >
> In this case it makes sense though, we can't just continue accepting
> every poor little narrow use-case protocol that comes along into the
> kernel :-)

Maybe. I'm not sure that I think we should exclude narrow cases. But
I have no strong feeling on that at this time.

To lay my intentions on the table: I am interested in enhancing flower and
by implication the flow dissector to support the header fields and
protocols supported by OvS. I can check the list but possibly ND is the
narrowest case protocol there.

> >>allow parsing of new protocols. This has the advantage of providing an
> >>constrained interface that could eliminate possibility of some types
> >>of bugs we've seen. Also, this allows adding support for "user"
> >>protocols that the kernel might not even know about (QUIC comes to
> >>mind).
> >
> > Not sure it is wise to make life easier for the proprietary
> > out-of-tree beasts...
> 
> It's going to be a problem with a whole host of application level
> protocols especially those run over UDP. QUIC is a great example. The
> actual protocol will probably only ever run in userspace, but it is
> inevitable that we want to provide targeted kernel support for packet
> steering. filtering, GRO/GSO if they have such things. Instead of
> implementing this in a specialized QUIC module, it will most likely
> make everyone happier to add these in a generic protocol-agnostic way.
> From QUIC POV they want to minimize any dependencies on the kernel and
> be able to iterate quickly, from a kernel POV we really don't want to
> have to explicitly support an endless stream of protocols like this.
> 
> Tom

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Mon, Feb 6, 2017 at 1:12 AM, Simon Horman <simon.horman@netronome.com> wrote:
> On Thu, Feb 02, 2017 at 10:36:31AM -0800, Tom Herbert wrote:
>> On Thu, Feb 2, 2017 at 9:48 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:
>> >>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> >>> [Repost due to gmail account problem]
>> >>>
>> >>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>> >>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>> >>>> > Allow dissection of Neighbour Discovery target IP, and source and
>> >>>> > destination link-layer addresses for neighbour solicitation and
>> >>>> > advertisement messages.
>> >>>> >
>> >>>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >>>> > ---
>> >>>>
>> >>>> Hi Simon
>> >>>>
>> >>>> Why is this needed ?
>> >>>>
>> >>>> Any code added in flow dissector needs to be extra careful,
>> >>>> we had various packet of deaths errors recently in this area.
>> >>>
>> >>> Hi Eric,
>> >>>
>> >>> there some activity to allow programming of OvS flows in hardware via TC
>> >>> with the flower classifier. As the ND fields in this patch are part of the
>> >>> OvS flow key I would like them considered for additions to flower and thus
>> >>> the dissector to allow compatibility with OvS.
>> >>>
>> >>Given that ARP is already there it seems only "fair" to have ND also.
>> >>But Eric is correct, this is quite a sensitive area of code.
>> >>
>> >>> I apologise if any 'deaths' have resulted from my recent work on the
>> >>> dissector. I am of course very open to ideas on how to avoid any future
>> >>> incidents.
>> >>
>> >>That's a tough problem. flow_dissector started off as simple mechanism
>> >>to just identify actual flows (really just TCP and UDP packets) for
>> >>the purposes of packet steering. But given the benefits of its
>> >>location low in the stack and the open ended capabilities for parsing
>> >>it seems to have mushroomed into a general catchall to parse a whole
>> >>bunch of different protocols. A lot of these go beyond simply
>> >>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
>> >>new use cases may be valid, but the result is a convoluted function (>
>> >>500 LOC by my count) and it seems to be quite easy to have subtle bugs
>> >>mostly in edge cases, several of which could have been exploited in
>> >>DDOS attacks.
>> >
>> > Agreed that we probably came to a point when we need to split
>> > __skb_flow_dissect into modular and pluggable pieces. Will not be
>> > trivial though.
>> >
>> > Also note that it depends on the __skb_flow_dissect user which code is
>> > actually used or not. For the critical path, that keys are defined by:
>> > flow_keys_dissector_keys
>> >
>> True, but the code doesn't separate out the critical path from all
>> these extended features which is resulted in a jumbled mess with no
>> modularity to speak of :-(
>>
>> > Most of the code Simon is adding is noop for non-flower usecase if:
>> > dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false
>> >
>> Sure, but that just makes this code corner cases which means it's hard
>> to maintain and harder to find bugs in the long run.
>
> I think that there is a bit of tension here between having generic reusable
> code on the one hand and having a small robust implementation for critical
> code.
>
> From my point of view having the flow dissector shared makes complete sense
> when all the users need a similar set of keys. This was mostly the case
> until one user, flower, started growing the number of keys it can use - I am
> partly responsible for that. So now we have one user that is placing a
> burden on the complexity and recently the robustness of code that is relied
> on by many users.
>
> I think that Jiri's point regarding dissector_uses_key() is the nub of the
> issue. On the one hand I believe, given my recent updates to the code in
> question, that covering more code by such conditionals - without necessarily
> adding more instances of the conditionals - would lead to more robust code
> for the current users. But as Tom points out paradoxically it may also lead
> to less robust code for paths that aren't executed much - specifically
> paths being added to support keys only used by flower.
>
> I suspect a similar paradox would exists for other, likely more complex,
> refactoring efforts.
>
> From my side I think that using dissector_uses_key() to cover more code
> makes sense as it allow features to be added to flower and for bugs in them
> to be ironed out with lower risk of effecting other users along the way.
>
>> >>At some point we need to stop adding new protocols to parse in
>> >>__skb_flow_dissect and push the processing back into the protocol
>> >>modules with a callout interface from flow_dissector (for instance if
>> >>we ever want VXLAN parsing in flow dissector this is the only
>> >>reasonable way to do it). That moves the complexity but doesn't solve
>> >>the problem of buggy code in this critical path. An alternative might
>> >>be to put a cap on flow_dissector and add a hook to BPF program to
>> >
>> > Loks like BPF is becoming an answer for everything these days :O
>> >
>> In this case it makes sense though, we can't just continue accepting
>> every poor little narrow use-case protocol that comes along into the
>> kernel :-)
>
> Maybe. I'm not sure that I think we should exclude narrow cases. But
> I have no strong feeling on that at this time.
>
> To lay my intentions on the table: I am interested in enhancing flower and
> by implication the flow dissector to support the header fields and
> protocols supported by OvS. I can check the list but possibly ND is the
> narrowest case protocol there.
>
Okay, but can you give us an idea of how many more of these protocols
are going to be added to flow_dissector. TBH I'm not very enthused
about making more flow_dissector more complex for the benefit of OVS.

Tom

>> >>allow parsing of new protocols. This has the advantage of providing an
>> >>constrained interface that could eliminate possibility of some types
>> >>of bugs we've seen. Also, this allows adding support for "user"
>> >>protocols that the kernel might not even know about (QUIC comes to
>> >>mind).
>> >
>> > Not sure it is wise to make life easier for the proprietary
>> > out-of-tree beasts...
>>
>> It's going to be a problem with a whole host of application level
>> protocols especially those run over UDP. QUIC is a great example. The
>> actual protocol will probably only ever run in userspace, but it is
>> inevitable that we want to provide targeted kernel support for packet
>> steering. filtering, GRO/GSO if they have such things. Instead of
>> implementing this in a specialized QUIC module, it will most likely
>> make everyone happier to add these in a generic protocol-agnostic way.
>> From QUIC POV they want to minimize any dependencies on the kernel and
>> be able to iterate quickly, from a kernel POV we really don't want to
>> have to explicitly support an endless stream of protocols like this.
>>
>> Tom

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Tue, 7 Feb 2017 09:36:20 -0800

> Okay, but can you give us an idea of how many more of these protocols
> are going to be added to flow_dissector. TBH I'm not very enthused
> about making more flow_dissector more complex for the benefit of OVS.

Especially since the kernel datapath of OVS has been marked
experimental.

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Tue, Feb 07, 2017 at 12:38:31PM -0500, David Miller wrote:
> From: Tom Herbert <tom@herbertland.com>
> Date: Tue, 7 Feb 2017 09:36:20 -0800
> 
> > Okay, but can you give us an idea of how many more of these protocols
> > are going to be added to flow_dissector. TBH I'm not very enthused
> > about making more flow_dissector more complex for the benefit of OVS.
> 
> Especially since the kernel datapath of OVS has been marked
> experimental.

Hi Dave, Hi Tom,

Firstly I'd like to apologise for posing what has turned out to be a
somewhat divisive patch.

After looking through things a little more I think the simple answer to
Tom's question is only ND. But there are also some fields of already
supported protocols which are covered in the OvS flow key but not the flow
dissector.

My analysis of OvS flow key fields for non-tunnel packet data - which I
think is the extent of what is relevant to the flow dissector - yields the
following list:

  New protocols:
  * ND (this patch)

  Fields of already supported protocols:
  * MPLS.lse (currently the label of the LSE is handled by the dissector
              so this could also be described as MPLS.lse.{tc,s,ttl})
  * IPV4.tos
  * IPV4.ttl
  * IPV6.hlimit
  * IPV6.tclass

  I do expect that some of the above will not be appropriate for existing
  users of the flow dissector; e.g. IPV4.ttl does not seem much use when
  calculating the hash of a flow for steering purposes. And I do not yet
  have a good idea of how to approach that beyond using different dissector
  keys.

  Further support for to already supported fields.
  * IPV[46].frag (supported by flow dissector : 1st frag flag;
                  not supported by flow dissector: subsequent frag flag)

>From my point of view some parts of what is above is more important than
others. In particular ND is probably not particularly important at least at
this time. I would be happy to withdraw this patchset if the complexity is
not deemed worth it at this time.


I'd like to take a moment to give a little background and restate that my
goal is not to create a burden for existing users of the flow dissector
- or any other part of the stack.

Netronome has worked on various approaches to an upstream offload of OvS.
One was hooks added to the OvS kernel datapath; an idea which was rejected
at least twice but none the less I'd be happy to revisit if there is
interest in it.

The result of trying various approaches is that it seems the most
acceptable is to use TC for programming flows into hardware which is where
my work on TC flower and the flow dissector is coming from.  A key part of
the reasoning being, as I understand it, that the flows could also be
programmed into hardware for non-OvS use-cases; technology that could be
useful e.g. in a post-OvS world.

I think the above paragraph gets back to Tom's original question regarding
making things more complex just for OvS (use-cases). Possibly ND is an edge
case even for OvS and on reflection my timing for posting it seems to have
been less than ideal.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

Responding to myself as my previous post doesn't seem to have hit netdev
for some reason.

On Wed, Feb 08, 2017 at 10:28:24AM +0100, Simon Horman wrote:
> On Tue, Feb 07, 2017 at 12:38:31PM -0500, David Miller wrote:
> > From: Tom Herbert <tom@herbertland.com>
> > Date: Tue, 7 Feb 2017 09:36:20 -0800
> > 
> > > Okay, but can you give us an idea of how many more of these protocols
> > > are going to be added to flow_dissector. TBH I'm not very enthused
> > > about making more flow_dissector more complex for the benefit of OVS.
> > 
> > Especially since the kernel datapath of OVS has been marked
> > experimental.
> 
> Hi Dave, Hi Tom,
> 
> Firstly I'd like to apologise for posing what has turned out to be a
> somewhat divisive patch.
> 
> After looking through things a little more I think the simple answer to
> Tom's question is only ND. But there are also some fields of already
> supported protocols which are covered in the OvS flow key but not the flow
> dissector.
> 
> My analysis of OvS flow key fields for non-tunnel packet data - which I
> think is the extent of what is relevant to the flow dissector - yields the
> following list:
> 
>   New protocols:
>   * ND (this patch)
> 
>   Fields of already supported protocols:
>   * MPLS.lse (currently the label of the LSE is handled by the dissector
>               so this could also be described as MPLS.lse.{tc,s,ttl})
>   * IPV4.tos
>   * IPV4.ttl
>   * IPV6.hlimit
>   * IPV6.tclass
> 
>   I do expect that some of the above will not be appropriate for existing
>   users of the flow dissector; e.g. IPV4.ttl does not seem much use when
>   calculating the hash of a flow for steering purposes. And I do not yet
>   have a good idea of how to approach that beyond using different dissector
>   keys.
> 
>   Further support for to already supported fields.
>   * IPV[46].frag (supported by flow dissector : 1st frag flag;
>                   not supported by flow dissector: subsequent frag flag)
> 
> From my point of view some parts of what is above is more important than
> others. In particular ND is probably not particularly important at least at
> this time. I would be happy to withdraw this patchset if the complexity is
> not deemed worth it at this time.
> 
> 
> I'd like to take a moment to give a little background and restate that my
> goal is not to create a burden for existing users of the flow dissector
> - or any other part of the stack.
> 
> Netronome has worked on various approaches to an upstream offload of OvS.
> One was hooks added to the OvS kernel datapath; an idea which was rejected
> at least twice but none the less I'd be happy to revisit if there is
> interest in it.
> 
> The result of trying various approaches is that it seems the most
> acceptable is to use TC for programming flows into hardware which is where
> my work on TC flower and the flow dissector is coming from.  A key part of
> the reasoning being, as I understand it, that the flows could also be
> programmed into hardware for non-OvS use-cases; technology that could be
> useful e.g. in a post-OvS world.
> 
> I think the above paragraph gets back to Tom's original question regarding
> making things more complex just for OvS (use-cases). Possibly ND is an edge
> case even for OvS and on reflection my timing for posting it seems to have
> been less than ideal.

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
> On Tue, Feb 07, 2017 at 12:38:31PM -0500, David Miller wrote:
>> From: Tom Herbert <tom@herbertland.com>
>> Date: Tue, 7 Feb 2017 09:36:20 -0800
>>
>> > Okay, but can you give us an idea of how many more of these protocols
>> > are going to be added to flow_dissector. TBH I'm not very enthused
>> > about making more flow_dissector more complex for the benefit of OVS.
>>
>> Especially since the kernel datapath of OVS has been marked
>> experimental.
>
> Hi Dave, Hi Tom,
>
> Firstly I'd like to apologise for posing what has turned out to be a
> somewhat divisive patch.
>
> After looking through things a little more I think the simple answer to
> Tom's question is only ND. But there are also some fields of already
> supported protocols which are covered in the OvS flow key but not the flow
> dissector.
>
> My analysis of OvS flow key fields for non-tunnel packet data - which I
> think is the extent of what is relevant to the flow dissector - yields the
> following list:
>
>   New protocols:
>   * ND (this patch)
>
>   Fields of already supported protocols:
>   * MPLS.lse (currently the label of the LSE is handled by the dissector
>               so this could also be described as MPLS.lse.{tc,s,ttl})
>   * IPV4.tos
>   * IPV4.ttl
>   * IPV6.hlimit
>   * IPV6.tclass
>
>   I do expect that some of the above will not be appropriate for existing
>   users of the flow dissector; e.g. IPV4.ttl does not seem much use when
>   calculating the hash of a flow for steering purposes. And I do not yet
>   have a good idea of how to approach that beyond using different dissector
>   keys.
>
>   Further support for to already supported fields.
>   * IPV[46].frag (supported by flow dissector : 1st frag flag;
>                   not supported by flow dissector: subsequent frag flag)
>
> From my point of view some parts of what is above is more important than
> others. In particular ND is probably not particularly important at least at
> this time. I would be happy to withdraw this patchset if the complexity is
> not deemed worth it at this time.
>
>
> I'd like to take a moment to give a little background and restate that my
> goal is not to create a burden for existing users of the flow dissector
> - or any other part of the stack.
>
> Netronome has worked on various approaches to an upstream offload of OvS.
> One was hooks added to the OvS kernel datapath; an idea which was rejected
> at least twice but none the less I'd be happy to revisit if there is
> interest in it.
>
> The result of trying various approaches is that it seems the most
> acceptable is to use TC for programming flows into hardware which is where
> my work on TC flower and the flow dissector is coming from.  A key part of
> the reasoning being, as I understand it, that the flows could also be
> programmed into hardware for non-OvS use-cases; technology that could be
> useful e.g. in a post-OvS world.
>
It's not an issue specific OVS, it's part of a broader problem in that
we are see more case where the core kernel paths are modified to
support somewhat narrow use case functionality (like OVS, advanced HW
accelerations, etc.). As we have seen recently this can make
maintainability and backports very difficult (see my comments about
backporting mlx5).

> I think the above paragraph gets back to Tom's original question regarding
> making things more complex just for OvS (use-cases). Possibly ND is an edge
> case even for OvS and on reflection my timing for posting it seems to have
> been less than ideal.

If it wasn't ND it would be something else... with all the activity
happening in networking features and HW this is a timely discussion.
Flow dissector presents a good example of a function that might become
a dumping ground for an endless stream of features if we don't figure
out how exercise some restraint.

Tom

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Wed, 8 Feb 2017 10:33:46 -0800

> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> I think the above paragraph gets back to Tom's original question regarding
>> making things more complex just for OvS (use-cases). Possibly ND is an edge
>> case even for OvS and on reflection my timing for posting it seems to have
>> been less than ideal.
> 
> If it wasn't ND it would be something else... with all the activity
> happening in networking features and HW this is a timely discussion.
> Flow dissector presents a good example of a function that might become
> a dumping ground for an endless stream of features if we don't figure
> out how exercise some restraint.

I agree on most points.

But, I would say that in this specific case, since we have ARP support in
there already it behooves us to support the ipv6 side in the form of ND
too.

Then we can put a line in the sand and say that future feature additions
in this area require serious discussion.

Ok Tom?

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Wed, Feb 8, 2017 at 10:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <tom@herbertland.com>
> Date: Wed, 8 Feb 2017 10:33:46 -0800
>
>> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>> I think the above paragraph gets back to Tom's original question regarding
>>> making things more complex just for OvS (use-cases). Possibly ND is an edge
>>> case even for OvS and on reflection my timing for posting it seems to have
>>> been less than ideal.
>>
>> If it wasn't ND it would be something else... with all the activity
>> happening in networking features and HW this is a timely discussion.
>> Flow dissector presents a good example of a function that might become
>> a dumping ground for an endless stream of features if we don't figure
>> out how exercise some restraint.
>
> I agree on most points.
>
> But, I would say that in this specific case, since we have ARP support in
> there already it behooves us to support the ipv6 side in the form of ND
> too.
>
> Then we can put a line in the sand and say that future feature additions
> in this area require serious discussion.
>
> Ok Tom?

Right, ND is okay on the basis that we already have ARP (although I
still may grumble from time to time that ARP, ND, and ICMP are being
identified as flows ;-) ).

I think there are two projects in the are that someone, maybe an
aspiring kernel network developer, might want to look into if they
have the time:

- Inevitably someone will want to support VXLAN or other UDP
encapsulations in flow dissector. The only correct way to do this is
going to be to do a lookup on UDP socket and have a flow_dissector
function related to the socket. This is the model for dealing with UDP
encapsulations in GRO that could be extended for flow dissection. We
cannot hard code port numbers in flow_dissector. The interesting part
here will be making a robust interface to avoid the pitfalls we've
seen in some of the protocols in flow_dissector.

- Allow calling a BPF function to do custom flow dissection. IIRC
there someone (Daniel?) had already implement flow_dissector in BPF
with pretty good results.

Tom

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Wed, Feb 08, 2017 at 08:10:06PM CET, tom@herbertland.com wrote:
>On Wed, Feb 8, 2017 at 10:54 AM, David Miller <davem@davemloft.net> wrote:
>> From: Tom Herbert <tom@herbertland.com>
>> Date: Wed, 8 Feb 2017 10:33:46 -0800
>>
>>> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>>> I think the above paragraph gets back to Tom's original question regarding
>>>> making things more complex just for OvS (use-cases). Possibly ND is an edge
>>>> case even for OvS and on reflection my timing for posting it seems to have
>>>> been less than ideal.
>>>
>>> If it wasn't ND it would be something else... with all the activity
>>> happening in networking features and HW this is a timely discussion.
>>> Flow dissector presents a good example of a function that might become
>>> a dumping ground for an endless stream of features if we don't figure
>>> out how exercise some restraint.
>>
>> I agree on most points.
>>
>> But, I would say that in this specific case, since we have ARP support in
>> there already it behooves us to support the ipv6 side in the form of ND
>> too.
>>
>> Then we can put a line in the sand and say that future feature additions
>> in this area require serious discussion.
>>
>> Ok Tom?
>
>Right, ND is okay on the basis that we already have ARP (although I
>still may grumble from time to time that ARP, ND, and ICMP are being
>identified as flows ;-) ).
>
>I think there are two projects in the are that someone, maybe an
>aspiring kernel network developer, might want to look into if they
>have the time:
>
>- Inevitably someone will want to support VXLAN or other UDP
>encapsulations in flow dissector. The only correct way to do this is
>going to be to do a lookup on UDP socket and have a flow_dissector
>function related to the socket. This is the model for dealing with UDP
>encapsulations in GRO that could be extended for flow dissection. We
>cannot hard code port numbers in flow_dissector. The interesting part
>here will be making a robust interface to avoid the pitfalls we've
>seen in some of the protocols in flow_dissector.
>
>- Allow calling a BPF function to do custom flow dissection. IIRC
>there someone (Daniel?) had already implement flow_dissector in BPF
>with pretty good results.

How will this help us for cls_flower case? Are you suggesting to put this
whole BPF occult to the next level and use it kernel-to-kernel? :D

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Tom Herbert]

On Wed, Feb 8, 2017 at 12:12 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Feb 08, 2017 at 08:10:06PM CET, tom@herbertland.com wrote:
>>On Wed, Feb 8, 2017 at 10:54 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Tom Herbert <tom@herbertland.com>
>>> Date: Wed, 8 Feb 2017 10:33:46 -0800
>>>
>>>> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>>>> I think the above paragraph gets back to Tom's original question regarding
>>>>> making things more complex just for OvS (use-cases). Possibly ND is an edge
>>>>> case even for OvS and on reflection my timing for posting it seems to have
>>>>> been less than ideal.
>>>>
>>>> If it wasn't ND it would be something else... with all the activity
>>>> happening in networking features and HW this is a timely discussion.
>>>> Flow dissector presents a good example of a function that might become
>>>> a dumping ground for an endless stream of features if we don't figure
>>>> out how exercise some restraint.
>>>
>>> I agree on most points.
>>>
>>> But, I would say that in this specific case, since we have ARP support in
>>> there already it behooves us to support the ipv6 side in the form of ND
>>> too.
>>>
>>> Then we can put a line in the sand and say that future feature additions
>>> in this area require serious discussion.
>>>
>>> Ok Tom?
>>
>>Right, ND is okay on the basis that we already have ARP (although I
>>still may grumble from time to time that ARP, ND, and ICMP are being
>>identified as flows ;-) ).
>>
>>I think there are two projects in the are that someone, maybe an
>>aspiring kernel network developer, might want to look into if they
>>have the time:
>>
>>- Inevitably someone will want to support VXLAN or other UDP
>>encapsulations in flow dissector. The only correct way to do this is
>>going to be to do a lookup on UDP socket and have a flow_dissector
>>function related to the socket. This is the model for dealing with UDP
>>encapsulations in GRO that could be extended for flow dissection. We
>>cannot hard code port numbers in flow_dissector. The interesting part
>>here will be making a robust interface to avoid the pitfalls we've
>>seen in some of the protocols in flow_dissector.
>>
>>- Allow calling a BPF function to do custom flow dissection. IIRC
>>there someone (Daniel?) had already implement flow_dissector in BPF
>>with pretty good results.
>
> How will this help us for cls_flower case? Are you suggesting to put this
> whole BPF occult to the next level and use it kernel-to-kernel? :D
>
I am merely suggesting that BPF offers a user programmable interface
that could allow implementing protocol support in the kernel for
functions like flow_dissector for protocols that we don't really want
explicit kernel code for. SOREUSEPORT-BPF is the canonical example of
such a capability.

Tom

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Wed, Feb 08, 2017 at 07:54:15PM CET, davem@davemloft.net wrote:
>From: Tom Herbert <tom@herbertland.com>
>Date: Wed, 8 Feb 2017 10:33:46 -0800
>
>> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>> I think the above paragraph gets back to Tom's original question regarding
>>> making things more complex just for OvS (use-cases). Possibly ND is an edge
>>> case even for OvS and on reflection my timing for posting it seems to have
>>> been less than ideal.
>> 
>> If it wasn't ND it would be something else... with all the activity
>> happening in networking features and HW this is a timely discussion.
>> Flow dissector presents a good example of a function that might become
>> a dumping ground for an endless stream of features if we don't figure
>> out how exercise some restraint.
>
>I agree on most points.
>
>But, I would say that in this specific case, since we have ARP support in
>there already it behooves us to support the ipv6 side in the form of ND
>too.
>
>Then we can put a line in the sand and say that future feature additions
>in this area require serious discussion.

Yeah, well, and if there is a functinality that is unacceptable for any
reason to put into flow_dissector, we have to do a flow_dissector2?

Note that I originally had separate dissection in cls_flower, you
suggested to use the existing flow_dissector. And I still believe it was
the right way to do it.

I think that better is to make existing flow dissector more modular.
I'll look into this.

Re: [oss-drivers] Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Wed, Feb 08, 2017 at 09:09:17PM +0100, Jiri Pirko wrote:
> Wed, Feb 08, 2017 at 07:54:15PM CET, davem@davemloft.net wrote:
> >From: Tom Herbert <tom@herbertland.com>
> >Date: Wed, 8 Feb 2017 10:33:46 -0800
> >
> >> On Wed, Feb 8, 2017 at 1:28 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >>> I think the above paragraph gets back to Tom's original question regarding
> >>> making things more complex just for OvS (use-cases). Possibly ND is an edge
> >>> case even for OvS and on reflection my timing for posting it seems to have
> >>> been less than ideal.
> >> 
> >> If it wasn't ND it would be something else... with all the activity
> >> happening in networking features and HW this is a timely discussion.
> >> Flow dissector presents a good example of a function that might become
> >> a dumping ground for an endless stream of features if we don't figure
> >> out how exercise some restraint.
> >
> >I agree on most points.
> >
> >But, I would say that in this specific case, since we have ARP support in
> >there already it behooves us to support the ipv6 side in the form of ND
> >too.
> >
> >Then we can put a line in the sand and say that future feature additions
> >in this area require serious discussion.

I think this serious discussion is all about the long term and am
completely in favour of that. But in the short term it would help me to
know if the line in the sand excludes proposing enhancements to protocols
already supported by the flow dissector; in particular MPLS and IP fields I
listed earlier in this thread.

Perhaps the answer is that it depends. But if the line in
the sand has been fortified I'd rather avoid trying to cross it.

> Yeah, well, and if there is a functinality that is unacceptable for any
> reason to put into flow_dissector, we have to do a flow_dissector2?
> 
> Note that I originally had separate dissection in cls_flower, you
> suggested to use the existing flow_dissector. And I still believe it was
> the right way to do it.
> 
> I think that better is to make existing flow dissector more modular.
> I'll look into this.

Making the flow dissector more modular makes some sense to me.
It seems that it should be a way to address sharing common code while
allowing more flexibility for users, such as flower, that need it.

Elsewhere in this thread there has been some discussion of BPF. While I
also think that makes sense I think that for in-kernel users it makes
more sense to allow leveraging of the flow dissector using C code.

Such an approach well allow some of the complexity that was recently added
to the flow dissector to move into more peripheral code. Which may or may
not be a win as I think was discussed earlier in this thread.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Thu, Feb 02, 2017 at 07:36:31PM CET, tom@herbertland.com wrote:
>On Thu, Feb 2, 2017 at 9:48 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Feb 02, 2017 at 06:24:40PM CET, tom@herbertland.com wrote:
>>>On Thu, Feb 2, 2017 at 7:58 AM, Simon Horman <simon.horman@netronome.com> wrote:
>>>> [Repost due to gmail account problem]
>>>>
>>>> On Thu, Feb 02, 2017 at 04:31:33AM -0800, Eric Dumazet wrote:
>>>>> On Thu, 2017-02-02 at 11:37 +0100, Simon Horman wrote:
>>>>> > Allow dissection of Neighbour Discovery target IP, and source and
>>>>> > destination link-layer addresses for neighbour solicitation and
>>>>> > advertisement messages.
>>>>> >
>>>>> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
>>>>> > ---
>>>>>
>>>>> Hi Simon
>>>>>
>>>>> Why is this needed ?
>>>>>
>>>>> Any code added in flow dissector needs to be extra careful,
>>>>> we had various packet of deaths errors recently in this area.
>>>>
>>>> Hi Eric,
>>>>
>>>> there some activity to allow programming of OvS flows in hardware via TC
>>>> with the flower classifier. As the ND fields in this patch are part of the
>>>> OvS flow key I would like them considered for additions to flower and thus
>>>> the dissector to allow compatibility with OvS.
>>>>
>>>Given that ARP is already there it seems only "fair" to have ND also.
>>>But Eric is correct, this is quite a sensitive area of code.
>>>
>>>> I apologise if any 'deaths' have resulted from my recent work on the
>>>> dissector. I am of course very open to ideas on how to avoid any future
>>>> incidents.
>>>
>>>That's a tough problem. flow_dissector started off as simple mechanism
>>>to just identify actual flows (really just TCP and UDP packets) for
>>>the purposes of packet steering. But given the benefits of its
>>>location low in the stack and the open ended capabilities for parsing
>>>it seems to have mushroomed into a general catchall to parse a whole
>>>bunch of different protocols. A lot of these go beyond simply
>>>identifying flows (ICMP parsing, ARP, or ND as in your patches). These
>>>new use cases may be valid, but the result is a convoluted function (>
>>>500 LOC by my count) and it seems to be quite easy to have subtle bugs
>>>mostly in edge cases, several of which could have been exploited in
>>>DDOS attacks.
>>
>> Agreed that we probably came to a point when we need to split
>> __skb_flow_dissect into modular and pluggable pieces. Will not be
>> trivial though.
>>
>> Also note that it depends on the __skb_flow_dissect user which code is
>> actually used or not. For the critical path, that keys are defined by:
>> flow_keys_dissector_keys
>>
>True, but the code doesn't separate out the critical path from all
>these extended features which is resulted in a jumbled mess with no
>modularity to speak of :-(

I'm looking at this right now. It is really not possible to split this
in some nice and efficient way. So what I did (patchset in reply to this
email) is I pushed the bits that are not needed for the default hash
dissection out to sub-functions. Also, the code is executed only when
the flow dissector user needs it - that is not the case of the default
hash dissection, so that should satisfy your concerns.

Every future dissection feature addition will be done like this. So
Simon can do it like that for ND and should be safe.


>
>> Most of the code Simon is adding is noop for non-flower usecase if:
>> dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) == false
>>
>Sure, but that just makes this code corner cases which means it's hard
>to maintain and harder to find bugs in the long run.

Sure, but you will never see the bugs in the default hash dissection.
And that is what you need. For cls_flower user, we'll experience it and
fix it. I see no problem.

[patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Make the main flow_dissect function a bit smaller and move the ARP
dissection into a separate function. Along with that, do the ARP header
processing only in case the flow dissection user requires it.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/flow_dissector.c | 111 ++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 52 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index c35aae1..10dc5bb 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -113,6 +113,61 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static bool __skb_flow_dissect_arp(const struct sk_buff *skb,
+				   struct flow_dissector *flow_dissector,
+				   void *target_container, void *data,
+				   int nhoff, int hlen)
+{
+	struct flow_dissector_key_arp *key_arp;
+	struct {
+		unsigned char ar_sha[ETH_ALEN];
+		unsigned char ar_sip[4];
+		unsigned char ar_tha[ETH_ALEN];
+		unsigned char ar_tip[4];
+	} *arp_eth, _arp_eth;
+	const struct arphdr *arp;
+	struct arphdr *_arp;
+
+	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ARP))
+		return true;
+
+	arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
+				   hlen, &_arp);
+	if (!arp)
+		return false;
+
+	if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
+	    arp->ar_pro != htons(ETH_P_IP) ||
+	    arp->ar_hln != ETH_ALEN ||
+	    arp->ar_pln != 4 ||
+	    (arp->ar_op != htons(ARPOP_REPLY) &&
+	     arp->ar_op != htons(ARPOP_REQUEST)))
+		return false;
+
+	arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
+				       sizeof(_arp_eth), data,
+				       hlen, &_arp_eth);
+	if (!arp_eth)
+		return false;
+
+	key_arp = skb_flow_dissector_target(flow_dissector,
+					    FLOW_DISSECTOR_KEY_ARP,
+					    target_container);
+
+	memcpy(&key_arp->sip, arp_eth->ar_sip, sizeof(key_arp->sip));
+	memcpy(&key_arp->tip, arp_eth->ar_tip, sizeof(key_arp->tip));
+
+	/* Only store the lower byte of the opcode;
+	 * this covers ARPOP_REPLY and ARPOP_REQUEST.
+	 */
+	key_arp->op = ntohs(arp->ar_op) & 0xff;
+
+	ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
+	ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
+
+	return true;
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
@@ -138,7 +193,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	struct flow_dissector_key_control *key_control;
 	struct flow_dissector_key_basic *key_basic;
 	struct flow_dissector_key_addrs *key_addrs;
-	struct flow_dissector_key_arp *key_arp;
 	struct flow_dissector_key_ports *key_ports;
 	struct flow_dissector_key_icmp *key_icmp;
 	struct flow_dissector_key_tags *key_tags;
@@ -382,59 +436,12 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		goto out_good;
 
 	case htons(ETH_P_ARP):
-	case htons(ETH_P_RARP): {
-		struct {
-			unsigned char ar_sha[ETH_ALEN];
-			unsigned char ar_sip[4];
-			unsigned char ar_tha[ETH_ALEN];
-			unsigned char ar_tip[4];
-		} *arp_eth, _arp_eth;
-		const struct arphdr *arp;
-		struct arphdr *_arp;
-
-		arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
-					   hlen, &_arp);
-		if (!arp)
-			goto out_bad;
-
-		if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
-		    arp->ar_pro != htons(ETH_P_IP) ||
-		    arp->ar_hln != ETH_ALEN ||
-		    arp->ar_pln != 4 ||
-		    (arp->ar_op != htons(ARPOP_REPLY) &&
-		     arp->ar_op != htons(ARPOP_REQUEST)))
+	case htons(ETH_P_RARP):
+		if (!__skb_flow_dissect_arp(skb, flow_dissector,
+					    target_container, data,
+					    nhoff, hlen))
 			goto out_bad;
-
-		arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
-					       sizeof(_arp_eth), data,
-					       hlen,
-					       &_arp_eth);
-		if (!arp_eth)
-			goto out_bad;
-
-		if (dissector_uses_key(flow_dissector,
-				       FLOW_DISSECTOR_KEY_ARP)) {
-
-			key_arp = skb_flow_dissector_target(flow_dissector,
-							    FLOW_DISSECTOR_KEY_ARP,
-							    target_container);
-
-			memcpy(&key_arp->sip, arp_eth->ar_sip,
-			       sizeof(key_arp->sip));
-			memcpy(&key_arp->tip, arp_eth->ar_tip,
-			       sizeof(key_arp->tip));
-
-			/* Only store the lower byte of the opcode;
-			 * this covers ARPOP_REPLY and ARPOP_REQUEST.
-			 */
-			key_arp->op = ntohs(arp->ar_op) & 0xff;
-
-			ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
-			ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
-		}
-
 		goto out_good;
-	}
 
 	default:
 		goto out_bad;
-- 
2.7.4

[patch net-next RFC 2/2] flow_dissecror: Move MPLS dissection into a separate function

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Make the main flow_dissect function a bit smaller and move the MPLS
dissection into a separate function. Along with that, do the MPLS header
processing only in case the flow dissection user requires it.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/core/flow_dissector.c | 52 ++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 10dc5bb..3ab935e 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -113,6 +113,33 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
+static bool __skb_flow_dissect_mpls(const struct sk_buff *skb,
+				    struct flow_dissector *flow_dissector,
+				    void *target_container, void *data,
+				    int nhoff, int hlen)
+{
+	struct flow_dissector_key_keyid *key_keyid;
+	struct mpls_label *hdr, _hdr[2];
+
+	if (!dissector_uses_key(flow_dissector,
+				FLOW_DISSECTOR_KEY_MPLS_ENTROPY))
+		return true;
+
+	hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
+				   hlen, &_hdr);
+	if (!hdr)
+		return false;
+
+	if ((ntohl(hdr[0].entry) & MPLS_LS_LABEL_MASK) >>
+	    MPLS_LS_LABEL_SHIFT == MPLS_LABEL_ENTROPY) {
+		key_keyid = skb_flow_dissector_target(flow_dissector,
+						      FLOW_DISSECTOR_KEY_MPLS_ENTROPY,
+						      target_container);
+		key_keyid->keyid = hdr[1].entry & htonl(MPLS_LS_LABEL_MASK);
+	}
+	return true;
+}
+
 static bool __skb_flow_dissect_arp(const struct sk_buff *skb,
 				   struct flow_dissector *flow_dissector,
 				   void *target_container, void *data,
@@ -403,30 +430,13 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	}
 
 	case htons(ETH_P_MPLS_UC):
-	case htons(ETH_P_MPLS_MC): {
-		struct mpls_label *hdr, _hdr[2];
+	case htons(ETH_P_MPLS_MC):
 mpls:
-		hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
-					   hlen, &_hdr);
-		if (!hdr)
+		if (!__skb_flow_dissect_mpls(skb, flow_dissector,
+					     target_container, data,
+					     nhoff, hlen))
 			goto out_bad;
-
-		if ((ntohl(hdr[0].entry) & MPLS_LS_LABEL_MASK) >>
-		     MPLS_LS_LABEL_SHIFT == MPLS_LABEL_ENTROPY) {
-			if (dissector_uses_key(flow_dissector,
-					       FLOW_DISSECTOR_KEY_MPLS_ENTROPY)) {
-				key_keyid = skb_flow_dissector_target(flow_dissector,
-								      FLOW_DISSECTOR_KEY_MPLS_ENTROPY,
-								      target_container);
-				key_keyid->keyid = hdr[1].entry &
-					htonl(MPLS_LS_LABEL_MASK);
-			}
-
-			goto out_good;
-		}
-
 		goto out_good;
-	}
 
 	case htons(ETH_P_FCOE):
 		if ((hlen - nhoff) < FCOE_HEADER_LEN)
-- 
2.7.4

Re: [patch net-next RFC 2/2] flow_dissecror: Move MPLS dissection into a separate function

[Simon Horman]

On Tue, Feb 21, 2017 at 03:33:19PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Make the main flow_dissect function a bit smaller and move the MPLS
> dissection into a separate function. Along with that, do the MPLS header
> processing only in case the flow dissection user requires it.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 21 Feb 2017 15:33:18 +0100

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Make the main flow_dissect function a bit smaller and move the ARP
> dissection into a separate function. Along with that, do the ARP header
> processing only in case the flow dissection user requires it.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

These two changes look fine to me, although I hope that the compiler
doesn't decide to inline it anyways.

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Simon Horman]

On Tue, Feb 21, 2017 at 01:32:35PM -0500, David Miller wrote:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Tue, 21 Feb 2017 15:33:18 +0100
> 
> > From: Jiri Pirko <jiri@mellanox.com>
> > 
> > Make the main flow_dissect function a bit smaller and move the ARP
> > dissection into a separate function. Along with that, do the ARP header
> > processing only in case the flow dissection user requires it.
> > 
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> 
> These two changes look fine to me, although I hope that the compiler
> doesn't decide to inline it anyways.

I think the approach taken would be a win regardless of inlining as
more logic is protected by dissector_uses_key() and thus less likely
to cause collateral damage.

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Tom Herbert]

On Tue, Feb 21, 2017 at 6:33 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Make the main flow_dissect function a bit smaller and move the ARP
> dissection into a separate function. Along with that, do the ARP header
> processing only in case the flow dissection user requires it.
>

Acked-by: Tom Herbert <tom@herbertland.com>

GRE might also be a good candidate to get its own function.


> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  net/core/flow_dissector.c | 111 ++++++++++++++++++++++++----------------------
>  1 file changed, 59 insertions(+), 52 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index c35aae1..10dc5bb 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -113,6 +113,61 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>  }
>  EXPORT_SYMBOL(__skb_flow_get_ports);
>
> +static bool __skb_flow_dissect_arp(const struct sk_buff *skb,
> +                                  struct flow_dissector *flow_dissector,
> +                                  void *target_container, void *data,
> +                                  int nhoff, int hlen)
> +{
> +       struct flow_dissector_key_arp *key_arp;
> +       struct {
> +               unsigned char ar_sha[ETH_ALEN];
> +               unsigned char ar_sip[4];
> +               unsigned char ar_tha[ETH_ALEN];
> +               unsigned char ar_tip[4];
> +       } *arp_eth, _arp_eth;
> +       const struct arphdr *arp;
> +       struct arphdr *_arp;
> +
> +       if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ARP))
> +               return true;
> +
> +       arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
> +                                  hlen, &_arp);
> +       if (!arp)
> +               return false;
> +
> +       if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
> +           arp->ar_pro != htons(ETH_P_IP) ||
> +           arp->ar_hln != ETH_ALEN ||
> +           arp->ar_pln != 4 ||
> +           (arp->ar_op != htons(ARPOP_REPLY) &&
> +            arp->ar_op != htons(ARPOP_REQUEST)))
> +               return false;
> +
> +       arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
> +                                      sizeof(_arp_eth), data,
> +                                      hlen, &_arp_eth);
> +       if (!arp_eth)
> +               return false;
> +
> +       key_arp = skb_flow_dissector_target(flow_dissector,
> +                                           FLOW_DISSECTOR_KEY_ARP,
> +                                           target_container);
> +
> +       memcpy(&key_arp->sip, arp_eth->ar_sip, sizeof(key_arp->sip));
> +       memcpy(&key_arp->tip, arp_eth->ar_tip, sizeof(key_arp->tip));
> +
> +       /* Only store the lower byte of the opcode;
> +        * this covers ARPOP_REPLY and ARPOP_REQUEST.
> +        */
> +       key_arp->op = ntohs(arp->ar_op) & 0xff;
> +
> +       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
> +       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
> +
> +       return true;
> +}
> +
>  /**
>   * __skb_flow_dissect - extract the flow_keys struct and return it
>   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
> @@ -138,7 +193,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>         struct flow_dissector_key_control *key_control;
>         struct flow_dissector_key_basic *key_basic;
>         struct flow_dissector_key_addrs *key_addrs;
> -       struct flow_dissector_key_arp *key_arp;
>         struct flow_dissector_key_ports *key_ports;
>         struct flow_dissector_key_icmp *key_icmp;
>         struct flow_dissector_key_tags *key_tags;
> @@ -382,59 +436,12 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>                 goto out_good;
>
>         case htons(ETH_P_ARP):
> -       case htons(ETH_P_RARP): {
> -               struct {
> -                       unsigned char ar_sha[ETH_ALEN];
> -                       unsigned char ar_sip[4];
> -                       unsigned char ar_tha[ETH_ALEN];
> -                       unsigned char ar_tip[4];
> -               } *arp_eth, _arp_eth;
> -               const struct arphdr *arp;
> -               struct arphdr *_arp;
> -
> -               arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
> -                                          hlen, &_arp);
> -               if (!arp)
> -                       goto out_bad;
> -
> -               if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
> -                   arp->ar_pro != htons(ETH_P_IP) ||
> -                   arp->ar_hln != ETH_ALEN ||
> -                   arp->ar_pln != 4 ||
> -                   (arp->ar_op != htons(ARPOP_REPLY) &&
> -                    arp->ar_op != htons(ARPOP_REQUEST)))
> +       case htons(ETH_P_RARP):
> +               if (!__skb_flow_dissect_arp(skb, flow_dissector,
> +                                           target_container, data,
> +                                           nhoff, hlen))
>                         goto out_bad;
> -
> -               arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
> -                                              sizeof(_arp_eth), data,
> -                                              hlen,
> -                                              &_arp_eth);
> -               if (!arp_eth)
> -                       goto out_bad;
> -
> -               if (dissector_uses_key(flow_dissector,
> -                                      FLOW_DISSECTOR_KEY_ARP)) {
> -
> -                       key_arp = skb_flow_dissector_target(flow_dissector,
> -                                                           FLOW_DISSECTOR_KEY_ARP,
> -                                                           target_container);
> -
> -                       memcpy(&key_arp->sip, arp_eth->ar_sip,
> -                              sizeof(key_arp->sip));
> -                       memcpy(&key_arp->tip, arp_eth->ar_tip,
> -                              sizeof(key_arp->tip));
> -
> -                       /* Only store the lower byte of the opcode;
> -                        * this covers ARPOP_REPLY and ARPOP_REQUEST.
> -                        */
> -                       key_arp->op = ntohs(arp->ar_op) & 0xff;
> -
> -                       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
> -                       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
> -               }
> -
>                 goto out_good;
> -       }
>
>         default:
>                 goto out_bad;
> --
> 2.7.4
>

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Jiri Pirko]

Tue, Feb 21, 2017 at 07:50:53PM CET, tom@herbertland.com wrote:
>On Tue, Feb 21, 2017 at 6:33 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Make the main flow_dissect function a bit smaller and move the ARP
>> dissection into a separate function. Along with that, do the ARP header
>> processing only in case the flow dissection user requires it.
>>
>
>Acked-by: Tom Herbert <tom@herbertland.com>
>
>GRE might also be a good candidate to get its own function.

Okay. Will look at that. Thanks.



>
>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  net/core/flow_dissector.c | 111 ++++++++++++++++++++++++----------------------
>>  1 file changed, 59 insertions(+), 52 deletions(-)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index c35aae1..10dc5bb 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -113,6 +113,61 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>>  }
>>  EXPORT_SYMBOL(__skb_flow_get_ports);
>>
>> +static bool __skb_flow_dissect_arp(const struct sk_buff *skb,
>> +                                  struct flow_dissector *flow_dissector,
>> +                                  void *target_container, void *data,
>> +                                  int nhoff, int hlen)
>> +{
>> +       struct flow_dissector_key_arp *key_arp;
>> +       struct {
>> +               unsigned char ar_sha[ETH_ALEN];
>> +               unsigned char ar_sip[4];
>> +               unsigned char ar_tha[ETH_ALEN];
>> +               unsigned char ar_tip[4];
>> +       } *arp_eth, _arp_eth;
>> +       const struct arphdr *arp;
>> +       struct arphdr *_arp;
>> +
>> +       if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ARP))
>> +               return true;
>> +
>> +       arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
>> +                                  hlen, &_arp);
>> +       if (!arp)
>> +               return false;
>> +
>> +       if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
>> +           arp->ar_pro != htons(ETH_P_IP) ||
>> +           arp->ar_hln != ETH_ALEN ||
>> +           arp->ar_pln != 4 ||
>> +           (arp->ar_op != htons(ARPOP_REPLY) &&
>> +            arp->ar_op != htons(ARPOP_REQUEST)))
>> +               return false;
>> +
>> +       arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
>> +                                      sizeof(_arp_eth), data,
>> +                                      hlen, &_arp_eth);
>> +       if (!arp_eth)
>> +               return false;
>> +
>> +       key_arp = skb_flow_dissector_target(flow_dissector,
>> +                                           FLOW_DISSECTOR_KEY_ARP,
>> +                                           target_container);
>> +
>> +       memcpy(&key_arp->sip, arp_eth->ar_sip, sizeof(key_arp->sip));
>> +       memcpy(&key_arp->tip, arp_eth->ar_tip, sizeof(key_arp->tip));
>> +
>> +       /* Only store the lower byte of the opcode;
>> +        * this covers ARPOP_REPLY and ARPOP_REQUEST.
>> +        */
>> +       key_arp->op = ntohs(arp->ar_op) & 0xff;
>> +
>> +       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
>> +       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
>> +
>> +       return true;
>> +}
>> +
>>  /**
>>   * __skb_flow_dissect - extract the flow_keys struct and return it
>>   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
>> @@ -138,7 +193,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>         struct flow_dissector_key_control *key_control;
>>         struct flow_dissector_key_basic *key_basic;
>>         struct flow_dissector_key_addrs *key_addrs;
>> -       struct flow_dissector_key_arp *key_arp;
>>         struct flow_dissector_key_ports *key_ports;
>>         struct flow_dissector_key_icmp *key_icmp;
>>         struct flow_dissector_key_tags *key_tags;
>> @@ -382,59 +436,12 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>                 goto out_good;
>>
>>         case htons(ETH_P_ARP):
>> -       case htons(ETH_P_RARP): {
>> -               struct {
>> -                       unsigned char ar_sha[ETH_ALEN];
>> -                       unsigned char ar_sip[4];
>> -                       unsigned char ar_tha[ETH_ALEN];
>> -                       unsigned char ar_tip[4];
>> -               } *arp_eth, _arp_eth;
>> -               const struct arphdr *arp;
>> -               struct arphdr *_arp;
>> -
>> -               arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
>> -                                          hlen, &_arp);
>> -               if (!arp)
>> -                       goto out_bad;
>> -
>> -               if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
>> -                   arp->ar_pro != htons(ETH_P_IP) ||
>> -                   arp->ar_hln != ETH_ALEN ||
>> -                   arp->ar_pln != 4 ||
>> -                   (arp->ar_op != htons(ARPOP_REPLY) &&
>> -                    arp->ar_op != htons(ARPOP_REQUEST)))
>> +       case htons(ETH_P_RARP):
>> +               if (!__skb_flow_dissect_arp(skb, flow_dissector,
>> +                                           target_container, data,
>> +                                           nhoff, hlen))
>>                         goto out_bad;
>> -
>> -               arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
>> -                                              sizeof(_arp_eth), data,
>> -                                              hlen,
>> -                                              &_arp_eth);
>> -               if (!arp_eth)
>> -                       goto out_bad;
>> -
>> -               if (dissector_uses_key(flow_dissector,
>> -                                      FLOW_DISSECTOR_KEY_ARP)) {
>> -
>> -                       key_arp = skb_flow_dissector_target(flow_dissector,
>> -                                                           FLOW_DISSECTOR_KEY_ARP,
>> -                                                           target_container);
>> -
>> -                       memcpy(&key_arp->sip, arp_eth->ar_sip,
>> -                              sizeof(key_arp->sip));
>> -                       memcpy(&key_arp->tip, arp_eth->ar_tip,
>> -                              sizeof(key_arp->tip));
>> -
>> -                       /* Only store the lower byte of the opcode;
>> -                        * this covers ARPOP_REPLY and ARPOP_REQUEST.
>> -                        */
>> -                       key_arp->op = ntohs(arp->ar_op) & 0xff;
>> -
>> -                       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
>> -                       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
>> -               }
>> -
>>                 goto out_good;
>> -       }
>>
>>         default:
>>                 goto out_bad;
>> --
>> 2.7.4
>>

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Jiri Pirko]

Tue, Feb 21, 2017 at 07:50:53PM CET, tom@herbertland.com wrote:
>On Tue, Feb 21, 2017 at 6:33 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Make the main flow_dissect function a bit smaller and move the ARP
>> dissection into a separate function. Along with that, do the ARP header
>> processing only in case the flow dissection user requires it.
>>
>
>Acked-by: Tom Herbert <tom@herbertland.com>
>
>GRE might also be a good candidate to get its own function.
>

Submitted with GRE bits. Note that I left you ack and Simon's revby out
since I did some cosmetic changes until rfc.

I would be glad if you both can check it again.

Thanks!


>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  net/core/flow_dissector.c | 111 ++++++++++++++++++++++++----------------------
>>  1 file changed, 59 insertions(+), 52 deletions(-)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index c35aae1..10dc5bb 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -113,6 +113,61 @@ __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>>  }
>>  EXPORT_SYMBOL(__skb_flow_get_ports);
>>
>> +static bool __skb_flow_dissect_arp(const struct sk_buff *skb,
>> +                                  struct flow_dissector *flow_dissector,
>> +                                  void *target_container, void *data,
>> +                                  int nhoff, int hlen)
>> +{
>> +       struct flow_dissector_key_arp *key_arp;
>> +       struct {
>> +               unsigned char ar_sha[ETH_ALEN];
>> +               unsigned char ar_sip[4];
>> +               unsigned char ar_tha[ETH_ALEN];
>> +               unsigned char ar_tip[4];
>> +       } *arp_eth, _arp_eth;
>> +       const struct arphdr *arp;
>> +       struct arphdr *_arp;
>> +
>> +       if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ARP))
>> +               return true;
>> +
>> +       arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
>> +                                  hlen, &_arp);
>> +       if (!arp)
>> +               return false;
>> +
>> +       if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
>> +           arp->ar_pro != htons(ETH_P_IP) ||
>> +           arp->ar_hln != ETH_ALEN ||
>> +           arp->ar_pln != 4 ||
>> +           (arp->ar_op != htons(ARPOP_REPLY) &&
>> +            arp->ar_op != htons(ARPOP_REQUEST)))
>> +               return false;
>> +
>> +       arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
>> +                                      sizeof(_arp_eth), data,
>> +                                      hlen, &_arp_eth);
>> +       if (!arp_eth)
>> +               return false;
>> +
>> +       key_arp = skb_flow_dissector_target(flow_dissector,
>> +                                           FLOW_DISSECTOR_KEY_ARP,
>> +                                           target_container);
>> +
>> +       memcpy(&key_arp->sip, arp_eth->ar_sip, sizeof(key_arp->sip));
>> +       memcpy(&key_arp->tip, arp_eth->ar_tip, sizeof(key_arp->tip));
>> +
>> +       /* Only store the lower byte of the opcode;
>> +        * this covers ARPOP_REPLY and ARPOP_REQUEST.
>> +        */
>> +       key_arp->op = ntohs(arp->ar_op) & 0xff;
>> +
>> +       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
>> +       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
>> +
>> +       return true;
>> +}
>> +
>>  /**
>>   * __skb_flow_dissect - extract the flow_keys struct and return it
>>   * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
>> @@ -138,7 +193,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>         struct flow_dissector_key_control *key_control;
>>         struct flow_dissector_key_basic *key_basic;
>>         struct flow_dissector_key_addrs *key_addrs;
>> -       struct flow_dissector_key_arp *key_arp;
>>         struct flow_dissector_key_ports *key_ports;
>>         struct flow_dissector_key_icmp *key_icmp;
>>         struct flow_dissector_key_tags *key_tags;
>> @@ -382,59 +436,12 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>                 goto out_good;
>>
>>         case htons(ETH_P_ARP):
>> -       case htons(ETH_P_RARP): {
>> -               struct {
>> -                       unsigned char ar_sha[ETH_ALEN];
>> -                       unsigned char ar_sip[4];
>> -                       unsigned char ar_tha[ETH_ALEN];
>> -                       unsigned char ar_tip[4];
>> -               } *arp_eth, _arp_eth;
>> -               const struct arphdr *arp;
>> -               struct arphdr *_arp;
>> -
>> -               arp = __skb_header_pointer(skb, nhoff, sizeof(_arp), data,
>> -                                          hlen, &_arp);
>> -               if (!arp)
>> -                       goto out_bad;
>> -
>> -               if (arp->ar_hrd != htons(ARPHRD_ETHER) ||
>> -                   arp->ar_pro != htons(ETH_P_IP) ||
>> -                   arp->ar_hln != ETH_ALEN ||
>> -                   arp->ar_pln != 4 ||
>> -                   (arp->ar_op != htons(ARPOP_REPLY) &&
>> -                    arp->ar_op != htons(ARPOP_REQUEST)))
>> +       case htons(ETH_P_RARP):
>> +               if (!__skb_flow_dissect_arp(skb, flow_dissector,
>> +                                           target_container, data,
>> +                                           nhoff, hlen))
>>                         goto out_bad;
>> -
>> -               arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp),
>> -                                              sizeof(_arp_eth), data,
>> -                                              hlen,
>> -                                              &_arp_eth);
>> -               if (!arp_eth)
>> -                       goto out_bad;
>> -
>> -               if (dissector_uses_key(flow_dissector,
>> -                                      FLOW_DISSECTOR_KEY_ARP)) {
>> -
>> -                       key_arp = skb_flow_dissector_target(flow_dissector,
>> -                                                           FLOW_DISSECTOR_KEY_ARP,
>> -                                                           target_container);
>> -
>> -                       memcpy(&key_arp->sip, arp_eth->ar_sip,
>> -                              sizeof(key_arp->sip));
>> -                       memcpy(&key_arp->tip, arp_eth->ar_tip,
>> -                              sizeof(key_arp->tip));
>> -
>> -                       /* Only store the lower byte of the opcode;
>> -                        * this covers ARPOP_REPLY and ARPOP_REQUEST.
>> -                        */
>> -                       key_arp->op = ntohs(arp->ar_op) & 0xff;
>> -
>> -                       ether_addr_copy(key_arp->sha, arp_eth->ar_sha);
>> -                       ether_addr_copy(key_arp->tha, arp_eth->ar_tha);
>> -               }
>> -
>>                 goto out_good;
>> -       }
>>
>>         default:
>>                 goto out_bad;
>> --
>> 2.7.4
>>

Re: [patch net-next RFC 1/2] flow_dissecror: Move ARP dissection into a separate function

[Simon Horman]

On Tue, Feb 21, 2017 at 03:33:18PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Make the main flow_dissect function a bit smaller and move the ARP
> dissection into a separate function. Along with that, do the ARP header
> processing only in case the flow dissection user requires it.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

This looks good to me.

Reviewed-by: Simon Horman <simon.horman@netronome.com>

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
>Allow dissection of Neighbour Discovery target IP, and source and
>destination link-layer addresses for neighbour solicitation and
>advertisement messages.
>
>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>---

[...]

>@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 						     FLOW_DISSECTOR_KEY_ICMP,
> 						     target_container);
> 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
>+
>+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
>+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&

IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
instead.


>+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
>+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
>+			key_nd = skb_flow_dissector_target(flow_dissector,
>+							   FLOW_DISSECTOR_KEY_ND,
>+							   target_container);
>+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
>+						  hlen, ipv6_payload_len)))
>+				goto out_bad;
>+		}

You should put this under "switch (ip_proto) {"

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
> >Allow dissection of Neighbour Discovery target IP, and source and
> >destination link-layer addresses for neighbour solicitation and
> >advertisement messages.
> >
> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >---
> 
> [...]
> 
> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> > 						     FLOW_DISSECTOR_KEY_ICMP,
> > 						     target_container);
> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
> >+
> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
> 
> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
> instead.

Thanks, will do.

> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
> >+			key_nd = skb_flow_dissector_target(flow_dissector,
> >+							   FLOW_DISSECTOR_KEY_ND,
> >+							   target_container);
> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
> >+						  hlen, ipv6_payload_len)))
> >+				goto out_bad;
> >+		}
> 
> You should put this under "switch (ip_proto) {"

I see that makes sense in terms of the check against ip_proto.
But I added it here to allow checking against key_icmp->code
and key_icmp->type as well.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Fri, Mar 10, 2017 at 03:19:13PM CET, simon.horman@netronome.com wrote:
>On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
>> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
>> >Allow dissection of Neighbour Discovery target IP, and source and
>> >destination link-layer addresses for neighbour solicitation and
>> >advertisement messages.
>> >
>> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >---
>> 
>> [...]
>> 
>> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> > 						     FLOW_DISSECTOR_KEY_ICMP,
>> > 						     target_container);
>> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
>> >+
>> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
>> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
>> 
>> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
>> instead.
>
>Thanks, will do.
>
>> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
>> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
>> >+			key_nd = skb_flow_dissector_target(flow_dissector,
>> >+							   FLOW_DISSECTOR_KEY_ND,
>> >+							   target_container);
>> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
>> >+						  hlen, ipv6_payload_len)))
>> >+				goto out_bad;
>> >+		}
>> 
>> You should put this under "switch (ip_proto) {"
>
>I see that makes sense in terms of the check against ip_proto.
>But I added it here to allow checking against key_icmp->code
>and key_icmp->type as well.

Sure. Just add under "switch (ip_proto) {" and call a wrapper nd
function from there. In that function, you check dissector_uses_key and
other needed things.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Fri, Mar 10, 2017 at 03:27:32PM +0100, Jiri Pirko wrote:
> Fri, Mar 10, 2017 at 03:19:13PM CET, simon.horman@netronome.com wrote:
> >On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
> >> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
> >> >Allow dissection of Neighbour Discovery target IP, and source and
> >> >destination link-layer addresses for neighbour solicitation and
> >> >advertisement messages.
> >> >
> >> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >> >---
> >> 
> >> [...]
> >> 
> >> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >> > 						     FLOW_DISSECTOR_KEY_ICMP,
> >> > 						     target_container);
> >> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
> >> >+
> >> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
> >> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
> >> 
> >> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
> >> instead.
> >
> >Thanks, will do.
> >
> >> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
> >> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
> >> >+			key_nd = skb_flow_dissector_target(flow_dissector,
> >> >+							   FLOW_DISSECTOR_KEY_ND,
> >> >+							   target_container);
> >> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
> >> >+						  hlen, ipv6_payload_len)))
> >> >+				goto out_bad;
> >> >+		}
> >> 
> >> You should put this under "switch (ip_proto) {"
> >
> >I see that makes sense in terms of the check against ip_proto.
> >But I added it here to allow checking against key_icmp->code
> >and key_icmp->type as well.
> 
> Sure. Just add under "switch (ip_proto) {" and call a wrapper nd
> function from there. In that function, you check dissector_uses_key and
> other needed things.

Sorry, but I'm still a little unclear on how that interacts with
the dissection of key_icmp.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Fri, Mar 10, 2017 at 04:20:21PM CET, simon.horman@netronome.com wrote:
>On Fri, Mar 10, 2017 at 03:27:32PM +0100, Jiri Pirko wrote:
>> Fri, Mar 10, 2017 at 03:19:13PM CET, simon.horman@netronome.com wrote:
>> >On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
>> >> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
>> >> >Allow dissection of Neighbour Discovery target IP, and source and
>> >> >destination link-layer addresses for neighbour solicitation and
>> >> >advertisement messages.
>> >> >
>> >> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >> >---
>> >> 
>> >> [...]
>> >> 
>> >> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >> > 						     FLOW_DISSECTOR_KEY_ICMP,
>> >> > 						     target_container);
>> >> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
>> >> >+
>> >> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
>> >> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
>> >> 
>> >> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
>> >> instead.
>> >
>> >Thanks, will do.
>> >
>> >> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
>> >> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
>> >> >+			key_nd = skb_flow_dissector_target(flow_dissector,
>> >> >+							   FLOW_DISSECTOR_KEY_ND,
>> >> >+							   target_container);
>> >> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
>> >> >+						  hlen, ipv6_payload_len)))
>> >> >+				goto out_bad;
>> >> >+		}
>> >> 
>> >> You should put this under "switch (ip_proto) {"
>> >
>> >I see that makes sense in terms of the check against ip_proto.
>> >But I added it here to allow checking against key_icmp->code
>> >and key_icmp->type as well.
>> 
>> Sure. Just add under "switch (ip_proto) {" and call a wrapper nd
>> function from there. In that function, you check dissector_uses_key and
>> other needed things.
>
>Sorry, but I'm still a little unclear on how that interacts with
>the dissection of key_icmp.

you do:
if (key_icmp->code != 0)
	return

Inside that function. Or something like that. Up to you. Just look at
__skb_flow_dissect_arp for example. First it checks dissector_uses_key,
then it does other checks.

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Simon Horman]

On Fri, Mar 10, 2017 at 03:27:32PM +0100, Jiri Pirko wrote:
> Fri, Mar 10, 2017 at 03:19:13PM CET, simon.horman@netronome.com wrote:
> >On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
> >> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
> >> >Allow dissection of Neighbour Discovery target IP, and source and
> >> >destination link-layer addresses for neighbour solicitation and
> >> >advertisement messages.
> >> >
> >> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >> >---
> >> 
> >> [...]
> >> 
> >> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >> > 						     FLOW_DISSECTOR_KEY_ICMP,
> >> > 						     target_container);
> >> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
> >> >+
> >> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
> >> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
> >> 
> >> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
> >> instead.
> >
> >Thanks, will do.
> >
> >> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
> >> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
> >> >+			key_nd = skb_flow_dissector_target(flow_dissector,
> >> >+							   FLOW_DISSECTOR_KEY_ND,
> >> >+							   target_container);
> >> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
> >> >+						  hlen, ipv6_payload_len)))
> >> >+				goto out_bad;
> >> >+		}
> >> 
> >> You should put this under "switch (ip_proto) {"
> >
> >I see that makes sense in terms of the check against ip_proto.
> >But I added it here to allow checking against key_icmp->code
> >and key_icmp->type as well.
> 
> Sure. Just add under "switch (ip_proto) {" and call a wrapper nd
> function from there. In that function, you check dissector_uses_key and
> other needed things.

Hi Jiri,

I'm sorry but I'm having a bit of trouble understanding how best to
structure the extraction of ICMP and ND.

The way I see things is this:

* ICMP extraction may occur for IPv4 or IPv6 although currently neither
  IPv4 nor IPv6 is a condition of ICMP extraction.
* ND extraction may only occur for IPv6
* ND extraction may only occur for certain ICMP code/type values;
  thus ICMP extraction should occur before ND extraction.

I wonder if a good alternative to the approach I took above in my patch
would  be to provide:
* ICMP extraction conditional on IPv4 and;
* ICMP extraction conditional on IPv6 followed by
  ND extraction conditional on ICMP type and code

Re: [PATCH/RFC net-next 1/2] flow dissector: ND support

[Jiri Pirko]

Mon, Mar 13, 2017 at 02:50:08PM CET, simon.horman@netronome.com wrote:
>On Fri, Mar 10, 2017 at 03:27:32PM +0100, Jiri Pirko wrote:
>> Fri, Mar 10, 2017 at 03:19:13PM CET, simon.horman@netronome.com wrote:
>> >On Tue, Feb 21, 2017 at 04:28:10PM +0100, Jiri Pirko wrote:
>> >> Thu, Feb 02, 2017 at 11:37:34AM CET, simon.horman@netronome.com wrote:
>> >> >Allow dissection of Neighbour Discovery target IP, and source and
>> >> >destination link-layer addresses for neighbour solicitation and
>> >> >advertisement messages.
>> >> >
>> >> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >> >---
>> >> 
>> >> [...]
>> >> 
>> >> >@@ -633,6 +702,18 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >> > 						     FLOW_DISSECTOR_KEY_ICMP,
>> >> > 						     target_container);
>> >> > 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
>> >> >+
>> >> >+		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
>> >> >+		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
>> >> 
>> >> IPPROTO_IPV6 say "IPv6-in-IPv4 tunnelling". Please use "NEXTHDR_IPV6"
>> >> instead.
>> >
>> >Thanks, will do.
>> >
>> >> >+		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
>> >> >+		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
>> >> >+			key_nd = skb_flow_dissector_target(flow_dissector,
>> >> >+							   FLOW_DISSECTOR_KEY_ND,
>> >> >+							   target_container);
>> >> >+			if (!(skb_flow_dissect_nd(skb, key_nd, data, nhoff,
>> >> >+						  hlen, ipv6_payload_len)))
>> >> >+				goto out_bad;
>> >> >+		}
>> >> 
>> >> You should put this under "switch (ip_proto) {"
>> >
>> >I see that makes sense in terms of the check against ip_proto.
>> >But I added it here to allow checking against key_icmp->code
>> >and key_icmp->type as well.
>> 
>> Sure. Just add under "switch (ip_proto) {" and call a wrapper nd
>> function from there. In that function, you check dissector_uses_key and
>> other needed things.
>
>Hi Jiri,
>
>I'm sorry but I'm having a bit of trouble understanding how best to
>structure the extraction of ICMP and ND.
>
>The way I see things is this:
>
>* ICMP extraction may occur for IPv4 or IPv6 although currently neither
>  IPv4 nor IPv6 is a condition of ICMP extraction.
>* ND extraction may only occur for IPv6
>* ND extraction may only occur for certain ICMP code/type values;
>  thus ICMP extraction should occur before ND extraction.
>
>I wonder if a good alternative to the approach I took above in my patch
>would  be to provide:
>* ICMP extraction conditional on IPv4 and;
>* ICMP extraction conditional on IPv6 followed by
>  ND extraction conditional on ICMP type and code

Makes sense to me.

[PATCH/RFC net-next 2/2] net/sched: cls_flower: Support matching on ND

[Simon Horman]

Allow matching on Neighbour Discovery target IP, and source and
destination link-layer addresses for neighbour solicitation and
advertisement messages.

Sample usage:

tc qdisc add dev eth0 ingress

tc filter add dev eth0 protocol ipv6 parent ffff: flower \
	indev eth0 ip_proto icmpv6 type 136 code 0 \
	nd_target 2001:470:7eb3:403:201:8eff:fe22:8fea \
	nd_tll 00:01:8e:22:8f:ea action drop

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/uapi/linux/pkt_cls.h |  7 +++++++
 net/core/flow_dissector.c    |  6 +++---
 net/sched/cls_flower.c       | 42 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 345551e71410..fc9e9bef726e 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -430,6 +430,13 @@ enum {
 	TCA_FLOWER_KEY_ARP_THA,		/* ETH_ALEN */
 	TCA_FLOWER_KEY_ARP_THA_MASK,	/* ETH_ALEN */
 
+	TCA_FLOWER_KEY_ND_TARGET,	/* struct in6_addr */
+	TCA_FLOWER_KEY_ND_TARGET_MASK,	/* struct in6_addr */
+	TCA_FLOWER_KEY_ND_SLL,		/* ETH_ALEN */
+	TCA_FLOWER_KEY_ND_SLL_MASK,	/* ETH_ALEN */
+	TCA_FLOWER_KEY_ND_TLL,		/* ETH_ALEN */
+	TCA_FLOWER_KEY_ND_TLL_MASK,	/* ETH_ALEN */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 745e03f180d0..513193825268 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -119,7 +119,7 @@ static bool skb_flow_dissect_nd(const struct sk_buff *skb,
 				int icmp_len)
 {
 	const struct nd_msg *nd;
-	struct nd_msg *_nd;
+	struct nd_msg _nd;
 	int off;
 
 	nd = __skb_header_pointer(skb, nhoff, sizeof(_nd), data, hlen, &_nd);
@@ -131,7 +131,7 @@ static bool skb_flow_dissect_nd(const struct sk_buff *skb,
 	off = sizeof(_nd);
 	while (icmp_len - off >= sizeof(struct nd_opt_hdr) + ETH_ALEN) {
 		const struct nd_opt_hdr *opt_hdr;
-		struct nd_opt_hdr *_opt_hdr;
+		struct nd_opt_hdr _opt_hdr;
 		unsigned char *ll_addr = NULL;
 		int opt_data_len;
 
@@ -704,7 +704,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
 
 		if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ND) &&
-		    ip_proto == IPPROTO_IPV6 && key_icmp->code == 0 &&
+		    ip_proto == IPPROTO_ICMPV6 && key_icmp->code == 0 &&
 		    (key_icmp->type == NDISC_NEIGHBOUR_SOLICITATION ||
 		     key_icmp->type == NDISC_NEIGHBOUR_ADVERTISEMENT)) {
 			key_nd = skb_flow_dissector_target(flow_dissector,
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 23c4d224dcb1..49337091a015 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -40,6 +40,7 @@ struct fl_flow_key {
 	};
 	struct flow_dissector_key_ports tp;
 	struct flow_dissector_key_icmp icmp;
+	struct flow_dissector_key_nd nd;
 	struct flow_dissector_key_arp arp;
 	struct flow_dissector_key_keyid enc_key_id;
 	union {
@@ -418,6 +419,12 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ARP_SHA_MASK]	= { .len = ETH_ALEN },
 	[TCA_FLOWER_KEY_ARP_THA]	= { .len = ETH_ALEN },
 	[TCA_FLOWER_KEY_ARP_THA_MASK]	= { .len = ETH_ALEN },
+	[TCA_FLOWER_KEY_ND_TARGET]	= { .len = sizeof(struct in6_addr) },
+	[TCA_FLOWER_KEY_ND_TARGET_MASK]	= { .len = sizeof(struct in6_addr) },
+	[TCA_FLOWER_KEY_ND_SLL]		= { .len = ETH_ALEN },
+	[TCA_FLOWER_KEY_ND_SLL_MASK]	= { .len = ETH_ALEN },
+	[TCA_FLOWER_KEY_ND_TLL]		= { .len = ETH_ALEN },
+	[TCA_FLOWER_KEY_ND_TLL_MASK]	= { .len = ETH_ALEN },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -589,6 +596,22 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       &mask->icmp.code,
 			       TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
 			       sizeof(key->icmp.code));
+		if (key->icmp.code == 0 && mask->icmp.code == 0xff &&
+		    (key->icmp.type == NDISC_NEIGHBOUR_SOLICITATION ||
+		     key->icmp.type == NDISC_NEIGHBOUR_ADVERTISEMENT) &&
+		    mask->icmp.type == 0xff) {
+			fl_set_key_val(tb, &key->nd.target,
+				       TCA_FLOWER_KEY_ND_TARGET,
+				       &mask->nd.target,
+				       TCA_FLOWER_KEY_ND_TARGET_MASK,
+				       sizeof(key->nd.target));
+			fl_set_key_val(tb, key->nd.sll, TCA_FLOWER_KEY_ND_SLL,
+				       mask->nd.sll, TCA_FLOWER_KEY_ND_SLL_MASK,
+				       sizeof(key->nd.sll));
+			fl_set_key_val(tb, key->nd.tll, TCA_FLOWER_KEY_ND_TLL,
+				       mask->nd.tll, TCA_FLOWER_KEY_ND_TLL_MASK,
+				       sizeof(key->nd.tll));
+		}
 	} else if (key->basic.n_proto == htons(ETH_P_ARP) ||
 		   key->basic.n_proto == htons(ETH_P_RARP)) {
 		fl_set_key_val(tb, &key->arp.sip, TCA_FLOWER_KEY_ARP_SIP,
@@ -725,6 +748,8 @@ static void fl_init_dissector(struct cls_fl_head *head,
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ARP, arp);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+			     FLOW_DISSECTOR_KEY_ND, nd);
+	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ENC_KEYID, enc_key_id);
@@ -1162,7 +1187,22 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		  fl_dump_key_val(skb, &key->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
-				  sizeof(key->icmp.code))))
+				  sizeof(key->icmp.code)) ||
+		 (key->icmp.code == 0 && mask->icmp.code == 0xff &&
+		  (key->icmp.type == NDISC_NEIGHBOUR_SOLICITATION ||
+		   key->icmp.type == NDISC_NEIGHBOUR_ADVERTISEMENT) &&
+		  mask->icmp.type == 0xff &&
+		  (fl_dump_key_val(skb, &key->nd.target,
+				   TCA_FLOWER_KEY_ND_TARGET,
+				   &mask->nd.target,
+				   TCA_FLOWER_KEY_ND_TARGET_MASK,
+				   sizeof(key->nd.target)) ||
+		   fl_dump_key_val(skb, key->nd.sll, TCA_FLOWER_KEY_ND_SLL,
+				   mask->nd.sll, TCA_FLOWER_KEY_ND_SLL_MASK,
+				   sizeof(key->nd.sll)) ||
+		   fl_dump_key_val(skb, key->nd.tll, TCA_FLOWER_KEY_ND_TLL,
+				   mask->nd.tll, TCA_FLOWER_KEY_ND_TLL_MASK,
+				   sizeof(key->nd.tll))))))
 		goto nla_put_failure;
 	else if ((key->basic.n_proto == htons(ETH_P_ARP) ||
 		  key->basic.n_proto == htons(ETH_P_RARP)) &&
-- 
2.7.0.rc3.207.g0ac5344