Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables

[Julia Lawall]

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
>