[PATCH net-next] neighbor: Add protocol attribute

[PATCH net-next] neighbor: Add protocol attribute

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Similar to routes and rules, add protocol attribute to neighbor entries
for easier tracking of how each was created.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 6c13072910ab..e93c59df9501 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];
 	struct hh_cache		hh;
@@ -173,6 +174,7 @@ struct pneigh_entry {
 	possible_net_t		net;
 	struct net_device	*dev;
 	u8			flags;
+	u8			protocol;
 	u8			key[0];
 };
 
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 998155444e0d..cd144e3099a3 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -28,6 +28,7 @@ enum {
 	NDA_MASTER,
 	NDA_LINK_NETNSID,
 	NDA_SRC_VNI,
+	NDA_PROTOCOL,  /* Originator of entry */
 	__NDA_MAX
 };
 
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index c3b58712e98b..56984695585d 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1799,6 +1799,7 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_device *dev = NULL;
 	struct neighbour *neigh;
 	void *dst, *lladdr;
+	u8 protocol = 0;
 	int err;
 
 	ASSERT_RTNL();
@@ -1838,6 +1839,14 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	dst = nla_data(tb[NDA_DST]);
 	lladdr = tb[NDA_LLADDR] ? nla_data(tb[NDA_LLADDR]) : NULL;
 
+	if (tb[NDA_PROTOCOL]) {
+		if (nla_len(tb[NDA_PROTOCOL]) != sizeof(u8)) {
+			NL_SET_ERR_MSG(extack, "Invalid protocol attribute");
+			goto out;
+		}
+		protocol = nla_get_u8(tb[NDA_PROTOCOL]);
+	}
+
 	if (ndm->ndm_flags & NTF_PROXY) {
 		struct pneigh_entry *pn;
 
@@ -1845,6 +1854,8 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		pn = pneigh_lookup(tbl, net, dst, dev, 1);
 		if (pn) {
 			pn->flags = ndm->ndm_flags;
+			if (protocol)
+				pn->protocol = protocol;
 			err = 0;
 		}
 		goto out;
@@ -1893,6 +1904,10 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	} else
 		err = __neigh_update(neigh, lladdr, ndm->ndm_state, flags,
 				     NETLINK_CB(skb).portid, extack);
+
+	if (protocol)
+		neigh->protocol = protocol;
+
 	neigh_release(neigh);
 
 out:
