linux-kernel - Re: [PATCH] linux/kernel.h: add container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=whDhHWQo_QjZp36=x=GLMGOJ2xnfsUk9xkUuWRz=i9gOg@mail.gmail.com>
Date:   Thu, 27 Aug 2020 13:46:33 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     James Bottomley <James.Bottomley@...senpartnership.com>,
        Allen Pais <allen.lkml@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH] linux/kernel.h: add container_from()

On Thu, Aug 27, 2020 at 12:28 PM Kees Cook <keescook@...omium.org> wrote:
>
> The common raw pattern for callbacks is:
>
> void callback(struct callback_handle *inner)
> {
>         struct outer *instance;
>         ...
>         instance = container_of(inner, struct outer, member_name_of_inner);
>
> There's so much redundancy here.

What?

It's not all that complicated or even particularly redundant. The main
redundancy comes from you splitting up the declaration from the
initialization - which is certainly fine, and often a good idea, but
it does mean that you mention "struct outer" and "instance" twice.

I don't see that kind of redundancy being a _problem_, though. "So
much redundancy" is just over-stating the issue completely.

In fact, we often encourage people to split declaration from
initialization exactly because it results in simpler expressions and
more legible code, even if that name is now redundant. So it's a small
extra typing of the type. Big deal.

The above is also a common pattern that once you know how
container_of() works, it's very legible.

Sure, if you're new to the kernel, and haven't seen "container_of()"
in other projects, it might initially be a bit of an odd pattern, but
that's the advantage of having one single standardized model: it
becomes a pattern, and you don't have to think about it.

And particularly with that argument-type pattern, you really have to
work at making over-long lines, since the indentation level will by
definition be just one.

Looking around, I do see a lot of people doing line-breaks, but that
tends to be when they insist on putting the variable initialization in
the declaration. And even then, it often seems pointless (eg

        struct idp_led *led = container_of(cdev,
                        struct idp_led, cdev);

was split for no good reason I can see, but it seems to be a pattern
in that file).

You really have to pick some pretty excessive type names (or variable
names) to get close to 80 characters. Again, to pick an example:

        struct timer_group_priv *priv = container_of(handle,
                        struct timer_group_priv, timer[handle->num]);

ends up being long even if you were to split it, but that funky
container_from() wouldn't have helped the real problem - the fact that
the above is complex and nasty.

And I had to _search_ for that example. All the normal cases of
split-line container-of's were due to doing it with the declaration,
or beause the first argument ended up being an expression in itself
and the nested expressions made it more complex.

And in the above example, the real complexity - and the reason the
line ends up long - is because the "member" isn't a member. The above
case works - and it's in fact *intended* to work, I'm not claiming
it's some mis-use of the macro.  But it's really a rather complex
case, where it would probably have been a good idea to add a comment
about how this really depends on handle->num being set correctly.

And in fact, it would probably have been a *perfect* example of where
a helper function really would have improved the code, not so much
from a line length perspective, but exactly because the above is a
much more complicated case than most container_of() cases are.

So a helper function like the kvm one I quoted would have been a good
idea. In ways that "container_from()" would not have been, since it
doesn't actually even address the source of complexity.

               Linus