Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net>
 <20231204-const-sysctl-v2-13-7a5060b11447@weissschuh.net> <ZW+lQqOSYFfeh8z2@bombadil.infradead.org>
 <4a93cdb4-031c-4f77-8697-ce7fb42afa4a@t-8ch.de>
In-Reply-To: <4a93cdb4-031c-4f77-8697-ce7fb42afa4a@t-8ch.de>
From:   Luis Chamberlain <mcgrof@kernel.org>
Date:   Tue, 5 Dec 2023 14:50:01 -0800
Message-ID: <CAB=NE6UCP05MgHF85TK+t2yvbOoaW_8Yu6QEyaYMdJcGayVjFQ@mail.gmail.com>
Subject: Re: [PATCH v2 13/18] sysctl: move sysctl type to ctl_table_header
To:     =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>
Cc:     Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Joel Granados <j.granados@samsung.com>,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 5, 2023 at 2:41=E2=80=AFPM Thomas Wei=C3=9Fschuh <linux@weisssc=
huh.net> wrote:
>
> On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote:
> > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Wei=C3=9Fschuh wrote:
> > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, str=
uct ctl_table_header *header)
> > >             return -EROFS;
> > >
> > >     /* Am I creating a permanently empty directory? */
> > > -   if (sysctl_is_perm_empty_ctl_header(header)) {
> > > +   if (header->ctl_table =3D=3D sysctl_mount_point ||
> > > +       sysctl_is_perm_empty_ctl_header(header)) {
> > >             if (!RB_EMPTY_ROOT(&dir->root))
> > >                     return -EINVAL;
> > >             sysctl_set_perm_empty_ctl_header(dir_h);
> >
> > While you're at it.
>
> This hunk is completely gone in v3/the code that you merged.

It is worse in that it is not obvious:

+       if (table =3D=3D sysctl_mount_point)
+               sysctl_set_perm_empty_ctl_header(head);

> Which kind of unsafety do you envision here?

Making the code obvious during patch review hy this is needed /
special, and if we special case this, why not remove enum, and make it
specific to only that one table. The catch is that it is not
immediately obvious that we actually call
sysctl_set_perm_empty_ctl_header() in other places, and it begs the
question if this can be cleaned up somehow.

  Luis