On Mon, Mar 18, 2024 at 12:25:55PM +0100, Julia Lawall wrote:
>
>
> On Mon, 18 Mar 2024, Dan Carpenter wrote:
>
> > On Sat, Mar 16, 2024 at 11:54:21PM +0530, Ayush Tiwari wrote:
> > > Constify static struct kobj_type gb_audio_module_type to prevent
> > > modification of data shared across many instances(instances here
> > > refer to multiple kobject instances being created, since this same
> > > gb_audio_module_type structure is used as a template for all audio
> > > manager module kobjs, it is effectively 'shared' across all these
> > > instances), ensuring that the structure's usage is consistent and
> > > predictable throughout the driver and allowing the compiler to place
> > > it in read-only memory. The gb_audio_module_type structure is used
> > > when initializing and adding kobj instances to the kernel's object
> > > hierarchy. After adding const, any attempt to alter
> > > gb_audio_module_type in the code would raise a compile-time error.
> > > This enforcement ensures that the sysfs interface and operations for
> > > audio modules remain stable.
> > >
> >
> > Basically the patch is fine. The only comments have been around the
> > commit message. And all the reviewers have said correct things... But
> > I'm still going to chime in as well.
> >
> > The commit message is too long for something very simple.
> >
> > Basically all kernel maintainers understand about constness. There is
> > sometimes trickiness around constness but in this specific case there
> > isn't anything subtle or interesting. You don't need to explain about
> > constness. Maybe you can say the word "hardenning" as an explanation.
> >
> > Julia asked you to write what steps you had done to ensure that the
> > patch doesn't break anything. And I was curious what she meant by that
> > because I had forgotten that it would be bad if there were a cast that
> > removed the const. So the bit about "any attempt to alter
> > gb_audio_module_type in the code would raise a compile-time error." is
> > not true.
> >
> > Also we assume that you have compile tested everything so you never need
> > to write that.
> >
> > The information which is missing from this commit message is the
> > checkpatch warning. I'm more familiar with checkpatch than a lot of
> > kernel maintainers and I had forgotten that this was a checkpatch
> > warning. Please copy and paste the warning.
> >
> > Basically what I want in a commit message is this:
> >
> > "Checkpatch complains that "gb_audio_module_type" should be const as
> > part of kernel hardenning. <Copy and paste relevant bits from
> > checkpatch>. I have reviewed how this struct is used and it's never
> > modified anywhere so checkpatch is correct that it can safely be
> > declared as const. Constify it."
>
> I would still prefer to see that the structure is only passed to a certain
> function, and that function only uses the structure in a certain way (fill
> in the exact details). In this case, one may just rely on the fact that
> the parameter that receives the value is also declared as const, or one
> could check that that const declaration is actually correct. I think that
> is what Ayush was trying to do, although in a somewhat verbose way. This
> case is a bit more complex than many others, because the structure isn't
> used locally, but is passed off to some other function.
Huh. Yeah. That's almost certainly how it was intended to be read.
I apologize. Maybe something like the following:
The "gb_audio_module_type" struct is only used in one place:
err = kobject_init_and_add(&m->kobj, &gb_audio_module_type, NULL, ...
so checkpatch is correct that it can be made const.
That kind of commit message wouldn't work if the struct was used a lot
but it works here.
regards,
dan carpenter
