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

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

[Kees Cook]

Fix the kern-doc markings for several of the overflow helpers and move
their location into the core kernel API documentation, where it belongs
(it's not driver-specific).

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Akira Yokosawa <akiyks@gmail.com>
Cc: linux-doc@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/core-api/kernel-api.rst |  6 ++++
 Documentation/driver-api/basics.rst   |  3 --
 include/linux/overflow.h              | 43 +++++++++++++++------------
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index 20569f26dde1..0d0c4f87057c 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -121,6 +121,12 @@ Text Searching
 CRC and Math Functions in Linux
 ===============================
 
+Arithmetic Overflow Checking
+----------------------------
+
+.. kernel-doc:: include/linux/overflow.h
+   :internal:
+
 CRC Functions
 -------------
 
diff --git a/Documentation/driver-api/basics.rst b/Documentation/driver-api/basics.rst
index 3e2dae954898..4b4d8e28d3be 100644
--- a/Documentation/driver-api/basics.rst
+++ b/Documentation/driver-api/basics.rst
@@ -107,9 +107,6 @@ Kernel utility functions
 .. kernel-doc:: kernel/panic.c
    :export:
 
-.. kernel-doc:: include/linux/overflow.h
-   :internal:
-
 Device Resource Management
 --------------------------
 
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 58eb34aa2af9..4b5b3ec91233 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -51,7 +51,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 	return unlikely(overflow);
 }
 
-/** check_add_overflow() - Calculate addition with overflow checking
+/**
+ * check_add_overflow - Calculate addition with overflow checking
  *
  * @a: first addend
  * @b: second addend
@@ -66,7 +67,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 #define check_add_overflow(a, b, d)	\
 	__must_check_overflow(__builtin_add_overflow(a, b, d))
 
-/** check_sub_overflow() - Calculate subtraction with overflow checking
+/**
+ * check_sub_overflow - Calculate subtraction with overflow checking
  *
  * @a: minuend; value to subtract from
  * @b: subtrahend; value to subtract from @a
@@ -81,7 +83,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 #define check_sub_overflow(a, b, d)	\
 	__must_check_overflow(__builtin_sub_overflow(a, b, d))
 
-/** check_mul_overflow() - Calculate multiplication with overflow checking
+/**
+ * check_mul_overflow - Calculate multiplication with overflow checking
  *
  * @a: first factor
  * @b: second factor
@@ -96,7 +99,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 #define check_mul_overflow(a, b, d)	\
 	__must_check_overflow(__builtin_mul_overflow(a, b, d))
 
-/** check_shl_overflow() - Calculate a left-shifted value and check overflow
+/**
+ * check_shl_overflow - Calculate a left-shifted value and check overflow
  *
  * @a: Value to be shifted
  * @s: How many bits left to shift
@@ -104,15 +108,16 @@ static inline bool __must_check __must_check_overflow(bool overflow)
  *
  * Computes *@d = (@a << @s)
  *
- * Returns true if '*d' cannot hold the result or when 'a << s' doesn't
+ * Returns true if '*@d' cannot hold the result or when '@a << @s' doesn't
  * make sense. Example conditions:
- * - 'a << s' causes bits to be lost when stored in *d.
- * - 's' is garbage (e.g. negative) or so large that the result of
- *   'a << s' is guaranteed to be 0.
- * - 'a' is negative.
- * - 'a << s' sets the sign bit, if any, in '*d'.
  *
- * '*d' will hold the results of the attempted shift, but is not
+ * - '@a << @s' causes bits to be lost when stored in *@d.
+ * - '@s' is garbage (e.g. negative) or so large that the result of
+ *   '@a << @s' is guaranteed to be 0.
+ * - '@a' is negative.
+ * - '@a << @s' sets the sign bit, if any, in '*@d'.
+ *
+ * '*@d' will hold the results of the attempted shift, but is not
  * considered "safe for use" if true is returned.
  */
 #define check_shl_overflow(a, s, d) __must_check_overflow(({		\
