Hi Kees,
On Mon, Sep 25, 2023 at 10:20:41AM -0700, Kees Cook wrote:
> Since __counted_by annotations may also require that code be changed to
> get initialization ordering correct, let's get an extra group of eyes on
> code that is working on these annotations.
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 737dcc7a2155..741285b8246e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11405,6 +11405,7 @@ F: kernel/configs/hardening.config
> F: mm/usercopy.c
> K: \b(add|choose)_random_kstack_offset\b
> K: \b__check_(object_size|heap_object)\b
> +K: \b__counted_by\b
>
Are you sure you want to volunteer to maintain every file that contains
"__counted_by"? That's what "K" does; get_maintainer.pl will list you (and
[email protected]) for every such file.
Other users of "K" have been surprised by this behavior. It seems that most
people expect it to only apply to patches, not to files. Given that you're
interested in using this functionality, have you considered updating
checkpatch.pl to handle it in the way that you probably expect that it works?
- Eric
On Tue, Sep 26, 2023 at 1:57 PM Eric Biggers <[email protected]> wrote:
>
> Hi Kees,
>
> On Mon, Sep 25, 2023 at 10:20:41AM -0700, Kees Cook wrote:
> > Since __counted_by annotations may also require that code be changed to
> > get initialization ordering correct, let's get an extra group of eyes on
> > code that is working on these annotations.
> >
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 737dcc7a2155..741285b8246e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11405,6 +11405,7 @@ F: kernel/configs/hardening.config
> > F: mm/usercopy.c
> > K: \b(add|choose)_random_kstack_offset\b
> > K: \b__check_(object_size|heap_object)\b
> > +K: \b__counted_by\b
> >
>
> Are you sure you want to volunteer to maintain every file that contains
> "__counted_by"? That's what "K" does; get_maintainer.pl will list you (and
> [email protected]) for every such file.
Do people call get_maintainer.pl on specific tree files as opposed to
invoking it against a .patch file? In the event of the .patch file
"K:" should only pick-up what's in the patch and not read into the
files outside of the context that the diff provides.
If needed, I could send a patch adding a "D:" which would only
consider patches and not tree files -- reducing noise.
>
> Other users of "K" have been surprised by this behavior. It seems that most
> people expect it to only apply to patches, not to files. Given that you're
> interested in using this functionality, have you considered updating
> checkpatch.pl to handle it in the way that you probably expect that it works?
>
> - Eric
>
Thanks
Justin
On Tue, Sep 26, 2023 at 5:35 PM Justin Stitt <[email protected]> wrote:
>
> On Tue, Sep 26, 2023 at 1:57 PM Eric Biggers <[email protected]> wrote:
> >
> > Hi Kees,
> >
> > On Mon, Sep 25, 2023 at 10:20:41AM -0700, Kees Cook wrote:
> > > Since __counted_by annotations may also require that code be changed to
> > > get initialization ordering correct, let's get an extra group of eyes on
> > > code that is working on these annotations.
> > >
> > > Signed-off-by: Kees Cook <[email protected]>
> > > ---
> > > MAINTAINERS | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 737dcc7a2155..741285b8246e 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -11405,6 +11405,7 @@ F: kernel/configs/hardening.config
> > > F: mm/usercopy.c
> > > K: \b(add|choose)_random_kstack_offset\b
> > > K: \b__check_(object_size|heap_object)\b
> > > +K: \b__counted_by\b
> > >
> >
> > Are you sure you want to volunteer to maintain every file that contains
> > "__counted_by"? That's what "K" does; get_maintainer.pl will list you (and
> > [email protected]) for every such file.
>
> Do people call get_maintainer.pl on specific tree files as opposed to
> invoking it against a .patch file? In the event of the .patch file
> "K:" should only pick-up what's in the patch and not read into the
> files outside of the context that the diff provides.
FWIW, b4 just uses the patches and not entire files:
...
try:
tos, ccs, tag_msg, patches = get_prep_branch_as_patches()
except RuntimeError:
logger.info('No commits in branch')
return
logger.info('Collecting To/Cc addresses')
# Go through the messages to make to/cc headers
for commit, msg in patches:
if not msg or not commit:
continue
logger.debug('Collecting from: %s', msg.get('subject'))
msgbytes = msg.as_bytes()
...
>
> If needed, I could send a patch adding a "D:" which would only
> consider patches and not tree files -- reducing noise.
>
> >
> > Other users of "K" have been surprised by this behavior. It seems that most
> > people expect it to only apply to patches, not to files. Given that you're
> > interested in using this functionality, have you considered updating
> > checkpatch.pl to handle it in the way that you probably expect that it works?
> >
> > - Eric
> >
>
> Thanks
> Justin