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

[Eric W. Biederman]

Christian Brauner <christian.brauner at ubuntu.com> writes:

> On Mon, Aug 17, 2020 at 06:17:35PM -0700, Linus Torvalds wrote:
>> On Mon, Aug 17, 2020 at 6:06 PM Eric W. Biederman <ebiederm at xmission.com> wrote:
>> >
>> > 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.
>> 
>> Yeah, that name is not great. I just don't want to make things even worse.
>> 
>> > 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.
>> 
>> Yeah, something along the lines of "rcu_lookup_fd_task(tsk,fd)" would
>> be a *lot* more descriptive than fcheck_task().
>> 
>> And I think "fnext_task()" could be "rcu_lookup_next_fd_task(tsk,fd)".
>> 
>> Yes, those are much longer names, but it's not like you end up typing
>> them all that often, and I think being descriptive would be worth it.
>> 
>> And "fcheck()" and "fcheck_files()" would be good to rename too along
>> the same lines.
>> 
>> Something like "rcu_lookup_fd()" and "rcu_lookup_fd_files()" respectively?
>> 
>> I'm obviously trying to go for a "rcu_lookup_fd*()" kind of pattern,
>> and I'm not married to _that_ particular pattern but I think it would
>> be better than what we have now.
>
> In fs/inode.c and a few other places we have the *_rcu suffix pattern
> already so maybe:
>
> fcheck() -> fd_file_rcu() or lookup_fd_rcu()
> fcheck_files() -> fd_files_rcu() or lookup_fd_files_rcu()
> fnext_task() -> fd_file_from_task_rcu() or lookup_next_fd_from_task_rcu()
>

So I sat down and played with it and here is what I wound up with is:

__fcheck_files -> files_lookup_fd_raw
fcheck_files   -> files_lookup_fd_locked
fcheck_files   -> files_lookup_fd_rcu
fcheck         -> lookup_fd_rcu
...
fcheck_task    -> task_lookup_fd_fcu
fnext_task     -> task_lookup_next_fd_rcu


That seems to match existing patterns of names, makes it relatively
clear what is going on, and has enough expressivity for for the entire
group of functions.  Plus the core name is short enough to remember
easily.

The tricky bit was realizing I needed to split fcheck_files.  As it
currently can be used in both locked and rcu scenarios.

Doing this I found one bug in my code and one bug in the existing
bpf code so.  So being extra aware of the rcu vs locked nature
of the locking makes a difference at least for me.

The bug in the existing code is that bpf_iter does get_file instead
of get_file_rcu.  Does anyone have any sense of how to add debugging
to get_file to notice when it is being called in the wrong context?

Eric