Re: [PATCH] kasan: update function name in comments

Re: [PATCH] kasan: update function name in comments

[Marco Elver]

On Sat, 19 Feb 2022 at 03:00, Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> On 2022/2/19 9:24, Peter Collingbourne wrote:
> > The function kasan_global_oob was renamed to kasan_global_oob_right,
> > but the comments referring to it were not updated. Do so.
> >
> > Link: https://linux-review.googlesource.com/id/I20faa90126937bbee77d9d44709556c3dd4b40be
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Fixes: e5f4728767d2 ("kasan: test: add globals left-out-of-bounds test")
>
> This Fixes tag is unneeded.
>
> Except the above nit, this patch looks good to me. Thanks.
>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Marco Elver <elver@google.com>

And yes, the Fixes tag should be removed to not have stable teams do
unnecessary work.

+Cc'ing missing mailing lists (use get_maintainers.pl - in particular,
LKML is missing, which should always be Cc'd for archival purposes so
that things like b4 can work properly).

> > ---
> >  lib/test_kasan.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > index 26a5c9007653..a8dfda9b9630 100644
> > --- a/lib/test_kasan.c
> > +++ b/lib/test_kasan.c
> > @@ -780,7 +780,7 @@ static void ksize_uaf(struct kunit *test)
> >  static void kasan_stack_oob(struct kunit *test)
> >  {
> >       char stack_array[10];
> > -     /* See comment in kasan_global_oob. */
> > +     /* See comment in kasan_global_oob_right. */
> >       char *volatile array = stack_array;
> >       char *p = &array[ARRAY_SIZE(stack_array) + OOB_TAG_OFF];
> >
> > @@ -793,7 +793,7 @@ static void kasan_alloca_oob_left(struct kunit *test)
> >  {
> >       volatile int i = 10;
> >       char alloca_array[i];
> > -     /* See comment in kasan_global_oob. */
> > +     /* See comment in kasan_global_oob_right. */
> >       char *volatile array = alloca_array;
> >       char *p = array - 1;
> >
> > @@ -808,7 +808,7 @@ static void kasan_alloca_oob_right(struct kunit *test)
> >  {
> >       volatile int i = 10;
> >       char alloca_array[i];
> > -     /* See comment in kasan_global_oob. */
> > +     /* See comment in kasan_global_oob_right. */
> >       char *volatile array = alloca_array;
> >       char *p = array + i;
> >
> >
>

Re: [PATCH] kasan: update function name in comments

[Peter Collingbourne]

 On Mon, Feb 21, 2022 at 3:15 AM Marco Elver <elver@google.com> wrote:
>
> On Sat, 19 Feb 2022 at 03:00, Miaohe Lin <linmiaohe@huawei.com> wrote:
> >
> > On 2022/2/19 9:24, Peter Collingbourne wrote:
> > > The function kasan_global_oob was renamed to kasan_global_oob_right,
> > > but the comments referring to it were not updated. Do so.
> > >
> > > Link: https://linux-review.googlesource.com/id/I20faa90126937bbee77d9d44709556c3dd4b40be
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > Fixes: e5f4728767d2 ("kasan: test: add globals left-out-of-bounds test")
> >
> > This Fixes tag is unneeded.
> >
> > Except the above nit, this patch looks good to me. Thanks.
> >
> > Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
>
> Reviewed-by: Marco Elver <elver@google.com>
>
> And yes, the Fixes tag should be removed to not have stable teams do
> unnecessary work.

I thought that Cc: stable@vger.kernel.org controlled whether the patch
is to be taken to the stable kernel and Fixes: was more of an
informational tag. At least that's what this seems to say:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight

> +Cc'ing missing mailing lists (use get_maintainers.pl - in particular,
> LKML is missing, which should always be Cc'd for archival purposes so
> that things like b4 can work properly).

get_maintainers.pl tends to list a lot of reviewers so I try to filter
it to only the most important recipients or only use it for
"important" patches (like the uaccess logging patch). It's also a bit
broken in my workflow --
https://lore.kernel.org/all/20210913233435.24585-1-pcc@google.com/
fixes one of the problems but there are others.

Doesn't b4 scan all the mailing lists? So I'd have imagined it
wouldn't matter which one you send it to.

Peter

Re: [PATCH] kasan: update function name in comments

[Marco Elver]

On Wed, 23 Feb 2022 at 23:31, Peter Collingbourne <pcc@google.com> wrote:
[...]
> > > > Link: https://linux-review.googlesource.com/id/I20faa90126937bbee77d9d44709556c3dd4b40be
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > Fixes: e5f4728767d2 ("kasan: test: add globals left-out-of-bounds test")
> > >
> > > This Fixes tag is unneeded.
> > >
> > > Except the above nit, this patch looks good to me. Thanks.
> > >
> > > Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > And yes, the Fixes tag should be removed to not have stable teams do
> > unnecessary work.
>
> I thought that Cc: stable@vger.kernel.org controlled whether the patch
> is to be taken to the stable kernel and Fixes: was more of an
> informational tag. At least that's what this seems to say:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight

These days patches that just have a Fixes tag (and no Cc: stable) will
be auto-picked in many (most?) cases (by empirical observation).

I think there were also tree-specific variances of this policy, but am
not sure anymore. What is the latest policy?

> > +Cc'ing missing mailing lists (use get_maintainers.pl - in particular,
> > LKML is missing, which should always be Cc'd for archival purposes so
> > that things like b4 can work properly).
>
> get_maintainers.pl tends to list a lot of reviewers so I try to filter
> it to only the most important recipients or only use it for
> "important" patches (like the uaccess logging patch). It's also a bit
> broken in my workflow --
> https://lore.kernel.org/all/20210913233435.24585-1-pcc@google.com/
> fixes one of the problems but there are others.

That's fair. It just seemed that something went wrong given
kasan-dev@googlegroups.com wasn't Cc'd. FWIW, syzbot uses
'get_maintainer.pl --git-min-percent=20' which is a bit less
aggressive with Cc'ing folks not mentioned explicitly in MAINTAINERS.

> Doesn't b4 scan all the mailing lists? So I'd have imagined it
> wouldn't matter which one you send it to.

Those under lore.kernel.org or lists.linux.dev. Seems linux-mm does
get redirected to lore: https://lore.kernel.org/linux-mm/ -- It's not
entirely obvious which are lore managed and which aren't (obviously
things like kasan-dev@googlegroups.com aren't).

Re: [PATCH] kasan: update function name in comments

[Andrew Morton]

On Thu, 24 Feb 2022 00:35:32 +0100 Marco Elver <elver@google.com> wrote:

> > I thought that Cc: stable@vger.kernel.org controlled whether the patch
> > is to be taken to the stable kernel and Fixes: was more of an
> > informational tag. At least that's what this seems to say:
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight
> 
> These days patches that just have a Fixes tag (and no Cc: stable) will
> be auto-picked in many (most?) cases (by empirical observation).
> 
> I think there were also tree-specific variances of this policy, but am
> not sure anymore. What is the latest policy?

The -stable maintainers have been asked not to do that for MM patches -
to only take those which the developers (usually I) have explicitly tagged
for backporting.

I don't know how rigorously this is being followed.  Probably OK for
patches to mm/* but if it's drivers/base/node.c then heaven knows.