[Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

Hi,

I'd like to discuss ways that we could deprecate APIs more sanely. At
present I've seen (and used) two approaches, fast and slow:

Fast:
Introduce new API in release N. Perform massive patch bombs drowning
maintainers in a single release for release N+1, and remove the old
API before -rc2.
Pros:
- it gets done in 2 releases!
Cons:
- writing the API transition patches may span multiple release prior
to N without entering linux-next, which causes rebase/conflict cycles
- patches developed prior to N may not get adequate testing
- maintainers drown in patches
Example: timer_setup() refactoring, kmalloc()->kmalloc_array()

Slow:
Introduce new API in release N. Trickle patches in over N+M releases,
removing old API when no more uses are present.
Pros:
- patches are written and handed to maintainers at a regular rate
- testing happens normally
Cons:
- new users of the old API continue to enter the kernel, causing an
endless cycle of needing more work
- API may never get deprecated
Example: strscpy(), VLA removal

Note that "Fast" isn't actually fast, since the bulk of the prep work
happens "in the dark".

The primary issue with "Slow" is the introduction of uses of the old
API. With a "small" API, this is managemable (e.g. with VLA removal we
started with only about 120 instances, and only 2-3 new ones got added
each release). With large mechanical changes (kmalloc_array()) it's
easy to keep up with new users, since the fixes are mechanical.

While trying to prepare to move away from bare strcpy(),
not-always-terminated strncpy(), and
read-entire-src-and-do-full-NUL-padding strlen(), there are literally
thousands of users to move to strscpy(), and I expect new users to
keep entering the kernel at a high rate. Adding warnings to
checkpatch.pl isn't sufficient since only a subset of maintainers
require patches be "checkpatch clean".

Some str*cpy() numbers for context:
$ for i in '' n l s; do j=str${i}cpy; echo -en "$j\t"; git grep
'\b'"$j"'\b' | grep -Ev '^(Documentation|tools)/' | wc -l ; done
strcpy  2490
strncpy 865
strlcpy 2361
strscpy 29

Linus correctly removed __deprecated since it's just noisy: marking
strcpy() as __deprecated would be an absolute nightmare. What would be
great would be a way to mark NEW users of an API with a warning, but
outside of voluntary checkpatch use, we have no global patch checker
that is always run. Adding some kind of "__grandfathered" mark to
existing users that would silence a __deprecated warning seems like
just creating two problems, and would create massive churn in the
existing code.

I'd really like a solution besides "masochism". :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

Hi all,

On Wed, 5 Sep 2018 15:57:02 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> I'd like to discuss ways that we could deprecate APIs more sanely. At
> present I've seen (and used) two approaches, fast and slow:

As the one who often has the "fun" of coping with API changes, I would
like to be involved in this discussion.

My first point would be that (almost) every time someone has tried the
"ultra fast" method (i.e. add new interface, convert everyone in the
current kernel, remove the old interface all in one go) we have had new
users of the old interface introduced at the same time.  (pain for the
linux-next bunny :-()
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Steven Rostedt]

On Thu, 6 Sep 2018 09:41:58 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi all,
> 
> On Wed, 5 Sep 2018 15:57:02 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> > I'd like to discuss ways that we could deprecate APIs more sanely. At
> > present I've seen (and used) two approaches, fast and slow:  
> 
> As the one who often has the "fun" of coping with API changes, I would
> like to be involved in this discussion.
> 
> My first point would be that (almost) every time someone has tried the
> "ultra fast" method (i.e. add new interface, convert everyone in the
> current kernel, remove the old interface all in one go) we have had new
> users of the old interface introduced at the same time.  (pain for the
> linux-next bunny :-()

Can this be solved with a script on kernel.org? Or a zero-day bot that
checks new commits (and perhaps patches to LKML) that checks for
deprecated functions being added by new code (like strcpy) and the
author would then get a nasty email about adding deprecated interfaces.

This would solve the issue of not everyone using the latest checkpatch,
as this wouldn't be a voluntary self-check. It would also quickly
educate developers on what code is not acceptable to be added.

-- Steve

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Julia Lawall]

On Wed, 5 Sep 2018, Kees Cook wrote:

> Hi,
>
> I'd like to discuss ways that we could deprecate APIs more sanely. At
> present I've seen (and used) two approaches, fast and slow:
>
> Fast:
> Introduce new API in release N. Perform massive patch bombs drowning
> maintainers in a single release for release N+1, and remove the old
> API before -rc2.
> Pros:
> - it gets done in 2 releases!
> Cons:
> - writing the API transition patches may span multiple release prior
> to N without entering linux-next, which causes rebase/conflict cycles
> - patches developed prior to N may not get adequate testing
> - maintainers drown in patches
> Example: timer_setup() refactoring, kmalloc()->kmalloc_array()
>
> Slow:
> Introduce new API in release N. Trickle patches in over N+M releases,
> removing old API when no more uses are present.
> Pros:
> - patches are written and handed to maintainers at a regular rate
> - testing happens normally
> Cons:
> - new users of the old API continue to enter the kernel, causing an
> endless cycle of needing more work
> - API may never get deprecated
> Example: strscpy(), VLA removal
>
> Note that "Fast" isn't actually fast, since the bulk of the prep work
> happens "in the dark".
>
> The primary issue with "Slow" is the introduction of uses of the old
> API. With a "small" API, this is managemable (e.g. with VLA removal we
> started with only about 120 instances, and only 2-3 new ones got added
> each release). With large mechanical changes (kmalloc_array()) it's
> easy to keep up with new users, since the fixes are mechanical.
>
> While trying to prepare to move away from bare strcpy(),
> not-always-terminated strncpy(), and
> read-entire-src-and-do-full-NUL-padding strlen(), there are literally
> thousands of users to move to strscpy(), and I expect new users to
> keep entering the kernel at a high rate. Adding warnings to
> checkpatch.pl isn't sufficient since only a subset of maintainers
> require patches be "checkpatch clean".
>
> Some str*cpy() numbers for context:
> $ for i in '' n l s; do j=str${i}cpy; echo -en "$j\t"; git grep
> '\b'"$j"'\b' | grep -Ev '^(Documentation|tools)/' | wc -l ; done
> strcpy  2490
> strncpy 865
> strlcpy 2361
> strscpy 29
>
> Linus correctly removed __deprecated since it's just noisy: marking
> strcpy() as __deprecated would be an absolute nightmare. What would be
> great would be a way to mark NEW users of an API with a warning, but
> outside of voluntary checkpatch use, we have no global patch checker
> that is always run. Adding some kind of "__grandfathered" mark to
> existing users that would silence a __deprecated warning seems like
> just creating two problems, and would create massive churn in the
> existing code.
>
> I'd really like a solution besides "masochism". :)

Could people be encouraged to try for medium, ie following the fast
approach but not actually removing the old function immediately?
I remember that the devm functions, for example, took off very slowly.
>From the above numbers it looks like strscpy is doing the same.  I no one
sees something being used, then know one will know that they should be
using it, and it risks never being picked up.

julia

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Julia Lawall]

