linux-kernel - Re: [PATCH v5] staging: greybus: Constify gb_audio_module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ea44d965-e727-4fc4-86cd-b727e1e0d2be@moroto.mountain>
Date: Mon, 18 Mar 2024 14:16:36 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Ayush Tiwari <ayushtiw0110@...il.com>
Cc: Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	linux-staging@...ts.linux.dev, outreachy@...ts.linux.dev
Subject: Re: [PATCH v5] staging: greybus: Constify gb_audio_module_type

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."

regards,
dan carpenter