@@ -2386,6 +2401,9 @@ static int neigh_fill_info(struct sk_buff *skb, struct neighbour *neigh,
 	    nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
 		goto nla_put_failure;
 
+	if (neigh->protocol && nla_put_u8(skb, NDA_PROTOCOL, neigh->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 
@@ -2417,6 +2435,9 @@ static int pneigh_fill_info(struct sk_buff *skb, struct pneigh_entry *pn,
 	if (nla_put(skb, NDA_DST, tbl->key_len, pn->key))
 		goto nla_put_failure;
 
+	if (pn->protocol && nla_put_u8(skb, NDA_PROTOCOL, pn->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 
@@ -3072,7 +3093,8 @@ static inline size_t neigh_nlmsg_size(void)
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_LLADDR */
 	       + nla_total_size(sizeof(struct nda_cacheinfo))
-	       + nla_total_size(4); /* NDA_PROBES */
+	       + nla_total_size(4)  /* NDA_PROBES */
+	       + nla_total_size(1); /* NDA_PROTOCOL */
 }
 
 static void __neigh_notify(struct neighbour *n, int type, int flags,
-- 
2.11.0

Re: [PATCH net-next] neighbor: Add protocol attribute

[Eric Dumazet]

On 12/07/2018 01:49 PM, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Similar to routes and rules, add protocol attribute to neighbor entries
> for easier tracking of how each was created.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  include/net/neighbour.h        |  2 ++
>  include/uapi/linux/neighbour.h |  1 +
>  net/core/neighbour.c           | 24 +++++++++++++++++++++++-
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 6c13072910ab..e93c59df9501 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -149,6 +149,7 @@ struct neighbour {
>  	__u8			nud_state;
>  	__u8			type;
>  	__u8			dead;
> +	u8			protocol;
>  	seqlock_t		ha_lock;
>  	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];

This looks like ha[] alignment would change, I am not sure how critical it is.

>  	struct hh_cache		hh;
> @@ -173,6 +174,7 @@ struct pneigh_entry {
>  	possible_net_t		net;
>  	struct net_device	*dev;
>  	u8			flags;
> +	u8			protocol;
>  	u8			key[0];
>  };
>  

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Ahern]

On 12/7/18 3:20 PM, Eric Dumazet wrote:
> 
> 
> On 12/07/2018 01:49 PM, David Ahern wrote:
>> From: David Ahern <dsahern@gmail.com>
>>
>> Similar to routes and rules, add protocol attribute to neighbor entries
>> for easier tracking of how each was created.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> ---
>>  include/net/neighbour.h        |  2 ++
>>  include/uapi/linux/neighbour.h |  1 +
>>  net/core/neighbour.c           | 24 +++++++++++++++++++++++-
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
>> index 6c13072910ab..e93c59df9501 100644
>> --- a/include/net/neighbour.h
>> +++ b/include/net/neighbour.h
>> @@ -149,6 +149,7 @@ struct neighbour {
>>  	__u8			nud_state;
>>  	__u8			type;
>>  	__u8			dead;
>> +	u8			protocol;
>>  	seqlock_t		ha_lock;
>>  	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];
> 
> This looks like ha[] alignment would change, I am not sure how critical it is.

Just adds 4 bytes to neighbour:

...
/* --- cacheline 2 boundary (128 bytes) --- */
long unsigned int          used;                 /*   128     8 */
atomic_t                   probes;               /*   136     4 */
__u8                       flags;                /*   140     1 */
__u8                       nud_state;            /*   141     1 */
__u8                       type;                 /*   142     1 */
__u8                       dead;                 /*   143     1 */
u8                         protocol;             /*   144     1 */

/* XXX 3 bytes hole, try to pack */
seqlock_t                  ha_lock;              /*   148     8 */
unsigned char              ha[32];               /*   156    32 */
/* XXX 4 bytes hole, try to pack */

/* --- cacheline 3 boundary (192 bytes) --- */
struct hh_cache            hh;                   /*   192    48 */

...

but does not change the actual allocation size which is rounded to 512.

Re: [PATCH net-next] neighbor: Add protocol attribute

[Eric Dumazet]

On 12/07/2018 02:24 PM, David Ahern wrote:
> On 12/7/18 3:20 PM, Eric Dumazet wrote:
>>
>>
>> On 12/07/2018 01:49 PM, David Ahern wrote:
>>> From: David Ahern <dsahern@gmail.com>
>>>
>>> Similar to routes and rules, add protocol attribute to neighbor entries
>>> for easier tracking of how each was created.
>>>
>>> Signed-off-by: David Ahern <dsahern@gmail.com>
>>> ---
>>>  include/net/neighbour.h        |  2 ++
>>>  include/uapi/linux/neighbour.h |  1 +
>>>  net/core/neighbour.c           | 24 +++++++++++++++++++++++-
>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
>>> index 6c13072910ab..e93c59df9501 100644
>>> --- a/include/net/neighbour.h
>>> +++ b/include/net/neighbour.h
>>> @@ -149,6 +149,7 @@ struct neighbour {
>>>  	__u8			nud_state;
>>>  	__u8			type;
>>>  	__u8			dead;
>>> +	u8			protocol;
>>>  	seqlock_t		ha_lock;
>>>  	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];
>>
>> This looks like ha[] alignment would change, I am not sure how critical it is.
> 
> Just adds 4 bytes to neighbour:
> 
> ...
> /* --- cacheline 2 boundary (128 bytes) --- */
> long unsigned int          used;                 /*   128     8 */
> atomic_t                   probes;               /*   136     4 */
> __u8                       flags;                /*   140     1 */
> __u8                       nud_state;            /*   141     1 */
> __u8                       type;                 /*   142     1 */
> __u8                       dead;                 /*   143     1 */
> u8                         protocol;             /*   144     1 */
> 
> /* XXX 3 bytes hole, try to pack */
> seqlock_t                  ha_lock;              /*   148     8 */
> unsigned char              ha[32];               /*   156    32 */
> /* XXX 4 bytes hole, try to pack */
> 
> /* --- cacheline 3 boundary (192 bytes) --- */
> struct hh_cache            hh;                   /*   192    48 */
> 
> ...
> 
> but does not change the actual allocation size which is rounded to 512.
> 

I have not talked about the allocation size, but alignment of ->ha field,
which is kind of assuming long alignment, in a strange way.

As I said, I do not know how performance critical this might be.

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 7 Dec 2018 15:03:04 -0800

> On 12/07/2018 02:24 PM, David Ahern wrote:
>> On 12/7/18 3:20 PM, Eric Dumazet wrote:
>> 
>> /* --- cacheline 3 boundary (192 bytes) --- */
>> struct hh_cache            hh;                   /*   192    48 */
>> 
>> ...
>> 
>> but does not change the actual allocation size which is rounded to 512.
>> 
> 
> I have not talked about the allocation size, but alignment of ->ha field,
> which is kind of assuming long alignment, in a strange way.

Right, neigh->ha[] should probably be kept 8-byte aligned.

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Ahern]

On 12/7/18 4:45 PM, David Miller wrote:
> 
> Right, neigh->ha[] should probably be kept 8-byte aligned.
> 

>From what I can see ha is only used with memcpy, and neighbour struct is
annotated with __randomize_layout. Are you saying that ha should be
marked with __aligned(8)?


@@ -150,7 +150,7 @@ struct neighbour {
        __u8                    type;
        __u8                    dead;
        seqlock_t               ha_lock;
-       unsigned char           ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned
long))];
+       unsigned char           ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned
long))] __aligned(8);
        struct hh_cache         hh;
        int                     (*output)(struct neighbour *, struct
sk_buff *);
        const struct neigh_ops  *ops;

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Mon, 10 Dec 2018 19:47:33 -0700

