[PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[Florent Fourcot]

neighbours table dump supports today two filtering:
 * based on interface index
 * based on master index

This patch adds a new filtering, based on layer two address. That will
help to replace something like it:

 ip neigh show | grep aa:11:22:bb:ee:ff

by a better command:

 ip neigh show lladdr aa:11:22:bb:ee:ff

Changes in v2:
  * Check NDA_LLADDR length

Signed-off-by: Florent Fourcot <florent.fourcot@wifirst.fr>
---
 net/core/neighbour.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 47b6c1f0fdbb..913b9dbcd276 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2641,9 +2641,25 @@ static bool neigh_ifindex_filtered(struct net_device *dev, int filter_idx)
 	return false;
 }
 
+static bool neigh_lladdr_filtered(struct neighbour *neigh, const u8 *lladdr,
+				  u32 lladdr_len)
+{
+	if (!lladdr)
+		return false;
+
+	if (lladdr_len != neigh->dev->addr_len)
+		return true;
+
+	if (memcmp(lladdr, neigh->ha, neigh->dev->addr_len) != 0)
+		return true;
+
+	return false;
+}
+
 struct neigh_dump_filter {
 	int master_idx;
 	int dev_idx;
+	struct nlattr *nla_lladdr;
 };
 
 static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
@@ -2656,13 +2672,20 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
 	int idx, s_idx = idx = cb->args[2];
 	struct neigh_hash_table *nht;
 	unsigned int flags = NLM_F_MULTI;
+	u8 *lladdr = NULL;
+	u32 lladdr_len;
 
-	if (filter->dev_idx || filter->master_idx)
+	if (filter->dev_idx || filter->master_idx || filter->nla_lladdr)
 		flags |= NLM_F_DUMP_FILTERED;
 
 	rcu_read_lock_bh();
 	nht = rcu_dereference_bh(tbl->nht);
 
+	if (filter->nla_lladdr) {
+		lladdr_len = nla_len(filter->nla_lladdr);
+		lladdr = nla_data(filter->nla_lladdr);
+	}
+
 	for (h = s_h; h < (1 << nht->hash_shift); h++) {
 		if (h > s_h)
 			s_idx = 0;
@@ -2672,7 +2695,8 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
 			if (idx < s_idx || !net_eq(dev_net(n->dev), net))
 				goto next;
 			if (neigh_ifindex_filtered(n->dev, filter->dev_idx) ||
-			    neigh_master_filtered(n->dev, filter->master_idx))
+			    neigh_master_filtered(n->dev, filter->master_idx) ||
+			    neigh_lladdr_filtered(n, lladdr, lladdr_len))
 				goto next;
 			if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
 					    cb->nlh->nlmsg_seq,