On Wed, 5 Sep 2018, Steven Rostedt wrote:

> On Thu, 6 Sep 2018 09:41:58 +1000
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > Hi all,
> >
> > On Wed, 5 Sep 2018 15:57:02 -0700 Kees Cook <keescook@chromium.org> wrote:
> > >
> > > I'd like to discuss ways that we could deprecate APIs more sanely. At
> > > present I've seen (and used) two approaches, fast and slow:
> >
> > As the one who often has the "fun" of coping with API changes, I would
> > like to be involved in this discussion.
> >
> > My first point would be that (almost) every time someone has tried the
> > "ultra fast" method (i.e. add new interface, convert everyone in the
> > current kernel, remove the old interface all in one go) we have had new
> > users of the old interface introduced at the same time.  (pain for the
> > linux-next bunny :-()
>
> Can this be solved with a script on kernel.org? Or a zero-day bot that
> checks new commits (and perhaps patches to LKML) that checks for
> deprecated functions being added by new code (like strcpy) and the
> author would then get a nasty email about adding deprecated interfaces.

There are some Coccinelle scripts already for functions the DRM people
want to get rid of.  They regularly trigger reports.  The advantage is to
show people what they should do, and not just tell people what they should
not do.  There could be a generic script that would just complain, for
cases where the relation between the undesired code and the desired code
is not easy to express.  Updating such a script would involve just adding
the name of the function to a list.

julia

>
> This would solve the issue of not everyone using the latest checkpatch,
> as this wouldn't be a voluntary self-check. It would also quickly
> educate developers on what code is not acceptable to be added.
>
> -- Steve
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Linus Walleij]

On Thu, Sep 6, 2018 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:

> Slow:
> Introduce new API in release N. Trickle patches in over N+M releases,
> removing old API when no more uses are present.
> Pros:
> - patches are written and handed to maintainers at a regular rate
> - testing happens normally
> Cons:
> - new users of the old API continue to enter the kernel, causing an
> endless cycle of needing more work
> - API may never get deprecated
> Example: strscpy(), VLA removal

GPIO descriptors trying to remove the global GPIO numbers, in
turn inspired by the IRQ number removal trying to remove
global IRQ numbers.

I am moving forward with the GPIO work, sometimes I even get
help.

As the process involves building manu esoteric architectures
I have been most helped by Arnd deleting cruft archs such as
blackfin and the 0day kbuild hitting me on the head for all
stupid mistakes I make, this has added some momentum to
the trickliness so it is more like a brook these days. It's still
not quick.

Yours,
Linus Walleij

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Geert Uytterhoeven]

On Thu, Sep 6, 2018 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
> I'd like to discuss ways that we could deprecate APIs more sanely. At
> present I've seen (and used) two approaches, fast and slow:

> Example: strscpy(), VLA removal

I didn't know VLAs were a kernel API ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

On Thu, Sep 6, 2018 at 3:11 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Sep 6, 2018 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>> I'd like to discuss ways that we could deprecate APIs more sanely. At
>> present I've seen (and used) two approaches, fast and slow:
>
>> Example: strscpy(), VLA removal
>
> I didn't know VLAs were a kernel API ;-)

I couldn't find a better short-hand that covered everything. "usage
patterns"? Whatever, people understand what I mean. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Geert Uytterhoeven]

Hi Kees,

On Thu, Sep 6, 2018 at 4:59 PM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Sep 6, 2018 at 3:11 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Sep 6, 2018 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
> >> I'd like to discuss ways that we could deprecate APIs more sanely. At
> >> present I've seen (and used) two approaches, fast and slow:
> >
> >> Example: strscpy(), VLA removal
> >
> > I didn't know VLAs were a kernel API ;-)
>
> I couldn't find a better short-hand that covered everything. "usage
> patterns"? Whatever, people understand what I mean. :)

Initially I though it made a big difference, as no VLA provider API was removed.
But it indeed becomes an equivalent problem when adding -Werror=vla.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

On Wed, Sep 5, 2018 at 7:24 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 6 Sep 2018 09:41:58 +1000
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
>> Hi all,
>>
>> On Wed, 5 Sep 2018 15:57:02 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >
>> > I'd like to discuss ways that we could deprecate APIs more sanely. At
>> > present I've seen (and used) two approaches, fast and slow:
>>
>> As the one who often has the "fun" of coping with API changes, I would
>> like to be involved in this discussion.
>>
>> My first point would be that (almost) every time someone has tried the
>> "ultra fast" method (i.e. add new interface, convert everyone in the
>> current kernel, remove the old interface all in one go) we have had new
>> users of the old interface introduced at the same time.  (pain for the
>> linux-next bunny :-()
>
> Can this be solved with a script on kernel.org? Or a zero-day bot that
> checks new commits (and perhaps patches to LKML) that checks for
> deprecated functions being added by new code (like strcpy) and the
> author would then get a nasty email about adding deprecated interfaces.
>
> This would solve the issue of not everyone using the latest checkpatch,
> as this wouldn't be a voluntary self-check. It would also quickly
> educate developers on what code is not acceptable to be added.

I think this boils down to how our development ecosystem works. Things
are pretty "voluntary" right now: one could easily ignore zero-day or
checkpatch all the time, and if a maintainer isn't checking these
things, a patch will go in.

I had two ideas recently. First, the more "social contract" one:

If there was an agreement by all maintainers that deprecated
functions/patterns should not be added, and we documented the
deprecation somewhere like Documentation/process/deprecated.rst, then
we could make the declaration that if such functions got added (it's
easy to mechanically check for them), it would be the responsibility
of the author and maintainer chain to see that it got fixed before the
release is cut. We already have this for things like "breaks the x86
allmodconfig build" or similar. The checking would be manual, and the
enforcement would be by agreement, but it'd be better than the kind of
"please don't do this" hand-waving we've had in the past.

Then there's the magical future CI idea, which builds on a bit more
automation without giving up our agency as maintainers:

If we designed a "minimum sanity checking" CI system that tracked the
same trees as linux-next, it could perform all the mechanical checking
we decided was proper ("allmodconfig" for every architecture[1],
per-patch buildability, no new instances of deprecated functions, etc
etc), then a given sha from a tree could be queried for "has this
passed CI?" In doing pull requests, etc, a maintainer could declare
stuff like "Please pull up to $SHA from me tree. It has cleared CI."
etc. And the maintainer above could run some sanity check like "curl
https://kernelci.org/check/$SHA | grep passed". And merging things
that didn't pass CI would be frowned upon, etc.

The latter doesn't force us to use different tools (no clicking around
on a web page, just add another command line check), and doesn't make
anyone push trees somewhere new (the CI would pull form linux-next's
list of trees, for example), but it would gain us a centralized view
of "did this tree pass basic checks".

