Received-SPF: pass (google.com: domain of linux-kernel+bounces-45292-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 30 Jan 2024 12:47:19 -0800
From: Luis Chamberlain <mcgrof@kernel.org>
To: Marco Pagani <marpagan@redhat.com>
Cc: linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] kernel/module: add a safer implementation of
 try_module_get()
Message-ID: <ZblgV0ApD-9cQWwl@bombadil.infradead.org>
References: <20240130193614.49772-1-marpagan@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130193614.49772-1-marpagan@redhat.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>

On Tue, Jan 30, 2024 at 08:36:14PM +0100, Marco Pagani wrote:
> The current implementation of try_module_get() requires the module to
> exist and be live as a precondition. While this may seem intuitive at
> first glance, enforcing the precondition can be tricky, considering that
> modules can be unloaded at any time if not previously taken. For
> instance, the caller could be preempted just before calling
> try_module_get(), and while preempted, the module could be unloaded and
> freed. More subtly, the module could also be unloaded at any point while
> executing try_module_get() before incrementing the refount with
> atomic_inc_not_zero().
> 
> Neglecting the precondition that the module must exist and be live can
> cause unexpected race conditions that can lead to crashes. However,
> ensuring that the precondition is met may require additional locking
> that increases the complexity of the code and can make it more
> error-prone.
> 
> This patch adds a slower yet safer implementation of try_module_get()
> that checks if the module is valid by looking into the mod_tree before
> taking the module's refcount. This new function can be safely called on
> stale and invalid module pointers, relieving developers from the burden
> of ensuring that the module exists and is live before attempting to take
> it.
> 
> The tree lookup and refcount increment are executed after taking the
> module_mutex to prevent the module from being unloaded after looking up
> the tree.
> 
> Signed-off-by: Marco Pagani <marpagan@redhat.com>

It very much sounds like there is a desire to have this but without a
user, there is no justification.

> +bool try_module_get_safe(struct module *module)
> +{
> +	struct module *mod;
> +	bool ret = true;
> +
> +	if (!module)
> +		goto out;
> +
> +	mutex_lock(&module_mutex);

If a user comes around then this should be mutex_lock_interruptible(),
and add might_sleep()

> +
> +	/*
> +	 * Check if the address points to a valid live module and take
> +	 * the refcount only if it points to the module struct.
> +	 */
> +	mod = __module_address((unsigned long)module);
> +	if (mod && mod == module && module_is_live(mod))
> +		__module_get(mod);
> +	else
> +		ret = false;
> +
> +	mutex_unlock(&module_mutex);
> +
> +out:
> +	return ret;
> +}
> +EXPORT_SYMBOL(try_module_get_safe);

And EXPORT_SYMBOL_GPL() would need to be used.

I'd also expect selftests to be expanded for this case, but again,
without a user, this is just trying to resolve a problem which does not
exist.

  Luis