[GIT PULL] sysctl constification changes for v6.11-rc1
Linus
Constifying ctl_table structs will prevent the modification of
proc_handler function pointers as they would reside in .rodata. To get
there, the proc_handler arguments must first be const qualified which
requires this (fairly large) treewide PR. Sending it in the tail end of
of the merge window after a suggestion from Kees to avoid unneeded merge
conflicts. It has been rebased on top of
7a3fad30fd8b4b5e370906b3c554f64026f56c2f.
I can send it later if it makes more sense on your side; please tell me
what you prefer.
This PR applies on top of what I see as your latest master, but if you
need to generate it, you can do so by executing two commands:
1. Semantic patch: The coccinelle script is here [1]
`make coccicheck MODE=patch SPFLAGS="--in-place --include-headers
--smpl-spacing" COCCI=COCCI_SCRIPT`
2. Sed command: The sed script is here [2]
`sed --in-place -f SED_SCRIPT fs/xfs/xfs_sysctl.c kernel/watchdog.c`
This is my first time sending out a semantic patch, so get back to me if
you have issues or prefer some other way of receiving it.
Testing was done in sysctl-testing (0-day) to avoid generating
unnecessary merge conflicts in linux-next. I do not expect any
error/regression given that all changes contained in this PR are
non-functional.
[1]
```
virtual patch
@r1@
identifier ctl, write, buffer, lenp, ppos;
identifier func !~
"appldata_(timer|interval)_handler|sched_(rt|rr)_handler|rds_tcp_skbuf_handler|proc_sctp_do_(hmac_alg|rto_min|rto_max|udp_port|alpha_beta|auth|probe_interval)";
@@
int func(
- struct ctl_table *ctl
+ const struct ctl_table *ctl
,int write, void *buffer, size_t *lenp, loff_t *ppos);
@r2@
identifier func, ctl, write, buffer, lenp, ppos;
@@
int func(
- struct ctl_table *ctl
+ const struct ctl_table *ctl
,int write, void *buffer, size_t *lenp, loff_t *ppos)
{ ... }
@r3@
identifier func;
@@
int func(
- struct ctl_table *
+ const struct ctl_table *
,int , void *, size_t *, loff_t *);
@r4@
identifier func, ctl;
@@
int func(
- struct ctl_table *ctl
+ const struct ctl_table *ctl
,int , void *, size_t *, loff_t *);
@r5@
identifier func, write, buffer, lenp, ppos;
@@
int func(
- struct ctl_table *
+ const struct ctl_table *
,int write, void *buffer, size_t *lenp, loff_t *ppos);
```
[2]
```
s/^xfs_stats_clear_proc_handler(const struct ctl_table
\*ctl,$/xfs_stats_clear_proc_handler(\
\tconst struct ctl_table\t*ctl,/
s/^xfs_panic_mask_proc_handler(const struct ctl_table
\*ctl,$/xfs_panic_mask_proc_handler(\
\tconst struct ctl_table\t*ctl,/
s/^xfs_deprecated_dointvec_minmax(const struct ctl_table
\*ctl,$/xfs_deprecated_dointvec_minmax(\
\tconst struct ctl_table\t*ctl,/
s/proc_watchdog_common(int which, struct ctl_table
\*table/proc_watchdog_common(int which, const struct ctl_table *table/
```
The following changes since commit 7a3fad30fd8b4b5e370906b3c554f64026f56c2f:
Merge tag 'random-6.11-rc1-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/crng/random (2024-07-24 10:29:50
-0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/
tags/constfy-sysctl-6.11-rc1
for you to fetch changes up to 78eb4ea25cd5fdbdae7eb9fdf87b99195ff67508:
sysctl: treewide: constify the ctl_table argument of proc_handlers
(2024-07-24 20:59:29 +0200)
sysctl: treewide: constify the ctl_table argument of proc_handlers
Summary
- const qualify struct ctl_table args in proc_handlers:
This is a prerequisite to moving the static ctl_table structs into .rodata
data which will ensure that proc_handler function pointers cannot be
modified.
----
Joel Granados (1):
sysctl: treewide: constify the ctl_table argument of proc_handlers
arch/arm64/kernel/armv8_deprecated.c | 2 +-
arch/arm64/kernel/fpsimd.c| 2 +-
arch/s390/appldata/appldata_base.c| 10 ++---
arch/s390/kernel/debug.c | 2 +-
arch/s390/kernel/topology.c | 2 +-
arch/s390/mm/cmm.c| 6 +--
arch/x86/kernel/itmt.c| 2 +-
drivers/cdrom/cdrom.c | 4 +-
drivers/char/random.c | 4 +-
drivers/macintosh/mac_hid.c | 2 +-
drivers/net/vrf.c | 2 +-
drivers/parport/procfs.c | 12 +++---
drivers/perf/arm_pmuv3.c | 2 +-
drivers/perf/riscv_pmu_sbi.c | 2 +-
fs/coredump.c | 2 +-
fs/dcache.c | 2 +-
fs/drop_caches.c | 2 +-
fs/exec.c | 2 +-
fs/file_table.c | 2 +-
fs/fs-writeback.c | 2 +-
fs/inode.c| 2 +-
fs/pipe.c
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > Hi Kees, > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > The 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. > > > > > > Split this per subsystem, please. > > > > I've done a few painful API transitions before, and I don't think the > > complexity of these changes needs a per-subsystem constification pass. I > > think this series is the right approach, but that patch 11 will need > > coordination with Linus. We regularly do system-wide prototype changes > > like this right at the end of the merge window before -rc1 comes out. > > That sounds good. > > > The requirements are pretty simple: it needs to be a obvious changes > > (this certainly is) and as close to 100% mechanical as possible. I think > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > script and get nearly the same results, etc. And all the other changes > > need to have landed. This change also has no "silent failure" conditions: > > anything mismatched will immediately stand out. > > Unfortunately coccinelle alone is not sufficient, as some helpers with > different prototypes are called by handlers and themselves are calling > handler and therefore need to change in the same commit. > But if I add a diff for those on top of the coccinelle script to the > changelog it should be obvious. Judging by Kees' comment on "100% mechanical", it might be better just having the diff and have Linus apply than rather than two step process? Have not these types of PRs, so am interested in what folks think. > > > So, have patches 1-10 go via their respective subsystems, and once all > > of those are in Linus's tree, send patch 11 as a stand-alone PR. > > Ack, I'll do that with the cover letter information requested by Joel. > > > (From patch 11, it looks like the seccomp read/write function changes > > could be split out? I'll do that now...) > > Thanks! > > Thomas -- Joel Granados signature.asc Description: PGP signature
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Wed, May 08, 2024 at 10:11:35AM -0700, Kees Cook wrote:
> On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote:
> > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Wei�schuh wrote:
> > > The 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.
> >
> > Split this per subsystem, please.
>
Thx for stepping in to move this forward.
> I've done a few painful API transitions before, and I don't think the
> complexity of these changes needs a per-subsystem constification pass. I
> think this series is the right approach, but that patch 11 will need
> coordination with Linus. We regularly do system-wide prototype changes
> like this right at the end of the merge window before -rc1 comes out.
This would be more for 6.11, as I expect the other subsystems to freeze
for the merge window.
>
> The requirements are pretty simple: it needs to be a obvious changes
> (this certainly is) and as close to 100% mechanical as possible. I think
> patch 11 easily qualifies. Linus should be able to run the same Coccinelle
> script and get nearly the same results, etc. And all the other changes
The coccinelle script is not enough. But that patch 11 should still be
trivial enough to go in before -rc1. right?
> need to have landed. This change also has no "silent failure" conditions:
> anything mismatched will immediately stand out.
>
> So, have patches 1-10 go via their respective subsystems, and once all
> of those are in Linus's tree, send patch 11 as a stand-alone PR.
Thomas: I can take the sysctl subsystem related patches ("[PATCH v3
10/11] sysctl: constify ctl_table arguments of utility function"), while
you push the others to their respective subsystems (If you have not
already)
>
> (From patch 11, it looks like the seccomp read/write function changes
> could be split out? I'll do that now...)
I saw that that patch has the necessary reviews. Get back to me if you
need me to take a quick look at it.
--
Joel Granados
signature.asc
Description: PGP signature
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
Kees Could you comment on the feasibility of this alternative from the Control Flow Integrity perspective. My proposal is to change the proc_handler to void* and back in the same release. So there would not be a kernel released with a void* proc_handler. > > However, there is an alternative way to do this that allows chunking. We > > first define the proc_handler as a void pointer (casting it where it is > > being used) [1]. Then we could do the constification by subsystem (like > > Jakub proposes). Finally we can "revert the void pointer change so we > > don't have one size fit all pointer as our proc_handler [2]. > > > > Here are some comments about the alternative: > > 1. We would need to make the first argument const in all the derived > >proc_handlers [3] > > 2. There would be no undefined behavior for two reasons: > >2.1. There is no case where we change the first argument. We know > > this because there are no compile errors after we make it const. > >2.2. We would always go from non-const to const. This is the case > > because all the stuff that is unchanged in non-const. > > 3. If the idea sticks, it should go into mainline as one patchset. I > >would not like to have a void* proc_handler in a kernel release. > > 4. I think this is a "win/win" solution were the constification goes > >through and it is divided in such a way that it is reviewable. > > > > I would really like to hear what ppl think about this "heretic" > > alternative. @Thomas, @Luis, @Kees @Jakub? > > Thanks for that alternative, I'm not a big fan though. > > Besides the wonky syntax, Control Flow Integrity should trap on > this construct. Functions are called through different pointers than > their actual types which is exactly what CFI is meant to prevent. > > Maybe people find it easier to review when using > "--word-diff" and/or "-U0" with git diff/show. > There is really nothing going an besides adding a few "const"s. > > But if the consensus prefers this solution, I'll be happy to adopt it. > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=4a383503b1ea650d4e12c1f5838974e879f5aa6f > > [2] > > https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=a3be65973d27ec2933b9e81e1bec60be3a9b460d > > [3] proc_dostring, proc_dobool, proc_dointvec > > > Thomas Best -- Joel Granados signature.asc Description: PGP signature
Re: [apparmor] [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Fri, May 03, 2024 at 04:09:40PM +0200, Thomas Weißschuh wrote: > Hey Joel, > ... > > # Motivation > > As I read it, the motivation for these constification efforts are: > > 1. It provides increased safety: Having things in .rodata section reduces > > the > >attack surface. This is especially relevant for structures that have > > function > >pointers (like ctl_table); having these in .rodata means that these > > pointers > >always point to the "intended" function and cannot be changed. > > 2. Compiler optimizations: This was just a comment in the patchsets that I > > have > >mentioned ([3,4,5]). Do you know what optimizations specifically? Does it > >have to do with enhancing locality for the data in .rodata? Do you have > > other > >specific optimizations in mind? > > I don't know about anything that would make it faster. > It's more about safety and transmission of intent to API users, > especially callback implementers. Noted. ... > > # Show the move > > I created [8] because there is no easy way to validate which objects made it > > into .rodata. I ran [8] for your Dec 2nd patcheset [7] and there are less in > > .rodata than I expected (the results are in [9]) Why is that? Is it > > something > > that has not been posted to the lists yet? > > Constifying the APIs only *allows* the actual table to be constified > themselves. > Then each table definition will have to be touched and "const" added. That is what I thought. thx for clarifying. > > See patches 17 and 18 in [7] for two examples. > > Some tables in net/ are already "const" as the static definitions are > never registered themselves but only their copies are. > ... best -- Joel Granados signature.asc Description: PGP signature
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
#x27;\[.*\]', '.*\.(c|h):[ \t]*static
(const )?struct ctl_table ']
ctl_table_structs = remove_tokens_re(exec_cmd( cmd ), regex_patterns)
cmd = "readelf -X -Ws build/vmlinux"
regex_patterns = ['.*OBJECT.*']
output_lines = filter_in_lines(exec_cmd( cmd ), regex_patterns);
regex_patterns = ['^.*OBJECT', '[ \t]+[A-Z]+[ \t]+[A-Z]+.*\(.*\)[ \t]+']
obj_elems = remove_tokens_re( output_lines, regex_patterns, uniq=False)
regex_patterns = ['^.*\(', '\)[ ]+.*$']
sec_names = remove_tokens_re( output_lines, regex_patterns, uniq=False)
for i in range(len(sec_names)):
obj_name = obj_elems[i]
if obj_name in ctl_table_structs:
print ("section: {}\t\tobj_name : {}". format(sec_names[i],
obj_name))
[9]
section: .rodataobj_name : kern_table
section: .rodataobj_name : sysctl_mount_point
section: .rodataobj_name : addrconf_sysctl
section: .rodataobj_name : ax25_param_table
section: .rodataobj_name : mpls_table
section: .rodataobj_name : mpls_dev_table
section: .data obj_name : sld_sysctls
section: .data obj_name : kern_panic_table
section: .data obj_name : kern_exit_table
section: .data obj_name : vm_table
section: .data obj_name : signal_debug_table
section: .data obj_name : usermodehelper_table
section: .data obj_name : kern_reboot_table
section: .data obj_name : user_table
section: .bss obj_name : sched_core_sysctls
section: .data obj_name : sched_fair_sysctls
section: .data obj_name : sched_rt_sysctls
section: .data obj_name : sched_dl_sysctls
section: .data obj_name : printk_sysctls
section: .data obj_name : pid_ns_ctl_table_vm
section: .data obj_name : seccomp_sysctl_table
section: .data obj_name : uts_kern_table
section: .data obj_name : vm_oom_kill_table
section: .data obj_name : vm_page_writeback_sysctls
section: .data obj_name : page_alloc_sysctl_table
section: .data obj_name : hugetlb_table
section: .data obj_name : fs_stat_sysctls
section: .data obj_name : fs_exec_sysctls
section: .data obj_name : fs_pipe_sysctls
section: .data obj_name : namei_sysctls
section: .data obj_name : fs_dcache_sysctls
section: .data obj_name : inodes_sysctls
section: .data obj_name : fs_namespace_sysctls
section: .data obj_name : dnotify_sysctls
section: .data obj_name : inotify_table
section: .data obj_name : epoll_table
section: .data obj_name : aio_sysctls
section: .data obj_name : locks_sysctls
section: .data obj_name : coredump_sysctls
section: .data obj_name : fs_shared_sysctls
section: .data obj_name : fs_dqstats_table
section: .data obj_name : root_table
section: .data obj_name : pty_table
section: .data obj_name : xfs_table
section: .data obj_name : ipc_sysctls
section: .data obj_name : key_sysctls
section: .data obj_name : kernel_io_uring_disabled_table
section: .data obj_name : tty_table
section: .data obj_name : random_table
section: .data obj_name : scsi_table
section: .data obj_name : iwcm_ctl_table
section: .data obj_name : net_core_table
section: .data obj_name : netns_core_table
section: .bss obj_name : nf_log_sysctl_table
section: .data obj_name : nf_log_sysctl_ftable
section: .data obj_name : vs_vars
section: .data obj_name : vs_vars_table
section: .data obj_name : ipv4_route_netns_table
section: .data obj_name : ipv4_route_table
section: .data obj_name : ip4_frags_ns_ctl_table
section: .data obj_name : ip4_frags_ctl_table
section: .data obj_name : ctl_forward_entry
section: .data obj_name : ipv4_table
section: .data obj_name : ipv4_net_table
section: .data obj_name : unix_table
section: .data obj_name : ipv6_route_table_template
section: .data obj_name : ipv6_icmp_table_template
section: .data obj_name : ip6_frags_ns_ctl_table
section: .data obj_name : ip6_frags_ctl_table
section: .data obj_name : ipv6_table_template
section: .data obj_name : ipv6_rotable
section: .data obj_name : sctp_net_table
section: .data obj_name : sctp_table
section: .data obj_name : smc_table
section: .data obj_name : lowpan_frags_ns_ctl_table
section: .data obj_name : lowpan_frags_ctl_table
--
Joel Granados
signature.asc
Description: PGP signature
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > The 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. > > Split this per subsystem, please. It is tricky to do that because it changes the first argument (ctl*) to const in the proc_handler function type defined in sysclt.h: " -typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer, +typedef int proc_handler(const struct ctl_table *ctl, int write, void *buffer, size_t *lenp, loff_t *ppos); " This means that all the proc_handlers need to change at the same time. However, there is an alternative way to do this that allows chunking. We first define the proc_handler as a void pointer (casting it where it is being used) [1]. Then we could do the constification by subsystem (like Jakub proposes). Finally we can "revert the void pointer change so we don't have one size fit all pointer as our proc_handler [2]. Here are some comments about the alternative: 1. We would need to make the first argument const in all the derived proc_handlers [3] 2. There would be no undefined behavior for two reasons: 2.1. There is no case where we change the first argument. We know this because there are no compile errors after we make it const. 2.2. We would always go from non-const to const. This is the case because all the stuff that is unchanged in non-const. 3. If the idea sticks, it should go into mainline as one patchset. I would not like to have a void* proc_handler in a kernel release. 4. I think this is a "win/win" solution were the constification goes through and it is divided in such a way that it is reviewable. I would really like to hear what ppl think about this "heretic" alternative. @Thomas, @Luis, @Kees @Jakub? Best [1] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=4a383503b1ea650d4e12c1f5838974e879f5aa6f [2] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=a3be65973d27ec2933b9e81e1bec60be3a9b460d [3] proc_dostring, proc_dobool, proc_dointvec -- Joel Granados signature.asc Description: PGP signature
Re: [apparmor] [PATCH 2/7] security: Remove the now superfluous sentinel element from ctl_table array
On Mon, Apr 15, 2024 at 03:02:43PM -0400, Paul Moore wrote: > On Mon, Apr 15, 2024 at 10:17 AM Paul Moore wrote: > > On Mon, Apr 15, 2024 at 9:44 AM Joel Granados > > wrote: > > > > > > Hey > > > > > > This is the only patch that I have not seen added to the next tree. > > > I'll put this in the sysctl-next > > > https://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/log/?h=sysctl-next > > > for testing. Please let me know if It is lined up to be upstream through > > > another path. > > > > I was hoping to see some ACKs from the associated LSM maintainers, but > > it's minor enough I'll go ahead and pull it into the lsm/dev tree this > > week. I'll send a note later when I do the merge. > > ... and now it's merged, it should be in the next cut of the > linux-next tree. Thanks! Awesome. I'll remove it from sysctl-next then to avoid any potential crashes. Thx -- Joel Granados signature.asc Description: PGP signature
Re: [apparmor] [PATCH 2/7] security: Remove the now superfluous sentinel element from ctl_table array
Hey This is the only patch that I have not seen added to the next tree. I'll put this in the sysctl-next https://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/log/?h=sysctl-next for testing. Please let me know if It is lined up to be upstream through another path. Best On Thu, Mar 28, 2024 at 04:57:49PM +0100, Joel Granados via B4 Relay wrote: > From: Joel Granados > > This commit comes at the tail end of a greater effort to remove the > empty elements at the end of the ctl_table arrays (sentinels) which will > reduce the overall build time size of the kernel and run time memory > bloat by ~64 bytes per sentinel (further information Link : > https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/) > ... -- Joel Granados signature.asc Description: PGP signature
Re: [apparmor] [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays
On Wed, Jun 21, 2023 at 04:15:46PM +0300, Jani Nikula wrote:
> On Wed, 21 Jun 2023, Joel Granados wrote:
> > On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
> >> On Wed, 21 Jun 2023, Joel Granados wrote:
> >> > Remove the empty end element from all the arrays that are passed to the
> >> > register sysctl calls. In some files this means reducing the explicit
> >> > array size by one. Also make sure that we are using the size in
> >> > ctl_table_header instead of evaluating the .procname element.
> >>
> >> Where's the harm in removing the end elements driver by driver? This is
> >> an unwieldy patch to handle.
> >
> > I totally agree. Its a big one!!! but I'm concerned of breaking
> > bisectibility:
> > * I could for example separate all the removes into separate commits and
> > then have a final commit that removes the check for the empty element.
> > But this will leave the tree in a state where the for loop will have
> > undefined behavior when it looks for the empty end element. It might
> > or might not work (probably not :) until the final commit where I fix
> > that.
> >
> > * I could also change the logic that looks for the final element,
> > commit that first and then remove the empty element one commit per
> > driver after that. But then for all the arrays that still have an
> > empty element, there would again be undefined behavior as it would
> > think that the last element is valid (when it is really the sentinel).
> >
> > Any ideas on how to get around these?
>
> First add size to the register calls, and allow the last one to be
> sentinel but do not require the sentinel.
>
> Start removing sentinels, adjusting the size passed in.
This is a great idea! and I think I don't even have to adjust the size
because if I change the logic to stop on the sentinel or the size; so when
the sentinel is there, it will stop before the size. And when the
sentinel is not there, it will stop on the correct size.
There might be issues with indirection calls. And there might also be
lots of places where I need to adjust a for loop (as dan has pointed
out) but its worth a try for V2.
Best
>
> Once enough sentinels have been removed, add warning if the final entry
> is a sentinel.
>
> Never really remove the check? (But surely you can rework the logic to
> not count the number of elements up front, only while iterating.)
>
>
> BR,
> Jani.
>
> >>
> >> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
> >> > b/drivers/gpu/drm/i915/i915_perf.c
> >> > index f43950219ffc..e4d7372afb10 100644
> >> > --- a/drivers/gpu/drm/i915/i915_perf.c
> >> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct
> >> > drm_device *dev, void *data,
> >> >
> >> > static struct ctl_table oa_table[] = {
> >> > {
> >> > - .procname = "perf_stream_paranoid",
> >> > - .data = &i915_perf_stream_paranoid,
> >> > - .maxlen = sizeof(i915_perf_stream_paranoid),
> >> > - .mode = 0644,
> >> > - .proc_handler = proc_dointvec_minmax,
> >> > - .extra1 = SYSCTL_ZERO,
> >> > - .extra2 = SYSCTL_ONE,
> >> > - },
> >> > +.procname = "perf_stream_paranoid",
> >> > +.data = &i915_perf_stream_paranoid,
> >> > +.maxlen = sizeof(i915_perf_stream_paranoid),
> >> > +.mode = 0644,
> >> > +.proc_handler = proc_dointvec_minmax,
> >> > +.extra1 = SYSCTL_ZERO,
> >> > +.extra2 = SYSCTL_ONE,
> >> > +},
> >> > {
> >> > - .procname = "oa_max_sample_rate",
> >> > - .data = &i915_oa_max_sample_rate,
> >> > - .maxlen = sizeof(i915_oa_max_sample_rate),
> >> > - .mode = 0644,
> >> > - .proc_handler = proc_dointvec_minmax,
> >> > - .extra1 = SYSCTL_ZERO,
> >> > - .extra2 = &oa_sample_rate_hard_limit,
> >> > - },
> >> > -{}
> >> > +.procname = "oa_max_sample_rate",
> >> > +.data = &i915_oa_max_sample_rate,
> >> > +.maxlen = sizeof(i915_oa_max_sample_rate),
> >> > +
Re: [apparmor] [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays
On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
> On Wed, 21 Jun 2023, Joel Granados wrote:
> > Remove the empty end element from all the arrays that are passed to the
> > register sysctl calls. In some files this means reducing the explicit
> > array size by one. Also make sure that we are using the size in
> > ctl_table_header instead of evaluating the .procname element.
>
> Where's the harm in removing the end elements driver by driver? This is
> an unwieldy patch to handle.
I totally agree. Its a big one!!! but I'm concerned of breaking bisectibility:
* I could for example separate all the removes into separate commits and
then have a final commit that removes the check for the empty element.
But this will leave the tree in a state where the for loop will have
undefined behavior when it looks for the empty end element. It might
or might not work (probably not :) until the final commit where I fix
that.
* I could also change the logic that looks for the final element,
commit that first and then remove the empty element one commit per
driver after that. But then for all the arrays that still have an
empty element, there would again be undefined behavior as it would
think that the last element is valid (when it is really the sentinel).
Any ideas on how to get around these?
>
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
> > b/drivers/gpu/drm/i915/i915_perf.c
> > index f43950219ffc..e4d7372afb10 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct drm_device
> > *dev, void *data,
> >
> > static struct ctl_table oa_table[] = {
> > {
> > -.procname = "perf_stream_paranoid",
> > -.data = &i915_perf_stream_paranoid,
> > -.maxlen = sizeof(i915_perf_stream_paranoid),
> > -.mode = 0644,
> > -.proc_handler = proc_dointvec_minmax,
> > -.extra1 = SYSCTL_ZERO,
> > -.extra2 = SYSCTL_ONE,
> > -},
> > + .procname = "perf_stream_paranoid",
> > + .data = &i915_perf_stream_paranoid,
> > + .maxlen = sizeof(i915_perf_stream_paranoid),
> > + .mode = 0644,
> > + .proc_handler = proc_dointvec_minmax,
> > + .extra1 = SYSCTL_ZERO,
> > + .extra2 = SYSCTL_ONE,
> > + },
> > {
> > -.procname = "oa_max_sample_rate",
> > -.data = &i915_oa_max_sample_rate,
> > -.maxlen = sizeof(i915_oa_max_sample_rate),
> > -.mode = 0644,
> > -.proc_handler = proc_dointvec_minmax,
> > -.extra1 = SYSCTL_ZERO,
> > -.extra2 = &oa_sample_rate_hard_limit,
> > -},
> > - {}
> > + .procname = "oa_max_sample_rate",
> > + .data = &i915_oa_max_sample_rate,
> > + .maxlen = sizeof(i915_oa_max_sample_rate),
> > + .mode = 0644,
> > + .proc_handler = proc_dointvec_minmax,
> > + .extra1 = SYSCTL_ZERO,
> > + .extra2 = &oa_sample_rate_hard_limit,
> > + }
> > };
>
> The existing indentation is off, but fixing it doesn't really belong in
> this patch.
Agreed. But I actually was trying to fix something that checkpatch
flagged. I'll change these back (which will cause this patch to be
flagged).
An alternative solution would be to fix the indentation as part of the
preparation patches. Tell me what you think.
Thx
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Joel Granados
signature.asc
Description: PGP signature
[apparmor] [PATCH 07/11] sysctl: Add size to register_sysctl
In order to remove the end element from the ctl_table struct arrays, we
explicitly define the size when registering the targes.
We add a size argument to register_sysctl and change all the callers to
pass the ARRAY_SIZE of their table arg.
Signed-off-by: Joel Granados
---
arch/arm/kernel/isa.c | 2 +-
arch/arm64/kernel/armv8_deprecated.c | 2 +-
arch/arm64/kernel/fpsimd.c| 6 +++--
arch/arm64/kernel/process.c | 3 ++-
arch/ia64/kernel/crash.c | 3 ++-
arch/powerpc/kernel/idle.c| 3 ++-
arch/powerpc/platforms/pseries/mobility.c | 3 ++-
arch/s390/appldata/appldata_base.c| 4 +++-
arch/s390/kernel/debug.c | 3 ++-
arch/s390/kernel/topology.c | 3 ++-
arch/s390/mm/cmm.c| 3 ++-
arch/s390/mm/pgalloc.c| 3 ++-
arch/x86/entry/vdso/vdso32-setup.c| 2 +-
arch/x86/kernel/itmt.c| 3 ++-
crypto/fips.c | 3 ++-
drivers/base/firmware_loader/fallback_table.c | 6 ++---
drivers/cdrom/cdrom.c | 3 ++-
drivers/char/hpet.c | 3 ++-
drivers/char/ipmi/ipmi_poweroff.c | 3 ++-
drivers/gpu/drm/i915/i915_perf.c | 3 ++-
drivers/hv/hv_common.c| 3 ++-
drivers/macintosh/mac_hid.c | 3 ++-
drivers/md/md.c | 3 ++-
drivers/misc/sgi-xp/xpc_main.c| 6 +++--
drivers/parport/procfs.c | 11 +
drivers/perf/arm_pmuv3.c | 3 ++-
drivers/scsi/scsi_sysctl.c| 3 ++-
drivers/scsi/sg.c | 3 ++-
fs/cachefiles/error_inject.c | 3 ++-
fs/coda/sysctl.c | 3 ++-
fs/devpts/inode.c | 3 ++-
fs/eventpoll.c| 2 +-
fs/lockd/svc.c| 3 ++-
fs/nfs/nfs4sysctl.c | 3 ++-
fs/nfs/sysctl.c | 3 ++-
fs/notify/fanotify/fanotify_user.c| 3 ++-
fs/notify/inotify/inotify_user.c | 3 ++-
fs/ntfs/sysctl.c | 3 ++-
fs/ocfs2/stackglue.c | 3 ++-
fs/proc/proc_sysctl.c | 23 ++-
fs/verity/signature.c | 4 +++-
fs/xfs/xfs_sysctl.c | 3 ++-
include/linux/sysctl.h| 6 +++--
kernel/pid_sysctl.h | 2 +-
kernel/time/timer.c | 2 +-
kernel/ucount.c | 2 +-
kernel/utsname_sysctl.c | 2 +-
lib/test_sysctl.c | 9 +---
net/sunrpc/sysctl.c | 3 ++-
net/sunrpc/xprtrdma/svc_rdma.c| 3 ++-
net/sunrpc/xprtrdma/transport.c | 4 +++-
net/sunrpc/xprtsock.c | 4 +++-
net/sysctl_net.c | 2 +-
security/apparmor/lsm.c | 3 ++-
security/loadpin/loadpin.c| 3 ++-
security/yama/yama_lsm.c | 3 ++-
56 files changed, 133 insertions(+), 76 deletions(-)
diff --git a/arch/arm/kernel/isa.c b/arch/arm/kernel/isa.c
index 20218876bef2..561432e3c55a 100644
--- a/arch/arm/kernel/isa.c
+++ b/arch/arm/kernel/isa.c
@@ -46,5 +46,5 @@ register_isa_ports(unsigned int membase, unsigned int
portbase, unsigned int por
isa_membase = membase;
isa_portbase = portbase;
isa_portshift = portshift;
- isa_sysctl_header = register_sysctl("bus/isa", ctl_isa_vars);
+ isa_sysctl_header = register_sysctl("bus/isa", ctl_isa_vars,
ARRAY_SIZE(ctl_isa_vars));
}
diff --git a/arch/arm64/kernel/armv8_deprecated.c
b/arch/arm64/kernel/armv8_deprecated.c
index 1febd412b4d2..68ed60a521a6 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -569,7 +569,7 @@ static void __init register_insn_emulation(struct
insn_emulation *insn)
sysctl->extra2 = &insn->max;
sysctl->proc_handler = emulation_proc_handler;
- register_sysctl("abi", sysctl);
+ register_sysctl("abi", sysctl, 1);
}
}
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 2fbafa5cc7ac..ecfb2ef6a036 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -595,7 +595,8 @@ static struct ctl_table sve_default_vl_table[] = {
static int __init sve_sysctl_init(void)
{
if (system_supports_sve())
- if (!register_sysctl("abi", s
[apparmor] [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays
Remove the empty end element from all the arrays that are passed to the register sysctl calls. In some files this means reducing the explicit array size by one. Also make sure that we are using the size in ctl_table_header instead of evaluating the .procname element. Signed-off-by: Joel Granados --- arch/arm/kernel/isa.c | 4 +- arch/arm64/kernel/armv8_deprecated.c | 8 ++-- arch/arm64/kernel/fpsimd.c| 6 +-- arch/arm64/kernel/process.c | 3 +- arch/ia64/kernel/crash.c | 3 +- arch/powerpc/kernel/idle.c| 3 +- arch/powerpc/platforms/pseries/mobility.c | 3 +- arch/s390/appldata/appldata_base.c| 7 ++-- arch/s390/kernel/debug.c | 3 +- arch/s390/kernel/topology.c | 3 +- arch/s390/mm/cmm.c| 3 +- arch/s390/mm/pgalloc.c| 3 +- arch/x86/entry/vdso/vdso32-setup.c| 3 +- arch/x86/kernel/cpu/intel.c | 3 +- arch/x86/kernel/itmt.c| 3 +- crypto/fips.c | 3 +- drivers/base/firmware_loader/fallback_table.c | 3 +- drivers/cdrom/cdrom.c | 3 +- drivers/char/hpet.c | 13 +++--- drivers/char/ipmi/ipmi_poweroff.c | 3 +- drivers/char/random.c | 3 +- drivers/gpu/drm/i915/i915_perf.c | 33 +++ drivers/hv/hv_common.c| 3 +- drivers/infiniband/core/iwcm.c| 3 +- drivers/infiniband/core/ucma.c| 3 +- drivers/macintosh/mac_hid.c | 3 +- drivers/md/md.c | 3 +- drivers/misc/sgi-xp/xpc_main.c| 6 +-- drivers/net/vrf.c | 3 +- drivers/parport/procfs.c | 42 --- drivers/perf/arm_pmuv3.c | 3 +- drivers/scsi/scsi_sysctl.c| 3 +- drivers/scsi/sg.c | 3 +- drivers/tty/tty_io.c | 3 +- drivers/xen/balloon.c | 3 +- fs/aio.c | 3 +- fs/cachefiles/error_inject.c | 3 +- fs/coda/sysctl.c | 3 +- fs/coredump.c | 3 +- fs/dcache.c | 3 +- fs/devpts/inode.c | 3 +- fs/eventpoll.c| 3 +- fs/exec.c | 3 +- fs/file_table.c | 3 +- fs/inode.c| 3 +- fs/lockd/svc.c| 3 +- fs/locks.c| 3 +- fs/namei.c| 3 +- fs/namespace.c| 3 +- fs/nfs/nfs4sysctl.c | 3 +- fs/nfs/sysctl.c | 3 +- fs/notify/dnotify/dnotify.c | 3 +- fs/notify/fanotify/fanotify_user.c| 3 +- fs/notify/inotify/inotify_user.c | 3 +- fs/ntfs/sysctl.c | 3 +- fs/ocfs2/stackglue.c | 3 +- fs/pipe.c | 3 +- fs/proc/proc_sysctl.c | 8 ++-- fs/quota/dquot.c | 3 +- fs/sysctls.c | 3 +- fs/userfaultfd.c | 3 +- fs/verity/signature.c | 3 +- fs/xfs/xfs_sysctl.c | 4 +- init/do_mounts_initrd.c | 3 +- ipc/ipc_sysctl.c | 3 +- ipc/mq_sysctl.c | 3 +- kernel/acct.c | 3 +- kernel/bpf/syscall.c | 3 +- kernel/delayacct.c| 3 +- kernel/exit.c | 3 +- kernel/hung_task.c| 3 +- kernel/kexec_core.c | 3 +- kernel/kprobes.c | 3 +- kernel/latencytop.c | 3 +- kernel/locking/lockdep.c | 3 +- kernel/panic.c| 3 +- kernel/pid_namespace.c| 3 +- kernel/pid_sysctl.h | 3 +- kernel/printk/sysctl.c| 3 +- kernel/reboot.c | 3 +- kernel/sched/autogroup.c | 3 +- kernel/sched/core.c | 3 +- kernel/sched/deadline.c | 3 +- kernel/sched/fair.c | 3 +- kernel/sched/rt.c
