Received-SPF: pass (google.com: domain of linux-kernel+bounces-5952-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 19 Dec 2023 13:09:31 -0800
From: Luis Chamberlain <mcgrof@kernel.org>
To: Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>
Cc: Julia Lawall <julia.lawall@inria.fr>,
	Joel Granados <j.granados@samsung.com>,
	Dan Carpenter <dan.carpenter@linaro.org>,
	Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
Message-ID: <ZYIGi9Gf7oVI7ksf@bombadil.infradead.org>
References: <20231207104357.kndqvzkhxqkwkkjo@localhost>
 <fa911908-a14d-4746-a58e-caa7e1d4b8d4@t-8ch.de>
 <20231208095926.aavsjrtqbb5rygmb@localhost>
 <8509a36b-ac23-4fcd-b797-f8915662d5e1@t-8ch.de>
 <20231212090930.y4omk62wenxgo5by@localhost>
 <ZXligolK0ekZ+Zuf@bombadil.infradead.org>
 <20231217120201.z4gr3ksjd4ai2nlk@localhost>
 <908dc370-7cf6-4b2b-b7c9-066779bc48eb@t-8ch.de>
 <ZYC37Vco1p4vD8ji@bombadil.infradead.org>
 <a0d96e7b-544f-42d5-b8da-85bc4ca087a9@t-8ch.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a0d96e7b-544f-42d5-b8da-85bc4ca087a9@t-8ch.de>
Sender: Luis Chamberlain <mcgrof@infradead.org>

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