@@ -2788,6 +2812,13 @@ static int neigh_valid_dump_req(const struct nlmsghdr *nlh,
 		case NDA_MASTER:
 			filter->master_idx = nla_get_u32(tb[i]);
 			break;
+		case NDA_LLADDR:
+			if (!nla_len(tb[i])) {
+				NL_SET_ERR_MSG(extack, "Invalid link address");
+				return -EINVAL;
+			}
+			filter->nla_lladdr = tb[i];
+			break;
 		default:
 			if (strict_check) {
 				NL_SET_ERR_MSG(extack, "Unsupported attribute in neighbor dump request");
-- 
2.30.2

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[David Ahern]

On 5/9/22 2:56 PM, Florent Fourcot wrote:
> neighbours table dump supports today two filtering:
>  * based on interface index
>  * based on master index
> 
> This patch adds a new filtering, based on layer two address. That will
> help to replace something like it:
> 
>  ip neigh show | grep aa:11:22:bb:ee:ff
> 
> by a better command:
> 
>  ip neigh show lladdr aa:11:22:bb:ee:ff
> 

that is done by a GET without the NLM_F_DUMP flag set; doing a table
dump and filtering to get the one entry is wrong.

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[Florent Fourcot]

Hello,

On 10/05/2022 03:38, David Ahern wrote:
> that is done by a GET without the NLM_F_DUMP flag set; doing a table
> dump and filtering to get the one entry is wrong.

GET command takes a L3/IP address and one interface index. It returns 
unique entry matching this tuple.

This patch is for reverse lookup: you have one link layer address, and 
you need entries matching this link layer address. You can receive 
several results, since:

  * One link layer address can be used for several IP addresses on the 
same interface
  * One link layer address can be found on several interfaces

Best regards,

-- 
Florent Fourcot

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[Florent Fourcot]

Hello David,

This patch has been marked as rejected after your comment.
Could you perhaps have a second look on it? And on my response above? I 
still think that my patch is relevant and add a currently not available 
feature.

I can work on alternative approach if necessary. Since neighbour tables 
are sometimes huge, performance overhead of userspace filtering for a 
simple MAC address lookup is currently high. And GET does not provide 
same feature.

Best regards,

-- 
Florent Fourcot

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[David Ahern]

On 5/24/22 2:49 PM, Florent Fourcot wrote:
> Hello David,
> 
> This patch has been marked as rejected after your comment.
> Could you perhaps have a second look on it? And on my response above? I
> still think that my patch is relevant and add a currently not available
> feature.
> 
> I can work on alternative approach if necessary. Since neighbour tables
> are sometimes huge, performance overhead of userspace filtering for a
> simple MAC address lookup is currently high. And GET does not provide
> same feature.
> 

Kernel side filtering has always been kept to simple, coarse grained
checks - like a device index or upper device index. It's a fine line
managing kernel cycles holding the rtnl vs cycles shipping the data to
userspace. e.g., a memcmp has a higher cost than a dev->index
comparison. I see the point about GET only - potential for many matches
and a lookup of the ll address is basically a filtered dump. Mixed
thoughts on whether this should be merged.

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[Florent Fourcot]

Hello David,

> 
> Kernel side filtering has always been kept to simple, coarse grained
> checks - like a device index or upper device index. It's a fine line
> managing kernel cycles holding the rtnl vs cycles shipping the data to
> userspace. e.g., a memcmp has a higher cost than a dev->index
> comparison. I see the point about GET only - potential for many matches
> and a lookup of the ll address is basically a filtered dump. Mixed
> thoughts on whether this should be merged.

Thanks for your feedback. As you know, this option will not slow down 
standard dump.

I understand your concern, but the choice is between:
  * putting all entries on socket to send data to userspace. It means 
several memcpy (at least one for L3 address, one for L2 address) for 
each entries
  * Use proposed filter, with a single memcmp. memcpy is not called for 
filtered out entries.

My solution looks faster, but I can send a v3 with some numbers if you 
think that it's important to get this patch merged.


Best regards,

-- 
Florent Fourcot

-- 
*Ce message et toutes les pièces jointes (ci-après le "message") sont 
établis à l’intention exclusive des destinataires désignés. Il contient des 
informations confidentielles et pouvant être protégé par le secret 
professionnel. Si vous recevez ce message par erreur, merci d'en avertir 
immédiatement l'expéditeur et de détruire le message. Toute utilisation de 
ce message non conforme à sa destination, toute diffusion ou toute 
publication, totale ou partielle, est interdite, sauf autorisation expresse 
de l'émetteur*

Re: [PATCH v2 net-next] net: neigh: add netlink filtering based on LLADDR for dump

[David Ahern]

On 6/9/22 1:58 AM, Florent Fourcot wrote:
> Hello David,
> 
>>
>> Kernel side filtering has always been kept to simple, coarse grained
>> checks - like a device index or upper device index. It's a fine line
>> managing kernel cycles holding the rtnl vs cycles shipping the data to
>> userspace. e.g., a memcmp has a higher cost than a dev->index
>> comparison. I see the point about GET only - potential for many matches
>> and a lookup of the ll address is basically a filtered dump. Mixed
>> thoughts on whether this should be merged.
> 
> Thanks for your feedback. As you know, this option will not slow down
> standard dump.
> 
> I understand your concern, but the choice is between:
>  * putting all entries on socket to send data to userspace. It means
> several memcpy (at least one for L3 address, one for L2 address) for
> each entries
>  * Use proposed filter, with a single memcmp. memcpy is not called for
> filtered out entries.
> 
> My solution looks faster, but I can send a v3 with some numbers if you
> think that it's important to get this patch merged.
> 

sure post a v3.