Re: [PATCH v2 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

Re: [PATCH v2 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

[Thomas Weißschuh]

(+Cc LKML to at least get the conversation into the archives)

Hi Joel,

On 2024-04-16 18:18:19+0200, Joel Granados wrote:
> On Mon, Apr 08, 2024 at 04:21:58PM +0200, Thomas Weißschuh wrote:
> > On 2024-04-08 10:59:28+0200, Joel Granados wrote:
> > > On Tue, Apr 02, 2024 at 07:15:37PM +0200, Thomas Weißschuh wrote:
> > > > On 2024-03-28 22:00:16+0100, Joel Granados wrote:
> > > > > On Sat, Mar 23, 2024 at 04:44:08PM +0100, Thomas Weißschuh wrote:
> > > > > > * Patch 1 is a bugfix for the stack_erasing sysctl handler
> > > > > > * Patches 2-10 change various helper functions throughout the kernel to
> > > > > >   be able to handle 'const ctl_table'.
> > > > > > * Patch 11 changes the signatures of all proc handlers through the tree.
> > > > > >   Some other signatures are also adapted, for details see the commit
> > > > > >   message.
> > > > > > 
> > > > > > Only patch 1 changes any code at all.
> > > > > > 
> > > > > > The series was compile-tested on top of next-20230322 for
> > > > > > i386, x86_64, arm, arm64, riscv, loongarch and s390.
> > > > > > 
> > > > > > This series was split from my larger series sysctl-const series [0].
> > > > > > It only focusses on the proc_handlers but is an important step to be
> > > > > > able to move all static definitions of ctl_table into .rodata.
> > > > > > 
> > > > > > [0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/
> > > > > > 
> > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Reduce recipient list
> > > > > How did you reduce it? and why did you reduce it? This is quite an
> > > > > extensive change; if anything we should have lots of eyes on it.
> > > > 
> > > > I used get_maintainer.pl for v1 and received negative feedback about
> > > > sending the series to too many people.
> > > I completely missed that. Was that a private mail to you? Do you have a
> > > link of the message? I checked my history and I do not have it.
> > 
> > Yes that message was private. From a fairly prominent maintainer.
> > 
> > > > Advice I got was to only send it to the people whose tree it will be
> > > > going through.
> > > > 
> > > > The only change affecting actual emitted object code in the series is
> > > > "stackleak: don't modify ctl_table argument" and that got acked by Kees
> > > > already quite some time ago.
> > > > If Kees wants to pick this up for one of his own PRs during this cycle
> > > > that would be nice, too.
> > > > 
> > > > I'm open for suggestions to increase the circle of recipients, but
> > > > blindly using get_maintainer.pl again doesn't seem the right way.
> > 
> > > IMO, this can go either way: You can get feedback that tells you that it
> > > is too many people that you are "bothering" and you can also get
> > > maintainers pinging you to be included in what they consider to be part
> > > of their job. You can't make everyone happy :)
> > 
> > Absolutely, this is my expectation, too.
> > 
> > But maintainers can nicely opt-in to all patches touching their
> > subsystems using lore and lei, while it's harder to opt-out of broad
> > recipient lists.
> > (Except putting me in their killfile...)

> Not sure how possible it is to be individually black listed in a
> maintainers inbox. But it is more of a reason to make it public; because
> at least you have done your due diligence and made it available so every
> one can see what is going on (even if you are black listed
> individually).

The reference to the killfile was more meant as a joke.

> > > > The only real feedback I got was from Dave and that I addressed.
> > > yes. I saw this.
> > > 
> > > > If somebody would have had fundamental issues with the aproach they
> > > > could have spoken up on v1.
> > > This seems reasonable. I'll try to get to it this week and add it to
> > > constfy branch if I do not see anything glaring.
> 
> I Just realized that you did not include any kernel lists. I thought
> that you had just removed individuals and left the lists. This reduces
> the number of eyes on the code which is particularly important in this
> specific case where there are so many changes touching so many
> subsystems.

This is absolutely an oversight, I intended to at least keep LKML.
I have to assume it was me being inattentive.

> Not only that, but it also breaks tools like lei and b4. I have configured b4 to
> look at https://lore.kernel.org/all to handle patches coming from contributors.
> If the change is not public it breaks my command (`b4 am -o - MESSAGE_ID | git
> am -3`).

Understood, as mentioned above the trimming went to far.

FYI:
b4 can do the `git am` itself with `b4 shazam MESSAGE_ID`.
Use the config `b4.shazam-am-flags` for the `-3` flag.

> I do not know who spooked you but I suggest you just remove this person
> from the to:/cc: of your patches and leave the rest as it is. Like I
> did with Mathew Wilcox after he asked me to do so here
> https://lore.kernel.org/all/ZZbJRiN8ENV%2FFoTV@casper.infradead.org/.
> Please resend the patchset including the relevant kernel mailing lists and
> maintainers but excepting the person that sent you the private e-mail.

In addition to the complaint I also got guidance from Thomas Gleixner to
reduce the scope of recipients.

What do you think about the following:

You do a review of v2 and give feedback on that and I'll incorporate
that feedback and afterwards send a v3.
In addition to the recipients of v2 I'll add LKML, Greg and Andrew Morton.
 
> This also goes for your "[PATCH] sysctl: treewide: constify ctl_table_header::ctl_table_arg"
> which is also not public.

Thanks for this pointer, too.
I'd like to handle it the same way as proposed above.


Sorry for all the back-and-forth,
Thomas

Re: [PATCH v2 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

[Joel Granados]

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

On Tue, Apr 16, 2024 at 09:32:24PM +0200, Thomas Weißschuh wrote:
> (+Cc LKML to at least get the conversation into the archives)
> 
> Hi Joel,
...
> 
> > Not only that, but it also breaks tools like lei and b4. I have configured b4 to
> > look at https://lore.kernel.org/all to handle patches coming from contributors.
> > If the change is not public it breaks my command (`b4 am -o - MESSAGE_ID | git
> > am -3`).
> 
> Understood, as mentioned above the trimming went to far.
> 
> FYI:
> b4 can do the `git am` itself with `b4 shazam MESSAGE_ID`.
> Use the config `b4.shazam-am-flags` for the `-3` flag.

Thx for the tip.

> 
> > I do not know who spooked you but I suggest you just remove this person
> > from the to:/cc: of your patches and leave the rest as it is. Like I
> > did with Mathew Wilcox after he asked me to do so here
> > https://lore.kernel.org/all/ZZbJRiN8ENV%2FFoTV@casper.infradead.org/.
> > Please resend the patchset including the relevant kernel mailing lists and
> > maintainers but excepting the person that sent you the private e-mail.
> 
> In addition to the complaint I also got guidance from Thomas Gleixner to
> reduce the scope of recipients.
> 
> What do you think about the following:
> 
> You do a review of v2 and give feedback on that and I'll incorporate
> that feedback and afterwards send a v3.
This will *not* work and this is why:
1. I will not be able to use b4 to gather SOBs (and the like) because
   there is no original mail
2. The review would be confusing in public as the original message will
   not have existed.
3. I will most likely forget to add relevant recepients.

Please resend the patch. `b4 send --resend [vN]` should work after you
have added the additional CCs.

> In addition to the recipients of v2 I'll add LKML, Greg and Andrew Morton.
It will just get lost in LKML. If you are *not* going to add all the
maintainers themselves, at least add the relevant kernel lists. These
are the lists that I propose (from running get_maintainers.pl on your
11/11 patch):

linux-fsdevel@vger.kernel.org
netdev@vger.kernel.org
linux-arm-kernel@lists.infradead.org
linux-s390@vger.kernel.org
linux-kernel@vger.kernel.org
linux-riscv@lists.infradead.org
linux-mm@kvack.org
linux-security-module@vger.kernel.org
bpf@vger.kernel.org
linuxppc-dev@lists.ozlabs.org
linux-xfs@vger.kernel.org
linux-trace-kernel@vger.kernel.org
linux-perf-users@vger.kernel.org
netfilter-devel@vger.kernel.org
coreteam@netfilter.org
kexec@lists.infradead.org
linux-hardening@vger.kernel.org
bridge@lists.linux.dev
lvs-devel@vger.kernel.org
linux-rdma@vger.kernel.org
rds-devel@oss.oracle.com
linux-sctp@vger.kernel.org
linux-nfs@vger.kernel.org
apparmor@lists.ubuntu.com

Its a long list because that patch touches everything :)


>  
> > This also goes for your "[PATCH] sysctl: treewide: constify ctl_table_header::ctl_table_arg"
> > which is also not public.
> 
> Thanks for this pointer, too.
> I'd like to handle it the same way as proposed above.
> 
> 
> Sorry for all the back-and-forth,
> Thomas

Best

-- 

Joel Granados

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