The former idea has the benefit of being immediately doable, given the
will/agreement of the community. The latter might even be possible now
with zero-day if it grew a way to query the state of commits and there
was greater transparency into which tools were running, etc.

-Kees

[1] Not all architectures actually _have_ a buildable allmodconfig.

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

Hi Kees,

On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
>
> If there was an agreement by all maintainers that deprecated
> functions/patterns should not be added, and we documented the
> deprecation somewhere like Documentation/process/deprecated.rst, then
> we could make the declaration that if such functions got added (it's
> easy to mechanically check for them), it would be the responsibility
> of the author and maintainer chain to see that it got fixed before the
> release is cut. We already have this for things like "breaks the x86
> allmodconfig build" or similar. The checking would be manual, and the
> enforcement would be by agreement, but it'd be better than the kind of
> "please don't do this" hand-waving we've had in the past.

I could do this in linux-next, of course, the same way I check for
missing signed-off-bys.  All I would need is the list of deprecated
things.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Kees,
>
> On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
>>
>> If there was an agreement by all maintainers that deprecated
>> functions/patterns should not be added, and we documented the
>> deprecation somewhere like Documentation/process/deprecated.rst, then
>> we could make the declaration that if such functions got added (it's
>> easy to mechanically check for them), it would be the responsibility
>> of the author and maintainer chain to see that it got fixed before the
>> release is cut. We already have this for things like "breaks the x86
>> allmodconfig build" or similar. The checking would be manual, and the
>> enforcement would be by agreement, but it'd be better than the kind of
>> "please don't do this" hand-waving we've had in the past.
>
> I could do this in linux-next, of course, the same way I check for
> missing signed-off-bys.  All I would need is the list of deprecated
> things.

Hopefully we can all agree on deprecating strcpy() and strncpy() in
favor of strscpy()?

-Kees

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Takashi Iwai]

On Fri, 07 Sep 2018 01:24:03 +0200,
Kees Cook wrote:
> 
> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Kees,
> >
> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
> >>
> >> If there was an agreement by all maintainers that deprecated
> >> functions/patterns should not be added, and we documented the
> >> deprecation somewhere like Documentation/process/deprecated.rst, then
> >> we could make the declaration that if such functions got added (it's
> >> easy to mechanically check for them), it would be the responsibility
> >> of the author and maintainer chain to see that it got fixed before the
> >> release is cut. We already have this for things like "breaks the x86
> >> allmodconfig build" or similar. The checking would be manual, and the
> >> enforcement would be by agreement, but it'd be better than the kind of
> >> "please don't do this" hand-waving we've had in the past.
> >
> > I could do this in linux-next, of course, the same way I check for
> > missing signed-off-bys.  All I would need is the list of deprecated
> > things.
> 
> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> favor of strscpy()?

How about providing some lightweight check script for git commit hook,
and let each maintainer install it? 


thanks,

Takashi

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Johannes Berg]

On Fri, 2018-09-07 at 09:03 +0200, Takashi Iwai wrote:
> 
> How about providing some lightweight check script for git commit hook,
> and let each maintainer install it? 

What Julia suggested seems easier: Just drop a script to check for it
into scripts/coccinelle/, and 0-day build bot should run it
automatically on pretty much everything, afaict?

johannes

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Takashi Iwai]

On Fri, 07 Sep 2018 09:20:37 +0200,
Johannes Berg wrote:
> 
> On Fri, 2018-09-07 at 09:03 +0200, Takashi Iwai wrote:
> > 
> > How about providing some lightweight check script for git commit hook,
> > and let each maintainer install it? 
> 
> What Julia suggested seems easier: Just drop a script to check for it
> into scripts/coccinelle/, and 0-day build bot should run it
> automatically on pretty much everything, afaict?

For trivial tasks like checking the missing sign-off, it's far quicker
to run locally.  Then you'll notice it before publishing, so no risk
that is pulled by others.

For more comprehensive checks, yeah, 0-day bot coverage would be
great.


Takashi

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Jani Nikula]

On Fri, 07 Sep 2018, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 07 Sep 2018 01:24:03 +0200,
> Kees Cook wrote:
>> 
>> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> > Hi Kees,
>> >
>> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >>
>> >> If there was an agreement by all maintainers that deprecated
>> >> functions/patterns should not be added, and we documented the
>> >> deprecation somewhere like Documentation/process/deprecated.rst, then
>> >> we could make the declaration that if such functions got added (it's
>> >> easy to mechanically check for them), it would be the responsibility
>> >> of the author and maintainer chain to see that it got fixed before the
>> >> release is cut. We already have this for things like "breaks the x86
>> >> allmodconfig build" or similar. The checking would be manual, and the
>> >> enforcement would be by agreement, but it'd be better than the kind of
>> >> "please don't do this" hand-waving we've had in the past.
>> >
>> > I could do this in linux-next, of course, the same way I check for
>> > missing signed-off-bys.  All I would need is the list of deprecated
>> > things.
>> 
>> Hopefully we can all agree on deprecating strcpy() and strncpy() in
>> favor of strscpy()?
>
> How about providing some lightweight check script for git commit hook,
> and let each maintainer install it? 

