[PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Kees Cook]

Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).

As found with Coccinelle[1], add __counted_by for struct mca_data.

[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci

Cc: "Martin Povišer" <povik+lin@cutebit.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: asahi@lists.linux.dev
Cc: alsa-devel@alsa-project.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 sound/soc/apple/mca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/apple/mca.c b/sound/soc/apple/mca.c
index ce77934f3eef..99e547ef95e6 100644
--- a/sound/soc/apple/mca.c
+++ b/sound/soc/apple/mca.c
@@ -161,7 +161,7 @@ struct mca_data {
 	struct mutex port_mutex;
 
 	int nclusters;
-	struct mca_cluster clusters[];
+	struct mca_cluster clusters[] __counted_by(nclusters);
 };
 
 static void mca_modify(struct mca_cluster *cl, int regoffset, u32 mask, u32 val)
-- 
2.34.1

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Gustavo A. R. Silva]

On 9/22/23 11:50, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct mca_data.
> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> 
> Cc: "Martin Povišer" <povik+lin@cutebit.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: asahi@lists.linux.dev
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
-- 
Gustavo

> ---
>   sound/soc/apple/mca.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/apple/mca.c b/sound/soc/apple/mca.c
> index ce77934f3eef..99e547ef95e6 100644
> --- a/sound/soc/apple/mca.c
> +++ b/sound/soc/apple/mca.c
> @@ -161,7 +161,7 @@ struct mca_data {
>   	struct mutex port_mutex;
>   
>   	int nclusters;
> -	struct mca_cluster clusters[];
> +	struct mca_cluster clusters[] __counted_by(nclusters);
>   };
>   
>   static void mca_modify(struct mca_cluster *cl, int regoffset, u32 mask, u32 val)

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Kees Cook]

On Fri, Sep 22, 2023 at 10:50:50AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct mca_data.

Friendly ping. Mark, can you pick this up please?

Thanks!

-Kees

> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> 
> Cc: "Martin Povišer" <povik+lin@cutebit.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: asahi@lists.linux.dev
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  sound/soc/apple/mca.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/apple/mca.c b/sound/soc/apple/mca.c
> index ce77934f3eef..99e547ef95e6 100644
> --- a/sound/soc/apple/mca.c
> +++ b/sound/soc/apple/mca.c
> @@ -161,7 +161,7 @@ struct mca_data {
>  	struct mutex port_mutex;
>  
>  	int nclusters;
> -	struct mca_cluster clusters[];
> +	struct mca_cluster clusters[] __counted_by(nclusters);
>  };
>  
>  static void mca_modify(struct mca_cluster *cl, int regoffset, u32 mask, u32 val)
> -- 
> 2.34.1
> 

-- 
Kees Cook

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Mark Brown]

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

On Fri, Oct 06, 2023 at 01:22:55PM -0700, Kees Cook wrote:
> On Fri, Sep 22, 2023 at 10:50:50AM -0700, Kees Cook wrote:

> > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > attribute. Flexible array members annotated with __counted_by can have
> > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > functions).
> > 
> > As found with Coccinelle[1], add __counted_by for struct mca_data.
> 
> Friendly ping. Mark, can you pick this up please?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

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

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Kees Cook]

On Fri, Oct 06, 2023 at 09:53:49PM +0100, Mark Brown wrote:
> On Fri, Oct 06, 2023 at 01:22:55PM -0700, Kees Cook wrote:
> > On Fri, Sep 22, 2023 at 10:50:50AM -0700, Kees Cook wrote:
> 
> > > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > > attribute. Flexible array members annotated with __counted_by can have
> > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > > functions).
> > > 
> > > As found with Coccinelle[1], add __counted_by for struct mca_data.
> > 
> > Friendly ping. Mark, can you pick this up please?
> 
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so 
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be addressed.
> 
> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, so sending again is generally a better approach though there are
> some other maintainers who like them - if in doubt look at how patches
> for the subsystem are normally handled.

I'm happy to do whatever you'd like for this kind of thing, but I'm
annoyed by this likely automated response seems to ask for the things
that have already happened or generally don't make sense. :P

- It _has_ been 2 weeks.
- Review comments have _not_ required changes.
- Sending a no-change patch is just as much email as sending a ping.
- It's not content-free: I'm asking if you're going to take it;
  patches have gotten lost in the past, so it's a valid question.
- I'm not interested in other subsystems, I'm interested in yours. :P

You've made it clear you don't want me to pick up these kinds of trivial
patches that would normally go through your tree, so I'm left waiting
with no indication if you've seen the patch.

My normal routine with treewide changes is to pick up trivial stuff that
has gotten review but the traditional maintainer hasn't responded to
in 2 weeks.

Do you want these kinds of patches to be re-sent every 2 weeks if they
haven't been replied to by you?