> On 12/7/18 4:45 PM, David Miller wrote:
>> 
>> Right, neigh->ha[] should probably be kept 8-byte aligned.
>> 
> 
> From what I can see ha is only used with memcpy, and neighbour struct is
> annotated with __randomize_layout. Are you saying that ha should be
> marked with __aligned(8)?

People who care about performance probably don't build with randomization
enabled, do they?

Even though it uses memcpy() it will be faster if it is 8 byte aligned
and we can probably explicitly take advantage of that alignment even
more if we add the marking as you suggest perhaps.

Given all of this, what is your opinion?

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Ahern]

On 12/10/18 10:59 PM, David Miller wrote:
> From: David Ahern <dsahern@gmail.com>
> Date: Mon, 10 Dec 2018 19:47:33 -0700
> 
>> On 12/7/18 4:45 PM, David Miller wrote:
>>>
>>> Right, neigh->ha[] should probably be kept 8-byte aligned.
>>>
>>
>> From what I can see ha is only used with memcpy, and neighbour struct is
>> annotated with __randomize_layout. Are you saying that ha should be
>> marked with __aligned(8)?
> 
> People who care about performance probably don't build with randomization
> enabled, do they?
> 
> Even though it uses memcpy() it will be faster if it is 8 byte aligned
> and we can probably explicitly take advantage of that alignment even
> more if we add the marking as you suggest perhaps.
> 
> Given all of this, what is your opinion?
> 

Arguably my take is ethernet centric. I do not see how 8-byte alignment
matters when copying 6 bytes. In my response to Eric I showed ha is
still 4-byte aligned and does not straddle cachelines. Those seem the
more relevant to me.

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Tue, 11 Dec 2018 08:22:04 -0700

> On 12/10/18 10:59 PM, David Miller wrote:
>> From: David Ahern <dsahern@gmail.com>
>> Date: Mon, 10 Dec 2018 19:47:33 -0700
>> 
>>> On 12/7/18 4:45 PM, David Miller wrote:
>>>>
>>>> Right, neigh->ha[] should probably be kept 8-byte aligned.
>>>>
>>>
>>> From what I can see ha is only used with memcpy, and neighbour struct is
>>> annotated with __randomize_layout. Are you saying that ha should be
>>> marked with __aligned(8)?
>> 
>> People who care about performance probably don't build with randomization
>> enabled, do they?
>> 
>> Even though it uses memcpy() it will be faster if it is 8 byte aligned
>> and we can probably explicitly take advantage of that alignment even
>> more if we add the marking as you suggest perhaps.
>> 
>> Given all of this, what is your opinion?
>> 
> 
> Arguably my take is ethernet centric. I do not see how 8-byte alignment
> matters when copying 6 bytes. In my response to Eric I showed ha is
> still 4-byte aligned and does not straddle cachelines. Those seem the
> more relevant to me.