@@ -176,7 +181,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
 			      __same_type(n, T))
 
 /**
- * size_mul() - Calculate size_t multiplication with saturation at SIZE_MAX
+ * size_mul - Calculate size_t multiplication with saturation at SIZE_MAX
  *
  * @factor1: first factor
  * @factor2: second factor
@@ -196,7 +201,7 @@ static inline size_t __must_check size_mul(size_t factor1, size_t factor2)
 }
 
 /**
- * size_add() - Calculate size_t addition with saturation at SIZE_MAX
+ * size_add - Calculate size_t addition with saturation at SIZE_MAX
  *
  * @addend1: first addend
  * @addend2: second addend
@@ -216,7 +221,7 @@ static inline size_t __must_check size_add(size_t addend1, size_t addend2)
 }
 
 /**
- * size_sub() - Calculate size_t subtraction with saturation at SIZE_MAX
+ * size_sub - Calculate size_t subtraction with saturation at SIZE_MAX
  *
  * @minuend: value to subtract from
  * @subtrahend: value to subtract from @minuend
@@ -239,7 +244,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 }
 
 /**
- * array_size() - Calculate size of 2-dimensional array.
+ * array_size - Calculate size of 2-dimensional array.
  *
  * @a: dimension one
  * @b: dimension two
@@ -252,7 +257,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 #define array_size(a, b)	size_mul(a, b)
 
 /**
- * array3_size() - Calculate size of 3-dimensional array.
+ * array3_size - Calculate size of 3-dimensional array.
  *
  * @a: dimension one
  * @b: dimension two
@@ -266,8 +271,8 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 #define array3_size(a, b, c)	size_mul(size_mul(a, b), c)
 
 /**
- * flex_array_size() - Calculate size of a flexible array member
- *                     within an enclosing structure.
+ * flex_array_size - Calculate size of a flexible array member
+ *                   within an enclosing structure.
  *
  * @p: Pointer to the structure.
  * @member: Name of the flexible array member.
@@ -284,7 +289,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
 		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
 
 /**
- * struct_size() - Calculate size of structure with trailing flexible array.
+ * struct_size - Calculate size of structure with trailing flexible array.
  *
  * @p: Pointer to the structure.
  * @member: Name of the array member.
-- 
2.34.1

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

[Matthew Wilcox]

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.

I don't usually complain about people getting that wrong, but when
people correct it to be wrong ...

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

[Kees Cook]

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.
> 
> I don't usually complain about people getting that wrong, but when
> people correct it to be wrong ...

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.

-- 
Kees Cook

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

[Akira Yokosawa]

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".

>>
>> I don't usually complain about people getting that wrong, but when
>> people correct it to be wrong ...

I'd say "wrong" if "./scripts/kernel-doc -v -none include/linux/overflow.h"
complained or the resulting reST doc didn't rendered properly, but that's
not the case here.

> 
> 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.

        Thanks, Akira
> 

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

[Kees Cook]

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.)

> > 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!

-- 
Kees Cook

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

[Bagas Sanjaya]

On 9/27/22 02:47, Kees Cook wrote:
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 58eb34aa2af9..4b5b3ec91233 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -51,7 +51,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>  	return unlikely(overflow);
>  }
>  
> -/** check_add_overflow() - Calculate addition with overflow checking
> +/**
> + * check_add_overflow - Calculate addition with overflow checking
>   *
>   * @a: first addend
>   * @b: second addend
> @@ -66,7 +67,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>  #define check_add_overflow(a, b, d)	\
>  	__must_check_overflow(__builtin_add_overflow(a, b, d))
>  
> -/** check_sub_overflow() - Calculate subtraction with overflow checking
> +/**
> + * check_sub_overflow - Calculate subtraction with overflow checking
>   *
>   * @a: minuend; value to subtract from
>   * @b: subtrahend; value to subtract from @a
> @@ -81,7 +83,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>  #define check_sub_overflow(a, b, d)	\
>  	__must_check_overflow(__builtin_sub_overflow(a, b, d))
>  
> -/** check_mul_overflow() - Calculate multiplication with overflow checking
> +/**
> + * check_mul_overflow - Calculate multiplication with overflow checking
>   *
>   * @a: first factor
>   * @b: second factor
> @@ -96,7 +99,8 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>  #define check_mul_overflow(a, b, d)	\
>  	__must_check_overflow(__builtin_mul_overflow(a, b, d))
>  
> -/** check_shl_overflow() - Calculate a left-shifted value and check overflow
> +/**
> + * check_shl_overflow - Calculate a left-shifted value and check overflow
>   *
>   * @a: Value to be shifted
>   * @s: How many bits left to shift
> @@ -104,15 +108,16 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>   *
>   * Computes *@d = (@a << @s)
>   *
> - * Returns true if '*d' cannot hold the result or when 'a << s' doesn't
> + * Returns true if '*@d' cannot hold the result or when '@a << @s' doesn't
>   * make sense. Example conditions:
> - * - 'a << s' causes bits to be lost when stored in *d.
> - * - 's' is garbage (e.g. negative) or so large that the result of
> - *   'a << s' is guaranteed to be 0.
> - * - 'a' is negative.
> - * - 'a << s' sets the sign bit, if any, in '*d'.
>   *
> - * '*d' will hold the results of the attempted shift, but is not
> + * - '@a << @s' causes bits to be lost when stored in *@d.
> + * - '@s' is garbage (e.g. negative) or so large that the result of
> + *   '@a << @s' is guaranteed to be 0.
> + * - '@a' is negative.
> + * - '@a << @s' sets the sign bit, if any, in '*@d'.
> + *
> + * '*@d' will hold the results of the attempted shift, but is not
>   * considered "safe for use" if true is returned.
>   */
>  #define check_shl_overflow(a, s, d) __must_check_overflow(({		\
> @@ -176,7 +181,7 @@ static inline bool __must_check __must_check_overflow(bool overflow)
>  			      __same_type(n, T))
>  
>  /**
> - * size_mul() - Calculate size_t multiplication with saturation at SIZE_MAX
> + * size_mul - Calculate size_t multiplication with saturation at SIZE_MAX
>   *
>   * @factor1: first factor
>   * @factor2: second factor
> @@ -196,7 +201,7 @@ static inline size_t __must_check size_mul(size_t factor1, size_t factor2)
>  }
>  
>  /**
> - * size_add() - Calculate size_t addition with saturation at SIZE_MAX
> + * size_add - Calculate size_t addition with saturation at SIZE_MAX
>   *
>   * @addend1: first addend
>   * @addend2: second addend
> @@ -216,7 +221,7 @@ static inline size_t __must_check size_add(size_t addend1, size_t addend2)
>  }
>  
>  /**
> - * size_sub() - Calculate size_t subtraction with saturation at SIZE_MAX
> + * size_sub - Calculate size_t subtraction with saturation at SIZE_MAX
>   *
>   * @minuend: value to subtract from
>   * @subtrahend: value to subtract from @minuend
> @@ -239,7 +244,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  }
>  
>  /**
> - * array_size() - Calculate size of 2-dimensional array.
> + * array_size - Calculate size of 2-dimensional array.
>   *
>   * @a: dimension one
>   * @b: dimension two
> @@ -252,7 +257,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  #define array_size(a, b)	size_mul(a, b)
>  
>  /**
> - * array3_size() - Calculate size of 3-dimensional array.
> + * array3_size - Calculate size of 3-dimensional array.
>   *
>   * @a: dimension one
>   * @b: dimension two
> @@ -266,8 +271,8 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  #define array3_size(a, b, c)	size_mul(size_mul(a, b), c)
>  
>  /**
> - * flex_array_size() - Calculate size of a flexible array member
> - *                     within an enclosing structure.
> + * flex_array_size - Calculate size of a flexible array member
> + *                   within an enclosing structure.
>   *
>   * @p: Pointer to the structure.
>   * @member: Name of the flexible array member.
> @@ -284,7 +289,7 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend)
>  		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))
>  
>  /**
> - * struct_size() - Calculate size of structure with trailing flexible array.
> + * struct_size - Calculate size of structure with trailing flexible array.
>   *
>   * @p: Pointer to the structure.
>   * @member: Name of the array member.

Hmm...

I can't apply this patch for testing. On what tree (and what commit) this
patch is based on?

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

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

[Akira Yokosawa]

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

>