Re: [GIT PULL] pidfd updates

[Christian Brauner]

On Thu, Apr 27, 2023 at 06:02:15PM +0100, Al Viro wrote:
> 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@xxxxxxxxxxxxxxxxxx> 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...

Yeah, I'd fully support this and would be very nice to have.