netdev - Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y7L0Qu/fy2QbTFpw@nanopsycho>
Date:   Mon, 2 Jan 2023 16:12:02 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     jacob.e.keller@...el.com, leon@...nel.org, netdev@...r.kernel.org
Subject: Re: [RFC net-next 04/10] devlink: always check if the devlink
 instance is registered

Mon, Jan 02, 2023 at 03:57:24PM CET, jiri@...nulli.us wrote:
>Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@...nel.org wrote:
>>Always check under the instance lock whether the devlink instance
>>is still / already registered.
>>
>>This is a no-op for the most part, as the unregistration path currently
>>waits for all references. On the init path, however, we may temporarily
>>open up a race with netdev code, if netdevs are registered before the
>>devlink instance. This is temporary, the next change fixes it, and this
>>commit has been split out for the ease of review.
>>
>>Note that in case of iterating over sub-objects which have their
>>own lock (regions and line cards) we assume an implicit dependency
>>between those objects existing and devlink unregistration.
>>
>>Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>>---
>> include/net/devlink.h |  1 +
>> net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
>> net/devlink/core.c    | 25 +++++++++++++++++++++----
>> net/devlink/netlink.c | 10 ++++++++--
>> 4 files changed, 59 insertions(+), 12 deletions(-)
>>
>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>index 6a2e4f21779f..36e013d3aa52 100644
>>--- a/include/net/devlink.h
>>+++ b/include/net/devlink.h
>>@@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
>> void devl_lock(struct devlink *devlink);
>> int devl_trylock(struct devlink *devlink);
>> void devl_unlock(struct devlink *devlink);
>>+bool devl_is_alive(struct devlink *devlink);
>> void devl_assert_locked(struct devlink *devlink);
>> bool devl_lock_is_held(struct devlink *devlink);
>> 
>>diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>>index 5f33d74eef83..6b18e70a39fd 100644
>>--- a/net/devlink/basic.c
>>+++ b/net/devlink/basic.c
>>@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
>> 		int idx = 0;
>> 
>> 		mutex_lock(&devlink->linecards_lock);
>>+		if (!devl_is_alive(devlink))
>>+			goto next_devlink;
>
>
>Thinking about this a bit more, things would be cleaner if reporters and
>linecards are converted to rely on instance lock as well. I don't see a
>good reason for a separate lock in both cases, really.
>
>Also, we could introduce devlinks_xa_for_each_registered_get_lock()
>iterator that would lock the instance as well right away to avoid
>this devl_is_alive() dance on multiple places when you iterate devlinks.

devlinks_xa_find_get_locked()
would do the check&lock at the end.



>
>
>>+
>> 		list_for_each_entry(linecard, &devlink->linecard_list, list) {
>> 			if (idx < dump->idx) {
>> 				idx++;

[...]