| Message ID | 20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | sysctl: constify sysctl ctl_tables | expand |
On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote:
> Tested by booting and with the sysctl selftests on x86.
Can I trouble you to rebase on sysctl-next?
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next
Luis
On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > Tested by booting and with the sysctl selftests on x86. > > Can I trouble you to rebase on sysctl-next? > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next Will do. Note: I noticed that patch "sysctl: move sysctl type to ctl_table_header" from this series seems to be the better alternative to commit fd696ee2395755a ("sysctl: Fix out of bounds access for empty sysctl registers") which is currently on sysctl-next. The patch from the series should only depend on "sysctl: drop sysctl_is_perm_empty_ctl_table" from my series. Thomas
Hi Luis, Joel, On 2023-12-05 09:04:08+0100, Thomas Weißschuh wrote: > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > Tested by booting and with the sysctl selftests on x86. > > > > Can I trouble you to rebase on sysctl-next? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > Will do. The rebased series is now available at https://git.sr.ht/~t-8ch/linux b4/const-sysctl Nothing much has changed in contrast to v2. The only functional change so far is the initialization of ctl_table_header::type in init_header(). I'll wait for Joels and maybe some more reviews before resending it. > [..] For the future I think it would make sense to combine the tree-wide constification of the structs with the removal of the sentinel values. This would reduce the impacts of the maintainers. Thomas
On Tue, Dec 05, 2023 at 06:16:53PM +0100, Thomas Weißschuh wrote: > Hi Luis, Joel, > > On 2023-12-05 09:04:08+0100, Thomas Weißschuh wrote: > > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > > Tested by booting and with the sysctl selftests on x86. > > > > > > Can I trouble you to rebase on sysctl-next? > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > > > Will do. > > The rebased series is now available at > https://git.sr.ht/~t-8ch/linux b4/const-sysctl I've applied this to sysctl-next as this all looks very sensible to me, except one patch which I'll chime in on, but I'm merging it to sysctl-next now without a promise to get this in as I really would like this to soak in on linux-next for a bit even if it does not get merged in the next kernel release. Exposing it on linux-next will surely iron out run time issues fast. > Nothing much has changed in contrast to v2. > The only functional change so far is the initialization of > ctl_table_header::type in init_header(). > > I'll wait for Joels and maybe some more reviews before resending it. It all is very trivial stuff, except a few patches, but it all is making sense, so my ask is to address feedback this week and post next week a new set so we can have changes merged as-is for Linux in case this really doesn't break anything. For some reason I raccall seeing som hacky sysclts that shared and modified an entry somewhere but the exact sysctl phases me, and I just cannot recall. > > [..] > > For the future I think it would make sense to combine the tree-wide constification > of the structs with the removal of the sentinel values. > > This would reduce the impacts of the maintainers. Indeed. Luis
Hey Thomas You have a couple of test bot issues for your 12/18 patch. Can you please address those for your next version. On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > Problem description: > > The kernel contains a lot of struct ctl_table throught the tree. > These are very often 'static' definitions. > It would be good to make the tables unmodifiable by marking them "const" Here I would remove "It would be good to". Just state it: "Make the tables unmodifiable...." > to avoid accidental or malicious modifications. > This is in line with a general effort to move as much data as possible > into .rodata. (See for example[0] and [1]) If you could find more examples, it would make a better case. > > Unfortunately the tables can not be made const right now because the > core registration functions expect mutable tables. > > This is for two main reasons: > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify > the table. > 2) The table is passed to the handler function as a non-const pointer. > > This series migrates the core and all handlers. awesome! > > Structure of the series: > > Patch 1-3: Cleanup patches > Patch 4-7: Non-logic preparation patches > Patch 8: Preparation patch changing a bit of logic > Patch 9-12: Treewide changes to handler function signature > Patch 13-14: Adaption of the sysctl core implementation > Patch 15: Adaption of the sysctl core interface > Patch 16: New entry for checkpatch > Patch 17-18: Constification of existing "struct ctl_table"s > > Tested by booting and with the sysctl selftests on x86. > > Note: > > This is intentionally sent only to a small number of people as I'd like > to get some more sysctl core-maintainer feedback before sending this to > essentially everybody. When you do send it to the broader audience, you should chunk up your big patches (12/18 and 11/18) and this is why: 1. To avoid mail rejections from lists: You have to tell a lot of people about the changes in one mail. That will make mail header too big for some lists and it will be rejected. This happened to me with [3] 2. Avoid being rejected for the wrong reasons :) Maintainers are busy ppl and sending them a set with so many files may elicit a rejection on the grounds that it involves too many subsystems at the same time. I suggest you chunk it up with directories in mind. Something similar to what I did for [4] where I divided stuff that when for fs/*, kernel/*, net/*, arch/* and drivers/*. That will complicate your patch a tad because you have to ensure that the tree can be compiled/run for every commit. But it will pay off once you push it to the broader public. [3] https://lore.kernel.org/all/20230621091000.424843-1-j.granados@samsung.com [4] https://lore.kernel.org/all/20230726140635.2059334-1-j.granados@samsung.com > > [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ > > --- > Changes in v2: > - Migrate all handlers. > - Remove intermediate "proc_handler_new" step (Thanks Joel). > - Drop RFC status. > - Prepare other parts of the tree. > - Link to v1: https://lore.kernel.org/r/20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net > > --- > Thomas Weißschuh (18): > watchdog/core: remove sysctl handlers from public header > sysctl: delete unused define SYSCTL_PERM_EMPTY_DIR > sysctl: drop sysctl_is_perm_empty_ctl_table > cgroup: bpf: constify ctl_table arguments and fields > seccomp: constify ctl_table arguments of utility functions > hugetlb: constify ctl_table arguments of utility functions > utsname: constify ctl_table arguments of utility function > stackleak: don't modify ctl_table argument > sysctl: treewide: constify ctl_table_root::set_ownership > sysctl: treewide: constify ctl_table_root::permissions > sysctl: treewide: constify ctl_table_header::ctl_table_arg > sysctl: treewide: constify the ctl_table argument of handlers > sysctl: move sysctl type to ctl_table_header > sysctl: move internal interfaces to const struct ctl_table > sysctl: allow registration of const struct ctl_table > const_structs.checkpatch: add ctl_table > sysctl: make ctl_table sysctl_mount_point const > sysctl: constify standard sysctl tables > > arch/arm64/kernel/armv8_deprecated.c | 2 +- > arch/arm64/kernel/fpsimd.c | 2 +- > arch/s390/appldata/appldata_base.c | 8 +-- > 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 | 4 +- > drivers/parport/procfs.c | 12 ++-- > fs/coredump.c | 2 +- > fs/dcache.c | 4 +- > fs/drop_caches.c | 2 +- > fs/exec.c | 4 +- > fs/file_table.c | 2 +- > fs/fs-writeback.c | 2 +- > fs/inode.c | 4 +- > fs/pipe.c | 2 +- > fs/proc/internal.h | 2 +- > fs/proc/proc_sysctl.c | 102 +++++++++++++++--------------- > fs/quota/dquot.c | 2 +- > fs/xfs/xfs_sysctl.c | 6 +- > include/linux/bpf-cgroup.h | 2 +- > include/linux/filter.h | 2 +- > include/linux/ftrace.h | 4 +- > include/linux/mm.h | 8 +-- > include/linux/nmi.h | 7 -- > include/linux/perf_event.h | 6 +- > include/linux/security.h | 2 +- > include/linux/sysctl.h | 78 +++++++++++------------ > include/linux/vmstat.h | 6 +- > include/linux/writeback.h | 2 +- > include/net/ndisc.h | 2 +- > include/net/neighbour.h | 6 +- > include/net/netfilter/nf_hooks_lwtunnel.h | 2 +- > ipc/ipc_sysctl.c | 12 ++-- > ipc/mq_sysctl.c | 2 +- > kernel/bpf/cgroup.c | 2 +- > kernel/bpf/syscall.c | 4 +- > kernel/delayacct.c | 4 +- > kernel/events/callchain.c | 2 +- > kernel/events/core.c | 4 +- > kernel/fork.c | 2 +- > kernel/hung_task.c | 4 +- > kernel/kexec_core.c | 2 +- > kernel/kprobes.c | 2 +- > kernel/latencytop.c | 4 +- > kernel/pid_namespace.c | 2 +- > kernel/pid_sysctl.h | 2 +- > kernel/printk/internal.h | 2 +- > kernel/printk/printk.c | 2 +- > kernel/printk/sysctl.c | 5 +- > kernel/sched/core.c | 8 +-- > kernel/sched/rt.c | 12 ++-- > kernel/sched/topology.c | 2 +- > kernel/seccomp.c | 8 +-- > kernel/stackleak.c | 9 +-- > kernel/sysctl.c | 84 ++++++++++++------------ > kernel/time/timer.c | 2 +- > kernel/trace/ftrace.c | 2 +- > kernel/trace/trace.c | 2 +- > kernel/trace/trace_events_user.c | 2 +- > kernel/trace/trace_stack.c | 2 +- > kernel/ucount.c | 4 +- > kernel/umh.c | 2 +- > kernel/utsname_sysctl.c | 4 +- > kernel/watchdog.c | 15 +++-- > mm/compaction.c | 8 +-- > mm/hugetlb.c | 10 +-- > mm/page-writeback.c | 18 +++--- > mm/page_alloc.c | 22 +++---- > mm/util.c | 12 ++-- > mm/vmstat.c | 4 +- > net/ax25/sysctl_net_ax25.c | 2 +- > net/bridge/br_netfilter_hooks.c | 6 +- > net/core/neighbour.c | 24 +++---- > net/core/sysctl_net_core.c | 22 +++---- > net/ieee802154/6lowpan/reassembly.c | 2 +- > net/ipv4/devinet.c | 8 +-- > net/ipv4/ip_fragment.c | 2 +- > net/ipv4/route.c | 4 +- > net/ipv4/sysctl_net_ipv4.c | 35 +++++----- > net/ipv4/xfrm4_policy.c | 2 +- > net/ipv6/addrconf.c | 29 +++++---- > net/ipv6/ndisc.c | 4 +- > net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +- > net/ipv6/reassembly.c | 2 +- > net/ipv6/route.c | 2 +- > net/ipv6/sysctl_net_ipv6.c | 10 +-- > net/ipv6/xfrm6_policy.c | 2 +- > net/mpls/af_mpls.c | 8 +-- > net/mptcp/ctrl.c | 2 +- > net/netfilter/ipvs/ip_vs_ctl.c | 16 ++--- > net/netfilter/nf_conntrack_standalone.c | 4 +- > net/netfilter/nf_hooks_lwtunnel.c | 2 +- > net/netfilter/nf_log.c | 4 +- > net/phonet/sysctl.c | 2 +- > net/rds/tcp.c | 4 +- > net/sctp/sysctl.c | 30 ++++----- > net/smc/smc_sysctl.c | 2 +- > net/sunrpc/sysctl.c | 6 +- > net/sunrpc/xprtrdma/svc_rdma.c | 2 +- > net/sysctl_net.c | 4 +- > net/unix/sysctl_net_unix.c | 2 +- > net/xfrm/xfrm_sysctl.c | 2 +- > scripts/const_structs.checkpatch | 1 + > security/apparmor/lsm.c | 2 +- > security/min_addr.c | 2 +- > security/yama/yama_lsm.c | 2 +- > 111 files changed, 427 insertions(+), 428 deletions(-) > --- > base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a > change-id: 20231116-const-sysctl-e14624f1295c > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net> >
On Tue, Dec 05, 2023 at 09:04:08AM +0100, Thomas Weißschuh wrote: > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > Tested by booting and with the sysctl selftests on x86. > > > > Can I trouble you to rebase on sysctl-next? > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > Will do. > > Note: > > I noticed that patch "sysctl: move sysctl type to ctl_table_header" from > this series seems to be the better alternative to > commit fd696ee2395755a ("sysctl: Fix out of bounds access for empty sysctl registers") > which is currently on sysctl-next. Indeed. By taking this out of the ctl_table, we would not need to make sure that we don't touch that (potentially non-existing) first element. This is what I think should be done (@Luis @Kees chime in if you have any thoughts): 1. Leave the current fix for 6.7. AFAIK, it is already queued for that release and it is a bit late in the cycle to put anything new in. 2. I think this patch has value on its own as a better solution to the "access invalid memory" issue. @thomas: remove this patch from your const set and send it in a different patch series. 3. const patchset would need to go on top of the new set. Having it on its own will allow it to go in faster and make it easier to review without having to thinkg about the const stuff as well. Best > > The patch from the series should only depend on > "sysctl: drop sysctl_is_perm_empty_ctl_table" from my series. > > Thomas
On Tue, Dec 05, 2023 at 06:16:53PM +0100, Thomas Weißschuh wrote: > Hi Luis, Joel, > > On 2023-12-05 09:04:08+0100, Thomas Weißschuh wrote: > > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > > Tested by booting and with the sysctl selftests on x86. > > > > > > Can I trouble you to rebase on sysctl-next? > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > > > Will do. > > The rebased series is now available at > https://git.sr.ht/~t-8ch/linux b4/const-sysctl > > Nothing much has changed in contrast to v2. > The only functional change so far is the initialization of > ctl_table_header::type in init_header(). > > I'll wait for Joels and maybe some more reviews before resending it. > > > [..] > > For the future I think it would make sense to combine the tree-wide constification > of the structs with the removal of the sentinel values. I don't see how these two would fit. And this is why: 1. The "remove sentinel" stuff is almost done. With the sets going into 6.7 we would only be missing everything under net/*. So you would not be able to combine them (except for the net stuff) 2. The motivation for the two sets is differnt. This would confuse rather than simplify the process. 3. In order to introduce the const stuff we would have to go through another round of "convincing" which can potentially derail the "remove sentinel" stuff. I would *not* like to combine them. I think the const set can stand on its own. > > This would reduce the impacts of the maintainers. > > > Thomas
On Tue, Dec 05, 2023 at 02:27:04PM -0800, Luis Chamberlain wrote: > On Tue, Dec 05, 2023 at 06:16:53PM +0100, Thomas Weißschuh wrote: > > Hi Luis, Joel, > > > > On 2023-12-05 09:04:08+0100, Thomas Weißschuh wrote: > > > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > > > Tested by booting and with the sysctl selftests on x86. > > > > > > > > Can I trouble you to rebase on sysctl-next? > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > > > > > Will do. > > > > The rebased series is now available at > > https://git.sr.ht/~t-8ch/linux b4/const-sysctl > > I've applied this to sysctl-next as this all looks very sensible to me, > except one patch which I'll chime in on, but I'm merging it to That is the "move sysctl type to ctl_table_header" right? > sysctl-next now without a promise to get this in as I really would like > this to soak in on linux-next for a bit even if it does not get merged > in the next kernel release. Exposing it on linux-next will surely > iron out run time issues fast. +1 for soaking it :) > > > Nothing much has changed in contrast to v2. > > The only functional change so far is the initialization of > > ctl_table_header::type in init_header(). > > > > I'll wait for Joels and maybe some more reviews before resending it. > > It all is very trivial stuff, except a few patches, but it all is making > sense, so my ask is to address feedback this week and post next week > a new set so we can have changes merged as-is for Linux in case this > really doesn't break anything. Any thoughts on the size of the tree-wide patches? > > For some reason I raccall seeing som hacky sysclts that shared and > modified an entry somewhere but the exact sysctl phases me, and I just > cannot recall. Its probably in net/*. There is were they are really taking advantage of ctl_table. > > > > [..] > > > > For the future I think it would make sense to combine the tree-wide constification > > of the structs with the removal of the sentinel values. > > > > This would reduce the impacts of the maintainers. > > Indeed. > > Luis
On 2023-12-07 11:43:57+0100, Joel Granados wrote: > Hey Thomas > > You have a couple of test bot issues for your 12/18 patch. Can you > please address those for your next version. I have these fixed locally, I assumed Luis would also pick them up directly until I have a proper v2, properly should have communicated that. > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > Problem description: > > > > The kernel contains a lot of struct ctl_table throught the tree. > > These are very often 'static' definitions. > > It would be good to make the tables unmodifiable by marking them "const" > Here I would remove "It would be good to". Just state it: "Make the > tables unmodifiable...." Ack. > > > to avoid accidental or malicious modifications. > > This is in line with a general effort to move as much data as possible > > into .rodata. (See for example[0] and [1]) > If you could find more examples, it would make a better case. I'll look for some. So far my constifications went in without them :-) > > > > Unfortunately the tables can not be made const right now because the > > core registration functions expect mutable tables. > > > > This is for two main reasons: > > > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify > > the table. > > 2) The table is passed to the handler function as a non-const pointer. > > > > This series migrates the core and all handlers. > awesome! > > > > > Structure of the series: > > > > Patch 1-3: Cleanup patches > > Patch 4-7: Non-logic preparation patches > > Patch 8: Preparation patch changing a bit of logic > > Patch 9-12: Treewide changes to handler function signature > > Patch 13-14: Adaption of the sysctl core implementation > > Patch 15: Adaption of the sysctl core interface > > Patch 16: New entry for checkpatch > > Patch 17-18: Constification of existing "struct ctl_table"s > > > > Tested by booting and with the sysctl selftests on x86. > > > > Note: > > > > This is intentionally sent only to a small number of people as I'd like > > to get some more sysctl core-maintainer feedback before sending this to > > essentially everybody. > When you do send it to the broader audience, you should chunk up your big > patches (12/18 and 11/18) and this is why: > 1. To avoid mail rejections from lists: > You have to tell a lot of people about the changes in one mail. That > will make mail header too big for some lists and it will be rejected. > This happened to me with [3] > 2. Avoid being rejected for the wrong reasons :) > Maintainers are busy ppl and sending them a set with so many files > may elicit a rejection on the grounds that it involves too many > subsystems at the same time. > I suggest you chunk it up with directories in mind. Something similar to > what I did for [4] where I divided stuff that when for fs/*, kernel/*, > net/*, arch/* and drivers/*. That will complicate your patch a tad > because you have to ensure that the tree can be compiled/run for every > commit. But it will pay off once you push it to the broader public. This will break bisections. All function signatures need to be switched in one step. I would strongly like to avoid introducing broken commits. The fact that these big commits have no functional changes at all makes me hope it can work.
On 2023-12-07 12:19:57+0100, Joel Granados wrote: > On Tue, Dec 05, 2023 at 06:16:53PM +0100, Thomas Weißschuh wrote: > > Hi Luis, Joel, > > > > On 2023-12-05 09:04:08+0100, Thomas Weißschuh wrote: > > > On 2023-12-04 21:50:14-0800, Luis Chamberlain wrote: > > > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > > > Tested by booting and with the sysctl selftests on x86. > > > > > > > > Can I trouble you to rebase on sysctl-next? > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=sysctl-next > > > > > > Will do. > > > > The rebased series is now available at > > https://git.sr.ht/~t-8ch/linux b4/const-sysctl > > > > Nothing much has changed in contrast to v2. > > The only functional change so far is the initialization of > > ctl_table_header::type in init_header(). > > > > I'll wait for Joels and maybe some more reviews before resending it. > > > > > [..] > > > > For the future I think it would make sense to combine the tree-wide constification > > of the structs with the removal of the sentinel values. > I don't see how these two would fit. And this is why: > 1. The "remove sentinel" stuff is almost done. With the sets going into > 6.7 we would only be missing everything under net/*. So you would not > be able to combine them (except for the net stuff) > 2. The motivation for the two sets is differnt. This would confuse > rather than simplify the process. > 3. In order to introduce the const stuff we would have to go through > another round of "convincing" which can potentially derail the > "remove sentinel" stuff. Good reasons, especially 1). > I would *not* like to combine them. I think the const set can stand on > its own. It was more about a process optimization. If somebody has to touch each sysctl table anyway and test the changes, doing so for both series would be easier for the sysctl and subsystem maintainers. But alas, it seems I have to do it myself and can't heap it onto your pile :-) > > > > This would reduce the impacts of the maintainers.
On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote: > On 2023-12-07 11:43:57+0100, Joel Granados wrote: > > Hey Thomas > > > > You have a couple of test bot issues for your 12/18 patch. Can you > > please address those for your next version. > > I have these fixed locally, I assumed Luis would also pick them up > directly until I have a proper v2, properly should have communicated > that. > > > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote: > > > Problem description: > > > > > > The kernel contains a lot of struct ctl_table throught the tree. > > > These are very often 'static' definitions. > > > It would be good to make the tables unmodifiable by marking them "const" > > Here I would remove "It would be good to". Just state it: "Make the > > tables unmodifiable...." > > Ack. > > > > > > to avoid accidental or malicious modifications. > > > This is in line with a general effort to move as much data as possible > > > into .rodata. (See for example[0] and [1]) > > > If you could find more examples, it would make a better case. > > I'll look for some. So far my constifications went in without them :-) > > > > > > > Unfortunately the tables can not be made const right now because the > > > core registration functions expect mutable tables. > > > > > > This is for two main reasons: > > > > > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify > > > the table. > > > 2) The table is passed to the handler function as a non-const pointer. > > > > > > This series migrates the core and all handlers. > > > awesome! > > > > > > > > Structure of the series: > > > > > > Patch 1-3: Cleanup patches > > > Patch 4-7: Non-logic preparation patches > > > Patch 8: Preparation patch changing a bit of logic > > > Patch 9-12: Treewide changes to handler function signature > > > Patch 13-14: Adaption of the sysctl core implementation > > > Patch 15: Adaption of the sysctl core interface > > > Patch 16: New entry for checkpatch > > > Patch 17-18: Constification of existing "struct ctl_table"s > > > > > > Tested by booting and with the sysctl selftests on x86. > > > > > > Note: > > > > > > This is intentionally sent only to a small number of people as I'd like > > > to get some more sysctl core-maintainer feedback before sending this to > > > essentially everybody. > > > When you do send it to the broader audience, you should chunk up your big > > patches (12/18 and 11/18) and this is why: > > 1. To avoid mail rejections from lists: > > You have to tell a lot of people about the changes in one mail. That > > will make mail header too big for some lists and it will be rejected. > > This happened to me with [3] > > 2. Avoid being rejected for the wrong reasons :) > > Maintainers are busy ppl and sending them a set with so many files > > may elicit a rejection on the grounds that it involves too many > > subsystems at the same time. > > I suggest you chunk it up with directories in mind. Something similar to > > what I did for [4] where I divided stuff that when for fs/*, kernel/*, > > net/*, arch/* and drivers/*. That will complicate your patch a tad > > because you have to ensure that the tree can be compiled/run for every > > commit. But it will pay off once you push it to the broader public. > > This will break bisections. All function signatures need to be switched I was suggesting a solution without breaking bisections of course. I can think of a couple of ways to do this in chunks but it might be premature. You can send it and if you get push back because of this then we can deal with chunking it down. I'm still concerned about the header size for those mails. How does the mail look like when you run the get maintainers script on it? > in one step. I would strongly like to avoid introducing broken commits. > > The fact that these big commits have no functional changes at all makes > me hope it can work.
On 2023-12-08 10:59:26+0100, Joel Granados wrote: > On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote: > > On 2023-12-07 11:43:57+0100, Joel Granados wrote: > [..] > > > I suggest you chunk it up with directories in mind. Something similar to > > > what I did for [4] where I divided stuff that when for fs/*, kernel/*, > > > net/*, arch/* and drivers/*. That will complicate your patch a tad > > > because you have to ensure that the tree can be compiled/run for every > > > commit. But it will pay off once you push it to the broader public. > > > > This will break bisections. All function signatures need to be switched > I was suggesting a solution without breaking bisections of course. I can > think of a couple of ways to do this in chunks but it might be > premature. You can send it and if you get push back because of this then > we can deal with chunking it down. I'm curious about those ways. I don't see how to split the big commit. > I'm still concerned about the header size for those mails. How does the > mail look like when you run the get maintainers script on it? The full series has 142 recipients in total, the biggest patch itself has 124. Before sending it I'd like to get feedback on the internal rework of the is_empty detection from you and/or Luis. https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e or the attached patch. > [..] From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> Date: Sun, 3 Dec 2023 21:56:46 +0100 Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the logic by always keeping the permanently_empty flag on the ctl_dir. The previous logic kept the flag in the leaf ctl_table and from there transferred it to the ctl_table from the directory. This also removes the need to have a mutable ctl_table and will allow the constification of those structs. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- fs/proc/proc_sysctl.c | 74 +++++++++++++++++++----------------------- include/linux/sysctl.h | 13 ++------ 2 files changed, 36 insertions(+), 51 deletions(-) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 35c97ad54f34..33f41af58e9b 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -17,6 +17,7 @@ #include <linux/bpf-cgroup.h> #include <linux/mount.h> #include <linux/kmemleak.h> +#include <linux/cleanup.h> #include "internal.h" #define list_for_each_table_entry(entry, header) \ @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; static const struct file_operations proc_sys_dir_file_operations; static const struct inode_operations proc_sys_dir_operations; -/* Support for permanently empty directories */ -static struct ctl_table sysctl_mount_point[] = { - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY } -}; - -/** - * register_sysctl_mount_point() - registers a sysctl mount point - * @path: path for the mount point - * - * Used to create a permanently empty directory to serve as mount point. - * There are some subtle but important permission checks this allows in the - * case of unprivileged mounts. - */ -struct ctl_table_header *register_sysctl_mount_point(const char *path) -{ - return register_sysctl(path, sysctl_mount_point); -} -EXPORT_SYMBOL(register_sysctl_mount_point); - -#define sysctl_is_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) -#define sysctl_set_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) -#define sysctl_clear_perm_empty_ctl_header(hptr) \ - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT) - void proc_sys_poll_notify(struct ctl_table_poll *poll) { if (!poll) @@ -226,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) /* Is this a permanently empty directory? */ - if (sysctl_is_perm_empty_ctl_header(dir_h)) + if (dir->permanently_empty) return -EROFS; - /* Am I creating a permanently empty directory? */ - if (header->ctl_table_size > 0 && - sysctl_is_perm_empty_ctl_header(header)) { - if (!RB_EMPTY_ROOT(&dir->root)) - return -EINVAL; - sysctl_set_perm_empty_ctl_header(dir_h); - } - dir_h->nreg++; header->parent = dir; err = insert_links(header); @@ -252,8 +219,6 @@ fail: erase_header(header); put_links(header); fail_links: - if (header->ctl_table == sysctl_mount_point) - sysctl_clear_perm_empty_ctl_header(dir_h); header->parent = NULL; drop_sysctl_table(dir_h); return err; @@ -440,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, struct ctl_table_header *head, struct ctl_table *table) { struct ctl_table_root *root = head->root; + struct ctl_dir *ctl_dir; struct inode *inode; struct proc_inode *ei; @@ -473,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, inode->i_mode |= S_IFDIR; inode->i_op = &proc_sys_dir_operations; inode->i_fop = &proc_sys_dir_file_operations; - if (sysctl_is_perm_empty_ctl_header(head)) + + ctl_dir = container_of(head, struct ctl_dir, header); + if (ctl_dir->permanently_empty) make_empty_dir_inode(inode); } @@ -1211,8 +1179,7 @@ static bool get_links(struct ctl_dir *dir, struct ctl_table_header *tmp_head; struct ctl_table *entry, *link; - if (header->ctl_table_size == 0 || - sysctl_is_perm_empty_ctl_header(header)) + if (header->ctl_table_size == 0 || dir->permanently_empty) return true; /* Are there links available for every entry in table? */ @@ -1533,6 +1500,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) } EXPORT_SYMBOL(unregister_sysctl_table); +/** + * register_sysctl_mount_point() - registers a sysctl mount point + * @path: path for the mount point + * + * Used to create a permanently empty directory to serve as mount point. + * There are some subtle but important permission checks this allows in the + * case of unprivileged mounts. + */ +struct ctl_table_header *register_sysctl_mount_point(const char *path) +{ + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); + + if (IS_ERR(dir)) + return NULL; + + guard(spinlock)(&sysctl_lock); + + if (!RB_EMPTY_ROOT(&dir->root)) { + drop_sysctl_table(&dir->header); + return NULL; + } + + dir->permanently_empty = true; + return &dir->header; +} +EXPORT_SYMBOL(register_sysctl_mount_point); + void setup_sysctl_set(struct ctl_table_set *set, struct ctl_table_root *root, int (*is_seen)(struct ctl_table_set *)) diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index ada36ef8cecb..57cb0060d7d7 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -137,17 +137,6 @@ struct ctl_table { void *data; int maxlen; umode_t mode; - /** - * enum type - Enumeration to differentiate between ctl target types - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently - * empty directory target to serve - * as mount point. - */ - enum { - SYSCTL_TABLE_TYPE_DEFAULT, - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY - } type; proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll; void *extra1; @@ -194,6 +183,8 @@ struct ctl_dir { /* Header must be at the start of ctl_dir */ struct ctl_table_header header; struct rb_root root; + /* Permanently empty directory target to serve as mount point. */ + bool permanently_empty; }; struct ctl_table_set {
On Mon, Dec 11, 2023 at 12:25:10PM +0100, Thomas Weißschuh wrote: > On 2023-12-08 10:59:26+0100, Joel Granados wrote: > > On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote: > > > On 2023-12-07 11:43:57+0100, Joel Granados wrote: > > > [..] > > > > > I suggest you chunk it up with directories in mind. Something similar to > > > > what I did for [4] where I divided stuff that when for fs/*, kernel/*, > > > > net/*, arch/* and drivers/*. That will complicate your patch a tad > > > > because you have to ensure that the tree can be compiled/run for every > > > > commit. But it will pay off once you push it to the broader public. > > > > > > This will break bisections. All function signatures need to be switched > > > I was suggesting a solution without breaking bisections of course. I can > > think of a couple of ways to do this in chunks but it might be > > premature. You can send it and if you get push back because of this then > > we can deal with chunking it down. > > I'm curious about those ways. I don't see how to split the big commit. My idea was to do something similar to your originl RFC, where you have an temporary proc_handler something like proc_hdlr_const (we would need to work on the name) and move each subsystem to the new handler while the others stay with the non-const one. At the end, the old proc_handler function name would disapear and would be completely replaced by the new proc_hdlr_const. This is of course extra work and might not be worth it if you don't get negative feedback related to tree-wide changes. Therefore I stick to my previous suggestion. Send the big tree-wide patches and only explore this option if someone screams. > > > I'm still concerned about the header size for those mails. How does the > > mail look like when you run the get maintainers script on it? > > The full series has 142 recipients in total, > the biggest patch itself has 124. This seems low. might just be enough to make into the lists that have the check. In any case it depends on the size of the header; if it is greater than 8912 you get a message back similar to this one: "linux-raid-owne ( 22K) BOUNCE linux-raid@vger.kernel.org: Header field too long (>8192)" > > Before sending it I'd like to get feedback on the internal rework of the > is_empty detection from you and/or Luis. Yep, this is still on my todo. With holidays just around the corner, I'll be a bit slower to get to it. I'll eventually get to it if someone else does not beat me to it :) Thx for reminding me. > > https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e > or the attached patch. > > > [..] > From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > Date: Sun, 3 Dec 2023 21:56:46 +0100 > Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Simplify the logic by always keeping the permanently_empty flag on the > ctl_dir. > The previous logic kept the flag in the leaf ctl_table and from there > transferred it to the ctl_table from the directory. > > This also removes the need to have a mutable ctl_table and will allow > the constification of those structs. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > fs/proc/proc_sysctl.c | 74 +++++++++++++++++++----------------------- > include/linux/sysctl.h | 13 ++------ > 2 files changed, 36 insertions(+), 51 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 35c97ad54f34..33f41af58e9b 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -17,6 +17,7 @@ > #include <linux/bpf-cgroup.h> > #include <linux/mount.h> > #include <linux/kmemleak.h> > +#include <linux/cleanup.h> > #include "internal.h" > > #define list_for_each_table_entry(entry, header) \ > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > static const struct file_operations proc_sys_dir_file_operations; > static const struct inode_operations proc_sys_dir_operations; > > -/* Support for permanently empty directories */ > -static struct ctl_table sysctl_mount_point[] = { > - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY } > -}; > - > -/** > - * register_sysctl_mount_point() - registers a sysctl mount point > - * @path: path for the mount point > - * > - * Used to create a permanently empty directory to serve as mount point. > - * There are some subtle but important permission checks this allows in the > - * case of unprivileged mounts. > - */ > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > -{ > - return register_sysctl(path, sysctl_mount_point); > -} > -EXPORT_SYMBOL(register_sysctl_mount_point); > - > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT) > - > void proc_sys_poll_notify(struct ctl_table_poll *poll) > { > if (!poll) > @@ -226,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > /* Is this a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > + if (dir->permanently_empty) > return -EROFS; > > - /* Am I creating a permanently empty directory? */ > - if (header->ctl_table_size > 0 && > - sysctl_is_perm_empty_ctl_header(header)) { > - if (!RB_EMPTY_ROOT(&dir->root)) > - return -EINVAL; > - sysctl_set_perm_empty_ctl_header(dir_h); > - } > - > dir_h->nreg++; > header->parent = dir; > err = insert_links(header); > @@ -252,8 +219,6 @@ fail: > erase_header(header); > put_links(header); > fail_links: > - if (header->ctl_table == sysctl_mount_point) > - sysctl_clear_perm_empty_ctl_header(dir_h); > header->parent = NULL; > drop_sysctl_table(dir_h); > return err; > @@ -440,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > struct ctl_table_header *head, struct ctl_table *table) > { > struct ctl_table_root *root = head->root; > + struct ctl_dir *ctl_dir; > struct inode *inode; > struct proc_inode *ei; > > @@ -473,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > inode->i_mode |= S_IFDIR; > inode->i_op = &proc_sys_dir_operations; > inode->i_fop = &proc_sys_dir_file_operations; > - if (sysctl_is_perm_empty_ctl_header(head)) > + > + ctl_dir = container_of(head, struct ctl_dir, header); > + if (ctl_dir->permanently_empty) > make_empty_dir_inode(inode); > } > > @@ -1211,8 +1179,7 @@ static bool get_links(struct ctl_dir *dir, > struct ctl_table_header *tmp_head; > struct ctl_table *entry, *link; > > - if (header->ctl_table_size == 0 || > - sysctl_is_perm_empty_ctl_header(header)) > + if (header->ctl_table_size == 0 || dir->permanently_empty) > return true; > > /* Are there links available for every entry in table? */ > @@ -1533,6 +1500,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > } > EXPORT_SYMBOL(unregister_sysctl_table); > > +/** > + * register_sysctl_mount_point() - registers a sysctl mount point > + * @path: path for the mount point > + * > + * Used to create a permanently empty directory to serve as mount point. > + * There are some subtle but important permission checks this allows in the > + * case of unprivileged mounts. > + */ > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > +{ > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); > + > + if (IS_ERR(dir)) > + return NULL; > + > + guard(spinlock)(&sysctl_lock); > + > + if (!RB_EMPTY_ROOT(&dir->root)) { > + drop_sysctl_table(&dir->header); > + return NULL; > + } > + > + dir->permanently_empty = true; > + return &dir->header; > +} > +EXPORT_SYMBOL(register_sysctl_mount_point); > + > void setup_sysctl_set(struct ctl_table_set *set, > struct ctl_table_root *root, > int (*is_seen)(struct ctl_table_set *)) > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index ada36ef8cecb..57cb0060d7d7 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -137,17 +137,6 @@ struct ctl_table { > void *data; > int maxlen; > umode_t mode; > - /** > - * enum type - Enumeration to differentiate between ctl target types > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > - * empty directory target to serve > - * as mount point. > - */ > - enum { > - SYSCTL_TABLE_TYPE_DEFAULT, > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > - } type; > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > void *extra1; > @@ -194,6 +183,8 @@ struct ctl_dir { > /* Header must be at the start of ctl_dir */ > struct ctl_table_header header; > struct rb_root root; > + /* Permanently empty directory target to serve as mount point. */ > + bool permanently_empty; > }; > > struct ctl_table_set { > -- > 2.38.5 >
On Mon, Dec 11, 2023 at 12:25:10PM +0100, Thomas Weißschuh wrote: > Before sending it I'd like to get feedback on the internal rework of the > is_empty detection from you and/or Luis. > > https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e > or the attached patch. Please send as a new patch as RFC and please ensure on the To field is first "Eric W. Biederman" <ebiederm@xmission.com> with a bit more elaborate commit log as suggested from my review below. If there are any hidden things me and Joel could probably miss I'm sure Eric will be easily able to spot it. > From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> > Date: Sun, 3 Dec 2023 21:56:46 +0100 > Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Simplify the logic by always keeping the permanently_empty flag on the > ctl_dir. > The previous logic kept the flag in the leaf ctl_table and from there > transferred it to the ctl_table from the directory. > > This also removes the need to have a mutable ctl_table and will allow > the constification of those structs. Please elaborate a bit more on this here in your next RFC. > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > fs/proc/proc_sysctl.c | 74 +++++++++++++++++++----------------------- > include/linux/sysctl.h | 13 ++------ > 2 files changed, 36 insertions(+), 51 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 35c97ad54f34..33f41af58e9b 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -17,6 +17,7 @@ > #include <linux/bpf-cgroup.h> > #include <linux/mount.h> > #include <linux/kmemleak.h> > +#include <linux/cleanup.h> > #include "internal.h" > > #define list_for_each_table_entry(entry, header) \ > @@ -29,32 +30,6 @@ static const struct inode_operations proc_sys_inode_operations; > static const struct file_operations proc_sys_dir_file_operations; > static const struct inode_operations proc_sys_dir_operations; > > -/* Support for permanently empty directories */ > -static struct ctl_table sysctl_mount_point[] = { > - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY } > -}; > - > -/** > - * register_sysctl_mount_point() - registers a sysctl mount point > - * @path: path for the mount point > - * > - * Used to create a permanently empty directory to serve as mount point. > - * There are some subtle but important permission checks this allows in the > - * case of unprivileged mounts. > - */ > -struct ctl_table_header *register_sysctl_mount_point(const char *path) > -{ > - return register_sysctl(path, sysctl_mount_point); > -} > -EXPORT_SYMBOL(register_sysctl_mount_point); > - > -#define sysctl_is_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_set_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY) > -#define sysctl_clear_perm_empty_ctl_header(hptr) \ > - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT) > - > void proc_sys_poll_notify(struct ctl_table_poll *poll) > { > if (!poll) > @@ -226,17 +201,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > > > /* Is this a permanently empty directory? */ > - if (sysctl_is_perm_empty_ctl_header(dir_h)) > + if (dir->permanently_empty) > return -EROFS; > > - /* Am I creating a permanently empty directory? */ > - if (header->ctl_table_size > 0 && > - sysctl_is_perm_empty_ctl_header(header)) { > - if (!RB_EMPTY_ROOT(&dir->root)) > - return -EINVAL; > - sysctl_set_perm_empty_ctl_header(dir_h); > - } > - > dir_h->nreg++; > header->parent = dir; > err = insert_links(header); > @@ -252,8 +219,6 @@ fail: > erase_header(header); > put_links(header); > fail_links: > - if (header->ctl_table == sysctl_mount_point) > - sysctl_clear_perm_empty_ctl_header(dir_h); > header->parent = NULL; > drop_sysctl_table(dir_h); > return err; > @@ -440,6 +405,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > struct ctl_table_header *head, struct ctl_table *table) > { > struct ctl_table_root *root = head->root; > + struct ctl_dir *ctl_dir; > struct inode *inode; > struct proc_inode *ei; > > @@ -473,7 +439,9 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, > inode->i_mode |= S_IFDIR; > inode->i_op = &proc_sys_dir_operations; > inode->i_fop = &proc_sys_dir_file_operations; > - if (sysctl_is_perm_empty_ctl_header(head)) > + > + ctl_dir = container_of(head, struct ctl_dir, header); > + if (ctl_dir->permanently_empty) > make_empty_dir_inode(inode); > } > > @@ -1211,8 +1179,7 @@ static bool get_links(struct ctl_dir *dir, > struct ctl_table_header *tmp_head; > struct ctl_table *entry, *link; > > - if (header->ctl_table_size == 0 || > - sysctl_is_perm_empty_ctl_header(header)) > + if (header->ctl_table_size == 0 || dir->permanently_empty) > return true; Why are we now checking the target directory whereas before it was the header? > /* Are there links available for every entry in table? */ > @@ -1533,6 +1500,33 @@ void unregister_sysctl_table(struct ctl_table_header * header) > } > EXPORT_SYMBOL(unregister_sysctl_table); > > +/** > + * register_sysctl_mount_point() - registers a sysctl mount point > + * @path: path for the mount point > + * > + * Used to create a permanently empty directory to serve as mount point. > + * There are some subtle but important permission checks this allows in the > + * case of unprivileged mounts. > + */ > +struct ctl_table_header *register_sysctl_mount_point(const char *path) > +{ > + struct ctl_dir *dir = sysctl_mkdir_p(&sysctl_table_root.default_set.dir, path); Even though __register_sysctl_table() did more than we needed it is not obvious how the sysctl_table_root.default_set.dir.header.nreg++ is not needed but I see get_subdir() also bumps it as it is called from sysctl_mkdir_p(), so we were already bumping it at least once. I suppose that is safe. Wouldn't we be able to re-register the same sysctl mount point more than once with this though? > + > + if (IS_ERR(dir)) > + return NULL; > + > + guard(spinlock)(&sysctl_lock); For those that did not get the memo (I guess the docs could be improved): https://lwn.net/Articles/940117/ > + > + if (!RB_EMPTY_ROOT(&dir->root)) { > + drop_sysctl_table(&dir->header); > + return NULL; > + } This seems to silently allow races in that the above allows an existing sysctl mount point to already exist and here we bail on an assumption we know we can't have. > + > + dir->permanently_empty = true; > + return &dir->header; > +} > +EXPORT_SYMBOL(register_sysctl_mount_point); > + > void setup_sysctl_set(struct ctl_table_set *set, > struct ctl_table_root *root, > int (*is_seen)(struct ctl_table_set *)) > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index ada36ef8cecb..57cb0060d7d7 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -137,17 +137,6 @@ struct ctl_table { > void *data; > int maxlen; > umode_t mode; > - /** > - * enum type - Enumeration to differentiate between ctl target types > - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations > - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently > - * empty directory target to serve > - * as mount point. > - */ > - enum { > - SYSCTL_TABLE_TYPE_DEFAULT, > - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY > - } type; > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > void *extra1; > @@ -194,6 +183,8 @@ struct ctl_dir { > /* Header must be at the start of ctl_dir */ > struct ctl_table_header header; > struct rb_root root; > + /* Permanently empty directory target to serve as mount point. */ > + bool permanently_empty; > }; > > struct ctl_table_set { It's a pretty aggressive cleanup, specially with the new hipster guard() call but I'd love Eric's eyeballs on a proper v2. Luis
On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > My idea was to do something similar to your originl RFC, where you have > an temporary proc_handler something like proc_hdlr_const (we would need > to work on the name) and move each subsystem to the new handler while > the others stay with the non-const one. At the end, the old proc_handler > function name would disapear and would be completely replaced by the new > proc_hdlr_const. > > This is of course extra work and might not be worth it if you don't get > negative feedback related to tree-wide changes. Therefore I stick to my > previous suggestion. Send the big tree-wide patches and only explore > this option if someone screams. I think we can do better, can't we just increase confidence in that we don't *need* muttable ctl_cables with something like smatch or coccinelle so that we can just make them const? Seems like a noble endeavor for us to generalize. Then we just breeze through by first fixing those that *are* using mutable tables by having it just de-register and then re-register new tables if they need to be changed, and then a new series is sent once we fix all those muttable tables. Luis
Luis Chamberlain <mcgrof@kernel.org> writes: > On Mon, Dec 11, 2023 at 12:25:10PM +0100, Thomas Weißschuh wrote: >> Before sending it I'd like to get feedback on the internal rework of the >> is_empty detection from you and/or Luis. >> >> https://git.sr.ht/~t-8ch/linux/commit/ea27507070f3c47be6febebe451bbb88f6ea707e >> or the attached patch. > > Please send as a new patch as RFC and please ensure on the To field is > first "Eric W. Biederman" <ebiederm@xmission.com> with a bit more > elaborate commit log as suggested from my review below. If there are > any hidden things me and Joel could probably miss I'm sure Eric will > be easily able to spot it. > >> From ea27507070f3c47be6febebe451bbb88f6ea707e Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net> >> Date: Sun, 3 Dec 2023 21:56:46 +0100 >> Subject: [PATCH] sysctl: move permanently empty flag to ctl_dir >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> Simplify the logic by always keeping the permanently_empty flag on the >> ctl_dir. >> The previous logic kept the flag in the leaf ctl_table and from there >> transferred it to the ctl_table from the directory. >> >> This also removes the need to have a mutable ctl_table and will allow >> the constification of those structs. > Please elaborate a bit more on this here in your next RFC. > It's a pretty aggressive cleanup, specially with the new hipster guard() > call but I'd love Eric's eyeballs on a proper v2. I will look at a v2 time permitting. My sense is that changing the locking probably make sense as a separate patch. Eric
On 2023-12-12 23:51:30-0800, Luis Chamberlain wrote: > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > > My idea was to do something similar to your originl RFC, where you have > > an temporary proc_handler something like proc_hdlr_const (we would need > > to work on the name) and move each subsystem to the new handler while > > the others stay with the non-const one. At the end, the old proc_handler > > function name would disapear and would be completely replaced by the new > > proc_hdlr_const. > > > > This is of course extra work and might not be worth it if you don't get > > negative feedback related to tree-wide changes. Therefore I stick to my > > previous suggestion. Send the big tree-wide patches and only explore > > this option if someone screams. > > I think we can do better, can't we just increase confidence in that we > don't *need* muttable ctl_cables with something like smatch or > coccinelle so that we can just make them const? The fact that the code compiles should be enough, no? Any funky casting that would trick the compiler to accept it would probably also confuse any other tool. > Seems like a noble endeavor for us to generalize. > > Then we just breeze through by first fixing those that *are* using > mutable tables by having it just de-register and then re-register > new tables if they need to be changed, and then a new series is sent > once we fix all those muttable tables. Ack. But I think the actual constification should really only be started after the first series for the infrastructure is in. Thomas
On Fri, 15 Dec 2023, Thomas Weißschuh wrote: > On 2023-12-12 23:51:30-0800, Luis Chamberlain wrote: > > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > > > My idea was to do something similar to your originl RFC, where you have > > > an temporary proc_handler something like proc_hdlr_const (we would need > > > to work on the name) and move each subsystem to the new handler while > > > the others stay with the non-const one. At the end, the old proc_handler > > > function name would disapear and would be completely replaced by the new > > > proc_hdlr_const. > > > > > > This is of course extra work and might not be worth it if you don't get > > > negative feedback related to tree-wide changes. Therefore I stick to my > > > previous suggestion. Send the big tree-wide patches and only explore > > > this option if someone screams. > > > > I think we can do better, can't we just increase confidence in that we > > don't *need* muttable ctl_cables with something like smatch or > > coccinelle so that we can just make them const? > > The fact that the code compiles should be enough, no? > Any funky casting that would trick the compiler to accept it would > probably also confuse any other tool. I don't know the context, but the fact that a particular file compiles doesn't mean that all of the lines in the file have been subjected to the compiler, due to ifdefs. julia > > > Seems like a noble endeavor for us to generalize. > > > > Then we just breeze through by first fixing those that *are* using > > mutable tables by having it just de-register and then re-register > > new tables if they need to be changed, and then a new series is sent > > once we fix all those muttable tables. > > Ack. But I think the actual constification should really only be started > after the first series for the infrastructure is in. > > Thomas >
Catching up with mail.... On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote: > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > > My idea was to do something similar to your originl RFC, where you have > > an temporary proc_handler something like proc_hdlr_const (we would need > > to work on the name) and move each subsystem to the new handler while > > the others stay with the non-const one. At the end, the old proc_handler > > function name would disapear and would be completely replaced by the new > > proc_hdlr_const. > > > > This is of course extra work and might not be worth it if you don't get > > negative feedback related to tree-wide changes. Therefore I stick to my > > previous suggestion. Send the big tree-wide patches and only explore > > this option if someone screams. > > I think we can do better, can't we just increase confidence in that we > don't *need* muttable ctl_cables with something like smatch or > coccinelle so that we can just make them const? > > Seems like a noble endeavor for us to generalize. > > Then we just breeze through by first fixing those that *are* using > mutable tables by having it just de-register and then re-register So let me see if I understand your {de,re}-register idea: When we have a situation (like in the networking code) where a ctl_table is being used in an unmuttable way, we do your {de,re}-register trick. The trick consists in unregistering an old ctl_table and reregistering with a whole new const changed table. In this way, whatever we register is always const. Once we address all the places where this happens, then we just change the handler to const and we are done. Is that correct? If that is indeed what you are proposing, you might not even need the un-register step as all the mutability that I have seen occurs before the register. So maybe instead of re-registering it, you can so a copy (of the changed ctl_table) to a const pointer and then pass that along to the register function. Can't think of anything else off the top of my head. Would have to actually see the code to evaluate further I think. > new tables if they need to be changed, and then a new series is sent > once we fix all those muttable tables. > > Luis best
On 2023-12-17 13:02:01+0100, Joel Granados wrote: > Catching up with mail.... > > On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote: > > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > > > My idea was to do something similar to your originl RFC, where you have > > > an temporary proc_handler something like proc_hdlr_const (we would need > > > to work on the name) and move each subsystem to the new handler while > > > the others stay with the non-const one. At the end, the old proc_handler > > > function name would disapear and would be completely replaced by the new > > > proc_hdlr_const. > > > > > > This is of course extra work and might not be worth it if you don't get > > > negative feedback related to tree-wide changes. Therefore I stick to my > > > previous suggestion. Send the big tree-wide patches and only explore > > > this option if someone screams. > > > > I think we can do better, can't we just increase confidence in that we > > don't *need* muttable ctl_cables with something like smatch or > > coccinelle so that we can just make them const? > > > > Seems like a noble endeavor for us to generalize. > > > > Then we just breeze through by first fixing those that *are* using > > mutable tables by having it just de-register and then re-register > So let me see if I understand your {de,re}-register idea: > When we have a situation (like in the networking code) where a ctl_table > is being used in an unmuttable way, we do your {de,re}-register trick. unmuttable? > The trick consists in unregistering an old ctl_table and reregistering > with a whole new const changed table. In this way, whatever we register > is always const. > > Once we address all the places where this happens, then we just change > the handler to const and we are done. > > Is that correct? I'm confused. The handlers can already be made const as shown in this series, which does convert the whole kernel tree. There is only one handler (the stackleak one) which modifies the table and this one is fixed as part of the series. (Plus the changes needed to the sysctl core to avoid mutation there) > If that is indeed what you are proposing, you might not even need the > un-register step as all the mutability that I have seen occurs before > the register. So maybe instead of re-registering it, you can so a copy > (of the changed ctl_table) to a const pointer and then pass that along > to the register function. Tables that are modified, but *not* through the handler, would crop during the constification of the table structs. Which should be a second step. But Luis' message was not completely clear to me. I guess I'm missing something. > Can't think of anything else off the top of my head. Would have to > actually see the code to evaluate further I think. > > > new tables if they need to be changed, and then a new series is sent > > once we fix all those muttable tables. Thomas
So we can split this up concentually in two: * constificaiton of the table handlers * constification of the table struct itself On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote: > The handlers can already be made const as shown in this series, The series did already produce issues with some builds, and so Julia's point is confirmed that the series only proves hanlders which you did build and for which 0-day has coverage for. The challenge here was to see if we could draw up a test case that would prove this without build tests, and what occurred to me was coccinelle or smatch. > > If that is indeed what you are proposing, you might not even need the > > un-register step as all the mutability that I have seen occurs before > > the register. So maybe instead of re-registering it, you can so a copy > > (of the changed ctl_table) to a const pointer and then pass that along > > to the register function. > > Tables that are modified, but *not* through the handler, would crop > during the constification of the table structs. > Which should be a second step. Instead of "croping up" at build time again, I wonder if we can do better with coccinelle / smatch. Joel, and yes, what you described is what I was suggesting, that is to avoid having to add a non-const handler a first step, instead we modify those callers which do require to modify the table by first a deregistration and later a registration. In fact to make this even easier a new call would be nice so to aslo be able to git grep when this is done in the kernel. But if what you suggest is true that there are no registrations which later modify the table, we don't need that. It is the uncertainty that we might have that this is a true statment that I wanted to challenge to see if we could do better. Can we avoid this being a stupid regression later by doing code analysis with coccinelle / smatch? The template of the above endeavor seems useful not only to this use case but to any place in the kernel where this previously has been done before, and hence my suggestion that this seems like a sensible thing to think over to see if we could generalize. Luis
Hi Luis and Julia, (Julia, there is a question and context for you inline, marked with your name) On 2023-12-18 13:21:49-0800, Luis Chamberlain wrote: > So we can split this up concentually in two: > > * constificaiton of the table handlers > * constification of the table struct itself > > On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote: > > The handlers can already be made const as shown in this series, > > The series did already produce issues with some builds, and so > Julia's point is confirmed that the series only proves hanlders > which you did build and for which 0-day has coverage for. > > The challenge here was to see if we could draw up a test case > that would prove this without build tests, and what occurred to > me was coccinelle or smatch. I used the following coccinelle script to find more handlers that I missed before: virtual patch virtual context virtual report @@ identifier func; identifier ctl; identifier write; identifier buffer; identifier lenp; identifier ppos; type loff_t; @@ int func( - struct ctl_table *ctl, + const struct ctl_table *ctl, int write, void *buffer, size_t *lenp, loff_t *ppos) { ... } It did not find any additional occurrences while it was able to match the existing changes. After that I manually reviewed all handlers that they are not modifying their table argument, which they don't. Should we do more? For Julia: Maybe you could advise on how to use coccinelle to find where a const function argument or one of its members are modified directly or passed to some other function as non-const arguments. See the coccinelle patch above. Is this possible? > > > If that is indeed what you are proposing, you might not even need the > > > un-register step as all the mutability that I have seen occurs before > > > the register. So maybe instead of re-registering it, you can so a copy > > > (of the changed ctl_table) to a const pointer and then pass that along > > > to the register function. > > > > Tables that are modified, but *not* through the handler, would crop > > during the constification of the table structs. > > Which should be a second step. > > Instead of "croping up" at build time again, I wonder if we can do > better with coccinelle / smatch. As for smatch: Doesn't smatch itself run as part of a normal build [0]? So it would have the same visibility issues as the compiler itself. > Joel, and yes, what you described is what I was suggesting, that is to > avoid having to add a non-const handler a first step, instead we modify > those callers which do require to modify the table by first a > deregistration and later a registration. In fact to make this even > easier a new call would be nice so to aslo be able to git grep when > this is done in the kernel. > > But if what you suggest is true that there are no registrations which > later modify the table, we don't need that. It is the uncertainty that > we might have that this is a true statment that I wanted to challenge > to see if we could do better. Can we avoid this being a stupid > regression later by doing code analysis with coccinelle / smatch? > > The template of the above endeavor seems useful not only to this use > case but to any place in the kernel where this previously has been done > before, and hence my suggestion that this seems like a sensible thing > to think over to see if we could generalize. I'd like to split the series and submit the part up until and including the constification of arguments first and on its own. It keeps the subsystem maintainers out of the discussion of the core sysctl changes. I'll submit the core sysctl changes after I figure out proper responses to all review comments and we can do this in parallel to the tree-wide preparation. What do you think Luis and Joel? [0] https://repo.or.cz/smatch.git/blob/HEAD:/smatch_scripts/test_kernel.sh
On Tue, 19 Dec 2023, Thomas Weißschuh wrote: > Hi Luis and Julia, > > (Julia, there is a question and context for you inline, marked with your name) > > On 2023-12-18 13:21:49-0800, Luis Chamberlain wrote: > > So we can split this up concentually in two: > > > > * constificaiton of the table handlers > > * constification of the table struct itself > > > > On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote: > > > The handlers can already be made const as shown in this series, > > > > The series did already produce issues with some builds, and so > > Julia's point is confirmed that the series only proves hanlders > > which you did build and for which 0-day has coverage for. > > > > The challenge here was to see if we could draw up a test case > > that would prove this without build tests, and what occurred to > > me was coccinelle or smatch. > > I used the following coccinelle script to find more handlers that I > missed before: > > virtual patch > virtual context > virtual report > > @@ > identifier func; > identifier ctl; > identifier write; > identifier buffer; > identifier lenp; > identifier ppos; > type loff_t; > @@ > > int func( > - struct ctl_table *ctl, > + const struct ctl_table *ctl, > int write, void *buffer, size_t *lenp, loff_t *ppos) { ... } > > It did not find any additional occurrences while it was able to match > the existing changes. > > After that I manually reviewed all handlers that they are not modifying > their table argument, which they don't. > > Should we do more? > > > For Julia: > > Maybe you could advise on how to use coccinelle to find where a const > function argument or one of its members are modified directly or passed > to some other function as non-const arguments. > See the coccinelle patch above. > > Is this possible? I will propose something. > > > > > If that is indeed what you are proposing, you might not even need the > > > > un-register step as all the mutability that I have seen occurs before > > > > the register. So maybe instead of re-registering it, you can so a copy > > > > (of the changed ctl_table) to a const pointer and then pass that along > > > > to the register function. > > > > > > Tables that are modified, but *not* through the handler, would crop > > > during the constification of the table structs. > > > Which should be a second step. > > > > Instead of "croping up" at build time again, I wonder if we can do > > better with coccinelle / smatch. > > As for smatch: > > Doesn't smatch itself run as part of a normal build [0]? > So it would have the same visibility issues as the compiler itself. I also believe that this is the case. julia > > Joel, and yes, what you described is what I was suggesting, that is to > > avoid having to add a non-const handler a first step, instead we modify > > those callers which do require to modify the table by first a > > deregistration and later a registration. In fact to make this even > > easier a new call would be nice so to aslo be able to git grep when > > this is done in the kernel. > > > > But if what you suggest is true that there are no registrations which > > later modify the table, we don't need that. It is the uncertainty that > > we might have that this is a true statment that I wanted to challenge > > to see if we could do better. Can we avoid this being a stupid > > regression later by doing code analysis with coccinelle / smatch? > > > > The template of the above endeavor seems useful not only to this use > > case but to any place in the kernel where this previously has been done > > before, and hence my suggestion that this seems like a sensible thing > > to think over to see if we could generalize. > > I'd like to split the series and submit the part up until and including > the constification of arguments first and on its own. > It keeps the subsystem maintainers out of the discussion of the core > sysctl changes. > > I'll submit the core sysctl changes after I figure out proper responses > to all review comments and we can do this in parallel to the tree-wide > preparation. > > What do you think Luis and Joel? > > [0] https://repo.or.cz/smatch.git/blob/HEAD:/smatch_scripts/test_kernel.sh >
On Tue, Dec 19, 2023 at 08:29:50PM +0100, Thomas Weißschuh wrote: > > I used the following coccinelle script to find more handlers that I > missed before: > > virtual patch > virtual context > virtual report > > @@ > identifier func; > identifier ctl; > identifier write; > identifier buffer; > identifier lenp; > identifier ppos; > type loff_t; > @@ > > int func( > - struct ctl_table *ctl, > + const struct ctl_table *ctl, > int write, void *buffer, size_t *lenp, loff_t *ppos) { ... } I think it would be useful to describe that the reason why you have the parameters in places as required is you want to limit the scope to the sysctl handler routines only, and these have these fixed arguments. make coccicheck MODE=patch COCCI=sysctl-const.cocci SPFLAGS="--in-place" > /dev/null git diff --stat | tail -1 80 files changed, 390 insertions(+), 306 deletions(-) git diff --stat | sha1sum ec90851ba02dad069b11822782c74665a01142db - > It did not find any additional occurrences while it was able to match > the existing changes. Fantastic, then please use and include the coccinelle rule to express that this is the goal, it is easier to review *one* cocinelle rule with intent rather than tons of changes. > After that I manually reviewed all handlers that they are not modifying > their table argument, which they don't. > > Should we do more? Now that we started to think about *what* is the goal, and trying to break it down it is easier to think about the ramifications of what we need checked and how tools can help. We break down the first goal into a simple grammatical expression listed above, we want to constify the proc hanlders use of the struct ctl_table. Given this, I think that for the first part the coccinelle grammar above should take care of the cases where the compiler would not have caught a few builds where your config was not testing the compiler build. Now could this still allow ctl->foo = bar ? I think so, so here is a simple sysctl-const-mod.cocci which can be used in coccicheck patch mode as well instead of coccicheck context mode as the context mode just produces a removal visually, and I prefer to see the removals with git diff instead due to color syntax highlighting: make coccicheck MODE=patch COCCI=sysctl-const-mod.cocci SPFLAGS="--in-place" > /dev/null virtual patch @ r1 @ identifier func; identifier ctl; identifier write; identifier buffer; identifier lenp; identifier ppos; type loff_t; @@ int func( struct ctl_table *ctl, int write, void *buffer, size_t *lenp, loff_t *ppos) { ... } @ r2 depends on r1 @ struct ctl_table *ctl; expression E1, E2; @@ ( - ctl->extra1 = E1; | - ctl->extra2 = E1; ) The git diff --stat: arch/arm64/kernel/armv8_deprecated.c | 2 -- drivers/net/vrf.c | 3 --- net/ipv4/devinet.c | 2 -- net/ipv4/route.c | 1 - net/ipv6/addrconf.c | 2 -- net/ipv6/route.c | 1 - net/mpls/af_mpls.c | 2 -- net/netfilter/ipvs/ip_vs_ctl.c | 6 +----- net/netfilter/nf_log.c | 2 +- net/sctp/sysctl.c | 5 ----- 10 files changed, 2 insertions(+), 24 deletions(-) So that needs review. And the OR could be expanded with more components of the struct ctl_table As I noted, I think this is a generically neat endeavor and so I think it would be nice to shorthand *any* member of the struct. ctl->any. Julia, is that possible? The depends on is rather loose so I *think* this means the second rule should only apply the rule on files which define handler. But that second rule could perhaps be made as a long term generic goal without the first rule. I first tried to attach the modification of the ctl table to the routine itself with so only the caller is modified: virtual patch @ r1 @ identifier func; struct ctl_table *ctl; identifier write; identifier buffer; identifier lenp; identifier ppos; type loff_t; @@ int func( struct ctl_table *ctl, int write, void *buffer, size_t *lenp, loff_t *ppos) { ... } @ r2 depends on r1 @ r1.ctl; expression E1, E2; @@ ( - ctl->extra1 = E1; | - ctl->extra2 = E1; ) But that didn't work. > > The template of the above endeavor seems useful not only to this use > > case but to any place in the kernel where this previously has been done > > before, and hence my suggestion that this seems like a sensible thing > > to think over to see if we could generalize. > > I'd like to split the series and submit the part up until and including > the constification of arguments first and on its own. The first part is the proc handlers. If after that you generalize when its used in any routine as this: virtual patch @@ identifier func; identifier ctl; @@ int func(..., - struct ctl_table *ctl, + const struct ctl_table *ctl, ...) { ... } You increase the required changes and scope. This does not handle the case where two different tables are in the same routine arguments, but that is a special case and could be hanlded through its own rule. > It keeps the subsystem maintainers out of the discussion of the core > sysctl changes. We'll need to involve subsystem maintainers eventually to deal with special cases which don't fit the goal we want to normalize to. I suggest you think about the changes in grammatical expressions, leading up to the final gaol, where we could do a full sweep and ensure no struct ctl_table is not const. That is, as you work your way up to the goal, I suspect you may need to modify a few loose drivers / components which may need special handling so that we could normalize on the intended grammatical requirements. Just as with the ctrl work Joel did -- a few drives needed to be modified so that the long term goal for the sentinel could be applied. I don't think time is wasted in formalizing this endeavor as it is a generic goal we tend to see. We're breaking this down then into a few goals, leaps: 1) Start off with a few key special routines which deal with the data structure we want to constify, so we create grammar rules which modify the expected function data types with the one we are intersted in with const. The value in this is that Coccinelle will find changes we need outside of the scope of our build. 2) Those routines then need checks to ensure that no variable is modified, so we need a scaper first to report these so we can inspect the routine and change it so that the grammar rule in 1) works without any expected compile failure. 3) Loosten the search to any routine that uses the struct we want to constify and address cases where the struct is used more than once in a routine. 4) Ensure globally we don't modify the struct as done in the report on goal 1). 5) Constify the struct Luis
> As I noted, I think this is a generically neat endeavor and so I think > it would be nice to shorthand *any* member of the struct. ctl->any. > Julia, is that possible? What do you mean by *any* member? If any is an identifier typed metavariable then that would get any immediate member. But maybe you want something like <+...ctl->any...+> that will match anything that has ctl->any as a subexpression? It may be necessary to put this in parentheses to address parsing issues, but the () won't need to be present in the matched code. You could also use assignment operator aop; rather than =, to also match += etc. julia
I came up with the following:
@@
type t;
const t *x;
identifier y,z;
expression a;
assignment operator aop;
@@
(
(<+...(<+...x->y...+>)[...]...+>) aop a
|
(<+...(<+...x->y...+>)->z...+>) aop a
|
* (<+...x->y...+>) aop a
)
@fn disable optional_qualifier@
identifier f,x;
type t;
parameter list[n] ps;
@@
f(ps,t *x,...) { ... }
@@
identifier fn.f;
expression list[fn.n] es;
type t;
const t *e;
@@
*f(es,e,...)
---------------
The first rule takes care of assignments, while the remaining rules check
function calls.
This is not extensively tested and has false positives. One case is when
you have a->b[x->y] = 12; and it is x not a that is const. Maybe I can
improve it to avoid this problem.
I would suggest to replace the occurrences of t by your specific type of
interest (and then drop the occurrences type t;), to reduce the amount of
work to be done and the chance of false positives.
This is also limited in that it only works on a single file. Thus in
particular the last rule on function calls will only be triggered when the
called function is defined in the same file.
Despite the current limitations, maybe it will find something useful.
julia
On Tue, Dec 19, 2023 at 10:21:25PM +0100, Julia Lawall wrote: > > As I noted, I think this is a generically neat endeavor and so I think > > it would be nice to shorthand *any* member of the struct. ctl->any. > > Julia, is that possible? > > What do you mean by *any* member? I meant when any code tries to assign a new variable to any of the members of the struct ctl_table *foo, so any foo->*any* > If any is an identifier typed > metavariable then that would get any immediate member. But maybe you want > something like > > <+...ctl->any...+> > > that will match anything that has ctl->any as a subexpression? If as just an expression, then no, we really want this to be tied to the data struture in question we want to modify. Luis
On Tue, 19 Dec 2023, Luis Chamberlain wrote: > On Tue, Dec 19, 2023 at 10:21:25PM +0100, Julia Lawall wrote: > > > As I noted, I think this is a generically neat endeavor and so I think > > > it would be nice to shorthand *any* member of the struct. ctl->any. > > > Julia, is that possible? > > > > What do you mean by *any* member? > > I meant when any code tries to assign a new variable to any of the > members of the struct ctl_table *foo, so any foo->*any* Declaring any to be an identifier metavariable would be sufficient. > > > If any is an identifier typed > > metavariable then that would get any immediate member. But maybe you want > > something like > > > > <+...ctl->any...+> > > > > that will match anything that has ctl->any as a subexpression? > > If as just an expression, then no, we really want this to be tied to > the data struture in question we want to modify. What about foo->a.b? Or maybe that doesn't occur in your structure? julia
On Wed, Dec 20, 2023 at 08:39:20AM +0100, Julia Lawall wrote: > > > On Tue, 19 Dec 2023, Luis Chamberlain wrote: > > > On Tue, Dec 19, 2023 at 10:21:25PM +0100, Julia Lawall wrote: > > > > As I noted, I think this is a generically neat endeavor and so I think > > > > it would be nice to shorthand *any* member of the struct. ctl->any. > > > > Julia, is that possible? > > > > > > What do you mean by *any* member? > > > > I meant when any code tries to assign a new variable to any of the > > members of the struct ctl_table *foo, so any foo->*any* > > Declaring any to be an identifier metavariable would be sufficient. Fantastic thanks! > > > If any is an identifier typed > > > metavariable then that would get any immediate member. But maybe you want > > > something like > > > > > > <+...ctl->any...+> > > > > > > that will match anything that has ctl->any as a subexpression? > > > > If as just an expression, then no, we really want this to be tied to > > the data struture in question we want to modify. > > What about foo->a.b? Or maybe that doesn't occur in your structure? We'll consider that too, good idea! Luis
On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote: > On 2023-12-17 13:02:01+0100, Joel Granados wrote: > > Catching up with mail.... > > > > On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote: > > > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote: > > > > My idea was to do something similar to your originl RFC, where you have > > > > an temporary proc_handler something like proc_hdlr_const (we would need > > > > to work on the name) and move each subsystem to the new handler while > > > > the others stay with the non-const one. At the end, the old proc_handler > > > > function name would disapear and would be completely replaced by the new > > > > proc_hdlr_const. > > > > > > > > This is of course extra work and might not be worth it if you don't get > > > > negative feedback related to tree-wide changes. Therefore I stick to my > > > > previous suggestion. Send the big tree-wide patches and only explore > > > > this option if someone screams. > > > > > > I think we can do better, can't we just increase confidence in that we > > > don't *need* muttable ctl_cables with something like smatch or > > > coccinelle so that we can just make them const? > > > > > > Seems like a noble endeavor for us to generalize. > > > > > > Then we just breeze through by first fixing those that *are* using > > > mutable tables by having it just de-register and then re-register > > So let me see if I understand your {de,re}-register idea: > > When we have a situation (like in the networking code) where a ctl_table > > is being used in an unmuttable way, we do your {de,re}-register trick. > > unmuttable? meant muttable here. > > > The trick consists in unregistering an old ctl_table and reregistering > > with a whole new const changed table. In this way, whatever we register > > is always const. > > > > Once we address all the places where this happens, then we just change > > the handler to const and we are done. > > > > Is that correct? > > I'm confused. > > The handlers can already be made const as shown in this series, which > does convert the whole kernel tree. > There is only one handler (the stackleak one) which modifies the table > and this one is fixed as part of the series. > > (Plus the changes needed to the sysctl core to avoid mutation there) > > > If that is indeed what you are proposing, you might not even need the > > un-register step as all the mutability that I have seen occurs before > > the register. So maybe instead of re-registering it, you can so a copy > > (of the changed ctl_table) to a const pointer and then pass that along > > to the register function. > > Tables that are modified, but *not* through the handler, would crop > during the constification of the table structs. > Which should be a second step. > > But Luis' message was not completely clear to me. > I guess I'm missing something. > > > Can't think of anything else off the top of my head. Would have to > > actually see the code to evaluate further I think. > > > > > new tables if they need to be changed, and then a new series is sent > > > once we fix all those muttable tables. > > Thomas
On Mon, Dec 18, 2023 at 01:21:49PM -0800, Luis Chamberlain wrote: > So we can split this up concentually in two: > > * constificaiton of the table handlers > * constification of the table struct itself > > On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote: > > The handlers can already be made const as shown in this series, > > The series did already produce issues with some builds, and so > Julia's point is confirmed that the series only proves hanlders > which you did build and for which 0-day has coverage for. > > The challenge here was to see if we could draw up a test case > that would prove this without build tests, and what occurred to > me was coccinelle or smatch. > > > > If that is indeed what you are proposing, you might not even need the > > > un-register step as all the mutability that I have seen occurs before > > > the register. So maybe instead of re-registering it, you can so a copy > > > (of the changed ctl_table) to a const pointer and then pass that along > > > to the register function. > > > > Tables that are modified, but *not* through the handler, would crop > > during the constification of the table structs. > > Which should be a second step. > > Instead of "croping up" at build time again, I wonder if we can do > better with coccinelle / smatch. > > Joel, and yes, what you described is what I was suggesting, that is to > avoid having to add a non-const handler a first step, instead we modify > those callers which do require to modify the table by first a > deregistration and later a registration. In fact to make this even > easier a new call would be nice so to aslo be able to git grep when > this is done in the kernel. > > But if what you suggest is true that there are no registrations which > later modify the table, we don't need that. It is the uncertainty that > we might have that this is a true statment that I wanted to challenge > to see if we could do better. Can we avoid this being a stupid > regression later by doing code analysis with coccinelle / smatch? That would be amazing! Having an analysis (coccinelle or smatch) that prevents the regression would be the cherry on top. So to further clarify: what you propose is an additional patch in the series that adds this regression prevention. Or do you want a general analysis that prevents changing a variable variable that was defined as const. > > The template of the above endeavor seems useful not only to this use > case but to any place in the kernel where this previously has been done > before, and hence my suggestion that this seems like a sensible thing > to think over to see if we could generalize. A generalized analysis for inadvertent de-constification would be great. It would point to places where we need remove the const qualifier or add it. > > Luis
On Tue, Dec 19, 2023 at 08:29:50PM +0100, Thomas Weißschuh wrote: > Hi Luis and Julia, > > (Julia, there is a question and context for you inline, marked with your name) > > On 2023-12-18 13:21:49-0800, Luis Chamberlain wrote: > > So we can split this up concentually in two: > > > > * constificaiton of the table handlers > > * constification of the table struct itself <--- snip ---> > > case but to any place in the kernel where this previously has been done > > before, and hence my suggestion that this seems like a sensible thing > > to think over to see if we could generalize. > > I'd like to split the series and submit the part up until and including > the constification of arguments first and on its own. > It keeps the subsystem maintainers out of the discussion of the core > sysctl changes. quick comment : Note that if this contains the tree-wide patches, it will inevitably bring in the rest of the maintainers. > > I'll submit the core sysctl changes after I figure out proper responses > to all review comments and we can do this in parallel to the tree-wide > preparation. > > What do you think Luis and Joel? Separating all this into patch series that have a defined motivation and that are self contained is the way to go IMO. Best > > [0] https://protect2.fireeye.com/v1/url?k=0ddcce36-6d3e536b-0ddd4579-000babd9f1ba-c68841e97c452963&q=1&e=d70f9b65-8465-4489-b777-b13eb1ffc99b&u=https%3A%2F%2Frepo.or.cz%2Fsmatch.git%2Fblob%2FHEAD%3A%2Fsmatch_scripts%2Ftest_kernel.sh
Problem description: The kernel contains a lot of struct ctl_table throught the tree. These are very often 'static' definitions. It would be good to make the tables unmodifiable by marking them "const" to avoid accidental or malicious modifications. This is in line with a general effort to move as much data as possible into .rodata. (See for example[0] and [1]) Unfortunately the tables can not be made const right now because the core registration functions expect mutable tables. This is for two main reasons: 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify the table. 2) The table is passed to the handler function as a non-const pointer. This series migrates the core and all handlers. Structure of the series: Patch 1-3: Cleanup patches Patch 4-7: Non-logic preparation patches Patch 8: Preparation patch changing a bit of logic Patch 9-12: Treewide changes to handler function signature Patch 13-14: Adaption of the sysctl core implementation Patch 15: Adaption of the sysctl core interface Patch 16: New entry for checkpatch Patch 17-18: Constification of existing "struct ctl_table"s Tested by booting and with the sysctl selftests on x86. Note: This is intentionally sent only to a small number of people as I'd like to get some more sysctl core-maintainer feedback before sending this to essentially everybody. [0] 43a7206b0963 ("driver core: class: make class_register() take a const *") [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/ --- Changes in v2: - Migrate all handlers. - Remove intermediate "proc_handler_new" step (Thanks Joel). - Drop RFC status. - Prepare other parts of the tree. - Link to v1: https://lore.kernel.org/r/20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net --- Thomas Weißschuh (18): watchdog/core: remove sysctl handlers from public header sysctl: delete unused define SYSCTL_PERM_EMPTY_DIR sysctl: drop sysctl_is_perm_empty_ctl_table cgroup: bpf: constify ctl_table arguments and fields seccomp: constify ctl_table arguments of utility functions hugetlb: constify ctl_table arguments of utility functions utsname: constify ctl_table arguments of utility function stackleak: don't modify ctl_table argument sysctl: treewide: constify ctl_table_root::set_ownership sysctl: treewide: constify ctl_table_root::permissions sysctl: treewide: constify ctl_table_header::ctl_table_arg sysctl: treewide: constify the ctl_table argument of handlers sysctl: move sysctl type to ctl_table_header sysctl: move internal interfaces to const struct ctl_table sysctl: allow registration of const struct ctl_table const_structs.checkpatch: add ctl_table sysctl: make ctl_table sysctl_mount_point const sysctl: constify standard sysctl tables arch/arm64/kernel/armv8_deprecated.c | 2 +- arch/arm64/kernel/fpsimd.c | 2 +- arch/s390/appldata/appldata_base.c | 8 +-- 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 | 4 +- drivers/parport/procfs.c | 12 ++-- fs/coredump.c | 2 +- fs/dcache.c | 4 +- fs/drop_caches.c | 2 +- fs/exec.c | 4 +- fs/file_table.c | 2 +- fs/fs-writeback.c | 2 +- fs/inode.c | 4 +- fs/pipe.c | 2 +- fs/proc/internal.h | 2 +- fs/proc/proc_sysctl.c | 102 +++++++++++++++--------------- fs/quota/dquot.c | 2 +- fs/xfs/xfs_sysctl.c | 6 +- include/linux/bpf-cgroup.h | 2 +- include/linux/filter.h | 2 +- include/linux/ftrace.h | 4 +- include/linux/mm.h | 8 +-- include/linux/nmi.h | 7 -- include/linux/perf_event.h | 6 +- include/linux/security.h | 2 +- include/linux/sysctl.h | 78 +++++++++++------------ include/linux/vmstat.h | 6 +- include/linux/writeback.h | 2 +- include/net/ndisc.h | 2 +- include/net/neighbour.h | 6 +- include/net/netfilter/nf_hooks_lwtunnel.h | 2 +- ipc/ipc_sysctl.c | 12 ++-- ipc/mq_sysctl.c | 2 +- kernel/bpf/cgroup.c | 2 +- kernel/bpf/syscall.c | 4 +- kernel/delayacct.c | 4 +- kernel/events/callchain.c | 2 +- kernel/events/core.c | 4 +- kernel/fork.c | 2 +- kernel/hung_task.c | 4 +- kernel/kexec_core.c | 2 +- kernel/kprobes.c | 2 +- kernel/latencytop.c | 4 +- kernel/pid_namespace.c | 2 +- kernel/pid_sysctl.h | 2 +- kernel/printk/internal.h | 2 +- kernel/printk/printk.c | 2 +- kernel/printk/sysctl.c | 5 +- kernel/sched/core.c | 8 +-- kernel/sched/rt.c | 12 ++-- kernel/sched/topology.c | 2 +- kernel/seccomp.c | 8 +-- kernel/stackleak.c | 9 +-- kernel/sysctl.c | 84 ++++++++++++------------ kernel/time/timer.c | 2 +- kernel/trace/ftrace.c | 2 +- kernel/trace/trace.c | 2 +- kernel/trace/trace_events_user.c | 2 +- kernel/trace/trace_stack.c | 2 +- kernel/ucount.c | 4 +- kernel/umh.c | 2 +- kernel/utsname_sysctl.c | 4 +- kernel/watchdog.c | 15 +++-- mm/compaction.c | 8 +-- mm/hugetlb.c | 10 +-- mm/page-writeback.c | 18 +++--- mm/page_alloc.c | 22 +++---- mm/util.c | 12 ++-- mm/vmstat.c | 4 +- net/ax25/sysctl_net_ax25.c | 2 +- net/bridge/br_netfilter_hooks.c | 6 +- net/core/neighbour.c | 24 +++---- net/core/sysctl_net_core.c | 22 +++---- net/ieee802154/6lowpan/reassembly.c | 2 +- net/ipv4/devinet.c | 8 +-- net/ipv4/ip_fragment.c | 2 +- net/ipv4/route.c | 4 +- net/ipv4/sysctl_net_ipv4.c | 35 +++++----- net/ipv4/xfrm4_policy.c | 2 +- net/ipv6/addrconf.c | 29 +++++---- net/ipv6/ndisc.c | 4 +- net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +- net/ipv6/reassembly.c | 2 +- net/ipv6/route.c | 2 +- net/ipv6/sysctl_net_ipv6.c | 10 +-- net/ipv6/xfrm6_policy.c | 2 +- net/mpls/af_mpls.c | 8 +-- net/mptcp/ctrl.c | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 16 ++--- net/netfilter/nf_conntrack_standalone.c | 4 +- net/netfilter/nf_hooks_lwtunnel.c | 2 +- net/netfilter/nf_log.c | 4 +- net/phonet/sysctl.c | 2 +- net/rds/tcp.c | 4 +- net/sctp/sysctl.c | 30 ++++----- net/smc/smc_sysctl.c | 2 +- net/sunrpc/sysctl.c | 6 +- net/sunrpc/xprtrdma/svc_rdma.c | 2 +- net/sysctl_net.c | 4 +- net/unix/sysctl_net_unix.c | 2 +- net/xfrm/xfrm_sysctl.c | 2 +- scripts/const_structs.checkpatch | 1 + security/apparmor/lsm.c | 2 +- security/min_addr.c | 2 +- security/yama/yama_lsm.c | 2 +- 111 files changed, 427 insertions(+), 428 deletions(-) --- base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a change-id: 20231116-const-sysctl-e14624f1295c Best regards,