Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare epoll target files

[Jann Horn <jannh-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>]

[resending as plaintext]

On Mon, Apr 24, 2017 at 5:39 PM, Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> With current epoll architecture target files are addressed
> with file_struct and file descriptor number, where the last
> is not unique. Moreover files can be transferred from another
> process via unix socket, added into queue and closed then
> so we won't find this descriptor in the task fdinfo list.
>
> Thus to checkpoint and restore such processes CRIU needs to
> find out where exactly the target file is present to add it into
> epoll queue. For this sake one can use kcmp call where
> some particular target file from the queue is compared with
> arbitrary file passed as an argument.
[...]
> +#ifdef CONFIG_EPOLL
> +static int kcmp_epoll_target(struct task_struct *task1,
> +                            struct task_struct *task2,
> +                            unsigned long idx1,
> +                            struct kcmp_epoll_slot __user *uslot)
> +{
> +       struct file *filp, *filp_epoll, *filp_tgt;
> +       struct kcmp_epoll_slot slot;
> +       struct files_struct *files;
> +
> +       if (copy_from_user(&slot, uslot, sizeof(slot)))
> +               return -EFAULT;
> +
> +       filp = get_file_raw_ptr(task1, idx1);
> +       if (!filp)
> +               return -EBADF;
> +
> +       files = get_files_struct(task2);
> +       if (!files)
> +               return -EBADF;
> +
> +       spin_lock(&files->file_lock);
> +       filp_epoll = fcheck_files(files, slot.efd);
> +       if (filp_epoll)
> +               get_file(filp_epoll);
> +       else
> +               filp_tgt = ERR_PTR(-EBADF);
> +       spin_unlock(&files->file_lock);
> +       put_files_struct(files);
> +
> +       if (filp_epoll) {
> +               filp_tgt = get_epoll_tfile_raw_ptr(filp_epoll, slot.tfd, slot.toff);
> +               fput(filp_epoll);
> +       } else
> +
> +       if (IS_ERR(filp_tgt))
> +               return PTR_ERR(filp_tgt);
> +
> +       return kcmp_ptr(filp, filp_tgt, KCMP_FILE);
> +}

I realize that the existing kcmp code has the same issue, but:

Why are you not taking a reference to filp or filp_tgt? This can end up
performing a comparison between a pointer to a freed struct file and a
pointer to a struct file that was allocated afterwards, right? So it can
return a false "is equal" result when the two files aren't actually the same
if one of the target tasks is running? This looks like it unnecessarily
exposes information about whether an allocation reuses the memory of
a previously freed allocation.