linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] docs: Introduce deprecated APIs list
@ 2018-10-17  2:17 Kees Cook
  2018-10-17  3:25 ` Randy Dunlap
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Kees Cook @ 2018-10-17  2:17 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: linux-doc, linux-kernel, Stephen Rothwell

As discussed in the "API replacement/deprecation" thread[1], this
makes an effort to document what things shouldn't get (re)added to the
kernel, by introducing Documentation/process/deprecated.rst. It also
adds the overflow kerndoc to ReST output, and tweaks the struct_size()
documentation to parse correctly.

[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/driver-api/basics.rst  |  3 +
 Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
 Documentation/process/index.rst      |  1 +
 include/linux/overflow.h             |  2 +-
 4 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/process/deprecated.rst

diff --git a/Documentation/driver-api/basics.rst b/Documentation/driver-api/basics.rst
index 826e85d50a16..e970fadf4d1a 100644
--- a/Documentation/driver-api/basics.rst
+++ b/Documentation/driver-api/basics.rst
@@ -121,6 +121,9 @@ Kernel utility functions
 .. kernel-doc:: kernel/rcu/update.c
    :export:
 
+.. kernel-doc:: include/linux/overflow.h
+   :internal:
+
 Device Resource Management
 --------------------------
 
diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
new file mode 100644
index 000000000000..95e261224a81
--- /dev/null
+++ b/Documentation/process/deprecated.rst
@@ -0,0 +1,99 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================================================================
+Deprecated Interfaces, Language Features, Attributes, and Conventions
+=====================================================================
+
+In a perfect world, it would be possible to convert all instances of
+some deprecated API into the new API and entirely remove the old API in
+a single development cycle. However, due to the size of the kernel, the
+maintainership hierarchy, and timing, it's not always feasible to do these
+kinds of conversions at once. This means that new instances may sneak into
+the kernel while old ones are being removed, only making the amount of
+work to remove the API grow. In order to educate developers about what
+has been deprecated and why, this list has been created as a place to
+point when uses of deprecated things are proposed for inclusion in the
+kernel.
+
+__deprecated
+------------
+While this attribute does visually mark an interface as deprecated,
+it `does not produce warnings during builds any more
+<https://git.kernel.org/linus/771c035372a036f83353eef46dbb829780330234>`_
+because one of the standing goals of the kernel is to build without
+warnings and no one was actually doing anything to remove these deprecated
+interfaces. While using `__deprecated` is nice to note an old API in
+a header file, it isn't the full solution. Such interfaces must either
+be fully removed from the kernel, or added to this file to discourage
+others from using them in the future.
+
+open-coded arithmetic in allocator arguments
+--------------------------------------------
+Dynamic size calculations (especially multiplication)
+should not be performed in memory allocator (or similar)
+function arguments.
+
+For example, do not use ``rows * cols`` as an argument, as in:
+``kmalloc(rows * cols, GFP_KERNEL)``.
+Instead, the 2-factor form of the allocator should be used:
+``kmalloc_array(rows, cols, GFP_KERNEL)``.
+If no 2-factor form is available, the saturate-on-overflow helpers should
+be used:
+``vmalloc(array_size(rows, cols))``.
+
+See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`,
+for more details as well as the related :c:func:`check_add_overflow` and
+:c:func:`check_mul_overflow` family of functions.
+
+simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull()
+----------------------------------------------------------------------
+The :c:func:`simple_strtol`, :c:func:`simple_strtoll`,
+:c:func:`simple_strtoul`, and :c:func:`simple_strtoull` functions
+explicitly ignore overflows, which may lead to unexpected results
+in callers. The respective :c:func:`kstrtol`, :c:func:`kstrtoll`,
+:c:func:`kstrtoul`, and :c:func:`kstrtoull` functions tend to be the
+correct replacements, though note that those require the string to be
+NUL or newline terminated.
+
+strcpy()
+--------
+:c:func:`strcpy` performs no bounds checking on the destination
+buffer. This could result in linear overflows beyond the
+end of the buffer, leading to all kinds of misbehaviors. While
+`CONFIG_FORTIFY_SOURCE=y` and various compiler flags help reduce the
+risk of using this function, there is no good reason to add new uses of
+this function. The safe replacement is :c:func:`strscpy`.
+
+strncpy() on NUL-terminated strings
+-----------------------------------
+Use of :c:func:`strncpy` does not guarantee that the destination buffer
+will be NUL terminated. This can lead to various linear read overflows
+and other misbehavior due to the missing termination. It also NUL-pads the
+destination buffer if the source contents are shorter than the destination
+buffer size, which may be a needless performance penalty for callers using
+only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
+(Users of :c:func:`strscpy` still needing NUL-padding will need an
+explicit :c:func:`memset` added.)
+
+If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can
+still be used, but destinations should be marked with the `__nonstring
+<https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
+attribute to avoid future compiler warnings.
+
+strlcpy()
+---------
+:c:func:`strlcpy` reads the entire source buffer first, possibly exceeding
+the given limit of bytes to copy. This is inefficient and can lead to
+linear read overflows if a source string is not NUL-terminated. The
+safe replacement is :c:func:`strscpy`.
+
+Variable Length Arrays (VLAs)
+-----------------------------
+Using stack VLAs produces much worse machine code than statically
+sized stack arrays. While these non-trivial `performance issues
+<https://git.kernel.org/linus/02361bc77888>`_ are reason enough to
+eliminate VLAs, they are also a security risk. Dynamic growth of a stack
+array may exceed the remaining memory in the stack segment. This could
+lead to a crash, possible overwriting sensitive contents at the end of the
+stack (when built without `CONFIG_THREAD_INFO_IN_TASK=y`), or overwriting
+memory adjacent to the stack (when built without `CONFIG_VMAP_STACK=y`)
diff --git a/Documentation/process/index.rst b/Documentation/process/index.rst
index 9ae3e317bddf..b4de2c682255 100644
--- a/Documentation/process/index.rst
+++ b/Documentation/process/index.rst
@@ -41,6 +41,7 @@ Other guides to the community that are of interest to most developers are:
    stable-kernel-rules
    submit-checklist
    kernel-docs