I looked up 771c035372a0 ("deprecate the '__deprecated' attribute
warnings entirely and for good"). It's easy to agree that's the right
thing to do for the regular build.

However, I think there's value in having __deprecated tagged to
functions. (Note, just that, without defining it as
__attribute__((deprecated)).) People looking the functions up can see
they should find alternatives, and people looking for things to do could
take on the conversion. I don't think a separate deprecated file will
work. It's all too detached.

Then you can just use -D__deprecated=__attribute__((deprecated)) to get
the warnings when you like, even on a per module/file basis, or add that
to W=<some level>, or add that to 0-day or whatever CI, ensuring patches
aren't adding new warnings.

No need to invent wheels for things where the compiler can help.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Jan Kara]

On Thu 06-09-18 16:24:03, Kees Cook wrote:
> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Kees,
> >
> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
> >>
> >> If there was an agreement by all maintainers that deprecated
> >> functions/patterns should not be added, and we documented the
> >> deprecation somewhere like Documentation/process/deprecated.rst, then
> >> we could make the declaration that if such functions got added (it's
> >> easy to mechanically check for them), it would be the responsibility
> >> of the author and maintainer chain to see that it got fixed before the
> >> release is cut. We already have this for things like "breaks the x86
> >> allmodconfig build" or similar. The checking would be manual, and the
> >> enforcement would be by agreement, but it'd be better than the kind of
> >> "please don't do this" hand-waving we've had in the past.
> >
> > I could do this in linux-next, of course, the same way I check for
> > missing signed-off-bys.  All I would need is the list of deprecated
> > things.
> 
> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> favor of strscpy()?

So if you want to start enforcing "no deprecated functions in new patches",
then I'm afraid you will need to get a wider consensus on every deprecated
function. Otherwise this ends up like another checkpatch which annoys
quite some people because they don't consider the warnings interesting.

To take your example above, if I get an email like: "You must not use
strcpy(name, ".."). Use strscpy() instead in your patch." then it should be
accompanied by a good explanation why the change needs to be made.
Otherwise people get annoyed by this check pushing more work onto them.
And this is different from other checks e.g. 0-day does - 99% of developers
agree that kernel should always built so they are OK with the burden of
fixing compilation of exotic configuration their patch breaks. And with
deprecation you need to do similar convincing for each and every deprecated
function...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

Hi Kees,

On Thu, Sep 06, 2018 at 11:24:11AM -0700, Kees Cook wrote:
> On Wed, Sep 5, 2018 at 7:24 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 6 Sep 2018 09:41:58 +1000
> > Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >> On Wed, 5 Sep 2018 15:57:02 -0700 Kees Cook <keescook@chromium.org> wrote:
> >> >
> >> > I'd like to discuss ways that we could deprecate APIs more sanely. At
> >> > present I've seen (and used) two approaches, fast and slow:
> >>
> >> As the one who often has the "fun" of coping with API changes, I would
> >> like to be involved in this discussion.
> >>
> >> My first point would be that (almost) every time someone has tried the
> >> "ultra fast" method (i.e. add new interface, convert everyone in the
> >> current kernel, remove the old interface all in one go) we have had new
> >> users of the old interface introduced at the same time.  (pain for the
> >> linux-next bunny :-()
> >
> > Can this be solved with a script on kernel.org? Or a zero-day bot that
> > checks new commits (and perhaps patches to LKML) that checks for
> > deprecated functions being added by new code (like strcpy) and the
> > author would then get a nasty email about adding deprecated interfaces.
> >
> > This would solve the issue of not everyone using the latest checkpatch,
> > as this wouldn't be a voluntary self-check. It would also quickly
> > educate developers on what code is not acceptable to be added.
> 
> I think this boils down to how our development ecosystem works. Things
> are pretty "voluntary" right now: one could easily ignore zero-day or
> checkpatch all the time, and if a maintainer isn't checking these
> things, a patch will go in.

I have the feeling that the problem is a bit different than what
you're actually describing. It's not so much that we ignore (as in
voluntarily ignore) these new deprecated functions, but more that it's
quite easy to overlook them if you're not following closely the
current effort being done, especially when you don't really have a
security culture.

And then, there's some changes where the benefit is really not obvious
(kmalloc_array comes to my mind).

Polling actively some file isn't going to work, since well, we're all
too busy and lazy to actually do that, but maybe adding
warnings/errors to checkpatch would help, in a similar way than what's
being done for the msleep vs mdelay warning, with a link to the
documentation to explain why that is needed in the first place.

I think that this would help to address the discoverability of those
API changes.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Julia Lawall]

On Fri, 7 Sep 2018, Jani Nikula wrote:

