Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <218cac2c-47ae-435d-d7d0-48e4937a7f99@canonical.com>
In-Reply-To: <218cac2c-47ae-435d-d7d0-48e4937a7f99@canonical.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed, 14 Dec 2022 13:54:04 -0800
Message-ID: <CAHk-=wiBjnEtm8cFBp=6YNFe51z6Mdb-BbEJyfka9w1fkWfPvg@mail.gmail.com>
Subject: Re: [GIT PULL] apparmor changes for v6.2
To:     John Johansen <john.johansen@canonical.com>
Cc:     LKLM <linux-kernel@vger.kernel.org>,
        "open list:SECURITY SUBSYSTEM" 
        <linux-security-module@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Stephen Rothwell <sfr@canb.auug.org.au>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 14, 2022 at 10:36 AM John Johansen
<john.johansen@canonical.com> wrote:
>
> John Johansen (45):
>        apparmor: make unpack_array return a trianary value

John, this is unacceptable.

I noticed it due to the conflict, but this really is garbage.

First off, the word is "ternary" (or possibly "tristate").

Secondly, we don't do types like this

    #define tri int

and even if we did, that's a *horrible* name not just for a type, but
for a #define.

Finally, what the heck is "TRI_TRUE/TRI_NONE/TRI_FALSE"? WTF?

It looks like it is used in one single place - the return value for
"unpack_array()" (now renamed "aa_unpack_array()"), and the TRI_FALSE
case is basically an error case for an invalid case.

And TRI_NONE is just a *different* failure case ("name does not exist"
vs "data is invalid").

And then, to make matters worse, ABSOLUTELY NOBODY CARES ABOUT THE
DIFFERENCE. All real users just want to see TRI_TRUE (for "success"),
and anything else is an error anyway.

Yes, yes, there's that one KUNIT test, which wants to actually see
that TRI_FALSE because it's testing that array-out-of-bounds case. It
also - for some unfathomable reason - seems to then want to see some
particular pointer values in that invalid data after the failure,
which seems bogus, but whatever.

In other words, that type is badly done and mis-named to start with,
but then the different ternary values themselves are confusingly
mis-named too in ways that make no sense.

And to cap it all off, NOBODY CARES about those horrid things anyway.

Anyway, I started out doing the mindless conflict resolution, but then
I just couldn't deal with that 'tri' type. There were just too many
things wrong with it for me to accept it, and I felt dirty for just
editing it.

Then I tried out just making it a

     typedef enum { TRI_TRUE/TRI_NONE/TRI_FALSE } ternary_t;

which fixes some of the syntactic issues.

But the whole naming confusion of the values and how NONE-vs-FALSE
wasn't actually a useful distinction anyway made me just axe it
completely.

I'm honestly baffled by why you didn't just make it return the size or
a negative error code, like is the norm. The size is limited to 16
bits anyway, so returning an 'int' with a negative error would have
been very natural.

But just to keep the pattern with some of the other users, and
minimize my surgery, I made it just return 'bool'.

I'm sorry to do all that surgery on it, but I just couldn't stomach
doing anything else.

The resulting merge diff is fairly messy, and to make matters worse I
can't actually *test* any of this. But the code looks more palatable
to me, and I did try to be careful about my surgery.

Apologies if I broke something,

            Linus