+   deprecated
 
 These are some overall technical guides that have been put here for now for
 lack of a better place.
diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 40b48e2133cb..2f224f43dd06 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
 }
 
 /**
- * struct_size() - Calculate size of structure with trailing array.
+ * function struct_size() - Calculate size of structure with trailing array.
  * @p: Pointer to the structure.
  * @member: Name of the array member.
  * @n: Number of elements in the array.
-- 
2.17.1


-- 
Kees Cook
Pixel Security

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17  2:17 [PATCH] docs: Introduce deprecated APIs list Kees Cook
@ 2018-10-17  3:25 ` Randy Dunlap
  2018-10-17 10:00   ` Jani Nikula
  2018-10-17 12:50 ` Jonathan Corbet
  2018-10-21 15:11 ` Miguel Ojeda
  2 siblings, 1 reply; 10+ messages in thread
From: Randy Dunlap @ 2018-10-17  3:25 UTC (permalink / raw)
  To: Kees Cook, Jonathan Corbet; +Cc: linux-doc, linux-kernel, Stephen Rothwell

On 10/16/18 7:17 PM, Kees Cook wrote:
> As discussed in the "API replacement/deprecation" thread[1], this
> makes an effort to document what things shouldn't get (re)added to the
> kernel, by introducing Documentation/process/deprecated.rst. It also
> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
> documentation to parse correctly.
> 
> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/driver-api/basics.rst  |  3 +
>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>  Documentation/process/index.rst      |  1 +
>  include/linux/overflow.h             |  2 +-
>  4 files changed, 104 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/process/deprecated.rst


> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 40b48e2133cb..2f224f43dd06 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>  }
>  
>  /**
> - * struct_size() - Calculate size of structure with trailing array.
> + * function struct_size() - Calculate size of structure with trailing array.

That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.

Is the root problem that the function name begins with "struct"?
Please explain in the patch description.

>   * @p: Pointer to the structure.
>   * @member: Name of the array member.
>   * @n: Number of elements in the array.


thanks.
-- 
~Randy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17  3:25 ` Randy Dunlap
@ 2018-10-17 10:00   ` Jani Nikula
  2018-10-17 17:08     ` Randy Dunlap
  0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2018-10-17 10:00 UTC (permalink / raw)
  To: Randy Dunlap, Kees Cook, Jonathan Corbet
  Cc: linux-doc, linux-kernel, Stephen Rothwell

On Tue, 16 Oct 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 10/16/18 7:17 PM, Kees Cook wrote:
>> As discussed in the "API replacement/deprecation" thread[1], this
>> makes an effort to document what things shouldn't get (re)added to the
>> kernel, by introducing Documentation/process/deprecated.rst. It also
>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>> documentation to parse correctly.
>> 
>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>> 
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  Documentation/driver-api/basics.rst  |  3 +
>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>  Documentation/process/index.rst      |  1 +
>>  include/linux/overflow.h             |  2 +-
>>  4 files changed, 104 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/process/deprecated.rst
>
>
>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>> index 40b48e2133cb..2f224f43dd06 100644
>> --- a/include/linux/overflow.h
>> +++ b/include/linux/overflow.h
>> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>>  }
>>  
>>  /**
>> - * struct_size() - Calculate size of structure with trailing array.
>> + * function struct_size() - Calculate size of structure with trailing array.
>
> That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.
>
> Is the root problem that the function name begins with "struct"?
> Please explain in the patch description.

Indeed, shouldn't be needed.

BR,
Jani.

>
>>   * @p: Pointer to the structure.
>>   * @member: Name of the array member.
>>   * @n: Number of elements in the array.
>
>
> thanks.

-- 
Jani Nikula, Intel Open Source Graphics Center

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17  2:17 [PATCH] docs: Introduce deprecated APIs list Kees Cook
  2018-10-17  3:25 ` Randy Dunlap
@ 2018-10-17 12:50 ` Jonathan Corbet
  2018-10-17 22:51   ` Kees Cook
  2018-10-21 15:11 ` Miguel Ojeda
  2 siblings, 1 reply; 10+ messages in thread
From: Jonathan Corbet @ 2018-10-17 12:50 UTC (permalink / raw)
  To: Kees Cook; +Cc: linux-doc, linux-kernel, Stephen Rothwell

On Tue, 16 Oct 2018 19:17:08 -0700
Kees Cook <keescook@chromium.org> wrote:

> As discussed in the "API replacement/deprecation" thread[1], this
> makes an effort to document what things shouldn't get (re)added to the
> kernel, by introducing Documentation/process/deprecated.rst. It also
> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
> documentation to parse correctly.

Seems like a good idea overall...a couple of quick comments

> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/driver-api/basics.rst  |  3 +
>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>  Documentation/process/index.rst      |  1 +

I wonder if "process" is the right place, or core-api?  I guess we have a
lot of similar stuff in process now.

[...]

> +open-coded arithmetic in allocator arguments
> +--------------------------------------------
> +Dynamic size calculations (especially multiplication)
> +should not be performed in memory allocator (or similar)
> +function arguments.
> +
> +For example, do not use ``rows * cols`` as an argument, as in:
> +``kmalloc(rows * cols, GFP_KERNEL)``.
> +Instead, the 2-factor form of the allocator should be used:
> +``kmalloc_array(rows, cols, GFP_KERNEL)``.
> +If no 2-factor form is available, the saturate-on-overflow helpers should
> +be used:
> +``vmalloc(array_size(rows, cols))``.
> +
> +See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`,
> +for more details as well as the related :c:func:`check_add_overflow` and
> +:c:func:`check_mul_overflow` family of functions.

I think this should say *why* developers are being told not to do it.  Does
this advice hold even in cases where the dimensions are known, under the
kernel's control, and guaranteed not to overflow even on Alan's port to
eight-bit CPUs?

To me it's also a bit confusing to present the arguments to kmalloc_array()
as "rows" and "cols" when they are really "n" and "size".

Thanks,

jon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17 10:00   ` Jani Nikula
@ 2018-10-17 17:08     ` Randy Dunlap
  2018-10-17 22:37       ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Randy Dunlap @ 2018-10-17 17:08 UTC (permalink / raw)
  To: Jani Nikula, Kees Cook, Jonathan Corbet
  Cc: linux-doc, linux-kernel, Stephen Rothwell

On 10/17/18 3:00 AM, Jani Nikula wrote:
> On Tue, 16 Oct 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
>> On 10/16/18 7:17 PM, Kees Cook wrote:
>>> As discussed in the "API replacement/deprecation" thread[1], this
>>> makes an effort to document what things shouldn't get (re)added to the
>>> kernel, by introducing Documentation/process/deprecated.rst. It also
>>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>>> documentation to parse correctly.
>>>
>>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>  Documentation/driver-api/basics.rst  |  3 +
>>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>>  Documentation/process/index.rst      |  1 +
>>>  include/linux/overflow.h             |  2 +-
>>>  4 files changed, 104 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/process/deprecated.rst
>>
>>
>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>> index 40b48e2133cb..2f224f43dd06 100644
>>> --- a/include/linux/overflow.h
>>> +++ b/include/linux/overflow.h
>>> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>>>  }
>>>  
>>>  /**
>>> - * struct_size() - Calculate size of structure with trailing array.
>>> + * function struct_size() - Calculate size of structure with trailing array.
>>
>> That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.
>>
>> Is the root problem that the function name begins with "struct"?
>> Please explain in the patch description.
> 
> Indeed, shouldn't be needed.
> 
> BR,
> Jani.
> 
>>
>>>   * @p: Pointer to the structure.
>>>   * @member: Name of the array member.
>>>   * @n: Number of elements in the array.
>>
>>
>> thanks.


Well, this is just a guess (no testing), but in scripts/kernel-doc (at line
1907 in 4.19-rc8), we can see:

	if ($identifier =~ m/^struct/) {
	    $decl_type = 'struct';
	} elsif ($identifier =~ m/^union/) {
	    $decl_type = 'union';
	} elsif ($identifier =~ m/^enum/) {
	    $decl_type = 'enum';
	} elsif ($identifier =~ m/^typedef/) {
	    $decl_type = 'typedef';
	} else {
	    $decl_type = 'function';
	}

I wouldn't be surprised if a function named "struct_size" looks like a
type struct.  Maybe it needs to be more strict, with either a space or
word boundary at the end of each type string.  E.g.:

	if ($identifier =~ m/^struct\b/) {
	    $decl_type = 'struct';
	} elsif ($identifier =~ m/^union\b/) {
	    $decl_type = 'union';
	} elsif ($identifier =~ m/^enum\b/) {
	    $decl_type = 'enum';
	} elsif ($identifier =~ m/^typedef\b/) {
	    $decl_type = 'typedef';
	} else {
	    $decl_type = 'function';
	}



-- 
~Randy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17 17:08     ` Randy Dunlap
@ 2018-10-17 22:37       ` Kees Cook
  2018-10-17 22:41         ` Randy Dunlap
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2018-10-17 22:37 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Jani Nikula, Jonathan Corbet, open list:DOCUMENTATION, LKML,
	Stephen Rothwell

On Wed, Oct 17, 2018 at 10:08 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 10/17/18 3:00 AM, Jani Nikula wrote:
>> On Tue, 16 Oct 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
>>> On 10/16/18 7:17 PM, Kees Cook wrote:
>>>> As discussed in the "API replacement/deprecation" thread[1], this
>>>> makes an effort to document what things shouldn't get (re)added to the
>>>> kernel, by introducing Documentation/process/deprecated.rst. It also
>>>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>>>> documentation to parse correctly.
>>>>
>>>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> ---
>>>>  Documentation/driver-api/basics.rst  |  3 +
>>>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>>>  Documentation/process/index.rst      |  1 +
>>>>  include/linux/overflow.h             |  2 +-
>>>>  4 files changed, 104 insertions(+), 1 deletion(-)
>>>>  create mode 100644 Documentation/process/deprecated.rst
>>>
>>>
>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>>> index 40b48e2133cb..2f224f43dd06 100644
>>>> --- a/include/linux/overflow.h
>>>> +++ b/include/linux/overflow.h
>>>> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>>>>  }
>>>>
>>>>  /**
>>>> - * struct_size() - Calculate size of structure with trailing array.
>>>> + * function struct_size() - Calculate size of structure with trailing array.
>>>
>>> That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.
>>>
>>> Is the root problem that the function name begins with "struct"?
>>> Please explain in the patch description.
>>
>> Indeed, shouldn't be needed.

I actually thought the problem was with it not knowing how to deal
with struct_size() being a macro instead of a real function.

> Well, this is just a guess (no testing), but in scripts/kernel-doc (at line
> 1907 in 4.19-rc8), we can see:
>
>         if ($identifier =~ m/^struct/) {
>             $decl_type = 'struct';
>         } elsif ($identifier =~ m/^union/) {
>             $decl_type = 'union';
>         } elsif ($identifier =~ m/^enum/) {
>             $decl_type = 'enum';
>         } elsif ($identifier =~ m/^typedef/) {
>             $decl_type = 'typedef';
>         } else {
>             $decl_type = 'function';
>         }
>
> I wouldn't be surprised if a function named "struct_size" looks like a
> type struct.  Maybe it needs to be more strict, with either a space or
> word boundary at the end of each type string.  E.g.:
>
>         if ($identifier =~ m/^struct\b/) {
>             $decl_type = 'struct';
>         } elsif ($identifier =~ m/^union\b/) {
>             $decl_type = 'union';
>         } elsif ($identifier =~ m/^enum\b/) {
>             $decl_type = 'enum';
>         } elsif ($identifier =~ m/^typedef\b/) {
>             $decl_type = 'typedef';
>         } else {
>             $decl_type = 'function';
>         }

But I see it's actually the prefix! :P

Using the above code fixes it for me. Can you send this fix with my
Tested-by, and I'll spin a v2 of my "deprecated.rst" patch without the
overflow.h change?

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17 22:37       ` Kees Cook
@ 2018-10-17 22:41         ` Randy Dunlap
  2018-10-17 23:34           ` Randy Dunlap
  0 siblings, 1 reply; 10+ messages in thread
From: Randy Dunlap @ 2018-10-17 22:41 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jani Nikula, Jonathan Corbet, open list:DOCUMENTATION, LKML,
	Stephen Rothwell

On 10/17/18 3:37 PM, Kees Cook wrote:
> On Wed, Oct 17, 2018 at 10:08 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
>> On 10/17/18 3:00 AM, Jani Nikula wrote:
>>> On Tue, 16 Oct 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
>>>> On 10/16/18 7:17 PM, Kees Cook wrote:
>>>>> As discussed in the "API replacement/deprecation" thread[1], this
>>>>> makes an effort to document what things shouldn't get (re)added to the
>>>>> kernel, by introducing Documentation/process/deprecated.rst. It also
>>>>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>>>>> documentation to parse correctly.
>>>>>
>>>>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>>>>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>>  Documentation/driver-api/basics.rst  |  3 +
>>>>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>>>>  Documentation/process/index.rst      |  1 +
>>>>>  include/linux/overflow.h             |  2 +-
>>>>>  4 files changed, 104 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 Documentation/process/deprecated.rst
>>>>
>>>>
>>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>>>> index 40b48e2133cb..2f224f43dd06 100644
>>>>> --- a/include/linux/overflow.h
>>>>> +++ b/include/linux/overflow.h
>>>>> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>>>>>  }
>>>>>
>>>>>  /**
>>>>> - * struct_size() - Calculate size of structure with trailing array.
>>>>> + * function struct_size() - Calculate size of structure with trailing array.
>>>>
>>>> That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.
>>>>
>>>> Is the root problem that the function name begins with "struct"?
>>>> Please explain in the patch description.
>>>
>>> Indeed, shouldn't be needed.
> 
> I actually thought the problem was with it not knowing how to deal
> with struct_size() being a macro instead of a real function.
> 
>> Well, this is just a guess (no testing), but in scripts/kernel-doc (at line
>> 1907 in 4.19-rc8), we can see:
>>
>>         if ($identifier =~ m/^struct/) {
>>             $decl_type = 'struct';
>>         } elsif ($identifier =~ m/^union/) {
>>             $decl_type = 'union';
>>         } elsif ($identifier =~ m/^enum/) {
>>             $decl_type = 'enum';
>>         } elsif ($identifier =~ m/^typedef/) {
>>             $decl_type = 'typedef';
>>         } else {
>>             $decl_type = 'function';
>>         }
>>
>> I wouldn't be surprised if a function named "struct_size" looks like a
>> type struct.  Maybe it needs to be more strict, with either a space or
>> word boundary at the end of each type string.  E.g.:
>>
>>         if ($identifier =~ m/^struct\b/) {
>>             $decl_type = 'struct';
>>         } elsif ($identifier =~ m/^union\b/) {
>>             $decl_type = 'union';
>>         } elsif ($identifier =~ m/^enum\b/) {
>>             $decl_type = 'enum';
>>         } elsif ($identifier =~ m/^typedef\b/) {
>>             $decl_type = 'typedef';
>>         } else {
>>             $decl_type = 'function';
>>         }
> 
> But I see it's actually the prefix! :P
> 
> Using the above code fixes it for me. Can you send this fix with my
> Tested-by, and I'll spin a v2 of my "deprecated.rst" patch without the
> overflow.h change?

OK, but I'll have to first test for unexpected consequences of it.
(i.e., run full docs build with and without the patch)

-- 
~Randy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17 12:50 ` Jonathan Corbet
@ 2018-10-17 22:51   ` Kees Cook
  0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2018-10-17 22:51 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: open list:DOCUMENTATION, LKML, Stephen Rothwell

On Wed, Oct 17, 2018 at 5:50 AM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Tue, 16 Oct 2018 19:17:08 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> As discussed in the "API replacement/deprecation" thread[1], this
>> makes an effort to document what things shouldn't get (re)added to the
>> kernel, by introducing Documentation/process/deprecated.rst. It also
>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>> documentation to parse correctly.
>
> Seems like a good idea overall...a couple of quick comments
>
>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  Documentation/driver-api/basics.rst  |  3 +
>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>  Documentation/process/index.rst      |  1 +
>
> I wonder if "process" is the right place, or core-api?  I guess we have a
> lot of similar stuff in process now.

Totally up to you. It seemed better suited for "process" (here's
things NOT to do) than "core-api" (here's things to use).

>
> [...]
>
>> +open-coded arithmetic in allocator arguments
>> +--------------------------------------------
>> +Dynamic size calculations (especially multiplication)
>> +should not be performed in memory allocator (or similar)
>> +function arguments.
>> +
>> +For example, do not use ``rows * cols`` as an argument, as in:
>> +``kmalloc(rows * cols, GFP_KERNEL)``.
>> +Instead, the 2-factor form of the allocator should be used:
>> +``kmalloc_array(rows, cols, GFP_KERNEL)``.
>> +If no 2-factor form is available, the saturate-on-overflow helpers should
>> +be used:
>> +``vmalloc(array_size(rows, cols))``.
>> +
>> +See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`,
>> +for more details as well as the related :c:func:`check_add_overflow` and
>> +:c:func:`check_mul_overflow` family of functions.
>
> I think this should say *why* developers are being told not to do it.  Does
> this advice hold even in cases where the dimensions are known, under the
> kernel's control, and guaranteed not to overflow even on Alan's port to
> eight-bit CPUs?

I will attempt to explain this better. When all factors are constants,
the compiler will warn if there is an overflow. If, however, anything
is a dynamic size, we run the risk of overflow. It's not true in all
cases (e.g. u8 var * sizeof()) but it's more robust to globally avoid
it. (What happens when that u8 becomes a u64 at some future time?)

> To me it's also a bit confusing to present the arguments to kmalloc_array()
> as "rows" and "cols" when they are really "n" and "size".

That's true, though I used rows * cols as an example because people
aren't always thinking about their multiplications as having n-many
sizes. e.g. "I just want a full screen of pixels." Let me see if I can
adjust it...

Thanks for the feedback!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17 22:41         ` Randy Dunlap
@ 2018-10-17 23:34           ` Randy Dunlap
  0 siblings, 0 replies; 10+ messages in thread
From: Randy Dunlap @ 2018-10-17 23:34 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jani Nikula, Jonathan Corbet, open list:DOCUMENTATION, LKML,
	Stephen Rothwell

On 10/17/18 3:41 PM, Randy Dunlap wrote:
> On 10/17/18 3:37 PM, Kees Cook wrote:
>> On Wed, Oct 17, 2018 at 10:08 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
>>> On 10/17/18 3:00 AM, Jani Nikula wrote:
>>>> On Tue, 16 Oct 2018, Randy Dunlap <rdunlap@infradead.org> wrote:
>>>>> On 10/16/18 7:17 PM, Kees Cook wrote:
>>>>>> As discussed in the "API replacement/deprecation" thread[1], this
>>>>>> makes an effort to document what things shouldn't get (re)added to the
>>>>>> kernel, by introducing Documentation/process/deprecated.rst. It also
>>>>>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>>>>>> documentation to parse correctly.
>>>>>>
>>>>>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>>>>>
>>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>>> ---
>>>>>>  Documentation/driver-api/basics.rst  |  3 +
>>>>>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>>>>>  Documentation/process/index.rst      |  1 +
>>>>>>  include/linux/overflow.h             |  2 +-
>>>>>>  4 files changed, 104 insertions(+), 1 deletion(-)
>>>>>>  create mode 100644 Documentation/process/deprecated.rst
>>>>>
>>>>>
>>>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
>>>>>> index 40b48e2133cb..2f224f43dd06 100644
>>>>>> --- a/include/linux/overflow.h
>>>>>> +++ b/include/linux/overflow.h
>>>>>> @@ -291,7 +291,7 @@ static inline __must_check size_t __ab_c_size(size_t n, size_t size, size_t c)
>>>>>>  }
>>>>>>
>>>>>>  /**
>>>>>> - * struct_size() - Calculate size of structure with trailing array.
>>>>>> + * function struct_size() - Calculate size of structure with trailing array.
>>>>>
>>>>> That syntax is not explained nor documented in Documentation/doc-guide/kernel-doc.rst.
>>>>>
>>>>> Is the root problem that the function name begins with "struct"?
>>>>> Please explain in the patch description.
>>>>
>>>> Indeed, shouldn't be needed.
>>
>> I actually thought the problem was with it not knowing how to deal
>> with struct_size() being a macro instead of a real function.
>>
>>> Well, this is just a guess (no testing), but in scripts/kernel-doc (at line
>>> 1907 in 4.19-rc8), we can see:
>>>
>>>         if ($identifier =~ m/^struct/) {
>>>             $decl_type = 'struct';
>>>         } elsif ($identifier =~ m/^union/) {
>>>             $decl_type = 'union';
>>>         } elsif ($identifier =~ m/^enum/) {
>>>             $decl_type = 'enum';
>>>         } elsif ($identifier =~ m/^typedef/) {
>>>             $decl_type = 'typedef';
>>>         } else {
>>>             $decl_type = 'function';
>>>         }
>>>
>>> I wouldn't be surprised if a function named "struct_size" looks like a
>>> type struct.  Maybe it needs to be more strict, with either a space or
>>> word boundary at the end of each type string.  E.g.:
>>>
>>>         if ($identifier =~ m/^struct\b/) {
>>>             $decl_type = 'struct';
>>>         } elsif ($identifier =~ m/^union\b/) {
>>>             $decl_type = 'union';
>>>         } elsif ($identifier =~ m/^enum\b/) {
>>>             $decl_type = 'enum';
>>>         } elsif ($identifier =~ m/^typedef\b/) {
>>>             $decl_type = 'typedef';
>>>         } else {
>>>             $decl_type = 'function';
>>>         }
>>
>> But I see it's actually the prefix! :P
>>
>> Using the above code fixes it for me. Can you send this fix with my
>> Tested-by, and I'll spin a v2 of my "deprecated.rst" patch without the
>> overflow.h change?
> 
> OK, but I'll have to first test for unexpected consequences of it.
> (i.e., run full docs build with and without the patch)

I've tested it successfully.  I'll send it out later tonight. (-EBUSY now)

-- 
~Randy

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] docs: Introduce deprecated APIs list
  2018-10-17  2:17 [PATCH] docs: Introduce deprecated APIs list Kees Cook
  2018-10-17  3:25 ` Randy Dunlap
  2018-10-17 12:50 ` Jonathan Corbet
@ 2018-10-21 15:11 ` Miguel Ojeda
  2 siblings, 0 replies; 10+ messages in thread
From: Miguel Ojeda @ 2018-10-21 15:11 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jonathan Corbet, Linux Doc Mailing List, linux-kernel, Stephen Rothwell

On Wed, Oct 17, 2018 at 4:20 AM Kees Cook <keescook@chromium.org> wrote:
>
> As discussed in the "API replacement/deprecation" thread[1], this
> makes an effort to document what things shouldn't get (re)added to the
> kernel, by introducing Documentation/process/deprecated.rst. It also
> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
> documentation to parse correctly.

If you remember, please CC me as well, since I would like to be
involved in this as well, given that  it touches the compiler
attributes :-)

Also, I will send a patch (for the deprecated.rst file, adding a
section about fallthrough comments) later on.

Thanks for adding the file!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-10-21 15:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-17  2:17 [PATCH] docs: Introduce deprecated APIs list Kees Cook
2018-10-17  3:25 ` Randy Dunlap
2018-10-17 10:00   ` Jani Nikula
2018-10-17 17:08     ` Randy Dunlap
2018-10-17 22:37       ` Kees Cook
2018-10-17 22:41         ` Randy Dunlap
2018-10-17 23:34           ` Randy Dunlap
2018-10-17 12:50 ` Jonathan Corbet
2018-10-17 22:51   ` Kees Cook
2018-10-21 15:11 ` Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).