From patchwork Wed Oct 18 15:47:43 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Antoine Tenart X-Patchwork-Id: 13427271 X-Patchwork-Delegate: kuba@kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 8CCBCBE62 for ; Wed, 18 Oct 2023 15:48:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="JWdvQ66g" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 10A87C433C8; Wed, 18 Oct 2023 15:48:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697644090; bh=e6WdhWT7uLorGiereKdNb2V7Boad3dLhWG4qLXS9Hf8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=JWdvQ66gSbZ2iIHTU/LWtReVBd4RWrCOWZ+G89/+4aw7ddY5Bw6vCVn64W7eeHD5N yYIdqtJQliSS7xnm8OjCbAwIw/nAn3MUAawJlshTJujVH7VeYfACAA7t3SeOmw9Dg1 waUq6PxY2LUgRtGookLaghOskpZlRypHnrUA4NZT6SLpG92ziqYgEHbOchLN+4Zj7n tEme+l7vDJco4HiPrtAHCA8lSI35UyswJ2DC97r7sy42fvINRWFlYgZFU47w4IQ6Bt GtEPJ5O/sp3e3lpObLClxISGVT012HAuWM43ALlRCMgi8fzqEbPPApIYEE7OAwPGPF Lh7tFnWnfiBzw== From: Antoine Tenart To: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, edumazet@google.com Cc: Antoine Tenart , netdev@vger.kernel.org, gregkh@linuxfoundation.org, mhocko@suse.com, stephen@networkplumber.org Subject: [RFC PATCH net-next 1/4] net-sysfs: remove rtnl_trylock from device attributes Date: Wed, 18 Oct 2023 17:47:43 +0200 Message-ID: <20231018154804.420823-2-atenart@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231018154804.420823-1-atenart@kernel.org> References: <20231018154804.420823-1-atenart@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org X-Patchwork-State: RFC We have an ABBA deadlock between net device unregistration and sysfs files being accessed[1][2]. To prevent this from happening all paths taking the rtnl lock after the sysfs one (actually kn->active refcount) use rtnl_trylock and return early (using restart_syscall)[3] which can make syscalls to spin for a long time when there is contention on the rtnl lock[4]. There are not many possibilities to improve the above: - Rework the entire net/ locking logic. - Invert two locks in one of the paths — not possible. But here it's actually possible to drop one of the locks safely: the kernfs_node refcount. More details in the code itself, which comes with lots of comments. [1] https://lore.kernel.org/netdev/49A4D5D5.5090602@trash.net/ [2] https://lore.kernel.org/netdev/m14oyhis31.fsf@fess.ebiederm.org/ [3] https://lore.kernel.org/netdev/20090226084924.16cb3e08@nehalam/ [4] https://lore.kernel.org/all/20210928125500.167943-1-atenart@kernel.org/T/ Signed-off-by: Antoine Tenart --- net/core/net-sysfs.c | 142 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 112 insertions(+), 30 deletions(-) diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index fccaa5bac0ed..76d8729340b7 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -40,6 +40,88 @@ static inline int dev_isalive(const struct net_device *dev) return dev->reg_state <= NETREG_REGISTERED; } +/* We have a possible ABBA deadlock between rtnl_lock and kernfs_node->active, + * when unregistering a net device and accessing associated sysfs files. Tx/Rx + * queues do embed their own kobject for their sysfs files but the same issue + * applies as a net device being unresgistered will remove those sysfs files as + * well. The potential deadlock is as follow: + * + * CPU 0 CPU 1 + * + * rtnl_lock vfs_read + * unregister_netdevice_many kernfs_seq_start + * device_del / kobject_put kernfs_get_active (kn->active++) + * kernfs_drain sysfs_kf_seq_show + * wait_event( rtnl_lock + * kn->active == KN_DEACTIVATED_BIAS) -> waits on CPU 0 to release + * -> waits on CPU 1 to decrease kn->active the rtnl lock. + * + * The historical fix was to use rtnl_trylock with restart_syscall to bail out + * of sysfs accesses when the lock wasn't taken. This fixed the above issue as + * it allowed CPU 1 to bail out of the ABBA situation. + * + * But this comes with performances issues, as syscalls are being restarted in + * loops when there is contention, with huge slow downs in specific scenarios + * (e.g. lots of virtual interfaces created at startup and userspace daemons + * querying their attributes). + * + * The idea below is to bail out of the active kernfs_node protection + * (kn->active) while still being in a safe position to continue the execution + * of our sysfs helpers. + */ +static inline struct kernfs_node *sysfs_rtnl_lock(struct kobject *kobj, + struct attribute *attr, + struct net_device *ndev) +{ + struct kernfs_node *kn; + + /* First, we hold a reference to the net device we might use in the + * locking section as the unregistration path might run in parallel. + * This will ensure the net device won't be freed before we return. + */ + dev_hold(ndev); + /* sysfs_break_active_protection was introduced to allow self-removal of + * devices and their associated sysfs files by bailing out of the + * sysfs/kernfs protection. We do this here to allow the unregistration + * path to complete in parallel. The following takes a reference on the + * kobject and the kernfs_node being accessed. + * + * This works because we hold a reference onto the net device and the + * unregistration path will wait for us eventually in netdev_run_todo + * (outside an rtnl lock section). + */ + kn = sysfs_break_active_protection(kobj, attr); + WARN_ON_ONCE(!kn); + /* Finally we do take the rtnl lock. This can't deadlock us as the + * unregistration path will be able to drain sysfs files (kernfs_node). + */ + rtnl_lock(); + return kn; +} + +static inline void __sysfs_rtnl_unlock(struct net_device *ndev, + struct kernfs_node *kn) +{ + rtnl_unlock(); + /* This will drop our references on the kobject and kernfs_node. A + * limitation is we can't have the sysfs removal logic depend on the + * kobject reference counting, we have to do this manually in our + * unregistration paths. + */ + if (kn) + sysfs_unbreak_active_protection(kn); +} + +static inline void sysfs_rtnl_unlock(struct net_device *ndev, + struct kernfs_node *kn) +{ + __sysfs_rtnl_unlock(ndev, kn); + /* Might unlock the last unregistration path step. It's not safe to + * access the net device after this. + */ + dev_put(ndev); +} + /* use same locking rules as GIF* ioctl's */ static ssize_t netdev_show(const struct device *dev, struct device_attribute *attr, char *buf, @@ -83,6 +165,7 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr, { struct net_device *netdev = to_net_dev(dev); struct net *net = dev_net(netdev); + struct kernfs_node *kn; unsigned long new; int ret; @@ -93,15 +176,14 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr, if (ret) goto err; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) { ret = (*set)(netdev, new); if (ret == 0) ret = len; } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); err: return ret; } @@ -181,7 +263,7 @@ static ssize_t carrier_store(struct device *dev, struct device_attribute *attr, struct net_device *netdev = to_net_dev(dev); /* The check is also done in change_carrier; this helps returning early - * without hitting the trylock/restart in netdev_store. + * without hitting the locking section in netdev_store. */ if (!netdev->netdev_ops->ndo_change_carrier) return -EOPNOTSUPP; @@ -205,16 +287,16 @@ static ssize_t speed_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; int ret = -EINVAL; /* The check is also done in __ethtool_get_link_ksettings; this helps - * returning early without hitting the trylock/restart below. + * returning early without hitting the locking section below. */ if (!netdev->ethtool_ops->get_link_ksettings) return ret; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (netif_running(netdev) && netif_device_present(netdev)) { struct ethtool_link_ksettings cmd; @@ -222,7 +304,7 @@ static ssize_t speed_show(struct device *dev, if (!__ethtool_get_link_ksettings(netdev, &cmd)) ret = sysfs_emit(buf, fmt_dec, cmd.base.speed); } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } static DEVICE_ATTR_RO(speed); @@ -231,16 +313,16 @@ static ssize_t duplex_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; int ret = -EINVAL; /* The check is also done in __ethtool_get_link_ksettings; this helps - * returning early without hitting the trylock/restart below. + * returning early without hitting the locking section below. */ if (!netdev->ethtool_ops->get_link_ksettings) return ret; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (netif_running(netdev)) { struct ethtool_link_ksettings cmd; @@ -262,7 +344,7 @@ static ssize_t duplex_show(struct device *dev, ret = sysfs_emit(buf, "%s\n", duplex); } } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } static DEVICE_ATTR_RO(duplex); @@ -428,6 +510,7 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr, { struct net_device *netdev = to_net_dev(dev); struct net *net = dev_net(netdev); + struct kernfs_node *kn; size_t count = len; ssize_t ret = 0; @@ -438,8 +521,7 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr, if (len > 0 && buf[len - 1] == '\n') --count; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) { ret = dev_set_alias(netdev, buf, count); @@ -449,7 +531,7 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr, netdev_state_change(netdev); } err: - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } @@ -499,16 +581,16 @@ static ssize_t phys_port_id_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; ssize_t ret = -EINVAL; /* The check is also done in dev_get_phys_port_id; this helps returning - * early without hitting the trylock/restart below. + * early without hitting the locking section below. */ if (!netdev->netdev_ops->ndo_get_phys_port_id) return -EOPNOTSUPP; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) { struct netdev_phys_item_id ppid; @@ -517,7 +599,7 @@ static ssize_t phys_port_id_show(struct device *dev, if (!ret) ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id); } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } @@ -527,17 +609,17 @@ static ssize_t phys_port_name_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; ssize_t ret = -EINVAL; /* The checks are also done in dev_get_phys_port_name; this helps - * returning early without hitting the trylock/restart below. + * returning early without hitting the locking section below. */ if (!netdev->netdev_ops->ndo_get_phys_port_name && !netdev->devlink_port) return -EOPNOTSUPP; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) { char name[IFNAMSIZ]; @@ -546,7 +628,7 @@ static ssize_t phys_port_name_show(struct device *dev, if (!ret) ret = sysfs_emit(buf, "%s\n", name); } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } @@ -556,18 +638,18 @@ static ssize_t phys_switch_id_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; ssize_t ret = -EINVAL; /* The checks are also done in dev_get_phys_port_name; this helps - * returning early without hitting the trylock/restart below. This works + * returning early without hitting the locking section below. This works * because recurse is false when calling dev_get_port_parent_id. */ if (!netdev->netdev_ops->ndo_get_port_parent_id && !netdev->devlink_port) return -EOPNOTSUPP; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) { struct netdev_phys_item_id ppid = { }; @@ -576,7 +658,7 @@ static ssize_t phys_switch_id_show(struct device *dev, if (!ret) ret = sysfs_emit(buf, "%*phN\n", ppid.id_len, ppid.id); } - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } @@ -586,15 +668,15 @@ static ssize_t threaded_show(struct device *dev, struct device_attribute *attr, char *buf) { struct net_device *netdev = to_net_dev(dev); + struct kernfs_node *kn; ssize_t ret = -EINVAL; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev); if (dev_isalive(netdev)) ret = sysfs_emit(buf, fmt_dec, netdev->threaded); - rtnl_unlock(); + sysfs_rtnl_unlock(netdev, kn); return ret; } From patchwork Wed Oct 18 15:47:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antoine Tenart X-Patchwork-Id: 13427272 X-Patchwork-Delegate: kuba@kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 91E94BE62 for ; Wed, 18 Oct 2023 15:48:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="KCcOTB1Q" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E529BC433C9; Wed, 18 Oct 2023 15:48:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697644093; bh=8VrO2XK6EPwuECvMc1tWMEmYMKy93Q5HNAycs9pP93s=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=KCcOTB1QYCXzh/jiRSbGrVTUij8t4W14ORWbFP6JYNmc+cPC7SwHl3YfuWtjiUIUq MkKCP51lXkrC10AqA5s/UkLlgRi2W3E6oq6DOWv5LleswHInt9z7nNxJsam3h2Y37l J66Ql33hxWwkiO1TBSvNr9CdwVDuxB4f4Ivx0M6BLgWcOnt7i/fofZOXRu4XUg7dbw +yINFhlQzy/ndRE2V2516N6wRcKyKLsYk/1e/P6GJ6hXecmScPk7SwiqdGIMWsPRGs ogFhCevNZsaLj/3fmwhdKGzLzid2AdmMtix+b0qhzsMpErx2rg7e45WkJ8l7Rqmf6S mTqmMmdUz4LBA== From: Antoine Tenart To: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, edumazet@google.com Cc: Antoine Tenart , netdev@vger.kernel.org, gregkh@linuxfoundation.org, mhocko@suse.com, stephen@networkplumber.org Subject: [RFC PATCH net-next 2/4] net-sysfs: move queue attribute groups outside the default groups Date: Wed, 18 Oct 2023 17:47:44 +0200 Message-ID: <20231018154804.420823-3-atenart@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231018154804.420823-1-atenart@kernel.org> References: <20231018154804.420823-1-atenart@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org X-Patchwork-State: RFC Rx/tx queues embed their own kobject for registering their per-queue sysfs files. The issue is they're using the kobject default groups for this and entirely rely on the kobject refcounting for releasing their sysfs paths. In order to remove rtnl_trylock calls we need sysfs files not to rely on their associated kobject refcounting for their release. Thus we here move queues sysfs files from the kobject default groups to their own groups which can be removed separately. Signed-off-by: Antoine Tenart --- include/linux/netdevice.h | 1 + include/net/netdev_rx_queue.h | 1 + net/core/net-sysfs.c | 27 ++++++++++++++++++++++----- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 1c7681263d30..90fa1aa93155 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -626,6 +626,7 @@ struct netdev_queue { struct Qdisc __rcu *qdisc_sleeping; #ifdef CONFIG_SYSFS struct kobject kobj; + const struct attribute_group **groups; #endif #if defined(CONFIG_XPS) && defined(CONFIG_NUMA) int numa_node; diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h index cdcafb30d437..b77bbc027176 100644 --- a/include/net/netdev_rx_queue.h +++ b/include/net/netdev_rx_queue.h @@ -15,6 +15,7 @@ struct netdev_rx_queue { struct rps_dev_flow_table __rcu *rps_flow_table; #endif struct kobject kobj; + const struct attribute_group **groups; struct net_device *dev; netdevice_tracker dev_tracker; diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 76d8729340b7..a9f712ef9925 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1138,7 +1138,6 @@ static void rx_queue_get_ownership(const struct kobject *kobj, static const struct kobj_type rx_queue_ktype = { .sysfs_ops = &rx_queue_sysfs_ops, .release = rx_queue_release, - .default_groups = rx_queue_default_groups, .namespace = rx_queue_namespace, .get_ownership = rx_queue_get_ownership, }; @@ -1172,10 +1171,15 @@ static int rx_queue_add_kobject(struct net_device *dev, int index) if (error) goto err; + queue->groups = rx_queue_default_groups; + error = sysfs_create_groups(kobj, queue->groups); + if (error) + goto err; + if (dev->sysfs_rx_queue_group) { error = sysfs_create_group(kobj, dev->sysfs_rx_queue_group); if (error) - goto err; + goto err_default_groups; } error = rx_queue_default_mask(dev, queue); @@ -1186,6 +1190,8 @@ static int rx_queue_add_kobject(struct net_device *dev, int index) return error; +err_default_groups: + sysfs_remove_groups(kobj, queue->groups); err: kobject_put(kobj); return error; @@ -1230,12 +1236,14 @@ net_rx_queue_update_kobjects(struct net_device *dev, int old_num, int new_num) } while (--i >= new_num) { - struct kobject *kobj = &dev->_rx[i].kobj; + struct netdev_rx_queue *queue = &dev->_rx[i]; + struct kobject *kobj = &queue->kobj; if (!refcount_read(&dev_net(dev)->ns.count)) kobj->uevent_suppress = 1; if (dev->sysfs_rx_queue_group) sysfs_remove_group(kobj, dev->sysfs_rx_queue_group); + sysfs_remove_groups(kobj, queue->groups); kobject_put(kobj); } @@ -1757,7 +1765,6 @@ static void netdev_queue_get_ownership(const struct kobject *kobj, static const struct kobj_type netdev_queue_ktype = { .sysfs_ops = &netdev_queue_sysfs_ops, .release = netdev_queue_release, - .default_groups = netdev_queue_default_groups, .namespace = netdev_queue_namespace, .get_ownership = netdev_queue_get_ownership, }; @@ -1779,15 +1786,24 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index) if (error) goto err; + queue->groups = netdev_queue_default_groups; + error = sysfs_create_groups(kobj, queue->groups); + if (error) + goto err; + #ifdef CONFIG_BQL error = sysfs_create_group(kobj, &dql_group); if (error) - goto err; + goto err_default_groups; #endif kobject_uevent(kobj, KOBJ_ADD); return 0; +#ifdef CONFIG_BQL +err_default_groups: + sysfs_remove_groups(kobj, queue->groups); +#endif err: kobject_put(kobj); return error; @@ -1841,6 +1857,7 @@ netdev_queue_update_kobjects(struct net_device *dev, int old_num, int new_num) #ifdef CONFIG_BQL sysfs_remove_group(&queue->kobj, &dql_group); #endif + sysfs_remove_groups(&queue->kobj, queue->groups); kobject_put(&queue->kobj); } From patchwork Wed Oct 18 15:47:45 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antoine Tenart X-Patchwork-Id: 13427273 X-Patchwork-Delegate: kuba@kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 31D5BBE62 for ; Wed, 18 Oct 2023 15:48:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="m8vH//zg" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AACB7C433C7; Wed, 18 Oct 2023 15:48:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697644096; bh=JdMJ8//57g6EuQRyejekxR37eWJ667og6tFzCb1Tjs4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=m8vH//zgJ/qM/m5pTeog8op6aIaWM1UQqur/ZlNj31f6oDIn+1U665lyxWQWnfrX/ erSrELGyjH6u6iJVVqXg8AFwgQgWklm6j7CoAbsTpgs2pjez0isA2Bhpity5dxj7lm PRy5qa1BSCoQvs9I9NHepcrAmtccpGnNGPb6e32pcRlWcCtMCa2Ca2lhfOLzW1lU2q H2l+mSMXGvqywRI0HERl9Gp3l11ZNZmlkWgat8ntmd6//i3hrCDGovHvNr9UJJVQ7g 35ampbKogDsLycZRwpxpveWdtTzdcuK82jm0vZ7mVYOHwQzPDF6yyuhLmXUMxXYavi VFMcfd5WX+zYw== From: Antoine Tenart To: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, edumazet@google.com Cc: Antoine Tenart , netdev@vger.kernel.org, gregkh@linuxfoundation.org, mhocko@suse.com, stephen@networkplumber.org Subject: [RFC PATCH net-next 3/4] net-sysfs: prevent uncleared queues from being re-added Date: Wed, 18 Oct 2023 17:47:45 +0200 Message-ID: <20231018154804.420823-4-atenart@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231018154804.420823-1-atenart@kernel.org> References: <20231018154804.420823-1-atenart@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org X-Patchwork-State: RFC With the (upcoming) removal of the rtnl_trylock/restart_syscall logic and because of how Tx/Rx queues are implemented (and their requirements), it might happen that a queue is re-added before having the chance to be cleared. In such rare case, do not complete the queue addition operation. Signed-off-by: Antoine Tenart --- net/core/net-sysfs.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index a9f712ef9925..75fb92c44291 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1160,6 +1160,20 @@ static int rx_queue_add_kobject(struct net_device *dev, int index) struct kobject *kobj = &queue->kobj; int error = 0; + /* Rx queues are cleared in rx_queue_release to allow later + * re-registration. This is triggered when their kobj refcount is + * dropped. + * + * If a queue is removed while both a read (or write) operation and a + * the re-addition of the same queue are pending (waiting on rntl_lock) + * it might happen that the re-addition will execute before the read, + * making the initial removal to never happen (queue's kobj refcount + * won't drop enough because of the pending read). In such rare case, + * return to allow the removal operation to complete. + */ + if (unlikely(kobj->state_initialized)) + return -EAGAIN; + /* Kobject_put later will trigger rx_queue_release call which * decreases dev refcount: Take that reference here */ @@ -1775,6 +1789,20 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index) struct kobject *kobj = &queue->kobj; int error = 0; + /* Tx queues are cleared in netdev_queue_release to allow later + * re-registration. This is triggered when their kobj refcount is + * dropped. + * + * If a queue is removed while both a read (or write) operation and a + * the re-addition of the same queue are pending (waiting on rntl_lock) + * it might happen that the re-addition will execute before the read, + * making the initial removal to never happen (queue's kobj refcount + * won't drop enough because of the pending read). In such rare case, + * return to allow the removal operation to complete. + */ + if (unlikely(kobj->state_initialized)) + return -EAGAIN; + /* Kobject_put later will trigger netdev_queue_release call * which decreases dev refcount: Take that reference here */ From patchwork Wed Oct 18 15:47:46 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antoine Tenart X-Patchwork-Id: 13427274 X-Patchwork-Delegate: kuba@kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 2DF15BE62 for ; Wed, 18 Oct 2023 15:48:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cxBPNxnb" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7D937C433C8; Wed, 18 Oct 2023 15:48:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697644099; bh=FAdcenIHw35N/BQIfshjMUdQiCYcYcQAoKRQgmuaMtA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=cxBPNxnbnSxW3+zQ1eENrVVN43rl1x8XlEoN+rRRy3r1qkOdklYEiulcivOCX7jvA 1tw0sVHyuQO7ZaDOobm4A2r7vAyMjTnwnbf7GU/UHOGYQqJUcG5olNGJfi8+V5jnvP p8b9DWz56y9Po0Yu9APoMk/3n+zRB3DtXZvC4oMpx/sY77Zgs92PaRKYeU1ks+3RQQ IVCHi+8DFEZ5h1e1BUZYwvEmfo8vd4jE/DZ3ErB5NKfDSHQhP8iiFbPBkHhhNBpYvJ ItnRzs+EpJ01oIIoDRegEmVr7XiZBsVPZ7cSMLY4OkzwUS5Ektd0p59vt1SUFfjvFH QV0/+lKjCzM9A== From: Antoine Tenart To: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, edumazet@google.com Cc: Antoine Tenart , netdev@vger.kernel.org, gregkh@linuxfoundation.org, mhocko@suse.com, stephen@networkplumber.org Subject: [RFC PATCH net-next 4/4] net-sysfs: remove rtnl_trylock from queue attributes Date: Wed, 18 Oct 2023 17:47:46 +0200 Message-ID: <20231018154804.420823-5-atenart@kernel.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231018154804.420823-1-atenart@kernel.org> References: <20231018154804.420823-1-atenart@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Patchwork-Delegate: kuba@kernel.org X-Patchwork-State: RFC Similar to the commit removing remove rtnl_trylock from device attributes we here apply the same technique to networking queues. Signed-off-by: Antoine Tenart --- net/core/net-sysfs.c | 132 ++++++++++++++++++++++++------------------- 1 file changed, 73 insertions(+), 59 deletions(-) diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index 75fb92c44291..1ef93a3ef4a5 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -1296,9 +1296,11 @@ static int net_rx_queue_change_owner(struct net_device *dev, int num, */ struct netdev_queue_attribute { struct attribute attr; - ssize_t (*show)(struct netdev_queue *queue, char *buf); - ssize_t (*store)(struct netdev_queue *queue, - const char *buf, size_t len); + ssize_t (*show)(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf); + ssize_t (*store)(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, const char *buf, + size_t len); }; #define to_netdev_queue_attr(_attr) \ container_of(_attr, struct netdev_queue_attribute, attr) @@ -1315,7 +1317,7 @@ static ssize_t netdev_queue_attr_show(struct kobject *kobj, if (!attribute->show) return -EIO; - return attribute->show(queue, buf); + return attribute->show(kobj, attr, queue, buf); } static ssize_t netdev_queue_attr_store(struct kobject *kobj, @@ -1329,7 +1331,7 @@ static ssize_t netdev_queue_attr_store(struct kobject *kobj, if (!attribute->store) return -EIO; - return attribute->store(queue, buf, count); + return attribute->store(kobj, attr, queue, buf, count); } static const struct sysfs_ops netdev_queue_sysfs_ops = { @@ -1337,7 +1339,8 @@ static const struct sysfs_ops netdev_queue_sysfs_ops = { .store = netdev_queue_attr_store, }; -static ssize_t tx_timeout_show(struct netdev_queue *queue, char *buf) +static ssize_t tx_timeout_show(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { unsigned long trans_timeout = atomic_long_read(&queue->trans_timeout); @@ -1355,18 +1358,18 @@ static unsigned int get_netdev_queue_index(struct netdev_queue *queue) return i; } -static ssize_t traffic_class_show(struct netdev_queue *queue, - char *buf) +static ssize_t traffic_class_show(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { struct net_device *dev = queue->dev; + struct kernfs_node *kn; int num_tc, tc; int index; if (!netif_is_multiqueue(dev)) return -ENOENT; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(kobj, attr, queue->dev); index = get_netdev_queue_index(queue); @@ -1376,7 +1379,7 @@ static ssize_t traffic_class_show(struct netdev_queue *queue, num_tc = dev->num_tc; tc = netdev_txq_to_tc(dev, index); - rtnl_unlock(); + sysfs_rtnl_unlock(queue->dev, kn); if (tc < 0) return -EINVAL; @@ -1393,24 +1396,26 @@ static ssize_t traffic_class_show(struct netdev_queue *queue, } #ifdef CONFIG_XPS -static ssize_t tx_maxrate_show(struct netdev_queue *queue, - char *buf) +static ssize_t tx_maxrate_show(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { return sysfs_emit(buf, "%lu\n", queue->tx_maxrate); } -static ssize_t tx_maxrate_store(struct netdev_queue *queue, - const char *buf, size_t len) +static ssize_t tx_maxrate_store(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, const char *buf, + size_t len) { - struct net_device *dev = queue->dev; int err, index = get_netdev_queue_index(queue); + struct net_device *dev = queue->dev; + struct kernfs_node *kn; u32 rate = 0; if (!capable(CAP_NET_ADMIN)) return -EPERM; /* The check is also done later; this helps returning early without - * hitting the trylock/restart below. + * hitting the locking section below. */ if (!dev->netdev_ops->ndo_set_tx_maxrate) return -EOPNOTSUPP; @@ -1419,18 +1424,19 @@ static ssize_t tx_maxrate_store(struct netdev_queue *queue, if (err < 0) return err; - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(kobj, attr, dev); err = -EOPNOTSUPP; if (dev->netdev_ops->ndo_set_tx_maxrate) err = dev->netdev_ops->ndo_set_tx_maxrate(dev, index, rate); - rtnl_unlock(); if (!err) { queue->tx_maxrate = rate; + sysfs_rtnl_unlock(dev, kn); return len; } + + sysfs_rtnl_unlock(dev, kn); return err; } @@ -1474,16 +1480,17 @@ static ssize_t bql_set(const char *buf, const size_t count, return count; } -static ssize_t bql_show_hold_time(struct netdev_queue *queue, - char *buf) +static ssize_t bql_show_hold_time(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { struct dql *dql = &queue->dql; return sysfs_emit(buf, "%u\n", jiffies_to_msecs(dql->slack_hold_time)); } -static ssize_t bql_set_hold_time(struct netdev_queue *queue, - const char *buf, size_t len) +static ssize_t bql_set_hold_time(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, const char *buf, + size_t len) { struct dql *dql = &queue->dql; unsigned int value; @@ -1502,8 +1509,8 @@ static struct netdev_queue_attribute bql_hold_time_attribute __ro_after_init = __ATTR(hold_time, 0644, bql_show_hold_time, bql_set_hold_time); -static ssize_t bql_show_inflight(struct netdev_queue *queue, - char *buf) +static ssize_t bql_show_inflight(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { struct dql *dql = &queue->dql; @@ -1514,13 +1521,16 @@ static struct netdev_queue_attribute bql_inflight_attribute __ro_after_init = __ATTR(inflight, 0444, bql_show_inflight, NULL); #define BQL_ATTR(NAME, FIELD) \ -static ssize_t bql_show_ ## NAME(struct netdev_queue *queue, \ - char *buf) \ +static ssize_t bql_show_ ## NAME(struct kobject *kobj, \ + struct attribute *attr, \ + struct netdev_queue *queue, char *buf) \ { \ return bql_show(buf, queue->dql.FIELD); \ } \ \ -static ssize_t bql_set_ ## NAME(struct netdev_queue *queue, \ +static ssize_t bql_set_ ## NAME(struct kobject *kobj, \ + struct attribute *attr, \ + struct netdev_queue *queue, \ const char *buf, size_t len) \ { \ return bql_set(buf, len, &queue->dql.FIELD); \ @@ -1600,9 +1610,11 @@ static ssize_t xps_queue_show(struct net_device *dev, unsigned int index, return len < PAGE_SIZE ? len : -EINVAL; } -static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf) +static ssize_t xps_cpus_show(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { struct net_device *dev = queue->dev; + struct kernfs_node *kn; unsigned int index; int len, tc; @@ -1611,32 +1623,34 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf) index = get_netdev_queue_index(queue); - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(kobj, attr, queue->dev); /* If queue belongs to subordinate dev use its map */ dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev; tc = netdev_txq_to_tc(dev, index); if (tc < 0) { - rtnl_unlock(); + sysfs_rtnl_unlock(queue->dev, kn); return -EINVAL; } - /* Make sure the subordinate device can't be freed */ - get_device(&dev->dev); - rtnl_unlock(); + /* Do not release the net device refcnt to make sure it won't be freed + * while xps_queue_show is running. + */ + __sysfs_rtnl_unlock(queue->dev, kn); len = xps_queue_show(dev, index, tc, buf, XPS_CPUS); - put_device(&dev->dev); + dev_put(dev); return len; } -static ssize_t xps_cpus_store(struct netdev_queue *queue, - const char *buf, size_t len) +static ssize_t xps_cpus_store(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, const char *buf, + size_t len) { struct net_device *dev = queue->dev; + struct kernfs_node *kn; unsigned int index; cpumask_var_t mask; int err; @@ -1658,13 +1672,9 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue, return err; } - if (!rtnl_trylock()) { - free_cpumask_var(mask); - return restart_syscall(); - } - + kn = sysfs_rtnl_lock(kobj, attr, dev); err = netif_set_xps_queue(dev, mask, index); - rtnl_unlock(); + sysfs_rtnl_unlock(dev, kn); free_cpumask_var(mask); @@ -1674,30 +1684,37 @@ static ssize_t xps_cpus_store(struct netdev_queue *queue, static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init = __ATTR_RW(xps_cpus); -static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf) +static ssize_t xps_rxqs_show(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, char *buf) { struct net_device *dev = queue->dev; + struct kernfs_node *kn; unsigned int index; - int tc; + int tc, ret; index = get_netdev_queue_index(queue); - if (!rtnl_trylock()) - return restart_syscall(); + kn = sysfs_rtnl_lock(kobj, attr, dev); tc = netdev_txq_to_tc(dev, index); - rtnl_unlock(); - if (tc < 0) - return -EINVAL; - return xps_queue_show(dev, index, tc, buf, XPS_RXQS); + /* Do not release the net device refcnt to make sure it won't be freed + * while xps_queue_show is running. + */ + __sysfs_rtnl_unlock(dev, kn); + + ret = tc >= 0 ? xps_queue_show(dev, index, tc, buf, XPS_RXQS) : -EINVAL; + dev_put(dev); + return ret; } -static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf, +static ssize_t xps_rxqs_store(struct kobject *kobj, struct attribute *attr, + struct netdev_queue *queue, const char *buf, size_t len) { struct net_device *dev = queue->dev; struct net *net = dev_net(dev); + struct kernfs_node *kn; unsigned long *mask; unsigned int index; int err; @@ -1717,16 +1734,13 @@ static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf, return err; } - if (!rtnl_trylock()) { - bitmap_free(mask); - return restart_syscall(); - } + kn = sysfs_rtnl_lock(kobj, attr, dev); cpus_read_lock(); err = __netif_set_xps_queue(dev, mask, index, XPS_RXQS); cpus_read_unlock(); - rtnl_unlock(); + sysfs_rtnl_unlock(dev, kn); bitmap_free(mask); return err ? : len;