| Message ID | 20230922175050.work.819-kees@kernel.org |
|---|---|
| State | Accepted |
| Commit | 59825951707eccf92782e109c04772d34fc07eb6 |
| Headers | show |
| Series | ASoC: apple: mca: Annotate struct mca_data with __counted_by | expand |
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 >
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.
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
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.
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!
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
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)
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(-)