[Devel] [PATCH vz7 v2] net: neigh: decrement the family specific qlen
Alexander Atanasov
alexander.atanasov at virtuozzo.com
Tue Jan 16 10:10:27 MSK 2024
On 16.01.24 5:12, Pavel Tikhomirov wrote:
> I'd prefer to have two separate clean patches ported instead of porting
> a merge of one into another (without a change explaining comment).
commit 8207f253a097fe15c93d85ac15ebb73c5e39e1e1
Author: Thomas Zeitlhofer <thomas.zeitlhofer+lkml at ze-it.at>
Date: Tue Nov 15 23:09:41 2022 +0100
@@ -409,7 +427,8 @@ static int __neigh_ifdown(struct neigh_table *tbl,
struct net_device *dev,
write_lock_bh(&tbl->lock);
neigh_flush_dev(tbl, dev, skip_perm);
pneigh_ifdown_and_unlock(tbl, dev);
- pneigh_queue_purge(&tbl->proxy_queue, dev ? dev_net(dev) : NULL);
+ pneigh_queue_purge(&tbl->proxy_queue, dev ? dev_net(dev) : NULL,
+ tbl->family);
if (skb_queue_empty_lockless(&tbl->proxy_queue))
del_timer_sync(&tbl->proxy_timer);
return 0;
The hook about device is in the commit i am backporting to vz7.
i made a mistake during the merge so it was not in the v1 patch.
The other commit you talk about is:
commit f8017317cb0b279b8ab98b0f3901a2e0ac880dad
Author: Chen Zhongjin <chenzhongjin at huawei.com>
Date: Tue Nov 1 20:15:52 2022 +0800
Which brings the fix about NULL dev - predates the one i am backporting.
Both logs are from mainstream tree.
>
> On 15/01/2024 22:56, 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 | 58 +++++++++++++++++++++--------------------
>> 2 files changed, 31 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..fa9da27c0676 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,8 @@ 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 ? dev_net(dev) : NULL,
>> + tbl->family);
>> if (skb_queue_empty(&tbl->proxy_queue))
>> del_timer_sync(&tbl->proxy_timer);
>> return 0;
>> @@ -1504,13 +1523,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 +1720,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 +2978,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)
>> {
>
--
Regards,
Alexander Atanasov
More information about the Devel
mailing list