Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640793 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare  epoll target files Jann Horn <jannh@google.com> - 2017-05-13 00:50 +0200
    Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare  epoll target files Cyrill Gorcunov <gorcunov@gmail.com> - 2017-05-13 01:00 +0200
      Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare  epoll target files Cyrill Gorcunov <gorcunov@gmail.com> - 2017-05-13 09:00 +0200
        Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare  epoll target files Cyrill Gorcunov <gorcunov@gmail.com> - 2017-05-13 09:20 +0200

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

[resending as plaintext]

On Mon, Apr 24, 2017 at 5:39 PM, Cyrill Gorcunov <gorcunov@gmail.com> 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.

[toc] | [next] | [standalone]

#1640799

On Sat, May 13, 2017 at 12:41:30AM +0200, Jann Horn wrote:
> [resending as plaintext]
> 
> 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.

It work with unlocked data on purpose for speed sake. Moreover even
if we grap a reference it is valid _only_ during comparision operation,
next we drop ref and it can be easily freed by os. Thus it's up to
a caller to keep references to files/task and other resources used.

	Cyrill

[toc] | [prev] | [next] | [standalone]

#1640845

On Fri, May 12, 2017 at 06:45:09PM -0700, Andrei Vagin wrote:
> On Sat, May 13, 2017 at 01:53:40AM +0300, Cyrill Gorcunov wrote:
> > On Sat, May 13, 2017 at 12:41:30AM +0200, Jann Horn wrote:
> > > [resending as plaintext]
> > > 
> > > 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.
> > 
> > It work with unlocked data on purpose for speed sake. Moreover even
> > if we grap a reference it is valid _only_ during comparision operation,
> > next we drop ref and it can be easily freed by os. Thus it's up to
> > a caller to keep references to files/task and other resources used.
> 
> Looks like we can take rcu_read_lock() to guarantee that these objects
> will not be freed, and rcu_read_lock() should not affect perfomance too much.

Rather they should be get_file_rcu/fput. Still I'm not convinced we need it,
but fine will update both: plain KCMP_FILE and KCMP_EPOLL_TFD since it won't
hurt performance.

[toc] | [prev] | [next] | [standalone]

#1640849

On Sat, May 13, 2017 at 09:55:14AM +0300, Cyrill Gorcunov wrote:
> On Fri, May 12, 2017 at 06:45:09PM -0700, Andrei Vagin wrote:
> > On Sat, May 13, 2017 at 01:53:40AM +0300, Cyrill Gorcunov wrote:
> > > On Sat, May 13, 2017 at 12:41:30AM +0200, Jann Horn wrote:
> > > > [resending as plaintext]
> > > > 
> > > > 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.
> > > 
> > > It work with unlocked data on purpose for speed sake. Moreover even
> > > if we grap a reference it is valid _only_ during comparision operation,
> > > next we drop ref and it can be easily freed by os. Thus it's up to
> > > a caller to keep references to files/task and other resources used.
> > 
> > Looks like we can take rcu_read_lock() to guarantee that these objects
> > will not be freed, and rcu_read_lock() should not affect perfomance too much.
> 
> Rather they should be get_file_rcu/fput. Still I'm not convinced we need it,
> but fine will update both: plain KCMP_FILE and KCMP_EPOLL_TFD since it won't
> hurt performance.

From manpage we wrote:

       Note the kcmp() is not protected against false positives which may occur
       if tasks are running.  One should stop tasks by sending SIGSTOP (see  sig‐
       nal(7)) prior to inspection with this system call to obtain meaningful results.

So no, not going to uglify source code and add get/put files there.

	Cyrill

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web