linux-kernel - Re: [PATCH v2] Documentation: livepatch: Add livepatch API page

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]

Date:   Thu, 16 Dec 2021 10:57:04 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     David Vernet <void@...ifault.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, jpoimboe@...hat.com, jikos@...nel.org,
        mbenes@...e.cz, joe.lawrence@...hat.com, corbet@....net
Subject: Re: [PATCH v2] Documentation: livepatch: Add livepatch API page

On Wed 2021-12-15 09:47:00, David Vernet wrote:
> The livepatch subsystem has several exported functions and objects with
> kerneldoc comments. Though the livepatch documentation contains
> handwritten descriptions of all of these exported functions, they are
> currently not pulled into the docs build using the kernel-doc directive.
> 
> Note that all of the handwritten API descriptions were left alone with
> the exception of Documentation/livepatch/system-state.rst, which was
> updated to allow the cross-referencing to work correctly. The file now
> follows the cross-referencing formatting guidance specified in
> Documentation/doc-guide/kernel-doc.rst. Furthermore, some comments
> around klp_shadow_free_all() were updated to say <obj, id> rather than
> <*, id> to match the rest of the file, and to prevent the docs build
> from emitting an "Inline emphasis start-string without end string"
> error.
> 
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -272,12 +272,12 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
>  EXPORT_SYMBOL_GPL(klp_shadow_free);
>  
>  /**
> - * klp_shadow_free_all() - detach and free all <*, id> shadow variables
> + * klp_shadow_free_all() - detach and free all <obj, id> shadow variables

This change is not good. The function releases all existing shadow
variables with the given @id for any @obj. And it is not longer clear.

I guess that the primary motivation was to remove  "Inline emphasis
start-string without end string" mentioned in the commit message.

A solution would be replace '*' with something else, for example, < , id>.
Another solution would be to describe it another way, for example:

 * klp_shadow_free_all() - detach and free all <obj, id> shadow variables
 *		with the given @id.

>   * @id:		data identifier
>   * @dtor:	custom callback that can be used to unregister the variable
>   *		and/or free data that the shadow variable points to (optional)
>   *
> - * This function releases the memory for all <*, id> shadow variable
> + * This function releases the memory for all <obj, id> shadow variable

Same here.

>   * instances, callers should stop referencing them accordingly.
>   */
>  void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
> @@ -288,7 +288,7 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
>  
>  	spin_lock_irqsave(&klp_shadow_lock, flags);
>  
> -	/* Delete all <*, id> from hash */
> +	/* Delete all <obj, id> from hash */

and here

>  	hash_for_each(klp_shadow_hash, i, shadow, node) {
>  		if (klp_shadow_match(shadow, shadow->obj, id))
>  			klp_shadow_free_struct(shadow, dtor);

BTW: There is likely the same problem in Documentation/livepatch/shadow-vars.rst.
     I see <*, id> there as well.


Otherwise, the patch looks fine to me.

Best Regards,
Petr