| Message ID | 20220509205646.20814-1-florent.fourcot@wifirst.fr (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] net: neigh: add netlink filtering based on LLADDR for dump | expand |
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.
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,
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,
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.
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,
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.
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");
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(-)