[CRIU] [PATCH 09/17] file: Implement fnext_task

[Eric W. Biederman]

Linus Torvalds <torvalds at linux-foundation.org> writes:

> I like the series, but I have to say that the extension of the
> horrible "fcheck*()" interfaces raises my hackles..
>
> That interface is very very questionable to begin with, and it's easy
> to get wrong.
>
> I don't see you getting it wrong - you do seem to hold the RCU read
> lock in the places I checked, but it worries me.
>
> I think my worry could be at least partially mitigated with more
> comments and docs:
>
> On Mon, Aug 17, 2020 at 3:10 PM Eric W. Biederman <ebiederm at xmission.com> wrote:
>>
>> +struct file *fnext_task(struct task_struct *task, unsigned int *ret_fd)
>
> Please please *please* make it clear that because this does *not*
> increment any reference counts, you have to be very very careful using
> the "struct file" pointer this returns.
>
> I really dislike the naming. The old "fcheck()" naming came from the
> fact that at least one original user just mainly checked if the result
> was NULL or not. And then others had to be careful to either hold the
> file_lock spinlock, or at least the RCU read lock so that the result
> didn't go away.
>
> Here, you have "fnext_task()", and it doesn't even have that "check"
> warning mark, or any comment about how dangerous it can be to use..
>
> So the rule is that the "struct file" pointer this returns can only be
> read under RCU, but the 'fd' it returns has a longer lifetime.
>
> I think your uses are ok, and I think this is an improvement in
> general, I just want a bigger "WARNING! Here be dragons!" sign (and
> naming could be nicer).

Fair.  The naming is my least favorite of these.

I struggle with the fcheck name as I have not seen or at least not
registed on the the user that just checks to see if the result is NULL.
So the name fcheck never made a bit of sense to me.

I will see if I can come up with some good descriptive comments around
these functions.  Along with describing what these things are doing I am
thinking maybe I should put "_rcu" in their names and have a debug check
that verifies "_rcu" is held.

I did the rcu must be held variants because that matches almost exactly
what the existing code is doing, and the smaller the change the fewer
the surprises and bugs.

Eric