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: <20230102150514.6321d2ae@kernel.org>
Date:   Mon, 2 Jan 2023 15:05:14 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
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

On Mon, 2 Jan 2023 14:58:16 +0100 Jiri Pirko 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.  
> 
> This would be probably very valuable to add as a comment inside the code
> for the future reader mind sake.

Where, tho?

I'm strongly against the pointlessly fine-grained locking going forward
so hopefully there won't be any more per-subobject locks added anyway.

> >+bool devl_is_alive(struct devlink *devlink)  
> 
> Why "alive"? To be consistent with the existing terminology, how about
> to name it devl_is_registered()?

I dislike the similarity to device_is_registered() which has very
different semantics. I prefer alive.

> Also, "devl_" implicates that it should be called with devlink instance
> lock held, so probably devlink_is_registered() would be better.

I'm guessing you realized this isn't correct later on.

> >+{
> >+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> >+}
> >+EXPORT_SYMBOL_GPL(devl_is_alive);
> >+
> >+/**
> >+ * devlink_try_get() - try to obtain a reference on a devlink instance
> >+ * @devlink: instance to reference
> >+ *
> >+ * Obtain a reference on a devlink instance. A reference on a devlink instance
> >+ * only implies that it's safe to take the instance lock. It does not imply
> >+ * that the instance is registered, use devl_is_alive() after taking
> >+ * the instance lock to check registration status.
> >+ */  
> 
> This comment is not related to the patch, should be added in a separate
> one.

The point of adding this comment is to say that one has to use
devl_is_alive() after accessing an instance by reference.
It is very much in the right patch.