linux-hardening - Re: [PATCH] overflow: Fix kern-doc markup for functions

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] [day] [month] [year] [list]

Message-ID: <69fc1b11-d26b-de44-ebee-cbb17cc2c893@gmail.com>
Date:   Tue, 27 Sep 2022 19:35:55 +0900
From:   Akira Yokosawa <akiyks@...il.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Matthew Wilcox <willy@...radead.org>, linux-doc@...r.kernel.org,
        linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
        Akira Yokosawa <akiyks@...il.com>,
        Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] overflow: Fix kern-doc markup for functions

Hi,

On Mon, 26 Sep 2022 20:02:16 -0700, Kees Cook wrote:
> On Tue, Sep 27, 2022 at 11:53:38AM +0900, Akira Yokosawa wrote:
>> Hi,
>>
>> Somehow Kees added me in Cc:, so let me comment.  :-)
>>
>> On Mon, 26 Sep 2022 14:09:10 -0700, Kees Cook wrote:
>>> On Mon, Sep 26, 2022 at 10:06:19PM +0100, Matthew Wilcox wrote:
>>>> On Mon, Sep 26, 2022 at 12:47:13PM -0700, Kees Cook wrote:
>>>>> -/** check_add_overflow() - Calculate addition with overflow checking
>>>>> +/**
>>>>> + * check_add_overflow - Calculate addition with overflow checking
>>>>>   *
>>>>>   * @a: first addend
>>>>>   * @b: second addend
>>>>
>>>> Why did you remove the ()?  And why didn't you delete the blank line?
>>>> According to our documentation, the canonical form is:
>>>>
>>>>   /**
>>>>    * function_name() - Brief description of function.
>>>>    * @arg1: Describe the first argument.
>>>>    * @arg2: Describe the second argument.
>>>>    *        One can provide multiple line descriptions
>>>>    *        for arguments.
>>
>> Matthew, you call it the "canonical form", my take is more of a "template
>> that is known to work".
> 
> Out of curiosity, why is the trailing "()" part of the standard
> template? Isn't it redundant? Or is trying to help differentiate between
> things that are non-callable? (i.e. a variable, etc.)

I don't know the historic background of this template and I have done
some digging of Git history. I'm afraid this won't be straight answers
to your questions.

This template originated from commit 0842b245a8e6 ("doc: document the
kernel-doc conventions for kernel hackers") back in 2008 and is more
or less unchanged with later additions of "Context:" and "Return:" keywords.

As far as I can see, the kernel-doc script removes "()" in the early phase
of parsing kernel-doc comments. So yes, "()" is redundant.

Up until v5.17 (ever since the epoch of v2.6.12-rc2), there was a comment
block on the format of function comments in the kernel-doc script and there
was no mention of "()", as quoted below:

    # So .. the trivial example would be:
    #
    # /**
    #  * my_function
    #  */
    #
    # If the Description: header tag is omitted, then there must be a blank line
    # after the last parameter specification.
    # e.g.
    # /**
    #  * my_function - does my stuff
    #  * @my_arg: its mine damnit
    #  *
    #  * Does my stuff explained.
    #  */
    #
    #  or, could also use:
    # /**
    #  * my_function - does my stuff
    #  * @my_arg: its mine damnit
    #  * Description: Does my stuff explained.
    #  */
    # etc.

Which suggests "()" has always been redundant since early days.

(Note: Nowadays, the first example without brief description will cause
a warning.)

This comment block was removed in commit 258092a89085 ("scripts:
kernel-doc: Drop obsolete comments"). Its changelog says:

    4. The "format of comments" comment block
    
    As suggested by Jani Nikula in a reply to my first version of this
    transformation, Documentation/doc-guide/kernel-doc.rst can serve as the
    information hub for comment formatting. The section DESCRIPTION already
    points there, so the original comment block can just be removed.

It sounds to me like the removal was premature at that time, because some
of those format variations were (and still are) not properly covered in
kernel-doc.rst.

> 
>>> Hunh, everywhere I'd looked didn't have the "()" (which seems
>>> redundant). The blank line was entirely aesthetics for me. If it's
>>> supposed to be without a blank, I can fix it up everwhere.
>>
>> So, I think this is more of a territory of preference or consistency
>> rather than that of correctness.  Those extra blank lines can be confusing
>> as most people expect it in front of description part.
>>
>> get_maintainer.pl says Kees is the sole maintainer of overflow.h, so
>> it's his call, I guess.
> 
> Well, maintainer or not, I want to make sure stuff is as readable as
> possible by everyone else too. :) I'm happy to skip the blank lines!

Yeah, that sounds nice to me!

        Thanks, Akira

>