Re: [PATCH] checkpatch: warn about direct use of send_sig_info and force_sig_info

[ebiederm@xmission.com (Eric W. Biederman)]

Marco Elver <elver@google.com> writes:

> On Thu, 6 May 2021 at 18:02, Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>> On Thu, May 6, 2021 at 5:02 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>> > On Thu, May 6, 2021 at 6:59 PM Marco Elver <elver@google.com> wrote:
>> > >
>> > > Setting up siginfo and using send_sig_info() and force_sig_info()
>> > > directly is discouraged. Instead, new code wanting to generate signals
>> > > should use the appropriate helper specific to the signal.
>> > >
>> > > Eric mentioned that he'd like to make these static at some point, but
>> > > until that can happen, let's try to avoid introducing new users of them.
>> > >
>> > > Cc: Eric W. Biederman <ebiederm@xmission.com>
>> > > Signed-off-by: Marco Elver <elver@google.com>
>> > > ---
>> > > Eric,
>> > >
>> > > While siginfo doesn't need changing often, when it does, it's quite the
>> > > adventure. We now have the various static asserts. The other thing is
>> > > usage of {send,force}_sig_info.
>> > >
>> > > I think the best option right now is to teach checkpatch.pl about it
>> > > until they become static.
>> > >
>> > > Fyi, I noticed one such new user here:
>> > > https://lkml.kernel.org/r/20210421024826.13529-1-wangjunqiang@iscas.ac.cn
>> > >
>> > > Thanks,
>> > > -- Marco
>> > > ---
>> > >  scripts/checkpatch.pl | 6 ++++++
>> > >  1 file changed, 6 insertions(+)
>> > >
>> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> > > index ccb412a74725..3a86aafc3bcd 100755
>> > > --- a/scripts/checkpatch.pl
>> > > +++ b/scripts/checkpatch.pl
>> > > @@ -7153,6 +7153,12 @@ sub process {
>> > >                              "Where possible, use lockdep_assert_held instead of assertions based on spin_is_locked\n" . $herecurr);
>> > >                 }
>> > >
>> > > +# check for direct use of send_sig_info(), force_sig_info()
>> > > +               if ($line =~ /\b((force|send)_sig_info)\(/) {
>> >
>> > I think this might be a little better as:
>> > if ($line =~ /\b((?:force|send)_sig_info)\(/) {
>> >
>> > Otherwise it's good as it is.
>> > Tested-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>> >
>>
>> Dwaipayan, do you want to also document this new rule on the
>> checkpatch documentation?
>> Marco, maybe you can assist us here with some pointer (lore.kernel.org
>> link) to the original discussion you had.
>
> It started somewhere here:
> https://lkml.kernel.org/r/m17dkjttpj.fsf@fess.ebiederm.org
>
> Eric has the full history here -- if I missed something, hopefully
> he'll nack or ack.

The practical problem is that siginfo_t is a complicated union.

Having fixed many many cases of this there is a very high probability in
making a mistake in filling siginfo_t.  Perhaps 1 in 10 times someone
fills out a siginfo_t manually.  So helpers that take just the
information that is intended to be in the structure as parameters and
fill in that information explicitly are a tremendous help, and let
developers when calling them focus on their actual development.

This all a very slow moving process and we don't have many call sites
for any kind of exception generating signals happen quickly.   Maybe one
or two a year.

I don't mind a checkpatch warning.  But making force_sig_info and
send_sig_info static is probably the better long term approach so people
simply don't have a problematic interface to call.

I will add that the ongoing addtion of SIGTRAP TRAP_PERF is currently
misusing si_errnno in Linus's tree.  We are reviewing and double
checking the fixes now.  Which is pretty much where this conversation
started this time around.

Eric