> On Fri, 07 Sep 2018, Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 07 Sep 2018 01:24:03 +0200,
> > Kees Cook wrote:
> >>
> >> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >> > Hi Kees,
> >> >
> >> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
> >> >>
> >> >> If there was an agreement by all maintainers that deprecated
> >> >> functions/patterns should not be added, and we documented the
> >> >> deprecation somewhere like Documentation/process/deprecated.rst, then
> >> >> we could make the declaration that if such functions got added (it's
> >> >> easy to mechanically check for them), it would be the responsibility
> >> >> of the author and maintainer chain to see that it got fixed before the
> >> >> release is cut. We already have this for things like "breaks the x86
> >> >> allmodconfig build" or similar. The checking would be manual, and the
> >> >> enforcement would be by agreement, but it'd be better than the kind of
> >> >> "please don't do this" hand-waving we've had in the past.
> >> >
> >> > I could do this in linux-next, of course, the same way I check for
> >> > missing signed-off-bys.  All I would need is the list of deprecated
> >> > things.
> >>
> >> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> >> favor of strscpy()?
> >
> > How about providing some lightweight check script for git commit hook,
> > and let each maintainer install it?
>
> I looked up 771c035372a0 ("deprecate the '__deprecated' attribute
> warnings entirely and for good"). It's easy to agree that's the right
> thing to do for the regular build.
>
> However, I think there's value in having __deprecated tagged to
> functions. (Note, just that, without defining it as
> __attribute__((deprecated)).) People looking the functions up can see
> they should find alternatives, and people looking for things to do could
> take on the conversion. I don't think a separate deprecated file will
> work. It's all too detached.
>
> Then you can just use -D__deprecated=__attribute__((deprecated)) to get
> the warnings when you like, even on a per module/file basis, or add that
> to W=<some level>, or add that to 0-day or whatever CI, ensuring patches
> aren't adding new warnings.
>
> No need to invent wheels for things where the compiler can help.

I came up with the following Coccinelle semantic patch.  The advantage is
that it can also give a hint as to what should be done.  The intent is
that it should be easily extensible.  At the moment, running in report
mode gives messages like:

drivers/nfc/pn544/i2c.c:543:1-7: Deprecated function strcpy.  Please use
strscpy, which ensures the result is null terminated and returns a negative
error code on overflow

julia

/// Report on calls to deprecated functions
///
// Confidence: High
// Copyright: (C) 2018 Julia Lawall, Inria. GPLv2.
// URL: http://coccinelle.lip6.fr
// Comments: To add new functions, instantiate new_function_name and new_comment.
// Options: --no-includes --include-headers

virtual context
virtual org
virtual report

@initialize:ocaml@
@@

let infos =
  [("strcpy",
    "Please use strscpy, which ensures the result is null terminated and returns a negative error code on overflow");
    ("strlcpy",
     "Please use strscpy, which ensures the result is null terminated and returns a negative error code on overflow");
    ("strncpy",
     "Please use strscpy, which ensures the result is null terminated and returns a negative error code on overflow");
    (* ("new_function_name", "new_comment") *)
  ]

let tbl = Hashtbl.create 101

let _ =
  List.iter (function (nm,comment) -> Hashtbl.add tbl nm comment) infos

// ----------------------------------------------------------------------------

@r depends on context || org || report@
identifier f : script:ocaml() { Hashtbl.mem tbl f };
position j0;
@@

*  f@j0(...)

// ----------------------------------------------------------------------------

@script:ocaml r_org depends on org@
f << r.f;
j0 << r.j0;
@@

let msg = Printf.sprintf "Deprecated function %s.  %s" f (Hashtbl.find tbl f) in
Coccilib.print_main msg j0

// ----------------------------------------------------------------------------

@script:ocaml r_report depends on report@
f << r.f;
j0 << r.j0;
@@

let p = List.hd j0 in
Printf.printf "%s:%d:%d-%d: Deprecated function %s.  %s\n"
  p.file p.line p.col p.col_end f (Hashtbl.find tbl f)

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Julia Lawall]

On Fri, 7 Sep 2018, Johannes Berg wrote:

> On Fri, 2018-09-07 at 09:03 +0200, Takashi Iwai wrote:
> >
> > How about providing some lightweight check script for git commit hook,
> > and let each maintainer install it?
>
> What Julia suggested seems easier: Just drop a script to check for it
> into scripts/coccinelle/, and 0-day build bot should run it
> automatically on pretty much everything, afaict?

I don't know what 0-day runs the scripts on, but my impression is that it
only reports on modified code and that only once.

julia

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Jani Nikula]

On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
> I came up with the following Coccinelle semantic patch.  The advantage is
> that it can also give a hint as to what should be done.  The intent is
> that it should be easily extensible.

The only real downside that I can see is that it centralizes the
deprecation information in the semantic patch instead of the functions
themselves.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Julia Lawall]

On Fri, 7 Sep 2018, Jani Nikula wrote:

> On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > I came up with the following Coccinelle semantic patch.  The advantage is
> > that it can also give a hint as to what should be done.  The intent is
> > that it should be easily extensible.
>
> The only real downside that I can see is that it centralizes the
> deprecation information in the semantic patch instead of the functions
> themselves.

It's possible to put comments in the code near the definition.  There are
a number of occurrences of deprecated already.  People who are just
copying code from something else may not actually look at the definition
of all of the functions they are copying, so there should be some benefit
to having the information somehow attached to uses.

julia

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Dan Carpenter]

On Fri, Sep 07, 2018 at 09:18:42AM +1000, Stephen Rothwell wrote:
> Hi Kees,
> 
> On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> > If there was an agreement by all maintainers that deprecated
> > functions/patterns should not be added, and we documented the
> > deprecation somewhere like Documentation/process/deprecated.rst, then
> > we could make the declaration that if such functions got added (it's
> > easy to mechanically check for them), it would be the responsibility
> > of the author and maintainer chain to see that it got fixed before the
> > release is cut. We already have this for things like "breaks the x86
> > allmodconfig build" or similar. The checking would be manual, and the
> > enforcement would be by agreement, but it'd be better than the kind of
> > "please don't do this" hand-waving we've had in the past.
> 
> I could do this in linux-next, of course, the same way I check for
> missing signed-off-bys.  All I would need is the list of deprecated
> things.
> 

We could just use a simple script.  It wouldn't add build warnings to
the normal build and it could also work for macros.


diff --git a/scripts/check_deprecated.pl b/scripts/check_deprecated.pl
new file mode 100755
index 000000000000..4f5571d0bfde
--- /dev/null
+++ b/scripts/check_deprecated.pl
@@ -0,0 +1,34 @@
+#!/usr/bin/perl
+
+use strict;
+
+open(LIST, "<", "scripts/deprecated_stuff");
+
+my %warnings;
+my $regex = "(";
+my $first = 1;
+while (<LIST>) {
+    if ($_ =~ /(\w+) (.*)/) {
+        if ($first) {
+            $regex = "$regex$1";
+            $first = 0;
+        } else {
+            $regex = "$regex|$1";
+        }
+
+        $warnings{$1} = $2;
+    }
+}
+$regex = "$regex)";
+
+close(LIST);
+
+while (<>) {
+    if (!($_ =~ /^\+/)) {
+        next;
+    }
+
+    if ($_ =~ /\b($regex)\b/) {
+        print $warnings{$1} . "\n";
+    }
+}
diff --git a/scripts/deprecated_stuff b/scripts/deprecated_stuff
new file mode 100644
index 000000000000..01623103aece
--- /dev/null
+++ b/scripts/deprecated_stuff
@@ -0,0 +1,2 @@
+strcpy strcpy is insecure
+blah some reason
-- 
2.11.0

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Alexandre Belloni]

