Messages in this thread | | | From | (Eric W. Biederman) | Date | Thu, 06 May 2021 12:51:05 -0500 | Subject | Re: [PATCH] checkpatch: warn about direct use of send_sig_info and force_sig_info |
| |
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
| |