| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dab5d779-8ea4-4653-a320-d805d08b2547@redhat.com>
Date: Tue, 23 Dec 2025 11:13:56 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Dmitry Skorodumov <skorodumov.dmitry@...wei.com>, netdev@...r.kernel.org,
Xiao Liang <shaw.leon@...il.com>, Jakub Kicinski <kuba@...nel.org>,
Kuniyuki Iwashima <kuniyu@...gle.com>, Guillaume Nault <gnault@...hat.com>,
Julian Vetter <julian@...er-limits.org>, Eric Dumazet <edumazet@...gle.com>,
Stanislav Fomichev <sdf@...ichev.me>,
Etienne Champetier <champetier.etienne@...il.com>,
"David S. Miller" <davem@...emloft.net>, linux-kernel@...r.kernel.org
Cc: Andrew Lunn <andrew+netdev@...n.ch>
Subject: Re: [PATCH net] ipvlan: Make the addrs_lock be per port
On 12/15/25 5:54 PM, Dmitry Skorodumov wrote:
> Make the addrs_lock be per port, not per ipvlan dev.
>
> Initial code seems to be written in the assumption,
> that any address change must occur under RTNL.
> But it is not so for the case of IPv6. So
>
> 1) Introduce per-port addrs_lock.
>
> 2) It was needed to fix places where it was forgotten
> to take lock (ipvlan_open/ipvlan_close)
>
> 3) Fix places, where list_for_each_entry_rcu()
> was used to iterate the list while holding a lock
>
> 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 could 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 taken on fast
> paths.
>
> Fixes: 8230819494b3 ("ipvlan: use per device spinlock to protect addrs list updates")
> Signed-off-by: Dmitry Skorodumov <skorodumov.dmitry@...wei.com>
> CC: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Dmitry Skorodumov <skorodumov.dmitry@...wei.com>
Duplicate signature: drop one.
Side note: you should have included a revision number in the subj prefix
(v2) and a summary of changes since v1 after the '---' separator
> ---
> drivers/net/ipvlan/ipvlan.h | 2 +-
> drivers/net/ipvlan/ipvlan_core.c | 12 ++++----
> drivers/net/ipvlan/ipvlan_main.c | 52 ++++++++++++++++++--------------
> 3 files changed, 37 insertions(+), 29 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_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index 2efa3ba148aa..22cb5ee7a231 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -109,14 +109,14 @@ struct ipvl_addr *ipvlan_find_addr(const struct ipvl_dev *ipvlan,
> {
> struct ipvl_addr *addr, *ret = NULL;
>
> - rcu_read_lock();
> - list_for_each_entry_rcu(addr, &ipvlan->addrs, anode) {
> + assert_spin_locked(&ipvlan->port->addrs_lock);
> +
> + list_for_each_entry(addr, &ipvlan->addrs, anode) {
> if (addr_equal(is_v6, addr, iaddr)) {
> ret = addr;
> break;
You could just return `addr`, and remove the `ret` variable
> }
> }
> - rcu_read_unlock();
> return ret;
> }
>
> @@ -125,14 +125,14 @@ bool ipvlan_addr_busy(struct ipvl_port *port, void *iaddr, bool is_v6)
> struct ipvl_dev *ipvlan;
> bool ret = false;
>
> - rcu_read_lock();
> - list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) {
> + assert_spin_locked(&port->addrs_lock);
> +
> + list_for_each_entry(ipvlan, &port->ipvlans, pnode) {
What protects the `ipvlans` list here? I think the RCU lock is still needed.
> if (ipvlan_find_addr(ipvlan, iaddr, is_v6)) {
> ret = true;
> break;
> }
> }
> - rcu_read_unlock();
> return ret;
> }
>
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 660f3db11766..b0b4f747f162 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);
> @@ -181,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;
Please omit unrelated formatting changes, this fix is already quite big
as is.
Please include the paired self-test in the next iteration (as noted by
Simon self-test can be included into 'net' series, too), thanks!
Paolo
Powered by blists - more mailing lists