On 07/09/2018 12:54:37+0300, Jani Nikula wrote:
> On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > I came up with the following Coccinelle semantic patch.  The advantage is
> > that it can also give a hint as to what should be done.  The intent is
> > that it should be easily extensible.
> 
> The only real downside that I can see is that it centralizes the
> deprecation information in the semantic patch instead of the functions
> themselves.
> 

Which is not bad because how do you expect maintainers to learn about
the deprecation?
I don't think anyone will go and read the strcpy's code or
documentation.

The same holds true for the introduction of new helpers. There is often
a treewide commit adding its usage that doesn't get send to all the
maintainers so there is zero chance to learn about them. For example the
recent:

0ed2dd03b94b treewide: Use struct_size() for devm_kmalloc() and friends
b4b06db115bb treewide: Use struct_size() for vmalloc()-family
acafe7e30216 treewide: Use struct_size() for kmalloc()-family

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Geert Uytterhoeven]

On Fri, Sep 7, 2018 at 1:19 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
> > If there was an agreement by all maintainers that deprecated
> > functions/patterns should not be added, and we documented the
> > deprecation somewhere like Documentation/process/deprecated.rst, then
> > we could make the declaration that if such functions got added (it's
> > easy to mechanically check for them), it would be the responsibility
> > of the author and maintainer chain to see that it got fixed before the
> > release is cut. We already have this for things like "breaks the x86
> > allmodconfig build" or similar. The checking would be manual, and the
> > enforcement would be by agreement, but it'd be better than the kind of
> > "please don't do this" hand-waving we've had in the past.
>
> I could do this in linux-next, of course, the same way I check for
> missing signed-off-bys.  All I would need is the list of deprecated
> things.

The sooner it is detected, the better.
So that means, in order of decreasing agility:
  1. git commit hook:
       - Needs installation of the hook everywere,
       - Anyone still not using git?
  2. checkpatch:
     - All patch authors should run checkpatch,
     - All maintainers should, too.
  3. coccinelle script et al run by bots:
     - Needs rework when detected (list bot), or fixup (already in next).

Any other options?

So checkpatch looks like the best option to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Jani Nikula]

On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 7 Sep 2018, Jani Nikula wrote:
>
>> On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > I came up with the following Coccinelle semantic patch.  The advantage is
>> > that it can also give a hint as to what should be done.  The intent is
>> > that it should be easily extensible.
>>
>> The only real downside that I can see is that it centralizes the
>> deprecation information in the semantic patch instead of the functions
>> themselves.
>
> It's possible to put comments in the code near the definition.  There are
> a number of occurrences of deprecated already.  People who are just
> copying code from something else may not actually look at the definition
> of all of the functions they are copying, so there should be some benefit
> to having the information somehow attached to uses.

Hmm, generate the semantic patch based on the __deprecated attributes?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On Fri, Sep 07, 2018 at 12:25:19PM +0200, Alexandre Belloni wrote:
> On 07/09/2018 12:54:37+0300, Jani Nikula wrote:

> > The only real downside that I can see is that it centralizes the
> > deprecation information in the semantic patch instead of the functions
> > themselves.

> Which is not bad because how do you expect maintainers to learn about
> the deprecation?
> I don't think anyone will go and read the strcpy's code or
> documentation.

Right, this is the most common pain point I see with these transitions -
people hear about these things through word of mouth which creates
confusion.  This thread is the first time I heard about strscpy()
existing, let alone that the other variants were deprecated in favour of
it.  A similar thing happened with the SPDX tags, people were querying
what was going on with the patches and links to mailing list posts were
getting passed around.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Theodore Y. Ts'o]

On Thu, Sep 06, 2018 at 04:24:03PM -0700, Kees Cook wrote:
> 
> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> favor of strscpy()?

There are some places where I use strncpy for a character array which
is *not* a null-terminated string.  What is the preferred alternative
for me?  I can suppress the problem when gcc complains about it using:

+       __u8    s_first_error_func[32] __nonstring;     /* function where the error happened */

But if we do a blanket deprecation, what should I use instead?

      	       	       		    	 - Ted

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

On Fri, Sep 7, 2018 at 7:33 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Thu, Sep 06, 2018 at 04:24:03PM -0700, Kees Cook wrote:
>>
>> Hopefully we can all agree on deprecating strcpy() and strncpy() in
>> favor of strscpy()?
>
> There are some places where I use strncpy for a character array which
> is *not* a null-terminated string.  What is the preferred alternative
> for me?  I can suppress the problem when gcc complains about it using:
>
> +       __u8    s_first_error_func[32] __nonstring;     /* function where the error happened */
>
> But if we do a blanket deprecation, what should I use instead?

strncpy() is a weird one. I think we can easily say "no strcpy()" but
for strncpy() we need to examine the existing use-cases:

- non-NUL-terminated: use memcpy?
- NEEDS trailing NUL padding: ... no solution yet. invent strscpy_pad() ?
- "safe" strcpy(): use strscpy()

-Kees

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Arnd Bergmann]

On Fri, Sep 7, 2018 at 6:12 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Sep 7, 2018 at 7:33 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> > On Thu, Sep 06, 2018 at 04:24:03PM -0700, Kees Cook wrote:
> >>
> >> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> >> favor of strscpy()?
> >
> > There are some places where I use strncpy for a character array which
> > is *not* a null-terminated string.  What is the preferred alternative
> > for me?  I can suppress the problem when gcc complains about it using:
> >
> > +       __u8    s_first_error_func[32] __nonstring;     /* function where the error happened */
> >
> > But if we do a blanket deprecation, what should I use instead?
>
> strncpy() is a weird one. I think we can easily say "no strcpy()" but
> for strncpy() we need to examine the existing use-cases:
>
> - non-NUL-terminated: use memcpy?
> - NEEDS trailing NUL padding: ... no solution yet. invent strscpy_pad() ?
> - "safe" strcpy(): use strscpy()

I suspect that a lot of the cases that want NUL-padding also don't
want NUL-termination: when you store a string on disk in a fixed-length
record or transfer it over the network, you don't want to leak stack
data to the medium, but you also don't need the terminating character
because you know the maximum length already.

