* [PATCH net 1/2] ipvlan: Make the addrs_lock be per port
[not found] <20251202141149.4144248-1-skorodumov.dmitry@huawei.com>
@ 2025-12-02 14:11 ` Dmitry Skorodumov
2025-12-02 14:11 ` [PATCH net 2/2] ipvlan: Take addr_lock in ipvlan_open() Dmitry Skorodumov
1 sibling, 0 replies; 4+ messages in thread
From: Dmitry Skorodumov @ 2025-12-02 14:11 UTC (permalink / raw)
To: netdev, Dmitry Skorodumov, Xiao Liang, Kuniyuki Iwashima,
Jakub Kicinski, Stanislav Fomichev, Etienne Champetier,
Paolo Abeni, David S. Miller, linux-kernel
Cc: Andrew Lunn, Eric Dumazet
Make the addrs_lock be per port, not per ipvlan dev.
This appears to be a very minor problem though.
Since it's highly unlikely that ipvlan_add_addr() will
be called on 2 CPU simultaneously. But nevertheless,
this may cause:
1. False-negative of ipvlan_addr_busy(): one interface
iterated through all port->ipvlans + ipvlan->addrs
under some ipvlan spinlock, and another added IP
under its own lock. Though this is only possible
for IPv6, since looks like only ipvlan_addr6_event() can be
called without rtnl_lock.
2. Race since ipvlan_ht_addr_add(port) is called under
different ipvlan->addrs_lock locks
This should not affect performance, since add/remove IP
is a rare situation and spinlock is not locked on fast
paths.
Fixes: 8230819494b3 ("ipvlan: use per device spinlock to protect addrs list updates")
Signed-off-by: Dmitry Skorodumov <skorodumov.dmitry@huawei.com>
CC: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/ipvlan/ipvlan.h | 2 +-
drivers/net/ipvlan/ipvlan_main.c | 20 ++++++++++----------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
index 50de3ee204db..80f84fc87008 100644
--- a/drivers/net/ipvlan/ipvlan.h
+++ b/drivers/net/ipvlan/ipvlan.h
@@ -69,7 +69,6 @@ struct ipvl_dev {
DECLARE_BITMAP(mac_filters, IPVLAN_MAC_FILTER_SIZE);
netdev_features_t sfeatures;
u32 msg_enable;
- spinlock_t addrs_lock;
};
struct ipvl_addr {
@@ -90,6 +89,7 @@ struct ipvl_port {
struct net_device *dev;
possible_net_t pnet;
struct hlist_head hlhead[IPVLAN_HASH_SIZE];
+ spinlock_t addrs_lock; /* guards hash-table and addrs */
struct list_head ipvlans;
u16 mode;
u16 flags;
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 660f3db11766..c390f4241621 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -75,6 +75,7 @@ static int ipvlan_port_create(struct net_device *dev)
for (idx = 0; idx < IPVLAN_HASH_SIZE; idx++)
INIT_HLIST_HEAD(&port->hlhead[idx]);
+ spin_lock_init(&port->addrs_lock);
skb_queue_head_init(&port->backlog);
INIT_WORK(&port->wq, ipvlan_process_multicast);
ida_init(&port->ida);
@@ -579,7 +580,6 @@ int ipvlan_link_new(struct net_device *dev, struct rtnl_newlink_params *params,
if (!tb[IFLA_MTU])
ipvlan_adjust_mtu(ipvlan, phy_dev);
INIT_LIST_HEAD(&ipvlan->addrs);
- spin_lock_init(&ipvlan->addrs_lock);
/* TODO Probably put random address here to be presented to the
* world but keep using the physical-dev address for the outgoing
@@ -657,13 +657,13 @@ void ipvlan_link_delete(struct net_device *dev, struct list_head *head)
struct ipvl_dev *ipvlan = netdev_priv(dev);
struct ipvl_addr *addr, *next;
- spin_lock_bh(&ipvlan->addrs_lock);
+ spin_lock_bh(&ipvlan->port->addrs_lock);
list_for_each_entry_safe(addr, next, &ipvlan->addrs, anode) {
ipvlan_ht_addr_del(addr);
list_del_rcu(&addr->anode);
kfree_rcu(addr, rcu);
}
- spin_unlock_bh(&ipvlan->addrs_lock);
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
ida_free(&ipvlan->port->ida, dev->dev_id);
list_del_rcu(&ipvlan->pnode);
@@ -847,16 +847,16 @@ static void ipvlan_del_addr(struct ipvl_dev *ipvlan, void *iaddr, bool is_v6)
{
struct ipvl_addr *addr;
- spin_lock_bh(&ipvlan->addrs_lock);
+ spin_lock_bh(&ipvlan->port->addrs_lock);
addr = ipvlan_find_addr(ipvlan, iaddr, is_v6);
if (!addr) {
- spin_unlock_bh(&ipvlan->addrs_lock);
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
return;
}
ipvlan_ht_addr_del(addr);
list_del_rcu(&addr->anode);
- spin_unlock_bh(&ipvlan->addrs_lock);
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
kfree_rcu(addr, rcu);
}
@@ -878,14 +878,14 @@ static int ipvlan_add_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
{
int ret = -EINVAL;
- spin_lock_bh(&ipvlan->addrs_lock);
+ spin_lock_bh(&ipvlan->port->addrs_lock);
if (ipvlan_addr_busy(ipvlan->port, ip6_addr, true))
netif_err(ipvlan, ifup, ipvlan->dev,
"Failed to add IPv6=%pI6c addr for %s intf\n",
ip6_addr, ipvlan->dev->name);
else
ret = ipvlan_add_addr(ipvlan, ip6_addr, true);
- spin_unlock_bh(&ipvlan->addrs_lock);
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
return ret;
}
@@ -946,14 +946,14 @@ static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
{
int ret = -EINVAL;
- spin_lock_bh(&ipvlan->addrs_lock);
+ spin_lock_bh(&ipvlan->port->addrs_lock);
if (ipvlan_addr_busy(ipvlan->port, ip4_addr, false))
netif_err(ipvlan, ifup, ipvlan->dev,
"Failed to add IPv4=%pI4 on %s intf.\n",
ip4_addr, ipvlan->dev->name);
else
ret = ipvlan_add_addr(ipvlan, ip4_addr, false);
- spin_unlock_bh(&ipvlan->addrs_lock);
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
return ret;
}
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH net 2/2] ipvlan: Take addr_lock in ipvlan_open()
[not found] <20251202141149.4144248-1-skorodumov.dmitry@huawei.com>
2025-12-02 14:11 ` [PATCH net 1/2] ipvlan: Make the addrs_lock be per port Dmitry Skorodumov
@ 2025-12-02 14:11 ` Dmitry Skorodumov
2025-12-04 14:42 ` Paolo Abeni
1 sibling, 1 reply; 4+ messages in thread
From: Dmitry Skorodumov @ 2025-12-02 14:11 UTC (permalink / raw)
To: netdev, Jakub Kicinski, Kuniyuki Iwashima, Xiao Liang,
Stanislav Fomichev, Dmitry Skorodumov, Etienne Champetier,
David S. Miller, Paolo Abeni, linux-kernel
Cc: Andrew Lunn, Eric Dumazet
It was forgotten to lock addrs in ipvlan_open().
Seems that code was initially written in assumption
that any address change occurs under rtnl_lock(). But
it's not true for the ipv6 case. So, we have to
take addr_lock in ipvlan_open().
Also, take the addrs_lock in ipvlan_close()
Fixes: 8230819494b3 ("ipvlan: use per device spinlock to protect addrs list updates")
Signed-off-by: Dmitry Skorodumov <skorodumov.dmitry@huawei.com>
CC: Paolo Abeni <pabeni@redhat.com>
---
drivers/net/ipvlan/ipvlan_main.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index c390f4241621..53d311af2f44 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -182,18 +182,18 @@ static void ipvlan_uninit(struct net_device *dev)
static int ipvlan_open(struct net_device *dev)
{
struct ipvl_dev *ipvlan = netdev_priv(dev);
+ struct ipvl_port *port = ipvlan->port;
struct ipvl_addr *addr;
- if (ipvlan->port->mode == IPVLAN_MODE_L3 ||
- ipvlan->port->mode == IPVLAN_MODE_L3S)
+ if (port->mode == IPVLAN_MODE_L3 || port->mode == IPVLAN_MODE_L3S)
dev->flags |= IFF_NOARP;
else
dev->flags &= ~IFF_NOARP;
- rcu_read_lock();
+ spin_lock_bh(&port->addrs_lock);
list_for_each_entry_rcu(addr, &ipvlan->addrs, anode)
ipvlan_ht_addr_add(ipvlan, addr);
- rcu_read_unlock();
+ spin_unlock_bh(&port->addrs_lock);
return 0;
}
@@ -207,10 +207,10 @@ static int ipvlan_stop(struct net_device *dev)
dev_uc_unsync(phy_dev, dev);
dev_mc_unsync(phy_dev, dev);
- rcu_read_lock();
+ spin_lock_bh(&ipvlan->port->addrs_lock);
list_for_each_entry_rcu(addr, &ipvlan->addrs, anode)
ipvlan_ht_addr_del(addr);
- rcu_read_unlock();
+ spin_unlock_bh(&ipvlan->port->addrs_lock);
return 0;
}
@@ -817,6 +817,8 @@ static int ipvlan_add_addr(struct ipvl_dev *ipvlan, void *iaddr, bool is_v6)
{
struct ipvl_addr *addr;
+ assert_spin_locked(&ipvlan->port->addrs_lock);
+
addr = kzalloc(sizeof(struct ipvl_addr), GFP_ATOMIC);
if (!addr)
return -ENOMEM;
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread