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

[Cyrill Gorcunov]

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