Ok, please resubmit as-is if you like.

Re: [PATCH net-next] neighbor: Add protocol attribute

[David Miller]

From: David Ahern <dsahern@kernel.org>
Date: Sat, 15 Dec 2018 14:09:06 -0800

> From: David Ahern <dsahern@gmail.com>
> 
> Similar to routes and rules, add protocol attribute to neighbor entries
> for easier tracking of how each was created.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Applied, thanks David.

[PATCH net-next] neighbor: Add protocol attribute

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Similar to routes and rules, add protocol attribute to neighbor entries
for easier tracking of how each was created.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 30fd50adf234..66221f1991c0 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))] __aligned(8);
 	struct hh_cache		hh;
@@ -173,6 +174,7 @@ struct pneigh_entry {
 	possible_net_t		net;
 	struct net_device	*dev;
 	u8			flags;
+	u8			protocol;
 	u8			key[0];
 };
 
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 998155444e0d..cd144e3099a3 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -28,6 +28,7 @@ enum {
 	NDA_MASTER,
 	NDA_LINK_NETNSID,
 	NDA_SRC_VNI,
+	NDA_PROTOCOL,  /* Originator of entry */
 	__NDA_MAX
 };
 
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 42b413774370..fb4372cb1de1 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1828,6 +1828,7 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_device *dev = NULL;
 	struct neighbour *neigh;
 	void *dst, *lladdr;
+	u8 protocol = 0;
 	int err;
 
 	ASSERT_RTNL();
@@ -1867,6 +1868,14 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	dst = nla_data(tb[NDA_DST]);
 	lladdr = tb[NDA_LLADDR] ? nla_data(tb[NDA_LLADDR]) : NULL;
 
+	if (tb[NDA_PROTOCOL]) {
+		if (nla_len(tb[NDA_PROTOCOL]) != sizeof(u8)) {
+			NL_SET_ERR_MSG(extack, "Invalid protocol attribute");
+			goto out;
+		}
+		protocol = nla_get_u8(tb[NDA_PROTOCOL]);
+	}
+
 	if (ndm->ndm_flags & NTF_PROXY) {
 		struct pneigh_entry *pn;
 
@@ -1874,6 +1883,8 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		pn = pneigh_lookup(tbl, net, dst, dev, 1);
 		if (pn) {
 			pn->flags = ndm->ndm_flags;
+			if (protocol)
+				pn->protocol = protocol;
 			err = 0;
 		}
 		goto out;
@@ -1924,6 +1935,10 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	} else
 		err = __neigh_update(neigh, lladdr, ndm->ndm_state, flags,
 				     NETLINK_CB(skb).portid, extack);
+
+	if (protocol)
+		neigh->protocol = protocol;
+
 	neigh_release(neigh);
 
 out:
@@ -2417,6 +2432,9 @@ static int neigh_fill_info(struct sk_buff *skb, struct neighbour *neigh,
 	    nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
 		goto nla_put_failure;
 
+	if (neigh->protocol && nla_put_u8(skb, NDA_PROTOCOL, neigh->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 
@@ -2448,6 +2466,9 @@ static int pneigh_fill_info(struct sk_buff *skb, struct pneigh_entry *pn,
 	if (nla_put(skb, NDA_DST, tbl->key_len, pn->key))
 		goto nla_put_failure;
 
+	if (pn->protocol && nla_put_u8(skb, NDA_PROTOCOL, pn->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 
@@ -3103,7 +3124,8 @@ static inline size_t neigh_nlmsg_size(void)
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_LLADDR */
 	       + nla_total_size(sizeof(struct nda_cacheinfo))
-	       + nla_total_size(4); /* NDA_PROBES */
+	       + nla_total_size(4)  /* NDA_PROBES */
+	       + nla_total_size(1); /* NDA_PROTOCOL */
 }
 
 static void __neigh_notify(struct neighbour *n, int type, int flags,
-- 
2.11.0