strncpy() does exactly the right thing for that case, it's just that
this pattern is now a corner case, and gcc tends to flag such
usage with a warning about missing termination (unless you
use __nonstring) but doesn't flag the more common usage when
it looks correct.

       Arnd

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Theodore Y. Ts'o]

On Fri, Sep 07, 2018 at 10:30:04PM +0200, Arnd Bergmann wrote:
> 
> I suspect that a lot of the cases that want NUL-padding also don't
> want NUL-termination: when you store a string on disk in a fixed-length
> record or transfer it over the network, you don't want to leak stack
> data to the medium, but you also don't need the terminating character
> because you know the maximum length already.
> 
> strncpy() does exactly the right thing for that case, it's just that
> this pattern is now a corner case, and gcc tends to flag such
> usage with a warning about missing termination (unless you
> use __nonstring) but doesn't flag the more common usage when
> it looks correct.

Yeah, the case I have is when I'm copying from a NUL-terminated string
into a fixed char array.  So if we had a function called
"copy_string_to_char_array" (we'll figure out a better name later)
which takes a source, destination, and size parameter, and which does
the functional equivalent of:

	memset(dest, 0, size);
	strncpy(src, dest, size);

... we could do something that's more efficient than the above, and
does exactly what I'm looking for in this case.

Of course, there could be other corner cases where strncpy() is
justified; this is just the use case I care about.  :-)

     	     	       	       	    - Ted

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Geert Uytterhoeven]

Hi Ted,

On Fri, Sep 7, 2018 at 10:56 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Fri, Sep 07, 2018 at 10:30:04PM +0200, Arnd Bergmann wrote:
> > I suspect that a lot of the cases that want NUL-padding also don't
> > want NUL-termination: when you store a string on disk in a fixed-length
> > record or transfer it over the network, you don't want to leak stack
> > data to the medium, but you also don't need the terminating character
> > because you know the maximum length already.
> >
> > strncpy() does exactly the right thing for that case, it's just that
> > this pattern is now a corner case, and gcc tends to flag such
> > usage with a warning about missing termination (unless you
> > use __nonstring) but doesn't flag the more common usage when
> > it looks correct.
>
> Yeah, the case I have is when I'm copying from a NUL-terminated string
> into a fixed char array.  So if we had a function called
> "copy_string_to_char_array" (we'll figure out a better name later)
> which takes a source, destination, and size parameter, and which does
> the functional equivalent of:
>
>         memset(dest, 0, size);

Why the memset()? strncpy() pads the destination with zeroes, so it
is not needed. This is basically the major reason to use strncpy().

>         strncpy(src, dest, size);

Oh, you're copying in the wrong direction ;-)

> ... we could do something that's more efficient than the above, and
> does exactly what I'm looking for in this case.
>
> Of course, there could be other corner cases where strncpy() is
> justified; this is just the use case I care about.  :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Theodore Y. Ts'o]

On Sat, Sep 08, 2018 at 10:15:01AM +0200, Geert Uytterhoeven wrote:
> Why the memset()? strncpy() pads the destination with zeroes, so it
> is not needed. This is basically the major reason to use strncpy().

I had completely forgotten about that!  I've always focused on when
strncpy would stop copying, and the lack of NUL termination in some
cases; not that it automatically zero'ed the padding.  Since I tend to
always memset the entire structure to be zero, I started assuming that
I couldn't count on strncpy to NUL pad.

Perhaps all we need to do is to rename strncpy to make it clear when
it's appropriate it, and when it simply is not.


> >         strncpy(src, dest, size);
> 
> Oh, you're copying in the wrong direction ;-)

Yeah, oops.  What I get for typing too quickly.  :-)

      	     	    	    	   - Ted

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Mauro Carvalho Chehab]

Em Thu, 6 Sep 2018 16:24:03 -0700
Kees Cook <keescook@chromium.org> escreveu:

> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Kees,
> >
> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:  
> >>
> >> If there was an agreement by all maintainers that deprecated
> >> functions/patterns should not be added, and we documented the
> >> deprecation somewhere like Documentation/process/deprecated.rst, then
> >> we could make the declaration that if such functions got added (it's
> >> easy to mechanically check for them), it would be the responsibility
> >> of the author and maintainer chain to see that it got fixed before the
> >> release is cut. We already have this for things like "breaks the x86
> >> allmodconfig build" or similar. The checking would be manual, and the
> >> enforcement would be by agreement, but it'd be better than the kind of
> >> "please don't do this" hand-waving we've had in the past.  
> >
> > I could do this in linux-next, of course, the same way I check for
> > missing signed-off-bys.  All I would need is the list of deprecated
> > things.  
> 
> Hopefully we can all agree on deprecating strcpy() and strncpy() in
> favor of strscpy()?

I suspect that that's the way to go for most use cases.

In the case of media, I double-checked: 100% of the usages can be
replaced by strscpy() [1]. I just sent a patchset for review with
such changes.

[1] doing a:
	
$ git grep -E "str.?cpy" drivers/media drivers/staging/media/ |grep -v strscpy

after this patchset, we have one case of strncpy_from_user(), 
at av7110:

		if (strncpy_from_user(textbuf, dc->data, 240) < 0) {
			ret = -EFAULT;
			break;
		}
		textbuf[239] = 0;

The above code seems OK, so no real need to change.

Yet, there are 104 occurrences of strncpy_from_user(). If they
all do something similar, it could make sense to have a 
strscpy_from_user() function. If you're willing to do so, 
feel free to also convert av7110 to it.

Thanks,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Mauro Carvalho Chehab]

Em Fri, 07 Sep 2018 12:54:37 +0300
Jani Nikula <jani.nikula@intel.com> escreveu:

> On Fri, 07 Sep 2018, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > I came up with the following Coccinelle semantic patch.  The advantage is
> > that it can also give a hint as to what should be done.  The intent is
> > that it should be easily extensible.  
> 
> The only real downside that I can see is that it centralizes the
> deprecation information in the semantic patch instead of the functions
> themselves.

While both works, the __deprecated markup at the header looks
better for me, as it makes easier to check inside a header file
what's deprecated. Also, it helps avoiding conflicts.

Anyway, if we either use Coccinelle or __deprecated, I doubt that
most people will remember to add a:
	-D__deprecated=__attribute__((deprecated))
