[Devel] [PATCH vz7] net: neigh: decrement the family specific qlen
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Mon Jan 15 13:11:10 MSK 2024
Do we also need f8017317cb0b2 ("net, neigh: Fix null-ptr-deref in
neigh_table_clear()") while on it?
Looks good.
Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
On 15/01/2024 16:42, Alexander Atanasov wrote:
> From: Thomas Zeitlhofer <thomas.zeitlhofer+lkml at ze-it.at>
>
> Commit 0ff4eb3d5ebb ("neighbour: make proxy_queue.qlen limit
> per-device") introduced the length counter qlen in struct neigh_parms.
> There are separate neigh_parms instances for IPv4/ARP and IPv6/ND, and
> while the family specific qlen is incremented in pneigh_enqueue(), the
> mentioned commit decrements always the IPv4/ARP specific qlen,
> regardless of the currently processed family, in pneigh_queue_purge()
> and neigh_proxy_process().
>
> As a result, with IPv6/ND, the family specific qlen is only incremented
> (and never decremented) until it exceeds PROXY_QLEN, and then, according
> to the check in pneigh_enqueue(), neighbor solicitations are not
> answered anymore. As an example, this is noted when using the
> subnet-router anycast address to access a Linux router. After a certain
> amount of time (in the observed case, qlen exceeded PROXY_QLEN after two
> days), the Linux router stops answering neighbor solicitations for its
> subnet-router anycast address and effectively becomes unreachable.
>
> Another result with IPv6/ND is that the IPv4/ARP specific qlen is
> decremented more often than incremented. This leads to negative qlen
> values, as a signed integer has been used for the length counter qlen,
> and potentially to an integer overflow.
>
> Fix this by introducing the helper function neigh_parms_qlen_dec(),
> which decrements the family specific qlen. Thereby, make use of the
> existing helper function neigh_get_dev_parms_rcu(), whose definition
> therefore needs to be placed earlier in neighbour.c. Take the family
> member from struct neigh_table to determine the currently processed
> family and appropriately call neigh_parms_qlen_dec() from
> pneigh_queue_purge() and neigh_proxy_process().
>
> Additionally, use an unsigned integer for the length counter qlen.
>
> Fixes: 0ff4eb3d5ebb ("neighbour: make proxy_queue.qlen limit per-device")
> Signed-off-by: Thomas Zeitlhofer <thomas.zeitlhofer+lkml at ze-it.at>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit 8207f253a097fe15c93d85ac15ebb73c5e39e1e1)
> https://virtuozzo.atlassian.net/browse/PSBM-153018
> https://bugs.openvz.org/browse/OVZ-7410
> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
> include/net/neighbour.h | 2 +-
> net/core/neighbour.c | 57 +++++++++++++++++++++--------------------
> 2 files changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 2326044d5294..c63016cc4b27 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -79,7 +79,7 @@ struct neigh_parms {
> struct rcu_head rcu_head;
>
> int reachable_time;
> - int qlen;
> + u32 qlen;
> int data[NEIGH_VAR_DATA_MAX];
> DECLARE_BITMAP(data_state, NEIGH_VAR_DATA_MAX);
> };
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index d5b28c158144..caed48f39896 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -223,7 +223,31 @@ static int neigh_del_timer(struct neighbour *n)
> return 0;
> }
>
> -static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net)
> +static struct neigh_parms *neigh_get_dev_parms_rcu(struct net_device *dev,
> + int family)
> +{
> + switch (family) {
> + case AF_INET:
> + return __in_dev_arp_parms_get_rcu(dev);
> + case AF_INET6:
> + return __in6_dev_nd_parms_get_rcu(dev);
> + }
> + return NULL;
> +}
> +
> +static void neigh_parms_qlen_dec(struct net_device *dev, int family)
> +{
> + struct neigh_parms *p;
> +
> + rcu_read_lock();
> + p = neigh_get_dev_parms_rcu(dev, family);
> + if (p)
> + p->qlen--;
> + rcu_read_unlock();
> +}
> +
> +static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net,
> + int family)
> {
> struct sk_buff_head tmp;
> unsigned long flags;
> @@ -237,13 +261,7 @@ static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net)
> struct net_device *dev = skb->dev;
>
> if (net == NULL || net_eq(dev_net(dev), net)) {
> - struct in_device *in_dev;
> -
> - rcu_read_lock();
> - in_dev = __in_dev_get_rcu(dev);
> - if (in_dev)
> - in_dev->arp_parms->qlen--;
> - rcu_read_unlock();
> + neigh_parms_qlen_dec(dev, family);
> __skb_unlink(skb, list);
> __skb_queue_tail(&tmp, skb);
> }
> @@ -321,7 +339,7 @@ int neigh_ifdown(struct neigh_table *tbl, struct net_device *dev)
> neigh_flush_dev(tbl, dev);
> pneigh_ifdown(tbl, dev);
> write_unlock_bh(&tbl->lock);
> - pneigh_queue_purge(&tbl->proxy_queue, dev_net(dev));
> + pneigh_queue_purge(&tbl->proxy_queue, dev_net(dev), tbl->family);
> if (skb_queue_empty(&tbl->proxy_queue))
> del_timer_sync(&tbl->proxy_timer);
> return 0;
> @@ -1504,13 +1522,8 @@ static void neigh_proxy_process(unsigned long arg)
>
> if (tdif <= 0) {
> struct net_device *dev = skb->dev;
> - struct in_device *in_dev;
>
> - rcu_read_lock();
> - in_dev = __in_dev_get_rcu(dev);
> - if (in_dev)
> - in_dev->arp_parms->qlen--;
> - rcu_read_unlock();
> + neigh_parms_qlen_dec(dev, tbl->family);
> __skb_unlink(skb, &tbl->proxy_queue);
>
> if (tbl->proxy_redo && netif_running(dev)) {
> @@ -1706,7 +1719,7 @@ int neigh_table_clear(int index, struct neigh_table *tbl)
> /* It is not clean... Fix it to unload IPv6 module safely */
> cancel_delayed_work_sync(&tbl->gc_work);
> del_timer_sync(&tbl->proxy_timer);
> - pneigh_queue_purge(&tbl->proxy_queue, NULL);
> + pneigh_queue_purge(&tbl->proxy_queue, NULL, tbl->family);
> neigh_ifdown(tbl, NULL);
> if (atomic_read(&tbl->entries))
> pr_crit("neighbour leakage\n");
> @@ -2964,18 +2977,6 @@ static int proc_unres_qlen(struct ctl_table *ctl, int write,
> return ret;
> }
>
> -static struct neigh_parms *neigh_get_dev_parms_rcu(struct net_device *dev,
> - int family)
> -{
> - switch (family) {
> - case AF_INET:
> - return __in_dev_arp_parms_get_rcu(dev);
> - case AF_INET6:
> - return __in6_dev_nd_parms_get_rcu(dev);
> - }
> - return NULL;
> -}
> -
> static void neigh_copy_dflt_parms(struct net *net, struct neigh_parms *p,
> int index)
> {
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.
More information about the Devel
mailing list