Re: [PATCH v7 1/4] lib/vsprintf: Export functions for Rust pointer formatting support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Dec 29, 2025 at 03:21:19PM +0800, Ke Sun wrote:
> Export ptr_to_hashval() and extract kptr_restrict_value() to enable
> Rust code to use kernel pointer formatting functionality.
> 
> - Export ptr_to_hashval() with EXPORT_SYMBOL_GPL for pointer hashing.
>   This function is used to hash kernel pointers before printing them,
>   preventing information leaks about kernel memory layout.
> 
> - Extract kptr_restrict handling logic from restricted_pointer() into
>   a separate exported function kptr_restrict_value() that can be
>   reused by both restricted_pointer() (for %pK format specifier) and
>   Rust code.
> 
> The kptr_restrict_value() function:
> - Returns 0, 1, or 2 to indicate the kptr_restrict sysctl value
> - Returns -1 for error case (IRQ context with kptr_restrict==1)
> - Modifies the pointer value through @pptr parameter when needed
>   (sets to NULL when access should be restricted)
> 
> These functions are needed for the HashedPtr and RestrictedPtr wrapper
> types in rust/kernel/ptr.rs to provide safe pointer formatting that
> matches C kernel patterns.
> 
> This refactoring:
> - Allows restricted_pointer() to reuse the common logic while
>   maintaining the same behavior
> - Enables Rust code to use the same kptr_restrict handling logic
> - Reduces code duplication

...

> +EXPORT_SYMBOL_GPL(ptr_to_hashval);

I do not like this. At bare minimum we should make sure it's namespaced, so the
author of the code deliberately needs to import it (I'm talking about C side).

Ideally we should have something which is Rust specific, like

EXPORT_SYMBOL_FOR_RUST_ONLY(foo);

Can we do that first?

...

> +/**
> + * kptr_restrict_value - Determine what pointer value should be used for %pK
> + * @pptr: Pointer to the pointer value (may be modified)
> + *
> + * This function determines what pointer value should be printed based on the
> + * kptr_restrict sysctl setting:

> + * - kptr_restrict == 0: Return 0 (use original pointer, will be hashed)
> + * - kptr_restrict == 1: Return 1 if current process has CAP_SYSLOG and same
> + *                       euid/egid, otherwise set *pptr to NULL and return 1
> + * - kptr_restrict >= 2: Set *pptr to NULL and return 2

It's the usual practice to give a enum with self-explained definitions and use
them. plain numbers are prone to mistakes during the evolution of the code.
Also it's hard to read inside the code their semantic.

> + * Returns:
> + *   - 0, 1, or 2: The kptr_restrict value if pointer should be used
> + *   - -1: Error case (IRQ context with kptr_restrict==1), *pptr unchanged
> + */

...

> +		if (in_hardirq() || in_serving_softirq() || in_nmi())
> +			return -1;

Give a meaningful Linux error code for this.

...

> +static noinline_for_stack
> +char *restricted_pointer(char *buf, char *end, const void *ptr,
> +			 struct printf_spec spec)
> +{

> +	int ret = kptr_restrict_value(&ptr);
> +
> +	if (ret < 0) {
> +		if (spec.field_width == -1)
> +			spec.field_width = 2 * sizeof(ptr);
> +		return error_string(buf, end, "pK-error", spec);
> +	}

This pattern is harder to maintain, decouple definition and assignment to make
it better.

	int ret;

	ret = kptr_restrict_value(&ptr);
	if (ret < 0) {

> +	if (ret == 0)
> +		return default_pointer(buf, end, ptr, spec);
>  
>  	return pointer_string(buf, end, ptr, spec);
>  }

-- 
With Best Regards,
Andy Shevchenko