or to call a coccinelle script to do such checks, even if properly
documented.

IMHO, whatever solution, the best would be to have a makefile
target equivalent to allmodconfig and allyesconfig that would 
add such extra maintainership logic.



Thanks,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Kees Cook]

On Mon, Sep 10, 2018 at 5:28 AM, Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
> Em Thu, 6 Sep 2018 16:24:03 -0700
> Kees Cook <keescook@chromium.org> escreveu:
>
>> On Thu, Sep 6, 2018 at 4:18 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> > Hi Kees,
>> >
>> > On Thu, 6 Sep 2018 11:24:11 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >>
>> >> If there was an agreement by all maintainers that deprecated
>> >> functions/patterns should not be added, and we documented the
>> >> deprecation somewhere like Documentation/process/deprecated.rst, then
>> >> we could make the declaration that if such functions got added (it's
>> >> easy to mechanically check for them), it would be the responsibility
>> >> of the author and maintainer chain to see that it got fixed before the
>> >> release is cut. We already have this for things like "breaks the x86
>> >> allmodconfig build" or similar. The checking would be manual, and the
>> >> enforcement would be by agreement, but it'd be better than the kind of
>> >> "please don't do this" hand-waving we've had in the past.
>> >
>> > I could do this in linux-next, of course, the same way I check for
>> > missing signed-off-bys.  All I would need is the list of deprecated
>> > things.
>>
>> Hopefully we can all agree on deprecating strcpy() and strncpy() in
>> favor of strscpy()?
>
> I suspect that that's the way to go for most use cases.
>
> In the case of media, I double-checked: 100% of the usages can be
> replaced by strscpy() [1]. I just sent a patchset for review with
> such changes.

Double-check the strncpy() uses: some may depend on the trailing NUL-padding.

> Yet, there are 104 occurrences of strncpy_from_user(). If they
> all do something similar, it could make sense to have a
> strscpy_from_user() function. If you're willing to do so,
> feel free to also convert av7110 to it.

Yeah. I think we need:

strscpy_pad()
memcpy_pad()
strscpy_from_user()

The strncpy uses are:

1- safer strcpy() (usually visible with leading/trailing "buf[size-1]=0")
2- padded strcpy() (to wipe the contents of a destination)
3- copy non-NUL-terminated array of characters

1 should use strscpy
2 needs a strscpy+trailing memset (e.g. strscpy_pad())
3 needs memcpy+trailing memset (e.g. memcpy_pad())

I suggest "memcpy_pad" to very clearly distinguish that it is not
NUL-terminated, but rather NUL-padded.

-Kees

-- 
Kees Cook
Pixel Security

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Jani Nikula]

On Mon, 10 Sep 2018, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> IMHO, whatever solution, the best would be to have a makefile
> target equivalent to allmodconfig and allyesconfig that would 
> add such extra maintainership logic.

There's 'make W=n' for enabling extra gcc checks. Seems like a good fit.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Mauro Carvalho Chehab]

Em Tue, 11 Sep 2018 11:10:22 +0300
Jani Nikula <jani.nikula@intel.com> escreveu:

> On Mon, 10 Sep 2018, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> > IMHO, whatever solution, the best would be to have a makefile
> > target equivalent to allmodconfig and allyesconfig that would 
> > add such extra maintainership logic.  
> 
> There's 'make W=n' for enabling extra gcc checks. Seems like a good fit.

I wouldn't mess with W. On media, we work to have zero warnings with W=1.
However, W=2 (and W=3) is almost unusable: lots of complains about 
signed/unsigned mix and other things that usually are ok. With gcc 8,
the amount of warnings with W=2/W=3 at core kAPI functions makes them
useless.

People will very likely ignore or miss deprecated stuff if we
place it as W=4.

Perhaps we could add a D=1, in order to point for deprecated stuff.

Regards,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] API replacement/deprecation

[Arnd Bergmann]

On Tue, Sep 11, 2018 at 11:35 AM Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> Em Tue, 11 Sep 2018 11:10:22 +0300
> Jani Nikula <jani.nikula@intel.com> escreveu:
>
> > On Mon, 10 Sep 2018, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> > > IMHO, whatever solution, the best would be to have a makefile
> > > target equivalent to allmodconfig and allyesconfig that would
> > > add such extra maintainership logic.
> >
> > There's 'make W=n' for enabling extra gcc checks. Seems like a good fit.
>
> I wouldn't mess with W. On media, we work to have zero warnings with W=1.
> However, W=2 (and W=3) is almost unusable: lots of complains about
> signed/unsigned mix and other things that usually are ok. With gcc 8,
> the amount of warnings with W=2/W=3 at core kAPI functions makes them
> useless.
>
> People will very likely ignore or miss deprecated stuff if we
> place it as W=4.
>
> Perhaps we could add a D=1, in order to point for deprecated stuff.

I don't like the idea of adding another dimension with D=1, but I'm all for
reviewing the sets of warnings we enable at W=2/W=3  and possibly
introducing a W=4.

I've done some research on this in the past, and can probably do that
again (not sure how valuable my old data still is).
https://github.com/Barro/compiler-warnings has a list of warnings
for all compiler releases we support, and some categorizations.

I would like to add a line for each of those warnings in
include/linux/compiler-warnings.h with the compiler version that
introduced it and the warning level we want to have it enabled at,
plus some comment about why that level. We can start with the
status quo (listing the level that each warning is currently
enabled at) and then do changes based on the output of
'make W=123', which I'm sure immediately shows some that
are misclassified based on the number of warnings they produce
for randconfig and allmodconfig builds.

The definitions that I'd use for the levels would be (close to the
current definition)

W=0 (default): -Wall warnings and some -Wextra warnings that
      have zero known false positives
W=1: Most -Wextra warnings, plus everything from -Wall
      that didn't make it into W=0
W=2: clang -Wmost and the the gcc equivalent, as long as
     they are useful in the kernel context. There should be no
     warnings in common header files
W=3: clang -Weverything warnings and the gcc equivalent,
     including very noisy ones, but still only the ones that make
     sense for the kernel.

I think being able to warn for deprecated functions would fit
into W=1 or W=2 if we want to bring that back, though given that
Linus just removed __deprecated with prejudice, I wouldn't
invest any work into that part myself.

        Arnd