-Kees

-- 
Kees Cook

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Mark Brown]

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

On Mon, Oct 09, 2023 at 10:17:33AM -0700, Kees Cook wrote:
> On Fri, Oct 06, 2023 at 09:53:49PM +0100, Mark Brown wrote:

> > Please don't send content free pings and please allow a reasonable time
> > for review.  People get busy, go on holiday, attend conferences and so 
> > on so unless there is some reason for urgency (like critical bug fixes)
> > please allow at least a couple of weeks for review.  If there have been
> > review comments then people may be waiting for those to be addressed.

> I'm happy to do whatever you'd like for this kind of thing, but I'm
> annoyed by this likely automated response seems to ask for the things
> that have already happened or generally don't make sense. :P

It's a form letter so not quite automated but sure.  Since it's the same
form letter I send for all these pings it covers a bunch of things that
might not apply in each individual case.

> - It _has_ been 2 weeks.

That's *at least* two weeks.  For a non-urgent change like this I'd
generally go with longer than that, for example I'd originally had these
changes queued for -rc5 to give the driver maintainers a couple of weeks
to look at them (my scripting understands -rcs more than dates so you'll
see more patches going in on Mondays).  

> - Review comments have _not_ required changes.
> - Sending a no-change patch is just as much email as sending a ping.

A no-change patch is directly and readily actionable, a ping typically
requires going and digging out the original mail or sending a reply
asking for a resend.

> - It's not content-free: I'm asking if you're going to take it;
>   patches have gotten lost in the past, so it's a valid question.

That is not something I can meaningfully distinguish from being content
free, it provides no new information.  Something with content would be
for example information about dependencies progressing.

> - I'm not interested in other subsystems, I'm interested in yours. :P

> You've made it clear you don't want me to pick up these kinds of trivial
> patches that would normally go through your tree, so I'm left waiting
> with no indication if you've seen the patch.

Sure, but that seems fairly normal for the kernel - when sending this
sort of stuff myself I'd be leaving it more like a month before I got
particularly worried.  One way or another it seems fairly common for
things to be left for at least a couple of weeks with things like
waiting for review, restrictions on when patches actually get applied
and just people being busy or whatever.

Personally for incoming patches when I'm leaving time for driver
maintainers I tend to go for leaving things for a -rc or two - things
like who's involved, how early it is in the week when the original patch
gets sent and how late in the release cycle we are will factor in there.
More urgent things like fixes will tend to go faster, minor stuff that
just needs to be handled sometime before the next release will tend to
be slower.

I don't send out mails saying that I've reviewed and queued things
before actually applying them since doing that tends to discourage other
people from doing review and I'd rather they did, this means I don't
generally send out entirely positive review comments prior to applying
anything unless I'm actively chasing for feedback from someone.  It can
also be a bit confusing for people if I tell them something is OK then
later run into test issues.

> My normal routine with treewide changes is to pick up trivial stuff that
> has gotten review but the traditional maintainer hasn't responded to
> in 2 weeks.

> Do you want these kinds of patches to be re-sent every 2 weeks if they
> haven't been replied to by you?

No, please leave it longer - that's the main thing here, you're not
leaving adequate time for non-urgent patches like this.  If you leave it
two weeks for maintainer review and I also leave it two weeks for
maintainer review then we will both expire the timers at the same time
and we're going to trample over each other.  For me it will typically be
a bit more or less than two weeks rather than two weeks to the day but
IIRC the time you applied something it was while the patch was actually
running through my CI.

Off the top of my head I'd say wait at least three weeks for this sort
of patch before doing anything and then prefer to do a resend, that's
should avoid most issues.  If you're going to just apply things yourself
I'd suggest waiting for -rc6 or so before doing so (assuming the patches
were initially sent reasonably early), that does seem like a reasonable
backstop so things don't completely miss releases.

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

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Kees Cook]

On Mon, Oct 09, 2023 at 08:43:44PM +0100, Mark Brown wrote:
> Off the top of my head I'd say wait at least three weeks for this sort
> of patch before doing anything and then prefer to do a resend, that's
> should avoid most issues.  If you're going to just apply things yourself
> I'd suggest waiting for -rc6 or so before doing so (assuming the patches
> were initially sent reasonably early), that does seem like a reasonable
> backstop so things don't completely miss releases.

Okay, sounds good. Thanks for the clarification!

-- 
Kees Cook

Re: [PATCH] ASoC: apple: mca: Annotate struct mca_data with __counted_by

[Mark Brown]

On Fri, 22 Sep 2023 10:50:50 -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> As found with Coccinelle[1], add __counted_by for struct mca_data.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: apple: mca: Annotate struct mca_data with __counted_by
      commit: 59825951707eccf92782e109c04772d34fc07eb6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark