From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4036E2E11B8 for ; Tue, 23 Dec 2025 10:14:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766484851; cv=none; b=ShWOqRYC86hre0i8LqhU832jCj9mQ+QaZ4C+riNw0Y5+R7HUaXlDbCRtGPqIGlHja+bnn4ZXUQ1XihE6NBmxCIa3wl0Mhe0LC0DAwVvTEz2ohEnchzs7HigZ8oq4caR7gs3pRuC3OazVxog1LV0pSeJ/Veao2X1IoVzIKQtr4SM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766484851; c=relaxed/simple; bh=d4tt4MzuujQIcwinJfE/H+S9mMdM1lQY1ynSYHlPwfQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=JLLu09KlMek9rt2cdlABIvhwRacaOZzNfFRK+hx7uTrh/Y14ec0Ci7S0aorIJ2oQCeK/cF31rX6AEA/teJouwwMNQ1JkXQeXtnbxZqQsJIXmFmMlxf0F9BzwFmWuvRUZAEjfyWH7PzhkOA+JjTOLMerWgZBYxOnJLCrgtpzWcns= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=Sxp390ly; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=Z8ADIqU4; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="Sxp390ly"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="Z8ADIqU4" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1766484846; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oFR3UankU/ZXumu63drTFEhhEbrQvsrwe1Tofny6yYE=; b=Sxp390ly+diXzzZ93ZV+wCT9qNQTZnJ9b6bedEUVqgOB5XrPnbPgalkeyCi3O6IKYvL/ZI NBkMMGli53nM/8vUt3enzKT+GAnLAI8uAW3cBbk0mz5tHZAiaX7LJXbKVjy3UPpgyCwSxT 8NneaA3WlArUMfSaLyn4aOLgE6TMyZ8= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-60-UpXVa33mPAm6Dz1gFS5IuQ-1; Tue, 23 Dec 2025 05:14:04 -0500 X-MC-Unique: UpXVa33mPAm6Dz1gFS5IuQ-1 X-Mimecast-MFC-AGG-ID: UpXVa33mPAm6Dz1gFS5IuQ_1766484843 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-477964c22e0so34312135e9.0 for ; Tue, 23 Dec 2025 02:14:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1766484843; x=1767089643; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=oFR3UankU/ZXumu63drTFEhhEbrQvsrwe1Tofny6yYE=; b=Z8ADIqU4OhZUiN2d0dNI2W9BzoB1KWgJLNrHKUrTk6UCHWXessNPoO0hfCtSicpN/1 nz5pbWTdVC0I4xWzHp2W6OHcMGMiaBz5uThRSabXYkMK6vnOfc5iazTSYYep1aD2g+wy 7/p8tq2EJfCQa3JF0ENhLidCyZnW9MSaE2ia64uC7pAzPpQQQ1in+r/Zed5KJmWwm/P5 wIcgXxD/lL/4RlZBKQxSnCV6LwGdjV/EGYeRn5w4HZPn9W4FXCn3utWOXlmpEWc5OH3J MEr7JIv8qXAAMHNaDMLl6845thSydti5oU71BhW/Lpy5nUj50ENRZ0+CiY4/DJcpWYxQ Fo5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1766484843; x=1767089643; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=oFR3UankU/ZXumu63drTFEhhEbrQvsrwe1Tofny6yYE=; b=YBmiDkAZgefqC9YgQJj/mU9oBPErPkvWkXPCPblwkAI6or9F/gmY0Wa9Ifop8RfK/z l/Fh2rnm2OPD1lBRo+VtxkSqYPbYRynzB3ZKYJ5SeIhjBHHhZGmeCZh7yrfYEZ0ADU7/ Bvq5TqxvGx5BFGgM1FgVyIT5cI5It6fpIjcC1CyaC8HoqGx2rgEGJa5oQX91L+cXCP5S xmkaoaAvlIHFxPuISg0FCulgcMRi2WWSWHRF1A9JexTQkXz1jvH6+8eoe0nYzkKccedi zFfph9rC2huO9o3uC7NbvtN5ewwNnn+RlEeQw5me66t4LfH/HwTVdPbetllhPYlAIryw P42w== X-Forwarded-Encrypted: i=1; AJvYcCWxXUJH/7eWVO1v98kr8IKNBQpWfbiKSCrXjykow9wMNgqqEHtOIPOBHMU6OHAFpEWzFL3F+oqShm483h8=@vger.kernel.org X-Gm-Message-State: AOJu0YxNyvk/YwgmB71ddKeDPCT/JADJRMaWr6vuMQeK/bk1xl9Psy3J bc1azklm05ZFaLhyX++mD1y4MQ+vATSGf91bBZLG9SBLCyJpbMtWWyc1gwCv9b0E+fLr5KbNsQM 7pD5X8O78If6LShRM2TXE9hWHLHd8jvdcB5uhvY3LeeFHBBHWbBgrwFQz7cz5ICbaSA== X-Gm-Gg: AY/fxX6euhOrkcSpqqN4w3M1oeCUL/30cvw9zdHiV+KPqCk/2ZJVAsBs/PWaiplcvKQ MGtghEIhGo29u7kDtphpFCDFo4xjyLy0CR5R2Y0ZX36cTfVxb0CVZkXS5FWKmdlzPin/MJVyCAV jRg0dP/71eQ4fR0MOKkJA1wzxWM8vMNpr00SNLsbFZygo3unsFI2HmTFzgkZLJTCD/6jofwDNPx Ehxdk/q/obxT6klOSiAFaTPAfPOcamraLpOsKi7qT4jSgaXwAJrzsaPz2WbD/HVD7rg9uRP0qfN tCskemvNTzMHremG7aeQsV/WE7xMebNs8aOzPSdXJ00EJAiqZ2boPI/iVx/ekluViE5ERTAtC09 8/5OP1TOIy8bv X-Received: by 2002:a05:600c:4746:b0:477:9fa0:7495 with SMTP id 5b1f17b1804b1-47d18be144fmr125219345e9.14.1766484843080; Tue, 23 Dec 2025 02:14:03 -0800 (PST) X-Google-Smtp-Source: AGHT+IHBVFBFV8JQghn5DbnVX+mIgO0xj1voHIdhg+zVPbfA8QA2nep/Xwt/TcDBUPc1v4/lT/IkiQ== X-Received: by 2002:a05:600c:4746:b0:477:9fa0:7495 with SMTP id 5b1f17b1804b1-47d18be144fmr125217095e9.14.1766484838326; Tue, 23 Dec 2025 02:13:58 -0800 (PST) Received: from [192.168.88.32] ([216.128.11.164]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47be3964226sm124149365e9.0.2025.12.23.02.13.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Dec 2025 02:13:57 -0800 (PST) Message-ID: Date: Tue, 23 Dec 2025 11:13:56 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net] ipvlan: Make the addrs_lock be per port To: Dmitry Skorodumov , netdev@vger.kernel.org, Xiao Liang , Jakub Kicinski , Kuniyuki Iwashima , Guillaume Nault , Julian Vetter , Eric Dumazet , Stanislav Fomichev , Etienne Champetier , "David S. Miller" , linux-kernel@vger.kernel.org Cc: Andrew Lunn References: <20251215165457.752634-1-skorodumov.dmitry@huawei.com> Content-Language: en-US From: Paolo Abeni In-Reply-To: <20251215165457.752634-1-skorodumov.dmitry@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 > CC: Paolo Abeni > Signed-off-by: Dmitry Skorodumov 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