linux-kernel - Re: [GIT PULL] pidfd updates

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]

Date:   Thu, 27 Apr 2023 18:02:15 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] pidfd updates

On Thu, Apr 27, 2023 at 08:21:34AM -0700, Linus Torvalds wrote:
> On Thu, Apr 27, 2023 at 12:39 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > int delayed_dup(struct file *file, unsigned flags)
> 
> Ok, this is strange. Let me think about it.
> 
> But even without thinking about it, this part I hate:
> 
> >         struct delayed_dup *p = kmalloc(sizeof(struct delayed_dup), GFP_KERNEL);
> 
> Sure, if this is only used in unimportant code where performance
> doesn't matter, doing a kmalloc is fine.
> 
> But if that is the only use, I think this is too subtle an interface.

Still hadn't finished with the zoo...

> Could we instead limit it to "we only have one pending delayed dup",
> and make this all be more like the restart-block thing, and be part of
> struct task_struct?

Interesting...  FWIW, *anything* that wants several descriptors has
special needs - there are some places like that (binder, for one)
and they have rather weird code implementing those.

Just to restate the obvious: this is not applicable for the most frequent
caller - open(2).  For the reasons that have nothing to do with performance.
If opening the file has hard-to-reverse side effects (like directory
modification due to O_CREAT), the things are very different.

What I hope for is a small number of patterns, with clear rules for
choosing the one that is applicable and helpers for each that would
reduce the amount of headache when using it.  And I've no problem
with "this particular pattern is not usable if you are adding more
than one descriptor" - that's not hard to understand and verify.
So I'm fine with doing that for one descriptor only and getting
rid of the allocation.

BTW, another pattern is the same sans the "delayed" part.  I.e.
"here's an opened file, get a descriptor and either attach the
file to it or fput() the damn thing; in any case, file reference
is consumed and descriptor-or-error is returned".  That one is
definitely only for single descriptor case.

In any case, I want to finish the survey of the callers first, just to
see what's there and whether anything is worth nicking.

While we are at it, I want to make close_fd() use a very big red flag.
To the point of grepping for new callers in -next and asking the folks
who introduce those to explain WTF they are doing...